Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix(forking): invalidate _deleted in ForkedStorageTrie if a subsequent put happens #612

Merged
merged 2 commits into from
Aug 18, 2020

Conversation

mikeseese
Copy link
Contributor

This PR resolves #571 by wrapping the Trie.put method in ForkedStorageTrie, checking if _deleted.get(rpcKey) === 1, and setting it to 0 if it is. This PR adds a simple test that gets to the root of the issue: _deleted was being set for a storage slot 0 when a bool was changed from true to false, but we never "undelete" it if it's set back to true. This caused an erroneous error when contracts looked at that boolean again and got 0 because ForkedStorageTrie determined it was set to 0 (interpreted as false) here

To add a reason for supporting merging #610, those Infura tests gave me a simple, ready to use way to reproduce the error and understand the root cause. From there, I determined I could create a non-Infura reproduction of the error.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 82.582% when pulling 34504fe on forking/fix-storage-deletion into 93c2488 on develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 82.582% when pulling 34504fe on forking/fix-storage-deletion into 93c2488 on develop.

@davidmurdoch davidmurdoch changed the title fix: invalidate _deleted in ForkedStorageTrie if a subsequent put happens fix(forking): invalidate _deleted in ForkedStorageTrie if a subsequent put happens Aug 18, 2020
@davidmurdoch davidmurdoch merged commit a8a4d8f into develop Aug 18, 2020
@davidmurdoch davidmurdoch deleted the forking/fix-storage-deletion branch August 18, 2020 19:45
@davidlucid
Copy link

@seesemichaelj thank you so much!

@chanhosuh
Copy link

This is great! Any idea when this will get merged into ganache-cli?

@mikeseese
Copy link
Contributor Author

@chanhosuh You can try it out now with v6.10.1-beta.2!

@chanhosuh
Copy link

@seesemichaelj you're awesome! :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forking mainnet results in erroneous revert reason
5 participants