From aef14eee0ebce7d54e9dbbb18309107d75dc5685 Mon Sep 17 00:00:00 2001
From: yukang <moorekang@gmail.com>
Date: Wed, 15 Nov 2023 11:35:25 +0800
Subject: [PATCH] RBF will also replace tx not in Pending

---
 test/src/specs/tx_pool/replace.rs | 28 +++++++++++++++++++++-------
 tx-pool/src/pool.rs               | 19 ++++++-------------
 2 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/test/src/specs/tx_pool/replace.rs b/test/src/specs/tx_pool/replace.rs
index 3c0a4e1144..6b4371aa90 100644
--- a/test/src/specs/tx_pool/replace.rs
+++ b/test/src/specs/tx_pool/replace.rs
@@ -505,6 +505,7 @@ impl Spec for RbfContainInvalidCells {
 pub struct RbfRejectReplaceProposed;
 
 // RBF Rule #6
+// We removed rule #6, even tx in `Gap` and `Proposed` status can be replaced.
 impl Spec for RbfRejectReplaceProposed {
     fn run(&self, nodes: &mut Vec<Node>) {
         let node0 = &nodes[0];
@@ -557,29 +558,42 @@ impl Spec for RbfRejectReplaceProposed {
             .capacity(capacity_bytes!(70).pack())
             .build();
 
+        let tx1_hash = txs[2].hash();
         let tx2 = clone_tx
             .as_advanced_builder()
             .set_outputs(vec![output2])
             .build();
 
+        eprintln!("begin to RBF ......");
         let res = node0
             .rpc_client()
             .send_transaction_result(tx2.data().into());
-        assert!(res.is_err(), "tx2 should be rejected");
-        assert!(res
-            .err()
-            .unwrap()
-            .to_string()
-            .contains("all conflict Txs should be in Pending status"));
+        assert!(res.is_ok());
+
+        let old_tx_status = node0.rpc_client().get_transaction(tx1_hash).tx_status;
+        assert_eq!(old_tx_status.status, Status::Rejected);
+        assert!(old_tx_status.reason.unwrap().contains("RBFRejected"));
+
+        let tx2_status = node0.rpc_client().get_transaction(tx2.hash()).tx_status;
+        assert_eq!(tx2_status.status, Status::Pending);
 
-        // when tx1 was confirmed, tx2 should be rejected
         let window_count = node0.consensus().tx_proposal_window().closest();
         node0.mine(window_count);
+        // since old tx is already in BlockAssembler,
+        // tx1 will be committed, even it is not in tx_pool and with `Rejected` status now
         let ret = wait_until(20, || {
             let res = rpc_client0.get_transaction(txs[2].hash());
             res.tx_status.status == Status::Committed
         });
         assert!(ret, "tx1 should be committed");
+        let tx1_status = node0.rpc_client().get_transaction(txs[2].hash()).tx_status;
+        assert_eq!(tx1_status.status, Status::Committed);
+
+        // tx2 will be marked as `Rejected` because callback of `remove_committed_txs` from tx1
+        let tx2_status = node0.rpc_client().get_transaction(tx2.hash()).tx_status;
+        assert_eq!(tx2_status.status, Status::Rejected);
+
+        // the same tx2 can not be sent again
         let res = node0
             .rpc_client()
             .send_transaction_result(tx2.data().into());
diff --git a/tx-pool/src/pool.rs b/tx-pool/src/pool.rs
index d64eb51c20..ab7b6863be 100644
--- a/tx-pool/src/pool.rs
+++ b/tx-pool/src/pool.rs
@@ -234,6 +234,11 @@ impl TxPool {
         {
             let conflicts = self.pool_map.resolve_conflict(tx);
             for (entry, reject) in conflicts {
+                debug!(
+                    "removed {} for commited: {}",
+                    entry.transaction().hash(),
+                    tx.hash()
+                );
                 callbacks.call_reject(self, &entry, reject);
             }
         }
@@ -525,6 +530,7 @@ impl TxPool {
         fee: Capacity,
         tx_size: usize,
     ) -> Result<(), Reject> {
+        // Rule #1, the node has enabled RBF, which is checked by caller
         assert!(self.enable_rbf());
         assert!(!conflict_ids.is_empty());
 
@@ -615,19 +621,6 @@ impl TxPool {
                     ));
                 }
             }
-
-            let mut entries_status = entries.iter().map(|e| e.status).collect::<Vec<_>>();
-            entries_status.push(conflict.status);
-            // Rule #6, all conflict Txs should be in `Pending` or `Gap` status
-            if entries_status
-                .iter()
-                .any(|s| ![Status::Pending, Status::Gap].contains(s))
-            {
-                // Here we only refer to `Pending` status, since `Gap` is an internal status
-                return Err(Reject::RBFRejected(
-                    "all conflict Txs should be in Pending status".to_string(),
-                ));
-            }
         }
 
         Ok(())