code clean and comments feedback

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<ProposalShortId> {

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<ProposalShortId> {
-        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<ProposalShortId> {
+        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) {

tx-pool/src/pool.rs

@@ -524,28 +524,25 @@ impl TxPool {
         (entries, size, cycles)
     }
 
-    pub(crate) fn find_conflict_tx(&self, txv: &TransactionView) -> HashSet<ProposalShortId> {
-        let tx_inputs: Vec<OutPoint> = 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<HashSet<ProposalShortId>, Reject> {
         assert!(self.enable_rbf());
-        let tx_inputs: Vec<OutPoint> = txv.input_pts_iter().collect();
-        let conflict_ids = self.pool_map.find_conflict_tx(&tx_inputs);
+        let tx_inputs: Vec<OutPoint> = 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<OutPoint> = txv.cell_deps_iter().map(|c| c.out_point()).collect();
-        let short_id = txv.proposal_short_id();
+        let tx_cells_deps: Vec<OutPoint> = 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",

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",