Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Stream dependency replacement with walk all value nodes #3519
Stream dependency replacement with walk all value nodes #3519
Changes from 6 commits
fc8cc7d
879e3fb
29a3468
b0f6f5c
862c3f7
682ddd0
c7a5e09
fad5592
09289d8
b4ba2c0
aead8fd
8553422
455ba48
987b646
dbabec9
6a2c600
92dc4bd
3731d78
68ebd67
97bbf25
a12aabb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are exposing a lot from
Trie
to theStateManager
, also with all these imports above. Can we abstract this away one level more by adding an async methodgetStorageMap(): StorageMap
to theTrie
class and call that instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% following your thoughts here. Is your basic concern that we're calling
walkAllValueNodes
here and have to useTrieNode
and other related types/helpers fromtrie
insidedumpStorageRange
? I'm not seeing where we instantiate a heavy newtrie
object (unless you're talking about further down in the process of walking the trie - so maybelookupNode
?) If so, that feels like something we should handle as a separate PR. Otherwise, yes, seems like we could certainly encapsulate the logic here ingetStorageMap
as you described and add a range filter as a parameter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks, yes, we doesn't need to decide/commit to verkle at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the idea would be having a single
getStorageMap
function that allows you to pass in a range and get astorageMap
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is exactly it. Just copy your code from
dumpStorageRange
and put it in its own function on thetrie
class with optional start and ends for the range..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This file was deleted.