feat: BlockchainTree by rakita · Pull Request #1212 · paradigmxyz/reth

rakita · 2023-02-07T19:09:41Z

closes: #1104

gakonst

@rakita good start, I think before merging this, would like to understand exactly where the point of integration with the rest of the code will be? is it going to be engine api? how are we going to expose a single block to get executed?

gakonst · 2023-02-08T01:14:27Z

crates/executor/src/blockchain_tree/chain.rs

+        _provider: &PROVIDER,
+        _consensus: &CONSENSUS,


The provider & consensus should prob be part of the struct?

rakita · 2023-02-08T09:09:53Z

@rakita good start, I think before merging this, would like to understand exactly where the point of integration with the rest of the code will be? is it going to be engine api? how are we going to expose a single block to get executed?

Let's move this discussion to the main task, it will get dissolved if we start discussion it on two places.

gakonst

Took another pass and looking nice. I think you already know this but let's add a few tests around insert/make-canonical for a few of the scenarios you mention, to make sure we handle things correctly.

I think we'll probably end up doing insert_chain on each new_payload and then on fork_choice_updated we'll be "flushing", which realistically means having the headers+bodies+execution stage run for that part.

crates/storage/db/src/tables/mod.rs

crates/primitives/src/block.rs

rakita · 2023-02-10T10:12:32Z

Took another pass and looking nice. I think you already know this but let's add a few tests around insert/make-canonical for a few of the scenarios you mention, to make sure we handle things correctly.

I think we'll probably end up doing insert_chain on each new_payload and then on fork_choice_updated we'll be "flushing", which realistically means having the headers+bodies+execution stage run for that part.

Yeah, testing is completely missing and some TODO are pending. I wanted to make a draft so that @rkrasiuk can see what is found inside and how it can be used.

rkrasiuk · 2023-02-10T10:57:21Z

@rakita i will take some time today to review this, thanks!

rkrasiuk

in general, the interface looks good. didn't do an in-depth review of the implementation. leaving couple of questions

crates/executor/src/blockchain_tree/mod.rs

rkrasiuk

@rakita this implementation is safe if it will be invoked exclusively by the engine API, because the latter is implemented as a Future and consumes the requests successively. otherwise, i believe, we would need some lock mechanism to prevent concurrent operations on the blockchain tree

crates/executor/src/blockchain_tree/block_indices.rs

crates/executor/src/blockchain_tree/chain.rs

mattsse

I think I understand the internals.
I didn't review very closely but I could kinda follow along.
test coverage looks great.

perhaps I'm being a bit pedantic, but I want to emphasize that we likely don't want pub field access for most of the things.

bin/reth/src/p2p/mod.rs

crates/executor/src/blockchain_tree/block_indices.rs

crates/executor/src/blockchain_tree/chain.rs

crates/executor/src/blockchain_tree/mod.rs

crates/executor/src/blockchain_tree/block_indices.rs

crates/executor/src/blockchain_tree/chain.rs

crates/storage/provider/src/providers/mod.rs

crates/storage/provider/src/test_utils/blocks.rs

onbjerg · 2023-03-10T15:38:14Z

crates/storage/provider/src/transaction.rs

