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

Fixes and refactors for use in ipld-eth-server #18

Merged
merged 8 commits into from
Apr 25, 2023
Merged

Fixes and refactors for use in ipld-eth-server #18

merged 8 commits into from
Apr 25, 2023

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Apr 24, 2023

This fixes a few things detected while integrating into ipld-eth-server for v5:

  • fix direct_by_leaf.StateDB.GetState to take a storage slot (preimage of leaf key) as argument
  • fix for hash caching bug on trie node

Also, refactors into sub-packages so we now have

  • direct_by_leaf - original StateDB package
  • trie_by_cid
  • sql for SQL DB util

And, DRYs up the tests and adds some basic trie tests.

roysc added 8 commits April 24, 2023 18:34
GetState takes a slot, not a leaf path
used in ipld-eth-server
* direct_by_leaf/ is original StateDB package
* sql/ for SQL DB interfaces
was erroneously storing the cid in the fullNode's flag.cache
Copy link
Contributor

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

LGTM! I like the reorganization 👍

Don't see a change for

  • fix direct_by_leaf.StateDB.GetState to take a storage slot (preimage of leaf key) as argument

And not sure that would make sense because in the db the records are indexed by leaf key and the processes above that will call down into the StateDB will also be hash-aware not preimage-aware.

@roysc
Copy link
Contributor Author

roysc commented Apr 25, 2023

@i-norden that's this one: e3e4e1e

The slot key is passed in when executing a SLOAD which reads directly from StateDB; it passes the key to StateTrie which hashes it internally to get the path. This basically can only be covered by testing against an EVM instance.

@roysc roysc merged commit 0eba688 into v5 Apr 25, 2023
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.

2 participants