Skip to content

Conversation

xqft
Copy link
Contributor

@xqft xqft commented Sep 25, 2025

Motivation

The prover's guest program takes as input a list of RLP encoded nodes, hashes them and later rebuilds the trie using from_nodes(), but this function does not cache the hashes already calculated, leading to double hashing.

This cuts calls of compute_hash by half (some remain because of new nodes that appear when modifying the state trie after block execution).

flamegraphs:
before:
image
after:
image

Proving time diffs:
image

@Copilot Copilot AI review requested due to automatic review settings September 25, 2025 16:06
@xqft xqft requested a review from a team as a code owner September 25, 2025 16:06
@github-actions github-actions bot added the L2 Rollup client label Sep 25, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes trie construction by caching pre-calculated hashes to avoid redundant hash computations during trie rebuilding from RLP-encoded nodes.

  • Introduces a with_hash method to NodeRef that allows setting pre-calculated hashes
  • Updates from_nodes method to use cached hashes when available instead of recomputing them
  • Reduces hash computation overhead by approximately half according to flamegraph analysis

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
crates/common/trie/node.rs Adds with_hash method to NodeRef for setting pre-calculated hashes
crates/common/trie/trie.rs Updates trie construction to use cached hashes via the new with_hash method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

Benchmark for f9320f6

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 35.8±0.46ms 35.2±0.66ms -1.68%
Trie/cita-trie insert 1k 3.6±0.02ms 3.6±0.11ms 0.00%
Trie/ethrex-trie insert 10k 56.0±0.38ms 54.6±0.53ms -2.50%
Trie/ethrex-trie insert 1k 6.2±0.04ms 6.1±0.02ms -1.61%

Copy link

Lines of code report

Total lines added: 13
Total lines removed: 0
Total lines changed: 13

Detailed view
+-----------------------------------+-------+------+
| File                              | Lines | Diff |
+-----------------------------------+-------+------+
| ethrex/crates/common/trie/node.rs | 249   | +6   |
+-----------------------------------+-------+------+
| ethrex/crates/common/trie/trie.rs | 908   | +7   |
+-----------------------------------+-------+------+

Comment on lines +88 to +91
/// # Warning
/// will lead to errors if setting the wrong hash, use only
/// when the hash was correctly precalculated
pub fn with_hash(self, hash: NodeHash) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have an indication in its name that it doesn't check the validity of the hash, and the warning comment should be a SAFETY one, so it's easier to grep for.

Suggested change
/// # Warning
/// will lead to errors if setting the wrong hash, use only
/// when the hash was correctly precalculated
pub fn with_hash(self, hash: NodeHash) -> Self {
/// # SAFETY: caller must ensure the hash is correct for the node.
/// Otherwise, the `Trie` will silently produce incorrect results and may
/// fail to query other nodes from the `TrieDB`.
pub fn with_hash_unchecked(self, hash: NodeHash) -> Self {

Some(rlp) => {
let node_ref: NodeRef =
get_embedded_node(all_nodes, rlp)?.into();
node_ref.with_hash(hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is safe to use it here since we passed the hash.is_valid() check, right?

Copy link
Contributor Author

@xqft xqft Sep 26, 2025

Choose a reason for hiding this comment

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

hash.is_valid() only checks that the hash is not empty, there isn't a way to check that a hash corresponds to a preimage without hashing again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L2 Rollup client
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants