feat(engine): proof fetching on state update for StateRootTask #12458

fgimenez · 2024-11-11T19:08:20Z

Towards #12053
Requires #12378 #12467 #12510

rkrasiuk · 2024-11-18T16:04:30Z

crates/trie/parallel/src/root.rs

@@ -237,6 +241,7 @@ impl From<ParallelStateRootError> for ProviderError {
            ParallelStateRootError::StorageRoot(StorageRootError::Database(error)) => {
                Self::Database(error)
            }
+            ParallelStateRootError::Other(other) => Self::Database(DatabaseError::Other(other)),


this should not be casted to database error. out of scope for this PR, but we have to fix this at some point ^^

crates/engine/tree/src/tree/root.rs

rkrasiuk · 2024-11-21T21:03:13Z

crates/engine/tree/src/tree/root.rs

+
+        pending_proofs.push_back(rx);
+
+        state.extend(hashed_state_update);


this extends the state immediately and that's what is used to perform state root updates, but this might result in invalid cached trie nodes. not sure yet, but worth double checking @fgimenez

I believe this is still relevant and should be solved by updating the state only when we finish calculating the proof

mattsse

left a few questions

crates/engine/tree/src/tree/root.rs

mattsse · 2024-11-22T09:47:10Z

crates/engine/tree/src/tree/root.rs

+                        rayon::spawn(move || {
+                            let result = calculate_state_root_from_proofs(
+                                view,
+                                &input_nodes_sorted,
+                                &input_state_sorted,
+                                multiproof,
+                                state,
+                            );
+                            let _ = tx.send(result);
+                        });


this spawns a new rayon job that then also branches out via par_iter
all of this looks very IO heavy

yep, that's the case

crates/engine/tree/src/tree/root.rs

rkrasiuk · 2024-11-26T20:54:54Z

crates/engine/tree/src/tree/root.rs

+                let destroyed = account.is_selfdestructed();
+                hashed_state_update.accounts.insert(
+                    hashed_address,
+                    if destroyed || account.is_empty() { None } else { Some(account.info.into()) },


account can be destroyed and recreated, worth checking again if this line is correct

rkrasiuk · 2024-11-26T20:55:57Z

crates/engine/tree/src/tree/root.rs

+
+            // TODO: replace with parallel proof
+            let result =
+                Proof::overlay_multiproof(provider.tx_ref(), input.as_ref().clone(), targets);


this clone is expensive, we should find a way to get rid of it

it happens inside the ParallelProof anyway, but maybe we can avoid it altogether

reth/crates/trie/parallel/src/proof.rs

Lines 65 to 69 in e8d63e4

let trie_nodes_sorted = Arc::new(self.input.nodes.into_sorted());

let hashed_state_sorted = Arc::new(self.input.state.into_sorted());

// Extend prefix sets with targets

let mut prefix_sets = self.input.prefix_sets.clone();

in parallel proof it only re-allocates. here it clones and re-allocates

rkrasiuk · 2024-11-26T20:58:09Z

crates/engine/tree/src/tree/root.rs

+                    )?);
+                }
+
+                let storage_root = next_root_from_proofs(storage_trie_nodes, |key: Nibbles| {


next_root_from_proofs function served its purpose, but is not always correct (see #12859). let's integrate sparse trie and debug it instead

…ection

… ongoing proof calculations received

…e_root_from_proofs

Co-authored-by: Roman Krasiuk <[email protected]>

…ate with single channel

Co-authored-by: Alexey Shekhirin <[email protected]>

rkrasiuk

taking notes of outstanding comments. let's merge this PR and iterate in follow ups

fgimenez added C-perf A change motivated by improving speed, memory usage or disk footprint A-blockchain-tree Related to sidechains, reorgs and pending blocks A-trie Related to Merkle Patricia Trie implementation labels Nov 11, 2024

fgimenez mentioned this pull request Nov 11, 2024

perf(engine): create standalone state root task #12053

Open

fgimenez force-pushed the fgimenez/srt-sync-async-impls branch from d0f48d5 to fdb55e9 Compare November 12, 2024 15:34

fgimenez force-pushed the fgimenez/srt-proof-fetching branch from 14ca9a0 to da06971 Compare November 12, 2024 15:35

fgimenez mentioned this pull request Nov 13, 2024

feat(trie): turn TrieWitness methods into public functions #12510

Merged

fgimenez force-pushed the fgimenez/srt-proof-fetching branch from c38de83 to a8e0cf2 Compare November 13, 2024 13:15

fgimenez force-pushed the fgimenez/srt-sync-async-impls branch from fdb55e9 to 5d2048f Compare November 13, 2024 13:34

fgimenez force-pushed the fgimenez/srt-proof-fetching branch from a8e0cf2 to 6e0e20e Compare November 13, 2024 13:35

Base automatically changed from fgimenez/srt-sync-async-impls to main November 18, 2024 14:19

fgimenez force-pushed the fgimenez/srt-proof-fetching branch from 6e0e20e to 3b04efb Compare November 18, 2024 15:35

fgimenez marked this pull request as ready for review November 18, 2024 15:50

fgimenez requested review from rkrasiuk, Rjected, shekhirin, mattsse and gakonst as code owners November 18, 2024 15:50

rkrasiuk reviewed Nov 18, 2024

View reviewed changes

fgimenez force-pushed the fgimenez/srt-proof-fetching branch 4 times, most recently from 52e1366 to 2ffe7c8 Compare November 21, 2024 09:43

shekhirin reviewed Nov 21, 2024

View reviewed changes

crates/engine/tree/src/tree/root.rs Outdated Show resolved Hide resolved

rkrasiuk requested changes Nov 21, 2024

View reviewed changes

mattsse reviewed Nov 22, 2024

View reviewed changes

fgimenez force-pushed the fgimenez/srt-proof-fetching branch 2 times, most recently from f982fa2 to 1665120 Compare November 24, 2024 16:49

shekhirin mentioned this pull request Nov 25, 2024

perf(engine): sparse trie calculation for state root task #12843

Merged

shekhirin reviewed Nov 25, 2024

View reviewed changes

crates/engine/tree/src/tree/root.rs Outdated Show resolved Hide resolved

rkrasiuk requested changes Nov 26, 2024

View reviewed changes

fgimenez and others added 22 commits November 27, 2024 10:03

feat(engine): proof fetching on state update for StateRootTask

098a08b

fix account trie nodes collection

154aa8c

add state root from proofs bench and simplify account trie nodes coll…

d950182

…ection

extend test to check state root value

06db565

use reth_testing_utils::generators

38d0b66

fix test

acf8254

prevent state root calculation and returning final result without the…

ea292e8

… ongoing proof calculations received

do not share provider among workers

02e75e8

add error traces for multiproof tasks

74bf59f

use map_init to initialize provider once per thread in calculate_stat…

9eadda3

…e_root_from_proofs

Update crates/engine/tree/src/tree/root.rs

7032bfe

Co-authored-by: Roman Krasiuk <[email protected]>

fmt

a4c3bbf

refactor: StateRootMessages and single channel for managing task state

cebcbd8

fix add proof

00345a6

fix add_proof

589b1b2

reduce test state elements

227fdc4

refactor: remove incoming state updates thread

992e96d

StateRootMessage -> InternalMessage

0d4e70b

feat(engine): use non-parallel multiproof for state root task (#12849)

44d3a61

add StateRootMessage::StateUpdate variant and manage internal task st…

776cef8

…ate with single channel

Update crates/engine/tree/src/tree/root.rs

6b3d67a

Co-authored-by: Alexey Shekhirin <[email protected]>

fix add_proof

344a9b8

fgimenez force-pushed the fgimenez/srt-proof-fetching branch from fbc22f5 to 344a9b8 Compare November 27, 2024 10:01

shekhirin approved these changes Nov 27, 2024

View reviewed changes

rkrasiuk approved these changes Nov 27, 2024

View reviewed changes

fgimenez added this pull request to the merge queue Nov 27, 2024

Merged via the queue into main with commit 1131bde Nov 27, 2024
42 checks passed

fgimenez deleted the fgimenez/srt-proof-fetching branch November 27, 2024 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(engine): proof fetching on state update for StateRootTask #12458

feat(engine): proof fetching on state update for StateRootTask #12458

fgimenez commented Nov 11, 2024 •

edited

Loading

rkrasiuk Nov 18, 2024

rkrasiuk Nov 21, 2024

shekhirin Nov 26, 2024

mattsse left a comment

mattsse Nov 22, 2024

fgimenez Nov 22, 2024

rkrasiuk Nov 26, 2024

rkrasiuk Nov 26, 2024

shekhirin Nov 26, 2024

rkrasiuk Nov 27, 2024

rkrasiuk Nov 26, 2024

rkrasiuk left a comment


		pending_proofs.push_back(rx);

		state.extend(hashed_state_update);

	let trie_nodes_sorted = Arc::new(self.input.nodes.into_sorted());
	let hashed_state_sorted = Arc::new(self.input.state.into_sorted());

	// Extend prefix sets with targets
	let mut prefix_sets = self.input.prefix_sets.clone();

feat(engine): proof fetching on state update for StateRootTask #12458

feat(engine): proof fetching on state update for StateRootTask #12458

Conversation

fgimenez commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkrasiuk left a comment

Choose a reason for hiding this comment

fgimenez commented Nov 11, 2024 •

edited

Loading