feat: make more block types generic #12812

klkvr · 2024-11-23T18:11:41Z

Preparation for abstraction of block readers

Changes in this PR:

Methods from Block struct are moved to a new extension trait BlockSealExt which is implemented for every Block trait implementations, allowing them to be called once we abstract the block type
BlockWriter AT is changed from Body to Block
BlockWithSenders and SealedBlockWithSenders are made generic over a single B: Block generic. SealedBlock is still generic over both. This is a bit tricky to simplify because some of the networking components which construct blocks only know separate header and body types when constructing SealedBlock, and converting those 2 generics to a single one would need an additional generic for those types. One of such components is BodiesDownloader which uses BodiesRequestFuture:

reth/crates/net/downloaders/src/bodies/request.rs

Line 40 in f61a764

pub(crate) struct BodiesRequestFuture<B: BodiesClient> {

This issue doesn't apply to other components because they usually receive full blocks

bin/reth/src/commands/debug_cmd/merkle.rs

crates/primitives/src/traits.rs

emhane · 2024-11-24T08:38:30Z

crates/cli/commands/src/init_state/without_evm.rs

@@ -33,7 +33,7 @@ pub fn setup_without_evm<Provider>(
 where
    Provider: StaticFileProviderFactory
        + StageCheckpointWriter
-        + BlockWriter<Body: reth_node_api::BlockBody>,
+        + BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>,


Suggested change

+ BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>,

+ BlockWriter<BlockHeader = reth_primitives::Header>,

BlockWriter doesn't have this AT

well seems like it should then. anywhere in there impl that a transaction is also used? we don't want to be using fully qualified syntax or yet another adapter there for no apparent reason. just use the AT Primitives: NodePrimitives for now. BlockPrimitives would make most sense.

emhane · 2024-11-24T08:38:41Z

crates/cli/commands/src/init_state/without_evm.rs

@@ -64,7 +64,8 @@ fn append_first_block<Provider>(
    total_difficulty: U256,
 ) -> Result<(), eyre::Error>
 where
-    Provider: BlockWriter<Body: reth_node_api::BlockBody> + StaticFileProviderFactory,
+    Provider: BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>


Suggested change

Provider: BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>

Provider: BlockWriter<BlockHeader = reth_primitives::Header>

emhane · 2024-11-24T08:42:13Z

crates/primitives/src/block.rs

@@ -204,72 +152,83 @@ impl<'a> arbitrary::Arbitrary<'a> for Block {

 /// Sealed block with senders recovered from transactions.
 #[derive(Debug, Clone, PartialEq, Eq, Default, Deref, DerefMut)]
-pub struct BlockWithSenders {
+pub struct BlockWithSenders<B: reth_primitives_traits::Block = Block> {


Suggested change

pub struct BlockWithSenders<B: reth_primitives_traits::Block = Block> {

pub struct BlockWithSenders<B = Block> {

it's always better to not constraint the generic in the type definition with a trait bound that implements a bunch of behaviour (ie trait methods), makes the code more flexible in testing

updated in f7c301d

emhane · 2024-11-24T08:46:21Z

crates/primitives/src/block.rs

    /// List of senders that match the transactions in the block
    pub senders: Vec<Address>,
 }

-impl BlockWithSenders {
+impl<B: reth_primitives_traits::Block> BlockWithSenders<B> {


BlockPrimitives would have been ideal here, then you have the transaction type accessible without fully qualified syntax and without making yet another adapter type
#12721 @mattsse

unsure what benefits #12721 would bring here because this type is supposed to wrap the Block

emhane · 2024-11-25T08:08:41Z

crates/static-file/static-file/src/segments/transactions.rs

@@ -44,7 +44,7 @@ where
                .ok_or(ProviderError::BlockBodyIndicesNotFound(block))?;

            let mut transactions_cursor = provider.tx_ref().cursor_read::<tables::Transactions<
-                <<Provider as StaticFileProviderFactory>::Primitives as NodePrimitives>::SignedTx,
+                <Provider::Primitives as NodePrimitives>::SignedTx,


same here, use TxTy<Provider>

emhane · 2024-11-25T08:11:03Z

crates/cli/commands/src/init_state/without_evm.rs

@@ -33,7 +33,7 @@ pub fn setup_without_evm<Provider>(
 where
    Provider: StaticFileProviderFactory
        + StageCheckpointWriter
-        + BlockWriter<Body: reth_node_api::BlockBody>,
+        + BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>,


well seems like it should then. anywhere in there impl that a transaction is also used? we don't want to be using fully qualified syntax or yet another adapter there for no apparent reason. just use the AT Primitives: NodePrimitives for now. BlockPrimitives would make most sense.

mattsse

cool, another big milestone

mattsse · 2024-11-25T10:39:20Z

crates/primitives-traits/src/block/mod.rs

+    /// Create new block instance.
+    fn new(header: Self::Header, body: Self::Body) -> Self;



this and split makes sense because we expect that block consists of those two things

mattsse · 2024-11-25T10:41:02Z

crates/primitives/src/block.rs

    /// List of senders that match the transactions in the block
    pub senders: Vec<Address>,
 }

-impl BlockWithSenders {
+impl<B: reth_primitives_traits::Block> BlockWithSenders<B> {


unsure what benefits #12721 would bring here because this type is supposed to wrap the Block

marking as completed

klkvr requested review from rakita, joshieDo, shekhirin, onbjerg, rkrasiuk, mattsse, Rjected, emhane, fgimenez and gakonst as code owners November 23, 2024 18:11

emhane requested changes Nov 24, 2024

View reviewed changes

bin/reth/src/commands/debug_cmd/merkle.rs Outdated Show resolved Hide resolved

emhane requested changes Nov 24, 2024

View reviewed changes

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

emhane requested changes Nov 24, 2024

View reviewed changes

klkvr added 7 commits November 25, 2024 04:01

feat: make more block types generic

5114fac

fixes

db15e9d

fix

9e8e1eb

use aliases

a587d19

relax bound

f7c301d

rename trait

d54846d

fix doc

94d8acd

klkvr force-pushed the klkvr/generic-blocks branch from 9a090bf to 94d8acd Compare November 25, 2024 00:01

klkvr mentioned this pull request Nov 25, 2024

feat: ChainStorageReader #12836

Merged

emhane previously requested changes Nov 25, 2024

View reviewed changes

mattsse approved these changes Nov 25, 2024

View reviewed changes

mattsse added C-enhancement New feature or request A-sdk Related to reth's use as a library labels Nov 25, 2024

mattsse added this pull request to the merge queue Nov 25, 2024

Merged via the queue into main with commit dcaa06a Nov 25, 2024
41 checks passed

mattsse deleted the klkvr/generic-blocks branch November 25, 2024 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make more block types generic #12812

feat: make more block types generic #12812

klkvr commented Nov 23, 2024 •

edited

Loading

emhane Nov 24, 2024

klkvr Nov 24, 2024

emhane Nov 25, 2024

emhane Nov 24, 2024

emhane Nov 24, 2024

klkvr Nov 25, 2024

emhane Nov 24, 2024

mattsse Nov 25, 2024

emhane Nov 25, 2024

emhane Nov 25, 2024

mattsse left a comment

mattsse Nov 25, 2024

mattsse Nov 25, 2024

	+ BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>,
	+ BlockWriter<BlockHeader = reth_primitives::Header>,

	Provider: BlockWriter<Block: reth_node_api::Block<Header = reth_primitives::Header>>
	Provider: BlockWriter<BlockHeader = reth_primitives::Header>

	pub struct BlockWithSenders<B: reth_primitives_traits::Block = Block> {
	pub struct BlockWithSenders<B = Block> {

		/// Create new block instance.
		fn new(header: Self::Header, body: Self::Body) -> Self;

feat: make more block types generic #12812

feat: make more block types generic #12812

Conversation

klkvr commented Nov 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

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

klkvr commented Nov 23, 2024 •

edited

Loading