chore(sdk): define helper trait BlockPrimitives

[emhane]

Based on #12647

• Defines helper trait BlockPrimitives, which links NodePrimitives::Block to NodePrimitives::BlockHeader, NodePrimitives::BlockBody and NodePrimitives::SignedTransaction. This makes the trait bounds necessary on impl bodies much less verbose!
• Makes NodePrimitives super trait of FullNodePrimitives, as it allows for the transition FullNodePrimitives -> BlockPrimitives -> NodePrimitives.
  NOTE: the reason I didn't do this in the first place is because I copied the pattern of FullNodeComponents -> RpcNodeCore used to simplify type definitions, introduced because FullNodeComponents wasn't built from a base trait without trait bounds (and re-writing that would have been a too big change for the task). Here we have the opportunity to have a plain (trait unbound) super trait though since we're writing primitives from scratch.

[klkvr]

• Fixes bug in stages. Bug: writing compact encoded signed transaction instead of the unsigned transaction to db

I believe we've been always writing signed transaction to both static files and database?

reth/crates/storage/provider/src/providers/static_file/writer.rs

Lines 543 to 547 in 166a234

	pub fn append_transaction(
	&mut self,
	tx_num: TxNumber,
	tx: &TransactionSignedNoHash,
	) -> ProviderResult<TxNumber> {

reth/crates/storage/db/src/tables/mod.rs

Line 295 in 166a234

table Transactions<Key = TxNumber, Value = TransactionSignedNoHash>;

[emhane]

• Fixes bug in stages. Bug: writing compact encoded signed transaction instead of the unsigned transaction to db

I believe we've been always writing signed transaction to both static files and database?

reth/crates/storage/provider/src/providers/static_file/writer.rs

Lines 543 to 547 in 166a234

	pub fn append_transaction(
	&mut self,
	tx_num: TxNumber,
	tx: &TransactionSignedNoHash,
	) -> ProviderResult<TxNumber> {

reth/crates/storage/db/src/tables/mod.rs

Line 295 in 166a234

table Transactions<Key = TxNumber, Value = TransactionSignedNoHash>;

TransactionSigned doesn't impl Compact though

[klkvr]

It does since #12539

related discussion #12694 (comment)

[emhane]

then it should impl Compact, but it doesn't. before StaticFileProviderRW::append_transaction took TransactionSignedNoHash. @klkvr

[klkvr]

then it should impl Compact, but it doesn't

It does impl Compact

reth/crates/primitives/src/transaction/mod.rs

Line 1054 in e3702cf

impl reth_codecs::Compact for TransactionSigned {

before StaticFileProviderRW::append_transaction took TransactionSignedNoHash

yep, but now it takes anything that implements Compact (should probably change that to N::SignedTx given that we have primitives on StaticFileProvider now)

sorry for the confusion with the 2 storage types, I was expecting the TransactionSignedNoHash to be gone by now

[emhane]

ok nvm, switching between a lot of branches to shrink scope, my bad

[emhane]

re-request review @klkvr

[klkvr]

unsure I understand why we need a separate trait for this? those bounds already exist for FullNodePrimitives, and I believe we could also move them to NodePrimitives?

[emhane]

for the same reason we don't have the compact feature reth-codec always enabled, cc @joshieDo . it makes testing more straight forward and it makes it more apparent to us how data flows in reth.

if you want to test for example consensus engine, you should just need an impl like

impl NodePrimitives for TestPrimitives {
    type Block = Block;
    type BlockHeader = Header;
    type BlockBody = BlockBody;
    type SignedTx = TransactionSigned;
    type TxType = ();
    type Receipt = ();
}

it's always misleading a weird for someone new to the code, to try and figure out how the code works by looking at the test, and then realise half way through that types are in scope, that are not used.

[emhane]

nice side effect of having NodePrimitives as super trait of BlockPrimitives, is that in case it's needed, a trait impl using BlockPrimitives, can still return a receipt type, just not use the receipt functionality in its scope

[klkvr]

ProviderFactory won't work unless primitives are FullNodePrimitives so I don't think you will be able to test anything with such primitives implementation

[klkvr]

I think it might make sense to move those bounds to NodePrimitives but separate trait doesn't seem very useful

[emhane]

provider is expected to use FullNodePrimitives in some places. node builder is expected to use FullNodePrimitives everywhere

[mattsse]

unclear what the benefit here is
what does this unblock?

[emhane]

it links the block type to the header, body and signed tx types. it should be used in places where FullNodePrimitives is not needed, and simplifies having to specify the super trait of BlockPrimitives everywhere, i.e. N: NodePrimitives<Block: Block<Header = N::BlockHeader, ..>, ..> to link the blockparts and tx to the block type

[mattsse]

this was removed because this AT, didn't make much sense that a transaction defines a transaction

[emhane]

since we won't have access to a base trait without trait bounds on associated types since #12846, there is no need for this pr