Skip to content

Conversation

@jrchatruc
Copy link
Collaborator

@jrchatruc jrchatruc commented Oct 6, 2025

Motivation

This PR reintroduces the optimizations reverted in #4734, now fixed. There were two problems with it:

  • There was a bug in our new (sorted) trie insertion that meant we incorrectly reconstructed certain storage tries.
  • There was a bug in our logic to keep downloading storage leaves on a pivot jump, where we would not correctly handle accounts whose storage root went back to a previous value we already had in our trie from a previous pivot. The way we handle this now is completely best effort: we try downloading storage leaves for a while and then just switch to storage healing afterwards. We will improve this when we rewrite snap sync; the main idea is to differentiate between big storage accounts and small ones. For big ones we want to ensure we download all their storage leaves no matter what; for small ones it's fine to do it best effort and just fallback to storage healing afterwards.

Description

Closes #issue_number

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Benchmark for d960b58

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 34.4±2.22ms 35.8±1.21ms +4.07%
Trie/cita-trie insert 1k 3.6±0.03ms 3.6±0.19ms 0.00%
Trie/ethrex-trie insert 10k 56.0±1.38ms 55.0±0.53ms -1.79%
Trie/ethrex-trie insert 1k 6.1±0.02ms 6.1±0.02ms 0.00%

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Benchmark for 27e30de

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 35.5±1.91ms 38.8±2.21ms +9.30%
Trie/cita-trie insert 1k 3.5±0.03ms 3.7±0.11ms +5.71%
Trie/ethrex-trie insert 10k 56.5±1.56ms 60.9±2.81ms +7.79%
Trie/ethrex-trie insert 1k 6.2±0.04ms 6.3±0.13ms +1.61%

@jrchatruc jrchatruc marked this pull request as ready for review October 6, 2025 17:55
@jrchatruc jrchatruc requested a review from a team as a code owner October 6, 2025 17:55
@ethrex-project-sync ethrex-project-sync bot moved this to In Review in ethrex_l1 Oct 6, 2025
@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Benchmark for a5dd3a7

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 36.4±2.31ms 36.0±2.10ms -1.10%
Trie/cita-trie insert 1k 3.6±0.03ms 3.5±0.02ms -2.78%
Trie/ethrex-trie insert 10k 55.8±1.38ms 55.7±1.20ms -0.18%
Trie/ethrex-trie insert 1k 6.2±0.02ms 6.1±0.16ms -1.61%

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Benchmark for 822ad39

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 37.1±1.07ms 39.5±1.02ms +6.47%
Trie/cita-trie insert 1k 3.6±0.02ms 3.7±0.12ms +2.78%
Trie/ethrex-trie insert 10k 56.0±1.60ms 56.2±1.16ms +0.36%
Trie/ethrex-trie insert 1k 6.1±0.01ms 6.1±0.09ms 0.00%

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Benchmark for 932000d

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 35.2±0.90ms 35.2±0.34ms 0.00%
Trie/cita-trie insert 1k 3.5±0.03ms 3.5±0.10ms 0.00%
Trie/ethrex-trie insert 10k 55.2±0.66ms 55.1±1.75ms -0.18%
Trie/ethrex-trie insert 1k 6.1±0.04ms 6.1±0.01ms 0.00%

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

Benchmark for 6ac36c3

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 37.5±2.19ms 37.6±1.38ms +0.27%
Trie/cita-trie insert 1k 3.6±0.02ms 3.6±0.16ms 0.00%
Trie/ethrex-trie insert 10k 55.9±0.82ms 56.7±0.86ms +1.43%
Trie/ethrex-trie insert 1k 6.2±0.07ms 6.2±0.07ms 0.00%

@github-actions
Copy link

github-actions bot commented Oct 7, 2025

Benchmark for 9739147

Click to view benchmark
Test Base PR %
Trie/cita-trie insert 10k 40.8±2.99ms 38.5±2.72ms -5.64%
Trie/cita-trie insert 1k 3.6±0.03ms 3.5±0.14ms -2.78%
Trie/ethrex-trie insert 10k 56.4±1.08ms 56.5±0.86ms +0.18%
Trie/ethrex-trie insert 1k 6.1±0.03ms 6.1±0.02ms 0.00%

]
metrics = ["ethrex-blockchain/metrics", "ethrex-l2/metrics"]
rocksdb = ["ethrex-storage/rocksdb"]
rocksdb = ["ethrex-storage/rocksdb", "ethrex-p2p/rocksdb"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we avoid coupling p2p with the specific db implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. A priori is not that easy to decouple it, and we need these optimizations. I'll file an issue to tackle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fedacking, please file the issue to tackle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in #4807

Comment on lines +1511 to +1512
for (account_hash, account) in account_states_snapshot {
trie.insert(account_hash.0.to_vec(), account.encode_to_vec())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is missing calls to the metrics module. The "inserting accounts" progress stays at 0% even when we're past that stage.

@jrchatruc jrchatruc added this pull request to the merge queue Oct 8, 2025
Merged via the queue into main with commit 8957ff2 Oct 8, 2025
49 checks passed
@jrchatruc jrchatruc deleted the merge_optimizations_fast_insert_semaphore branch October 8, 2025 15:43
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Oct 8, 2025
@rodrigo-o rodrigo-o restored the merge_optimizations_fast_insert_semaphore branch October 8, 2025 17:51
@rodrigo-o rodrigo-o deleted the merge_optimizations_fast_insert_semaphore branch October 8, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

8 participants