Trie: Internal Refactoring #2662
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Got all but one of the Trie tests passing with the new findPath(). the failure is weird. it doesn't do anything too different that the other tests around it, so i can't quite tell why it's failing |
packages/trie/src/trie/trie.ts
Outdated
| } catch (error: any) { | ||
| if (error.message === 'Missing node in DB' && !throwIfMissing) { | ||
| // pass | ||
| } else { | ||
| throw error | ||
| } | ||
| } | ||
| return { node: null, stack, remaining } | ||
| return { node, stack, remaining } |
There was a problem hiding this comment.
At the end I thought that we might be able to skip this node return value since the node returned is just the last item on the stack (if the stack is complete), so this information is just redundant. I generally had the impression that this could be significantly simplified (so: the return format), not sure e.g., is remaining necessary or used? Might be though.
packages/trie/src/trie/trie.ts
Outdated
| @@ -935,8 +932,8 @@ export class Trie { | |||
| * Returns a copy of the underlying trie. | |||
| * @param includeCheckpoints - If true and during a checkpoint, the copy will contain the checkpointing metadata and will use the same scratch as underlying db. | |||
| */ | |||
| copy(includeCheckpoints = true): Trie { | |||
| const trie = new Trie({ | |||
| async copy(includeCheckpoints = true): Promise<Trie> { | |||
There was a problem hiding this comment.
Note: if we keep this we need to mention in the docs (breaking).
Good cath though, we might want to re-visit the other copy() methods as well.
There was a problem hiding this comment.
i'll dig into it a little bit more, to see if it's actually necessary for it to be async.
we encourage users to use the await Trie.create() method, but many of our tests are written with new Trie(). So yes, either a bunch of rewrites to make it al async, or maybe i will find that the create method doesn't actually need to be async.
There was a problem hiding this comment.
No, the create() method needs to be async, that's the whole point why we introduced this method, to allow for async trie instantiation when using e.g. LevelDB as a database which has async reads and writes.
So it is rather our laziness that we still use (our own discouraged) way of using new Trie() directly and we should optimally rewrite the tests (goes for some other libraries as well, I guess at least VM and Blockchain? 🤔)
🎉 Something is off with CI though, would be great if you can fix as some first next step, so that one gets an overview of the tests still failing. |
The issues here run deep, and while I can do my best to limit this PR to non-breaking changes, we may find it impossible to fix one issue without also fixing a few others, which makes breaking changes all the more likely. one helpful non-breaking change i'd love to make right away would be to give the Trie class a |
|
I have a working solution that does not require the breaking change to |
Ah, yes, so that was totally not meant as that breaking changes - if necessary or useful - should be avoided, rather a note to myself to mention this in the release notes. 🙂 In the case we discuss here e.g. it makes super much sense (and we (you?) should rather look into the other libraries with a create method if we want to adopt there as well (that would be actually great, but should be a separate PR).
No, that's not by design or something, it just "is work" to add these debuggers, lol. 😋 Yes, so that would be actually nice. And if you follow the path with this "guard" (if this.DEBUG { debug(...) } or something as we have in SM, VM, devp2p,... it should not impact performance. |
packages/trie/src/trie/trie.ts
Outdated
| async copy(includeCheckpoints = true): Promise<Trie> { | ||
| const trie = await Trie.create({ | ||
| copy(includeCheckpoints = true): Trie { | ||
| const trie = new Trie({ |
There was a problem hiding this comment.
Ah, please, as mentioned, do not revert this. This was a great change! 🤩
There was a problem hiding this comment.
oh -- sure. even though it makes this PR breaking?
There was a problem hiding this comment.
ah, just saw your comment above
There was a problem hiding this comment.
Yes, now it's the time to make these kind of breaking changes if they are useful. 🙂
|
Just a general note: when I was starting with this, I was not yet 100% sure if "this is the way to go", this was just started as some (promising, attention: personal judgement 😋) experiment. If we do it this way, this would throw away a lot of logic, in particular this job queuing thing with this PrioritizedTaskExecutor - or however this is named. I am currently not seeing why such a task executor is needed, from my understanding Trie node retrieval seems to be a totally deterministic process with no surprises (this So anyhow: to confirm that we are not the right track here it would be good if you do substantial mainnet sync trials with the PR on the sideline, just let this running a bit, see how it goes, if the client goes out of memory at some point e.g. with these changes (there is some mention of OOM prevention in "the old code", I am personally not seeing yet where this should happen with max 64 (?) subsequent DB reads). For the performance perspective it would be good if you take some couple of 100/1000 tx loaded blocks and take this version and then the old one and see how sync times compare (optimally post here). |
|
I do remember this prioritized task executor and I tried to remove it at some point (or at least how I remember is by changing it to an optimized version with binary sort). However this had a side effect why it is still this task executor instead of the binary search one. We should maybe find out what the original motivation for this task executor. |
Hmm, yeah, this thing is really "interesting". 🤔 🙂 I injected a console log for findPath
1
2
3
4
5
6
findPath
1
2
3
4
findPath
1
2
3
4
5
findPath
1
findPath
1
2
3
4
5
findPath
1
2
3
4
findPath
1
2
3
4
5
findPathSo totally linear and simple (and low number, will go a bit higher when in state has gronw, this is in block ~500.000 or so, anyhow). So this would keep me on the track that this is not doing so much useful. One theory would be that this was mitigating some flaws of the complicated Promise structrue before (things couldn't execute or something?). But just a theory, does not necessarily need to be true. I guess we just need to observe this a bit, client sync is likely a good testbed here, solid amount of state + real world scenario. Generally of course also: the higher (block number), the better (on the other hand: testbed before was likely not so ambitious? 🤔) |
|
I agree on the above ^ comment @holgerd77. My gut also says that this prioritized queue thing is a leftover of a very old implementation (it would be somewhat likely this indeed has to do with the old callback structure) and that we can safely remove it, but for completeness just wanted to mention my old experiences. I think if we just let the client run for a while and it does not hiccup we are safe. |
|
I am so glad to hear all of this! @jochem-brouwer and @holgerd77 The git history on the Trie library is a little vague on exactly who wrote what parts of it, and I don't want to offend any current team members by suggesting big changes. I totally agree that much of the weird complexity to this library can be stripped away, it just requires some finesse, and some basic fundamental changes. I'm feeling empowered to tear this library apart, and remake it as a more user friendly and "general purpose" set of tools. Should I treat this PR as a vehicle for all of that? Or should I try to make granular incremental changes in separate PR's? |
|
I think I wrote/rewrote most of the trie library, go ahead refactoring it :) If you edit a single method, I think it is great if this is a single PR. However if the refactor is "all over the place" this should be self contained in a single PR also, IMO. |
ScottyPoi
force-pushed
the
trie-refactor-find-path
branch
from
a6b9a07 to
9de7dc0
Compare
There was a problem hiding this comment.
Hi Scotty,
had a first look on this, some first-round comments! 🙂
This is an impressive amount of new code! 😋
Can you please prioritize branch updates on this, also try to get as much tests as possible (also and in particular non-Trie related) working and continue to do so, so that it gets more transparent where the PR is atm?
Please let us know if you need any help on branch updates or the like, often people responsible for the respective PR can assist a bit.
Ugh. Really excited to see what will come out of this!
What a change set! 😜 😆
|
|
||
| const trie = new Trie({ useKeyHashing: true }) | ||
| const verified = await trie.verifyProof(keccak256(proofBuf[0]), address.bytes, proofBuf) | ||
| const veritrie = await Trie.fromProof(keccak256(proofBuf[0]), proofBuf) |
There was a problem hiding this comment.
Ah, that's a nice new API! 🙂 👍
(not sure if final but at least going into a good direction, always looked wrong/out-of-place to me with the explicit Trie instantiation)
| @@ -297,7 +297,7 @@ export class DefaultStateManager implements StateManager { | |||
| } | |||
|
|
|||
| const key = this._prefixCodeHashes ? concatBytes(CODEHASH_PREFIX, codeHash) : codeHash | |||
| await this._trie.database().put(key, value) | |||
| await this._trie.put(key, value) | |||
There was a problem hiding this comment.
This was a "hacky" way of using the trie database to plainly store bytecode for contracts (the cleaner way would be to explicitly define the database for code along StateManager instantiation or something, or at least have some internal variable codeDB which we set to this._trie.database() on instantiation to make this more clear).
Anyhow, removing this will likely not work. 🙂
(maybe you can short term mitigate by re-adding the database() call + a short comment above on the hack, otherwise other devs will fall upon this again (I have also already a couple of times).
| // t.fail(`should have successfully ran block; got error ${err.message}`) | ||
| // } | ||
| // } | ||
| // }) |
There was a problem hiding this comment.
Can you use a /* */ comment here to reduce the diff displayed?
| // } catch (e) { | ||
| // st.pass('successfully threw') | ||
| // } | ||
| // }) |
There was a problem hiding this comment.
Same here, please use /* */ commenting out.
holgerd77
changed the title
ScottyPoi
force-pushed
the
trie-refactor-find-path
branch
from
73bc8f9 to
91911a3
Compare
ScottyPoi
force-pushed
the
trie-refactor-find-path
branch
from
567cfec to
639f7ac
Compare
Trie: Internal Refactoring
Nodes
src/trie/node/
BASE_NODE
abstract class defines basic node structure
raw()rlpEncode()hash()NODE_TYPES
TNode
Operations
src/trie/operations/
Functions in operations directory are used internally by Trie class methods
TNodeTrie
MerklePatriciaTrie
trie/merklePatriciaTrie.ts
The
MerklePatriciaTrieclass implements basic Trie functionality (no DB)Public
root()lookupNodeByHash()setRootByHashresolveProofNodegetPathgetNodeInternal
_getChildOf_getNodePathWalkResultobject { node, path, remainingNibbles }_insertAtNode_deleteAtNode_cleanupNodeTrieWithDB
trie/trieDB.ts
The
TrieWithDBclass extends theMerklePatriciaTrieclass with methods for uisng a database.TrieWithDBenablesAdded Methods:
database()setDatabase()checkpoint()hasCheckpoints()storeNode()persistRoot()DB_ROOT_KEYcommitrevertpruneCheckpointsmaxCheckpointsflushCheckpointsgarbageCollectverifyPrunedIntegrity_markReachableNodesTrieWrap
trie/trieWrapper.ts
TrieWrapextendsTrieWithDBwith additional functionality and provides a simple public interface for most uses.static methods
async createfromProofverifyProofproof methods
createProofupdateFromProofverifyProoftrie methods
putgetdelbatchsetRootNodestream / walk
createReadStreamwalkTriewalkTrieRecursively