Fix/2.5.0.0.7 rbf logic

CHANGELOG.md

@@ -17,7 +17,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
     - `get-tenure-info?` added
     - `get-block-info?` removed
 
-## [2.5.0.0.7]
+## [2.5.0.0.8]
 
 ### Added
 
@@ -33,6 +33,12 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 - Multi miner fixes jude (#5040)
 - Remove spurious deadlock condition whenever the sortition DB is opened
 
+## [2.5.0.0.7]
+
+### Fixed
+
+- Fix the RBF logic in the mempool admission which did not previously handle sponsor and origin nonce interactions correctly.
+
 ## [2.5.0.0.6]
 
 ### Changed

stackslib/src/core/mempool.rs

@@ -916,7 +916,7 @@ impl<'a> MemPoolTx<'a> {
         &mut self,
         coinbase_height: u64,
         txid: &Txid,
-        prior_txid: Option<Txid>,
+        prior_txids: &[Txid],
     ) -> Result<Option<Txid>, MemPoolRejection> {
         // is this the first-ever txid at this coinbase height?
         let sql = "SELECT 1 FROM mempool WHERE height = ?1";
@@ -931,7 +931,7 @@ impl<'a> MemPoolTx<'a> {
 
         MemPoolTx::with_bloom_state(self, |ref mut dbtx, ref mut bloom_counter| {
             // remove replaced transaction
-            if let Some(prior_txid) = prior_txid {
+            for prior_txid in prior_txids.iter() {
                 bloom_counter.remove_raw(dbtx, &prior_txid.0)?;
             }
 
@@ -2075,6 +2075,17 @@ impl MemPoolDB {
     ) -> Result<(), MemPoolRejection> {
         let length = tx_bytes.len() as u64;
 
+        // don't allow a self sponsor with different nonces
+        if origin_address == sponsor_address && origin_nonce != sponsor_nonce {
+            info!("TX conflicts with own sponsor/origin nonces";
+                  "new_txid" => %txid,
+                  "origin_addr" => %origin_address,
+                  "origin_nonce" => origin_nonce,
+                  "sponsor_addr" => %sponsor_address,
+                  "sponsor_nonce" => sponsor_nonce);
+            return Err(MemPoolRejection::ConflictingNonceInMempool);
+        }
+
         // this transaction is said to arrive during this _tenure_, not during this _block_.
         // In epoch 2.x, these are the same as `tip_consensus_hash` and `tip_block_header_hash`.
         // In Nakamoto, they may be different.
@@ -2102,71 +2113,97 @@ impl MemPoolDB {
         };
 
         // do we already have txs with either the same origin nonce or sponsor nonce ?
-        let prior_tx = {
-            match MemPoolDB::get_tx_metadata_by_address(tx, true, origin_address, origin_nonce)? {
-                Some(prior_tx) => Some(prior_tx),
-                None => MemPoolDB::get_tx_metadata_by_address(
-                    tx,
-                    false,
-                    sponsor_address,
-                    sponsor_nonce,
-                )?,
+        let mut prior_txs: Vec<MemPoolTxMetadata> = vec![];
+        let prior_txs_to_check = [
+            MemPoolDB::get_tx_metadata_by_address(tx, true, origin_address, origin_nonce)?,
+            MemPoolDB::get_tx_metadata_by_address(tx, false, origin_address, origin_nonce)?,
+            MemPoolDB::get_tx_metadata_by_address(tx, true, sponsor_address, sponsor_nonce)?,
+            MemPoolDB::get_tx_metadata_by_address(tx, false, sponsor_address, sponsor_nonce)?,
+        ];
+        for prior_tx_opt in prior_txs_to_check {
+            let Some(prior_tx) = prior_tx_opt else {
+                continue;
+            };
+            // only add if not already found before
+            let found_before = prior_txs
+                .iter()
+                .find(|already_in_list| already_in_list.txid == prior_tx.txid)
+                .is_some();
+            if !found_before {
+                prior_txs.push(prior_tx);
             }
-        };
+        }
 
         let mut replace_reason = MemPoolDropReason::REPLACE_BY_FEE;
 
-        // if so, is this a replace-by-fee? or a replace-in-chain-tip?
-        let add_tx = if let Some(ref prior_tx) = prior_tx {
-            if tx_fee > prior_tx.tx_fee {
-                // is this a replace-by-fee ?
-                debug!(
-                    "Can replace {} with {} for {},{} by fee ({} < {})",
-                    &prior_tx.txid, &txid, origin_address, origin_nonce, &prior_tx.tx_fee, &tx_fee
-                );
-                replace_reason = MemPoolDropReason::REPLACE_BY_FEE;
-                true
-            } else if !MemPoolDB::are_blocks_in_same_fork(
-                chainstate,
-                &prior_tx.tenure_consensus_hash,
-                &prior_tx.tenure_block_header_hash,
-                &consensus_hash,
-                &block_header_hash,
-            )? {
-                // is this a replace-across-fork ?
-                debug!(
-                    "Can replace {} with {} for {},{} across fork",
-                    &prior_tx.txid, &txid, origin_address, origin_nonce
-                );
-                replace_reason = MemPoolDropReason::REPLACE_ACROSS_FORK;
-                true
-            } else {
-                // there's a >= fee tx in this fork, cannot add
-                info!("TX conflicts with sponsor/origin nonce in same fork with >= fee";
-                      "new_txid" => %txid,
-                      "old_txid" => %prior_tx.txid,
-                      "origin_addr" => %origin_address,
-                      "origin_nonce" => origin_nonce,
-                      "sponsor_addr" => %sponsor_address,
-                      "sponsor_nonce" => sponsor_nonce,
-                      "new_fee" => tx_fee,
-                      "old_fee" => prior_tx.tx_fee);
-                false
-            }
-        } else {
-            // no conflicting TX with this origin/sponsor, go ahead and add
+        let add_tx = if prior_txs.len() == 0 {
             true
+        } else {
+            let mut all_passed = true;
+            for prior_tx in prior_txs.iter() {
+                if tx_fee > prior_tx.tx_fee {
+                    // is this a replace-by-fee ?
+                    debug!("Can replace TX by fee";
+                           "new_txid" => %txid,
+                           "old_txid" => %prior_tx.txid,
+                           "origin_addr" => %origin_address,
+                           "origin_nonce" => origin_nonce,
+                           "sponsor_addr" => %sponsor_address,
+                           "sponsor_nonce" => sponsor_nonce,
+                           "new_fee" => tx_fee,
+                           "old_fee" => prior_tx.tx_fee);
+                    replace_reason = MemPoolDropReason::REPLACE_BY_FEE;
+                } else if !MemPoolDB::are_blocks_in_same_fork(
+                    chainstate,
+                    &prior_tx.tenure_consensus_hash,
+                    &prior_tx.tenure_block_header_hash,
+                    &consensus_hash,
+                    &block_header_hash,
+                )? {
+                    // is this a replace-across-fork ?
+                    debug!("Can replace TX across fork";
+                           "new_txid" => %txid,
+                           "old_txid" => %prior_tx.txid,
+                           "origin_addr" => %origin_address,
+                           "origin_nonce" => origin_nonce,
+                           "sponsor_addr" => %sponsor_address,
+                           "sponsor_nonce" => sponsor_nonce,
+                           "new_fee" => tx_fee,
+                           "old_fee" => prior_tx.tx_fee);
+                    replace_reason = MemPoolDropReason::REPLACE_ACROSS_FORK;
+                } else {
+                    // there's a >= fee tx in this fork, cannot add
+                    info!("TX conflicts with sponsor/origin nonce in same fork with >= fee";
+                          "new_txid" => %txid,
+                          "old_txid" => %prior_tx.txid,
+                          "origin_addr" => %origin_address,
+                          "origin_nonce" => origin_nonce,
+                          "sponsor_addr" => %sponsor_address,
+                          "sponsor_nonce" => sponsor_nonce,
+                          "new_fee" => tx_fee,
+                          "old_fee" => prior_tx.tx_fee);
+                    all_passed = false;
+                }
+            }
+            all_passed
         };
 
         if !add_tx {
             return Err(MemPoolRejection::ConflictingNonceInMempool);
         }
 
-        tx.update_bloom_counter(
-            coinbase_height,
-            &txid,
-            prior_tx.as_ref().map(|tx| tx.txid.clone()),
-        )?;
+        let prior_txids: Vec<_> = prior_txs.iter().map(|tx| tx.txid.clone()).collect();
+        tx.update_bloom_counter(coinbase_height, &txid, &prior_txids)?;
+
+        debug!("Inserting TX into mempool";
+               "txid" => %txid,
+               "replaced_txids" => ?prior_txids);
+
+        let sql = "DELETE FROM mempool WHERE txid = ?";
+        for txid in prior_txids.iter() {
+            tx.execute(sql, &[txid])
+                .map_err(|e| MemPoolRejection::DBError(db_error::SqliteError(e)))?;
+        }
 
         let sql = "INSERT OR REPLACE INTO mempool (
             txid,
@@ -2204,8 +2241,10 @@ impl MemPoolDB {
         tx.update_mempool_pager(&txid)?;
 
         // broadcast drop event if a tx is being replaced
-        if let (Some(prior_tx), Some(event_observer)) = (prior_tx, event_observer) {
-            event_observer.mempool_txs_dropped(vec![prior_tx.txid], replace_reason);
+        if let Some(event_observer) = event_observer {
+            if !prior_txids.is_empty() {
+                event_observer.mempool_txs_dropped(prior_txids, replace_reason);
+            }
         };
 
         Ok(())

stackslib/src/core/tests/mod.rs

@@ -1408,15 +1408,16 @@ fn mempool_db_load_store_replace_tx(#[case] behavior: MempoolCollectionBehavior)
     let mut mempool_tx = mempool.tx_begin().unwrap();
 
     eprintln!("add all txs");
-    for (i, mut tx) in txs.drain(..).enumerate() {
+    let txs_len = txs.len();
+    for (ix, mut tx) in txs.drain(..).enumerate() {
         // make sure each address is unique per tx (not the case in codec_all_transactions)
         let origin_address = StacksAddress {
             version: 22,
-            bytes: Hash160::from_data(&i.to_be_bytes()),
+            bytes: Hash160::from_data(&ix.to_be_bytes()),
         };
         let sponsor_address = StacksAddress {
             version: 22,
-            bytes: Hash160::from_data(&(i + 1).to_be_bytes()),
+            bytes: Hash160::from_data(&(ix + txs_len).to_be_bytes()),
         };
 
         tx.set_tx_fee(123);
@@ -2838,3 +2839,107 @@ fn test_filter_txs_by_type() {
         },
     );
 }
+
+#[test]
+/// Test the handling of Origin/Sponsor RBF interactions
+///  when origin and sponsor interact across two txs.
+fn mempool_db_rbf_origin_sponsor() {
+    let mut chainstate = instantiate_chainstate(false, 0x80000000, function_name!());
+    let chainstate_path = chainstate_path(function_name!());
+    let mut mempool = MemPoolDB::open_test(false, 0x80000000, &chainstate_path).unwrap();
+
+    // create initial transaction
+    let mut mempool_tx = mempool.tx_begin().unwrap();
+    let height = 100;
+
+    let make_tx = |o_h160: &Hash160, o_nonce, s_h160: &Hash160, s_nonce, fee| {
+        let origin_condition =
+            TransactionSpendingCondition::Singlesig(SinglesigSpendingCondition {
+                signer: o_h160.clone(),
+                hash_mode: SinglesigHashMode::P2PKH,
+                key_encoding: TransactionPublicKeyEncoding::Compressed,
+                nonce: o_nonce,
+                tx_fee: 0,
+                signature: MessageSignature::from_raw(&vec![0xff; 65]),
+            });
+        let sponsor_condition =
+            TransactionSpendingCondition::Singlesig(SinglesigSpendingCondition {
+                signer: s_h160.clone(),
+                hash_mode: SinglesigHashMode::P2PKH,
+                key_encoding: TransactionPublicKeyEncoding::Compressed,
+                nonce: s_nonce,
+                tx_fee: fee,
+                signature: MessageSignature::from_raw(&vec![0xff; 65]),
+            });
+        let stx_address = StacksAddress {
+            version: 1,
+            bytes: Hash160([0xff; 20]),
+        };
+        let payload = TransactionPayload::TokenTransfer(
+            PrincipalData::from(QualifiedContractIdentifier {
+                issuer: stx_address.into(),
+                name: "hello-contract-name".into(),
+            }),
+            123,
+            TokenTransferMemo([0u8; 34]),
+        );
+        StacksTransaction {
+            version: TransactionVersion::Testnet,
+            chain_id: 0x80000000,
+            auth: TransactionAuth::Sponsored(origin_condition, sponsor_condition),
+            anchor_mode: TransactionAnchorMode::Any,
+            post_condition_mode: TransactionPostConditionMode::Allow,
+            post_conditions: Vec::new(),
+            payload,
+        }
+    };
+
+    let p_0 = Hash160::from_data(&[0; 16]);
+    let p_1 = Hash160::from_data(&[1; 16]);
+
+    let tx_1 = make_tx(&p_0, 1, &p_1, 2, 100);
+    let tx_2 = make_tx(&p_1, 2, &p_0, 1, 110);
+
+    assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_1.txid()).unwrap());
+    assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_2.txid()).unwrap());
+    MemPoolDB::try_add_tx(
+        &mut mempool_tx,
+        &mut chainstate,
+        &ConsensusHash([0x1; 20]),
+        &BlockHeaderHash([0x2; 32]),
+        false,
+        tx_1.txid(),
+        tx_1.serialize_to_vec(),
+        tx_1.get_tx_fee(),
+        height,
+        &tx_1.origin_address(),
+        tx_1.get_origin_nonce(),
+        tx_1.sponsor_address().as_ref().unwrap(),
+        tx_1.get_sponsor_nonce().unwrap(),
+        None,
+    )
+    .unwrap();
+
+    assert!(MemPoolDB::db_has_tx(&mempool_tx, &tx_1.txid()).unwrap());
+    assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_2.txid()).unwrap());
+
+    MemPoolDB::try_add_tx(
+        &mut mempool_tx,
+        &mut chainstate,
+        &ConsensusHash([0x1; 20]),
+        &BlockHeaderHash([0x2; 32]),
+        false,
+        tx_2.txid(),
+        tx_2.serialize_to_vec(),
+        tx_2.get_tx_fee(),
+        height,
+        &tx_2.origin_address(),
+        tx_2.get_origin_nonce(),
+        tx_2.sponsor_address().as_ref().unwrap(),
+        tx_2.get_sponsor_nonce().unwrap(),
+        None,
+    )
+    .unwrap();
+    assert!(MemPoolDB::db_has_tx(&mempool_tx, &tx_2.txid()).unwrap());
+    assert!(!MemPoolDB::db_has_tx(&mempool_tx, &tx_1.txid()).unwrap());
+}

stackslib/src/net/prune.rs

@@ -199,9 +199,11 @@ impl PeerNetwork {
             match org_neighbors.get_mut(&org) {
                 None => {}
                 Some(ref mut neighbor_infos) => {
-                    neighbor_infos.sort_by(|&(ref _nk1, ref stats1), &(ref _nk2, ref stats2)| {
-                        PeerNetwork::compare_neighbor_uptime_health(stats1, stats2)
-                    });
+                    neighbor_infos.sort_unstable_by(
+                        |&(ref _nk1, ref stats1), &(ref _nk2, ref stats2)| {
+                            PeerNetwork::compare_neighbor_uptime_health(stats1, stats2)
+                        },
+                    );
                 }
             }
         }