refactor(mempool): simplify design

- For the MVP, it suffices to use the queue as the entire storage
  (without a separate tx store), because all txs that are accepted in
  the mempool are indexed (no future nonces allowed).
- Remove `Mempool` from type names, it follows from module location.
- Switch from heap to balanced search tree, since we are not supporting
  future nonce for the MVP, we can just return the last `n` txs on
  get_txs(n), which fits a search tree better.

Cargo.toml

@@ -21,15 +21,15 @@ as_conversions = "deny"
 [workspace.dependencies]
 assert_matches = "1.5.0"
 axum = "0.6.12"
-clap = { version = "4.3.10" }
+clap = "4.3.10"
+derive_more = "0.99"
 hyper = "1.2.0"
+papyrus_config = "0.3.0"
 pretty_assertions = "1.4.0"
 rstest = "0.17.0"
 serde = { version = "1.0.193", features = ["derive"] }
 serde_json = "1.0"
-starknet_api = { git = "https://github.com/starkware-libs/starknet-api.git", rev = "48d61bb" }
-papyrus_config = "0.3.0"
+starknet_api = { git = "https://github.com/starkware-libs/starknet-api.git", rev = "016ff6c" }
 thiserror = "1.0"
 tokio = { version = "1", features = ["full"] }
 validator = "0.12"
-

crates/mempool/Cargo.toml

@@ -9,6 +9,7 @@ license.workspace = true
 workspace = true
 
 [dependencies]
+derive_more.workspace = true
 starknet_api.workspace = true
 
 [dev-dependencies]

crates/mempool/src/lib.rs

@@ -1 +1,2 @@
+// TODO: change to pub(crate) once this is used by the (not yet implemented) mempool struct.
 pub mod priority_queue;

crates/mempool/src/priority_queue.rs

@@ -2,101 +2,74 @@
 #[path = "priority_queue_test.rs"]
 pub mod priority_queue_test;
 
-use std::{
-    cmp::Ordering,
-    collections::{BinaryHeap, HashMap},
-};
+use std::{cmp::Ordering, collections::BTreeSet};
 
 use starknet_api::{
     internal_transaction::InternalTransaction,
-    transaction::{
-        DeclareTransaction, DeployAccountTransaction, InvokeTransaction, Tip, TransactionHash,
-    },
+    transaction::{DeclareTransaction, DeployAccountTransaction, InvokeTransaction, Tip},
 };
 
-fn get_tip(tx: &InternalTransaction) -> Tip {
-    match tx {
-        InternalTransaction::Declare(declare_tx) => match &declare_tx.tx {
-            DeclareTransaction::V3(declare_tx_v3) => declare_tx_v3.tip,
-            _ => panic!("Unexpected transaction version."),
-        },
-        InternalTransaction::DeployAccount(deploy_account_tx) => match &deploy_account_tx.tx {
-            DeployAccountTransaction::V3(tx_v3) => tx_v3.tip,
-            _ => panic!("Unexpected transaction version."),
-        },
-        InternalTransaction::Invoke(invoke_tx) => match &invoke_tx.tx {
-            InvokeTransaction::V3(tx_v3) => tx_v3.tip,
-            _ => panic!("Unexpected transaction version."),
-        },
-    }
-}
+// Assumption: for the MVP only one transaction from the same contract class can be in the mempool
+// at a time. When this changes, saving the transactions themselves on the queu might no longer be
+// appropriate, because we'll also need to stores transactions without indexing them. For example,
+// transactions with future nonces will need to be stored, and potentially indexed on block commits.
+#[derive(Clone, Debug, Default, derive_more::Deref, derive_more::DerefMut)]
+pub struct PriorityQueue(BTreeSet<Transaction>);
 
