-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(levm): fix interaction with cache and db #1531
Conversation
…thereum_rust into levm/fixes_cache_db
This reverts commit 58d42b0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
account | ||
} | ||
} | ||
get_account(&mut self.cache, &self.db, address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could have a different name for the two functions, since having the same one could lead to confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm maybe, the arguments are different though. They have the same name because they do the same thing but the VM
one is like a wrapper. A better name doesn't come to my mind, what do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that there isn't a good name for both, maybe it could be get_account_cache_or_db, but it says more about implementation than anything else. Actual version looks good to me in that sense 👍
Motivation
Description
TLDR: We don't want to interact directly with the db without consulting before with the cache. This won't fix nor break tests because in every EFTest we send the cache empty, but it is a necessary fix for executing multiple transactions within a block. As a side effect we have to move contract creation and insertion to cache to
transact()
, so that we can revert if address is already occupied.Idea:
transact()
, not tonew()
.db.get_account_info()
in theVM::new()
, we want to query the cache first and then the DB. For this we have theget_account()
method in VM, but inVM::new()
it doesn't work because there is no vm created yet. The idea is to grab that behavior and take it to aget_account(address, mut cache, db)
function that has the same behavior as now. The VM get_account method can be a handrail to call this, which doesget_account(address, self.cache, self.db)
.Additional Changes:
VM::new()
Closes #issue_number