statemanager: add put/delete test case #3787
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…lete-stateRoott-test-case
|
Additional changes I've added to this PR
Note, 2-3 should only ever happen when we are reverting an ephemeral state change (i.e. an account gets touched in the course of block execution but the tx reverts or an account is otherwise deleted prior to being written to state) I'm not 100% confident this is the longterm approach we want since it would preferable to always just do our account updates ephemerally in a cache and only write to the trie once we are ready to "commit" and "flush" values to the db. |
packages/verkle/src/util.ts
Outdated
| equalsBytes(startingNode, tree.root()) && entries.push(['0x', bytesToHex(startingNode)]) | ||
| if (node instanceof LeafVerkleNode) { | ||
| // Leaf node paths/hashes were added in the previous inner node's iteration | ||
| return [] |
There was a problem hiding this comment.
I think we could simplify a bit by just pushing the hashes if( node instanceOf InternalNode), and then returning entries (which would be the empty array in the case of a leafverkleNode, since it's been initialized as that)
There was a problem hiding this comment.
I've pushed that change and made some minor simplifications/added some comments. Let me know how you like it.
| let entries: [PrefixedHexString, PrefixedHexString][] = [] | ||
| // Retrieve starting node from DB | ||
| const rawNode = await tree['_db'].get(startingNode) | ||
| if (rawNode === undefined) return |
There was a problem hiding this comment.
Why do we want to return undefined here instead of [] ?
packages/verkle/src/verkleTree.ts
Outdated
| @@ -217,8 +217,20 @@ export class VerkleTree { | |||
| ['put'], | |||
| ) | |||
| } | |||
| // Push new/updated leafNode to putStack | |||
| putStack.push([leafNode.hash(), leafNode]) | |||
| if (leafNode.values.filter((val) => val !== LeafVerkleNodeValue.Untouched).length === 0) { | |||
There was a problem hiding this comment.
I was wondering, when troubleshooting over the weekend, if there should be another way to mark a node as "to be deleted" that does not require to "put untouched on all leafNode values".
I think your intuition of a lifecycle where we "commit" and "flush" sounds right. I don't think it's necessarily what we should do now (maybe worth discussing that tomorrow on the call?) as this PR definitely brings enough improvements to be merged (and can get us to iterate on verkle tests quicker), but I also feel like we need, in the end, a better architecture.
This PR adds a test case that does the following:
This is currently failing and I suspect that this causes some failures that we are seeing in the test fixtures, most likely related to the handling of Untouched values and account deletion