chore(l2): optimize trie hashing and BranchNode encoding for zkVMs
#4723
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for d9f57daClick to view benchmark
|
Benchmark for 74c4f47Click to view benchmark
|
Benchmark for 77c8cd7Click to view benchmark
|
Benchmark for ec5b844Click to view benchmark
|
Benchmark for c6d25c5Click to view benchmark
|
Benchmark for 77fe7f9Click to view benchmark
|
ilitteri
approved these changes
Nov 5, 2025
Benchmark for c85ba9dClick to view benchmark
|
Oppen
reviewed
Nov 10, 2025
| // Encoded as Vec<u8> | ||
| impl RLPEncode for NodeHash { | ||
| fn encode(&self, buf: &mut dyn bytes::BufMut) { | ||
| RLPEncode::encode(&Into::<Vec<u8>>::into(self), buf) |
Contributor
There was a problem hiding this comment.
Why do we build a vec to encode here?
Contributor
Author
There was a problem hiding this comment.
Hmm not sure, maybe to take advantage of NodeHash::as_ref()? might try changing it now that you mentioned it
Contributor
Author
There was a problem hiding this comment.
I think I'll do it in a different PR
Oppen
reviewed
Nov 10, 2025
Oppen
reviewed
Nov 10, 2025
| } | ||
|
|
||
| // Duplicated to prealloc the buffer and avoid calculating the payload length twice | ||
| fn encode_to_vec(&self) -> Vec<u8> { |
Contributor
There was a problem hiding this comment.
Maybe we should make encode_to_vec in the generic implementation call the length method instead, wdyt?
Contributor
Author
There was a problem hiding this comment.
The thing with that is that the generic length encodes into a buffer (a zero capacity Vec) and then returns the length of that buffer. Ideally RLPEncode should have a way to hint what the encoded size would be to prealloc the buffer (this is done manually in BranchNode::encode_to_vec), and default to a non-prealloced buffer.
If we call the current length we end up encoding twice
Oppen
approved these changes
Nov 10, 2025
Co-authored-by: Mario Rugiero <[email protected]>
Benchmark for e1fc597Click to view benchmark
|
Benchmark for a4026d4Click to view benchmark
|
xqft
added a commit
that referenced
this pull request
Nov 11, 2025
…4723) **Motivation** Our trie does a preorder traversal for recursive hashing, which allocates a lot of buffers (from the encoder and the keccak hasher) all the way down until it starts actually hashing a leaf. A way better approach to implement recursive hashing is to do a postorder traversal, in which we start allocating memory when we get to the lowest node that needs to be hashed, after which the memory is dealloc'd and the next (parent or sibling) node allocs again. This approach was inspired by risc0's trie, although we had a preorder traversal hashing for the `commit()` function already. This PR also optimizes a `BranchNode` encoding by skipping our `Encoder` type and writing directly to a preallocated buffer, instead of using two buffers and copying data from one to the other (this is because the current `Encoder` can't know the length of the encoded data beforehand, but we can calculate it if we know what we are encoding). This could also be implemented for other node types, but they are the minority of node types and the perf. gains are negligible. A third, smaller optimization is to prevent cloning the cached/computed hashes from every node. **Description** - adds `memoize_hashes` to both `Node` and `NodeRef` to implement postorder traversal - adds utility functions to calculate encoded lengths of a `NodeHash` and a string of bytes - changes `BranchNode::encode_raw()` to encode into a single buffer - adds `compute_hash_ref` to return a reference of the cached hashed **Testing** This branch was used in a client snapsynced to Mainnet, running successfully from nov. 4 to nov. 5. **Flamegraphs** block 23385900 Mainnet this reduces cycles of `hash_no_commit` (trie hashing) by 40%, which is 5% of total cycles before: <img width="3024" height="646" alt="image" src="https://github.com/user-attachments/assets/a3f8bb05-eb80-4cf5-9347-984a7a7b4501" /> after: <img width="3024" height="618" alt="image" src="https://github.com/user-attachments/assets/1551d8a0-0829-4798-b244-5bca14b56f76" /> **Proving times** RTX 4090 ``` 10m 07s -> 09m 52s (block 23426995 Mainnet) 16m 42s -> 16m 22s (block 23426996 Mainnet) ``` --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Edgar <[email protected]> Co-authored-by: Ivan Litteri <[email protected]> Co-authored-by: Mario Rugiero <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Our trie does a preorder traversal for recursive hashing, which allocates a lot of buffers (from the encoder and the keccak hasher) all the way down until it starts actually hashing a leaf. A way better approach to implement recursive hashing is to do a postorder traversal, in which we start allocating memory when we get to the lowest node that needs to be hashed, after which the memory is dealloc'd and the next (parent or sibling) node allocs again. This approach was inspired by risc0's trie, although we had a preorder traversal hashing for the
commit()function already.This PR also optimizes a
BranchNodeencoding by skipping ourEncodertype and writing directly to a preallocated buffer, instead of using two buffers and copying data from one to the other (this is because the currentEncodercan't know the length of the encoded data beforehand, but we can calculate it if we know what we are encoding). This could also be implemented for other node types, but they are the minority of node types and the perf. gains are negligible.A third, smaller optimization is to prevent cloning the cached/computed hashes from every node.
Description
memoize_hashesto bothNodeandNodeRefto implement postorder traversalNodeHashand a string of bytesBranchNode::encode_raw()to encode into a single buffercompute_hash_refto return a reference of the cached hashedTesting
This branch was used in a client snapsynced to Mainnet, running successfully from nov. 4 to nov. 5.
Flamegraphs block 23385900 Mainnet
this reduces cycles of
hash_no_commit(trie hashing) by 40%, which is 5% of total cyclesbefore:
after:
Proving times RTX 4090