Race condition causing a tx to be added to SubPool::Queued instead of SubPool::Pending

[kien-rise]

Describe the bug

Look at this fn in crates/transaction-pool/src/lib.rs:

    async fn add_transaction(
        &self,
        origin: TransactionOrigin,
        transaction: Self::Transaction,
    ) -> PoolResult<TxHash> {
        let (_, tx) = self.validate(origin, transaction).await;
        let mut results = self.pool.add_transactions(origin, std::iter::once(tx));
        results.pop().expect("result length is the same as the input")
    }

Anything can happen between self.validate(...) and self.pool.add_transactions(...) call. One possible scenario is:

1. self.validate(...) is called, returns state_nonce = 42.
2. A block is mined, updating the account nonce to 48.
3. self.pool.add_transactions is called, use the old state_nonce (42).

This scenario causes the following the loop in crates/transaction-pool/src/pool/txpool.rs to break in the first iteration:

// Traverse all transactions of the sender and update existing transactions
for (id, tx) in descendants {
    let current_pool = tx.subpool;

    // If there's a nonce gap, we can shortcircuit
    if next_nonce != id.nonce {
        break // <<<<< BREAK HERE
    }
    ...

As a result, the incoming transaction is added to SubPool::Queued instead of SubPool::Pending, which is a pre-condition to cause #12286.

Steps to reproduce

Apply this change to crates/transaction-pool/src/lib.rs.

    async fn add_transaction(
        &self,
         origin: TransactionOrigin,
         transaction: Self::Transaction,
     ) -> PoolResult<TxHash> {
-        let (_, tx) = self.validate(origin, transaction).await;
-        let mut results = self.pool.add_transactions(origin, std::iter::once(tx));
+        let (_, tx0) = self.validate(origin, transaction.clone()).await;
+        let (_, tx1) = self.validate(origin, transaction).await;
+        let state_nonce_0 = if let TransactionValidationOutcome::Valid { state_nonce, .. } = tx0 { state_nonce } else { 0 };
+        let state_nonce_1 = if let TransactionValidationOutcome::Valid { state_nonce, .. } = tx1 { state_nonce } else { 0 };
+        assert_eq!(state_nonce_0, state_nonce_1);
+        let mut results = self.pool.add_transactions(origin, std::iter::once(tx0));
         results.pop().expect("result length is the same as the input")
     }

Then, run some spammers to a lot of transactions.

Node logs

No response

Platform(s)

Mac (Apple Silicon)

What version/commit are you on?

v1.1.0

What database version are you on?

N/A

Which chain / network are you on?

N/A

What type of node are you running?

Archive (default)

What prune config do you use, if any?

No response

If you've built Reth from source, provide the full command you used

No response

Code of Conduct

• I agree to follow the Code of Conduct

[github-actions]

This issue is stale because it has been open for 21 days with no activity.

[github-actions]

This issue was closed because it has been inactive for 7 days since being marked as stale.