Merge pull request #4215 from chenyukang/yukang-fix-112.x-issues

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/component/pool_map.rs

@@ -7,7 +7,7 @@ use crate::component::sort_key::{AncestorsScoreSortKey, EvictKey};
 use crate::error::Reject;
 use crate::TxEntry;
 
-use ckb_logger::trace;
+use ckb_logger::{debug, trace};
 use ckb_types::core::error::OutPointError;
 use ckb_types::packed::OutPoint;
 use ckb_types::prelude::*;
@@ -190,6 +190,11 @@ impl PoolMap {
 
     pub(crate) fn remove_entry(&mut self, id: &ProposalShortId) -> Option<TxEntry> {
         self.entries.remove_by_id(id).map(|entry| {
+            debug!(
+                "remove entry {} from status: {:?}",
+                entry.inner.transaction().hash(),
+                entry.status
+            );
             self.update_ancestors_index_key(&entry.inner, EntryOp::Remove);
             self.update_descendants_index_key(&entry.inner, EntryOp::Remove);
             self.remove_entry_edges(&entry.inner);
@@ -455,6 +460,7 @@ impl PoolMap {
             entry.add_ancestor_weight(&ancestor.inner);
         }
         if entry.ancestors_count > self.max_ancestors_count {
+            debug!("debug: exceeded maximum ancestors count");
             return Err(Reject::ExceededMaximumAncestorsCount);
         }
 

tx-pool/src/pool.rs

@@ -228,6 +228,7 @@ impl TxPool {
     fn remove_committed_tx(&mut self, tx: &TransactionView, callbacks: &Callbacks) {
         let short_id = tx.proposal_short_id();
         if let Some(entry) = self.pool_map.remove_entry(&short_id) {
+            debug!("remove_committed_tx for {}", tx.hash());
             callbacks.call_committed(self, &entry)
         }
         {
@@ -249,6 +250,8 @@ impl TxPool {
             .collect();
 
         for entry in removed {
+            let tx_hash = entry.transaction().hash();
+            debug!("remove_expired {} timestamp({})", tx_hash, entry.timestamp);
             self.pool_map.remove_entry(&entry.proposal_short_id());
             let reject = Reject::Expiry(entry.timestamp);
             callbacks.call_reject(self, &entry, reject);
@@ -549,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
@@ -563,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
@@ -403,6 +408,7 @@ impl TxPoolService {
                     self.process_orphan_tx(&tx).await;
                 }
                 Err(reject) => {
+                    debug!("after_process {} remote reject: {} ", tx_hash, reject);
                     if is_missing_input(reject) && all_inputs_is_unknown(snapshot, &tx) {
                         self.add_orphan(tx, peer, declared_cycle).await;
                     } else {
@@ -445,6 +451,7 @@ impl TxPoolService {
                         });
                     }
                     Err(reject) => {
+                        debug!("after_process {} reject: {} ", tx_hash, reject);
                         if matches!(
                             reject,
                             Reject::Resolve(..)
@@ -1081,19 +1088,23 @@ fn _submit_entry(
     entry: TxEntry,
     callbacks: &Callbacks,
 ) -> Result<(), Reject> {
+    let tx_hash = entry.transaction().hash();
     match status {
         TxStatus::Fresh => {
             if tx_pool.add_pending(entry.clone())? {
+                debug!("submit_entry pending {}", tx_hash);
                 callbacks.call_pending(tx_pool, &entry);
             }
         }
         TxStatus::Gap => {
             if tx_pool.add_gap(entry.clone())? {
+                debug!("submit_entry gap {}", tx_hash);
                 callbacks.call_pending(tx_pool, &entry);
             }
         }
         TxStatus::Proposed => {
             if tx_pool.add_proposed(entry.clone())? {
+                debug!("submit_entry proposed {}", tx_hash);
                 callbacks.call_proposed(tx_pool, &entry, true);
             }
         }
@@ -1147,6 +1158,11 @@ fn _update_tx_pool_for_reorg(
         for (id, entry) in proposals {
             debug!("begin to proposed: {:x}", id);
             if let Err(e) = tx_pool.proposed_rtx(&id) {
+                debug!(
+                    "Failed to add proposed tx {}, reason: {}",
+                    entry.transaction().hash(),
+                    e
+                );
                 callbacks.call_reject(tx_pool, &entry, e);
             } else {
                 callbacks.call_proposed(tx_pool, &entry, false)