Merge pull request #4216 from chenyukang/yukang-fix-rbf-invalid-cells

Backport: Add a check for RBF to avoid invalid cell deps

test/src/main.rs

@@ -471,6 +471,7 @@ fn all_specs() -> Vec<Box<dyn Spec>> {
         Box::new(RbfTooManyDescendants),
         Box::new(RbfContainNewTx),
         Box::new(RbfContainInvalidInput),
+        Box::new(RbfContainInvalidCells),
         Box::new(RbfRejectReplaceProposed),
         Box::new(CompactBlockEmpty),
         Box::new(CompactBlockEmptyParentUnknown),

test/src/specs/tx_pool/replace.rs

@@ -3,7 +3,7 @@ use ckb_jsonrpc_types::Status;
 use ckb_logger::info;
 use ckb_types::{
     core::{capacity_bytes, Capacity, TransactionView},
-    packed::{Byte32, CellInput, CellOutputBuilder, OutPoint},
+    packed::{Byte32, CellDep, CellInput, CellOutputBuilder, OutPoint},
     prelude::*,
 };
 
@@ -432,6 +432,76 @@ impl Spec for RbfContainInvalidInput {
     }
 }
 
+pub struct RbfContainInvalidCells;
+
+// RBF Rule, contains cell from conflicts txs
+impl Spec for RbfContainInvalidCells {
+    fn run(&self, nodes: &mut Vec<Node>) {
+        let node0 = &nodes[0];
+
+        node0.mine_until_out_bootstrap_period();
+
+        // build txs chain
+        let tx0 = node0.new_transaction_spend_tip_cellbase();
+        let mut txs = vec![tx0];
+        let max_count = 5;
+        while txs.len() <= max_count {
+            let parent = txs.last().unwrap();
+            let child = parent
+                .as_advanced_builder()
+                .set_inputs(vec![{
+                    CellInput::new_builder()
+                        .previous_output(OutPoint::new(parent.hash(), 0))
+                        .build()
+                }])
+                .set_outputs(vec![parent.output(0).unwrap()])
+                .build();
+            txs.push(child);
+        }
+        assert_eq!(txs.len(), max_count + 1);
+        // send Tx chain
+        for tx in txs[..=max_count - 1].iter() {
+            let ret = node0.rpc_client().send_transaction_result(tx.data().into());
+            assert!(ret.is_ok());
+        }
+
+        let clone_tx = txs[2].clone();
+        // Set tx2 fee to a higher value
+        let output2 = CellOutputBuilder::default()
+            .capacity(capacity_bytes!(70).pack())
+            .build();
+
+        // build a cell from conflicts txs's output
+        let cell = CellDep::new_builder()
+            .out_point(OutPoint::new(txs[2].hash(), 0))
+            .build();
+        let tx2 = clone_tx
+            .as_advanced_builder()
+            .set_inputs(vec![{
+                CellInput::new_builder()
+                    .previous_output(OutPoint::new(txs[1].hash(), 0))
+                    .build()
+            }])
+            .set_cell_deps(vec![cell])
+            .set_outputs(vec![output2])
+            .build();
+
+        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("new Tx contains cell deps from conflicts"));
+    }
+
+    fn modify_app_config(&self, config: &mut ckb_app_config::CKBAppConfig) {
+        config.tx_pool.min_rbf_rate = ckb_types::core::FeeRate(1500);
+    }
+}
+
 pub struct RbfRejectReplaceProposed;
 
 // RBF Rule #6

tx-pool/src/pool.rs

@@ -552,8 +552,10 @@ impl TxPool {
 
         // Rule #2, new tx don't contain any new unconfirmed inputs
         let mut inputs = HashSet::new();
+        let mut outputs = HashSet::new();
         for c in conflicts.iter() {
             inputs.extend(c.inner.transaction().input_pts_iter());
+            outputs.extend(c.inner.transaction().output_pts_iter());
         }
 
         if rtx
@@ -566,6 +568,16 @@ impl TxPool {
             ));
         }
 
+        if rtx
+            .transaction
+            .cell_deps_iter()
+            .any(|dep| outputs.contains(&dep.out_point()))
+        {
+            return Err(Reject::RBFRejected(
+                "new Tx contains cell deps from conflicts".to_string(),
+            ));
+        }
+
         // Rule #5, the replaced tx's descendants can not more than 100
         // and the ancestor of the new tx don't have common set with the replaced tx's descendants
         let mut replace_count: usize = 0;

tx-pool/src/process.rs

@@ -129,9 +129,14 @@ impl TxPoolService {
                 for id in conflicts.iter() {
                     let removed = tx_pool.pool_map.remove_entry_and_descendants(id);
                     for old in removed {
+                        debug!(
+                            "remove conflict tx {} for RBF by new tx {}",
+                            old.transaction().hash(),
+                            entry.transaction().hash()
+                        );
                         let reject = Reject::RBFRejected(format!(
-                            "replaced by {}",
-                            entry.proposal_short_id()
+                            "replaced by tx {}",
+                            entry.transaction().hash()
                         ));
                         // remove old tx from tx_pool, not happened in service so we didn't call reject callbacks
                         // here we call them manually