feat: add new BlockchainProvider type #9656

fgimenez · 2024-07-19T16:30:06Z

mattsse

I thought about this a bit, as you highlighted we're now running into cyclic dep issues.
we could first move the types to provider like this but I wonder if we could improve the engine/tree layout instead (or later)

mattsse · 2024-07-20T07:48:15Z

crates/storage/provider/src/providers/state/in_memory.rs

I wonder if we should move these types to a new crate instead.

the provider crate is already a bit messy, so I wonder how to structure this.
I think the ideal solution here is that the engine/tree does not require the new provider type directly and instead operates on provider traits and has the Inmemorystate as a separate field, because otherwise we need to go through the provider to update state and exposing these functions publicly on the provider type sounds a bit dangerous

we should also consider splitting the content of engine/tree into multiple smaller crates for

persistence + database

orchestrator + handlers

state for the inmemory types

makes total sense, will prepare changes for this

cool, I think we can have the orchestrator and related types in one crate and then the actual engineapi impl in another, and the db+persistence in an additional crate

so that
engineapiimpl imports orchestrator + persistence

fgimenez

Ok, I have removed some redundant tree trait impls that will be handled differently now in the communication of the beaconenggine and the tree, and changed some of them to use the in memory representation

I've also left some questions mainly regarding the pending state provider @mattsse ptal

fgimenez · 2024-07-22T17:48:04Z

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

+        trace!(target: "providers::blockchain", "Getting provider for pending state");
+
+        if let Some(block) = self.canonical_in_memory_state.pending_block_num_hash() {
+            if let Ok(pending) = self.tree.pending_state_provider(block.hash) {


Not sure what to do with this pending_state_provider, should we implement it in the in memory state?

I think we'd need to check if there's a pending block in the canonical memory state, if not this should fallback to latest

this is similar to the previous question, this method returns a ProviderResult<StateProviderBox> how do we obtain a StateProvider including in memory information? I think this affects all the methods in the StateProviderFactory impl

fgimenez · 2024-07-22T17:48:22Z

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

+    }
+
+    fn pending_state_by_hash(&self, block_hash: B256) -> ProviderResult<Option<StateProviderBox>> {
+        if let Some(state) = self.tree.find_pending_state_provider(block_hash) {


Same, also related to pending state provider

fgimenez · 2024-07-22T17:48:50Z

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

+    DB: Send + Sync,
+{
+    fn subscribe_to_canonical_state(&self) -> CanonStateNotifications {
+        self.tree.subscribe_to_canonical_state()


Should we impl this trait CanonStateSubscriptions?

yeah, we need this in rpc and other components, what we need to do is, move the broadcast channel into the canonicalinmemory type, as a replacement for

reth/crates/blockchain-tree/src/blockchain_tree.rs

Lines 73 to 74 in 8c690ee

/// Broadcast channel for canon state changes notifications.

canon_state_notification_sender: CanonStateNotificationSender,

makes sense, done here 09e51ed, already being used in the CanonStateSubscriptions impl here 26bc11b

fgimenez · 2024-07-22T17:50:49Z

crates/chain-state/src/in_memory.rs

        &self.0.execution_outcome().receipts
    }
+
+    /// Returns a vector of `Receipt` of executed block that determines the state.
+    pub fn flattened_receipts(&self) -> Vec<Receipt> {


This is required by a method in BlockReader is it ok to flatten like this the Vec<Vec<Option<Receipt>>> that Receipts contains?

Unfortunately the Receipts type refers to multiple blocks' worth of receipts because the ExecutionOutcome is for both single and multiple blocks. In this case we can expect there to be only one element in the outer Vec. One thing we could do is debug_assert that the list is length one, and .first().unwrap() until we find a better way to handle this, does that make sense?

makes total sense, done here ef07c86 ptal

makes sense to me, I like the comment

mattsse

awesome, finding the corresponding values will be a bit more tricky because now that we store canonical blocks in two locations we also need to check both of them when we try to find blocks.

similar situation as with the existing blockchainprovider but now the separation is not pending vs canonical but memory vs disk, for now we can simply check both locations, the inmemory lookup should be super cheap because this is just an rwlock.

functionality wise this pr looks complete and we can iterate on this and the inmemorycanonical state in more smaller scoped prs

I believe we also need to keep track of the block range we currently have in memory and think about race conditions a bit.
since we flush to disko before we update the memory state, we have a guarantee that the state is present somewhere, so we should likely always check inmemory first and then Hit disk if we couldn't find it

mattsse · 2024-07-22T20:37:31Z

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

+            BlockSource::Pending => self
+                .canonical_in_memory_state
+                .state_by_hash(hash)
+                .map(|block_state| block_state.block().block().clone().unseal()),
+            BlockSource::Canonical => self.database.block_by_hash(hash)?,


this is now a bit blurry, because the canonical in memory state is also canonical, so we need to handle this like any.
and pending should just check the pending block

very good point, done here 6a00ec8, now we handle BlockSource::Canonical and BlockSource::Any with the same code, I've changed it a bit to check the in memory representation before hitting the db, let me know wdyt. And for BlockSource::Pending we just return the pending block if present.

mattsse · 2024-07-22T20:39:27Z

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

+        self.ensure_canonical_block(block_number)?;
+        self.database.history_by_block_number(block_number)
+    }
+
+    fn history_by_block_hash(&self, block_hash: BlockHash) -> ProviderResult<StateProviderBox> {
+        trace!(target: "providers::blockchain", ?block_hash, "Getting history by block hash");
+        self.database.history_by_block_hash(block_hash)
+    }


these would also need to check the in memory provider as well because from caller pov all blocks in there are also historical blocks, part of the canonical chain

hmm but these methods are returning a StateProviderBox , which is Box<dyn StateProvider> and in memory we are only storing a set of blocks, how can we impl StateProvider for the in memory rep. of a few blocks?

we're still lacking this feature: #9614
to unblock this pr we can ignore the memory state for now and track in an issue instead

ok, I've left todo's about this

mattsse · 2024-07-22T20:40:20Z

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

+        trace!(target: "providers::blockchain", "Getting provider for pending state");
+
+        if let Some(block) = self.canonical_in_memory_state.pending_block_num_hash() {
+            if let Ok(pending) = self.tree.pending_state_provider(block.hash) {


I think we'd need to check if there's a pending block in the canonical memory state, if not this should fallback to latest

mattsse · 2024-07-22T20:43:06Z

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

+    DB: Send + Sync,
+{
+    fn subscribe_to_canonical_state(&self) -> CanonStateNotifications {
+        self.tree.subscribe_to_canonical_state()


yeah, we need this in rpc and other components, what we need to do is, move the broadcast channel into the canonicalinmemory type, as a replacement for

reth/crates/blockchain-tree/src/blockchain_tree.rs

Lines 73 to 74 in 8c690ee

/// Broadcast channel for canon state changes notifications.

canon_state_notification_sender: CanonStateNotificationSender,

…criptions impl

mattsse

lets send it

fgimenez force-pushed the fgimenez/blockchain-provider-type branch from f2b99de to c51f37a Compare July 19, 2024 16:46

mattsse reviewed Jul 20, 2024

View reviewed changes

Rjected changed the title ~~feat: add new BlockahainProvider type~~ feat: add new BlockchainProvider type Jul 22, 2024

fgimenez force-pushed the fgimenez/blockchain-provider-type branch from c51f37a to 5847557 Compare July 22, 2024 15:45

fgimenez commented Jul 22, 2024

View reviewed changes

mattsse requested changes Jul 22, 2024

View reviewed changes

fgimenez force-pushed the fgimenez/blockchain-provider-type branch from 7669fdd to ef07c86 Compare July 23, 2024 08:28

fgimenez added 11 commits July 23, 2024 16:59

feat: add new BlockchainProvider type

5a70d67

add missing method in StageCheckpointReader impl

940210b

fix impls

1075d85

introduce BlockState::flattened_receipts

7966e48

refactor with more CanonicalInMemoryState methods

e35a6ef

flattened_receipts -> executed_block_receipts

a9f6f66

fix find_block_by_hash

3f59254

add canon_state_notiication_sender to CanonicalInMemoryState

09e51ed

add todo's for in memory overlay

f911a97

use canonical_in_memory_state.subscribe_canon_state in CanonStateSubs…

26bc11b

…criptions impl

clippy

3afd7f3

fgimenez force-pushed the fgimenez/blockchain-provider-type branch from 6a00ec8 to 3afd7f3 Compare July 23, 2024 15:18

fix docs

77893a2

fgimenez marked this pull request as ready for review July 23, 2024 15:30

fgimenez requested review from rakita, joshieDo, shekhirin and gakonst as code owners July 23, 2024 15:30

mattsse approved these changes Jul 23, 2024

View reviewed changes

mattsse added this pull request to the merge queue Jul 23, 2024

Merged via the queue into main with commit 2209381 Jul 23, 2024
32 checks passed

mattsse deleted the fgimenez/blockchain-provider-type branch July 23, 2024 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new BlockchainProvider type #9656

feat: add new BlockchainProvider type #9656

fgimenez commented Jul 19, 2024 •

edited

Loading

mattsse left a comment

mattsse Jul 20, 2024 •

edited

Loading

fgimenez Jul 20, 2024

mattsse Jul 20, 2024

fgimenez left a comment

fgimenez Jul 22, 2024

mattsse Jul 22, 2024

fgimenez Jul 23, 2024 •

edited

Loading

fgimenez Jul 22, 2024

fgimenez Jul 22, 2024

mattsse Jul 22, 2024

fgimenez Jul 23, 2024

fgimenez Jul 22, 2024

Rjected Jul 22, 2024

fgimenez Jul 23, 2024

Rjected Jul 23, 2024

mattsse left a comment

mattsse Jul 22, 2024

fgimenez Jul 23, 2024

mattsse Jul 22, 2024

fgimenez Jul 23, 2024

mattsse Jul 23, 2024

fgimenez Jul 23, 2024

mattsse Jul 22, 2024

mattsse Jul 22, 2024

mattsse left a comment

	/// Broadcast channel for canon state changes notifications.
	canon_state_notification_sender: CanonStateNotificationSender,

feat: add new BlockchainProvider type #9656

feat: add new BlockchainProvider type #9656

Conversation

fgimenez commented Jul 19, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jul 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgimenez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgimenez Jul 23, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

fgimenez commented Jul 19, 2024 •

edited

Loading

mattsse Jul 20, 2024 •

edited

Loading

fgimenez Jul 23, 2024 •

edited

Loading