From dc2daef83876d70311895e37294012177931d03d Mon Sep 17 00:00:00 2001 From: yukang Date: Thu, 7 Dec 2023 15:32:18 +0800 Subject: [PATCH] code clean and comments feedback --- tx-pool/src/component/edges.rs | 28 ++++++++++++++++++++++------ tx-pool/src/component/pool_map.rs | 14 +++++++------- tx-pool/src/pool.rs | 24 +++++++++++------------- tx-pool/src/process.rs | 4 ++-- 4 files changed, 42 insertions(+), 28 deletions(-) diff --git a/tx-pool/src/component/edges.rs b/tx-pool/src/component/edges.rs index 87d4e3cc52..76f33055b4 100644 --- a/tx-pool/src/component/edges.rs +++ b/tx-pool/src/component/edges.rs @@ -1,5 +1,7 @@ -use ckb_logger::error; -use ckb_types::packed::{Byte32, OutPoint, ProposalShortId}; +use ckb_types::{ + core::tx_pool::Reject, + packed::{Byte32, OutPoint, ProposalShortId}, +}; use std::collections::{hash_map::Entry, HashMap, HashSet}; #[derive(Default, Debug, Clone)] @@ -28,12 +30,26 @@ impl Edges { self.deps.len() } - pub(crate) fn insert_input(&mut self, out_point: OutPoint, txid: ProposalShortId) { + pub(crate) fn insert_input( + &mut self, + out_point: OutPoint, + txid: ProposalShortId, + ) -> Result<(), Reject> { // inputs is occupied means double speanding happened here - if self.inputs.contains_key(&out_point) { - error!("double spending happened {:?} {:?}", out_point, txid); + match self.inputs.entry(out_point.clone()) { + Entry::Occupied(occupied) => { + let msg = + format!( + "txpool unexpected double-spending out_point: {:?} old_tx: {:?} new_tx: {:?}", + out_point, occupied.get(), txid + ); + Err(Reject::RBFRejected(msg)) + } + Entry::Vacant(vacant) => { + vacant.insert(txid); + Ok(()) + } } - self.inputs.insert(out_point, txid); } pub(crate) fn remove_input(&mut self, out_point: &OutPoint) -> Option { diff --git a/tx-pool/src/component/pool_map.rs b/tx-pool/src/component/pool_map.rs index 84d143971a..a4c7d702dc 100644 --- a/tx-pool/src/component/pool_map.rs +++ b/tx-pool/src/component/pool_map.rs @@ -189,8 +189,8 @@ impl PoolMap { } trace!("pool_map.add_{:?} {}", status, entry.transaction().hash()); self.check_and_record_ancestors(&mut entry)?; + self.record_entry_edges(&entry)?; self.insert_entry(&entry, status); - self.record_entry_edges(&entry); self.record_entry_descendants(&entry); self.track_entry_statics(); Ok(true) @@ -263,10 +263,9 @@ impl PoolMap { conflicts } - pub(crate) fn find_conflict_tx(&self, tx_inputs: &[OutPoint]) -> HashSet { - tx_inputs - .iter() - .filter_map(|out_point| self.edges.get_input_ref(out_point).cloned()) + pub(crate) fn find_conflict_tx(&self, tx: &TransactionView) -> HashSet { + tx.input_pts_iter() + .filter_map(|out_point| self.edges.get_input_ref(&out_point).cloned()) .collect() } @@ -390,7 +389,7 @@ impl PoolMap { } } - fn record_entry_edges(&mut self, entry: &TxEntry) { + fn record_entry_edges(&mut self, entry: &TxEntry) -> Result<(), Reject> { let tx_short_id: ProposalShortId = entry.proposal_short_id(); let header_deps = entry.transaction().header_deps(); let related_dep_out_points: Vec<_> = entry.related_dep_out_points().cloned().collect(); @@ -399,7 +398,7 @@ impl PoolMap { // if input reference a in-pool output, connect it // otherwise, record input for conflict check for i in inputs { - self.edges.insert_input(i.to_owned(), tx_short_id.clone()); + self.edges.insert_input(i.to_owned(), tx_short_id.clone())?; } // record dep-txid @@ -412,6 +411,7 @@ impl PoolMap { .header_deps .insert(tx_short_id, header_deps.into_iter().collect()); } + Ok(()) } fn record_entry_descendants(&mut self, entry: &TxEntry) { diff --git a/tx-pool/src/pool.rs b/tx-pool/src/pool.rs index cbd3b098d8..2f2e5452df 100644 --- a/tx-pool/src/pool.rs +++ b/tx-pool/src/pool.rs @@ -524,28 +524,25 @@ impl TxPool { (entries, size, cycles) } - pub(crate) fn find_conflict_tx(&self, txv: &TransactionView) -> HashSet { - let tx_inputs: Vec = txv.input_pts_iter().collect(); - self.pool_map.find_conflict_tx(&tx_inputs) - } - pub(crate) fn check_rbf( &self, snapshot: &Snapshot, - txv: &TransactionView, - fee: Capacity, - tx_size: usize, + entry: &TxEntry, ) -> Result, Reject> { assert!(self.enable_rbf()); - let tx_inputs: Vec = txv.input_pts_iter().collect(); - let conflict_ids = self.pool_map.find_conflict_tx(&tx_inputs); + let tx_inputs: Vec = entry.transaction().input_pts_iter().collect(); + let conflict_ids = self.pool_map.find_conflict_tx(entry.transaction()); if conflict_ids.is_empty() { return Ok(conflict_ids); } - let tx_cells_deps: Vec = txv.cell_deps_iter().map(|c| c.out_point()).collect(); - let short_id = txv.proposal_short_id(); + let tx_cells_deps: Vec = entry + .transaction() + .cell_deps_iter() + .map(|c| c.out_point()) + .collect(); + let short_id = entry.proposal_short_id(); // Rule #1, the node has enabled RBF, which is checked by caller let conflicts = conflict_ids @@ -556,7 +553,8 @@ impl TxPool { // Rule #4, new tx's fee need to higher than min_rbf_fee computed from the tx_pool configuration // Rule #3, new tx's fee need to higher than conflicts, here we only check the root tx - if let Some(min_replace_fee) = self.calculate_min_replace_fee(&conflicts, tx_size) { + let fee = entry.fee; + if let Some(min_replace_fee) = self.calculate_min_replace_fee(&conflicts, entry.size) { if fee < min_replace_fee { return Err(Reject::RBFRejected(format!( "Tx's current fee is {}, expect it to >= {} to replace old txs", diff --git a/tx-pool/src/process.rs b/tx-pool/src/process.rs index 5d9159353d..69935cb64d 100644 --- a/tx-pool/src/process.rs +++ b/tx-pool/src/process.rs @@ -108,7 +108,7 @@ impl TxPoolService { // here we double confirm RBF rules before insert entry // check_rbf must be invoked in `write` lock to avoid concurrent issues. let conflicts = if tx_pool.enable_rbf() { - tx_pool.check_rbf(&snapshot, entry.transaction(), entry.fee, entry.size)? + tx_pool.check_rbf(&snapshot, &entry)? } else { HashSet::new() }; @@ -249,7 +249,7 @@ impl TxPoolService { // Try an RBF cheap check, here if the tx is resolved as Dead, // we assume there must be conflicted happened in txpool now, // if there is no conflicted transactions reject it - let conflicts = tx_pool.find_conflict_tx(&rtx.transaction); + let conflicts = tx_pool.pool_map.find_conflict_tx(&rtx.transaction); if conflicts.is_empty() { error!( "{} is resolved as Dead, but there is no conflicted tx",