From c32e0d19067443330f3c55a4961716b31a32ce23 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Tue, 29 Oct 2024 13:31:38 +0700 Subject: [PATCH 01/10] make parked nonces replaceable --- crates/astria-sequencer/src/mempool/mod.rs | 19 --- .../src/mempool/transactions_container.rs | 115 +++++++++++++----- 2 files changed, 85 insertions(+), 49 deletions(-) diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index 623f6f963b..6f504fbe41 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -548,25 +548,6 @@ mod tests { "already present" ); - // try to replace nonce - let tx1_replacement = MockTxBuilder::new() - .nonce(1) - .chain_id("test-chain-id") - .build(); - assert_eq!( - mempool - .insert( - tx1_replacement.clone(), - 0, - account_balances.clone(), - tx_cost.clone(), - ) - .await - .unwrap_err(), - InsertionError::NonceTaken, - "nonce replace not allowed" - ); - // add too low nonce let tx0 = MockTxBuilder::new().nonce(0).build(); assert_eq!( diff --git a/crates/astria-sequencer/src/mempool/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index 54ba8266ac..5e5852b0ef 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -301,6 +301,10 @@ impl TransactionsForAccount for PendingTransactionsForAccount { self.txs().contains_key(&previous_nonce) || ttx.signed_tx.nonce() == current_account_nonce } + fn nonce_replacement_enabled(&self) -> bool { + false + } + fn has_balance_to_cover( &self, ttx: &TimemarkedTransaction, @@ -377,6 +381,10 @@ impl TransactionsForAccount true } + fn nonce_replacement_enabled(&self) -> bool { + true + } + fn has_balance_to_cover( &self, _: &TimemarkedTransaction, @@ -410,6 +418,9 @@ pub(super) trait TransactionsForAccount: Default { current_account_nonce: u32, ) -> bool; + /// Returns true if the container allows for nonce replacement. + fn nonce_replacement_enabled(&self) -> bool; + /// Returns `Ok` if adding `ttx` would not break the balance precondition, i.e. enough /// balance to cover all transactions. /// Note: some implementations may clone the `current_account_balance` hashmap. @@ -444,11 +455,14 @@ pub(super) trait TransactionsForAccount: Default { } if let Some(existing_ttx) = self.txs().get(&ttx.signed_tx.nonce()) { - return Err(if existing_ttx.tx_hash == ttx.tx_hash { - InsertionError::AlreadyPresent + 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(()) } else { - InsertionError::NonceTaken - }); + Err(InsertionError::NonceTaken) + }; } if !self.is_sequential_nonce_precondition_met(&ttx, current_account_nonce) { @@ -1483,14 +1497,9 @@ mod tests { fn transactions_container_add() { let mut pending_txs = PendingTransactions::new(TX_TTL); // transactions to add to accounts - let ttx_s0_0_0 = MockTTXBuilder::new().nonce(0).build(); - // Same nonce and signer as `ttx_s0_0_0`, but different chain id. - let ttx_s0_0_1 = MockTTXBuilder::new() - .nonce(0) - .chain_id("different-chain-id") - .build(); - let ttx_s0_2_0 = MockTTXBuilder::new().nonce(2).build(); - let ttx_s1_0_0 = MockTTXBuilder::new() + let ttx_s0_0 = MockTTXBuilder::new().nonce(0).build(); + let ttx_s0_2 = MockTTXBuilder::new().nonce(2).build(); + let ttx_s1_0 = MockTTXBuilder::new() .nonce(0) .signer(get_bob_signing_key()) .build(); @@ -1507,7 +1516,7 @@ mod tests { // adding too low nonce shouldn't create account assert_eq!( pending_txs - .add(ttx_s0_0_0.clone(), 1, &account_balances) + .add(ttx_s0_0.clone(), 1, &account_balances) .unwrap_err(), InsertionError::NonceTooLow, "shouldn't be able to add nonce too low transaction" @@ -1519,39 +1528,26 @@ mod tests { // add one transaction pending_txs - .add(ttx_s0_0_0.clone(), 0, &account_balances) + .add(ttx_s0_0.clone(), 0, &account_balances) .unwrap(); assert_eq!(pending_txs.txs.len(), 1, "one account should exist"); // re-adding transaction should fail assert_eq!( - pending_txs - .add(ttx_s0_0_0, 0, &account_balances) - .unwrap_err(), + pending_txs.add(ttx_s0_0, 0, &account_balances).unwrap_err(), InsertionError::AlreadyPresent, "re-adding same transaction should fail" ); - // nonce replacement fails - assert_eq!( - pending_txs - .add(ttx_s0_0_1, 0, &account_balances) - .unwrap_err(), - InsertionError::NonceTaken, - "nonce replacement not supported" - ); - // nonce gaps not supported assert_eq!( - pending_txs - .add(ttx_s0_2_0, 0, &account_balances) - .unwrap_err(), + pending_txs.add(ttx_s0_2, 0, &account_balances).unwrap_err(), InsertionError::NonceGap, "gapped nonces in pending transactions not allowed" ); // add transactions for account 2 - pending_txs.add(ttx_s1_0_0, 0, &account_balances).unwrap(); + pending_txs.add(ttx_s1_0, 0, &account_balances).unwrap(); // check internal structures assert_eq!(pending_txs.txs.len(), 2, "two accounts should exist"); @@ -1582,6 +1578,65 @@ mod tests { ); } + #[test] + fn transactions_container_nonce_replacement_parked() { + let mut parked_txs = ParkedTransactions::::new(TX_TTL, 10); + let account_balances = mock_balances(1, 1); + + // create two transactions with same nonce but different hash + let tx_0 = MockTTXBuilder::new().nonce(0).chain_id("test-0").build(); + let tx_1 = MockTTXBuilder::new().nonce(0).chain_id("test-1").build(); + + // add first transaction + parked_txs.add(tx_0.clone(), 0, &account_balances).unwrap(); + + // replacing transaction should be ok + let res = parked_txs.add(tx_1.clone(), 0, &account_balances); + assert!(res.is_ok(), "nonce replacement for parked should be ok"); + + // a single transaction should be in the parked txs + assert_eq!( + parked_txs.len(), + 1, + "one transaction should be in the parked txs" + ); + + // the transaction should have been replaced + assert!(parked_txs.contains_tx(&tx_1.tx_hash)); + assert!(!parked_txs.contains_tx(&tx_0.tx_hash)); + } + + #[test] + fn transactions_container_nonce_replacement_pending() { + let mut pending_txs = PendingTransactions::new(TX_TTL); + let account_balances = mock_balances(1, 1); + + // create two transactions with same nonce but different hash + let tx_0 = MockTTXBuilder::new() + .nonce(0) + .group(Group::UnbundleableSudo) + .build(); + let tx_1 = MockTTXBuilder::new() + .nonce(0) + .group(Group::UnbundleableGeneral) + .build(); + + // add first transaction + pending_txs.add(tx_0.clone(), 0, &account_balances).unwrap(); + + // replacing transaction should fail + let res = pending_txs.add(tx_1.clone(), 0, &account_balances); + assert_eq!( + res, + Err(InsertionError::NonceTaken), + "nonce replacement for parked should return false" + ); + + // the transaction should not have been replaced + assert!(pending_txs.contains_tx(&tx_0.tx_hash)); + assert!(!pending_txs.contains_tx(&tx_1.tx_hash)); + } + #[test] fn transactions_container_remove() { let mut pending_txs = PendingTransactions::new(TX_TTL); From 92599a1e83426c8ded7874b439bf6529505d90ea Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Tue, 29 Oct 2024 13:44:11 +0700 Subject: [PATCH 02/10] update changelog and make tests uniform --- crates/astria-sequencer/CHANGELOG.md | 1 + .../src/mempool/transactions_container.rs | 10 ++-------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/crates/astria-sequencer/CHANGELOG.md b/crates/astria-sequencer/CHANGELOG.md index 9e18802d96..c7616d6a26 100644 --- a/crates/astria-sequencer/CHANGELOG.md +++ b/crates/astria-sequencer/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump penumbra dependencies [#1740](https://github.com/astriaorg/astria/pull/1740). - Move fee event recording to transaction from block [#1718](https://github.com/astriaorg/astria/pull/1718). +- Nonce replacment for pending transactions is now allowed in the mempool [#1763](https://github.com/astriaorg/astria/pull/1763). ## [1.0.0-rc.2] - 2024-10-23 diff --git a/crates/astria-sequencer/src/mempool/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index 5e5852b0ef..a2cb62c17c 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -1612,14 +1612,8 @@ mod tests { let account_balances = mock_balances(1, 1); // create two transactions with same nonce but different hash - let tx_0 = MockTTXBuilder::new() - .nonce(0) - .group(Group::UnbundleableSudo) - .build(); - let tx_1 = MockTTXBuilder::new() - .nonce(0) - .group(Group::UnbundleableGeneral) - .build(); + let tx_0 = MockTTXBuilder::new().nonce(0).chain_id("test-0").build(); + let tx_1 = MockTTXBuilder::new().nonce(0).chain_id("test-1").build(); // add first transaction pending_txs.add(tx_0.clone(), 0, &account_balances).unwrap(); From dff29a1c32b9a35b307b4f744ad3bb804b41f639 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Tue, 29 Oct 2024 16:54:07 +0700 Subject: [PATCH 03/10] add removal tracking logic --- .../astria-sequencer/app-genesis-state.json | 132 ++++++++++++++++++ crates/astria-sequencer/src/mempool/mod.rs | 65 ++++++--- .../src/mempool/transactions_container.rs | 44 ++++-- crates/astria-sequencer/src/metrics.rs | 20 ++- .../src/service/mempool/mod.rs | 13 ++ 5 files changed, 241 insertions(+), 33 deletions(-) create mode 100644 crates/astria-sequencer/app-genesis-state.json diff --git a/crates/astria-sequencer/app-genesis-state.json b/crates/astria-sequencer/app-genesis-state.json new file mode 100644 index 0000000000..0ff7f8c23f --- /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 6f504fbe41..ca1dc6b4d8 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 a2cb62c17c..14067c4b77 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 a59f4d33bf..d834499de8 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 88b59a5b77..0a1a764666 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()), } }; From d472b0d9f35ad57ed22a93ba66ecb2dad553d36a Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Tue, 29 Oct 2024 16:54:07 +0700 Subject: [PATCH 04/10] add removal tracking logic --- crates/astria-core/src/protocol/abci.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/astria-core/src/protocol/abci.rs b/crates/astria-core/src/protocol/abci.rs index 4047ae679f..34e2db2670 100644 --- a/crates/astria-core/src/protocol/abci.rs +++ b/crates/astria-core/src/protocol/abci.rs @@ -26,6 +26,7 @@ impl AbciErrorCode { pub const NONCE_TAKEN: Self = Self(unsafe { NonZeroU32::new_unchecked(15) }); pub const ACCOUNT_SIZE_LIMIT: Self = Self(unsafe { NonZeroU32::new_unchecked(16) }); pub const PARKED_FULL: Self = Self(unsafe { NonZeroU32::new_unchecked(17) }); + pub const NONCE_REPLACEMENT: Self = Self(unsafe { NonZeroU32::new_unchecked(18) }); } impl AbciErrorCode { @@ -64,6 +65,9 @@ impl AbciErrorCode { "the account has reached the maximum number of parked transactions".into() } Self::PARKED_FULL => "the mempool is out of space for more parked transactions".into(), + Self::NONCE_REPLACEMENT => { + "the transaction was replaced by a different transaction with the same nonce".into() + } Self(other) => { format!("invalid error code {other}: should be unreachable (this is a bug)") } From 4659fb6facf736634d8ed29e033b35e8be07b8ee Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Tue, 29 Oct 2024 17:02:07 +0700 Subject: [PATCH 05/10] remove extra file --- .../astria-sequencer/app-genesis-state.json | 132 ------------------ 1 file changed, 132 deletions(-) delete mode 100644 crates/astria-sequencer/app-genesis-state.json diff --git a/crates/astria-sequencer/app-genesis-state.json b/crates/astria-sequencer/app-genesis-state.json deleted file mode 100644 index 0ff7f8c23f..0000000000 --- a/crates/astria-sequencer/app-genesis-state.json +++ /dev/null @@ -1,132 +0,0 @@ -{ - "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 From 217010a45765c3a1b2570dc3b9948bf11efe7089 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Tue, 29 Oct 2024 17:06:34 +0700 Subject: [PATCH 06/10] nits --- crates/astria-sequencer/CHANGELOG.md | 2 +- crates/astria-sequencer/src/mempool/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/astria-sequencer/CHANGELOG.md b/crates/astria-sequencer/CHANGELOG.md index c7616d6a26..c7afd3e66f 100644 --- a/crates/astria-sequencer/CHANGELOG.md +++ b/crates/astria-sequencer/CHANGELOG.md @@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump penumbra dependencies [#1740](https://github.com/astriaorg/astria/pull/1740). - Move fee event recording to transaction from block [#1718](https://github.com/astriaorg/astria/pull/1718). -- Nonce replacment for pending transactions is now allowed in the mempool [#1763](https://github.com/astriaorg/astria/pull/1763). +- Nonce replacment for parked transactions is now allowed in the mempool [#1763](https://github.com/astriaorg/astria/pull/1763). ## [1.0.0-rc.2] - 2024-10-23 diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index ca1dc6b4d8..b47e08d7c6 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -200,7 +200,7 @@ impl Mempool { } } - /// Inserts a transaction into the mempool. Allows for transaction replacement of parked + /// Inserts a transaction into the mempool. Allows for nonce replacement of parked /// transactions. Will return the reason for insertion failure if failure occurs. #[instrument(skip_all)] pub(crate) async fn insert( From df700fff876a032f7e7ba24586153493f40cd431 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Wed, 30 Oct 2024 08:59:02 +0700 Subject: [PATCH 07/10] fix edge case and add more tests --- crates/astria-sequencer/src/mempool/mod.rs | 71 +++++++++++ .../src/mempool/transactions_container.rs | 95 ++++++++++++++ .../src/service/mempool/tests.rs | 120 +++++++++++++++++- 3 files changed, 284 insertions(+), 2 deletions(-) diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index b47e08d7c6..e1f363296a 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -251,6 +251,17 @@ impl Mempool { } } Ok(_) => { + // check if first transaction in parked was replaced + if let Some(replaced_hash) = parked.remove_replaced(&timemarked_tx) { + // remove from contained txs + self.lock_contained_txs().await.remove(replaced_hash); + // add to removal cache + self.comet_bft_removal_cache + .write() + .await + .add(replaced_hash, RemovalReason::NonceReplacement(id)); + } + // check parked for txs able to be promoted let to_promote = parked.find_promotables( timemarked_tx.address(), @@ -1150,6 +1161,66 @@ mod tests { assert!(!mempool.is_tracked(tx1.id().get()).await); } + #[tokio::test] + async fn tx_tracked_nonce_replacement_straight_to_pending() { + // tests that a transaction that is replaced by a transaction that is able to be + // placed into pending is removed from the tracked set + + let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap())); + let mempool = Mempool::new(metrics, 100); + let account_balances = mock_balances(10, 10); + let tx_cost_parked = mock_tx_cost(20, 10, 0); + let tx_cost_pending = 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(), 1, account_balances.clone(), tx_cost_parked) + .await + .unwrap(); + // replace with different transaction which goes straight to pending + mempool + .insert(tx1_1.clone(), 1, account_balances.clone(), tx_cost_pending) + .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_nonce_replacement_modify_parked() { + // tests that transactions that are waiting in parked can be replaced + // by other transactions that will also go to parked + 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_nonce_replacement_removed() { 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 14067c4b77..230b826007 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -860,6 +860,38 @@ impl ParkedTransactions Option<[u8; 32]> { + // Take the collection for this account out of `self` temporarily. + let mut account_txs = self.txs.remove(added_ttx.address())?; + + let found_tx = if let Some(first_entry) = account_txs.txs_mut().first_entry() { + if first_entry.get().nonce() == added_ttx.nonce() { + let removed_hash = first_entry.get().tx_hash; + first_entry.remove(); + Some(removed_hash) + } else { + None + } + } else { + None + }; + + // Re-add the collection to `self` if it's not empty. + if !account_txs.txs().is_empty() { + let _ = self.txs.insert(*added_ttx.address(), account_txs); + } + + found_tx + } + /// Removes and returns the transactions that can be promoted from parked to pending for /// an account. Will only return sequential nonces from `target_nonce` whose costs are /// covered by the `available_balance`. @@ -2180,6 +2212,69 @@ mod tests { ); } + #[tokio::test] + async fn parked_transactions_removed_replaced_works() { + let mut parked_txs = ParkedTransactions::::new(TX_TTL, 100); + let account_balances = mock_balances(100, 100); + + let ttx_1 = MockTTXBuilder::new().nonce(1).chain_id("test-1").build(); + let ttx_2 = MockTTXBuilder::new().nonce(2).build(); + + let replacement_tx = MockTTXBuilder::new().nonce(1).chain_id("test-2").build(); + + // add transactions to pending + parked_txs.add(ttx_1.clone(), 0, &account_balances).unwrap(); + parked_txs.add(ttx_2.clone(), 0, &account_balances).unwrap(); + + // check for replacement should find the transaction with the matching nonce + let removed_tx = parked_txs.remove_replaced(&replacement_tx); + assert_eq!(removed_tx, Some(ttx_1.tx_hash)); + + // other transactions should still be in the container + assert_eq!(parked_txs.len(), 1); + assert!(parked_txs.contains_tx(&ttx_2.tx_hash)); + } + + #[tokio::test] + async fn parked_transactions_removed_replaced_empty_ok() { + let mut parked_txs = ParkedTransactions::::new(TX_TTL, 100); + + let replacement_tx = MockTTXBuilder::new().nonce(1).build(); + + // check for replacement should be fine on empty accounts + let removed_tx = parked_txs.remove_replaced(&replacement_tx); + assert_eq!(removed_tx, None); + } + + #[tokio::test] + async fn parked_transactions_removed_replaced_wrong_nonces_ok() { + let mut parked_txs = ParkedTransactions::::new(TX_TTL, 100); + let account_balances = mock_balances(100, 100); + + let ttx_1 = MockTTXBuilder::new().nonce(1).build(); + let ttx_2 = MockTTXBuilder::new().nonce(2).build(); + + let replacement_tx_higher = MockTTXBuilder::new().nonce(2).build(); + let replacement_tx_lower = MockTTXBuilder::new().nonce(0).build(); + + // add transactions to pending + parked_txs.add(ttx_1.clone(), 0, &account_balances).unwrap(); + parked_txs.add(ttx_2.clone(), 0, &account_balances).unwrap(); + + // replacement for higher nonce should be ignored + let removed_tx = parked_txs.remove_replaced(&replacement_tx_higher); + assert_eq!(removed_tx, None); + + // replacement for lower nonce should be ignored + let removed_tx = parked_txs.remove_replaced(&replacement_tx_lower); + assert_eq!(removed_tx, None); + + // other transactions should still be in the container + assert_eq!(parked_txs.len(), 2); + assert!(parked_txs.contains_tx(&ttx_1.tx_hash)); + assert!(parked_txs.contains_tx(&ttx_2.tx_hash)); + } + #[tokio::test] async fn pending_transactions_find_demotables() { let mut pending_txs = PendingTransactions::new(TX_TTL); diff --git a/crates/astria-sequencer/src/service/mempool/tests.rs b/crates/astria-sequencer/src/service/mempool/tests.rs index 1edb81e541..e402d6e7d8 100644 --- a/crates/astria-sequencer/src/service/mempool/tests.rs +++ b/crates/astria-sequencer/src/service/mempool/tests.rs @@ -1,6 +1,12 @@ use std::num::NonZeroU32; -use astria_core::Protobuf as _; +use astria_core::{ + protocol::transaction::v1::{ + action::Transfer, + TransactionBody, + }, + Protobuf as _, +}; use prost::Message as _; use telemetry::Metrics; use tendermint::{ @@ -13,13 +19,19 @@ use tendermint::{ use crate::{ app::{ - test_utils::MockTxBuilder, + test_utils::{ + get_alice_signing_key, + MockTxBuilder, + ALICE_ADDRESS, + BOB_ADDRESS, + }, App, }, mempool::{ Mempool, RemovalReason, }, + test_utils::astria_address_from_hex_string, }; #[tokio::test] @@ -52,6 +64,110 @@ async fn future_nonces_are_accepted() { assert_eq!(mempool.len().await, 1); } +#[tokio::test] +async fn too_expensive_txs_are_replaceable() { + // The mempool should allow replacement of transactions that an account does + // not have enough balance to afford. + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + + let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap())); + let mut mempool = Mempool::new(metrics, 100); + let mut app = App::new(snapshot, mempool.clone(), metrics).await.unwrap(); + let chain_id = "test".to_string(); + let genesis_state = crate::app::test_utils::genesis_state(); + + // get balance higher than alice's + let alice_balance = genesis_state + .accounts() + .iter() + .find(|a| a.address == astria_address_from_hex_string(ALICE_ADDRESS)) + .unwrap() + .balance; + let too_expensive_amount = alice_balance + 10; + + app.init_chain(storage.clone(), genesis_state, vec![], chain_id.clone()) + .await + .unwrap(); + app.commit(storage.clone()).await; + + let tx_too_expensive = TransactionBody::builder() + .actions(vec![ + Transfer { + to: astria_address_from_hex_string(BOB_ADDRESS), + amount: too_expensive_amount, + asset: crate::test_utils::nria().into(), + fee_asset: crate::test_utils::nria().into(), + } + .into(), + ]) + .nonce(0) + .chain_id(chain_id.clone()) + .try_build() + .unwrap() + .sign(&get_alice_signing_key()); + + let tx_ok = TransactionBody::builder() + .actions(vec![ + Transfer { + to: astria_address_from_hex_string(BOB_ADDRESS), + amount: 1, + asset: crate::test_utils::nria().into(), + fee_asset: crate::test_utils::nria().into(), + } + .into(), + ]) + .nonce(0) + .chain_id(chain_id.clone()) + .try_build() + .unwrap() + .sign(&get_alice_signing_key()); + let req_too_expensive = CheckTx { + tx: tx_too_expensive.to_raw().encode_to_vec().into(), + kind: CheckTxKind::New, + }; + let req_ok = CheckTx { + tx: tx_ok.to_raw().encode_to_vec().into(), + kind: CheckTxKind::New, + }; + + // too expensive should enter the mempool + let rsp_too_expensive = super::handle_check_tx( + req_too_expensive, + storage.latest_snapshot(), + &mut mempool, + metrics, + ) + .await; + assert_eq!(rsp_too_expensive.code, Code::Ok, "{rsp_too_expensive:#?}"); + + // ok should enter the mempool + let rsp_ok = + super::handle_check_tx(req_ok, storage.latest_snapshot(), &mut mempool, metrics).await; + assert_eq!(rsp_ok.code, Code::Ok, "{rsp_ok:#?}"); + + // recheck on too expensive should be nonce replacement + let req_too_expensive = CheckTx { + tx: tx_too_expensive.to_raw().encode_to_vec().into(), + kind: CheckTxKind::Recheck, + }; + let rsp_too_expensive_recheck = super::handle_check_tx( + req_too_expensive, + storage.latest_snapshot(), + &mut mempool, + metrics, + ) + .await; + assert_eq!( + rsp_too_expensive_recheck.code, + Code::Err(NonZeroU32::new(18).unwrap()), + "{rsp_too_expensive_recheck:#?}" + ); + + // mempool should contain single transaction + assert_eq!(mempool.len().await, 1); +} + #[tokio::test] async fn rechecks_pass() { // The mempool should not fail rechecks of transactions. From 722c3697e7ecf58f0f00ad289265d4b361699d04 Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Wed, 30 Oct 2024 09:06:48 +0700 Subject: [PATCH 08/10] fix imports --- .../src/service/mempool/tests.rs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/crates/astria-sequencer/src/service/mempool/tests.rs b/crates/astria-sequencer/src/service/mempool/tests.rs index f5972a4c85..25f29a6071 100644 --- a/crates/astria-sequencer/src/service/mempool/tests.rs +++ b/crates/astria-sequencer/src/service/mempool/tests.rs @@ -19,21 +19,25 @@ use tendermint::{ use crate::{ app::{ + benchmark_and_test_utils::{ + genesis_state, + ALICE_ADDRESS, + BOB_ADDRESS, + }, test_utils::{ get_alice_signing_key, MockTxBuilder, - ALICE_ADDRESS, - BOB_ADDRESS, }, - benchmark_and_test_utils::genesis_state, - test_utils::MockTxBuilder, App, }, + benchmark_and_test_utils::{ + astria_address_from_hex_string, + nria, + }, mempool::{ Mempool, RemovalReason, }, - test_utils::astria_address_from_hex_string, }; #[tokio::test] @@ -77,7 +81,7 @@ async fn too_expensive_txs_are_replaceable() { let mut mempool = Mempool::new(metrics, 100); let mut app = App::new(snapshot, mempool.clone(), metrics).await.unwrap(); let chain_id = "test".to_string(); - let genesis_state = crate::app::test_utils::genesis_state(); + let genesis_state = genesis_state(); // get balance higher than alice's let alice_balance = genesis_state @@ -98,8 +102,8 @@ async fn too_expensive_txs_are_replaceable() { Transfer { to: astria_address_from_hex_string(BOB_ADDRESS), amount: too_expensive_amount, - asset: crate::test_utils::nria().into(), - fee_asset: crate::test_utils::nria().into(), + asset: nria().into(), + fee_asset: nria().into(), } .into(), ]) @@ -114,8 +118,8 @@ async fn too_expensive_txs_are_replaceable() { Transfer { to: astria_address_from_hex_string(BOB_ADDRESS), amount: 1, - asset: crate::test_utils::nria().into(), - fee_asset: crate::test_utils::nria().into(), + asset: nria().into(), + fee_asset: nria().into(), } .into(), ]) From 512a12215ed1a4625781079780a921aa8a9ca83d Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Wed, 30 Oct 2024 09:13:50 +0700 Subject: [PATCH 09/10] remove test copy --- .../astria-sequencer/app-genesis-state.json | 132 ++++++++++++++++++ crates/astria-sequencer/src/mempool/mod.rs | 27 ---- 2 files changed, 132 insertions(+), 27 deletions(-) create mode 100644 crates/astria-sequencer/app-genesis-state.json diff --git a/crates/astria-sequencer/app-genesis-state.json b/crates/astria-sequencer/app-genesis-state.json new file mode 100644 index 0000000000..0ff7f8c23f --- /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 c537ff71ae..ea3fa07e64 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -1225,33 +1225,6 @@ mod tests { assert!(mempool.is_tracked(tx1_1.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())); From 635cfe01ec8249f1a05a417efb38dd785aca39cc Mon Sep 17 00:00:00 2001 From: Lily Johnson Date: Wed, 30 Oct 2024 09:20:48 +0700 Subject: [PATCH 10/10] nits --- .../astria-sequencer/app-genesis-state.json | 132 ------------------ .../src/mempool/transactions_container.rs | 2 +- .../src/service/mempool/mod.rs | 5 +- 3 files changed, 2 insertions(+), 137 deletions(-) delete mode 100644 crates/astria-sequencer/app-genesis-state.json diff --git a/crates/astria-sequencer/app-genesis-state.json b/crates/astria-sequencer/app-genesis-state.json deleted file mode 100644 index 0ff7f8c23f..0000000000 --- a/crates/astria-sequencer/app-genesis-state.json +++ /dev/null @@ -1,132 +0,0 @@ -{ - "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/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index f3503b14e4..5600ef7ff0 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -1681,7 +1681,7 @@ mod tests { assert_eq!( res, Err(InsertionError::NonceTaken), - "nonce replacement for parked should return false" + "nonce replacement for pending should fail" ); // the transaction should not have been replaced diff --git a/crates/astria-sequencer/src/service/mempool/mod.rs b/crates/astria-sequencer/src/service/mempool/mod.rs index 0a1a764666..c571746650 100644 --- a/crates/astria-sequencer/src/service/mempool/mod.rs +++ b/crates/astria-sequencer/src/service/mempool/mod.rs @@ -102,10 +102,7 @@ impl IntoCheckTxResponse for RemovalReason { 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:#?}" - ), + log: format!("nonce replacement;transaction replaced with: {replacing_hash:#?}"), ..response::CheckTx::default() }, }