bug: Correct nonce validation (#409)

* feat: Correct nonce validation

* clenup

* Update crates/consensus/src/verification.rs

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

crates/consensus/src/verification.rs

@@ -2,11 +2,14 @@
 use crate::{config, Config};
 use reth_interfaces::{consensus::Error, Result as RethResult};
 use reth_primitives::{
-    Account, BlockLocked, BlockNumber, SealedHeader, Transaction, TxEip1559, TxEip2930, TxLegacy,
-    EMPTY_OMMER_ROOT, H256, U256,
+    BlockLocked, BlockNumber, Header, SealedHeader, Transaction, TransactionSignedEcRecovered,
+    TxEip1559, TxEip2930, TxLegacy, EMPTY_OMMER_ROOT, H256, U256,
 };
 use reth_provider::{AccountProvider, HeaderProvider};
-use std::time::SystemTime;
+use std::{
+    collections::{hash_map::Entry, HashMap},
+    time::SystemTime,
+};
 
 /// Validate header standalone
 pub fn validate_header_standalone(
@@ -122,41 +125,52 @@ pub fn validate_transaction_regarding_header(
     Ok(())
 }
 
-/// Validate transaction in regards of State
-pub fn validate_transaction_regarding_account(
-    transaction: &Transaction,
-    account: &Account,
-) -> reth_interfaces::Result<()> {
-    // Signer account shoudn't have bytecode. Presence of bytecode means this is a smartcontract.
-    if account.has_bytecode() {
-        return Err(Error::SignerAccountHasBytecode.into())
-    }
+/// Iterate over all transactions, verify them agains each other and against the block.
+/// There is no gas check done as [REVM](https://github.com/bluealloy/revm/blob/fd0108381799662098b7ab2c429ea719d6dfbf28/crates/revm/src/evm_impl.rs#L113-L131) already checks that.
+pub fn validate_all_transaction_regarding_block_and_nonces<
+    'a,
+    Provider: HeaderProvider + AccountProvider,
+>(
+    transactions: impl Iterator<Item = &'a TransactionSignedEcRecovered>,
+    header: &Header,
+    provider: Provider,
+    config: &Config,
+) -> RethResult<()> {
+    let mut account_nonces = HashMap::new();
 
-    let (nonce, gas_price, gas_limit, value) = match transaction {
-        Transaction::Legacy(TxLegacy { nonce, gas_price, gas_limit, value, .. }) => {
-            (nonce, gas_price, gas_limit, value)
+    for transaction in transactions {
+        validate_transaction_regarding_header(
+            transaction,
+            config,
+            header.number,
+            header.base_fee_per_gas,
+        )?;
+
+        // Get nonce, if there is previous transaction from same sender we need
+        // to take that nonce.
+        let nonce = match account_nonces.entry(transaction.signer()) {
+            Entry::Occupied(mut entry) => {
+                let nonce = *entry.get();
+                *entry.get_mut() += 1;
+                nonce
+            }
+            Entry::Vacant(entry) => {
+                let account = provider.basic_account(transaction.signer())?.unwrap_or_default();
+                // Signer account shoudn't have bytecode. Presence of bytecode means this is a
+                // smartcontract.
+                if account.has_bytecode() {
+                    return Err(Error::SignerAccountHasBytecode.into())
+                }
+                let nonce = account.nonce;
+                entry.insert(account.nonce + 1);
+                nonce
+            }
+        };
+
+        // check nonce
+        if transaction.nonce() != nonce {
+            return Err(Error::TransactionNonceNotConsistent.into())
         }
-        Transaction::Eip2930(TxEip2930 { nonce, gas_price, gas_limit, value, .. }) => {
-            (nonce, gas_price, gas_limit, value)
-        }
-        Transaction::Eip1559(TxEip1559 { nonce, gas_limit, max_fee_per_gas, value, .. }) => {
-            (nonce, max_fee_per_gas, gas_limit, value)
-        }
-    };
-
-    // check nonce
-    if account.nonce + 1 != *nonce {
-        return Err(Error::TransactionNonceNotConsistent.into())
-    }
-
-    // max fee that transaction can potentially spend
-    let max_fee = (*gas_limit as u128).saturating_mul(*gas_price).saturating_add(*value);
-
-    // check if account balance has enought to cover worst case
-    if max_fee > account.balance.as_u128() {
-        return Err(
-            Error::InsufficientFunds { max_fee, available_funds: account.balance.as_u128() }.into()
-        )
     }
 
     Ok(())
@@ -329,30 +343,29 @@ pub fn full_validation<Provider: HeaderProvider + AccountProvider>(
     let parent = validate_block_regarding_chain(block, &provider)?;
     validate_header_regarding_parent(&parent, &block.header, config)?;
 
-    for transaction in block.body.iter() {
-        validate_transaction_regarding_header(
-            transaction,
-            config,
-            block.header.number,
-            block.header.base_fee_per_gas,
-        )?;
+    // NOTE: depending on the need of the stages, recovery could be done in different place.
+    let transactions = block
+        .body
+        .iter()
+        .map(|tx| tx.try_ecrecovered().ok_or(Error::TransactionSignerRecoveryError))
+        .collect::<Result<Vec<_>, _>>()?;
 
-        // NOTE: depending on the need of the stages, recovery could be done in different place.
-        let recovered =
-            transaction.try_ecrecovered().ok_or(Error::TransactionSignerRecoveryError)?;
-
-        let account =
-            provider.basic_account(recovered.signer())?.ok_or(Error::SignerAccountNotExisting)?;
-
-        validate_transaction_regarding_account(transaction, &account)?;
-    }
+    validate_all_transaction_regarding_block_and_nonces(
+        transactions.iter(),
+        &block.header,
+        provider,
+        config,
+    )?;
     Ok(())
 }
 
 #[cfg(test)]
 mod tests {
     use reth_interfaces::Result;
-    use reth_primitives::{hex_literal::hex, Address, BlockHash, Header};
+    use reth_primitives::{
+        hex_literal::hex, Account, Address, BlockHash, Bytes, Header, Signature, TransactionKind,
+        TransactionSigned,
+    };
 
     use super::*;
 
@@ -419,6 +432,25 @@ mod tests {
             Ok(self.parent.clone())
         }
     }
+
+    fn mock_tx(nonce: u64) -> TransactionSignedEcRecovered {
+        let request = Transaction::Eip2930(TxEip2930 {
+            chain_id: 1u64,
+            nonce,
+            gas_price: 0x28f000fff,
+            gas_limit: 10,
+            to: TransactionKind::Call(Address::default()),
+            value: 3,
+            input: Bytes::from(vec![1, 2]),
+            access_list: Default::default(),
+        });
+
+        let signature = Signature { odd_y_parity: true, r: U256::default(), s: U256::default() };
+
+        let tx = TransactionSigned::from_transaction_and_signature(request, signature);
+        let signer = Address::zero();
+        TransactionSignedEcRecovered::from_signed_transaction(tx, signer)
+    }
     /// got test block
     fn mock_block() -> (BlockLocked, Header) {
         // https://etherscan.io/block/15867168 where transaction root and receipts root are cleared
@@ -477,4 +509,61 @@ mod tests {
             "Should fail with error"
         );
     }
+
+    #[test]
+    fn sanity_tx_nonce_check() {
+        let (block, _) = mock_block();
+        let tx1 = mock_tx(0);
+        let tx2 = mock_tx(1);
+        let provider = Provider::new_known();
+        let config = Config::default();
+
+        let txs = vec![tx1, tx2];
+        validate_all_transaction_regarding_block_and_nonces(
+            txs.iter(),
+            &block.header,
+            provider,
+            &config,
+        )
+        .expect("To Pass");
+    }
+
+    #[test]
+    fn nonce_gap_in_first_transaction() {
+        let (block, _) = mock_block();
+        let tx1 = mock_tx(1);
+        let provider = Provider::new_known();
+        let config = Config::default();
+
+        let txs = vec![tx1];
+        assert_eq!(
+            validate_all_transaction_regarding_block_and_nonces(
+                txs.iter(),
+                &block.header,
+                provider,
+                &config,
+            ),
+            Err(Error::TransactionNonceNotConsistent.into())
+        )
+    }
+
+    #[test]
+    fn nonce_gap_on_second_tx_from_same_signer() {
+        let (block, _) = mock_block();
+        let tx1 = mock_tx(0);
+        let tx2 = mock_tx(3);
+        let provider = Provider::new_known();
+        let config = Config::default();
+
+        let txs = vec![tx1, tx2];
+        assert_eq!(
+            validate_all_transaction_regarding_block_and_nonces(
+                txs.iter(),
+                &block.header,
+                provider,
+                &config,
+            ),
+            Err(Error::TransactionNonceNotConsistent.into())
+        );
+    }
 }

crates/primitives/src/transaction/mod.rs

@@ -788,20 +788,13 @@ impl TransactionSignedEcRecovered {
         self.signer
     }
 
-    /// Create [TransactionSignedEcRecovered] from [TransactionSigned] and author of transaction
-    pub fn from_signed_transactions_and_signer(
-        signed_transaction: TransactionSigned,
-        signer: Address,
-    ) -> Self {
-        Self { signed_transaction, signer }
-    }
-
     /// Transform back to [`TransactionSigned`]
     pub fn into_signed(self) -> TransactionSigned {
         self.signed_transaction
     }
 
-    /// Create [`TransactionSignedEcRecovered`] from [`TransactionSigned`] and [`Address`].
+    /// Create [`TransactionSignedEcRecovered`] from [`TransactionSigned`] and [`Address`] of the
+    /// signer.
     pub fn from_signed_transaction(signed_transaction: TransactionSigned, signer: Address) -> Self {
         Self { signed_transaction, signer }
     }

crates/stages/src/stages/execution.rs

@@ -209,7 +209,7 @@ impl<DB: Database> Stage<DB> for ExecutionStage {
                 .into_iter()
                 .zip(signers.into_iter())
                 .map(|(tx, signer)| {
-                    TransactionSignedEcRecovered::from_signed_transactions_and_signer(tx, signer)
+                    TransactionSignedEcRecovered::from_signed_transaction(tx, signer)
                 })
                 .collect();