Make triedb compatible with Reth v1.9.3 #180
Conversation
✅ Heimdall Review Status
|
and-cb
force-pushed
the
andrea/reth-v1.9.3
branch
from
a869364 to
8a5b330
Compare
| pub fn slice(&self, range: impl RangeBounds<usize>) -> Self { | ||
| let start = range.start_bound().map(|b| *b); | ||
| let end = range.end_bound().map(|b| *b); | ||
| Self::from_nibbles(&self.nibbles()[(start, end)]) | ||
| } |
There was a problem hiding this comment.
Not urgent, but we may also want a copy-less slice method, which returns a view of the underlying nibbles instead of an owned slice.
| if let Some(hash) = pointer.rlp().as_hash() { | ||
| // No overlay, just add the pointer by hash | ||
| root_builder.add_branch(path, hash, true); | ||
| root_builder.add_branch(path.try_into().unwrap(), hash, true); |
There was a problem hiding this comment.
Should this properly handle the error or do we not expect the unwrap to be hit?
There was a problem hiding this comment.
Good question. Answering it requires some context: the current code implicitly assumes that all Nibbles that are passed around to third-party functions (e.g. to reth) have the correct size (32 bytes / 64 nibbles). Failure to do so results in panics in third-party code, and we have seen multiple examples of that in the past.
The code that I wrote still relies in the implicit assumption that path has the right size. The only difference between this new code and the old one is where we get the panic: now we get it right away (from .try_into().unwrap()), while previously we would get it later on in third-party code.
If path does not have the right size, then it's a bug. As with most bugs, we prefer to panic, rather than bubbling up an error to the user.
This is by the way the exact reason why I wrote this in the PR description:
A better approach would be to avoid passing
Nibbles/RawPatharound, and rather create aPathenum that can be eitherAccountPathorStoragePath. That way it will be much easier to reason about the size of paths and about the safety of converting such paths to Nibbles.
When I say that "it will be much easier to reason about the size of paths" I refer exactly to code like this one: right now there is a possibility that this code might panic because path was not constructed properly, but if we refactor the code to use an enum, then we can simply request the necessary bits instead of truncating path or making implicit assumptions about its length.
refcell
left a comment
refcell
left a comment
There was a problem hiding this comment.
A few comments on use of unwrap but otherwise changes look good to me
and-cb
force-pushed
the
andrea/reth-v1.9.3
branch
from
8a5b330 to
5ffe0e3
Compare
and-cb
force-pushed
the
andrea/reth-v1.9.3
branch
from
5ffe0e3 to
71caf87
Compare
cb-heimdall
dismissed
BrianBland’s stale review
Approved review 3536331830 from BrianBland is now dismissed due to new commit. Re-request for approval.
|
Review Error for BrianBland @ 2025-12-04 00:44:26 UTC |
The biggest change in this PR is that
alloy-triehas been upgraded to the latest major version. This upgrade changed theNibblesstructure so that it can store at most 32 bytes only. Some of the triedb code was usingNibblesto store more than that (which by itself was a source of bugs even with the previous version ofalloy-trie). This PR addresses the problem by introducing a newRawPathstruct that is essentially an unbounded version ofNibbles.A better approach would be to avoid passing
Nibbles/RawPatharound, and rather create aPathenum that can be eitherAccountPathorStoragePath. That way it will be much easier to reason about the size of paths and about the safety of converting such paths toNibbles. However that is a much larger refactor; for this PR I prefer to focus on getting things to work with minimum effort.