refactor(storage): unify blocks insertion logic #12694

klkvr · 2024-11-20T00:25:52Z

Updates UnifiedStorageWriter to reuse code paths from BodyStage for blocks writing. This is done by extending BlockWriter methods with an additional write_transactions_to parameter:

reth/crates/storage/provider/src/traits/block.rs

Lines 67 to 71 in b8980e5

    
           fn insert_block( 
        
               &self, 
        
               block: SealedBlockWithSenders<Header, Self::Body>, 
        
               write_transactions_to: StorageLocation, 
        
           ) -> ProviderResult<StoredBlockBodyIndices>;

reth/crates/storage/provider/src/traits/block.rs

Lines 10 to 19 in b8980e5

    
           /// An enum that represents the storage location for a piece of data. 
        
           #[derive(Debug, Copy, Clone, PartialEq, Eq)] 
        
           pub enum StorageLocation { 
        
               /// Write only to static files. 
        
               StaticFiles, 
        
               /// Write only to the database. 
        
               Database, 
        
               /// Write to both the database and static files. 
        
               Both, 
        
           }

StorageLocation::Database is a default for insert_block and only writes transactions to database, used by legacy engine and components which need to operate on database without ever commiting anything (like debugging CLI commands)
StorageLocation::Both writes to both database and staticfiles, used by new engine right now.
StorageLocation::StaticFiles only writes transactions to staticfiles, used by BodyStage. Should be used by new engine as well eventually.

Same can be done with headers and receipts as well

This required a drive-by change of making SealedBlockWithSenders generic over header and body like SealedBlock

mattsse

need @joshieDo input here

all of this makes sense

his required a drive-by change of making SealedBlockWithSenders generic over header and body like SealedBlock

this is fine for me in this pr, but would have preferred a separate one for this.

one question about elapsed insert tracking

mattsse · 2024-11-20T16:58:48Z

crates/storage/provider/src/providers/database/metrics.rs

why do we no longer need these metrics?

EDIT: ah because we now operate on block bodies

makes sense

mattsse · 2024-11-20T17:03:32Z

crates/storage/provider/src/providers/database/provider.rs

