feat(storage): better sender recovery if not found in database (#4471)

Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>

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<RangeInclusive<TxNumber>> = 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::<tables::TxSenders, true>(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::<tables::TxSenders, true>(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::<true>(0..=0);
             assert_eq!(

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

@@ -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,63 @@ impl<'this, TX: DbTxMut<'this> + DbTx<'this>> DatabaseProvider<'this, TX> {
             self.get_or_take::<tables::TxSenders, 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::<Vec<_>>(),
-                    missing_senders,
-                )
-                .ok_or(BlockExecutionError::Validation(
-                    BlockValidationError::SenderRecoveryError,
-                ))?,
+        // 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());
+            {
+                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 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));
+                    }
+                }
+            }
+
+            // Recover senders
+            let recovered_senders = TransactionSigned::recover_signers(
+                missing_senders.iter().map(|(_, _, tx)| *tx).collect::<Vec<_>>(),
+                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))?;
+
+            // 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));
+            }
+
+            // 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"
+            );
+        }
 
         if TAKE {
             // Remove TxHashNumber

crates/transaction-pool/src/pool/best.rs

@@ -70,7 +70,7 @@ pub(crate) struct BestTransactions<T: TransactionOrdering> {
     pub(crate) independent: BTreeSet<PendingTransaction<T>>,
     /// There might be the case where a yielded transactions is invalid, this will track it.
     pub(crate) invalid: HashSet<TxHash>,
-    /// 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