Make Node Types: Clone

[Cyber-Mitch]

Make NodeTypes: Clone

Description

The NodeTypes trait and its implementations are used extensively in the codebase, particularly when restricting trait bounds or using associated types. Since NodeTypes is intended to be stateless and only defines types, it should implement the Clone trait to make it easier to derive Clone for types that use NodeTypes.

Issue

The NodeTypes trait and its implementations did not implement the Clone trait.

Solutions Snippet

1. Add Clone to the NodeTypes trait definition:

   pub trait NodeTypes: Clone + Send + Sync + Unpin + 'static {
       // ...
   }

@mattsse closes #14345

[mattsse]

still some failing tests

[Cyber-Mitch]

still some failing tests

Yes i noticed. I was able to finally get make pr to work and i got some errors in files where the clone has to be also implemented, so i made few modifications to it be deriving clone. especially in the commiments.rs file. Every thing is done now, I just ran the command make pr command again. And there's a particular test taking long. This one benches/state_root_task.rs so when that is done i will commit and push the recent changes just give me a little time. @mattsse

[Cyber-Mitch]

The make pr command is till running this benches/state_root_task.rs I don't know why it's taking time @mattsse but i just commited and push the changes i made due to the previous errors, i ran cargo nextest run and build and passed all test and was nolonger throwing errors, but the issue here is make pr command is still running. but i think you can run the test from your end again it should pass this time

[mattsse]

a few more nits

[mattsse]

I think these are now redundant NodeTypes + Clone because NodeTypes already implies Clone

[Cyber-Mitch]

oh okay

[mattsse]

most of those are already clone so we likely don't need all of these changes

[Cyber-Mitch]

oh okay. I'm on it right away

[Cyber-Mitch]

most of those are already clone so we likely don't need all of these changes

Hey @mattsse i checked they aren't really redundant. If i remove the clone it throws an error like this the trait bound `C: Clone` is not satisfied. That was why i added them there initially.

Example
line 149 to 160 throws an error. The AnyNodeTypes<P, C, SC, S> Throws an error

impl<P, C, SC, S> NodeTypes for AnyNodeTypes<P, C, SC, S>
where
    P: NodePrimitives + Send + Sync + Unpin + 'static,
    C: EthChainSpec<Header = P::BlockHeader> + 'static,
    SC: StateCommitment,
    S: Default + Send + Sync + Unpin + Debug + 'static,
{
    type Primitives = P;
    type ChainSpec = C;
    type StateCommitment = SC;
    type Storage = S;
}

Error

the trait bound `C: Clone` is not satisfied
the trait `Clone` is not implemented for `C`, which is required by `AnyNodeTypes<P, C, SC, S>: Clone`rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20[0]?0#file:///home/reentrancy/reth/crates/node/types/src/lib.rs)
lib.rs(108, 10): required for `AnyNodeTypes<P, C, SC, S>` to implement `Clone`
lib.rs(28, 22): required by a bound in `NodeTypes`
lib.rs(152, 55): consider further restricting this bound: ` + std::clone::Clone`
the trait bound `SC: Clone` is not satisfied
the trait `Clone` is not implemented for `SC`, which is required by `AnyNodeTypes<P, C, SC, S>: Clone`rustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic%20message%20[4]?4#file:///home/reentrancy/reth/crates/node/types/src/lib.rs)
lib.rs(108, 10): required for `AnyNodeTypes<P, C, SC, S>` to implement `Clone`

[Cyber-Mitch]

Hey @mattsse i made a new commit, was able to remove some though, if i removed the rest it will throw errors. Check it out.

[kustrun]

Hey @Cyber-Mitch,

I believe the Clone trait can be encapsulated even further. Would love to hear your thoughts—feel free to check out my PR: https://github.com/kustrun/reth/pull/1/files#diff-ad159e48f13cb6b654f3866465707834a49427e87e6409cc8ee9b532b89ca988

[Cyber-Mitch]

Hey @Cyber-Mitch,

I believe the Clone trait can be encapsulated even further. Would love to hear your thoughts—feel free to check out my PR: https://github.com/kustrun/reth/pull/1/files#diff-ad159e48f13cb6b654f3866465707834a49427e87e6409cc8ee9b532b89ca988

okay. This is good @kustrun But, did you run test to see if all test cases pass though?

[kustrun]

Yes, the tests are passing (was having some troubles running them locally): #14997

[kustrun]

@emhane This one is the duplicate of merged #14997.