Merge pull request #4315 from chenyukang/yukang-fix-celldep-limit

Don't account for cell dep for MAX_ANCESTORS_COUNT

rpc/src/tests/module/pool.rs

@@ -184,7 +184,8 @@ fn test_send_transaction_exceeded_maximum_ancestors_count() {
         });
     }
 
-    // the default value of pool config `max_ancestors_count` is 125, only 125 txs will be added to committed list of the block template
+    // the default value of pool config `max_ancestors_count` is 125,
+    // only 125 txs will be added to committed list of the block template
     suite.wait_block_template_array_ge("transactions", 1);
 
     let response = suite.rpc(&RpcTestRequest {

test/src/main.rs

@@ -488,6 +488,7 @@ fn all_specs() -> Vec<Box<dyn Spec>> {
         Box::new(CompactBlockRelayLessThenSharedBestKnown),
         Box::new(InvalidLocatorSize),
         Box::new(SizeLimit),
+        Box::new(TxPoolLimitAncestorCount),
         Box::new(SendDefectedBinary::new(
             "send_defected_binary_reject_known_bugs",
             true,

test/src/specs/tx_pool/limit.rs

@@ -1,9 +1,16 @@
-use crate::{Node, Spec};
-
+use crate::{
+    util::{cell::gen_spendable, transaction::always_success_transaction},
+    Node, Spec,
+};
 use ckb_logger::info;
-use ckb_types::core::FeeRate;
+use ckb_types::{
+    core::{cell::CellMetaBuilder, DepType, FeeRate},
+    packed::CellDepBuilder,
+};
 use std::{thread::sleep, time::Duration};
 
+use ckb_types::{packed::OutPoint, prelude::*};
+
 pub struct SizeLimit;
 
 const MAX_MEM_SIZE_FOR_SIZE_LIMIT: usize = 2000;
@@ -59,3 +66,73 @@ impl Spec for SizeLimit {
         config.tx_pool.min_fee_rate = FeeRate::zero();
     }
 }
+
+pub struct TxPoolLimitAncestorCount;
+impl Spec for TxPoolLimitAncestorCount {
+    fn run(&self, nodes: &mut Vec<Node>) {
+        let node0 = &nodes[0];
+
+        let initial_inputs = gen_spendable(node0, 130);
+        let input_a = &initial_inputs[0];
+
+        // Commit transaction root
+        let tx_a = {
+            let tx_a = always_success_transaction(node0, input_a);
+            node0.submit_transaction(&tx_a);
+            tx_a
+        };
+
+        let cell_dep = CellDepBuilder::default()
+            .dep_type(DepType::Code.into())
+            .out_point(OutPoint::new(tx_a.hash(), 0))
+            .build();
+
+        // Create 125 transactions cell dep on tx_a
+        for i in 1..=125 {
+            let cur = always_success_transaction(node0, initial_inputs.get(i).unwrap());
+            let cur = cur.as_advanced_builder().cell_dep(cell_dep.clone()).build();
+            let _ = node0.rpc_client().send_transaction(cur.data().into());
+        }
+
+        // Create a new transaction consume the cell dep, it will be succeed in submit
+        let input = CellMetaBuilder::from_cell_output(tx_a.output(0).unwrap(), Default::default())
+            .out_point(OutPoint::new(tx_a.hash(), 0))
+            .build();
+        let last = always_success_transaction(node0, &input);
+
+        let res = node0
+            .rpc_client()
+            .send_transaction_result(last.data().into());
+        assert!(res.is_ok());
+
+        // create a transaction chain
+        let input_c = &initial_inputs[129];
+        // Commit transaction root
+        let tx_c = {
+            let tx_c = always_success_transaction(node0, input_c);
+            node0.submit_transaction(&tx_c);
+            tx_c
+        };
+
+        let mut prev = tx_c.clone();
+        // Create transaction chain
+        for i in 0..125 {
+            let input =
+                CellMetaBuilder::from_cell_output(prev.output(0).unwrap(), Default::default())
+                    .out_point(OutPoint::new(prev.hash(), 0))
+                    .build();
+            let cur = always_success_transaction(node0, &input);
+            let res = node0
+                .rpc_client()
+                .send_transaction_result(cur.data().into());
+            prev = cur.clone();
+            if i >= 124 {
+                assert!(res.is_err());
+                let msg = res.err().unwrap().to_string();
+                assert!(msg.contains("PoolRejectedTransactionByMaxAncestorsCountLimit"));
+            } else {
+                assert!(res.is_ok());
+            }
+        }
+    }
+}

tx-pool/src/component/entry.rs

@@ -39,6 +39,8 @@ pub struct TxEntry {
     pub descendants_cycles: Cycle,
     /// descendants txs count
     pub descendants_count: usize,
+    /// dicrect ancestors txs count
+    pub direct_ancestors_count: usize,
     /// The unix timestamp when entering the Txpool, unit: Millisecond
     pub timestamp: u64,
 }
@@ -71,6 +73,7 @@ impl TxEntry {
             descendants_cycles: cycles,
             descendants_count: 1,
             ancestors_count: 1,
+            direct_ancestors_count: 1,
         }
     }
 

tx-pool/src/component/links.rs

@@ -5,19 +5,22 @@ use std::collections::{HashMap, HashSet};
 pub struct TxLinks {
     pub parents: HashSet<ProposalShortId>,
     pub children: HashSet<ProposalShortId>,
+    pub direct_parents: HashSet<ProposalShortId>,
 }
 
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Eq, PartialEq)]
 pub enum Relation {
     Parents,
     Children,
+    DirectParents,
 }
 
 impl TxLinks {
     fn get_direct_ids(&self, relation: Relation) -> &HashSet<ProposalShortId> {
         match relation {
             Relation::Parents => &self.parents,
             Relation::Children => &self.children,
+            Relation::DirectParents => &self.direct_parents,
         }
     }
 }
@@ -68,9 +71,18 @@ impl TxLinksMap {
             stage.remove(&id);
             relation_ids.insert(id);
         }
+        // for direct parents, we don't store children in links map
+        // so filter those not in links map, they maybe removed from tx-pool now
+        if relation == Relation::DirectParents {
+            relation_ids.retain(|id| self.inner.contains_key(id));
+        }
         relation_ids
     }
 
