Skip to content

perf(l1): give branch-node hashing a monomorphic RLP encoder #7104

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.

Sign up for GitHub

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

Merged
iovoid merged 1 commit into from
Aug 6, 2026
+43 −6
Merged

perf(l1): give branch-node hashing a monomorphic RLP encoder #7104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/common/trie/node/branch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ impl BranchNode {
/// Computes the node's hash, using the provided buffer
pub fn compute_hash_no_alloc(&self, buf: &mut Vec<u8>, crypto: &dyn Crypto) -> NodeHash {
buf.clear();
self.encode(buf);
self.encode_into_vec(buf);
let hash = NodeHash::from_encoded(buf, crypto);
buf.clear();
hash
Expand Down
47 changes: 42 additions & 5 deletions crates/common/trie/rlp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,19 @@ use crate::{Nibbles, NodeHash};
// where `NativeCrypto` is the correct provider.
impl RLPEncode for BranchNode {
fn encode(&self, buf: &mut dyn bytes::BufMut) {

@MegaRedHand MegaRedHand Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try dropping BufMut from here first instead. encode_to_vec is duplicating logic from here

@ilitteri ilitteri Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be done as a follow-up right?

@MegaRedHand MegaRedHand Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We should do it as a follow-up

// Resolve each child's hash once: the length pass and the encode pass
// both needed it, so a 16-choice branch was paying 32 resolutions.
let hashes: [&NodeHash; 16] =
array::from_fn(|i| self.choices[i].compute_hash_ref(&NativeCrypto));

let value_len = <[u8] as RLPEncode>::length(&self.value);
let payload_len = self.choices.iter().fold(value_len, |acc, child| {
acc + RLPEncode::length(child.compute_hash_ref(&NativeCrypto))
});
let payload_len = hashes
.iter()
.fold(value_len, |acc, hash| acc + RLPEncode::length(*hash));

encode_length(payload_len, buf);
for child in self.choices.iter() {
match child.compute_hash_ref(&NativeCrypto) {
for hash in hashes {
match hash {
NodeHash::Hashed(hash) => hash.0.encode(buf),
NodeHash::Inline((_, 0)) => buf.put_u8(RLP_NULL),
NodeHash::Inline((encoded, len)) => buf.put_slice(&encoded[..*len as usize]),
Expand Down Expand Up @@ -69,6 +74,38 @@ impl RLPEncode for BranchNode {
}
}

impl BranchNode {
/// Concrete-typed sibling of `<Self as RLPEncode>::encode`, appending into a
/// `Vec<u8>` rather than a `&mut dyn BufMut`.
///
/// `RLPEncode::encode` must take a trait object, so hashing a branch paid a
/// vtable dispatch for each of its ~41 `put_u8`/`put_slice` calls. Hashing is
/// the hot consumer (every `memoize_hashes` walk re-encodes each dirty
/// branch), so it gets a monomorphic path; `RLPEncode::encode` is untouched
/// for every other caller.
pub fn encode_into_vec(&self, buf: &mut Vec<u8>) {
let hashes: [&NodeHash; 16] =
array::from_fn(|i| self.choices[i].compute_hash_ref(&NativeCrypto));

let value_len = <[u8] as RLPEncode>::length(&self.value);
let payload_len = hashes
.iter()
.fold(value_len, |acc, hash| acc + RLPEncode::length(*hash));

encode_length(payload_len, buf);
for hash in hashes {
match hash {
NodeHash::Hashed(hash) => hash.0.encode(&mut *buf),
NodeHash::Inline((_, 0)) => buf.push(RLP_NULL),
NodeHash::Inline((encoded, len)) => {
buf.extend_from_slice(&encoded[..*len as usize])
}
}
}
<[u8] as RLPEncode>::encode(&self.value, buf);
}
Comment on lines +86 to +106

@greptile-apps greptile-apps Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test concrete encoder equivalence

compute_hash_no_alloc now relies on a second, independently maintained branch encoder for consensus-critical bytes. Add a regression test comparing encode_into_vec with RLPEncode::encode across hashed, empty, and inline children and RLP length boundaries so future drift cannot silently produce inconsistent trie hashes or state roots.

Knowledge Base Used: Common Types, Trie, Crypto, RLP, and Config

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/common/trie/rlp.rs
Line: 86-106

Comment:
**Test concrete encoder equivalence**

`compute_hash_no_alloc` now relies on a second, independently maintained branch encoder for consensus-critical bytes. Add a regression test comparing `encode_into_vec` with `RLPEncode::encode` across hashed, empty, and inline children and RLP length boundaries so future drift cannot silently produce inconsistent trie hashes or state roots.

**Knowledge Base Used:** [Common Types, Trie, Crypto, RLP, and Config](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethrex/-/docs/common-types.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}

impl RLPEncode for ExtensionNode {
fn encode(&self, buf: &mut dyn bytes::BufMut) {
let mut encoder = Encoder::new(buf).encode_bytes(&self.prefix.encode_compact());
Expand Down
Loading