diff --git a/crates/mempool/src/mempool.rs b/crates/mempool/src/mempool.rs index 0bdca009..ac31acc9 100644 --- a/crates/mempool/src/mempool.rs +++ b/crates/mempool/src/mempool.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; -use starknet_api::core::ContractAddress; -use starknet_api::transaction::TransactionHash; +use starknet_api::core::{ContractAddress, Nonce}; +use starknet_api::transaction::{Tip, TransactionHash}; use starknet_mempool_types::mempool_types::{ AccountState, MempoolInput, MempoolResult, ThinTransaction, }; @@ -16,8 +16,10 @@ pub mod mempool_test; #[derive(Debug, Default)] pub struct Mempool { // TODO: add docstring explaining visibility and coupling of the fields. - txs_queue: TransactionQueue, + // All transactions currently held in the mempool. tx_pool: TransactionPool, + // Transactions eligible for sequencing. + tx_queue: TransactionQueue, } impl Mempool { @@ -34,6 +36,10 @@ impl Mempool { Mempool::default() } + pub fn iter(&self) -> impl Iterator { + self.tx_queue.iter() + } + /// Retrieves up to `n_txs` transactions with the highest priority from the mempool. /// Transactions are guaranteed to be unique across calls until `commit_block` is invoked. // TODO: the last part about commit_block is incorrect if we delete txs in get_txs and then push @@ -41,11 +47,9 @@ impl Mempool { // library. pub fn get_txs(&mut self, n_txs: usize) -> MempoolResult> { let mut eligible_txs: Vec = Vec::with_capacity(n_txs); - - let txs = self.txs_queue.pop_last_chunk(n_txs); - for tx in txs { - self.tx_pool.remove(tx.tx_hash)?; - eligible_txs.push(tx.0); + for tx_hash in self.tx_queue.pop_last_chunk(n_txs) { + let tx = self.tx_pool.remove(tx_hash)?; + eligible_txs.push(tx); } Ok(eligible_txs) @@ -75,7 +79,7 @@ impl Mempool { let tx = input.tx; self.tx_pool.insert(tx.clone())?; - self.txs_queue.insert(TransactionReference::new(tx)); + self.tx_queue.insert(TransactionReference::new(&tx)); Ok(()) } @@ -85,11 +89,21 @@ impl Mempool { /// execution fields). /// TODO(Mohammad): rename this struct to `ThinTransaction` once that name /// becomes available, to better reflect its purpose and usage. -#[derive(Clone, Debug, Default, derive_more::Deref)] -pub struct TransactionReference(pub ThinTransaction); +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub struct TransactionReference { + pub sender_address: ContractAddress, + pub nonce: Nonce, + pub tx_hash: TransactionHash, + pub tip: Tip, +} impl TransactionReference { - pub fn new(tx: ThinTransaction) -> Self { - TransactionReference(tx) + pub fn new(tx: &ThinTransaction) -> Self { + TransactionReference { + sender_address: tx.sender_address, + nonce: tx.nonce, + tx_hash: tx.tx_hash, + tip: tx.tip, + } } } diff --git a/crates/mempool/src/mempool_test.rs b/crates/mempool/src/mempool_test.rs index 89043e5c..027ae3cf 100644 --- a/crates/mempool/src/mempool_test.rs +++ b/crates/mempool/src/mempool_test.rs @@ -9,7 +9,7 @@ use starknet_api::{contract_address, patricia_key}; use starknet_mempool_types::errors::MempoolError; use starknet_mempool_types::mempool_types::{Account, ThinTransaction}; -use crate::mempool::{Mempool, MempoolInput}; +use crate::mempool::{Mempool, MempoolInput, TransactionReference}; /// Creates a valid input for mempool's `add_tx` with optional default value for /// `sender_address`. @@ -48,12 +48,13 @@ fn mempool() -> Mempool { // transactions. #[track_caller] fn check_mempool_txs_eq(mempool: &Mempool, expected_txs: &[ThinTransaction]) { - let mempool_txs = mempool.txs_queue.iter(); + let mempool_txs = mempool.tx_queue.iter(); + let expected_txs = expected_txs.iter().map(TransactionReference::new); assert!( zip_eq(expected_txs, mempool_txs) // Deref the inner mempool tx type. - .all(|(expected_tx, mempool_tx)| *expected_tx == *mempool_tx) + .all(|(expected_tx, mempool_tx)| expected_tx == *mempool_tx) ); } diff --git a/crates/mempool/src/transaction_pool.rs b/crates/mempool/src/transaction_pool.rs index 5891725a..57b38a3f 100644 --- a/crates/mempool/src/transaction_pool.rs +++ b/crates/mempool/src/transaction_pool.rs @@ -20,29 +20,29 @@ pub struct TransactionPool { } impl TransactionPool { - // TODO(Mohammad): Remove the cloning of tx once the `TransactionReference` is updated. pub fn insert(&mut self, tx: ThinTransaction) -> MempoolResult<()> { let tx_hash = tx.tx_hash; - // Insert transaction to pool, if it is new. - if let hash_map::Entry::Vacant(entry) = self.tx_pool.entry(tx_hash) { - entry.insert(tx.clone()); - } else { - return Err(MempoolError::DuplicateTransaction { tx_hash }); - } - + // Insert transaction into the mapping by account. let txs_from_account_entry = self.txs_by_account.entry(tx.sender_address).or_default(); match txs_from_account_entry.entry(tx.nonce) { btree_map::Entry::Vacant(txs_from_account) => { - txs_from_account.insert(TransactionReference::new(tx)); + txs_from_account.insert(TransactionReference::new(&tx)); } btree_map::Entry::Occupied(_) => { - panic!( - "Transaction pool consistency error: transaction with hash {tx_hash} does not \ - appear in main mapping, but it appears in the account mapping" - ); + return Err(MempoolError::DuplicateTransaction { tx_hash }); } } + + // Insert transaction to pool, if it is new. + if let hash_map::Entry::Vacant(entry) = self.tx_pool.entry(tx_hash) { + entry.insert(tx); + } else { + panic!( + "Transaction pool consistency error: transaction with hash {tx_hash} does not \ + appear in the account mapping, but it appears in the main mapping" + ); + } Ok(()) } diff --git a/crates/mempool/src/transaction_queue.rs b/crates/mempool/src/transaction_queue.rs index f4746b01..7898141b 100644 --- a/crates/mempool/src/transaction_queue.rs +++ b/crates/mempool/src/transaction_queue.rs @@ -2,6 +2,7 @@ use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, VecDeque}; use starknet_api::core::{ContractAddress, Nonce}; +use starknet_api::transaction::TransactionHash; use starknet_mempool_types::mempool_types::ThinTransaction; use crate::mempool::TransactionReference; @@ -29,21 +30,18 @@ impl TransactionQueue { } // TODO(gilad): remove collect - pub fn pop_last_chunk(&mut self, n_txs: usize) -> Vec { + pub fn pop_last_chunk(&mut self, n_txs: usize) -> Vec { let txs: Vec = (0..n_txs).filter_map(|_| self.queue.pop_last().map(|tx| tx.0)).collect(); - for tx in &txs { self.address_to_nonce.remove(&tx.sender_address); } - txs + txs.into_iter().map(|tx| tx.tx_hash).collect() } - // TODO(Mohammad): return reference once `TransactionReference` can impl Copy. - #[cfg(any(feature = "testing", test))] - pub fn iter(&self) -> impl Iterator + '_ { - self.queue.iter().map(|tx| tx.0.clone()) + pub fn iter(&self) -> impl Iterator { + self.queue.iter().map(|tx| &tx.0) } pub fn _get_nonce(&self, address: &ContractAddress) -> Option<&Nonce> { @@ -51,16 +49,6 @@ impl TransactionQueue { } } -impl From> for TransactionQueue { - fn from(txs: Vec) -> Self { - let mut tx_queue = TransactionQueue::default(); - for tx in txs { - tx_queue.insert(tx); - } - tx_queue - } -} - #[derive(Clone, Debug, derive_more::Deref, derive_more::From)] struct QueuedTransaction(pub TransactionReference);