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

implement aleth/geth/parity compatibility mode -- 100% pass test #459

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

jangko
Copy link
Contributor

@jangko jangko commented Feb 19, 2020

aleth/geth/parity compatibility mode:

affected test cases both in GST and BCT:

  • stSStoreTest\InitCollision.json
  • stRevertTest\RevertInCreateInInit.json
  • stCreate2\RevertInCreateInInitCreate2.json

pyEVM sided with original Nimbus EVM

implementation difference:
Aleth/geth/parity using accounts cache. When contract creation happened on an existing but 'empty' account with non empty storage will get new empty storage root. Aleth cs. only clear the storage cache while both pyEVM and Nimbus will modify the state trie.
During the next SSTORE call, aleth cs. calculate gas used based on this cached 'original storage value'. In other hand pyEVM and Nimbus will fetch 'original storage value' from persisted state trie.

Both Yellow Paper and EIP2200 are not clear about this situation but since aleth/geth/and parity implement this behavior, we perhaps also need to implement it.

initially, parity also does not support this behavior. see openethereum/parity-ethereum#10065

Definitions of terms are as below(from EIP1283):

  • Storage slot's original value: This is the value of the storage if a reversion happens on the current transaction.
  • Storage slot's current value: This is the value of the storage before SSTORE operation happens.
  • Storage slot's new value: This is the value of the storage after SSTORE operation happens.

aleth/geth/parity revert to empty cache.
pyEVM and Nimbus revert to 'original state trie'.

@zah
Copy link
Contributor

zah commented Feb 20, 2020

I think we can get feedback from the py-evm dev team regarding this issue, but siding with Geth, Parity and Aleth seems like the safer bet. After all, nearly everyone running a node on mainnet is using a client that agrees with the new compatibility mode (and more importantly, the test suite).

@zah zah merged commit 2fbabd2 into master Feb 20, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix_eip2200 branch February 20, 2020 07:08
#
# pyEVM sided with original Nimbus EVM
#
# implementation difference:
Copy link
Member

Choose a reason for hiding this comment

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

+1 for documenting this here 💯

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.

3 participants