Consistent TransactionPriorityId ordering/equality (#2832)

anza-xyz · Sep 5, 2024 · e196a08 · e196a08
1 parent 1ab8e07
commit e196a08
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 24 deletions.
diff --git a/core/src/banking_stage/scheduler_messages.rs b/core/src/banking_stage/scheduler_messages.rs
@@ -21,7 +21,7 @@ impl Display for TransactionBatchId {
 }
 
 /// A unique identifier for a transaction.
-#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub struct TransactionId(u64);
 
 impl TransactionId {

diff --git a/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs b/core/src/banking_stage/transaction_scheduler/scheduler_controller.rs
@@ -858,7 +858,7 @@ mod tests {
             &Keypair::new(),
             &Pubkey::new_unique(),
             1,
-            1,
+            1000,
             bank.last_blockhash(),
         );
         let tx2 = create_and_fund_prioritized_transfer(
@@ -867,7 +867,7 @@ mod tests {
             &Keypair::new(),
             &Pubkey::new_unique(),
             1,
-            2,
+            2000,
             bank.last_blockhash(),
         );
         let tx1_hash = tx1.message().hash();
@@ -914,7 +914,7 @@ mod tests {
             &Keypair::new(),
             &pk,
             1,
-            1,
+            1000,
             bank.last_blockhash(),
         );
         let tx2 = create_and_fund_prioritized_transfer(
@@ -923,7 +923,7 @@ mod tests {
             &Keypair::new(),
             &pk,
             1,
-            2,
+            2000,
             bank.last_blockhash(),
         );
         let tx1_hash = tx1.message().hash();
@@ -1105,7 +1105,7 @@ mod tests {
             &Keypair::new(),
             &Pubkey::new_unique(),
             1,
-            1,
+            1000,
             bank.last_blockhash(),
         );
         let tx2 = create_and_fund_prioritized_transfer(
@@ -1114,7 +1114,7 @@ mod tests {
             &Keypair::new(),
             &Pubkey::new_unique(),
             1,
-            2,
+            2000,
             bank.last_blockhash(),
         );
         let tx1_hash = tx1.message().hash();

diff --git a/core/src/banking_stage/transaction_scheduler/transaction_id_generator.rs b/core/src/banking_stage/transaction_scheduler/transaction_id_generator.rs
@@ -1,16 +1,21 @@
 use crate::banking_stage::scheduler_messages::TransactionId;
 
-/// Simple sequential ID generator for `TransactionId`s.
+/// Simple reverse-sequential ID generator for `TransactionId`s.
 /// These IDs uniquely identify transactions during the scheduling process.
-#[derive(Default)]
 pub struct TransactionIdGenerator {
     next_id: u64,
 }
 
+impl Default for TransactionIdGenerator {
+    fn default() -> Self {
+        Self { next_id: u64::MAX }
+    }
+}
+
 impl TransactionIdGenerator {
     pub fn next(&mut self) -> TransactionId {
         let id = self.next_id;
-        self.next_id = self.next_id.wrapping_add(1);
+        self.next_id = self.next_id.wrapping_sub(1);
         TransactionId::new(id)
     }
 }
diff --git a/core/src/banking_stage/transaction_scheduler/transaction_priority_id.rs b/core/src/banking_stage/transaction_scheduler/transaction_priority_id.rs
@@ -5,8 +5,7 @@ use {
 };
 
 /// A unique identifier tied with priority ordering for a transaction/packet:
-///     - `id` has no effect on ordering
-#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub(crate) struct TransactionPriorityId {
     pub(crate) priority: u64,
     pub(crate) id: TransactionId,
@@ -18,18 +17,6 @@ impl TransactionPriorityId {
     }
 }
 
-impl Ord for TransactionPriorityId {
-    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-        self.priority.cmp(&other.priority)
-    }
-}
-
-impl PartialOrd for TransactionPriorityId {
-    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
 impl Hash for TransactionPriorityId {
     fn hash<H: Hasher>(&self, state: &mut H) {
         self.id.hash(state)
@@ -41,3 +28,42 @@ impl TopLevelId<Self> for TransactionPriorityId {
         *self
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_transaction_priority_id_ordering() {
+        // Higher priority first
+        {
+            let id1 = TransactionPriorityId::new(1, TransactionId::new(1));
+            let id2 = TransactionPriorityId::new(2, TransactionId::new(1));
+            assert!(id1 < id2);
+            assert!(id1 <= id2);
+            assert!(id2 > id1);
+            assert!(id2 >= id1);
+        }
+
+        // Equal priority then compare by id
+        {
+            let id1 = TransactionPriorityId::new(1, TransactionId::new(1));
+            let id2 = TransactionPriorityId::new(1, TransactionId::new(2));
+            assert!(id1 < id2);
+            assert!(id1 <= id2);
+            assert!(id2 > id1);
+            assert!(id2 >= id1);
+        }
+
+        // Equal priority and id
+        {
+            let id1 = TransactionPriorityId::new(1, TransactionId::new(1));
+            let id2 = TransactionPriorityId::new(1, TransactionId::new(1));
+            assert_eq!(id1, id2);
+            assert!(id1 >= id2);
+            assert!(id1 <= id2);
+            assert!(id2 >= id1);
+            assert!(id2 <= id1);
+        }
+    }
+}