From 134c192986300a2e17312d8abd8d1a0ddeef08ef Mon Sep 17 00:00:00 2001 From: yukang Date: Tue, 31 Oct 2023 18:40:27 +0800 Subject: [PATCH 1/4] add RBFReject for invalid cell from conflicted txs --- test/src/main.rs | 1 + test/src/specs/tx_pool/replace.rs | 72 ++++++++++++++++++++++++++++++- tx-pool/src/pool.rs | 12 ++++++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/test/src/main.rs b/test/src/main.rs index 030617b27c..a51b8a1a1b 100644 --- a/test/src/main.rs +++ b/test/src/main.rs @@ -471,6 +471,7 @@ fn all_specs() -> Vec> { Box::new(RbfTooManyDescendants), Box::new(RbfContainNewTx), Box::new(RbfContainInvalidInput), + Box::new(RbfContainInvalidCells), Box::new(RbfRejectReplaceProposed), Box::new(CompactBlockEmpty), Box::new(CompactBlockEmptyParentUnknown), diff --git a/test/src/specs/tx_pool/replace.rs b/test/src/specs/tx_pool/replace.rs index 0e37877801..3c0a4e1144 100644 --- a/test/src/specs/tx_pool/replace.rs +++ b/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) { + 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 diff --git a/tx-pool/src/pool.rs b/tx-pool/src/pool.rs index 20e3028e32..156a8e2d27 100644 --- a/tx-pool/src/pool.rs +++ b/tx-pool/src/pool.rs @@ -549,8 +549,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 +565,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; From 12b5c17a213d79db592168ed1936e88204947f32 Mon Sep 17 00:00:00 2001 From: yukang Date: Tue, 31 Oct 2023 17:04:15 +0800 Subject: [PATCH 2/4] add debug logs for txpool --- tx-pool/src/component/pool_map.rs | 8 +++++++- tx-pool/src/pool.rs | 3 +++ tx-pool/src/process.rs | 11 +++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/tx-pool/src/component/pool_map.rs b/tx-pool/src/component/pool_map.rs index a0830a42d4..dc573ab675 100644 --- a/tx-pool/src/component/pool_map.rs +++ b/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 { 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); } diff --git a/tx-pool/src/pool.rs b/tx-pool/src/pool.rs index 156a8e2d27..d64eb51c20 100644 --- a/tx-pool/src/pool.rs +++ b/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); diff --git a/tx-pool/src/process.rs b/tx-pool/src/process.rs index e9be766d56..bbf2ba1777 100644 --- a/tx-pool/src/process.rs +++ b/tx-pool/src/process.rs @@ -403,6 +403,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 +446,7 @@ impl TxPoolService { }); } Err(reject) => { + debug!("after_process {} reject: {} ", tx_hash, reject); if matches!( reject, Reject::Resolve(..) @@ -1081,19 +1083,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 +1153,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) From e8809cfe9dfb8d08224c478531d2b465e151e59f Mon Sep 17 00:00:00 2001 From: yukang Date: Tue, 31 Oct 2023 18:56:09 +0800 Subject: [PATCH 3/4] add debug log for rbf --- tx-pool/src/process.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tx-pool/src/process.rs b/tx-pool/src/process.rs index bbf2ba1777..f57cd6a6f7 100644 --- a/tx-pool/src/process.rs +++ b/tx-pool/src/process.rs @@ -129,6 +129,11 @@ 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() From a4b76fad7201882ba1e91d2bac757745465581b2 Mon Sep 17 00:00:00 2001 From: yukang Date: Tue, 31 Oct 2023 19:17:47 +0800 Subject: [PATCH 4/4] fix log for RBF --- tx-pool/src/process.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tx-pool/src/process.rs b/tx-pool/src/process.rs index f57cd6a6f7..b717e1b90b 100644 --- a/tx-pool/src/process.rs +++ b/tx-pool/src/process.rs @@ -135,8 +135,8 @@ impl TxPoolService { 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