Stream dependency replacement with walk all value nodes #3519
Conversation
ScottyPoi
force-pushed
the
stream-dependency-replacement-with-walkAllValueNodes
branch
from
400d040
to
29a3468
Compare
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. |
|
Checked the bundle size with this change and it looks like it reduces the |
acolytec3
force-pushed
the
stream-dependency-replacement-with-walkAllValueNodes
branch
from
d4d18eb
to
fad5592
Compare
| } | ||
| } | ||
| }) | ||
| .then((_) => |
There was a problem hiding this comment.
We are exposing a lot from Trie to the StateManager, also with all these imports above. Can we abstract this away one level more by adding an async method getStorageMap(): StorageMap to the Trie class and call that instead?
There was a problem hiding this comment.
To reiterate here: I would really like to update here!
I am still not sure e.g. if it is (was) a good idea that we instantiate a new object for every trie node walk throuh (and should rather go bare metal and use the plain Uint8Array or something for performance reasons), and if we so deep-use the trie internals it would make it harder to encapsule respectively to refactor.
I also do think that regarding verkle statefull integration it will be beneficial if we stay 1:1 with as many methods as possible and this one would also be a candidate for a 1:1 replacement if we add this higher level method.
(let me know if there are downsides I might not see atm)
There was a problem hiding this comment.
I'm not 100% following your thoughts here. Is your basic concern that we're calling walkAllValueNodes here and have to use TrieNode and other related types/helpers from trie inside dumpStorageRange? I'm not seeing where we instantiate a heavy new trie object (unless you're talking about further down in the process of walking the trie - so maybe lookupNode?) If so, that feels like something we should handle as a separate PR. Otherwise, yes, seems like we could certainly encapsulate the logic here in getStorageMap as you described and add a range filter as a parameter.
There was a problem hiding this comment.
Is your basic concern that we're calling walkAllValueNodes here and have to use TrieNode and other related types/helpers from trie inside dumpStorageRange
Yes, that's my main concern.
We make heavy use of API parts from Trie reaching deep into Trie and which might not have been a good idea to have been exposed in the first place, and if we officially use these ourselves we are stuck with these structures until the next breaking release round.
So I would strongly prefer here to use/introduce a higher level and more abstracted API call into trie (and, again: which then might be the same for Verkle) which does not make so much use of Trie (half-)internals.
There was a problem hiding this comment.
And, to re-iterate on the last part: a higher level getStorageMap() method already lays the ground for more shared state manager code by stateful verkle and MPT, while this specifc Trie code drives us further apart from that. So I think that is the better abstraction.
There was a problem hiding this comment.
Ok, thanks, yes, we doesn't need to decide/commit to verkle at this point
There was a problem hiding this comment.
So the idea would be having a single getStorageMap function that allows you to pass in a range and get a storageMap in return, if no range is passed in default to the full range, which is to say a full dump of storage?
There was a problem hiding this comment.
Yes, I guess so, do not have this fully on the plate, but basically a method in the Trie class which can then cover all the use cases here, and each optimally with a one-line-code call into trie.
You can also comment if this makes sense or if I am overlooking something. If you think this is feasible it would be great if you can update on the PR!
There was a problem hiding this comment.
So the idea would be having a single getStorageMap function that allows you to pass in a range and get a storageMap in return, if no range is passed in default to the full range, which is to say a full dump of storage?
Yes, this is exactly it. Just copy your code from dumpStorageRange and put it in its own function on the trie class with optional start and ends for the range..
There was a problem hiding this comment.
Can someone please actually DO it? 🙂😆 This is not more that an hours work and it would be really good if we get this one merged.
|
Yeah, actually this looks really, really great! 🎉 So this is for the VM. Before:
After:
Note that both the |
…eplacement-with-walkAllValueNodes
…f github.com:ethereumjs/ethereumjs-monorepo into stream-dependency-replacement-with-walkAllValueNodes
…f github.com:ethereumjs/ethereumjs-monorepo into stream-dependency-replacement-with-walkAllValueNodes
acolytec3
deleted the
stream-dependency-replacement-with-walkAllValueNodes
branch
This change picks up where #3507 left off, but with a change of plan to use
walkAllValueNodesto implement storage-dump functionality, and remove the stream dependency currently being used.