From 552a1c988fab5b317337013fc908d7a9a0741d23 Mon Sep 17 00:00:00 2001 From: Fraser Hutchison Date: Fri, 17 Jan 2025 12:31:18 +0000 Subject: [PATCH] avoid passing state into builder_queue --- crates/astria-sequencer/src/app/mod.rs | 2 +- .../src/mempool/benchmarks.rs | 11 +- crates/astria-sequencer/src/mempool/mod.rs | 46 +++----- .../src/mempool/transactions_container.rs | 100 +++++++----------- 4 files changed, 57 insertions(+), 102 deletions(-) diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 0d82480fd6..3dd9e29c49 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -588,7 +588,7 @@ impl App { }; // get copy of transactions to execute from mempool - let pending_txs = self.mempool.builder_queue(&self.state).await; + let pending_txs = self.mempool.builder_queue().await; let mut unused_count = pending_txs.len(); for (tx_hash, tx) in pending_txs { diff --git a/crates/astria-sequencer/src/mempool/benchmarks.rs b/crates/astria-sequencer/src/mempool/benchmarks.rs index 7d2bcbc3cd..eac3920142 100644 --- a/crates/astria-sequencer/src/mempool/benchmarks.rs +++ b/crates/astria-sequencer/src/mempool/benchmarks.rs @@ -193,20 +193,11 @@ fn builder_queue(bencher: divan::Bencher) { .build() .unwrap(); - let mut mock_state = runtime.block_on(mock_state_getter()); - - // iterate over all signers and put their balances and nonces into the mock state - for i in 0..SIGNER_COUNT { - let signing_key = SigningKey::from([i; 32]); - let signing_address = signing_key.address_bytes(); - mock_state_put_account_nonce(&mut mock_state, &signing_address, 0); - } - bencher .with_inputs(|| init_mempool::()) .bench_values(move |mempool| { runtime.block_on(async { - mempool.builder_queue(&mock_state).await; + mempool.builder_queue().await; }); }); } diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index 0253cba591..de71d097e2 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -292,12 +292,8 @@ impl Mempool { /// Returns a copy of all transactions and their hashes ready for execution, sorted first by the /// difference between a transaction and the account's current nonce and then by the time that /// the transaction was first seen by the appside mempool. - #[instrument(skip_all, err(level = Level::DEBUG))] - pub(crate) async fn builder_queue( - &self, - state: &S, - ) -> Vec<([u8; 32], Arc)> { - self.pending.read().await.builder_queue(state).await + pub(crate) async fn builder_queue(&self) -> Vec<([u8; 32], Arc)> { + self.pending.read().await.builder_queue() } /// Removes the target transaction and all transactions for associated account with higher @@ -589,10 +585,9 @@ mod tests { #[tokio::test] async fn single_account_flow_extensive() { // This test tries to hit the more complex edges of the mempool with a single account. - // The test adds the nonces [1,2,0,4], creates a builder queue with the account - // nonce at 1, and then cleans the pool to nonce 4. This tests some of the - // odder edge cases that can be hit if a node goes offline or fails to see - // some transactions that other nodes include into their proposed blocks. + // The test adds the nonces [1,2,0,4], creates a builder queue, and then cleans the pool to + // nonce 4. This tests some of the odder edge cases that can be hit if a node goes offline + // or fails to see some transactions that other nodes include into their proposed blocks. let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap())); let mempool = Mempool::new(metrics, 100); let account_balances = mock_balances(100, 100); @@ -642,21 +637,13 @@ mod tests { // assert size assert_eq!(mempool.len().await, 4); - // mock state with nonce at 1 - let mut mock_state = mock_state_getter().await; - mock_state_put_account_nonce( - &mut mock_state, - astria_address_from_hex_string(ALICE_ADDRESS).as_bytes(), - 1, - ); - - // grab building queue, should return transactions [1,2] since [0] was below and [4] is - // gapped - let builder_queue = mempool.builder_queue(&mock_state).await; + // grab building queue, should return transactions [0,1,2] since [4] is gapped + let builder_queue = mempool.builder_queue().await; // see contains first two transactions that should be pending - assert_eq!(builder_queue[0].1.nonce(), 1, "nonce should be one"); - assert_eq!(builder_queue[1].1.nonce(), 2, "nonce should be two"); + assert_eq!(builder_queue[0].1.nonce(), 0, "nonce should be zero"); + assert_eq!(builder_queue[1].1.nonce(), 1, "nonce should be one"); + assert_eq!(builder_queue[2].1.nonce(), 2, "nonce should be two"); // see mempool's transactions just cloned, not consumed assert_eq!(mempool.len().await, 4); @@ -665,6 +652,7 @@ mod tests { // to pending // setup state + let mut mock_state = mock_state_getter().await; mock_state_put_account_nonce( &mut mock_state, astria_address_from_hex_string(ALICE_ADDRESS).as_bytes(), @@ -682,7 +670,7 @@ mod tests { assert_eq!(mempool.len().await, 1); // see transaction [4] properly promoted - let mut builder_queue = mempool.builder_queue(&mock_state).await; + let mut builder_queue = mempool.builder_queue().await; let (_, returned_tx) = builder_queue.pop().expect("should return last transaction"); assert_eq!(returned_tx.nonce(), 4, "nonce should be four"); } @@ -727,7 +715,7 @@ mod tests { 1, ); - let builder_queue = mempool.builder_queue(&mock_state).await; + let builder_queue = mempool.builder_queue().await; assert_eq!( builder_queue.len(), 1, @@ -746,7 +734,7 @@ mod tests { mempool.run_maintenance(&mock_state, false).await; // see builder queue now contains them - let builder_queue = mempool.builder_queue(&mock_state).await; + let builder_queue = mempool.builder_queue().await; assert_eq!( builder_queue.len(), 3, @@ -795,7 +783,7 @@ mod tests { 1, ); - let builder_queue = mempool.builder_queue(&mock_state).await; + let builder_queue = mempool.builder_queue().await; assert_eq!( builder_queue.len(), 4, @@ -812,7 +800,7 @@ mod tests { mempool.run_maintenance(&mock_state, false).await; // see builder queue now contains single transactions - let builder_queue = mempool.builder_queue(&mock_state).await; + let builder_queue = mempool.builder_queue().await; assert_eq!( builder_queue.len(), 1, @@ -832,7 +820,7 @@ mod tests { mempool.run_maintenance(&mock_state, false).await; - let builder_queue = mempool.builder_queue(&mock_state).await; + let builder_queue = mempool.builder_queue().await; assert_eq!( builder_queue.len(), 3, diff --git a/crates/astria-sequencer/src/mempool/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index 47c3d91ec1..67ae6a945f 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -28,7 +28,6 @@ use tokio::time::{ use tracing::{ error, instrument, - Level, }; use super::RemovalReason; @@ -213,6 +212,10 @@ impl PendingTransactionsForAccount { self.txs.last_key_value().map(|(nonce, _)| *nonce) } + fn current_account_nonce(&self) -> Option { + self.txs.first_key_value().map(|(nonce, _)| *nonce) + } + /// Removes and returns transactions that exceed the balances in `available_balances`. fn find_demotables( &mut self, @@ -774,11 +777,7 @@ impl PendingTransactions { /// Returns a copy of transactions and their hashes sorted by nonce difference and then time /// first seen. - #[instrument(skip_all, err(level = Level::DEBUG))] - pub(super) async fn builder_queue( - &self, - state: &S, - ) -> Vec<([u8; 32], Arc)> { + pub(super) fn builder_queue(&self) -> Vec<([u8; 32], Arc)> { // Used to hold the values in Vec for sorting. struct QueueEntry { tx: Arc, @@ -789,25 +788,12 @@ impl PendingTransactions { let mut queue = Vec::with_capacity(self.len()); // Add all transactions to the queue. for (address, account_txs) in &self.txs { - let current_account_nonce = match state.get_account_nonce(address).await { - Ok(nonce) => nonce, - Err(error) => { - error!(address = %telemetry::display::base64(address), "failed to fetch account nonce for builder queue: {error:#}"); - // use the lowest nonce in pending as the current nonce, this should be the - // same value as the nonce returned by the state. the pending queue shouldn't - // be empty, if it is we can continue to the next account - if let Some(nonce) = account_txs - .txs() - .values() - .map(TimemarkedTransaction::nonce) - .min() - { - nonce - } else { - error!(address = %telemetry::display::base64(address), "pending queue is empty during builder queue step"); - continue; - } - } + let Some(current_account_nonce) = account_txs.current_account_nonce() else { + error!( + address = %telemetry::display::base64(address), + "pending queue is empty during builder queue step" + ); + continue; }; for ttx in account_txs.txs.values() { @@ -888,7 +874,6 @@ mod tests { denom_3, mock_balances, mock_state_getter, - mock_state_put_account_nonce, mock_tx_cost, ALICE_ADDRESS, BOB_ADDRESS, @@ -1765,9 +1750,9 @@ mod tests { ); } - #[tokio::test] + #[test] #[expect(clippy::too_many_lines, reason = "it's a test")] - async fn transactions_container_clean_account_stale_expired() { + fn transactions_container_clean_account_stale_expired() { let mut pending_txs = PendingTransactions::new(TX_TTL); // transactions to add to accounts @@ -1983,8 +1968,8 @@ mod tests { ); } - #[tokio::test] - async fn pending_transactions_builder_queue() { + #[test] + fn pending_transactions_builder_queue() { let mut pending_txs = PendingTransactions::new(TX_TTL); // transactions to add to accounts @@ -2017,25 +2002,12 @@ mod tests { .add(ttx_s1_3.clone(), 1, &account_balances) .unwrap(); - // should return all transactions from Alice and last two from Bob - let mut mock_state = mock_state_getter().await; - mock_state_put_account_nonce( - &mut mock_state, - astria_address_from_hex_string(ALICE_ADDRESS).as_bytes(), - 1, - ); - mock_state_put_account_nonce( - &mut mock_state, - astria_address_from_hex_string(BOB_ADDRESS).as_bytes(), - 2, - ); - - // get builder queue - let builder_queue = pending_txs.builder_queue(&mock_state).await; + // get builder queue - should return all transactions from Alice and Bob + let builder_queue = pending_txs.builder_queue(); assert_eq!( builder_queue.len(), - 3, - "three transactions should've been popped" + 4, + "four transactions should've been popped" ); // check that the transactions are in the expected order @@ -2046,13 +2018,18 @@ mod tests { ); let (second_tx_hash, _) = builder_queue[1]; assert_eq!( - second_tx_hash, ttx_s1_2.tx_hash, + second_tx_hash, ttx_s1_1.tx_hash, "expected other low nonce diff (0) to be second" ); let (third_tx_hash, _) = builder_queue[2]; assert_eq!( - third_tx_hash, ttx_s1_3.tx_hash, - "expected highest nonce diff to be last" + third_tx_hash, ttx_s1_2.tx_hash, + "expected middle nonce diff (1) to be third" + ); + let (fourth_tx_hash, _) = builder_queue[3]; + assert_eq!( + fourth_tx_hash, ttx_s1_3.tx_hash, + "expected highest nonce diff (2) to be last" ); // ensure transactions not removed @@ -2063,8 +2040,8 @@ mod tests { ); } - #[tokio::test] - async fn parked_transactions_find_promotables() { + #[test] + fn parked_transactions_find_promotables() { let mut parked_txs = ParkedTransactions::::new(TX_TTL, 100); // transactions to add to accounts @@ -2130,8 +2107,8 @@ mod tests { ); } - #[tokio::test] - async fn pending_transactions_find_demotables() { + #[test] + fn pending_transactions_find_demotables() { let mut pending_txs = PendingTransactions::new(TX_TTL); // transactions to add to account @@ -2209,8 +2186,8 @@ mod tests { ); } - #[tokio::test] - async fn pending_transactions_remaining_account_balances() { + #[test] + fn pending_transactions_remaining_account_balances() { let mut pending_txs = PendingTransactions::new(TX_TTL); // transactions to add to account @@ -2265,10 +2242,9 @@ mod tests { ); } - #[tokio::test] - async fn builder_queue_should_be_sorted_by_action_group_type() { + #[test] + fn builder_queue_should_be_sorted_by_action_group_type() { let mut pending_txs = PendingTransactions::new(TX_TTL); - let mock_state = mock_state_getter().await; // create transactions in reverse order let ttx_unbundleable_sudo = MockTTXBuilder::new() @@ -2308,7 +2284,7 @@ mod tests { // get the builder queue // note: the account nonces are set to zero when not initialized in the mock state - let builder_queue = pending_txs.builder_queue(&mock_state).await; + let builder_queue = pending_txs.builder_queue(); // check that the transactions are in the expected order let (first_tx_hash, _) = builder_queue[0]; @@ -2336,8 +2312,8 @@ mod tests { ); } - #[tokio::test] - async fn parked_transactions_size_limit_works() { + #[test] + fn parked_transactions_size_limit_works() { let mut parked_txs = ParkedTransactions::::new(TX_TTL, 1); // transactions to add to account