VM: state_db is accessible only via a contextmanager now#76
VM: state_db is accessible only via a contextmanager now#76pipermerriam merged 1 commit intoethereum:masterfrom
Conversation
pipermerriam
left a comment
pipermerriam
left a comment
There was a problem hiding this comment.
One minor request to split something off the Chain object and some other minor comments.
This looks really good and I like where all of this is going. Feel free to merge when that one function has been extracted and tests are green.
|
|
||
| return imported_block | ||
|
|
||
| def ensure_blocks_are_equal(self, block1, block2): |
There was a problem hiding this comment.
This seems like it should be a stand-alone function, maybe renamed to validate_block_equality. Doesn't appear to access self anywhere in the function body and it should easier to test in isolation that way.
There was a problem hiding this comment.
The main reason why I kept it as a Chain method is so that we can easily replace it in tests via the new Chain.configure(), but I guess with pytest's monkeypatch fixture we could achieve the same without having to worry about reverting after the test is done. I'm happy to give that a go if you think it's worth turning it into a standalone function
| state = State(db=self.db, root_hash=self.block.header.state_root) | ||
| yield state | ||
| if read_only: | ||
| # TODO: This is a bit of a hack; ideally we should raise an error whenever the |
There was a problem hiding this comment.
I think I like the idea of implementing this as a wrapper around the self.db which raises errors on any write operations. Might be a good easy pickings ticket to open.
| self.header.uncles_hash = keccak(rlp.encode(self.uncles)) | ||
| return self | ||
|
|
||
| # TODO: Check with Piper what's the use case for having the mine() method allowing callsites |
There was a problem hiding this comment.
I believe this functionality can be removed. This was from an earlier architecture when I hadn't quite wrapped my head around what the block creation workflow was going to be.
There was a problem hiding this comment.
Cool, I'll submit a separate PR removing that
gsalgado
force-pushed
the
statedb-contextmanager
branch
from
f1a6417 to
124f8aa
Compare
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
2 participants
Issue #67
The state db is no longer stored as an instance variable anywehre; instead it is always accessed
via the VM.state_db() contextmanager, which takes care of updating the header's state_root on
exit.
One important thing to note is the fact that now the header's state_root is updated multiple times
during, say, the processing of a transaction (because there are multiple methods called to perform
that, and several of them make use of the state_db contextmanager), whereas before it'd only be
updated once, at the very end of the tx processing. Because of that, if we now have an uncaught
exception while adding a transaction to a block, we may end up with the wrong state_root, but
an uncaught exception there probably means we have a bug that prevents us from processing the
block anyway, so maybe ending up with the wrong state_root in this case is not an actual issue.