-fn get_tx_hash(tx: &InternalTransaction) -> TransactionHash {
-    match tx {
-        InternalTransaction::Declare(declare_tx) => declare_tx.tx_hash,
-        InternalTransaction::DeployAccount(deploy_account_tx) => deploy_account_tx.tx_hash,
-        InternalTransaction::Invoke(invoke_tx) => invoke_tx.tx_hash,
+impl PriorityQueue {
+    pub fn push(&mut self, tx: InternalTransaction) {
+        let mempool_tx = Transaction(tx);
+        self.insert(mempool_tx);
     }
-}
 
-#[derive(Clone, Debug)]
-pub struct MempoolTransaction {
-    pub tx: InternalTransaction,
-    pub tx_hash: TransactionHash,
-}
-
-impl PartialEq for MempoolTransaction {
-    fn eq(&self, other: &MempoolTransaction) -> bool {
-        get_tip(&self.tx) == get_tip(&other.tx)
+    // Removes and returns the transaction with the highest tip.
+    pub fn pop(&mut self) -> Option<InternalTransaction> {
+        self.pop_last().map(|tx| tx.0)
     }
 }
 
-impl Eq for MempoolTransaction {}
+#[derive(Clone, Debug, derive_more::Deref)]
+pub struct Transaction(pub InternalTransaction);
 
-impl PartialOrd for MempoolTransaction {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
+impl Transaction {
+    fn tip(&self) -> Tip {
+        match &self.0 {
+            InternalTransaction::Declare(declare_tx) => match &declare_tx.tx {
+                DeclareTransaction::V3(tx_v3) => tx_v3.tip,
+                _ => unimplemented!(),
+            },
+            InternalTransaction::DeployAccount(deploy_account_tx) => match &deploy_account_tx.tx {
+                DeployAccountTransaction::V3(tx_v3) => tx_v3.tip,
+                _ => unimplemented!(),
+            },
+            InternalTransaction::Invoke(invoke_tx) => match &invoke_tx.tx {
+                InvokeTransaction::V3(tx_v3) => tx_v3.tip,
+                _ => unimplemented!(),
+            },
+        }
     }
 }
 
-impl Ord for MempoolTransaction {
-    fn cmp(&self, other: &Self) -> Ordering {
-        get_tip(&self.tx).cmp(&get_tip(&other.tx))
+// Compare transactions based on their tip only, which implies `Eq`, because `tip` is uint.
+impl PartialEq for Transaction {
+    fn eq(&self, other: &Transaction) -> bool {
+        self.tip() == other.tip()
     }
 }
 
-#[derive(Clone, Default)]
-pub struct PriorityQueue {
-    heap: BinaryHeap<MempoolTransaction>,
-    pub tx_hash_to_tx_map: HashMap<TransactionHash, InternalTransaction>,
-}
-
-impl PriorityQueue {
-    pub fn new() -> Self {
-        PriorityQueue {
-            heap: BinaryHeap::new(),
-            tx_hash_to_tx_map: HashMap::new(),
-        }
-    }
-
-    pub fn push(&mut self, tx: InternalTransaction) {
-        let tx_hash = get_tx_hash(&tx);
-        self.tx_hash_to_tx_map.insert(tx_hash, tx.clone());
-        let mempool_tx = MempoolTransaction { tx, tx_hash };
+/// Marks Transaction as capable of strict equality comparisons, signaling to the compiler it
+/// adheres to equality semantics.
+// Note: this depends on the implementation of `PartialEq`, see its docstring.
+impl Eq for Transaction {}
 
-        self.heap.push(mempool_tx);
+impl Ord for Transaction {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.tip().cmp(&other.tip())
     }
+}
 
-    // Removes and returns the transaction with the highest tip.
-    pub fn pop(&mut self) -> Option<TransactionHash> {
-        let mempool_tx = self.heap.pop();
-        match mempool_tx {
-            Some(mempool_tx) => {
-                let tx_hash = mempool_tx.tx_hash;
-                self.tx_hash_to_tx_map.remove(&tx_hash);
-                Some(tx_hash)
-            }
-            None => None,
-        }
+impl PartialOrd for Transaction {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }

crates/mempool/src/priority_queue_test.rs

@@ -51,12 +51,12 @@ async fn test_priority_queue() {
     let tx1 = create_tx_for_testing(Tip(100), tx_hash_100);
     let tx3 = create_tx_for_testing(Tip(10), tx_hash_10);
 
-    let mut pq = PriorityQueue::new();
-    pq.push(tx1);
-    pq.push(tx2);
-    pq.push(tx3);
+    let mut pq = PriorityQueue::default();
+    pq.push(tx1.clone());
+    pq.push(tx2.clone());
+    pq.push(tx3.clone());
 
-    assert_eq!(pq.pop().unwrap(), tx_hash_100);
-    assert_eq!(pq.pop().unwrap(), tx_hash_50);
-    assert_eq!(pq.pop().unwrap(), tx_hash_10);
+    assert_eq!(pq.pop().unwrap(), tx1);
+    assert_eq!(pq.pop().unwrap(), tx2);
+    assert_eq!(pq.pop().unwrap(), tx3);
 }