+    use std::ops::DerefMut;
+
+    #[test]
+    fn insert_get_take() {


There are way too few tests here considering everything in the stages crate has been moved here at this point.

What is moved from stage still gets tested from it. With test data i tried to cover cases that can happen in both getting and taking data from database, and additionally, BlockchainTree is testing this functionality in its own way.

As part of the refactor of the Transaction struct during our tech debt cleanup, we should introduce more tests

crates/executor/src/substate.rs

crates/executor/src/blockchain_tree/mod.rs

crates/executor/src/blockchain_tree/block_indices.rs

crates/executor/src/blockchain_tree/chain.rs

crates/storage/provider/src/transaction.rs

crates/executor/src/blockchain_tree/mod.rs

gakonst · 2023-03-14T03:26:13Z

@onbjerg @rkrasiuk please take another pass

@mattsse if you got bandwidth would love your eyes on this as you're wrapping RPC

rkrasiuk

leaving some minor comments and cautiously approving. well done overall! just a lot of moving parts here and we've done multiple passes at this over the last couple of days

bin/reth/src/db/mod.rs

crates/executor/src/blockchain_tree/block_indices.rs

rkrasiuk · 2023-03-14T08:46:08Z

@rakita pending conflict resolution

crates/executor/src/blockchain_tree/block_indices.rs

crates/executor/src/blockchain_tree/chain.rs

crates/executor/src/blockchain_tree/mod.rs

crates/primitives/src/block.rs

crates/primitives/src/header.rs

rkrasiuk · 2023-03-14T14:19:22Z

@rakita some tests seem to be failing

rkrasiuk

lessssssgoooooooooooooooooooooooooooooooooooo

gakonst

sweet. nice work on the tree tester, was really missing. let's ship it and iterate as we get closer to SyncController + Engine API tests on Hive

rakita mentioned this pull request Feb 7, 2023

BlockchainTree sidechain/pending block inmemory structure #1104

Closed

gakonst reviewed Feb 8, 2023

View reviewed changes

onbjerg added C-enhancement New feature or request A-consensus Related to the consensus engine labels Feb 8, 2023

gakonst reviewed Feb 9, 2023

View reviewed changes

crates/storage/db/src/tables/mod.rs Show resolved Hide resolved

crates/primitives/src/block.rs Outdated Show resolved Hide resolved

rkrasiuk reviewed Feb 13, 2023

View reviewed changes

crates/executor/src/blockchain_tree/mod.rs Outdated Show resolved Hide resolved

crates/executor/src/blockchain_tree/mod.rs Outdated Show resolved Hide resolved

rakita force-pushed the rakita/blockchain_tree branch 2 times, most recently from 50924bc to 43556de Compare February 21, 2023 17:45

rakita changed the base branch from main to rakita/refactor_stages_to_providers February 23, 2023 12:40

rkrasiuk reviewed Feb 23, 2023

View reviewed changes

rakita force-pushed the rakita/blockchain_tree branch from 2e67384 to ef3be7e Compare February 24, 2023 10:23

Base automatically changed from rakita/refactor_stages_to_providers to main March 1, 2023 20:42

rakita force-pushed the rakita/blockchain_tree branch 2 times, most recently from 87f66af to e794bfb Compare March 2, 2023 00:50

feat: BlockchainTree, pending and sidechain blocks

65e56b4

rakita force-pushed the rakita/blockchain_tree branch from e794bfb to 65e56b4 Compare March 2, 2023 00:58

rakita added 6 commits March 2, 2023 15:39

Update pipeline sync status, upate plain state on revert, tie reorg fn

0c84127

Refactor History/Hashing unwind stages.

e1ebfe3

Merge remote-tracking branch 'origin/main' into o/r/blockchain_tree

9b1d6c1

tests and debug

601849d

Merge remote-tracking branch 'origin/main' into o/r/blockchain_tree

1f981c6

nits

79c3e03

rakita force-pushed the rakita/blockchain_tree branch from 287e067 to 79c3e03 Compare March 4, 2023 17:13

rakita and others added 4 commits March 6, 2023 15:46

add tests for insert get_take fn

3e536bb

bug: Remove old account/storage root in merkle trie

33879bc

Merge remote-tracking branch 'origin/main' into o/r/blockchain_tree

55d71ed

Return new_root

423e75f

rakita requested a review from rkrasiuk March 10, 2023 09:20

update state root for tests

3eb05ff

rakita force-pushed the rakita/blockchain_tree branch from 1806404 to 3eb05ff Compare March 10, 2023 09:59

mattsse requested changes Mar 10, 2023

View reviewed changes

onbjerg requested changes Mar 10, 2023

View reviewed changes

rakita added 2 commits March 10, 2023 19:03

Introudce Arc<ChainSpec>, rm pub from Tree internals

a4a1077

nit and PR comments, first part

062cbf8

rakita requested a review from Rjected as a code owner March 10, 2023 20:41

gakonst reviewed Mar 10, 2023

View reviewed changes

rakita added 4 commits March 11, 2023 20:20

nits and bits, part2

06d6d5c

nits and bits p3

2db7a91

Merge remote-tracking branch 'origin/main' into r/blockchain_tree

33cad82

get take and test data refactor

5d0b7d4

rkrasiuk approved these changes Mar 14, 2023

View reviewed changes

bin/reth/src/db/mod.rs Outdated Show resolved Hide resolved

crates/executor/src/blockchain_tree/block_indices.rs Show resolved Hide resolved

crates/executor/src/blockchain_tree/block_indices.rs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into r/blockchain_tree

7d18b4a

mattsse reviewed Mar 14, 2023

View reviewed changes

fmt, nits.

f594321

TreeTester and some cleanup

d23ee32

rakita requested review from gakonst, mattsse, onbjerg and rkrasiuk March 14, 2023 15:48

rkrasiuk approved these changes Mar 14, 2023

View reviewed changes

gakonst approved these changes Mar 14, 2023

View reviewed changes

gakonst merged commit 237fd5c into main Mar 14, 2023

gakonst deleted the rakita/blockchain_tree branch March 14, 2023 18:17

gakonst mentioned this pull request Mar 15, 2023

perf: write friendly ExecutionResult #1674

Merged

5 tasks

Conversation

rakita commented Feb 7, 2023

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

gakonst Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

rakita commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rakita commented Feb 10, 2023

Uh oh!

rkrasiuk commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrasiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rkrasiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

onbjerg Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

rakita Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

gakonst Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gakonst commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrasiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rakita commented Feb 8, 2023 •

edited

Loading

rkrasiuk commented Feb 10, 2023 •

edited

Loading

gakonst commented Mar 14, 2023 •

edited

Loading