perf(l1): give branch-node hashing a monomorphic RLP encoder - #7104
Conversation
`RLPEncode::encode` takes `&mut dyn bytes::BufMut`, so encoding a branch node pays a vtable dispatch for each of its ~41 `put_u8`/`put_slice` calls. That is on the hashing path: `compute_hash_no_alloc` encodes into a `Vec<u8>` and every `memoize_hashes` walk re-encodes each dirty branch. Add `BranchNode::encode_into_vec`, a concrete-typed sibling that appends into a `Vec<u8>` (mirroring the existing `encode_to_vec`, which is already monomorphic but allocates a fresh buffer), and point `compute_hash_no_alloc` at it. `RLPEncode::encode` is unchanged for every other caller. Also resolve each child hash once instead of twice: the payload-length pass and the encode pass each called `compute_hash_ref`, so a 16-choice branch did 32 resolutions. Measured in the LambdaVM zkVM guest on mainnet block 25368371, where every executed RISC-V instruction is proven and an indirect call cannot be hidden by an out-of-order core: BranchNode::encode + put_slice + put_u8 2,122,427 -> 1,312,210 (-38%) memcpy 908,885 -> 586,969 total block execution 20,544,222 -> 19,408,617 (-5.53%) `memcpy` falls too because `extend_from_slice` on a `Vec` with known capacity beats the trait object's `put_slice`. Native gains will be much smaller, but the change strictly removes indirection.
|
github-actions
Bot
added
the
performance
🤖 Kimi Code ReviewOverall Assessment: This is a solid performance optimization that eliminates redundant hash computations and vtable dispatches in the trie's hot path. The logic is correct, but there are maintainability risks due to code duplication. Specific Feedback: 1. Consensus-Critical Code Duplication Risk (
2. Import Visibility (
3. Bounds Checking (
4. Method Naming Consistency (
5. Optimization Validation
Positive Aspects:
Security/Consensus: No vulnerabilities introduced. The encoding logic preserves exact byte-for-byte compatibility with the previous implementation. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. The change in crates/common/trie/rlp.rs and crates/common/trie/node/branch.rs looks like a safe perf-only optimization: branch encoding still emits the same RLP shape, but it now resolves each child hash once and avoids I could not run the trie test suite in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThe PR adds a concrete Vec-based RLP encoder for branch nodes and routes branch hashing through it to remove trait-object dispatch. It also resolves each child hash once and reuses those references for payload sizing and encoding.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking recommendation to lock the duplicated consensus-critical encoders together using an equivalence test. The new encoder currently matches the canonical branch RLP output for hashed, empty, and inline children, but the absence of a direct equivalence test leaves future divergence undetected. Files Needing Attention: crates/common/trie/rlp.rs
|
| Filename | Overview |
|---|---|
| crates/common/trie/node/branch.rs | Routes allocation-reusing branch hashing through the new concrete Vec encoder; the hashing and buffer lifecycle remain unchanged. |
| crates/common/trie/rlp.rs | Adds a byte-equivalent monomorphic branch encoder and reuses child hashes, but lacks a direct regression test enforcing equivalence with the canonical encoder. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
B[BranchNode] --> E[encode_into_vec]
E --> H[Resolve 16 child hashes]
H --> L[Compute RLP payload length]
L --> V[Append encoded branch to Vec]
V --> K[NodeHash::from_encoded]
Prompt To Fix All With AI
### Issue 1
crates/common/trie/rlp.rs:86-106
**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.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "perf(trie): give branch-node hashing a m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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); | ||
| } |
There was a problem hiding this comment.
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!
🤖 Claude Code ReviewReview: perf(trie): give branch-node hashing a monomorphic RLP encoder (PR 7104)Summary: The PR adds Findings
Things that look good
Automated review by Claude (Anthropic) · sonnet · custom prompt |
| @@ -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) { | |||
There was a problem hiding this comment.
I think we should try dropping BufMut from here first instead. encode_to_vec is duplicating logic from here
There was a problem hiding this comment.
This could be done as a follow-up right?
There was a problem hiding this comment.
Yes. We should do it as a follow-up
edg-l
changed the title
4 participants
Motivation
RLPEncode::encodetakes&mut dyn bytes::BufMut. Encoding a branch node writes ~41 items through that trait object (16 children — each an RLP length byte plus a 32-byte hash — plus the header and value), so each one is a vtable dispatch.That lands on the hashing path:
BranchNode::compute_hash_no_allocencodes into aVec<u8>before keccak, and everymemoize_hasheswalk re-encodes each dirty branch. Noteencode_to_vecdirectly below it is already monomorphic (buf.push/extend_from_slice) — it just allocates a fresh buffer each call, so the hashing path can't use it.Description
Add
BranchNode::encode_into_vec(&self, buf: &mut Vec<u8>), a concrete-typed sibling that appends into a caller-ownedVec<u8>, and pointcompute_hash_no_allocat it.RLPEncode::encodeis untouched for every other caller.Also resolve each child's hash once rather than twice — the payload-length pass and the encode pass each called
compute_hash_ref, so a 16-choice branch performed 32 resolutions.cargo test -p ethrex-trie(61 tests),fmtandclippyare clean.Effect
The win scales with how poorly indirect calls are amortised. These numbers come from the LambdaVM zkVM guest executing mainnet block 25368371, where every RISC-V instruction is proven and there is no out-of-order core to hide a vtable dispatch:
BranchNode::encode+put_slice+put_u8memcpymemcpydrops as well becauseextend_from_sliceinto aVecwith known capacity beats the trait object'sput_slice.Native gains will be considerably smaller — a modern core predicts and pipelines these calls well — but the change strictly removes indirection, so it should not regress. Happy to add a native benchmark before merging if you'd like a number for that path.
Companion to #7101 (same guest, same profiling run).