From 991d8ef59d7510eac85d62d95332ce5e284cfbe2 Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Mon, 4 Sep 2023 12:33:56 +0100 Subject: [PATCH 1/7] test(storage): recover senders if not found in database --- crates/storage/provider/Cargo.toml | 1 + .../provider/src/providers/database/mod.rs | 31 +++++++++++++++++++ .../src/providers/database/provider.rs | 2 +- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/crates/storage/provider/Cargo.toml b/crates/storage/provider/Cargo.toml index bcff000a09d3..45382391b567 100644 --- a/crates/storage/provider/Cargo.toml +++ b/crates/storage/provider/Cargo.toml @@ -38,6 +38,7 @@ reth-db = { path = "../db", features = ["test-utils"] } reth-primitives = { workspace = true, features = ["arbitrary", "test-utils"] } reth-rlp.workspace = true reth-trie = { path = "../../trie", features = ["test-utils"] } +reth-interfaces = { workspace = true, features = ["test-utils"] } parking_lot.workspace = true tempfile = "3.3" assert_matches.workspace = true diff --git a/crates/storage/provider/src/providers/database/mod.rs b/crates/storage/provider/src/providers/database/mod.rs index 45529c9f0490..4273964c136e 100644 --- a/crates/storage/provider/src/providers/database/mod.rs +++ b/crates/storage/provider/src/providers/database/mod.rs @@ -396,9 +396,11 @@ mod tests { use crate::{BlockHashReader, BlockNumReader, BlockWriter, TransactionsProvider}; use assert_matches::assert_matches; use reth_db::{ + tables, test_utils::{create_test_rw_db, ERROR_TEMPDIR}, DatabaseEnv, }; + use reth_interfaces::test_utils::{generators, generators::random_block}; use reth_primitives::{ hex_literal::hex, ChainSpecBuilder, PruneMode, PruneModes, SealedBlock, H256, }; @@ -485,4 +487,33 @@ mod tests { assert_matches!(provider.transaction_id(block.body[0].hash), Ok(None)); } } + + #[test] + fn get_take_block_transaction_range_recover_senders() { + let chain_spec = ChainSpecBuilder::mainnet().build(); + let db = create_test_rw_db(); + let factory = ProviderFactory::new(db, Arc::new(chain_spec)); + + let mut rng = generators::rng(); + let block = random_block(&mut rng, 0, None, Some(3), None); + + { + let provider = factory.provider_rw().unwrap(); + + assert_matches!(provider.insert_block(block.clone(), None, None), Ok(_)); + + let senders = provider.get_or_take::(0..=0); + assert_eq!(senders, Ok(vec![(0, block.body[0].recover_signer().unwrap())])); + assert_eq!(provider.transaction_sender(0), Ok(None)); + + let result = provider.get_take_block_transaction_range::(0..=0); + assert_eq!( + result, + Ok(vec![( + 0, + block.body.iter().cloned().map(|tx| tx.into_ecrecovered().unwrap()).collect() + )]) + ) + } + } } diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 9b30cbafbb72..ae8d87cabcdc 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -404,7 +404,7 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { } /// Get requested blocks transaction with signer - fn get_take_block_transaction_range( + pub(crate) fn get_take_block_transaction_range( &self, range: impl RangeBounds + Clone, ) -> Result)>> { From dbabee26fefeb355765a860f4b0274d00b212e88 Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Mon, 4 Sep 2023 13:12:23 +0100 Subject: [PATCH 2/7] feat(storage): better sender recovery if not found in database --- .../provider/src/providers/database/mod.rs | 24 ++++++--- .../src/providers/database/provider.rs | 50 +++++++++++-------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/crates/storage/provider/src/providers/database/mod.rs b/crates/storage/provider/src/providers/database/mod.rs index 4273964c136e..e9da00b85059 100644 --- a/crates/storage/provider/src/providers/database/mod.rs +++ b/crates/storage/provider/src/providers/database/mod.rs @@ -402,10 +402,10 @@ mod tests { }; use reth_interfaces::test_utils::{generators, generators::random_block}; use reth_primitives::{ - hex_literal::hex, ChainSpecBuilder, PruneMode, PruneModes, SealedBlock, H256, + hex_literal::hex, ChainSpecBuilder, PruneMode, PruneModes, SealedBlock, TxNumber, H256, }; use reth_rlp::Decodable; - use std::sync::Arc; + use std::{ops::RangeInclusive, sync::Arc}; #[test] fn common_history_provider() { @@ -497,14 +497,26 @@ mod tests { let mut rng = generators::rng(); let block = random_block(&mut rng, 0, None, Some(3), None); - { + let tx_ranges: Vec> = vec![0..=0, 1..=1, 2..=2, 0..=1, 1..=2]; + for range in tx_ranges { let provider = factory.provider_rw().unwrap(); assert_matches!(provider.insert_block(block.clone(), None, None), Ok(_)); - let senders = provider.get_or_take::(0..=0); - assert_eq!(senders, Ok(vec![(0, block.body[0].recover_signer().unwrap())])); - assert_eq!(provider.transaction_sender(0), Ok(None)); + let senders = provider.get_or_take::(range.clone()); + assert_eq!( + senders, + Ok(range + .clone() + .map(|tx_number| ( + tx_number, + block.body[tx_number as usize].recover_signer().unwrap() + )) + .collect()) + ); + + let db_senders = provider.senders_by_tx_range(range); + assert_eq!(db_senders, Ok(vec![])); let result = provider.get_take_block_transaction_range::(0..=0); assert_eq!( diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index ae8d87cabcdc..7dc8e519578d 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -8,7 +8,7 @@ use crate::{ PruneCheckpointReader, PruneCheckpointWriter, StageCheckpointReader, StorageReader, TransactionsProvider, WithdrawalsProvider, }; -use itertools::{izip, Itertools}; +use itertools::{izip, Itertools, PeekingNext}; use reth_db::{ common::KeyValue, cursor::{DbCursorRO, DbCursorRW, DbDupCursorRO}, @@ -34,7 +34,7 @@ use reth_primitives::{ ChainInfo, ChainSpec, Hardfork, Head, Header, PruneCheckpoint, PruneModes, PrunePart, Receipt, SealedBlock, SealedBlockWithSenders, SealedHeader, StorageEntry, TransactionMeta, TransactionSigned, TransactionSignedEcRecovered, TransactionSignedNoHash, TxHash, TxNumber, - Withdrawal, H160, H256, U256, + Withdrawal, H256, U256, }; use reth_revm_primitives::{ config::revm_spec, @@ -435,26 +435,34 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { self.get_or_take::(first_transaction..=last_transaction)?; // Recover senders manually if not found in db - let senders_len = senders.len(); - let transactions_len = transactions.len(); - let missing_senders = transactions_len - senders_len; - let mut senders_recovered: Vec<(u64, H160)> = (first_transaction.. - first_transaction + missing_senders as u64) - .zip( - TransactionSigned::recover_signers( - transactions.iter().take(missing_senders).map(|(_, tx)| tx).collect::>(), - missing_senders, - ) - .ok_or(BlockExecutionError::Validation( - BlockValidationError::SenderRecoveryError, - ))?, + if senders.len() != transactions.len() { + let mut missing_senders = Vec::with_capacity(transactions.len() - senders.len()); + { + let mut senders = senders.iter().peekable(); + + for (i, (tx_number, transaction)) in transactions.iter().enumerate() { + if senders.peeking_next(|(key, _)| key == tx_number).is_none() { + missing_senders.push((i, tx_number, transaction)); + } + } + } + + let recovered_senders = TransactionSigned::recover_signers( + missing_senders.iter().map(|(_, _, tx)| *tx).collect::>(), + missing_senders.len(), ) - .collect(); - // It's only possible to have missing senders at the beginning of the range, and not in the - // middle or in the end, so it's safe to do `senders_recovered.extend(senders.iter())` - senders_recovered.extend(senders.iter()); - senders = senders_recovered; - debug_assert_eq!(senders.len(), transactions_len, "missing one or more senders"); + .ok_or(BlockExecutionError::Validation(BlockValidationError::SenderRecoveryError))?; + + for ((i, tx_number, _), sender) in missing_senders.into_iter().zip(recovered_senders) { + senders.insert(i, (*tx_number, sender)); + } + + debug_assert!( + senders.iter().tuple_windows().all(|(a, b)| a.0 < b.0), + "senders not sorted" + ); + debug_assert_eq!(senders.len(), transactions.len(), "missing one or more senders"); + } if TAKE { // Remove TxHashNumber From 06d48ff60955a042e5223cf7067d947a6fee73bd Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Mon, 4 Sep 2023 13:15:12 +0100 Subject: [PATCH 3/7] add comments --- .../storage/provider/src/providers/database/provider.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 7dc8e519578d..56a5b6513dd1 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -436,6 +436,8 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { // Recover senders manually if not found in db if senders.len() != transactions.len() { + // Find all missing senders, their corresponding tx numbers and indexes to the original + // `senders` vector at which the recovered senders will be inserted let mut missing_senders = Vec::with_capacity(transactions.len() - senders.len()); { let mut senders = senders.iter().peekable(); @@ -447,21 +449,26 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { } } + // Recover senders let recovered_senders = TransactionSigned::recover_signers( missing_senders.iter().map(|(_, _, tx)| *tx).collect::>(), missing_senders.len(), ) .ok_or(BlockExecutionError::Validation(BlockValidationError::SenderRecoveryError))?; + // Insert recovered senders along with tx numbers at the corresponding indexes to the + // original `senders` vector for ((i, tx_number, _), sender) in missing_senders.into_iter().zip(recovered_senders) { senders.insert(i, (*tx_number, sender)); } + // Debug assertions which are triggered during the test to ensure that all senders are + // present and sorted + debug_assert_eq!(senders.len(), transactions.len(), "missing one or more senders"); debug_assert!( senders.iter().tuple_windows().all(|(a, b)| a.0 < b.0), "senders not sorted" ); - debug_assert_eq!(senders.len(), transactions.len(), "missing one or more senders"); } if TAKE { From 60edc6c058f43aaf85a22a061bfb7fc6e5d1bdd3 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Sun, 3 Sep 2023 06:40:26 -0700 Subject: [PATCH 4/7] chore: fix deps sanity check (#4462) --- crates/consensus/beacon/src/engine/mod.rs | 4 +--- crates/transaction-pool/src/pool/best.rs | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 5b5bd6413a44..5a8279cf547d 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -1888,9 +1888,7 @@ mod tests { BeaconForkChoiceUpdateError, }; use assert_matches::assert_matches; - use reth_primitives::{ - stage::StageCheckpoint, ChainSpec, ChainSpecBuilder, PruneModes, H256, MAINNET, - }; + use reth_primitives::{stage::StageCheckpoint, ChainSpec, ChainSpecBuilder, H256, MAINNET}; use reth_provider::{BlockWriter, ProviderFactory}; use reth_rpc_types::engine::{ ExecutionPayloadV1, ForkchoiceState, ForkchoiceUpdated, PayloadStatus, diff --git a/crates/transaction-pool/src/pool/best.rs b/crates/transaction-pool/src/pool/best.rs index 5fc5ebc93137..6d87b7fe3220 100644 --- a/crates/transaction-pool/src/pool/best.rs +++ b/crates/transaction-pool/src/pool/best.rs @@ -66,7 +66,7 @@ pub(crate) struct BestTransactions { pub(crate) independent: BTreeSet>, /// There might be the case where a yielded transactions is invalid, this will track it. pub(crate) invalid: HashSet, - /// Used to recieve any new pending transactions that have been added to the pool after this + /// Used to receive any new pending transactions that have been added to the pool after this /// iterator was snapshotted /// /// These new pending transactions are inserted into this iterator's pool before yielding the From acf50173a0efed1defe5913fd7e9460aad3e4694 Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Mon, 4 Sep 2023 16:37:12 +0100 Subject: [PATCH 5/7] frienship ended with peeking next --- .../src/providers/database/provider.rs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 56a5b6513dd1..2b26a40b9f12 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -8,7 +8,7 @@ use crate::{ PruneCheckpointReader, PruneCheckpointWriter, StageCheckpointReader, StorageReader, TransactionsProvider, WithdrawalsProvider, }; -use itertools::{izip, Itertools, PeekingNext}; +use itertools::{izip, Itertools}; use reth_db::{ common::KeyValue, cursor::{DbCursorRO, DbCursorRW, DbDupCursorRO}, @@ -442,8 +442,26 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { { let mut senders = senders.iter().peekable(); + // `transactions` contain all entries. `senders` contain _some_ of the senders for + // these transactions. Both are sorted and indexed by `TxNumber`. + // + // The general idea is to iterate on both `transactions` and `senders`, and advance + // the `senders` iteration only if it matches the current `transactions` entry's + // `TxNumber`. Otherwise, add the transaction to the list of missing senders. for (i, (tx_number, transaction)) in transactions.iter().enumerate() { - if senders.peeking_next(|(key, _)| key == tx_number).is_none() { + if let Some((sender_tx_number, _)) = senders.peek() { + if sender_tx_number == tx_number { + // If current sender's `TxNumber` matches current transaction's + // `TxNumber`, advance the senders iterator. + senders.next(); + } else { + // If current sender's `TxNumber` doesn't match current transaction's + // `TxNumber`, add it to missing senders. + missing_senders.push((i, tx_number, transaction)); + } + } else { + // If there's no more senders left, but we're still iterating over + // transactions, add them to missing senders missing_senders.push((i, tx_number, transaction)); } } From e1a85ac4709596375ed585265acbd6a8b63a68eb Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Tue, 5 Sep 2023 11:26:46 +0100 Subject: [PATCH 6/7] more comments --- crates/storage/provider/src/providers/database/provider.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 2b26a40b9f12..8c341e50b36d 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -435,9 +435,11 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { self.get_or_take::(first_transaction..=last_transaction)?; // Recover senders manually if not found in db + // SAFETY: Transactions are always guaranteed to be in the database whereas + // senders might be pruned. if senders.len() != transactions.len() { // Find all missing senders, their corresponding tx numbers and indexes to the original - // `senders` vector at which the recovered senders will be inserted + // `senders` vector at which the recovered senders will be inserted. let mut missing_senders = Vec::with_capacity(transactions.len() - senders.len()); { let mut senders = senders.iter().peekable(); @@ -477,6 +479,7 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { // Insert recovered senders along with tx numbers at the corresponding indexes to the // original `senders` vector for ((i, tx_number, _), sender) in missing_senders.into_iter().zip(recovered_senders) { + // Insert will put recovered senders at necessary positions and shift the rest senders.insert(i, (*tx_number, sender)); } From d317e844e2af83bc0fea5d203702789dac4b7693 Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Tue, 5 Sep 2023 13:53:24 +0100 Subject: [PATCH 7/7] reserve senders --- crates/storage/provider/src/providers/database/provider.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 8c341e50b36d..73e1ae74af53 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -438,6 +438,7 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> { // SAFETY: Transactions are always guaranteed to be in the database whereas // senders might be pruned. if senders.len() != transactions.len() { + senders.reserve(transactions.len() - senders.len()); // Find all missing senders, their corresponding tx numbers and indexes to the original // `senders` vector at which the recovered senders will be inserted. let mut missing_senders = Vec::with_capacity(transactions.len() - senders.len());