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

Use node hash as db key #3472

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Use node hash as db key #3472

merged 4 commits into from
Jun 24, 2024

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Jun 24, 2024

This makes several small updates to verkle preparing for implementation of trie.push

  • Update VerkleTrie class to use output from verkleCrypto.serializeCommitment as db key for root node instead of ROOT_DB_KEY (serialized commitment is needed for the verkle state root when we start creating block execution proofs)
  • Update findPath test to use verklenode.hash instead of the partial path as the db key (to ensure that we can find nodes from previous state roots and not overwrite them everytime a node at a given partial path changes)
  • Update findPath test to use result from findPath when inserting a new node into the trie. This gives a cleaner example of how trie.put should work
  • Updates verkle-cryptography-wasm to v0.4.5

@gabrocheleau gabrocheleau merged commit a8a8973 into master Jun 24, 2024
33 of 34 checks passed
@acolytec3 acolytec3 added dependencies Pull requests that update a dependency file package: verkle labels Jun 26, 2024
@acolytec3 acolytec3 mentioned this pull request Jun 26, 2024
18 tasks
@holgerd77 holgerd77 deleted the use-node-hash-as-db-key branch July 1, 2024 12:05
@holgerd77
Copy link
Member

Some context question: there is this notion of "path based DB storage/access" for clients which was discussed recently and other clients I guess also practically implemented.

Never dug too much into that, do you guys have some oversight/intuition/whatever how much this is related to this key scheme we change here? Might the old format also have advantages in this regard, or is this unrelated? (and might it be an option/being useful to allow both e.g.?)

Just throwing in the ring, not necessarily needed to start a big topic/discussion, so feel free to answer as short as you want! 🙂

@acolytec3
Copy link
Contributor Author

I discussed it with @kevaundray in the context of rust-verkle a while back as a way to simplify the process of tree walking. The idea is that since the internal nodes are all referenced as partial paths of various stems/tree keys used to walk down to various leaf nodes, it would make tree walking simply a matter of retrieving elements from the DB based on whether that partial path existed in the DB. I don't know if it's optimal in any true sense or not but the reason I switched was I came to the realization that using the partial path as the DB key meant that that we would still be referencing the DB node by the same key even after we update a node (e.g. when we insert a new leaf node below a given internal node) and then we would lose the previous state of that node. That would result in us not being able to track historical state and would never be able to revert state back to a previous state root once the nodes have been written to the DB. Since this is what happens when the state manager flushes the contents of the cache to the DB, this would break things for a stateful StateManager if we for example reorg multiple blocks since we'd no longer have the previous states of the nodes that were getting rolled back. This solves for that issue by simply using the node hash as the DB key. We still use the idea of partial paths when walking the trie in findPath but this way we avoid the issue of losing historical state data for purposes of rollbacks of the stateroot.

@holgerd77
Copy link
Member

Thanks for this extensive explanation 🙏 (sorry, came only now towards reading it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file package: verkle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants