Replace Tree generic with Arc<dyn TreeViewer>

[mattsse]

Describe the feature

This generic doesn't have any performance benefits because it's just just for simple get calls, which will also be refactored shortly.

It would make the setup a lot more convenient if we'd use Arc< dyn TreeViewer> here instead. Treeviewer trait is object safe so this should be very simple.

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

Lines 74 to 75 in 041e293

	/// The blockchain tree instance.
	tree: Tree,

This would let us cut down on a bunch of bloated types in the node-builder

reth/crates/node-builder/src/builder.rs

Lines 63 to 64 in 041e293

	type RethFullProviderType<DB, Evm> =
	BlockchainProvider<DB, ShareableBlockchainTree<DB, EvmProcessorFactory<Evm>>>;

and allows us to be less constraint during the node-builder setup

Additional context

No response

[KyrylR]

May I?

[mattsse]

assigned

[KyrylR]

Hello! I just started working on this issue and noticed the following structures:

impl<DB, Tree> BlockchainTreeEngine for BlockchainProvider<DB, Tree>
where
    DB: Send + Sync,
    Tree: BlockchainTreeEngine,
{
    // ...
}

impl<DB, Tree> BlockchainTreePendingStateProvider for BlockchainProvider<DB, Tree>
where
    DB: Send + Sync,
    Tree: BlockchainTreePendingStateProvider,
{
    fn find_pending_state_provider(
        &self,
        block_hash: BlockHash,
    ) -> Option<Box<dyn BundleStateDataProvider>> {
        self.tree.find_pending_state_provider(block_hash)
    }
}

impl<DB, Tree> CanonStateSubscriptions for BlockchainProvider<DB, Tree>
where
    DB: Send + Sync,
    Tree: CanonStateSubscriptions,
{
    fn subscribe_to_canonical_state(&self) -> CanonStateNotifications {
        self.tree.subscribe_to_canonical_state()
    }
}

If we intend to change to Arc<dyn TreeViewer> (as far as I understand), like this:

#[derive(Clone, Debug)]
pub struct BlockchainProvider<DB> {
    /// Provider type used to access the database.
    database: ProviderFactory<DB>,
    /// The blockchain tree instance.
    tree: Arc<dyn BlockchainTreeViewer>,
    /// Tracks the chain info wrt forkchoice updates
    chain_info: ChainInfoTracker,
}

Correct me if I'm wrong.

From what I can see:

The BlockchainTreeEngine trait implementation performs write operations on the blockchain tree; therefore, by its nature, it is incompatible with BlockchainTreeViewer.

The BlockchainTreePendingStateProvider and CanonStateSubscriptions trait implementations provide read-only operations related to accessing the pending state and subscribing to canonical state updates, respectively. Therefore, it requires a new trait to be created or modification of an existing one. (I believe the latter option could introduce some conflicts, so it's less feasible.)

Maybe I need to create a new trait, TreeViewer?

UPD: And yeah, a tree viewer should definitely not contain BlockchainTreeEngine, as it's a viewer. 😅

cc @mattsse

[mattsse]

this is a good point, I think what we want is a trait that unifies all the functions we need for the BlockchainProvider so that we have a single trait we can use as dyn

[KyrylR]

Got it, so I'll proceed with the creation of a unified trait

But what do we do with BlockchainTreeEngine? Just remove it?

[mattsse]

sg!
I'd like to rename this to something like PendingStateInfo, because we're gonna change the tree interface very soon

BlockchainTreeEngine

if unused can be removed

[mattsse]

@KyrylR just checking in
do you have bandwidth for this? :) lmk if you need more pointers

this will become a blocker shortly

[KyrylR]

Hello, sorry for the delay
I am going to open a PR today, really wanna give it a try

[mattsse]

cool!

[KyrylR]

I have a few more questions

Right now I am heading in the direction with such a trait:

pub trait TreeViewer:
    BlockchainTreeViewer
    + BlockchainTreePendingStateProvider
    + CanonStateSubscriptions
    + BlockchainTreeEngine
{
}

So, we have such a structure in the end:

#[derive(Clone)]
#[allow(missing_debug_implementations)]
pub struct BlockchainProvider<DB> {
    /// Provider type used to access the database.
    database: ProviderFactory<DB>,
    /// The blockchain tree instance.
    tree: Arc<dyn TreeViewer>,
    /// Tracks the chain info wrt forkchoice updates
    chain_info: ChainInfoTracker,
}

Is this the intended way? For me, it feels not right

I am not a full-time Rust dev, so I could still miss some obvious details

By the keyword dyn, I assume it should be a trait, but I'm still struggling with its definition

I cannot define anything alongside BlockchainTreeViewer, as from interfaces/src/blockchain_tree, I cannot (and I believe should not) access the BlockchainTreePendingStateProvider and CanonStateSubscriptions

I'd like to rename this to something like PendingStateInfo

Are you referring to the TreeViewer that I intend to create?

Off-topic: Is it okay to tag you when I have a question? Or is a simple message enough?

cc @mattsse

[mattsse]

Off-topic: Is it okay to tag you when I have a question? Or is a simple message enough?

anytime, can also DM on tg @mattsse

this looks okay, we need to combine all traits we use in the the impls, with a helper trait like you did,
then we can add a blanket impl for all T : BlockchainTreeViewer + BlockchainTreePendingStateProvider + CanonStateSubscriptions + BlockchainTreeEngine

this is only a temporary solution but is what we want

[KyrylR]

Got it, the last point where I encounter a problem is with ShareableBlockchainTree:

pub struct ShareableBlockchainTree<DB, EF> {
    /// BlockchainTree
    pub tree: Arc<RwLock<BlockchainTree<DB, EF>>>,
}

I am trying to reuse it like this:

let blockchain_tree = Arc::new(ShareableBlockchainTree::new(tree));

// Set up the blockchain provider
let blockchain_db =
    BlockchainProvider::new(provider_factory.clone(), blockchain_tree.clone())?;

But the problem that I see is the structure of this code is Arc -> Arc -> BlockchainTree

Is this ok?

cc @mattsse