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

Only delete (account) storage values if they are present #1898

Merged
merged 2 commits into from Dec 12, 2019

Conversation

carver
Copy link
Contributor

@carver carver commented Dec 11, 2019

What was wrong?

Fixes: ethereum/trinity#1376

How was it fixed?

Was trying to delete a storage value that was missing. Other changes made this problem more apparent.

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@carver carver changed the title Only delete storage values if they are present Only delete (contract) storage values if they are present Dec 11, 2019
@carver carver changed the title Only delete (contract) storage values if they are present Only delete (account) storage values if they are present Dec 11, 2019
@carver carver requested a review from lithp December 11, 2019 19:34
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

This is quite rubber-stampy as I'm not fully up on the context of this. I assume the deletion here has to do with value == 0 which is effectively like deleting the value in a storage slot.

@carver
Copy link
Contributor Author

carver commented Dec 12, 2019

This is quite rubber-stampy as I'm not fully up on the context of this. I assume the deletion here has to do with value == 0 which is effectively like deleting the value in a storage slot.

Yup! A new BatchDB added in the previous PR means that we sometimes raise a KeyError now when getting or deleting a value. (The HexaryTrie has the weird behavior of never raising a KeyError when asking for a missing key, so we were ignoring the scenario). So we now handle KeyError cases, in part by checking the value before deleting, and in part by handling a KeyError raised when retrieving the previous value.

It looks a bit redundant to hande it in both ways, but that's because we have to deal with the quirk of not wanting to delete the value if the HexaryTrie is already empty there (with a b'' value).

@carver carver merged commit bfdf695 into ethereum:master Dec 12, 2019
@carver carver deleted the only-delete-missing-storage branch December 12, 2019 00:28
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.

"key could not be deleted in JournalDB, because it was missing"
2 participants