add removal tracking logic

crates/astria-sequencer/app-genesis-state.json

@@ -0,0 +1,132 @@
+{
+  "chainId": "test-1",
+  "addressPrefixes": {
+    "base": "astria",
+    "ibcCompat": "astriacompat"
+  },
+  "accounts": [
+    {
+      "address": {
+        "bech32m": "astria1rsxyjrcm255ds9euthjx6yc3vrjt9sxrm9cfgm"
+      },
+      "balance": {
+        "lo": "1000000000000000000"
+      }
+    },
+    {
+      "address": {
+        "bech32m": "astria1xnlvg0rle2u6auane79t4p27g8hxnj36ja960z"
+      },
+      "balance": {
+        "lo": "1000000000000000000"
+      }
+    },
+    {
+      "address": {
+        "bech32m": "astria1vpcfutferpjtwv457r63uwr6hdm8gwr3pxt5ny"
+      },
+      "balance": {
+        "lo": "1000000000000000000"
+      }
+    }
+  ],
+  "authoritySudoAddress": {
+    "bech32m": "astria1rsxyjrcm255ds9euthjx6yc3vrjt9sxrm9cfgm"
+  },
+  "ibcSudoAddress": {
+    "bech32m": "astria1rsxyjrcm255ds9euthjx6yc3vrjt9sxrm9cfgm"
+  },
+  "ibcRelayerAddresses": [
+    {
+      "bech32m": "astria1rsxyjrcm255ds9euthjx6yc3vrjt9sxrm9cfgm"
+    },
+    {
+      "bech32m": "astria1xnlvg0rle2u6auane79t4p27g8hxnj36ja960z"
+    }
+  ],
+  "nativeAssetBaseDenomination": "nria",
+  "ibcParameters": {
+    "ibcEnabled": true,
+    "inboundIcs20TransfersEnabled": true,
+    "outboundIcs20TransfersEnabled": true
+  },
+  "allowedFeeAssets": [
+    "nria"
+  ],
+  "fees": {
+    "bridgeLock": {
+      "base": {
+        "lo": "12"
+      },
+      "multiplier": {
+        "lo": "1"
+      }
+    },
+    "bridgeSudoChange": {
+      "base": {
+        "lo": "24"
+      },
+      "multiplier": {}
+    },
+    "bridgeUnlock": {
+      "base": {
+        "lo": "12"
+      },
+      "multiplier": {}
+    },
+    "feeAssetChange": {
+      "base": {},
+      "multiplier": {}
+    },
+    "feeChange": {
+      "base": {},
+      "multiplier": {}
+    },
+    "ibcRelay": {
+      "base": {},
+      "multiplier": {}
+    },
+    "ibcRelayerChange": {
+      "base": {},
+      "multiplier": {}
+    },
+    "ibcSudoChange": {
+      "base": {},
+      "multiplier": {}
+    },
+    "ics20Withdrawal": {
+      "base": {
+        "lo": "24"
+      },
+      "multiplier": {}
+    },
+    "initBridgeAccount": {
+      "base": {
+        "lo": "48"
+      },
+      "multiplier": {}
+    },
+    "rollupDataSubmission": {
+      "base": {
+        "lo": "32"
+      },
+      "multiplier": {
+        "lo": "1"
+      }
+    },
+    "sudoAddressChange": {
+      "base": {},
+      "multiplier": {}
+    },
+    "transfer": {
+      "base": {
+        "lo": "12"
+      },
+      "multiplier": {}
+    },
+    "validatorUpdate": {
+      "base": {},
+      "multiplier": {}
+    }
+  }
+}

crates/astria-sequencer/src/mempool/mod.rs

@@ -49,6 +49,7 @@ pub(crate) enum RemovalReason {
     NonceStale,
     LowerNonceInvalidated,
     FailedPrepareProposal(String),
+    NonceReplacement([u8; 32]),
 }
 
 /// How long transactions are considered valid in the mempool.
