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

mattsse

left a few questions

mattsse · 2024-11-22T09:38:25Z

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

+            // try to receive state updates without blocking
+            match self.state_stream.rx.try_recv() {


this is now a busy loop, right?
ideally we can use just recv here because then this can yield

yep, if we recv here we would be blocking until all the state updates are received, ie. until the block is executed, and the benefits of the task would be diminished? the proof calculation for each state root change would be triggered, but we would not trigger the state root calculation from proofs.

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

mattsse · 2024-11-22T09:49:41Z

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

+    }
+
+    fn run(mut self) -> StateRootResult {
+        let mut task_state = StateRootTaskState::Idle(MultiProof::default(), B256::default());


not super easy to follow how state management is done here

looks like we're sending a few things around and need to await things from different event variants from different sources, hence the try_recv to not block indefinitely on one kind of receiver?

I assume we can make this work with only one channel if we unify the message variants into one single enum?

looks like we're sending a few things around and need to await things from different event variants from different sources, hence the try_recv to not block indefinitely on one kind of receiver?

yes, exactly

I assume we can make this work with only one channel if we unify the message variants into one single enum?

yep sounds good, will prepare changes for this

ok done, the main changes are:

we have now a StateRootMessage enum with all the message variants for state change inside the task.

the task creates an internal channel for sending and receive messages of type StateRootMessage

the task also creates a background thread that just receives items from the input state stream, wraps them into the correspondent StateRootMessage variant and forwards them to the internal StateRootMessage channel. It calls recv in the external channel receiver, so that it yields

run now calls recv in the internal StateRootMessage receiver (yielding) and processes each message

there's a separate ProofSequencer type to manage proofs in order.

@mattsse ptal, still not tested on the mainnet machine, but local checks are looking good

…ection

… ongoing proof calculations received

…e_root_from_proofs

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

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 from 8e7f598 to e734d3f Compare November 22, 2024 17:54

fgimenez added 3 commits November 24, 2024 13:36

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

60d2b3e

fix account trie nodes collection

c251741

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

31d88b3

…ection

fgimenez and others added 13 commits November 24, 2024 13:36

extend test to check state root value

44c39aa

use reth_testing_utils::generators

e518b7c

fix test

0f787e0

prevent state root calculation and returning final result without the…

15178e2

… ongoing proof calculations received

do not share provider among workers

d52bdad

add error traces for multiproof tasks

3ca5a25

use map_init to initialize provider once per thread in calculate_stat…

06f3f32

…e_root_from_proofs

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

6849f8e

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

fmt

2fa00fd

refactor: StateRootMessages and single channel for managing task state

3e45f38

fix add proof

2c08410

fix add_proof

9a4aa01

reduce test state elements

1665120

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

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

mattsse left a comment

mattsse Nov 22, 2024

fgimenez Nov 22, 2024

mattsse Nov 22, 2024

fgimenez Nov 22, 2024

mattsse Nov 22, 2024

fgimenez Nov 22, 2024

fgimenez Nov 22, 2024


		pending_proofs.push_back(rx);

		state.extend(hashed_state_update);

		// try to receive state updates without blocking
		match self.state_stream.rx.try_recv() {

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

Are you sure you want to change the base?

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

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

fgimenez commented Nov 11, 2024 •

edited

Loading