+    pub fn add_link(&mut self, short_id: ProposalShortId, links: TxLinks) {
+        self.inner.insert(short_id, links);
+    }
+
     pub fn calc_ancestors(&self, short_id: &ProposalShortId) -> HashSet<ProposalShortId> {
         self.calc_relative_ids(short_id, Relation::Parents)
     }
@@ -131,6 +143,16 @@ impl TxLinksMap {
             .map(|links| links.parents.insert(parent))
     }
 
+    pub fn add_direct_parent(
+        &mut self,
+        short_id: &ProposalShortId,
+        parent: ProposalShortId,
+    ) -> Option<bool> {
+        self.inner
+            .get_mut(short_id)
+            .map(|links| links.direct_parents.insert(parent))
+    }
+
     pub fn clear(&mut self) {
         self.inner.clear();
     }

tx-pool/src/component/pool_map.rs

@@ -429,21 +429,26 @@ impl PoolMap {
         let tx_short_id: ProposalShortId = entry.proposal_short_id();
         let outputs = entry.transaction().output_pts();
         let mut children = HashSet::new();
+        let mut direct_children = HashSet::new();
 
         // collect children
         for o in outputs {
             if let Some(ids) = self.edges.get_deps_ref(&o).cloned() {
                 children.extend(ids);
             }
             if let Some(id) = self.edges.get_input_ref(&o).cloned() {
-                children.insert(id);
+                children.insert(id.clone());
+                direct_children.insert(id);
             }
         }
         // update children
         if !children.is_empty() {
             for child in &children {
                 self.links.add_parent(child, tx_short_id.clone());
             }
+            for child in &direct_children {
+                self.links.add_direct_parent(child, tx_short_id.clone());
+            }
             if let Some(links) = self.links.inner.get_mut(&tx_short_id) {
                 links.children.extend(children);
             }
@@ -458,8 +463,10 @@ impl PoolMap {
         let mut parents: HashSet<ProposalShortId> = HashSet::with_capacity(
             entry.transaction().inputs().len() + entry.transaction().cell_deps().len(),
         );
-        let short_id = entry.proposal_short_id();
+        let mut direct_parents: HashSet<ProposalShortId> =
+            HashSet::with_capacity(entry.transaction().inputs().len());
 
+        let short_id = entry.proposal_short_id();
         for input in entry.transaction().inputs() {
             let input_pt = input.previous_output();
             if let Some(deps) = self.edges.deps.get(&input_pt) {
@@ -469,7 +476,8 @@ impl PoolMap {
             let parent_hash = &input_pt.tx_hash();
             let id = ProposalShortId::from_tx_hash(parent_hash);
             if self.links.inner.contains_key(&id) {
-                parents.insert(id);
+                parents.insert(id.clone());
+                direct_parents.insert(id);
             }
         }
         for cell_dep in entry.transaction().cell_deps() {
@@ -484,25 +492,33 @@ impl PoolMap {
             .links
             .calc_relation_ids(parents.clone(), Relation::Parents);
 
+        let direct_ancestors = self
+            .links
+            .calc_relation_ids(direct_parents.clone(), Relation::DirectParents);
+
         // update parents references
         for ancestor_id in &ancestors {
             let ancestor = self.get_by_id_checked(ancestor_id);
+            // cell deps don't accord into ancestors
             entry.add_ancestor_weight(&ancestor.inner);
         }
-        if entry.ancestors_count > self.max_ancestors_count {
-            debug!("debug: exceeded maximum ancestors count");
+        entry.direct_ancestors_count = direct_ancestors.len() + 1;
+        if entry.direct_ancestors_count > self.max_ancestors_count {
             return Err(Reject::ExceededMaximumAncestorsCount);
         }
 
         for parent in &parents {
             self.links.add_child(parent, short_id.clone());
         }
 
-        let links = TxLinks {
-            parents,
-            children: Default::default(),
-        };
-        self.links.inner.insert(short_id, links);
+        self.links.add_link(
+            short_id,
+            TxLinks {
+                parents,
+                direct_parents,
+                children: Default::default(),
+            },
+        );
 
         Ok(true)
     }

tx-pool/src/component/tests/links.rs

@@ -0,0 +1,32 @@
+use crate::component::links::{Relation, TxLinks, TxLinksMap};
+use ckb_types::packed::ProposalShortId;
+use ckb_types::prelude::Entity;
+use std::collections::HashSet;
+
+#[test]
+fn test_link_map() {
+    let mut map = TxLinksMap::default();
+    let id1 = ProposalShortId::from_slice(&[1; 10]).unwrap();
+    let id2 = ProposalShortId::from_slice(&[2; 10]).unwrap();
+    let id3 = ProposalShortId::from_slice(&[3; 10]).unwrap();
+    let id4 = ProposalShortId::from_slice(&[4; 10]).unwrap();
+
+    map.add_link(id1.clone(), TxLinks::default());
+    map.add_link(id2.clone(), TxLinks::default());
+    map.add_link(id3.clone(), TxLinks::default());
+    map.add_link(id4.clone(), TxLinks::default());
+
+    map.add_parent(&id1, id2.clone());
+    let expect: HashSet<ProposalShortId> = vec![id2.clone()].into_iter().collect();
+    assert_eq!(map.get_parents(&id1).unwrap(), &expect);
+
+    map.add_direct_parent(&id1, id2.clone());
+    map.add_direct_parent(&id2, id3.clone());
+    map.add_direct_parent(&id3, id4.clone());
+    let direct_parents = map.calc_relation_ids([id1.clone()].into(), Relation::DirectParents);
+    assert_eq!(direct_parents.len(), 4);
+
+    map.remove(&id3);
+    let direct_parents = map.calc_relation_ids([id1.clone()].into(), Relation::DirectParents);
+    assert_eq!(direct_parents.len(), 2);
+}

tx-pool/src/component/tests/mod.rs

@@ -1,5 +1,6 @@
 mod chunk;
 mod entry;
+mod links;
 mod orphan;
 mod pending;
 mod proposed;