@@ -143,11 +144,12 @@ impl<'a> ContainedTxLock<'a> {
 ///
 /// The mempool exposes the pending transactions through `builder_queue()`, which returns a copy of
 /// all pending transactions sorted in the order in which they should be executed. The sort order
-/// is firstly by the difference between the transaction nonce and the account's current nonce
-/// (ascending), and then by time first seen (ascending).
+/// is first by the transaction group (derived from the contained actions), then by the difference
+/// between the transaction nonce and the account's current nonce (ascending), and then by time
+/// first seen (ascending).
 ///
 /// The mempool implements the following policies:
-/// 1. Nonce replacement is not allowed.
+/// 1. Nonce replacement is only allowed for transactions in the parked queue.
 /// 2. Accounts cannot have more than `MAX_PARKED_TXS_PER_ACCOUNT` transactions in their parked
 ///    queues.
 /// 3. There is no account limit on pending transactions.
@@ -156,10 +158,7 @@ impl<'a> ContainedTxLock<'a> {
 ///    that account with a higher nonce will be removed as well. This is due to the fact that we do
 ///    not execute failing transactions, so a transaction 'failing' will mean that further account
 ///    nonces will not be able to execute either.
-///
-/// Future extensions to this mempool can include:
-/// - maximum mempool size
-/// - account balance aware pending queue
+/// 6. Parked transactions are globally limited to a configured amount.
 #[derive(Clone)]
 pub(crate) struct Mempool {
     pending: Arc<RwLock<PendingTransactions>>,
@@ -201,8 +200,8 @@ impl Mempool {
         }
     }
 
-    /// Inserts a transaction into the mempool and does not allow for transaction replacement.
-    /// Will return the reason for insertion failure if failure occurs.
+    /// Inserts a transaction into the mempool. Allows for transaction replacement of parked
+    /// transactions. Will return the reason for insertion failure if failure occurs.
     #[instrument(skip_all)]
     pub(crate) async fn insert(
         &self,
@@ -230,26 +229,28 @@ impl Mempool {
                     current_account_nonce,
                     &current_account_balances,
                 ) {
-                    Ok(()) => {
+                    Ok(hash) => {
                         // log current size of parked
                         self.metrics
                             .set_transactions_in_mempool_parked(parked.len());
 
                         // track in contained txs
                         self.lock_contained_txs().await.add(id);
+
+                        // remove the replaced transaction
+                        if let Some(replaced_hash) = hash {
+                            self.lock_contained_txs().await.remove(replaced_hash);
+                            self.comet_bft_removal_cache
+                                .write()
+                                .await
+                                .add(replaced_hash, RemovalReason::NonceReplacement(id));
+                        }
                         Ok(())
                     }
                     Err(err) => Err(err),
                 }
             }
-            error @ Err(
-                InsertionError::AlreadyPresent
-                | InsertionError::NonceTooLow
-                | InsertionError::NonceTaken
-                | InsertionError::AccountSizeLimit
-                | InsertionError::ParkedSizeLimit,
-            ) => error,
-            Ok(()) => {
+            Ok(_) => {
                 // check parked for txs able to be promoted
                 let to_promote = parked.find_promotables(
                     timemarked_tx.address(),
@@ -285,6 +286,7 @@ impl Mempool {
 
                 Ok(())
             }
+            Err(err) => Err(err),
         }
     }
 
@@ -1148,6 +1150,33 @@ mod tests {
         assert!(!mempool.is_tracked(tx1.id().get()).await);
     }
 
+    #[tokio::test]
+    async fn tx_tracked_nonce_replacement_removed() {
+        let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));
+        let mempool = Mempool::new(metrics, 100);
+        let account_balances = mock_balances(100, 100);
+        let tx_cost = mock_tx_cost(10, 10, 0);
+
+        let tx1_0 = MockTxBuilder::new().nonce(1).chain_id("test-0").build();
+        let tx1_1 = MockTxBuilder::new().nonce(1).chain_id("test-1").build();
+
+        // insert initial transaction into parked
+        mempool
+            .insert(tx1_0.clone(), 0, account_balances.clone(), tx_cost.clone())
+            .await
+            .unwrap();
+        // replace with different transaction
+        mempool
+            .insert(tx1_1.clone(), 0, account_balances.clone(), tx_cost.clone())
+            .await
+            .unwrap();
+
+        // check that the first transaction was removed and the replacement
+        // is tracked
+        assert!(!mempool.is_tracked(tx1_0.id().get()).await);
+        assert!(mempool.is_tracked(tx1_1.id().get()).await);
+    }
+
     #[tokio::test]
     async fn tx_tracked_reinsertion_ok() {
         let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap()));

crates/astria-sequencer/src/mempool/transactions_container.rs

@@ -430,7 +430,9 @@ pub(super) trait TransactionsForAccount: Default {
         current_account_balance: &HashMap<IbcPrefixed, u128>,
     ) -> bool;
 
-    /// Adds transaction to the container. Note: does NOT allow for nonce replacement.
+    /// Adds transaction to the container.
+    ///
+    /// Returns the hash of the replaced transaction if a nonce replacement occured.
     ///
     /// Will fail if in `SequentialNonces` mode and adding the transaction would create a nonce gap.
     /// Will fail if adding the transaction would exceed balance constraints.
@@ -445,7 +447,7 @@ pub(super) trait TransactionsForAccount: Default {
         ttx: TimemarkedTransaction,
         current_account_nonce: u32,
         current_account_balances: &HashMap<IbcPrefixed, u128>,
-    ) -> Result<(), InsertionError> {
+    ) -> Result<Option<[u8; 32]>, InsertionError> {
         if self.is_at_tx_limit() {
             return Err(InsertionError::AccountSizeLimit);
         }
@@ -454,12 +456,25 @@ pub(super) trait TransactionsForAccount: Default {
             return Err(InsertionError::NonceTooLow);
         }
 
+        // check if the nonce is already taken
         if let Some(existing_ttx) = self.txs().get(&ttx.signed_tx.nonce()) {
             return if existing_ttx.tx_hash == ttx.tx_hash {
                 Err(InsertionError::AlreadyPresent)
             } else if self.nonce_replacement_enabled() {
-                self.txs_mut().insert(ttx.signed_tx.nonce(), ttx);
-                Ok(())
+                let existing_hash = existing_ttx.tx_hash;
+
+                // replace the existing transaction and return the replaced hash for removal
+                let removed = self.txs_mut().insert(ttx.signed_tx.nonce(), ttx);
+                if let Some(removed) = removed {
+                    Ok(Some(removed.tx_hash))
+                } else {
+                    // this should never happen
+                    error!(
+                        tx_hash = %telemetry::display::hex(&existing_hash),
+                        "transaction replacement not found during 2nd lookup"
+                    );
+                    Ok(None)
+                }
             } else {
                 Err(InsertionError::NonceTaken)
             };
@@ -475,7 +490,7 @@ pub(super) trait TransactionsForAccount: Default {
 
         self.txs_mut().insert(ttx.signed_tx.nonce(), ttx);
 
-        Ok(())
+        Ok(None)
     }
 
     /// Removes transactions with the given nonce and higher.
@@ -612,6 +627,8 @@ pub(super) trait TransactionsContainer<T: TransactionsForAccount> {
 
     /// Adds the transaction to the container.
     ///
+    /// Returns the hash of the replaced transaction if a nonce replacement occured.
+    ///
     /// `current_account_nonce` should be the current nonce of the account associated with the
     /// transaction. If this ever decreases, the `TransactionsContainer` containers could become
     /// invalid.
@@ -620,22 +637,23 @@ pub(super) trait TransactionsContainer<T: TransactionsForAccount> {
         ttx: TimemarkedTransaction,
         current_account_nonce: u32,
         current_account_balances: &HashMap<IbcPrefixed, u128>,
-    ) -> Result<(), InsertionError> {
+    ) -> Result<Option<[u8; 32]>, InsertionError> {
         self.check_total_tx_count()?;
 
         match self.txs_mut().entry(*ttx.address()) {
             hash_map::Entry::Occupied(entry) => {
-                entry
+                return entry
                     .into_mut()
-                    .add(ttx, current_account_nonce, current_account_balances)?;
+                    .add(ttx, current_account_nonce, current_account_balances);
             }
             hash_map::Entry::Vacant(entry) => {
                 let mut txs = T::new();
+                // replacement shouldn't occur since container is empty
                 txs.add(ttx, current_account_nonce, current_account_balances)?;
                 entry.insert(txs);
             }
         }
-        Ok(())
+        Ok(None)
     }
 
     /// Removes the given transaction and any transactions with higher nonces for the relevant
@@ -1590,9 +1608,13 @@ mod tests {
         // add first transaction
         parked_txs.add(tx_0.clone(), 0, &account_balances).unwrap();
 
-        // replacing transaction should be ok
+        // replacing transaction should return the replaced tx hash
         let res = parked_txs.add(tx_1.clone(), 0, &account_balances);
-        assert!(res.is_ok(), "nonce replacement for parked should be ok");
+        assert_eq!(
+            res.unwrap(),
+            Some(tx_0.tx_hash),
+            "nonce replacement for parked should return the replaced tx hash"
+        );
 
         // a single transaction should be in the parked txs
         assert_eq!(