Skip to content
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

Upgrade to Py-EVM 0.2.0a10 compatible #53

Merged
merged 3 commits into from
Jan 24, 2018

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jan 24, 2018

What was wrong?

The interface of Py-EVM is changed, so it's not compatible with the latest Py-EVM.

How was it fixed?

  1. Update eth_tester.backends.pyevm.main._execute_and_revert_transaction.
  2. Update Py-EVM version.
  3. In Py-EVM, BaseChainDB.set_as_canonical_chain_head was renamed to BaseChainDB._set_as_canonical_chain_head, so I also have to change where eth-tester calls this function, but it may look not good to call a protected function of py-evm. Do we need to open an issue for that?

Cute Animal Picture

norway-1758183_1280

@hwwhww
Copy link
Contributor Author

hwwhww commented Jan 24, 2018

@pipermerriam it needs review, thanks!

@@ -249,11 +249,11 @@ def take_snapshot(self):
def revert_to_snapshot(self, snapshot):
block = self.chain.get_block_by_hash(snapshot)
if block.number > 0:
self.chain.chaindb.set_as_canonical_chain_head(block.header)
self.chain.chaindb._set_as_canonical_chain_head(block.header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ewww, do we no longer have a public API we can access for this? We should fix that (not now but soon) by moving it back to being a public API.

cc @carver to ask what the reason was for making this private?

Copy link
Contributor

@carver carver Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some ways, it was to highlight issues like this. Headers entered this way would not have a score lookup, or a hash->header lookup. You should only call this if you really know what you're doing. Giving callers a way to reach into the EVM and violate its own fork choice rule seems like asking for trouble.

The public call should generally be to persist_header_to_db(). Or, in this case, it seems like we should actually be using py-evm's own snapshot() and revert().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, off the top of my head, the py-evm snapshot() and revert() methods only work within block boundaries. Need to go recheck this. If that's the case we need to come up with an alternate solution.

@pipermerriam
Copy link
Member

Good to go when CI passes. I'll get you a release when merged. Feel free to poke me if it seems like I've forgotten

@pipermerriam pipermerriam merged commit e985aef into ethereum:master Jan 24, 2018
@pipermerriam
Copy link
Member

@hwwhww released as v0.1.0b14

pacrob pushed a commit to pacrob/eth-tester that referenced this pull request Oct 25, 2023
Export type annotations to importing projects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants