Add statefulVerkleStateManager
#3628
Conversation
|
Note, this still has extraneous commits that already exist on master in it (from the kaustinen7 readiness PR). Will purge those out and clean up the commit trie at some point here soon. |
acolytec3
force-pushed
the
statefulVerkleStateManager
branch
from
94edd6c to
adad631
Compare
| this._caches?.account?.del(address) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't want to get yourself too much on the nerve with this refactoring/DRY stuff (but maybe a bit 😂), but maybe you can put such code parts like the putAccount() directly in a src/helpers/verkle.ts file or similar with an adjusted function signature putAccount(self/this: SM, address: Address, account?: Account): Promise<void> taking the code from the stateless verkle SM directly over so that these kind of code parts just never reach this file?
Then we directly have a unified version and do not need to post-DRY this and this should not be significantly ore work (just run the statless tests from statelessVerkleStateManager.spec.ts along)!
There was a problem hiding this comment.
I guess I'm not 100% sure what you mean since the implementations are not the same. The stateless SM puts things in an in memory map via direct this._state[verkleKey] = serializedBytes and the stateful implementation uses await this.trie.put(stem, suffixes, values) to "put" data. The thing I could do differently is to have encodeVerkleLeafBasicData accept an account rather than the various fields and that would de-dup a few lines of code.
There was a problem hiding this comment.
Ok, yeah, nevermind, you are right, haven't looked/though closely enough. I think I am generally still latently unsatisfied that we always copy over 2/3 of code which is redundant/the same.
Maybe we can also tackle this in a more fine-grained way. We could e.g. centralize the generic DEBUG strings and re-use, things like that (for sure not in this PR).
So don't let me distract you here! 🙂 I will nevertheless not give up on the topic, maybe open some more low-reaching small issues on this after this PR got merged.
| await this._trie.put( | ||
| chunkStems[stem], | ||
| chunkSuffixes.slice( | ||
| 128 + (256 * stem - 1), | ||
| codeChunks.length <= 128 + 256 * stem ? codeChunks.length : 128 + 256 * stem, | ||
| ), | ||
| codeChunks.slice( | ||
| 128 + (256 * stem - 1), | ||
| codeChunks.length <= 128 + 256 * stem ? codeChunks.length : 128 + 256 * stem, | ||
| ), |
There was a problem hiding this comment.
To slightly improve clarity, I would define chunkId = stem - 1 and then use that for the 2 instances where we use stem - 1
There was a problem hiding this comment.
Where are you seeing this? In the code below here, it's not stem - 1 but (VERKLE_NODE_WIDTH * stem) - 1 since these arrays are zero-indexed. I'm not referencing a specific chunkId but a section of the overall array of chunks corresponding to the stem we're putting them under.
| await this._trie.put( | ||
| chunkStems[stem], | ||
| chunkSuffixes.slice( | ||
| 128 + (256 * stem - 1), |
There was a problem hiding this comment.
I think we should use the constants for these values for 2 reasons:
- If the spec change, we can easily just update the constant.
- It makes the meaning of these numbers obvious (e.g. CODE_OFFSET, VERKLE_NODE_WIDTH, etc.)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
🙂 🎉 |
Implements a stateful version of the
statemanagerinterface that uses a verkle trie as a backend.