diff --git a/crates/astria-sequencer/app-genesis-state.json b/crates/astria-sequencer/app-genesis-state.json new file mode 100644 index 000000000..0ff7f8c23 --- /dev/null +++ b/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": {} + } + } +} \ No newline at end of file diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index 6f504fbe4..ca1dc6b4d 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/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>, @@ -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, ¤t_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())); diff --git a/crates/astria-sequencer/src/mempool/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index a2cb62c17..14067c4b7 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -430,7 +430,9 @@ pub(super) trait TransactionsForAccount: Default { current_account_balance: &HashMap, ) -> 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, - ) -> Result<(), InsertionError> { + ) -> Result, 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 { /// 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 { ttx: TimemarkedTransaction, current_account_nonce: u32, current_account_balances: &HashMap, - ) -> Result<(), InsertionError> { + ) -> Result, 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!( diff --git a/crates/astria-sequencer/src/metrics.rs b/crates/astria-sequencer/src/metrics.rs index a59f4d33b..d834499de 100644 --- a/crates/astria-sequencer/src/metrics.rs +++ b/crates/astria-sequencer/src/metrics.rs @@ -39,6 +39,7 @@ pub struct Metrics { transactions_in_mempool_total: Gauge, transactions_in_mempool_parked: Gauge, mempool_recosted: Counter, + mempool_nonce_replacement: Counter, internal_logic_error: Counter, } @@ -161,6 +162,10 @@ impl Metrics { self.mempool_recosted.increment(1); } + pub(crate) fn increment_mempool_nonce_replacement(&self) { + self.mempool_nonce_replacement.increment(1); + } + pub(crate) fn increment_internal_logic_error(&self) { self.internal_logic_error.increment(1); } @@ -347,14 +352,17 @@ impl telemetry::Metrics for Metrics { )? .register()?; - let internal_logic_error = builder + let mempool_nonce_replacement = builder .new_counter_factory( - INTERNAL_LOGIC_ERROR, - "The number of times a transaction has been rejected due to logic errors in the \ - mempool", + MEMPOOL_NONCE_REPLACEMENT, + "The number of times users have replaced a nonce in the mempool", )? .register()?; + let internal_logic_error = builder + .new_counter_factory(INTERNAL_LOGIC_ERROR, "The number of internal logic errors")? + .register()?; + Ok(Self { prepare_proposal_excluded_transactions_cometbft_space, prepare_proposal_excluded_transactions_sequencer_space, @@ -382,6 +390,7 @@ impl telemetry::Metrics for Metrics { transactions_in_mempool_total, transactions_in_mempool_parked, mempool_recosted, + mempool_nonce_replacement, internal_logic_error, }) } @@ -411,6 +420,7 @@ metric_names!(const METRICS_NAMES: TRANSACTIONS_IN_MEMPOOL_TOTAL, TRANSACTIONS_IN_MEMPOOL_PARKED, MEMPOOL_RECOSTED, + MEMPOOL_NONCE_REPLACEMENT, INTERNAL_LOGIC_ERROR ); @@ -425,6 +435,7 @@ mod tests { CHECK_TX_REMOVED_FAILED_STATELESS, CHECK_TX_REMOVED_TOO_LARGE, INTERNAL_LOGIC_ERROR, + MEMPOOL_NONCE_REPLACEMENT, MEMPOOL_RECOSTED, PREPARE_PROPOSAL_EXCLUDED_TRANSACTIONS, PREPARE_PROPOSAL_EXCLUDED_TRANSACTIONS_COMETBFT_SPACE, @@ -502,6 +513,7 @@ mod tests { "transactions_in_mempool_parked", ); assert_const(MEMPOOL_RECOSTED, "mempool_recosted"); + assert_const(MEMPOOL_NONCE_REPLACEMENT, "mempool_nonce_replacement"); assert_const(INTERNAL_LOGIC_ERROR, "internal_logic_error"); } } diff --git a/crates/astria-sequencer/src/service/mempool/mod.rs b/crates/astria-sequencer/src/service/mempool/mod.rs index 88b59a5b7..0a1a76466 100644 --- a/crates/astria-sequencer/src/service/mempool/mod.rs +++ b/crates/astria-sequencer/src/service/mempool/mod.rs @@ -99,6 +99,15 @@ impl IntoCheckTxResponse for RemovalReason { .into(), ..response::CheckTx::default() }, + RemovalReason::NonceReplacement(replacing_hash) => response::CheckTx { + code: Code::Err(AbciErrorCode::NONCE_REPLACEMENT.value()), + info: AbciErrorCode::NONCE_REPLACEMENT.to_string(), + log: format!( + "transaction replaced by a parked transaction with a lower nonce: \ + {replacing_hash:#?}" + ), + ..response::CheckTx::default() + }, } } } @@ -295,6 +304,10 @@ async fn check_removed_comet_bft( metrics.increment_check_tx_removed_failed_execution(); return Err(removal_reason.into_check_tx_response()); } + RemovalReason::NonceReplacement(_) => { + metrics.increment_mempool_nonce_replacement(); + return Err(removal_reason.into_check_tx_response()); + } _ => return Err(removal_reason.into_check_tx_response()), } };