Replace Tree generic with Arc<dyn TreeViewer> about reth HOT 12 CLOSED

assigned

from reth.

May I?

from reth.

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

from reth.

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

from reth.

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

from reth.

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

from reth.

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

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

from reth.

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

from reth.

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

this will become a blocker shortly

from reth.

cool!

from reth.

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

from reth.

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

from reth.