+            // write transactions
+            for transaction in body.transactions() {
+                let hash = transaction.tx_hash();
+                let start = Instant::now();


this is way too expensive to do per tx

was this the case before?

I'd rather not do this at all

this was the case for live sync but not for the pipeline, should I remove it or just make optional?

I'd prefer removing this entirely, doesn't seem very useful, just needlessly expensive

wdyt @joshieDo

don't want to block this pr for this,
but would like to get rid of this detailed time tracking entirely because these additional syscall per tx are likely expensive (400 timesyscalls for a 200txs block)

removed the per-tx metrics

mattsse · 2024-11-20T17:04:31Z

crates/storage/provider/src/traits/block.rs

this makes sense

mattsse · 2024-11-20T17:05:56Z

crates/storage/provider/src/traits/block.rs

+    /// Write to both the database and static files.
+    Both,


does this mean data is duplicated?

not obvious where we need this

right now we are using insert_block which does this by default and on reorgs we use take_block_transactions_range which relies on all transactions being present in database

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

Line 880 in b8980e5

let transactions = self

to get rid of this we'd need a separate fn for reorgs in new engine which I am planning to do in a follow-up

mattsse · 2024-11-20T18:58:17Z

crates/storage/provider/src/providers/database/provider.rs

+            // write transactions
+            for transaction in body.transactions() {
+                let hash = transaction.tx_hash();
+                let start = Instant::now();


don't want to block this pr for this,
but would like to get rid of this detailed time tracking entirely because these additional syscall per tx are likely expensive (400 timesyscalls for a 200txs block)

klkvr · 2024-11-20T19:06:46Z

pending @joshieDo

joshieDo · 2024-11-20T22:28:15Z

crates/storage/db-api/src/models/mod.rs

@@ -225,6 +227,7 @@ impl_compression_for_compact!(
    Bytecode,
    AccountBeforeTx,
    TransactionSignedNoHash,
+    TransactionSigned,


why ? this should not be written to storage

joshieDo · 2024-11-20T22:30:30Z

crates/storage/provider/src/providers/database/provider.rs

+            .database()
+            .then(|| {
+                self.tx.cursor_write::<tables::Transactions<
+                    <Self::Body as reth_primitives_traits::BlockBody>::Transaction,


are we sure we are writing TransactionSignedNoHash and not TransactionSigned here?

TransactionSigned Compact impl delegates to TransactionSignedNoHash

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

Lines 1053 to 1067 in e3702cf

#[cfg(any(test, feature = "reth-codec"))]

impl reth_codecs::Compact for TransactionSigned {

fn to_compact<B>(&self, buf: &mut B) -> usize

where

B: bytes::BufMut + AsMut<[u8]>,

{

let tx: TransactionSignedNoHash = self.clone().into();

tx.to_compact(buf)

}

fn from_compact(buf: &[u8], len: usize) -> (Self, &[u8]) {

let (tx, buf) = TransactionSignedNoHash::from_compact(buf, len);

(tx.into(), buf)

}

}

those should be unified soon with #12596

need to make sure this happens before we get to readers abstraction though so that we don't do hashing on every storage read

oh i see we introduced it here: #12539

klkvr changed the title ~~[wip] refactor: unify blocks insertion logic~~ refactor: unify blocks insertion logic Nov 20, 2024

klkvr marked this pull request as ready for review November 20, 2024 15:55

klkvr requested review from rakita, joshieDo, shekhirin, onbjerg, rkrasiuk, Rjected, emhane, mattsse, fgimenez and gakonst as code owners November 20, 2024 15:55

klkvr changed the title ~~refactor: unify blocks insertion logic~~ refactor(storage): unify blocks insertion logic Nov 20, 2024

klkvr added C-debt Refactor of code section that is hard to understand or maintain A-db Related to the database A-sdk Related to reth's use as a library labels Nov 20, 2024

mattsse reviewed Nov 20, 2024

View reviewed changes

chore: add header and body generics to SealedBlockWithSenders

b245531

klkvr mentioned this pull request Nov 20, 2024

chore: add header and body generics to SealedBlockWithSenders #12717

Merged

[wip] refactor: unify blocks insertion logic

f1725c3

klkvr force-pushed the klkvr/unify-storage branch from b8980e5 to f1725c3 Compare November 20, 2024 17:28

klkvr changed the base branch from main to klkvr/block-with-senders November 20, 2024 17:28

mattsse approved these changes Nov 20, 2024

View reviewed changes

klkvr added 2 commits November 20, 2024 23:04

remove metrics

53a1fa1

remove metrics

fee5e6d

Base automatically changed from klkvr/block-with-senders to main November 20, 2024 19:12

joshieDo reviewed Nov 20, 2024

View reviewed changes

joshieDo approved these changes Nov 20, 2024

View reviewed changes

klkvr added this pull request to the merge queue Nov 20, 2024

klkvr mentioned this pull request Nov 20, 2024

chore(sdk): define helper trait BlockPrimitives #12721

Open

Merged via the queue into main with commit 0c59841 Nov 20, 2024
34 checks passed

klkvr deleted the klkvr/unify-storage branch November 20, 2024 23:12

klkvr mentioned this pull request Nov 21, 2024

refactor: unify logic for blocks removal #12743

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(storage): unify blocks insertion logic #12694

refactor(storage): unify blocks insertion logic #12694

klkvr commented Nov 20, 2024 •

edited

Loading

mattsse left a comment

mattsse Nov 20, 2024

mattsse Nov 20, 2024

klkvr Nov 20, 2024

mattsse Nov 20, 2024

mattsse Nov 20, 2024

klkvr Nov 20, 2024

mattsse Nov 20, 2024

mattsse Nov 20, 2024

klkvr Nov 20, 2024

mattsse Nov 20, 2024

klkvr commented Nov 20, 2024

joshieDo Nov 20, 2024

joshieDo Nov 20, 2024

klkvr Nov 20, 2024

klkvr Nov 20, 2024 •

edited

Loading

joshieDo Nov 20, 2024

	fn insert_block(
	&self,
	block: SealedBlockWithSenders<Header, Self::Body>,
	write_transactions_to: StorageLocation,
	) -> ProviderResult<StoredBlockBodyIndices>;

	/// An enum that represents the storage location for a piece of data.
	#[derive(Debug, Copy, Clone, PartialEq, Eq)]
	pub enum StorageLocation {
	/// Write only to static files.
	StaticFiles,
	/// Write only to the database.
	Database,
	/// Write to both the database and static files.
	Both,
	}

	#[cfg(any(test, feature = "reth-codec"))]
	impl reth_codecs::Compact for TransactionSigned {
	fn to_compact<B>(&self, buf: &mut B) -> usize
	where
	B: bytes::BufMut + AsMut<[u8]>,
	{
	let tx: TransactionSignedNoHash = self.clone().into();
	tx.to_compact(buf)
	}

	fn from_compact(buf: &[u8], len: usize) -> (Self, &[u8]) {
	let (tx, buf) = TransactionSignedNoHash::from_compact(buf, len);
	(tx.into(), buf)
	}
	}

refactor(storage): unify blocks insertion logic #12694

refactor(storage): unify blocks insertion logic #12694

Conversation

klkvr commented Nov 20, 2024 • edited Loading

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

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

klkvr commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr commented Nov 20, 2024 •

edited

Loading

klkvr Nov 20, 2024 •

edited

Loading