Cluster mempool #33629

pull sdaftuar wants to merge 66 commits into bitcoin:master from sdaftuar:2025-02-cluster-mempool changing 60 files +1844 −3212
  1. sdaftuar commented at 7:50 pm on October 14, 2025: member

    [Reopening #28676 here as a new PR, because GitHub is slow to load the page making it hard to scroll through and see comments. Also, that PR was originally opened with a prototype implementation which has changed significantly with the introduction of TxGraph.]

    This is an implementation of the cluster mempool proposal.

    This branch implements the following observable behavior changes:

    • Maintains a partitioning of the mempool into connected clusters (via the txgraph class), which are limited in vsize to 101 kvB by default, and limited in count to 64 by default.
    • Each cluster is sorted (“linearized”) to try to optimize for selecting highest-feerate-subsets of a cluster first
    • Transaction selection for mining is updated to use the cluster linearizations, selecting highest feerate “chunks” first for inclusion in a block template.
    • Mempool eviction is updated to use the cluster linearizations, selecting lowest feerate “chunks” first for removal.
    • The RBF rules are updated to: (a) drop the requirement that no new inputs are introduced; (b) change the feerate requirement to instead check that the feerate diagram of the mempool will strictly improve; (c) replace the direct conflicts limit with a directly-conflicting-clusters limit.
    • The CPFP carveout rule is eliminated (it doesn’t make sense in a cluster-limited mempool)
    • The ancestor and descendant limits are no longer enforced.
    • New cluster count/cluster vsize limits are now enforced instead.
    • Transaction relay now uses chunk feerate comparisons to determine the order that newly received transactions are announced to peers.

    Additionally, the cached ancestor and descendant data are dropped from the mempool, along with the multi_index indices that were maintained to sort the mempool by ancestor and descendant feerates. For compatibility (eg with wallet behavior or RPCs exposing this), this information is now calculated dynamically instead.

  2. Allow moving an Epoch::Marker 51430680ec
  3. mempool: Store iterators into mapTx in mapNextTx
    This takes the same amount of space as CTransaction pointers, and saves a map
    lookup in many common uses.
    6c73e47448
  4. Allow moving CTxMemPoolEntry objects, disallow copying cd0bea2197
  5. Make CTxMemPoolEntry derive from TxGraph::Ref c5706ea462
  6. DrahtBot renamed this:
    Cluster mempool
    Cluster mempool
    on Oct 14, 2025
  7. DrahtBot commented at 7:50 pm on October 14, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33629.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK instagibbs

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33725 (ci, iwyu: Treat warnings as errors for src/init and src/policy by hebasto)
    • #33616 (policy: don’t CheckEphemeralSpends on reorg by instagibbs)
    • #33591 (Cluster mempool followups by sdaftuar)
    • #33421 (node: add BlockTemplateCache by ismaelsadeeq)
    • #33214 (rpc: require integer verbosity; remove boolean ‘verbose’ by fqlx)
    • #32587 (test: Fix reorg patterns in tests to use proper fork-based approach by yuvicc)
    • #31974 (Drop testnet3 by Sjors)
    • #31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)
    • #31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
    • #30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #28690 (build: Introduce internal kernel library by TheCharlatan)
    • #17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
    • #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
    • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
    • #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • the the -> the [duplicate word]
    • feeerate -> feerate [spelling error]

    drahtbot_id_5_m

  8. glozow added the label TX fees and policy on Oct 14, 2025
  9. glozow added the label Validation on Oct 14, 2025
  10. glozow added the label Mempool on Oct 14, 2025
  11. glozow added this to the milestone 31.0 on Oct 14, 2025
  12. in src/policy/policy.h:192 in f2eff17c6c outdated
    188@@ -188,4 +189,8 @@ static inline int64_t GetVirtualTransactionInputSize(const CTxIn& tx)
    189     return GetVirtualTransactionInputSize(tx, 0, 0);
    190 }
    191 
    192+int64_t GetSigOpsAdjustedWeight(int64_t nWeight, int64_t nSigOpCost, unsigned int bytes_per_sigop);
    


    ismaelsadeeq commented at 10:27 am on October 15, 2025:

    In “Add sigops adjusted weight calculator” f2eff17c6c4fc945f6fd761564212802107a1d7d

    nit: use snake case.

  13. in src/init.cpp:645 in 2801e80528 outdated
    640@@ -641,6 +641,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
    641     argsman.AddArg("-limitdescendantcount=<n>", strprintf("Do not accept transactions if any ancestor would have <n> or more in-mempool descendants (default: %u)", DEFAULT_DESCENDANT_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    642     argsman.AddArg("-limitdescendantsize=<n>", strprintf("Do not accept transactions if any ancestor would have more than <n> kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    643     argsman.AddArg("-test=<option>", "Pass a test-only option. Options include : " + Join(TEST_OPTIONS_DOC, ", ") + ".", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    644+    argsman.AddArg("-limitclustercount=<n>", strprintf("Do not accept transactions connected to <n> or more existing in-mempool transactions (default: %u, maximum: %u)", DEFAULT_CLUSTER_LIMIT, MAX_CLUSTER_COUNT_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    645+    argsman.AddArg("-limitclustersize=<n>", strprintf("Do not accept transactions whose size with all in-mempool connected transactions exceeds <n> kilobytes (default: %u)", DEFAULT_CLUSTER_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    


    ismaelsadeeq commented at 11:47 am on October 15, 2025:

    In “Add new (unused) limits for cluster size/count” 2801e80528a3a1c2949a8fda6338882613a673e5

    nit: be specific it is in virtual size

    0    argsman.AddArg("-limitclustersize=<n>", strprintf("Do not accept transactions whose size with all in-mempool connected transactions exceeds <n> virtual kilobytes (default: %u)", DEFAULT_CLUSTER_SIZE_LIMIT_KVB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    

    glozow commented at 7:18 pm on October 15, 2025:

    It would be nice to expose these through the getmempoolinfo RPC.

     0diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
     1index 492693c9cef..1b62c74f0b3 100644
     2--- a/src/rpc/mempool.cpp
     3+++ b/src/rpc/mempool.cpp
     4@@ -812,6 +812,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
     5     ret.pushKV("fullrbf", true);
     6     ret.pushKV("permitbaremultisig", pool.m_opts.permit_bare_multisig);
     7     ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
     8+    ret.pushKV("limitclustercount", pool.m_opts.limits.cluster_count);
     9+    ret.pushKV("limitclustersize", pool.m_opts.limits.cluster_size_vbytes);
    10     return ret;
    11 }
    12 
    13@@ -836,6 +838,8 @@ static RPCHelpMan getmempoolinfo()
    14                 {RPCResult::Type::BOOL, "fullrbf", "True if the mempool accepts RBF without replaceability signaling inspection (DEPRECATED)"},
    15                 {RPCResult::Type::BOOL, "permitbaremultisig", "True if the mempool accepts transactions with bare multisig outputs"},
    16                 {RPCResult::Type::NUM, "maxdatacarriersize", "Maximum number of bytes that can be used by OP_RETURN outputs in the mempool"},
    17+                {RPCResult::Type::NUM, "limitclustercount", "Maximum number of transactions that can be in a cluster (configured by -limitclustercount)"},
    18+                {RPCResult::Type::NUM, "limitclustersize", "Maximum size of a cluster in virtual bytes (configured by -limitclustersize)"},
    19             }},
    20         RPCExamples{
    21             HelpExampleCli("getmempoolinfo", "")
    

    sdaftuar commented at 0:04 am on October 16, 2025:
    Done, thanks.

    sdaftuar commented at 0:28 am on October 17, 2025:
    Thanks – I’m going to make it “virtual size with all in-mempool…” rather than “virtual kilobytes”, which I think is more legible?
  14. in test/functional/feature_rbf.py:24 in 429bdbecfd outdated
    20@@ -21,6 +21,8 @@
    21 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
    22 
    23 MAX_REPLACEMENT_LIMIT = 100
    24+MAX_CLUSTER_LIMIT = 64
    


    ismaelsadeeq commented at 12:10 pm on October 15, 2025:

    In “test: update feature_rbf.py replacement test” 429bdbecfde93c54374fb3098e357e18556e7e21

    nit: This should live in functional/test_framework/mempool_util.py


    sdaftuar commented at 2:14 pm on October 21, 2025:
    Done.
  15. in test/functional/mempool_updatefromblock.py:222 in 5a388c0d59 outdated
    219@@ -220,7 +220,7 @@ def test_chainlimits_exceeded(self):
    220 
    221     def run_test(self):
    222         # Mine in batches of 25 to test multi-block reorg under chain limits
    223-        self.transaction_graph_test(size=CUSTOM_ANCESTOR_COUNT, n_tx_to_mine=[25, 50, 75])
    224+        self.transaction_graph_test(size=64, n_tx_to_mine=[25, 50, 75])
    


    ismaelsadeeq commented at 12:47 pm on October 15, 2025:

    In “Do not allow mempool clusters to exceed configured limits” 5a388c0d595b2318fea4b1dce977e2d5ff1abc48

    If you define the max cluster count in mempool util it can be reused here.


    sdaftuar commented at 2:14 pm on October 21, 2025:
    Done.
  16. in src/test/mempool_tests.cpp:550 in 5a388c0d59 outdated
    546@@ -547,7 +547,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
    547         AddToMempool(pool, entry.Fee(100LL).FromTx(tx5));
    548     AddToMempool(pool, entry.Fee(900LL).FromTx(tx7));
    549 
    550-    pool.TrimToSize(pool.DynamicMemoryUsage() / 2); // should maximize mempool size by only removing 5/7
    551+    pool.TrimToSize(pool.DynamicMemoryUsage() * 0.75); // should maximize mempool size by only removing 5/7
    


    ismaelsadeeq commented at 1:12 pm on October 15, 2025:

    In “Do not allow mempool clusters to exceed configured limits” 5a388c0d595b2318fea4b1dce977e2d5ff1abc48

    I try to not use the magic number by trimming using

    pool.DynamicMemoryUsage() - usage_of_5 - usage_of_7 but test fail because tx 6 is also evicted. However when I subtract only the usage of 5 the test succeed.

    I grab the usages by computing the delta in memory usage after insertion of the tx.


    sdaftuar commented at 1:31 pm on October 19, 2025:
    This test is further updated in a later commit in this PR – please let me know what you think of the state it ends up in and if it could use further improvements.
  17. in src/util/epochguard.h:62 in 51430680ec outdated
    58@@ -59,8 +59,8 @@ class LOCKABLE Epoch
    59     public:
    60         Marker() = default;
    61         Marker(const Marker&) = default;
    62-        Marker(Marker&&) = delete;
    63-        Marker& operator=(Marker&&) = delete;
    64+        Marker(Marker&&) = default;
    


    instagibbs commented at 1:48 pm on October 15, 2025:

    51430680ecb722e1d4ee4a26dac5724050f41c9e

    The why of this is unclear to me through 8cdd7bb11adeaeb4709c670bef4b57362cfbebcb , worth reordering or expanding commit message?


    sdaftuar commented at 11:29 pm on October 15, 2025:

    Ok it’s been a really long time since I’ve thought about this, but I believe the issue was that I needed epoch markers to be movable so that I could make CTxMemPoolEntry movable.

    I don’t exactly remember why CTxMemPoolEntry needs to be movable but I believe it has something to do with inheriting from TxGraph::Ref. I guess Ref’s can’t be copied, but they can be moved?


    ajtowns commented at 1:27 pm on October 20, 2025:

    CTxMemPoolEntry seems to only need to be movable for mempool_entries.emplace_back(ConsumeTxMempoolEntry(...)) in test/fuzz/policy_estimator.cpp.

    I’m a bit surprised Ref::operator=(Ref&&) isn’t virtual – moving a Ref that might actually a mempool entry seems dangerous; but otoh, moving by operator= only seems to be used in fuzz/txgraph.cpp, which means we don’t actually move anything that’s not just a Ref.

  18. in src/node/interfaces.cpp:723 in 1102ac7f74 outdated
    724         LOCK(m_node.mempool->cs);
    725-        return m_node.mempool->CheckPackageLimits({tx}, entry.GetTxSize());
    726+        // Use CTxMemPool's ChangeSet interface to check whether the chain
    727+        // limits would be violated. Note that the changeset will be destroyed
    728+        // when it goes out of scope.
    729+        auto changeset = m_node.mempool->GetChangeSet();
    


    ismaelsadeeq commented at 1:53 pm on October 15, 2025:

    In “Check cluster limits when using -walletrejectlongchains” 1102ac7f74ac2f48760b46be58c7deb70fa727cf

    nit: also state that it should be safe to create a new change set here because we lock mempool cs.


    sdaftuar commented at 1:34 pm on October 19, 2025:
    Seems redundant, given that we have an EXCLUSIVE_LOCKS_REQUIRED annotation on GetChangeSet() already? Will leave this as-is.
  19. in src/txmempool.cpp:137 in 6c73e47448 outdated
    136@@ -137,12 +137,11 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<Txid>& vHashesToU
    137         {
    


    instagibbs commented at 2:39 pm on October 15, 2025:

    6c73e4744837a7dc138a9177df3a48f30a1ba6c1

    Had to double-check the claim on space usage, seems to hold

    https://www.boost.org/doc/libs/1_73_0/libs/multi_index/doc/performance.html#:~:text=On%20the%20other%20hand%2C%20the,N%2D1)p%2C%20where


    sdaftuar commented at 11:33 pm on October 15, 2025:
    Actually, in fairness I think there might be some platforms where the space usage may be different – I think I ran into that at some point in the history of this PR with one of our CI jobs, but I don’t recall the details now. (Even still, I think saving the map lookup is likely worth it.)
  20. in src/validation.cpp:1614 in 5a388c0d59 outdated
    1610@@ -1604,6 +1611,12 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    1611         return PackageMempoolAcceptResult(package_state, std::move(results));
    1612     }
    1613 
    1614+    // Check if the transaction would exceed the cluster size limit.
    


    instagibbs commented at 4:47 pm on October 15, 2025:

    5a388c0d595b2318fea4b1dce977e2d5ff1abc48

    0    // Check if the transactions would exceed the cluster size limit.
    

    sdaftuar commented at 0:04 am on October 16, 2025:
    Fixed.
  21. in test/functional/mempool_cluster.py:59 in d3f8b6380f
    54+        bad_tx = self.wallet.create_self_transfer(utxo_to_spend=utxo_to_spend)
    55+        assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, bad_tx["hex"])
    56+
    57+        # TODO: verify that the size limits are also enforced.
    58+        # TODO: add tests that exercise rbf, package submission, and package
    59+        # rbf and verify that cluster limits are enforced.
    


    glozow commented at 7:11 pm on October 15, 2025:

    Wrote up some tests for cluster count and size limits in RBFs, package RBFs, package submission, at different configs of -limitclustercount and -limitclustersize, etc:

      0diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
      1index 3da8b477a2f..c75eb22bf9b 100755
      2--- a/test/functional/mempool_cluster.py
      3+++ b/test/functional/mempool_cluster.py
      4@@ -4,59 +4,322 @@
      5 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
      6 """Test cluster mempool accessors and limits"""
      7 
      8+from decimal import Decimal
      9+
     10+from test_framework.mempool_util import (
     11+    DEFAULT_CLUSTER_LIMIT,
     12+    DEFAULT_CLUSTER_SIZE_LIMIT_KVB,
     13+)
     14+from test_framework.messages import (
     15+    COIN,
     16+)
     17 from test_framework.test_framework import BitcoinTestFramework
     18 from test_framework.wallet import (
     19     MiniWallet,
     20 )
     21 from test_framework.util import (
     22+    assert_equal,
     23+    assert_greater_than,
     24+    assert_greater_than_or_equal,
     25     assert_raises_rpc_error,
     26 )
     27 
     28-MAX_CLUSTER_COUNT = 64
     29+def cleanup(func):
     30+    def wrapper(self, *args, **kwargs):
     31+        try:
     32+            func(self, *args, **kwargs)
     33+        finally:
     34+            # Mine blocks to clear the mempool and replenish the wallet's confirmed UTXOs.
     35+            while (len(self.nodes[0].getrawmempool()) > 0):
     36+                self.generate(self.nodes[0], 1)
     37+            self.wallet.rescan_utxos(include_mempool=True)
     38+    return wrapper
     39 
     40 class MempoolClusterTest(BitcoinTestFramework):
     41     def set_test_params(self):
     42         self.num_nodes = 1
     43 
     44-    def run_test(self):
     45-        node = self.nodes[0]
     46-        self.wallet = MiniWallet(node)
     47-
     48-        node = self.nodes[0]
     49-        parent_tx = self.wallet.send_self_transfer(from_node=node)
     50+    def add_chain_cluster(self, node, cluster_count, target_vsize=None):
     51+        """Create a cluster of transactions, with the count specified.
     52+        The topology is a chain: the i'th transaction depends on the (i-1)'th transaction.
     53+        Optionally provide a target_vsize for each transaction.
     54+        """
     55+        parent_tx = self.wallet.send_self_transfer(from_node=node, confirmed_only=True, target_vsize=target_vsize)
     56         utxo_to_spend = parent_tx["new_utxo"]
     57-        ancestors = [parent_tx["txid"]]
     58-        while len(node.getrawmempool()) < MAX_CLUSTER_COUNT:
     59-            next_tx = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_to_spend)
     60+        all_txids = [parent_tx["txid"]]
     61+        all_results = [parent_tx]
     62+
     63+        while len(all_results) < cluster_count:
     64+            next_tx = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_to_spend, target_vsize=target_vsize)
     65+            assert next_tx["txid"] in node.getrawmempool()
     66+
     67             # Confirm that each transaction is in the same cluster as the first.
     68-            assert node.getmempoolcluster(next_tx['txid']) == node.getmempoolcluster(parent_tx['txid'])
     69+            assert_equal(node.getmempoolcluster(next_tx['txid']), node.getmempoolcluster(parent_tx['txid']))
     70 
     71             # Confirm that the ancestors are what we expect
     72             mempool_ancestors = node.getmempoolancestors(next_tx['txid'])
     73-            assert sorted(mempool_ancestors) == sorted(ancestors)
     74+            assert_equal(sorted(mempool_ancestors), sorted(all_txids))
     75 
     76             # Confirm that each successive transaction is added as a descendant.
     77-            assert all([ next_tx["txid"] in node.getmempooldescendants(x) for x in ancestors ])
     78+            assert all([ next_tx["txid"] in node.getmempooldescendants(x) for x in all_txids ])
     79 
     80             # Update for next iteration
     81-            ancestors.append(next_tx["txid"])
     82+            all_results.append(next_tx)
     83+            all_txids.append(next_tx["txid"])
     84             utxo_to_spend = next_tx["new_utxo"]
     85 
     86-        assert node.getmempoolcluster(parent_tx['txid'])['txcount'] == MAX_CLUSTER_COUNT
     87+        assert node.getmempoolcluster(parent_tx['txid'])['txcount'] == cluster_count
     88+        return all_results
     89+
     90+    def check_feerate_diagram(self, node):
     91+        """Sanity check the feerate diagram."""
     92         feeratediagram = node.getmempoolfeeratediagram()
     93         last_val = [0, 0]
     94         for x in feeratediagram:
     95+            # The vsize is always positive, except for the first iteration
     96             assert x['vsize'] > 0 or x['fee'] == 0
     97-            assert last_val[0]*x['fee'] >= last_val[1]*x['vsize']
     98+            # Monotonically decreasing fee per vsize
     99+            assert_greater_than_or_equal(last_val[0] * x['fee'], last_val[1] * x['vsize'])
    100             last_val = [x['vsize'], x['fee']]
    101 
    102+    def test_limit_enforcement(self, cluster_submitted, target_vsize_per_tx=None):
    103+        """
    104+        the cluster may change as a result of these transactions, so cluster_submitted is mutated accordingly
    105+        """
    106+        # Cluster has already been submitted and has at least 3 transactions, otherwise this test won't work.
    107+        assert_greater_than_or_equal(len(cluster_submitted), 3)
    108+        node = self.nodes[0]
    109+        last_result = cluster_submitted[-1]
    110+        # We assume that this is the maximum cluster count
    111+        cluster_limit = node.getmempoolcluster(cluster_submitted[-1]["txid"])['txcount']
    112+
    113         # Test that adding one more transaction to the cluster will fail.
    114-        bad_tx = self.wallet.create_self_transfer(utxo_to_spend=utxo_to_spend)
    115+        bad_tx = self.wallet.create_self_transfer(utxo_to_spend=last_result["new_utxo"], target_vsize=target_vsize_per_tx)
    116         assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, bad_tx["hex"])
    117 
    118-        # TODO: verify that the size limits are also enforced.
    119-        # TODO: add tests that exercise rbf, package submission, and package
    120-        # rbf and verify that cluster limits are enforced.
    121+        # It should also work during replacement
    122+        utxo_to_double_spend = self.wallet.get_utxo(confirmed_only=True)
    123+        fee = Decimal("0.000001")
    124+        tx_to_replace = self.wallet.create_self_transfer(utxo_to_spend=utxo_to_double_spend, fee=fee)
    125+        node.sendrawtransaction(tx_to_replace["hex"])
    126+
    127+        # Multiply fee by 5, which should easily cover the cost to replace. Otherwise, use the target vsize at 10sat/vB
    128+        fee_to_use = target_vsize_per_tx * 10 if target_vsize_per_tx is not None else int(fee * COIN * 5)
    129+        bad_tx_also_replacement = self.wallet.create_self_transfer_multi(
    130+            utxos_to_spend=[last_result["new_utxo"], utxo_to_double_spend],
    131+            target_vsize=target_vsize_per_tx,
    132+            fee_per_output=fee_to_use,
    133+        )
    134+        assert_raises_rpc_error(-26, "replacement-failed", node.sendrawtransaction, bad_tx_also_replacement["hex"])
    135+
    136+        # Replace the last transaction. We are extending the cluster by one, but also removing one: 64 + 1 - 1 = 64
    137+        # In the case of vsize, it should similarly cancel out.
    138+        second_to_last_utxo = cluster_submitted[-2]["new_utxo"]
    139+        fee_to_beat = cluster_submitted[-1]["fee"]
    140+        vsize_to_use = cluster_submitted[-1]["tx"].get_vsize() if target_vsize_per_tx is not None else None
    141+        good_tx_replacement = self.wallet.create_self_transfer(utxo_to_spend=second_to_last_utxo, fee=fee_to_beat * 5, target_vsize=vsize_to_use)
    142+        node.sendrawtransaction(good_tx_replacement["hex"], maxfeerate=0)
    143+
    144+        cluster_submitted[-1] = good_tx_replacement
    145+
    146+    def test_limit_enforcement_package(self, cluster_submitted):
    147+        node = self.nodes[0]
    148+        # Create a package from the second to last transaction. This shouldn't work because the effect is 64 + 2 - 1 = 65
    149+        last_utxo = cluster_submitted[-2]["new_utxo"]
    150+        fee_to_beat = cluster_submitted[-1]["fee"]
    151+        # We do not use package RBF here because it has additional restrictions on mempool ancestors.
    152+        parent_tx_bad = self.wallet.create_self_transfer(utxo_to_spend=last_utxo, fee=fee_to_beat * 5)
    153+        child_tx_bad = self.wallet.create_self_transfer(utxo_to_spend=parent_tx_bad["new_utxo"])
    154+        package_result = node.submitpackage([parent_tx_bad["hex"], child_tx_bad["hex"]], maxfeerate=0)
    155+        # The parent should be submitted, but the child rejected.
    156+        result_parent_only = node.submitpackage([parent_tx_bad["hex"], child_tx_bad["hex"]])
    157+
    158+        assert parent_tx_bad["txid"] in node.getrawmempool()
    159+        assert child_tx_bad["txid"] not in node.getrawmempool()
    160+        assert_equal(result_parent_only["package_msg"], "transaction failed")
    161+        assert_equal(result_parent_only["tx-results"][child_tx_bad["wtxid"]]["error"], "too-large-cluster")
    162+
    163+        # Now, create a package from the second to last transaction. This should work because the effect is 64 + 2 - 2 = 64
    164+        third_to_last_utxo = cluster_submitted[-3]["new_utxo"]
    165+        parent_tx_good = self.wallet.create_self_transfer(utxo_to_spend=third_to_last_utxo)
    166+        child_tx_good = self.wallet.create_self_transfer(utxo_to_spend=parent_tx_good["new_utxo"], fee=fee_to_beat * 5)
    167+        result_both_good = node.submitpackage([parent_tx_good["hex"], child_tx_good["hex"]], maxfeerate=0)
    168+        assert_equal(result_both_good["package_msg"], "success")
    169+        assert parent_tx_good["txid"] in node.getrawmempool()
    170+        assert child_tx_good["txid"] in node.getrawmempool()
    171+
    172+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
    173+    def test_cluster_count_limit(self, max_cluster_count):
    174+        node = self.nodes[0]
    175+        cluster_submitted = self.add_chain_cluster(node, max_cluster_count)
    176+        self.check_feerate_diagram(node)
    177+        for result in cluster_submitted:
    178+            assert_equal(node.getmempoolcluster(result["txid"])['txcount'], max_cluster_count)
    179+
    180+        self.log.info("Test that cluster count limit is enforced")
    181+        self.test_limit_enforcement(cluster_submitted)
    182+        self.log.info("Test that the resulting cluster count is correctly calculated in a package")
    183+        self.test_limit_enforcement_package(cluster_submitted)
    184+
    185+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
    186+    def test_cluster_size_limit(self, max_cluster_size_vbytes):
    187+        node = self.nodes[0]
    188+        # This number should be smaller than the cluster count limit.
    189+        num_txns = 10
    190+        # Leave some buffer so it is possible to add a reasonably-sized transaction.
    191+        target_vsize_per_tx = int((max_cluster_size_vbytes - 500) / num_txns)
    192+        cluster_submitted = self.add_chain_cluster(node, num_txns, target_vsize_per_tx)
    193+
    194+        vsize_remaining = max_cluster_size_vbytes - node.getmempoolcluster(cluster_submitted[0]["txid"])['vsize']
    195+        self.log.info("Test that cluster size limit is enforced")
    196+        self.test_limit_enforcement(cluster_submitted, target_vsize_per_tx=vsize_remaining + 4)
    197+
    198+        # Try another cluster and add a small transaction: it should succeed
    199+        last_result = cluster_submitted[-1]
    200+        small_tx = self.wallet.create_self_transfer(utxo_to_spend=last_result["new_utxo"], target_vsize=vsize_remaining)
    201+        node.sendrawtransaction(small_tx["hex"])
    202+
    203+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
    204+    def test_cluster_merging(self, max_cluster_count):
    205+        node = self.nodes[0]
    206+
    207+        self.log.info(f"Test merging 2 clusters with transaction counts totaling {max_cluster_count}")
    208+        for num_txns_cluster1 in [1, 5, 10]:
    209+            # Create a chain of transactions
    210+            cluster1 = self.add_chain_cluster(node, num_txns_cluster1)
    211+            for result in cluster1:
    212+                node.sendrawtransaction(result["hex"])
    213+            utxo_from_cluster1 = cluster1[-1]["new_utxo"]
    214+
    215+            # Make the next cluster, which contains the remaining transactions
    216+            assert_greater_than(max_cluster_count, num_txns_cluster1)
    217+            num_txns_cluster2 = max_cluster_count - num_txns_cluster1
    218+            cluster2 = self.add_chain_cluster(node, num_txns_cluster2)
    219+            for result in cluster2:
    220+                node.sendrawtransaction(result["hex"])
    221+            utxo_from_cluster2 = cluster2[-1]["new_utxo"]
    222+
    223+            # Now create a transaction that spends from both clusters, which would merge them.
    224+            tx_merger = self.wallet.create_self_transfer_multi(utxos_to_spend=[utxo_from_cluster1, utxo_from_cluster2])
    225+            assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, tx_merger["hex"])
    226+
    227+            # Spending from the clusters independently should work
    228+            tx_spending_cluster1 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_from_cluster1)
    229+            tx_spending_cluster2 = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_from_cluster2)
    230+            assert tx_spending_cluster1["txid"] in node.getrawmempool()
    231+            assert tx_spending_cluster2["txid"] in node.getrawmempool()
    232+
    233+        self.log.info(f"Test merging {max_cluster_count} clusters with 1 transaction spending from all of them")
    234+        utxos_to_merge = []
    235+        for _ in range(max_cluster_count):
    236+            # Use a confirmed utxo to ensure distinct clusters
    237+            confirmed_utxo = self.wallet.get_utxo(confirmed_only=True)
    238+            singleton = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=confirmed_utxo)
    239+            assert singleton["txid"] in node.getrawmempool()
    240+            utxos_to_merge.append(singleton["new_utxo"])
    241+
    242+        assert_equal(len(utxos_to_merge), max_cluster_count)
    243+        tx_merger = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_to_merge)
    244+        assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, tx_merger["hex"])
    245+
    246+        # Spending from 1 fewer cluster should work
    247+        tx_merger_all_but_one = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_to_merge[:-1])
    248+        node.sendrawtransaction(tx_merger_all_but_one["hex"])
    249+        assert tx_merger_all_but_one["txid"] in node.getrawmempool()
    250+
    251+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
    252+    def test_cluster_merging_size(self, max_cluster_size_vbytes):
    253+        node = self.nodes[0]
    254+
    255+        self.log.info(f"Test merging clusters with sizes totaling {max_cluster_size_vbytes} vB")
    256+        num_txns = 10
    257+        # Leave some buffer so it is possible to add a reasonably-sized transaction.
    258+        target_vsize_per_tx = int((max_cluster_size_vbytes - 500) / num_txns)
    259+        utxos_to_merge = []
    260+        vsize_remaining = max_cluster_size_vbytes
    261+        for _ in range(num_txns):
    262+            confirmed_utxo = self.wallet.get_utxo(confirmed_only=True)
    263+            singleton = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=confirmed_utxo)
    264+            assert singleton["txid"] in node.getrawmempool()
    265+            utxos_to_merge.append(singleton["new_utxo"])
    266+            vsize_remaining -= singleton["tx"].get_vsize()
    267+
    268+        assert_greater_than_or_equal(vsize_remaining, 500)
    269+
    270+        # Create a transaction spending from all clusters that exceeds the cluster size limit.
    271+        tx_merger_too_big = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_to_merge, target_vsize=vsize_remaining + 4, fee_per_output=10000)
    272+        assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, tx_merger_too_big["hex"])
    273+
    274+        # A transaction that is slightly smaller should work.
    275+        tx_merger_small = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_to_merge[:-1], target_vsize=vsize_remaining - 4, fee_per_output=10000)
    276+        node.sendrawtransaction(tx_merger_small["hex"])
    277+        assert tx_merger_small["txid"] in node.getrawmempool()
    278+
    279+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
    280+    def test_cluster_limit_rbf(self, max_cluster_count):
    281+        node = self.nodes[0]
    282+
    283+        # Use min feerate for the to-be-replaced transactions. There are many, so replacement cost can be expensive.
    284+        min_feerate = node.getmempoolinfo()["mempoolminfee"]
    285+
    286+        self.log.info("Test that cluster size calculation takes RBF into account")
    287+        utxos_created_by_parents = []
    288+        fees_rbf_sats = 0
    289+        for _ in range(max_cluster_count - 1):
    290+            parent_tx = self.wallet.send_self_transfer(from_node=node, confirmed_only=True)
    291+            utxo_to_replace = parent_tx["new_utxo"]
    292+            child_tx = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_to_replace, fee_rate=min_feerate)
    293+
    294+            fees_rbf_sats += int(child_tx["fee"] * COIN)
    295+            utxos_created_by_parents.append(utxo_to_replace)
    296+
    297+        # This transaction would create a cluster of size max_cluster_count
    298+        # Importantly, the node should account for the fact that half of the transactions will be replaced.
    299+        tx_merger_replacer = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_created_by_parents, fee_per_output=fees_rbf_sats * 2)
    300+        node.sendrawtransaction(tx_merger_replacer["hex"])
    301+        assert tx_merger_replacer["txid"] in node.getrawmempool()
    302+        assert_equal(node.getmempoolcluster(tx_merger_replacer["txid"])['txcount'], max_cluster_count)
    303+
    304+        self.log.info("Test that cluster size calculation takes package RBF into account")
    305+        utxos_to_replace = []
    306+        fee_rbf_decimal = 0
    307+        for _ in range(max_cluster_count):
    308+            confirmed_utxo = self.wallet.get_utxo(confirmed_only=True)
    309+            tx_to_replace = self.wallet.send_self_transfer(from_node=node, utxo_to_spend=confirmed_utxo, fee_rate=min_feerate)
    310+            fee_rbf_decimal += tx_to_replace["fee"]
    311+            utxos_to_replace.append(confirmed_utxo)
    312+
    313+        tx_replacer = self.wallet.create_self_transfer_multi(utxos_to_spend=utxos_to_replace)
    314+        tx_replacer_sponsor = self.wallet.create_self_transfer(utxo_to_spend=tx_replacer["new_utxos"][0], fee=fee_rbf_decimal * 2)
    315+        node.submitpackage([tx_replacer["hex"], tx_replacer_sponsor["hex"]], maxfeerate=0)
    316+        assert tx_replacer["txid"] in node.getrawmempool()
    317+        assert tx_replacer_sponsor["txid"] in node.getrawmempool()
    318+        assert_equal(node.getmempoolcluster(tx_replacer["txid"])['txcount'], 2)
    319+
    320+
    321+    def run_test(self):
    322+        node = self.nodes[0]
    323+        self.wallet = MiniWallet(node)
    324+        self.generate(self.wallet, 400)
    325+
    326+        self.test_cluster_limit_rbf(DEFAULT_CLUSTER_LIMIT)
    327+
    328+        for cluster_size_limit_kvb in [10, 20, 33, 100, DEFAULT_CLUSTER_SIZE_LIMIT_KVB]:
    329+            self.log.info(f"-> Resetting node with -limitclustersize={cluster_size_limit_kvb}")
    330+            self.restart_node(0, extra_args=[f"-limitclustersize={cluster_size_limit_kvb}"])
    331+
    332+            cluster_size_limit = cluster_size_limit_kvb * 1000
    333+            self.test_cluster_size_limit(cluster_size_limit)
    334+            self.test_cluster_merging_size(cluster_size_limit)
    335+
    336+        for cluster_count_limit in [4, 10, 16, 32, DEFAULT_CLUSTER_LIMIT]:
    337+            self.log.info(f"-> Resetting node with -limitclustercount={cluster_count_limit}")
    338+            self.restart_node(0, extra_args=[f"-limitclustercount={cluster_count_limit}"])
    339+
    340+            self.test_cluster_count_limit(cluster_count_limit)
    341+            if cluster_count_limit > 10:
    342+                self.test_cluster_merging(cluster_count_limit)
    343+
    344 
    345 if __name__ == '__main__':
    346     MempoolClusterTest(__file__).main()
    347diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py
    348index 3c4609c0b44..89e2558307e 100644
    349--- a/test/functional/test_framework/mempool_util.py
    350+++ b/test/functional/test_framework/mempool_util.py
    351@@ -33,6 +33,8 @@ from .wallet import (
    352 DEFAULT_MIN_RELAY_TX_FEE = 100
    353 # Default for -incrementalrelayfee in sat/kvB
    354 DEFAULT_INCREMENTAL_RELAY_FEE = 100
    355+DEFAULT_CLUSTER_LIMIT = 64
    356+DEFAULT_CLUSTER_SIZE_LIMIT_KVB = 101
    357 
    358 TRUC_MAX_VSIZE = 10000
    359 TRUC_CHILD_MAX_VSIZE = 1000
    

    sdaftuar commented at 11:19 pm on October 15, 2025:
    Looks good, will include – thank you for the test coverage!

    sdaftuar commented at 11:57 am on October 16, 2025:
    I did make a few changes (the linter alerted on a few lines) so please take a look again at what is here.

    glozow commented at 1:56 pm on October 16, 2025:
    My bad on the linter, lgtm!
  22. in src/node/miner.cpp:149 in ae1ac54103 outdated
    147-    int nDescendantsUpdated = 0;
    148     if (m_mempool) {
    149-        addPackageTxs(nPackagesSelected, nDescendantsUpdated);
    150+        LOCK(m_mempool->cs);
    151+        m_mempool->StartBlockBuilding();
    152+        addChunks();
    


    instagibbs commented at 8:30 pm on October 15, 2025:

    ae1ac5410383e57ed16867580a5fd355a46953de

    Seems more self-contained to start/stop the builder inside addChunks


    sdaftuar commented at 11:38 pm on October 15, 2025:
    I think the reason I didn’t is probably because addChunks() has multiple places where it returns, so it seemed a little messy to have to invoke StopBlockBuilding() within that function. But if you think it’s worth it, I can rework the function and push the calls down into it.

    instagibbs commented at 1:06 pm on October 16, 2025:
    sounds reasonable, leave as is
  23. in src/node/miner.cpp:250 in ae1ac54103 outdated
    380-            packageSize = modit->nSizeWithAncestors;
    381-            packageFees = modit->nModFeesWithAncestors;
    382-            packageSigOpsCost = modit->nSigOpCostWithAncestors;
    383-        }
    384+    chunk_feerate = m_mempool->GetBlockBuilderChunk(selected_transactions);
    385+    FeePerVSize chunk_feerate_vsize = ToFeePerVSize(chunk_feerate);
    


    instagibbs commented at 8:40 pm on October 15, 2025:

    ae1ac5410383e57ed16867580a5fd355a46953de

    if we’re essentially only using this, why not inline chunk_feerate instead?


    sdaftuar commented at 11:43 pm on October 15, 2025:
    In this commit, chunk_feerate.fee is used further down, but this gets cleaned up further in #33591 with the introduction of CFeeRate::GetFeePerVSize, at which point this could be inlined. I’ll make that change in #33591.

    sdaftuar commented at 12:03 pm on October 16, 2025:
    Oh, actually we need the weight further down for the check to see if the chunk will fit in the block, so I’m inclined to leave this as-is?

    instagibbs commented at 1:07 pm on October 16, 2025:
    oops you’re right
  24. in src/node/miner.cpp:244 in ae1ac54103 outdated
    364-                // Either no entry in mapModifiedTx, or it's worse than mapTx.
    365-                // Increment mi for the next loop iteration.
    366-                ++mi;
    367-            }
    368-        }
    369+    std::vector<CTxMemPoolEntry::CTxMemPoolEntryRef> selected_transactions;
    


    instagibbs commented at 8:57 pm on October 15, 2025:

    ae1ac5410383e57ed16867580a5fd355a46953de

    Was getting confused with the two vectors, are they even needed?

     0diff --git a/src/node/miner.cpp b/src/node/miner.cpp
     1index 52103bda82..1bfe771444 100644
     2--- a/src/node/miner.cpp
     3+++ b/src/node/miner.cpp
     4@@ -206,8 +206,8 @@ bool BlockAssembler::TestPackage(FeePerWeight package_feerate, int64_t packageSi
     5 // Perform transaction-level checks before adding to block:
     6 // - transaction finality (locktime)
     7-bool BlockAssembler::TestPackageTransactions(const std::vector<const CTxMemPoolEntry *>& txs) const
     8+bool BlockAssembler::TestPackageTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const
     9 {
    10-    for (auto tx : txs) {
    11-        if (!IsFinalTx(tx->GetTx(), nHeight, m_lock_time_cutoff)) {
    12+    for (const auto tx : txs) {
    13+        if (!IsFinalTx(tx.get().GetTx(), nHeight, m_lock_time_cutoff)) {
    14             return false;
    15         }
    16@@ -243,13 +243,11 @@ void BlockAssembler::addChunks()
    17 
    18     std::vector<CTxMemPoolEntry::CTxMemPoolEntryRef> selected_transactions;
    19+    selected_transactions.reserve(MAX_CLUSTER_COUNT_LIMIT);
    20     FeePerWeight chunk_feerate;
    21 
    22+    // This fills selected_transactions
    23     chunk_feerate = m_mempool->GetBlockBuilderChunk(selected_transactions);
    24     FeePerVSize chunk_feerate_vsize = ToFeePerVSize(chunk_feerate);
    25 
    26-    std::vector<const CTxMemPoolEntry*> chunk_txs;
    27-    // We'll add at most one chunk per iteration below, and chunk count is bounded by
    28-    // the cluster size limit.
    29-    chunk_txs.reserve(MAX_CLUSTER_COUNT_LIMIT);
    30     while (selected_transactions.size() > 0) {
    31         // Check to see if min fee rate is still respected.
    32@@ -260,12 +258,10 @@ void BlockAssembler::addChunks()
    33 
    34         int64_t package_sig_ops = 0;
    35-        chunk_txs.clear();
    36         for (const auto& tx : selected_transactions) {
    37-            chunk_txs.emplace_back(&tx.get());
    38             package_sig_ops += tx.get().GetSigOpCost();
    39         }
    40 
    41         // Check to see if this chunk will fit.
    42-        if (!TestPackage(chunk_feerate, package_sig_ops) || !TestPackageTransactions(chunk_txs)) {
    43+        if (!TestPackage(chunk_feerate, package_sig_ops) || !TestPackageTransactions(selected_transactions)) {
    44             m_mempool->SkipBuilderChunk();
    45             // This chunk won't fit, so we let it be removed from the heap and
    46@@ -283,6 +279,6 @@ void BlockAssembler::addChunks()
    47             // This chunk will fit, so add it to the block.
    48             nConsecutiveFailed = 0;
    49-            for (const auto& tx : chunk_txs) {
    50-                AddToBlock(*tx);
    51+            for (const auto& tx : selected_transactions) {
    52+                AddToBlock(tx);
    53             }
    54             pblocktemplate->m_package_feerates.emplace_back(chunk_feerate_vsize);
    55diff --git a/src/node/miner.h b/src/node/miner.h
    56index 58fedcc1af..a42e2e2cb4 100644
    57--- a/src/node/miner.h
    58+++ b/src/node/miner.h
    59@@ -117,5 +117,5 @@ private:
    60       * These checks should always succeed, and they're here
    61       * only as an extra check in case of suboptimal node configuration */
    62-    bool TestPackageTransactions(const std::vector<const CTxMemPoolEntry *>& txs) const;
    63+    bool TestPackageTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const;
    64 };
    

    sdaftuar commented at 0:03 am on October 16, 2025:
    Good point! Taking your change.

    sdaftuar commented at 12:10 pm on October 16, 2025:
    Incorporated in a8be743aeb42ec8ab613f822989a11a2f2ce70ac
  25. in src/node/miner.cpp:269 in ae1ac54103 outdated
    411+            chunk_txs.emplace_back(&tx.get());
    412+            package_sig_ops += tx.get().GetSigOpCost();
    413+        }
    414 
    415+        // Check to see if this chunk will fit.
    416+        if (!TestPackage(chunk_feerate, package_sig_ops) || !TestPackageTransactions(chunk_txs)) {
    


    instagibbs commented at 9:04 pm on October 15, 2025:

    ae1ac5410383e57ed16867580a5fd355a46953de

    nit: I think renaming TestPackage to TestPackageBlockLimits or something might help readability going forward


    sdaftuar commented at 0:15 am on October 17, 2025:
    Done in #33591 (d7109669b54d299e84dfe90d2ebf591ff673f51c)
  26. in src/validation.cpp:1023 in 2aad9f01e5 outdated
    1084@@ -1107,6 +1085,12 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
    1085     for (auto it : all_conflicts) {
    1086         m_subpackage.m_changeset->StageRemoval(it);
    1087     }
    1088+
    1089+    if (const auto err_string{ImprovesFeerateDiagram(*m_subpackage.m_changeset)}) {
    1090+        // If we can't calculate a feerate, it's because the cluster size limits were hit.
    


    instagibbs commented at 9:31 pm on October 15, 2025:

    2aad9f01e514b77538eb780138a8e52484eaf5a8

    is this right? If so this warrants a test (note to self and others)

    0        // If we can't calculate a feerate, it's because the cluster size limits were hit, and we may want to try package RBF.
    

    sdaftuar commented at 11:56 pm on October 15, 2025:
    Will take this doc change in #33591.

    sdaftuar commented at 12:15 pm on October 16, 2025:
    Done in 6881d21c57ce122f0c2f5900e6caab7ba806d279.
  27. in src/txmempool.h:421 in 2aad9f01e5 outdated
    518+        std::vector<const TxGraph::Ref *> entries;
    519+        entries.reserve(iters_conflicting.size());
    520+        for (auto it : iters_conflicting) {
    521+            entries.emplace_back(&*it);
    522+        }
    523+        return m_txgraph->CountDistinctClusters(entries, TxGraph::Level::MAIN);
    


    instagibbs commented at 9:46 pm on October 15, 2025:

    2aad9f01e514b77538eb780138a8e52484eaf5a8

    nit: belt and suspenders

    0        Assume(!m_txgraph->IsOversized(TxGraph::Level::MAIN));
    1        return m_txgraph->CountDistinctClusters(entries, TxGraph::Level::MAIN);
    

    sdaftuar commented at 12:06 pm on October 16, 2025:
    Done.
  28. in test/functional/mempool_package_rbf.py:198 in 2aad9f01e5 outdated
    196+        num_coins = 101
    197         parent_coins = self.coins[:num_coins]
    198         del self.coins[:num_coins]
    199 
    200-        # Original transactions: 51 transactions with 1 descendants each -> 102 total transactions
    201+        # Original transactions: 101 transactions with 1 descendants each -> 202 total transactions, 101 clusters
    


    instagibbs commented at 9:55 pm on October 15, 2025:

    2aad9f01e514b77538eb780138a8e52484eaf5a8

    Didn’t carefully look at this test case, but we should make sure we are testing the case of package RBF conflicting against clusters of up to count 64

    Looks like the test is still only conflicting with size 2


    instagibbs commented at 7:18 pm on October 29, 2025:

    just extended it to clusters of size three and made log more sensible

     0diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
     1index 759e3cb07d..c54534f6f8 100755
     2--- a/test/functional/mempool_package_rbf.py
     3+++ b/test/functional/mempool_package_rbf.py
     4@@ -94,5 +94,5 @@ class PackageRBFTest(BitcoinTestFramework):
     5 
     6     def test_package_rbf_basic(self):
     7-        self.log.info("Test that a child can pay to replace its parents' conflicts of cluster size 2")
     8+        self.log.info("Test that a child can pay to replace its parents' conflicts")
     9         node = self.nodes[0]
    10         # Reuse the same coins so that the transactions conflict with one another.
    11@@ -196,10 +196,10 @@ class PackageRBFTest(BitcoinTestFramework):
    12         del self.coins[:num_coins]
    13 
    14-        # Original transactions: 101 transactions with 1 descendants each -> 202 total transactions, 101 clusters
    15-        size_two_clusters = []
    16+        # Original transactions: 101 transactions with 2 descendants each -> 303 total transactions, 101 clusters
    17+        size_three_clusters = []
    18         for coin in parent_coins:
    19-            size_two_clusters.append(self.wallet.send_self_transfer_chain(from_node=node, chain_length=2, utxo_to_spend=coin))
    20-        expected_txns = [txn["tx"] for parent_child_txns in size_two_clusters for txn in parent_child_txns]
    21-        assert_equal(len(expected_txns), num_coins * 2)
    22+            size_three_clusters.append(self.wallet.send_self_transfer_chain(from_node=node, chain_length=3, utxo_to_spend=coin))
    23+        expected_txns = [txn["tx"] for parent_child_txns in size_three_clusters for txn in parent_child_txns]
    24+        assert_equal(len(expected_txns), num_coins * 3)
    25         self.assert_mempool_contents(expected=expected_txns)
    26 
    27@@ -241,5 +241,5 @@ class PackageRBFTest(BitcoinTestFramework):
    28         pkg_results = node.submitpackage([package_parent["hex"], package_child["hex"]], maxfeerate=0)
    29         assert_equal(pkg_results["package_msg"], "success")
    30-        self.assert_mempool_contents(expected=[singleton_tx["tx"], size_two_clusters[-1][0]["tx"], size_two_clusters[-1][1]["tx"], package_parent["tx"], package_child["tx"]] )
    31+        self.assert_mempool_contents(expected=[singleton_tx["tx"], size_three_clusters[-1][0]["tx"], size_three_clusters[-1][1]["tx"], size_three_clusters[-1][2]["tx"], package_parent["tx"], package_child["tx"]] )
    32 
    33         self.generate(node, 1)
    

    sdaftuar commented at 10:53 pm on October 30, 2025:
    Done in c00612c15592faa094d60c7bba8eb9966f5910d6
  29. instagibbs commented at 9:58 pm on October 15, 2025: member

    reviewed through 8cdd7bb11adeaeb4709c670bef4b57362cfbebcb

    focusing on logic this time around, only glanced at tests

  30. sdaftuar force-pushed on Oct 16, 2025
  31. sdaftuar force-pushed on Oct 16, 2025
  32. in src/node/miner.h:112 in a8be743aeb outdated
    112       * @pre BlockAssembler::m_mempool must not be nullptr
    113     */
    114-    void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(!m_mempool->cs);
    115+    void addChunks() EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
    116 
    117     // helper functions for addPackageTxs()
    


    ismaelsadeeq commented at 1:31 pm on October 16, 2025:

    In “Select transactions for blocks based on chunk feerate” a8be743aeb42ec8ab613f822989a11a2f2ce70ac

    nit: change to helper functions for addChunks since addPackageTxs is now gone.

    0    // helper functions for addChunks()
    

    sdaftuar commented at 2:14 pm on October 21, 2025:
    Done.
  33. in src/node/miner.h:114 in a8be743aeb outdated
    117     // helper functions for addPackageTxs()
    118-    /** Remove confirmed (inBlock) entries from given set */
    119-    void onlyUnconfirmed(CTxMemPool::setEntries& testSet);
    120     /** Test if a new package would "fit" in the block */
    121-    bool TestPackage(uint64_t packageSize, int64_t packageSigOpsCost) const;
    122+    bool TestPackage(FeePerWeight package_feerate, int64_t packageSigOpsCost) const;
    


    ismaelsadeeq commented at 1:36 pm on October 16, 2025:

    In “Select transactions for blocks based on chunk feerate” a8be743aeb42ec8ab613f822989a11a2f2ce70ac

    nit: It will be better for us replace package with chunk here and other places in the miner?

     0diff --git a/src/node/miner.cpp b/src/node/miner.cpp
     1index 1bfe7714440..e80d55c1f31 100644
     2--- a/src/node/miner.cpp
     3+++ b/src/node/miner.cpp
     4@@ -192,12 +192,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
     5     return std::move(pblocktemplate);
     6 }
     7 
     8-bool BlockAssembler::TestPackage(FeePerWeight package_feerate, int64_t packageSigOpsCost) const
     9+bool BlockAssembler::TestChunk(FeePerWeight chunk_feerate, int64_t chunk_sigops_cost) const
    10 {
    11-    if (nBlockWeight + package_feerate.size >= m_options.nBlockMaxWeight) {
    12+    if (nBlockWeight + chunk_feerate.size >= m_options.nBlockMaxWeight) {
    13         return false;
    14     }
    15-    if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) {
    16+    if (nBlockSigOpsCost + chunk_sigops_cost >= MAX_BLOCK_SIGOPS_COST) {
    17         return false;
    18     }
    19     return true;
    20@@ -205,7 +205,7 @@ bool BlockAssembler::TestPackage(FeePerWeight package_feerate, int64_t packageSi
    21 
    22 // Perform transaction-level checks before adding to block:
    23 // - transaction finality (locktime)
    24-bool BlockAssembler::TestPackageTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const
    25+bool BlockAssembler::TestChunkTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const
    26 {
    27     for (const auto tx : txs) {
    28         if (!IsFinalTx(tx.get().GetTx(), nHeight, m_lock_time_cutoff)) {
    29@@ -256,13 +256,13 @@ void BlockAssembler::addChunks()
    30             return;
    31         }
    32 
    33-        int64_t package_sig_ops = 0;
    34+        int64_t chunk_sig_ops = 0;
    35         for (const auto& tx : selected_transactions) {
    36-            package_sig_ops += tx.get().GetSigOpCost();
    37+            chunk_sig_ops += tx.get().GetSigOpCost();
    38         }
    39 
    40         // Check to see if this chunk will fit.
    41-        if (!TestPackage(chunk_feerate, package_sig_ops) || !TestPackageTransactions(selected_transactions)) {
    42+        if (!TestChunk(chunk_feerate, chunk_sig_ops) || !TestChunkTransactions(selected_transactions)) {
    43             m_mempool->SkipBuilderChunk();
    44             // This chunk won't fit, so we let it be removed from the heap and
    45             // we'll try the next best.
    46@@ -281,7 +281,7 @@ void BlockAssembler::addChunks()
    47             for (const auto& tx : selected_transactions) {
    48                 AddToBlock(tx);
    49             }
    50-            pblocktemplate->m_package_feerates.emplace_back(chunk_feerate_vsize);
    51+            pblocktemplate->m_chunk_feerates.emplace_back(chunk_feerate_vsize);
    52         }
    53 
    54         selected_transactions.clear();
    55diff --git a/src/node/miner.h b/src/node/miner.h
    56index a42e2e2cb48..5fee028d9cd 100644
    57--- a/src/node/miner.h
    58+++ b/src/node/miner.h
    59@@ -47,9 +47,9 @@ struct CBlockTemplate
    60     // Sigops per transaction, not including coinbase transaction (unlike CBlock::vtx).
    61     std::vector<int64_t> vTxSigOpsCost;
    62     std::vector<unsigned char> vchCoinbaseCommitment;
    63-    /* A vector of package fee rates, ordered by the sequence in which
    64-     * packages are selected for inclusion in the block template.*/
    65-    std::vector<FeePerVSize> m_package_feerates;
    66+    /* A vector of chunk fee rates, ordered by the sequence in which
    67+     * chunks are selected for inclusion in the block template.*/
    68+    std::vector<FeePerVSize> m_chunk_feerates;
    69 };
    70 
    71 /** Generate a new block, without valid proof-of-work */
    72@@ -109,14 +109,14 @@ private:
    73     */
    74     void addChunks() EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
    75 
    76-    // helper functions for addPackageTxs()
    77-    /** Test if a new package would "fit" in the block */
    78-    bool TestPackage(FeePerWeight package_feerate, int64_t packageSigOpsCost) const;
    79-    /** Perform checks on each transaction in a package:
    80+    // helper functions for addChunkTxs()
    81+    /** Test if a new chunk would "fit" in the block */
    82+    bool TestChunk(FeePerWeight chunk_feerate, int64_t chunk_sigops_cost) const;
    83+    /** Perform checks on each transaction in a chunk:
    84       * locktime, premature-witness, serialized size (if necessary)
    85       * These checks should always succeed, and they're here
    86       * only as an extra check in case of suboptimal node configuration */
    87-    bool TestPackageTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const;
    88+    bool TestChunkTransactions(const std::vector<CTxMemPoolEntryRef>& txs) const;
    89 };
    90 
    91 /**
    
  34. in src/node/miner.cpp:287 in a8be743aeb outdated
    461-        ++nPackagesSelected;
    462-        pblocktemplate->m_package_feerates.emplace_back(packageFees, static_cast<int32_t>(packageSize));
    463-
    464-        // Update transactions that depend on each of these
    465-        nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx);
    466+        selected_transactions.clear();
    


    ismaelsadeeq commented at 1:58 pm on October 16, 2025:

    In “Select transactions for blocks based on chunk feerate” a8be743aeb42ec8ab613f822989a11a2f2ce70ac

    nit: maybe reserve again after clear here?


    sdaftuar commented at 1:40 pm on October 19, 2025:
    clear() doesn’t change the capacity of the vector, so there’s no need to reserve() again.
  35. in src/test/util/setup_common.cpp:538 in afb9003bc6 outdated
    534@@ -535,49 +535,64 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
    535 std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit)
    536 {
    537     std::vector<CTransactionRef> mempool_transactions;
    538-    std::deque<std::pair<COutPoint, CAmount>> unspent_prevouts;
    539+    std::deque<std::pair<COutPoint, CAmount>> unspent_prevouts, undo_info;
    


    ismaelsadeeq commented at 2:02 pm on October 16, 2025:

    In “test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits” afb9003bc602a77ed15bf4e79277d960937a5c28

    nitty-nit: instead of undo, spent outputs might be better?


    sdaftuar commented at 2:05 pm on October 19, 2025:
    I think “spent” would be confusing, as the purpose of this object is to store the data needed to undo the removal of a utxo from the unspent_prevouts list, in the event that a transaction spending the prevout fails to get into the mempool.
  36. in src/node/mini_miner.cpp:81 in 729cec4f6d outdated
    82-                                      /*fee_ancestor=*/txiter->GetModFeesWithAncestors()});
    83+            size_t ancestor_count{0};
    84+            size_t ancestor_size{0};
    85+            CAmount ancestor_fee{0};
    86+            mempool.CalculateAncestorData(*txiter, ancestor_count, ancestor_size, ancestor_fee);
    87+            auto [mapiter, success] = m_entries_by_txid.emplace(txiter->GetTx().GetHash(), MiniMinerMempoolEntry(txiter->GetSharedTx(), txiter->GetTxSize(), int64_t(ancestor_size), txiter->GetModifiedFee(), ancestor_fee));
    


    instagibbs commented at 2:06 pm on October 16, 2025:

    729cec4f6ddb880210b7e0011fff8b0a5f88c933

    micro-nit: readability of format prior to this commit was better, can it remain a multiline ctor for MiniMinerMempoolEntry with annotations?


    ismaelsadeeq commented at 2:28 pm on October 17, 2025:
    I think it will even be better to have a struct for this to avoid the repetition of the declaration of the variables and passing the as out params, according to guidelines we prefer returning values https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c-functions-and-methods

    sdaftuar commented at 12:09 pm on October 21, 2025:
    After cluster mempool, mini_miner should be modified anyway (and I think this whole MiniMinerMempoolEntry object will disappear, see for example my draft here). So I don’t think it’s worth re-implementing a new interface in this PR.
  37. in src/validation.cpp:1005 in 31a045700e outdated
    1001@@ -1002,40 +1002,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    1002     if (auto ancestors{m_subpackage.m_changeset->CalculateMemPoolAncestors(ws.m_tx_handle, maybe_rbf_limits)}) {
    1003         ws.m_ancestors = std::move(*ancestors);
    1004     } else {
    1005-        // If CalculateMemPoolAncestors fails second time, we want the original error string.
    


    ismaelsadeeq commented at 2:16 pm on October 16, 2025:

    In “policy: Remove CPFP carveout rule” 31a045700ee13a6746c4f4de253e64a0b8a61334

    In doc/policy/mempool-limits.md, Exemptions section CPFP carveout is now stale and should also be deleted


    glozow commented at 6:22 pm on October 17, 2025:

    I think the whole file can be deleted, as ancestor/descendant limits are now also obsolete.

    I think we can write a new doc for cluster limits in a followup? And a release note (/me ducks)


    sdaftuar commented at 10:54 pm on October 30, 2025:
    Agreed – I was thinking I’d tackle this in #33591, if that seems reasonable?
  38. in src/policy/rbf.h:24 in e6315c2432 outdated
    20@@ -21,8 +21,8 @@
    21 class CFeeRate;
    22 class uint256;
    23 
    24-/** Maximum number of transactions that can be replaced by RBF (Rule #5). This includes all
    25- * mempool conflicts and their descendants. */
    26+/** Maximum number of unique clusters that can be affected by an RBF (Rule #5);
    


    ismaelsadeeq commented at 2:49 pm on October 16, 2025:

    In “Implement new RBF logic for cluster mempool” e6315c24326016cfaee5bd046e8b2e4e1088ac6b

    Delete the bip125 rule numbers references.


    sdaftuar commented at 1:45 pm on October 19, 2025:
    This is a reference to Rule #5 in doc/policy/mempool-replacements.md.
  39. in test/functional/feature_rbf.py:338 in e6315c2432 outdated
    341+        # This will not raise an exception
    342+        tx2_id = self.nodes[0].sendrawtransaction(tx2_hex, 0)
    343+        assert tx2_id in self.nodes[0].getrawmempool()
    344 
    345     def test_too_many_replacements(self):
    346         """Replacements that evict too many transactions are rejected"""
    


    ismaelsadeeq commented at 3:06 pm on October 16, 2025:

    In “Implement new RBF logic for cluster mempool” e6315c24326016cfaee5bd046e8b2e4e1088ac6b

    nit: replace transactions with clusters here and other places

    0        """Replacements that evict too many clusters are rejected"""
    

    sdaftuar commented at 2:22 pm on October 21, 2025:
    Fixed this in a couple of places, should be good now I think.
  40. in src/test/rbf_tests.cpp:41 in e6315c2432 outdated
    38@@ -39,35 +39,6 @@ static inline CTransactionRef make_tx(const std::vector<CTransactionRef>& inputs
    39 
    40 // Make two child transactions from parent (which must have at least 2 outputs).
    41 // Each tx will have the same outputs, using the amounts specified in output_values.
    


    ismaelsadeeq commented at 3:10 pm on October 16, 2025:

    In “Implement new RBF logic for cluster mempool” e6315c24326016cfaee5bd046e8b2e4e1088ac6b

  41. ismaelsadeeq commented at 3:15 pm on October 16, 2025: member

    Reviewed the main implementation e6315c24326016cfaee5bd046e8b2e4e1088ac6b

    I’ve not noticed any major issue, dropped a few minor comments and nits. If any comment become stale after the update feel free to ignore it

  42. in src/policy/truc_policy.cpp:82 in 10872f7ec9 outdated
    81                              ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());
    82         }
    83 
    84-        const bool has_parent{mempool_ancestors.size() + in_package_parents.size() > 0};
    85+        if (mempool_parents.size()) {
    86+            if (pool.GetNumAncestors(mempool_parents[0]) + in_package_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) {
    


    instagibbs commented at 3:36 pm on October 16, 2025:

    10872f7ec923803f711cd2c3af93a0e17121330e

    You’re still looking up ancestors in TRUC prior to knowing how large the resulting graph would be, but you’re only doing it once against a single in-mempool parent, so it should be bounded to 64 results total.

    Maybe leave a comment to that effect or stick that in the commit message?


    sdaftuar commented at 2:23 pm on October 21, 2025:
    Updated the commit message.
  43. instagibbs commented at 3:38 pm on October 16, 2025: member

    reviewed through 10872f7ec923803f711cd2c3af93a0e17121330e

    focused on commit structure and comprehension only, next will be focusing on testing

  44. in test/functional/mempool_cluster.py:82 in 8f62e9177b outdated
    77+        last_val = [0, 0]
    78+        for x in feeratediagram:
    79+            # The vsize is always positive, except for the first iteration
    80+            assert x['vsize'] > 0 or x['fee'] == 0
    81+            # Monotonically decreasing fee per vsize
    82+            assert_greater_than_or_equal(last_val[0] * x['fee'], last_val[1] * x['vsize'])
    


    glozow commented at 4:38 pm on October 16, 2025:

    This started failing when I added it to other tests, then I realized it’s the wrong direction

    0            assert_greater_than_or_equal(last_val[1] * x['vsize'], last_val[0] * x['fee'])
    

    glozow commented at 7:14 pm on October 16, 2025:

    I ended up writing some feerate diagram tests to check my understanding of how it works. It might be helpful for reviewers and/or if you’re interested in taking them:

      0diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
      1index ee8005a1cd2..c86e6a1b5c0 100755
      2--- a/test/functional/mempool_cluster.py
      3+++ b/test/functional/mempool_cluster.py
      4@@ -7,6 +7,8 @@
      5 from decimal import Decimal
      6 
      7 from test_framework.mempool_util import (
      8+    assert_equal_feerate_diagram,
      9+    check_feerate_diagram_monotonically_decreasing,
     10     DEFAULT_CLUSTER_LIMIT,
     11     DEFAULT_CLUSTER_SIZE_LIMIT_KVB,
     12 )
     13@@ -71,17 +73,6 @@ class MempoolClusterTest(BitcoinTestFramework):
     14         assert node.getmempoolcluster(parent_tx['txid'])['txcount'] == cluster_count
     15         return all_results
     16 
     17-    def check_feerate_diagram(self, node):
     18-        """Sanity check the feerate diagram."""
     19-        feeratediagram = node.getmempoolfeeratediagram()
     20-        last_val = [0, 0]
     21-        for x in feeratediagram:
     22-            # The vsize is always positive, except for the first iteration
     23-            assert x['vsize'] > 0 or x['fee'] == 0
     24-            # Monotonically decreasing fee per vsize
     25-            assert_greater_than_or_equal(last_val[0] * x['fee'], last_val[1] * x['vsize'])
     26-            last_val = [x['vsize'], x['fee']]
     27-
     28     def test_limit_enforcement(self, cluster_submitted, target_vsize_per_tx=None):
     29         """
     30         the cluster may change as a result of these transactions, so cluster_submitted is mutated accordingly
     31@@ -149,7 +140,7 @@ class MempoolClusterTest(BitcoinTestFramework):
     32     def test_cluster_count_limit(self, max_cluster_count):
     33         node = self.nodes[0]
     34         cluster_submitted = self.add_chain_cluster(node, max_cluster_count)
     35-        self.check_feerate_diagram(node)
     36+        check_feerate_diagram_monotonically_decreasing(node.getmempoolfeeratediagram())
     37         for result in cluster_submitted:
     38             assert_equal(node.getmempoolcluster(result["txid"])['txcount'], max_cluster_count)
     39 
     40@@ -292,6 +283,151 @@ class MempoolClusterTest(BitcoinTestFramework):
     41         assert tx_replacer_sponsor["txid"] in node.getrawmempool()
     42         assert_equal(node.getmempoolcluster(tx_replacer["txid"])['txcount'], 2)
     43 
     44+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
     45+    def test_feerate_diagram(self):
     46+        node = self.nodes[0]
     47+        self.log.info("Test that the feerate diagram shows chunks correctly")
     48+
     49+        # 1 sat/vB as Decimal BTC/kvB
     50+        feerate_1000sat_kvb = Decimal(1000) / COIN
     51+
     52+        def sats_to_btc(sats):
     53+            """Convert int sats to Decimal BTC with 8 decimal places"""
     54+            return Decimal(sats) / Decimal(1e8)
     55+
     56+        # txA (0sat / 500vB) <- txB (1000sat / 500vB)
     57+        # Use v3 to allow 0 fee
     58+        txA = self.wallet.create_self_transfer(confirmed_only=True, fee=0, fee_rate=0, version=3, target_vsize=500)
     59+        txB = self.wallet.create_self_transfer(utxo_to_spend=txA["new_utxo"], fee=sats_to_btc(1000), version=3, target_vsize=500)
     60+        result_ab = node.submitpackage([txA["hex"], txB["hex"]])
     61+        assert_equal(result_ab["package_msg"], "success")
     62+
     63+        # one cluster, one chunk with feerate 1sat/vB
     64+        assert_equal(result_ab["tx-results"][txA["wtxid"]]["fees"]["effective-feerate"], feerate_1000sat_kvb)
     65+        assert_equal(result_ab["tx-results"][txB["wtxid"]]["fees"]["effective-feerate"], feerate_1000sat_kvb)
     66+        assert_equal(node.getmempoolcluster(txA["txid"])['txcount'], 2)
     67+        expected_feerate_diagram_ab = [
     68+            [0, 0],
     69+            [1000, 1000], # [txA, txB] 1sat/vB
     70+        ]
     71+        assert_equal_feerate_diagram(expected_feerate_diagram_ab, node.getmempoolfeeratediagram())
     72+
     73+        # txC (0sat / 1000vB) <- txD (2000sat / 1000vB)
     74+        # Use v3 to allow 0 fee
     75+        txC = self.wallet.create_self_transfer(confirmed_only=True, fee=0, fee_rate=0, version=3, target_vsize=1000)
     76+        txD = self.wallet.create_self_transfer(utxo_to_spend=txC["new_utxo"], fee=sats_to_btc(2000), version=3, target_vsize=1000)
     77+        result_cd = node.submitpackage([txC["hex"], txD["hex"]])
     78+
     79+        # one cluster, one chunks with feerate 1sat/vB
     80+        assert_equal(result_cd["package_msg"], "success")
     81+        assert_equal(result_cd["tx-results"][txC["wtxid"]]["fees"]["effective-feerate"], feerate_1000sat_kvb)
     82+        assert_equal(result_cd["tx-results"][txD["wtxid"]]["fees"]["effective-feerate"], feerate_1000sat_kvb)
     83+        assert_equal(node.getmempoolcluster(txC["txid"])['txcount'], 2)
     84+        # Same chunk feerate as [txA, txB], but [txC, txD] has larger vsize.
     85+        expected_feerate_diagram_cd = [
     86+            [0, 0],
     87+            [1000, 1000], # [txA, txB] 1sat/vB
     88+            [3000, 3000], # [txC, txD] 1sat/vB
     89+        ]
     90+        assert_equal_feerate_diagram(expected_feerate_diagram_cd, node.getmempoolfeeratediagram())
     91+
     92+        self.log.info("Test that the feerate diagram uses modified fees")
     93+        # txE (800sat / 400vB)
     94+        # 799sat of fees will come from prioritisetransaction
     95+        txE = self.wallet.create_self_transfer(confirmed_only=True, fee=sats_to_btc(1), target_vsize=400)
     96+        node.prioritisetransaction(txid=txE["txid"], fee_delta=799)
     97+        node.sendrawtransaction(txE["hex"])
     98+        assert_equal(node.getmempoolcluster(txE["txid"])['txcount'], 1)
     99+        expected_feerate_diagram_e = [
    100+            [0, 0],
    101+            [800, 400], # [txE] 2sat/vB
    102+            [1800, 1400], # [txA, txB] 1sat/vB
    103+            [3800, 3400], # [txC, txD] 1sat/vB
    104+        ]
    105+        assert_equal_feerate_diagram(expected_feerate_diagram_e, node.getmempoolfeeratediagram())
    106+
    107+        # txF (750sat / 500vB) <- txG (6250sat / 500vB)
    108+        txF = self.wallet.create_self_transfer(confirmed_only=True, fee=sats_to_btc(750), target_vsize=500)
    109+        txG = self.wallet.create_self_transfer(utxo_to_spend=txF["new_utxo"], fee=sats_to_btc(6250), target_vsize=500)
    110+
    111+        # Submit them individually to see txF's chunk feerate change.
    112+        node.sendrawtransaction(txF["hex"])
    113+        assert_equal(node.getmempoolcluster(txF["txid"])['txcount'], 1)
    114+        # txF has a feerate of 1.5sat/vB, so it's in the middle
    115+        expected_feerate_diagram_f = [
    116+            [0, 0],
    117+            [800, 400], # [txE] 2sat/vB
    118+            [1550, 900], # [txF] 1.5sat/vB
    119+            [2550, 1900], # [txA, txB] 1sat/vB
    120+            [4550, 3900], # [txC, txD] 1sat/vB
    121+        ]
    122+        assert_equal_feerate_diagram(expected_feerate_diagram_f, node.getmempoolfeeratediagram())
    123+
    124+        # txG bumps txF's chunk feerate to 7sat/vB
    125+        node.sendrawtransaction(txG["hex"])
    126+        assert_equal(node.getmempoolcluster(txF["txid"])['txcount'], 2)
    127+        expected_feerate_diagram_g = [
    128+            [0, 0],
    129+            [7000, 1000], # [txF, txG] 7sat/vB
    130+            [7800, 1400], # [txE] 2sat/vB
    131+            [8800, 2400], # [txA, txB] 1sat/vB
    132+            [10800, 4400], # [txC, txD] 1sat/vB
    133+        ]
    134+        assert_equal_feerate_diagram(expected_feerate_diagram_g, node.getmempoolfeeratediagram())
    135+
    136+        self.log.info("Test that prioritisetransaction on a mempool entry affects the feerate diagram")
    137+        # Prioritise txD to make its chunk feerate a little more than 8sat/vB
    138+        node.prioritisetransaction(txid=txD["txid"], fee_delta=14012)
    139+        expected_feerate_diagram_d_prio = [
    140+            [0, 0],
    141+            [16012, 2000], # [txC, txD] 8.006sat/vB
    142+            [23012, 3000], # [txF, txG] 7sat/vB
    143+            [23812, 3400], # [txE] 2sat/vB
    144+            [24812, 4400], # [txA, txB] 1sat/vB
    145+        ]
    146+        assert_equal_feerate_diagram(expected_feerate_diagram_d_prio, node.getmempoolfeeratediagram())
    147+
    148+        # De-prioritise txG to split up the chunk, putting txF behind txE and txG at the very end.
    149+        node.prioritisetransaction(txid=txG["txid"], fee_delta=-6195)
    150+        expected_feerate_diagram_g_deprio = [
    151+            [0, 0],
    152+            [16012, 2000], # [txC, txD] 8.006sat/vB
    153+            [16812, 2400], # [txE] 2sat/vB
    154+            [17562, 2900], # [txF] 1.5sat/vB
    155+            [18562, 3900], # [txA, txB] 1sat/vB
    156+            [18617, 4400], # [txG] 0.11sat/vB
    157+        ]
    158+        assert_equal_feerate_diagram(expected_feerate_diagram_g_deprio, node.getmempoolfeeratediagram())
    159+
    160+        # txH (30sat / 300vB) Spend txE and txG to merge their clusters, but keeping the chunking the same.
    161+        txH = self.wallet.create_self_transfer_multi(utxos_to_spend=[txE["new_utxo"], txG["new_utxo"]], fee_per_output=30, target_vsize=300)
    162+        node.sendrawtransaction(txH["hex"])
    163+        # The cluster is now EFGH
    164+        assert_equal(node.getmempoolcluster(txE["txid"])['txcount'], 4)
    165+        expected_feerate_diagram_h = [
    166+            [0, 0],
    167+            [16012, 2000], # [txC, txD] 8.006sat/vB
    168+            [16812, 2400], # [txE] 2sat/vB
    169+            [17562, 2900], # [txF] 1.5sat/vB
    170+            [18562, 3900], # [txA, txB] 1sat/vB
    171+            [18617, 4400], # [txG] 0.11sat/vB
    172+            [18647, 4700], # [txH] 0.1sat/vB
    173+        ]
    174+        assert_equal_feerate_diagram(expected_feerate_diagram_h, node.getmempoolfeeratediagram())
    175+
    176+        # txI (2150sat / 200vB) bumps txF, txG, txH to 1.99sat/vB, combining them into a single chunk
    177+        txI = self.wallet.create_self_transfer(utxo_to_spend=txH["new_utxos"][0], fee=sats_to_btc(2150), target_vsize=200)
    178+        node.sendrawtransaction(txI["hex"])
    179+        # The cluster is now EFGHI
    180+        assert_equal(node.getmempoolcluster(txI["txid"])['txcount'], 5)
    181+        expected_feerate_diagram_i = [
    182+            [0, 0],
    183+            [16012, 2000], # [txC, txD] 8.006sat/vB
    184+            [16812, 2400], # [txE] 2sat/vB
    185+            [19797, 3900], # [txF, txG, txH, txI] 1.99sat/vB
    186+            [20797, 4900], # [txA, txB] 1sat/vB
    187+        ]
    188+        assert_equal_feerate_diagram(expected_feerate_diagram_i, node.getmempoolfeeratediagram())
    189 
    190     def run_test(self):
    191         node = self.nodes[0]
    192@@ -299,6 +435,7 @@ class MempoolClusterTest(BitcoinTestFramework):
    193         self.generate(self.wallet, 400)
    194 
    195         self.test_cluster_limit_rbf(DEFAULT_CLUSTER_LIMIT)
    196+        self.test_feerate_diagram()
    197 
    198         for cluster_size_limit_kvb in [10, 20, 33, 100, DEFAULT_CLUSTER_SIZE_LIMIT_KVB]:
    199             self.log.info(f"-> Resetting node with -limitclustersize={cluster_size_limit_kvb}")
    200diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py
    201index 89e2558307e..2bce87bda2f 100644
    202--- a/test/functional/test_framework/mempool_util.py
    203+++ b/test/functional/test_framework/mempool_util.py
    204@@ -10,6 +10,7 @@ from .blocktools import (
    205 )
    206 from .messages import (
    207     COutPoint,
    208+    COIN,
    209     CTransaction,
    210     CTxIn,
    211     CTxInWitness,
    212@@ -22,6 +23,7 @@ from .script import (
    213 from .util import (
    214     assert_equal,
    215     assert_greater_than,
    216+    assert_greater_than_or_equal,
    217     create_lots_of_big_transactions,
    218     gen_return_txouts,
    219 )
    220@@ -131,3 +133,28 @@ def create_large_orphan():
    221     tx.wit.vtxinwit[0].scriptWitness.stack = [CScript(b'X' * 390000)]
    222     tx.vout = [CTxOut(100, CScript([OP_RETURN, b'a' * 20]))]
    223     return tx
    224+
    225+def check_feerate_diagram_monotonically_decreasing(feerate_diagram):
    226+    """Sanity check the feerate diagram."""
    227+    last_val = [0, 0]
    228+    for x in feerate_diagram:
    229+        # The vsize is always positive, except for the first iteration
    230+        assert x['vsize'] > 0 or x['fee'] == 0
    231+        # Monotonically decreasing fee per vsize
    232+        assert_greater_than_or_equal(last_val[0] * x['vsize'], last_val[1] * x['fee'])
    233+        last_val = [x['vsize'], x['fee']]
    234+
    235+def assert_equal_feerate_diagram(expected, actual):
    236+    """Check that expected and actual are equal, handling Decimal values and giving helpful error messages.
    237+    expected: list of [fee, vsize] pairs where fee is an integer number of satoshis
    238+    actual: list of { "fee": Decimal, "vsize": int } from the getmempoolfeeratediagram RPC
    239+    Also sanity checks that the actual feerates are monotonically decreasing.
    240+    """
    241+    assert_equal(len(expected), len(actual))
    242+    for i in range(len(expected)):
    243+        # We convert the Decimal to an integer number to avoid Decimal comparisons.
    244+        # For example, Decimal('0') == Decimal('0E-8') and Decimal('0.0001') == Decimal('0.00010000')
    245+        assert_equal(expected[i][0], int(actual[i]["fee"] * COIN))
    246+        assert_equal(expected[i][1], actual[i]["vsize"])
    247+
    248+    check_feerate_diagram_monotonically_decreasing(actual)
    

    sdaftuar commented at 12:06 pm on October 30, 2025:

    My hesitation to include stricter tests on the chunks is that we currently have no guarantee that our linearizations will be anything more than topological, so it feels brittle to add tests that put an expectation on linearization quality. EDIT: actually we can say more, specifically about the properties of chunking, which this code does test – but if I’m understanding some of the test cases right I think there are also assumptions about linearization quality being mixed in too.

    Perhaps something like this would make more sense in the context of implementing SFL?

  45. in src/net_processing.cpp:5428 in 936f04e770 outdated
    5425@@ -5426,7 +5426,7 @@ class CompareInvMempoolOrder
    5426     {
    5427         /* As std::make_heap produces a max-heap, we want the entries with the
    5428          * fewest ancestors/highest fee to sort later. */
    


    ismaelsadeeq commented at 1:55 pm on October 17, 2025:

    In “Use cluster linearization for transaction relay sort order” 936f04e770eeb4ef8477722cb1f23f29746883a1

    nit: This comment is stale?

    0         * higher mining score to sort later. */
    

    sdaftuar commented at 2:26 pm on October 21, 2025:
    Done.
  46. in src/txmempool.cpp:737 in 9b31836abf outdated
    733@@ -733,7 +734,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
    734                 assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull());
    735                 setParentCheck.insert(*it2);
    736             }
    737-            // We are iterating through the mempool entries sorted in order by ancestor count.
    738+            // We are iterating through the mempool entries sorted topologically and by score.
    


    ismaelsadeeq commented at 2:11 pm on October 17, 2025:

    In “Remove CTxMemPool::GetSortedDepthAndScore” 9b31836abfc086e4693ecdb1ceaec0a8e21dbb8f

    nit

    0            // We are iterating through the mempool entries sorted topologically and by mining score.
    

    sdaftuar commented at 2:27 pm on October 21, 2025:
    Done.
  47. in test/functional/mempool_truc.py:241 in cf2f5211a8 outdated
    234+        tx_v3_child_large2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v3_parent_large2["new_utxo"], target_vsize=child_target_vsize, version=3)
    235+        # Parent and child are within TRUC limits
    236+        assert_greater_than_or_equal(TRUC_MAX_VSIZE, tx_v3_parent_large2["tx"].get_vsize())
    237+        assert_greater_than_or_equal(TRUC_CHILD_MAX_VSIZE, tx_v3_child_large2["tx"].get_vsize())
    238+        assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, tx_v3_child_large2["hex"])
    239+        self.check_mempool([tx_v3_parent_large2["txid"]])
    


    ismaelsadeeq commented at 2:43 pm on October 17, 2025:

    In “Add test case for cluster size limits to TRUC logic” cf2f5211a8fab1438feb1cf1ccb86dccedd4b6c8

    also add a test for cluster count

    0        self.log.info("Test that a decreased limitclustercount also applies to TRUC cluster")
    1        self.restart_node(0, extra_args=["-limitclustercount=1", "-acceptnonstdtxn=1"])
    2        assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, tx_v3_child_large2["hex"])
    3        self.check_mempool([tx_v3_parent_large2["txid"]])
    

    sdaftuar commented at 2:29 pm on October 21, 2025:
    I took this change but I just realized that this is redundant with the first test that appears earlier in this function, right?
  48. in src/node/interfaces.cpp:668 in b23b156c7b outdated
    664@@ -665,10 +665,7 @@ class ChainImpl : public Chain
    665     bool hasDescendantsInMempool(const Txid& txid) override
    666     {
    667         if (!m_node.mempool) return false;
    668-        LOCK(m_node.mempool->cs);
    669-        const auto entry{m_node.mempool->GetEntry(txid)};
    670-        if (entry == nullptr) return false;
    671-        return entry->GetCountWithDescendants() > 1;
    672+        return m_node.mempool->HasDescendants(txid);
    


    ismaelsadeeq commented at 2:49 pm on October 17, 2025:

    In “Use mempool/txgraph to determine if a tx has descendants” b23b156c7bdf112a4fe64acd71e398cc7c0bd231

    nice I like this pattern, not leaking internals, I think it will be better to adopt this pattern also in https://github.com/bitcoin/bitcoin/pull/33629/commits/d2f75c555caf1f6a62b8ab1ef0f2543f0c84cc39#r2432661442


    sdaftuar commented at 12:14 pm on October 21, 2025:
    Can you clarify what you have in mind here?
  49. in src/txmempool.cpp:1305 in e6315c2432 outdated
    1320-    FeeFrac replacement_feerate{0, 0};
    1321-    for (auto it : m_entry_vec) {
    1322-        replacement_feerate += {it->GetModifiedFee(), it->GetTxSize()};
    1323-    }
    1324 
    1325-    auto err_string{m_pool->CheckConflictTopology(m_to_remove)};
    


    glozow commented at 6:26 pm on October 17, 2025:

    in e6315c24326016cfaee5bd046e8b2e4e1088ac6b

    Nice! We should also remove “All conflicting clusters (connected components of mempool transactions) must be clusters of up to size 2.” from the package RBF rules in policy/packages.md


    sdaftuar commented at 2:24 pm on October 21, 2025:
    Done in 845ec3a3f806dee6c9ad053655fb466bd599a73d
  50. in doc/policy/mempool-replacements.md:15 in fba5200d59 outdated
    16-   one of the directly conflicting transactions. An unconfirmed input spends an output from a
    17-   currently-unconfirmed transaction.
    18-
    19-   *Rationale*: When RBF was originally implemented, the mempool did not keep track of
    20-   ancestor feerates yet. This rule was suggested as a temporary restriction.
    21+2. [REDACTED]
    


    glozow commented at 6:42 pm on October 17, 2025:
    fba5200d59af87db240a1b061f995c63b0ed5ee8 nit: Could be “(Removed)” to be consistent with the other rules we no longer implement?

    sdaftuar commented at 2:24 pm on October 21, 2025:
    Done.
  51. in doc/policy/mempool-replacements.md:71 in fba5200d59 outdated
    66@@ -79,3 +67,5 @@ This set of rules is similar but distinct from BIP125.
    67 * Signaling for replace-by-fee is no longer required as of [PR 30592](https://github.com/bitcoin/bitcoin/pull/30592).
    68 
    69 * The incremental relay feerate default is 0.1sat/vB ([PR #33106](https://github.com/bitcoin/bitcoin/pull/33106)).
    70+
    71+* Feerate diagram policy enabled in conjunction with switch to cluster mempool as of **v??.0**.
    


    glozow commented at 6:43 pm on October 17, 2025:
    0* Feerate diagram policy enabled in conjunction with switch to cluster mempool as of **v31.0**.
    
  52. in doc/policy/mempool-replacements.md:41 in fba5200d59 outdated
    45+   *Rationale*: Limit CPU usage required to update the mempool for so many transactions being
    46+   removed at once.
    47 
    48-6. The replacement transaction's feerate is greater than the feerates of all directly conflicting
    49-   transactions.
    50+6. The feerate diagram of the mempool must be strictly improved by the replacement transaction.
    


    glozow commented at 6:46 pm on October 17, 2025:

    fba5200d59af87db240a1b061f995c63b0ed5ee8

    Happy for this to go into a followup:

    Maybe worth adding a bit more color for people who don’t have any context: A singleton replacing a singleton must pay higher total fees and have a higher feerate (covers vast majority of cases). For more complex cases, see this link to more comprehensive explanation.

  53. in test/functional/mempool_ephemeral_dust.py:422 in e6315c2432 outdated
    418@@ -419,8 +419,7 @@ def test_no_minrelay_fee(self):
    419 
    420         res = self.nodes[0].submitpackage([dusty_tx["hex"] for dusty_tx in dusty_txs] + [insufficient_sweep_tx["hex"]])
    421         assert_equal(res['package_msg'], "transaction failed")
    422-        #assert_equal(res['tx-results'][insufficient_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {insufficient_sweep_tx['txid']} (wtxid={insufficient_sweep_tx['wtxid']}) did not spend parent's ephemeral dust")
    


    glozow commented at 7:47 pm on October 17, 2025:
    b9cbca76762 Reviewer note, I originally wanted to point out the extra commented line but then read the commit message and saw it changes back later in e6315c24326
  54. in src/validation.cpp:1043 in e6315c2432 outdated
    1084@@ -1107,6 +1085,12 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
    1085     for (auto it : all_conflicts) {
    1086         m_subpackage.m_changeset->StageRemoval(it);
    1087     }
    1088+
    1089+    if (const auto err_string{ImprovesFeerateDiagram(*m_subpackage.m_changeset)}) {
    1090+        // If we can't calculate a feerate, it's because the cluster size limits were hit.
    1091+        return state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "replacement-failed", err_string->second);
    


    glozow commented at 8:00 pm on October 17, 2025:

    e6315c24326: In both single and package settings, ImprovesFeerateDiagram is where cluster size limits are checked for the first time if RBF is happening. The check is cached and many RBF checks are cheaper than CheckMemPoolPolicyLimits, so that doesn’t seem problematic.

    However, this is assigning TX_RECONSIDERABLE as the error when cluster limits are exceeded, which will cause net_processing to give it some special treatment (we may redownload and revalidate in some circumstances, which may waste resources), which should only be for errors that can be fixed by adding a child (e.g. to pay for fee requirements or sweep dust). I don’t think it makes sense to apply that here, or it should at least be consistent with the other too-large-cluster error.

    Also, I think the error string should be “too-large-cluster” (but that’s more of a nit).

    Putting CheckMemPoolPolicyLimits before replacement checks would automatically fix this. That feels cleaner to me, so ImprovesFeerateDiagram doesn’t need to do double duty.

    We could also examine the err_string here and assign TX_RECONSIDERABLE if it’s actually a feerate diagram error, TX_MEMPOOL_POLICY if it’s a cluster limit error.


    sdaftuar commented at 2:44 pm on October 19, 2025:

    Not sure I follow the reasoning here – an RBF that fails due to cluster size limits might succeed if a child is added that adds additional conflicts, thereby making space in the affected cluster.

    EDIT: Oh – I guess your point is that a transaction which is NOT an RBF might fail due to cluster size limits, and not be treated as RECONSIDERABLE even though a child might be able to free up cluster space as well by introducing new conflicts… That seems like a reasonable objection, but it’s not clear to me the best way to address.


    sdaftuar commented at 8:15 pm on October 20, 2025:

    I’m inclined to implement your suggestion, so that any cluster size limit failures are treated as non-RECONSIDERABLE for now. TRUC transactions would never hit these cluster limits, so users who would be concerned about cluster size limits as a potential pinning vector for CPFP packages already have a mechanism to avoid it.

    (I also considered having all cluster size limits being treated as RECONSIDERABLE, but decided that is also not great, partly because we certainly shouldn’t let a transaction whose cluster size just with its own unconfirmed ancestors be treated as RECONSIDERABLE, and I didn’t want to spend a lot of time trying to figure out if there might be some DoS vector that opened up as a result.)

    We can try to improve on this behavior in the future if there are use cases that would need smarter behavior here; but for now this would be consistent with how the mempool’s ancestor/descendant limits have always worked anyway.


    instagibbs commented at 6:26 am on October 21, 2025:

    we certainly shouldn’t let a transaction whose cluster size just with its own unconfirmed ancestors be treated as RECONSIDERABLE,

    certainly seems like Future Kindred Eviction Work, agreed on distinguishing error types and not attempting a package RBF in this case.


    glozow commented at 9:35 am on October 21, 2025:

    I also considered having all cluster size limits being treated as RECONSIDERABLE, but decided that is also not great, partly because we certainly shouldn’t let a transaction whose cluster size just with its own unconfirmed ancestors be treated as RECONSIDERABLE

    👍 definitely think we should not allow reconsideration in that case

    Sounds good to me to make it non-reconsiderable for now, and maybe change it in the future in a consistent way. I agree it’s possible for a child to make a tx no longer break cluster limits, but it’s not clear to me how important it is to support that case.


    sdaftuar commented at 2:25 pm on October 21, 2025:
    Implemented this change in 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a. Should I try to squash this into place or is this fine as its own commit?

    glozow commented at 2:40 pm on October 21, 2025:
    I prefer a squash but up to you.

    glozow commented at 3:09 pm on October 28, 2025:
    If not squashed, perhaps move it earlier so that the test can have the correct string when introduced?

    sdaftuar commented at 10:57 pm on October 30, 2025:
    I think I managed to squash this into 9d0c8d4d5ec2a50ba4756b9882d8801f24542a2e; hopefully the changes all look sane…
  55. in src/validation.cpp:1 in e6315c2432 outdated


    glozow commented at 8:17 pm on October 17, 2025:

    Maybe for a followup, but since CheckTopologyLimits is being removed, I think the package RBF logic can have one more tweak in e6315c24326016cfaee5bd046e8b2e4e1088ac6b, which I think @sipa brought up a few weeks ago:

    We currently have the requirement that the package feerate is higher than the parent feerate because “we don’t want the child to be only paying anti-DoS fees” but that’s more restrictive than it needs to be. It’s ok if the child has a lower feerate than the parent and only pays anti-DoS fees, as long as it pays for itself to be above the mempool minimum feerate.

    Here’s a branch to do this + a few test cases and a doc update: https://github.com/glozow/bitcoin/commits/2025-10-cluster-package-rbf/

      0diff --git a/doc/policy/packages.md b/doc/policy/packages.md
      1index 7522a984435..2e8f5cf8e0d 100644
      2--- a/doc/policy/packages.md
      3+++ b/doc/policy/packages.md
      4@@ -38,17 +38,10 @@ The following rules are enforced for all packages:
      5 
      6    - Packages are 1-parent-1-child, with no in-mempool ancestors of the package.
      7 
      8-   - All conflicting clusters (connected components of mempool transactions) must be clusters of up to size 2.
      9+   - The child's individual feerate must meet the mempool minimum feerate.
     10 
     11-   - No more than MAX_REPLACEMENT_CANDIDATES transactions can be replaced, analogous to
     12-     regular [replacement rule](./mempool-replacements.md) 5).
     13-
     14-   - Replacements must pay more total fees at the incremental relay fee (analogous to
     15-     regular [replacement rules](./mempool-replacements.md) 3 and 4).
     16-
     17-   - Parent feerate must be lower than package feerate.
     18-
     19-   - Must improve [feerate diagram](https://delvingbitcoin.org/t/mempool-incentive-compatibility/553). (#29242)
     20+   - All other [replacement rules](./mempool-replacements.md) are met: no more than 100 distinct clusters, total package
     21+   fees pay for the package size at the incremental relay feerate, and the feerate diagram improves.
     22 
     23    - *Rationale*: Basic support for package RBF can be used by wallets
     24      by making chains of no longer than two, then directly conflicting
     25@@ -56,18 +49,6 @@ The following rules are enforced for all packages:
     26      result in more robust fee bumping. More general package RBF may be
     27      enabled in the future.
     28 
     29-* When packages are evaluated against ancestor/descendant limits, the union of all transactions'
     30-  descendants and ancestors is considered. (#21800)
     31-
     32-   - *Rationale*: This is essentially a "worst case" heuristic intended for packages that are
     33-     heavily connected, i.e. some transaction in the package is the ancestor or descendant of all
     34-     the other transactions.
     35-
     36-* [CPFP Carve Out](./mempool-limits.md#CPFP-Carve-Out) is disabled in packaged contexts. (#21800)
     37-
     38-   - *Rationale*: This carve out cannot be accurately applied when there are multiple transactions'
     39-     ancestors and descendants being considered at the same time.
     40-
     41 The following rules are only enforced for packages to be submitted to the mempool (not
     42 enforced for test accepts):
     43 
     44diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
     45index 1f167586fe9..fbdaa49f70e 100644
     46--- a/src/test/txpackage_tests.cpp
     47+++ b/src/test/txpackage_tests.cpp
     48@@ -1078,6 +1078,7 @@ BOOST_AUTO_TEST_CASE(package_cpfp_tests)
     49 BOOST_AUTO_TEST_CASE(package_rbf_tests)
     50 {
     51     mineBlocks(5);
     52+    MockMempoolMinFee(CFeeRate(1000));
     53     LOCK(::cs_main);
     54     size_t expected_pool_size = m_node.mempool->size();
     55     CKey child_key{GenerateRandomKey()};
     56@@ -1159,16 +1160,19 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests)
     57             tx_parent_3, /*input_vout=*/0, /*input_height=*/101,
     58             child_key, child_spk, coinbase_value - 199 - 1300, /*submit=*/false));
     59 
     60+        CTransactionRef tx_parent_4 = MakeTransactionRef(CreateValidMempoolTransaction(
     61+            m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
     62+            coinbaseKey, parent_spk, coinbase_value - 1763, /*submit=*/false));
     63+        CTransactionRef tx_child_4 = MakeTransactionRef(CreateValidMempoolTransaction(
     64+            tx_parent_4, /*input_vout=*/0, /*input_height=*/101,
     65+            child_key, child_spk, coinbase_value - 1763 - 60, /*submit=*/false));
     66+
     67         // In all packages, the parents conflict with each other
     68         BOOST_CHECK(tx_parent_1->GetHash() != tx_parent_2->GetHash() && tx_parent_2->GetHash() != tx_parent_3->GetHash());
     69 
     70         // 1 parent paying 200sat, 1 child paying 200sat.
     71         Package package1{tx_parent_1, tx_child_1};
     72-        // 1 parent paying 800sat, 1 child paying 200sat.
     73-        Package package2{tx_parent_2, tx_child_2};
     74-        // 1 parent paying 199sat, 1 child paying 1300sat.
     75-        Package package3{tx_parent_3, tx_child_3};
     76-
     77+        {
     78         const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt);
     79         if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
     80             BOOST_ERROR(err_1.value());
     81@@ -1177,11 +1181,20 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests)
     82         auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
     83         BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
     84         BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
     85+        // Both validated individually.
     86+        BOOST_CHECK_EQUAL(it_parent_1->second.m_wtxids_fee_calculations.value().size(), 1);
     87+        BOOST_CHECK_EQUAL(it_parent_1->second.m_wtxids_fee_calculations.value().front(), tx_parent_1->GetWitnessHash());
     88+        BOOST_CHECK_EQUAL(it_child_1->second.m_wtxids_fee_calculations.value().size(), 1);
     89+        BOOST_CHECK_EQUAL(it_child_1->second.m_wtxids_fee_calculations.value().front(), tx_child_1->GetWitnessHash());
     90+        }
     91         expected_pool_size += 2;
     92         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
     93 
     94         // This replacement is actually not package rbf; the parent carries enough fees
     95         // to replace the entire package on its own.
     96+        // 1 parent paying 800sat, 1 child paying 200sat.
     97+        Package package2{tx_parent_2, tx_child_2};
     98+        {
     99         const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false, std::nullopt);
    100         if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
    101             BOOST_ERROR(err_2.value());
    102@@ -1190,8 +1203,17 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests)
    103         auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
    104         BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    105         BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    106+        // Both validated individually.
    107+        BOOST_CHECK_EQUAL(it_parent_2->second.m_wtxids_fee_calculations.value().size(), 1);
    108+        BOOST_CHECK_EQUAL(it_parent_2->second.m_wtxids_fee_calculations.value().front(), tx_parent_2->GetWitnessHash());
    109+        BOOST_CHECK_EQUAL(it_child_2->second.m_wtxids_fee_calculations.value().size(), 1);
    110+        BOOST_CHECK_EQUAL(it_child_2->second.m_wtxids_fee_calculations.value().front(), tx_child_2->GetWitnessHash());
    111+        }
    112         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
    113 
    114+        // 1 parent paying 199sat, 1 child paying 1300sat.
    115+        Package package3{tx_parent_3, tx_child_3};
    116+        {
    117         // Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF feerate rules
    118         const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false, std::nullopt);
    119         if (auto err_3{CheckPackageMempoolAcceptResult(package3, submit3, /*expect_valid=*/true, m_node.mempool.get())}) {
    120@@ -1212,27 +1234,91 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests)
    121         BOOST_CHECK(it_child_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
    122         BOOST_CHECK_EQUAL(it_parent_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300);
    123         BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300);
    124-
    125+        }
    126         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
    127 
    128         // Finally, check that we can prioritise tx_child_1 to get package1 into the mempool.
    129         // It should not be possible to resubmit package1 and get it in without prioritisation.
    130+        // 1 parent paying 200sat, 1 child paying 200sat.
    131+        Package package4{package1};
    132+        {
    133         const auto submit4 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt);
    134-        if (auto err_4{CheckPackageMempoolAcceptResult(package1, submit4, /*expect_valid=*/false, m_node.mempool.get())}) {
    135+        if (auto err_4{CheckPackageMempoolAcceptResult(package4, submit4, /*expect_valid=*/false, m_node.mempool.get())}) {
    136             BOOST_ERROR(err_4.value());
    137         }
    138+        }
    139+
    140+        // 1 parent paying 200sat, 1 child paying 1563sat.
    141+        Package package5{package1};
    142         m_node.mempool->PrioritiseTransaction(tx_child_1->GetHash(), 1363);
    143-        const auto submit5 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt);
    144-        if (auto err_5{CheckPackageMempoolAcceptResult(package1, submit5, /*expect_valid=*/true, m_node.mempool.get())}) {
    145+        {
    146+        const auto submit5 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package5, false, std::nullopt);
    147+        if (auto err_5{CheckPackageMempoolAcceptResult(package5, submit5, /*expect_valid=*/true, m_node.mempool.get())}) {
    148             BOOST_ERROR(err_5.value());
    149         }
    150-        it_parent_1 = submit5.m_tx_results.find(tx_parent_1->GetWitnessHash());
    151-        it_child_1 = submit5.m_tx_results.find(tx_child_1->GetWitnessHash());
    152-        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    153-        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    154-        LOCK(m_node.mempool->cs);
    155-        BOOST_CHECK(m_node.mempool->GetIter(tx_parent_1->GetHash()).has_value());
    156-        BOOST_CHECK(m_node.mempool->GetIter(tx_child_1->GetHash()).has_value());
    157+        auto it_parent_5 = submit5.m_tx_results.find(tx_parent_1->GetWitnessHash());
    158+        auto it_child_5 = submit5.m_tx_results.find(tx_child_1->GetWitnessHash());
    159+        BOOST_CHECK_EQUAL(it_parent_5->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    160+        BOOST_CHECK_EQUAL(it_child_5->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    161+        // Validated together.
    162+        const auto expected_wtxids_5 = std::vector<Wtxid>({tx_parent_1->GetWitnessHash(), tx_child_1->GetWitnessHash()});
    163+        BOOST_CHECK(it_parent_5->second.m_wtxids_fee_calculations.value() == expected_wtxids_5);
    164+        BOOST_CHECK(it_child_5->second.m_wtxids_fee_calculations.value() == expected_wtxids_5);
    165+        const CFeeRate feerate_parent_child_5(1363 + 200 + 200, GetVirtualTransactionSize(*tx_parent_1) + GetVirtualTransactionSize(*tx_child_1));
    166+        BOOST_CHECK(it_parent_5->second.m_effective_feerate.value() == feerate_parent_child_5);
    167+        BOOST_CHECK(it_child_5->second.m_effective_feerate.value() == feerate_parent_child_5);
    168+        }
    169+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
    170+
    171+        // Check that the grouping for package RBF does not allow a parent to pay for a child below mempool minimum feerate.
    172+        // This parent cannot pay for its own replacement, and can get assistance from the child, but the child should
    173+        // not be eligible for submission at all.
    174+        // 1 parent paying 1763sat, 1 child paying 60sat.
    175+        Package package6{tx_parent_4, tx_child_4};
    176+        {
    177+        const auto submit6 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package6, false, std::nullopt);
    178+        if (auto err_6{CheckPackageMempoolAcceptResult(package6, submit6, /*expect_valid=*/false, m_node.mempool.get())}) {
    179+            BOOST_ERROR(err_6.value());
    180+        }
    181+        auto it_parent_6 = submit6.m_tx_results.find(tx_parent_4->GetWitnessHash());
    182+        auto it_child_6 = submit6.m_tx_results.find(tx_child_4->GetWitnessHash());
    183+        BOOST_CHECK_EQUAL(it_parent_6->second.m_result_type, MempoolAcceptResult::ResultType::INVALID);
    184+        BOOST_CHECK_EQUAL(it_child_6->second.m_result_type, MempoolAcceptResult::ResultType::INVALID);
    185+        // Parent does not meet rule 4
    186+        BOOST_CHECK_EQUAL(it_parent_6->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);
    187+        BOOST_CHECK_EQUAL(it_parent_6->second.m_state.GetRejectReason(), "insufficient fee");
    188+        // Child needs parent's output
    189+        BOOST_CHECK_EQUAL(it_child_6->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS);
    190+        BOOST_CHECK_EQUAL(it_child_6->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent");
    191+        }
    192+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
    193+
    194+        // It is ok if the child pays fewer fees but subsidizes the parent's replacement cost, as long as the child meets the mempool minimum feerate.
    195+        // 1 parent paying 1763sat, 1 child paying 160sat.
    196+        // Even though the parent has a higher feerate, this isn't "parent pays for child" because the child meets the mempool minimum feerate.
    197+        Package package7{package6};
    198+        m_node.mempool->PrioritiseTransaction(tx_child_4->GetHash(), 100);
    199+        {
    200+        const auto submit7 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package7, false, std::nullopt);
    201+        if (auto err_7{CheckPackageMempoolAcceptResult(package7, submit7, /*expect_valid=*/true, m_node.mempool.get())}) {
    202+            BOOST_ERROR(err_7.value());
    203+        }
    204+        auto it_parent_7 = submit7.m_tx_results.find(tx_parent_4->GetWitnessHash());
    205+        auto it_child_7 = submit7.m_tx_results.find(tx_child_4->GetWitnessHash());
    206+        BOOST_CHECK_EQUAL(it_parent_7->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    207+        BOOST_CHECK_EQUAL(it_child_7->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
    208+        // The two transactions were validated together.
    209+        BOOST_CHECK_EQUAL(it_parent_7->second.m_wtxids_fee_calculations.value().size(), 2);
    210+        BOOST_CHECK_EQUAL(it_parent_7->second.m_wtxids_fee_calculations.value().front(), tx_parent_4->GetWitnessHash());
    211+        BOOST_CHECK_EQUAL(it_parent_7->second.m_wtxids_fee_calculations.value().back(), tx_child_4->GetWitnessHash());
    212+        BOOST_CHECK_EQUAL(it_child_7->second.m_wtxids_fee_calculations.value().size(), 2);
    213+        BOOST_CHECK_EQUAL(it_child_7->second.m_wtxids_fee_calculations.value().front(), tx_parent_4->GetWitnessHash());
    214+        BOOST_CHECK_EQUAL(it_child_7->second.m_wtxids_fee_calculations.value().back(), tx_child_4->GetWitnessHash());
    215+        const CFeeRate feerate_parent_child_7(1763 + 160, GetVirtualTransactionSize(*tx_parent_4) + GetVirtualTransactionSize(*tx_child_4));
    216+        BOOST_CHECK(it_parent_7->second.m_effective_feerate.value() == feerate_parent_child_7);
    217+        BOOST_CHECK(it_child_7->second.m_effective_feerate.value() == feerate_parent_child_7);
    218+        }
    219+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
    220     }
    221 }
    222 BOOST_AUTO_TEST_SUITE_END()
    223diff --git a/src/validation.cpp b/src/validation.cpp
    224index 1102f2bb051..508892faea2 100644
    225--- a/src/validation.cpp
    226+++ b/src/validation.cpp
    227@@ -1048,8 +1048,23 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
    228         return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child");
    229     }
    230 
    231-    // If the package has in-mempool parents, we won't consider a package RBF
    232-    // since it would result in a cluster larger than 2.
    233+    // Use the child as the transaction for attributing errors because it is presumably sponsoring this package RBF.
    234+    auto& child_ws = workspaces[1];
    235+    const Txid& child_hash = child_ws.m_ptx->GetHash();
    236+
    237+    // Check if this child is eligible to be a sponsor. We allow a child with a lower feerate than that of its
    238+    // parents (and thus only helping pay for the replacement). However, the child itself must meet the mempool minimum
    239+    // feerate, otherwise it would sponsor this replacement, potentially using the parents' fees to pay for its entry,
    240+    // but be immediately evicted next time we trim the mempool.
    241+    if (!CheckFeeRate(child_ws.m_vsize, child_ws.m_modified_fees, child_ws.m_state)) {
    242+        // Add the CheckFeeRate error as the debug string.
    243+        return package_state.Invalid(PackageValidationResult::PCKG_POLICY,
    244+                                     "package RBF failed: child cannot sponsor because it does not meet mempool minimum feerate",
    245+                                     child_ws.m_state.ToString());
    246+    }
    247+
    248+
    249+    // If the package has in-mempool parents, we won't consider a package RBF for now.
    250     // N.B. To relax this constraint we will need to revisit how CCoinsViewMemPool::PackageAddTransaction
    251     // is being used inside AcceptMultipleTransactions to track available inputs while processing a package.
    252     // Specifically we would need to check that the ancestors of the new
    253@@ -1062,6 +1077,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
    254         }
    255     }
    256 
    257+    // The remaining checks are similar to those for a single transaction's replacement(s).
    258     // Aggregate all conflicts into one set.
    259     CTxMemPool::setEntries direct_conflict_iters;
    260     for (Workspace& ws : workspaces) {
    261@@ -1069,9 +1085,6 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
    262         direct_conflict_iters.merge(ws.m_iters_conflicting);
    263     }
    264 
    265-    const auto& parent_ws = workspaces[0];
    266-    const auto& child_ws = workspaces[1];
    267-
    268     // Don't consider replacements that would cause us to remove a large number of mempool entries.
    269     // This limit is not increased in a package RBF. Use the aggregate number of transactions.
    270     CTxMemPool::setEntries all_conflicts;
    271@@ -1087,8 +1100,6 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
    272         m_subpackage.m_conflicting_size += it->GetTxSize();
    273     }
    274 
    275-    // Use the child as the transaction for attributing errors to.
    276-    const Txid& child_hash = child_ws.m_ptx->GetHash();
    277     if (const auto err_string{PaysForRBF(/*original_fees=*/m_subpackage.m_conflicting_fees,
    278                                          /*replacement_fees=*/m_subpackage.m_total_modified_fees,
    279                                          /*replacement_vsize=*/m_subpackage.m_total_vsize,
    280@@ -1097,16 +1108,6 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
    281                                      "package RBF failed: insufficient anti-DoS fees", *err_string);
    282     }
    283 
    284-    // Ensure this two transaction package is a "chunk" on its own; we don't want the child
    285-    // to be only paying anti-DoS fees
    286-    const CFeeRate parent_feerate(parent_ws.m_modified_fees, parent_ws.m_vsize);
    287-    const CFeeRate package_feerate(m_subpackage.m_total_modified_fees, m_subpackage.m_total_vsize);
    288-    if (package_feerate <= parent_feerate) {
    289-        return package_state.Invalid(PackageValidationResult::PCKG_POLICY,
    290-                                     "package RBF failed: package feerate is less than or equal to parent feerate",
    291-                                     strprintf("package feerate %s <= parent feerate is %s", package_feerate.ToString(), parent_feerate.ToString()));
    292-    }
    293-
    294     // Check if it's economically rational to mine this package rather than the ones it replaces.
    295     // This takes the place of ReplacementChecks()'s PaysMoreThanConflicts() in the package RBF setting.
    296     if (const auto err_tup{ImprovesFeerateDiagram(*m_subpackage.m_changeset)}) {
    297diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
    298index 759e3cb07d3..c1983cab57b 100755
    299--- a/test/functional/mempool_package_rbf.py
    300+++ b/test/functional/mempool_package_rbf.py
    301@@ -178,14 +178,10 @@ class PackageRBFTest(BitcoinTestFramework):
    302         package_hex4, package_txns4 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE)
    303         node.submitpackage(package_hex4)
    304         self.assert_mempool_contents(expected=package_txns4)
    305-        package_hex5, _package_txns5 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE)
    306-        pkg_results5 = node.submitpackage(package_hex5)
    307-        assert 'package RBF failed: package feerate is less than or equal to parent feerate' in pkg_results5["package_msg"]
    308-        self.assert_mempool_contents(expected=package_txns4)
    309+        package_hex5, package_txns5 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE + DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE)
    310+        node.submitpackage(package_hex5)
    311+        self.assert_mempool_contents(expected=package_txns5)
    312 
    313-        package_hex5_1, package_txns5_1 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE + Decimal("0.00000001"))
    314-        node.submitpackage(package_hex5_1)
    315-        self.assert_mempool_contents(expected=package_txns5_1)
    316         self.generate(node, 1)
    317 
    318     def test_package_rbf_max_conflicts(self):
    

    sdaftuar commented at 8:07 pm on October 20, 2025:
    Agreed, though I think this type of behavior change regarding what packages we accept can happen in a separate PR after cluster mempool. (In particular I think it’s worth a separate discussion around what use cases we would be trying to enable with such a change.)

    glozow commented at 9:35 am on October 21, 2025:
    Great, marking resolved and will refer to this in a followup!
  56. glozow commented at 8:19 pm on October 17, 2025: member
    Focused on reviewing the RBF logic in e6315c24326016cfaee5bd046e8b2e4e1088ac6b
  57. sdaftuar force-pushed on Oct 21, 2025
  58. sdaftuar force-pushed on Oct 21, 2025
  59. in src/txmempool.cpp:1005 in 1ae9fd3f7a outdated
    1134+            if (piter) {
    1135+                m_pool->m_txgraph->AddDependency(**piter, *entryptr);
    1136+            }
    1137+        }
    1138+    }
    1139+    m_dependencies_processed = true;
    


    brunoerg commented at 6:58 am on October 22, 2025:

    Unkilled mutant:

     0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
     1index b0c9ab043c..8d7e4d2bb9 100644
     2--- a/src/txmempool.cpp
     3+++ b/src/txmempool.cpp
     4@@ -994,7 +994,7 @@ void CTxMemPool::ChangeSet::ProcessDependencies()
     5             }
     6         }
     7     }
     8-    m_dependencies_processed = true;
     9+    m_dependencies_processed = false;
    10     return;
    11  }
    
  60. in src/txmempool.cpp:132 in 1ae9fd3f7a outdated
    268-                if (staged_ancestors.size() + package.size() > static_cast<uint64_t>(m_opts.limits.ancestor_count)) {
    269-                    return util::Error{Untranslated(strprintf("too many unconfirmed parents [limit: %u]", m_opts.limits.ancestor_count))};
    270-                }
    271+    auto ancestors = m_txgraph->GetAncestors(entry, TxGraph::Level::MAIN);
    272+    setEntries ret;
    273+    if (ancestors.size() > 0) {
    


    brunoerg commented at 7:04 am on October 22, 2025:

    Unkilled mutant:

     0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
     1index b0c9ab043c..aef4fdde8d 100644
     2--- a/src/txmempool.cpp
     3+++ b/src/txmempool.cpp
     4@@ -129,7 +129,7 @@ CTxMemPool::setEntries CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEnt
     5 {
     6     auto ancestors = m_txgraph->GetAncestors(entry, TxGraph::Level::MAIN);
     7     setEntries ret;
     8-    if (ancestors.size() > 0) {
     9+    if (ancestors.size() < 0) {
    10         for (auto ancestor : ancestors) {
    11             if (ancestor != &entry) {
    12                 ret.insert(mapTx.iterator_to(static_cast<const CTxMemPoolEntry&>(*ancestor)));
    
  61. in src/txmempool.cpp:124 in 1ae9fd3f7a outdated
    236-    }
    237-
    238-    return ancestors;
    239+    LOCK(cs);
    240+    auto entry = GetEntry(txid);
    241+    if (!entry) return false;
    


    brunoerg commented at 7:07 am on October 22, 2025:

    Unkilled mutant:

     0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
     1index b0c9ab043c..bdbbc8325d 100644
     2--- a/src/txmempool.cpp
     3+++ b/src/txmempool.cpp
     4@@ -121,7 +121,7 @@ bool CTxMemPool::HasDescendants(const Txid& txid) const
     5 {
     6     LOCK(cs);
     7     auto entry = GetEntry(txid);
     8-    if (!entry) return false;
     9+    if (1==0) return false;
    10     return m_txgraph->GetDescendants(*entry, TxGraph::Level::MAIN).size() > 1;
    11 }
    
  62. in test/functional/mempool_package_limits.py:29 in 1ae9fd3f7a outdated
    25@@ -26,7 +26,7 @@ def func_wrapper(self, *args, **kwargs):
    26         testres_error_expected = node.testmempoolaccept(rawtxs=package_hex)
    27         assert_equal(len(testres_error_expected), len(package_hex))
    28         for txres in testres_error_expected:
    29-            assert "package-mempool-limits" in txres["package-error"]
    30+            assert "too-large-cluster" in txres["package-error"]
    


    instagibbs commented at 7:08 am on October 22, 2025:
    there’s a single lasting reference to “package-mempool-limits” in the comment in this file

    sdaftuar commented at 11:02 pm on October 30, 2025:
    Fixed in f784ae88327c61b08684e1e073b010a244e2b52e.
  63. in src/txmempool.h:403 in 1ae9fd3f7a outdated
    396+    FeePerVSize GetMainChunkFeerate(const CTxMemPoolEntry& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs) {
    397+        return ToFeePerVSize(m_txgraph->GetMainChunkFeerate(tx));
    398+    }
    399+    std::vector<const CTxMemPoolEntry*> GetCluster(Txid txid) const EXCLUSIVE_LOCKS_REQUIRED(cs) {
    400+        auto tx = GetIter(txid);
    401+        if (!tx) return {};
    


    brunoerg commented at 7:27 am on October 22, 2025:

    Unkilled mutant:

     0diff --git a/src/txmempool.h b/src/txmempool.h
     1index 4aeae00f69..c4332f3344 100644
     2--- a/src/txmempool.h
     3+++ b/src/txmempool.h
     4@@ -398,7 +398,7 @@ public:
     5     }
     6     std::vector<const CTxMemPoolEntry*> GetCluster(Txid txid) const EXCLUSIVE_LOCKS_REQUIRED(cs) {
     7         auto tx = GetIter(txid);
     8-        if (!tx) return {};
     9+        if (1==0) return {};
    10         auto cluster = m_txgraph->GetCluster(**tx, TxGraph::Level::MAIN);
    11         std::vector<const CTxMemPoolEntry*> ret;
    12         ret.reserve(cluster.size());
    
  64. in src/txmempool.cpp:958 in 1ae9fd3f7a outdated
    1087     m_pool->ApplyDelta(tx->GetHash(), delta);
    1088-    if (delta) m_to_add.modify(newit, [&delta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(delta); });
    1089+
    1090+    TxGraph::Ref ref(m_pool->m_txgraph->AddTransaction(FeePerWeight(fee+delta, GetSigOpsAdjustedWeight(GetTransactionWeight(*tx), sigops_cost, ::nBytesPerSigOp))));
    1091+    auto newit = m_to_add.emplace(std::move(ref), tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first;
    1092+    if (delta) {
    


    brunoerg commented at 7:39 am on October 22, 2025:

    Unkilled mutant:

     0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
     1index b0c9ab043c..9142d8d2c6 100644
     2--- a/src/txmempool.cpp
     3+++ b/src/txmempool.cpp
     4@@ -947,7 +947,7 @@ CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTran
     5
     6     TxGraph::Ref ref(m_pool->m_txgraph->AddTransaction(FeePerWeight(fee+delta, GetSigOpsAdjustedWeight(GetTransactionWeight(*tx), sigops_cost, ::nBytesPerSigOp))));
     7     auto newit = m_to_add.emplace(std::move(ref), tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first;
     8-    if (delta) {
     9+    if (1==1) {
    10         m_to_add.modify(newit, [&delta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(delta); });
    11     }
    

    glozow commented at 1:35 pm on October 28, 2025:
    Yes, calling UpdateModifiedFee(0) does nothing. I think it’s fine to keep this :shrug:

    brunoerg commented at 4:42 pm on October 28, 2025:
    Perfect, I wasn’t aware of UpdateModifiedFee, I just took a look and this is a saturated addition, so it won’t have any effect. This can be ignored.
  65. in src/test/rbf_tests.cpp:228 in 1ae9fd3f7a outdated
    270+    // If we mine the parent_tx's, then the clusters split (102 clusters).
    271+    pool.removeForBlock({parent_tx_1, parent_tx_2}, /* dummy */ 1);
    272+
    273+    // Add some descendants now to each of the direct children (we can do this now that the clusters have split).
    274+    for (const auto& child : direct_children) {
    275+        add_descendants(child, 10, pool);
    


    brunoerg commented at 7:53 am on October 22, 2025:

    Removing this doesn’t make the test to fail:

     0diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp
     1index 45dd8bcb12..8ef8f0618c 100644
     2--- a/src/test/rbf_tests.cpp
     3+++ b/src/test/rbf_tests.cpp
     4@@ -225,7 +225,7 @@ BOOST_FIXTURE_TEST_CASE(rbf_conflicts_calculator, TestChain100Setup)
     5
     6     // Add some descendants now to each of the direct children (we can do this now that the clusters have split).
     7     for (const auto& child : direct_children) {
     8-        add_descendants(child, 10, pool);
     9+
    10     }
    
  66. in src/policy/truc_policy.cpp:207 in 1ae9fd3f7a outdated
    203@@ -195,48 +204,57 @@ std::optional<std::pair<std::string, CTransactionRef>> SingleTRUCChecks(const CT
    204     }
    205 
    206     // Check that TRUC_ANCESTOR_LIMIT would not be violated.
    207-    if (mempool_ancestors.size() + 1 > TRUC_ANCESTOR_LIMIT) {
    208+    if (mempool_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) {
    


    brunoerg commented at 9:07 am on October 22, 2025:

    Unkilled mutant:

     0diff --git a/src/policy/truc_policy.cpp b/src/policy/truc_policy.cpp
     1index dd16eccb7c..c60dff899f 100644
     2--- a/src/policy/truc_policy.cpp
     3+++ b/src/policy/truc_policy.cpp
     4@@ -204,7 +204,7 @@ std::optional<std::pair<std::string, CTransactionRef>> SingleTRUCChecks(const CT
     5     }
     6
     7     // Check that TRUC_ANCESTOR_LIMIT would not be violated.
     8-    if (mempool_parents.size() + 1 > TRUC_ANCESTOR_LIMIT) {
     9+    if (mempool_parents.size() / 1 > TRUC_ANCESTOR_LIMIT) {
    10         return std::make_pair(strprintf("tx %s (wtxid=%s) would have too many ancestors",
    11                          ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()),
    12             nullptr);
    
  67. brunoerg commented at 10:00 am on October 22, 2025: contributor
    I ran a mutation testing analysis on this PR. The mutants were generated incrementally. Unit and functional tests look great overall - over 80% of mutation score. I’ve dropped some unkilled mutants here, feel free to address them (write tests to kill) or ignore if unproductive.
  68. in src/bench/mempool_stress.cpp:272 in 4c359dabd2 outdated
    267+
    268+BENCHMARK(MemPoolAncestorsDescendants, benchmark::PriorityLevel::HIGH);
    269+BENCHMARK(MemPoolAddTransactions, benchmark::PriorityLevel::HIGH);
    270 BENCHMARK(ComplexMemPool, benchmark::PriorityLevel::HIGH);
    271 BENCHMARK(MempoolCheck, benchmark::PriorityLevel::HIGH);
    272+//BENCHMARK(MemPoolMiningScoreCheck, benchmark::PriorityLevel::HIGH);
    


    glozow commented at 1:17 pm on October 28, 2025:

    In 4c359dabd2a7ca5a5d7469070f55e66a88196291

    Is this benchmark supposed to be deleted? It seems like it was written a long time ago.


    sdaftuar commented at 12:40 pm on October 30, 2025:
    Oops, yes I think this must be a holdover from a very early draft of this PR, prior to txgraph. I’ll remove.

    sdaftuar commented at 10:58 pm on October 30, 2025:
    Gone now in 110e62ce76f029e32eeed44238db8d3c09ddb2f7.
  69. in src/txmempool.h:53 in 83c8753abf outdated
    49@@ -50,6 +50,9 @@ struct bilingual_str;
    50 /** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */
    51 static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF;
    52 
    53+/** How many linearization iterations required for TxGraph clusters */
    


    glozow commented at 1:28 pm on October 28, 2025:

    in 83c8753abf9c2af75cfbacd5488605332da588a4, I think this is missing a few words?

    0/** How many linearization iterations required for TxGraph clusters to have "acceptable" quality, if they cannot be optimally linearized with fewer iterations. */
    

    sdaftuar commented at 10:59 pm on October 30, 2025:
    Done in a6649dffe92319a17790bf2add5206a84726f34d.
  70. in src/txmempool.cpp:416 in 91d9bfcca6 outdated
    412@@ -413,6 +413,7 @@ static CTxMemPool::Options&& Flatten(CTxMemPool::Options&& opts, bilingual_str&
    413 CTxMemPool::CTxMemPool(Options opts, bilingual_str& error)
    414     : m_opts{Flatten(std::move(opts), error)}
    415 {
    416+    m_txgraph = MakeTxGraph(64, 101'000, 10'000);
    


    glozow commented at 1:37 pm on October 28, 2025:

    nit 91d9bfcca62bd6c168476f6af442bd7049ff872e

    I was going to complain about this magic number, maybe squash 83c8753abf9c2af75cfbacd5488605332da588a4 into its parent?


    sdaftuar commented at 11:00 pm on October 30, 2025:
    This was squashed.
  71. in src/txmempool.cpp:594 in 2812471c2b outdated
    892@@ -891,6 +893,7 @@ void CTxMemPool::PrioritiseTransaction(const Txid& hash, const CAmount& nFeeDelt
    893         delta = SaturatingAdd(delta, nFeeDelta);
    894         txiter it = mapTx.find(hash);
    895         if (it != mapTx.end()) {
    896+            m_txgraph->SetTransactionFee(*it, it->GetModifiedFee() + nFeeDelta);
    


    glozow commented at 1:42 pm on October 28, 2025:

    2812471c2bfce6215d6d58c1defd39c7a050edbd: Maybe deserves a comment

    0            // PrioritiseTransaction calls stack on previous ones. Set the new transaction fee to be current modified fee + feedelta.
    1            m_txgraph->SetTransactionFee(*it, it->GetModifiedFee() + nFeeDelta);
    

    sdaftuar commented at 11:00 pm on October 30, 2025:
    Done in b0c8081258fc37816e4675e17fa9ea8d308921f3
  72. in src/test/fuzz/tx_pool.cpp:133 in 0e25736d6e outdated
    124@@ -112,6 +125,19 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
    125         assert(tx_pool.size() < info_all.size());
    126         WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
    127     }
    128+
    129+    if (fuzzed_data_provider.ConsumeBool()) {
    130+        // Try eviction
    131+        LOCK2(::cs_main, tx_pool.cs);
    132+        tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage()));
    133+        tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1);
    


    glozow commented at 2:46 pm on October 28, 2025:

    in 0e25736d6e0

    It would help the fuzzer run faster to just call check once at the end, instead of after each round of potential removals.


    sdaftuar commented at 11:01 pm on October 30, 2025:
    Done in 0097138011b6e502bfbae528304a6a7e6b6462ad.
  73. in src/validation.cpp:1363 in 675fa79bb2 outdated
    1334@@ -1354,6 +1335,32 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
    1335         return MempoolAcceptResult::Failure(ws.m_state);
    1336     }
    1337 
    1338+    // Now that we've verified the cluster limit is respected, we can perform
    1339+    // calculations involving the full ancestors of the tx.
    


    glozow commented at 3:07 pm on October 28, 2025:
    It seems a bit unfortunate that, prior to 675fa79bb280129fb87e5d1884e670a35a03f5cd, there isn’t a cap on ancestor count when we call CalculateMemPoolAncestors. Could the rework be done earlier?

    sdaftuar commented at 5:36 pm on October 30, 2025:

    I took a stab at it and it’s a bit tedious (I managed to move it up earlier in the commit history, but stopped when I got to a conflict with a commit that also touches truc_policy in different ways).

    My overall take is it’s not such a big deal, as the concern is not around correctness of the code but a potential CPU DoS if exposed over the network, which doesn’t strike me as all that concerning for an intermediate commit.

  74. glozow commented at 3:15 pm on October 28, 2025: member
    The empty commits can be dropped. You could prefix some commits with “prep” or “cleanup,” but I think it’s pretty clear already.
  75. in src/txmempool.cpp:430 in 1ae9fd3f7a
    427     CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
    428 
    429-    for (const auto& it : GetSortedDepthAndScore()) {
    430+    std::optional<Wtxid> last_wtxid = std::nullopt;
    431+
    432+    for (const auto& it : GetSortedScoreWithTopology()) {
    


    instagibbs commented at 4:59 pm on October 29, 2025:

    Adds some GetFeerateDiagram coverage that will get hit in various contexts including fuzz

     0diff --git a/src/txmempool.cpp b/src/txmempool.cpp
     1index b0c9ab043c..3dcc0dd3e4 100644
     2--- a/src/txmempool.cpp
     3+++ b/src/txmempool.cpp
     4@@ -426,7 +426,13 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
     5     CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
     6 
     7+    const auto score_with_topo{GetSortedScoreWithTopology()};
     8+
     9+    // Number of chunks bounded by number of txs
    10+    const auto diagram{GetFeerateDiagram()};
    11+    assert(diagram.size() <= score_with_topo.size());
    12+
    13     std::optional<Wtxid> last_wtxid = std::nullopt;
    14 
    15-    for (const auto& it : GetSortedScoreWithTopology()) {
    16+    for (const auto& it : score_with_topo) {
    17         checkTotal += it->GetTxSize();
    18         check_total_fee += it->GetFee();
    

    sdaftuar commented at 11:03 pm on October 30, 2025:
    There was an off-by-one here (feerate diagrams start with a [0, 0]), but otherwise I incorporated this into ed23b4537ae8a8a814738909fe9a84c548f2855d.
  76. in src/test/fuzz/tx_pool.cpp:115 in 1ae9fd3f7a
    110+        for (const auto& tx : block_template->block.vtx) {
    111+            const auto res = AcceptToMemoryPool(chainstate, tx, GetTime(), true, /*test_accept=*/false);
    112+            if (res.m_result_type == MempoolAcceptResult::ResultType::VALID) {
    113+                hashes_to_update.push_back(tx->GetHash());
    114+            } else {
    115+                tx_pool.removeRecursive(*tx, MemPoolRemovalReason::REORG /* dummy */);
    


    instagibbs commented at 5:03 pm on October 29, 2025:
    what does dummy mean here?

    sdaftuar commented at 9:32 am on October 31, 2025:

    I think this was a copy-paste thing from elsewhere in the code (this “dummy” comment appears at line 123 as well, and seems to have been introduced when this fuzz test was first created) – I presume the origin is just that the MemPoolRemovalReason doesn’t matter for how removeRecursive() behaves, it’s just passed on to callbacks.

    Anyway I’ve removed it from this line.

  77. in src/test/fuzz/tx_pool.cpp:132 in 1ae9fd3f7a
    124@@ -112,6 +125,19 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
    125         assert(tx_pool.size() < info_all.size());
    126         WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
    127     }
    128+
    129+    if (fuzzed_data_provider.ConsumeBool()) {
    130+        // Try eviction
    131+        LOCK2(::cs_main, tx_pool.cs);
    132+        tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage()));
    


    instagibbs commented at 5:04 pm on October 29, 2025:

    no-op trimming makes sense too (probably a smarter of way of having this hit than I suggest here)

    0        tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage() * 2));
    

    sdaftuar commented at 9:33 am on October 31, 2025:
    Incorporated in 0097138011b6
  78. in src/test/miner_tests.cpp:60 in 1ae9fd3f7a
    55@@ -56,7 +56,9 @@ struct MinerTestingSetup : public TestingSetup {
    56         // instead.
    57         m_node.mempool.reset();
    58         bilingual_str error;
    59-        m_node.mempool = std::make_unique<CTxMemPool>(MemPoolOptionsForTest(m_node), error);
    60+        auto opts = MemPoolOptionsForTest(m_node);
    61+        opts.limits.cluster_size_vbytes = 1'250'000;
    


    instagibbs commented at 5:10 pm on October 29, 2025:
    could we get this magic number explained?

    sdaftuar commented at 9:34 am on October 31, 2025:
    Done in d00d8a8a84e43cb79e81482a2a85532209dd9ab2
  79. in src/test/miner_tests.cpp:337 in 1ae9fd3f7a outdated
    343-        }
    344+        auto txs = CreateBigSigOpsCluster(txFirst[0]);
    345 
    346+        int64_t legacy_sigops = 0;
    347+        for (auto& t : txs) {
    348+            AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(t));
    


    instagibbs commented at 5:19 pm on October 29, 2025:

    without comment I found this hard to understand

    0            // If we don't set the # of sig ops in the CTxMemPoolEntry, template creation fails during sanity checks
    1            AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(t));
    

    sdaftuar commented at 9:34 am on October 31, 2025:
    Done in d00d8a8a84e43cb79e81482a2a85532209dd9ab2
  80. in test/functional/mempool_cluster.py:98 in 1ae9fd3f7a
    93+
    94+        # Test that adding one more transaction to the cluster will fail.
    95+        bad_tx = self.wallet.create_self_transfer(utxo_to_spend=last_result["new_utxo"], target_vsize=target_vsize_per_tx)
    96+        assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, bad_tx["hex"])
    97+
    98+        # It should also work during replacement
    


    instagibbs commented at 7:04 pm on October 29, 2025:
    0        # It should also limit cluster sizes during replacement
    

    sdaftuar commented at 9:35 am on October 31, 2025:
    Done in 74654eaf1bef5b578b97111af20b3de39c39cec6
  81. in test/functional/mempool_cluster.py:104 in 1ae9fd3f7a
     99+        utxo_to_double_spend = self.wallet.get_utxo(confirmed_only=True)
    100+        fee = Decimal("0.000001")
    101+        tx_to_replace = self.wallet.create_self_transfer(utxo_to_spend=utxo_to_double_spend, fee=fee)
    102+        node.sendrawtransaction(tx_to_replace["hex"])
    103+
    104+        # Multiply fee by 5, which should easily cover the cost to replace. Otherwise, use the target vsize at 10sat/vB
    


    instagibbs commented at 7:04 pm on October 29, 2025:
    0        # Multiply fee by 5, which should easily cover the cost to replace (but is still too large a cluster). Otherwise, use the target vsize at 10sat/vB
    

    sdaftuar commented at 9:35 am on October 31, 2025:
    Done in 74654eaf1bef5b578b97111af20b3de39c39cec6
  82. in src/test/fuzz/tx_pool.cpp:105 in 1ae9fd3f7a outdated
     99@@ -104,6 +100,23 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
    100         auto assembler = BlockAssembler{chainstate, &tx_pool, options};
    101         auto block_template = assembler.CreateNewBlock();
    102         Assert(block_template->block.vtx.size() >= 1);
    103+
    104+        // Try updating the mempool for this block, as though it were mined.
    105+        LOCK2(::cs_main, tx_pool.cs);
    


    instagibbs commented at 7:30 pm on October 29, 2025:

    Since this harness is making blocks, I think it’d be good to have sigops naturally appearing in the constructed blocks. Ran this for a bit, seemed to work?

     0diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
     1index 8a5863c5d5..413ab0db4c 100644
     2--- a/src/test/fuzz/tx_pool.cpp
     3+++ b/src/test/fuzz/tx_pool.cpp
     4@@ -291,9 +291,18 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
     5                 tx_mut.vin.push_back(in);
     6             }
     7+
     8+            // Check sigops in mempool + block template creation
     9+            bool add_sigops{fuzzed_data_provider.ConsumeBool()};
    10+
    11             const auto amount_fee = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-1000, amount_in);
    12             const auto amount_out = (amount_in - amount_fee) / num_out;
    13             for (int i = 0; i < num_out; ++i) {
    14-                tx_mut.vout.emplace_back(amount_out, P2WSH_OP_TRUE);
    15+                if (i == 0 && add_sigops) {
    16+                    tx_mut.vout.emplace_back(amount_out, CScript() << std::vector<unsigned char>(33, 0x02) << OP_CHECKSIG);
    17+                } else {
    18+                    tx_mut.vout.emplace_back(amount_out, P2WSH_OP_TRUE);
    19+                }
    20             }
    21+
    22             auto tx = MakeTransactionRef(tx_mut);
    23             // Restore previously removed outpoints
    

    sdaftuar commented at 9:36 am on October 31, 2025:
    Done in 0097138011b6e502bfbae528304a6a7e6b6462ad
  83. instagibbs approved
  84. instagibbs commented at 7:36 pm on October 29, 2025: member

    ACK 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a

    lots of less important things to clean up, I’d rather get this branch further under (fuzz) test and have iteration done on cleanup PR

  85. Create a txgraph inside CTxMemPool a6649dffe9
  86. Add sigops adjusted weight calculator 43ba20ff99
  87. Add accessor for sigops-adjusted weight 8ca3bb384a
  88. Add transactions to txgraph, but without cluster dependencies
    Effectively this is treating all transactions in txgraph as being in a cluster
    of size 1.
    b0c8081258
  89. ajtowns requested review from ajtowns on Oct 30, 2025
  90. sdaftuar force-pushed on Oct 30, 2025
  91. sdaftuar commented at 9:49 am on October 31, 2025: member

    I’ve addressed most of the review feedback, other than the mutation testing notes from @brunoerg which I still need to review (thank you for the analysis!), and some documentation items that were brought up here and here.

    I’ll work on incorporating the documentation improvements in #33591.

  92. instagibbs approved
  93. instagibbs commented at 2:43 pm on October 31, 2025: member

    reACK ed23b4537ae8a8a814738909fe9a84c548f2855d

    git range-diff master 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a ed23b4537ae8a8a814738909fe9a84c548f2855d

  94. sdaftuar commented at 1:26 am on November 1, 2025: member
    Looks like there’s a thread safety issue ([CI run in #33591 caught it](https://github.com/bitcoin/bitcoin/actions/runs/18986107827/job/54230018048?pr=33591)); if I understand correctly, it looks like in AcceptSingleTransaction() we acquire the lock and release it before the MemPoolAccept object is destroyed, causing a change to txgraph (when the changeset is destroyed) while the lock is not held. Will fix.
  95. squashme: fix locking so that mempool lock is held through subpackage destruction 51d213d144
  96. Add new (unused) limits for cluster size/count 3963e1df5e
  97. test: update feature_rbf.py replacement test
    Preparatory commit to the rbf functional test, before changes are made to the
    rbf rules as part of cluster mempool.
    ede26116de
  98. [test] rework/delete feature_rbf tests requiring large clusters c88272447d
  99. Do not allow mempool clusters to exceed configured limits
    Include an adjustment to mempool_tests.cpp due to the additional memory used by
    txgraph.
    
    Includes a temporary change to the mempool_ephemeral_dust.py functional test,
    due to validation checks being reordered. This change will revert once the RBF
    rules are changed in a later commit.
    6b791cc4ee
  100. Check cluster limits when using -walletrejectlongchains 2593995b59
  101. Rework miner_tests to not require large cluster limit 83555789f4
  102. Limit mempool size based on chunk feerate
    Rather than evicting the transactions with the lowest descendant feerate,
    instead evict transactions that have the lowest chunk feerate.
    
    Once mining is implemented based on choosing transactions with highest chunk
    feerate (see next commit), mining and eviction will be opposites, so that we
    will evict the transactions that would be mined last.
    87d9e2856f
  103. bench: rewrite ComplexMemPool to not create oversized clusters 63c2b58d07
  104. Select transactions for blocks based on chunk feerate
    Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
    35679dbe04
  105. test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits d2496bb5a7
  106. policy: Remove CPFP carveout rule
    The addition of a cluster size limit makes the CPFP carveout rule useless,
    because carveout cannot be used to bypass the cluster size limit. Remove this
    policy rule and update tests to no longer rely on the behavior.
    b1b10b2923
  107. Implement new RBF logic for cluster mempool
    With a total ordering on mempool transactions, we are now able to calculate a
    transaction's mining score at all times. Use this to improve the RBF logic:
    
    - we no longer enforce a "no new unconfirmed parents" rule
    
    - we now require that the mempool's feerate diagram must improve in order
      to accept a replacement
    
    - the topology restrictions for conflicts in the package rbf setting have been
      eliminated
    
    Revert the temporary change to mempool_ephemeral_dust.py that were previously
    made due to RBF validation checks being reordered.
    
    Co-authored-by: Gregory Sanders <gsanders87@gmail.com>, glozow <gloriajzhao@gmail.com>
    187f78991f
  108. Remove the ancestor and descendant indices from the mempool ce44903d49
  109. Use cluster linearization for transaction relay sort order
    Previously, transaction batches were first sorted by ancestor count and then
    feerate, to ensure transactions are announced in a topologically valid order,
    while prioritizing higher feerate transactions. Ancestor count is a crude
    topological sort criteria, so replace this with linearization order so that the
    highest feerate transactions (as would be observed by the mining algorithm) are
    relayed before lower feerate ones, in a topologically valid way.
    
    This also fixes a test that only worked due to the ancestor-count-based sort
    order.
    d2e4eedd8f
  110. Remove CTxMemPool::GetSortedDepthAndScore
    The mempool clusters and linearization permit sorting the mempool topologically
    without making use of ancestor counts (as long as the graph is not oversized).
    
    Co-authored-by: Pieter Wuille <pieter@wuille.net>
    da5a530761
  111. Reimplement GetTransactionAncestry() to not rely on cached data
    In preparation for removing ancestor data from CTxMemPoolEntry, recalculate the
    ancestor statistics on demand wherever needed.
    e7a1cf82bd
  112. rpc: Calculate ancestor data from scratch for mempool rpc calls e59f9eafe0
  113. Remove dependency on cached ancestor data in mini-miner b0f1cf8977
  114. Stop enforcing ancestor size/count limits
    The cluster limits should be sufficient.
    
    Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
    df783fd2f7
  115. Add test case for cluster size limits to TRUC logic 7a25fb7e8e
  116. Use mempool/txgraph to determine if a tx has descendants
    Remove a reference to GetCountWithDescendants() in preparation for removing
    this function and the associated cached state from the mempool.
    11f0f8d510
  117. Calculate descendant information for mempool RPC output on-the-fly
    This is in preparation for removing the cached descendant state from the
    mempool.
    279ee6fdef
  118. test: remove rbf carveout test from mempool_limit.py d75594777c
  119. Stop enforcing descendant size/count limits
    Cluster size limits should be enough.
    97c803d85a
  120. Eliminate RBF workaround for CPFP carveout transactions
    The new cluster mempool RBF rules take into account clusters sizes exactly, so
    with the removal of descendant count enforcement this idea is obsolete.
    dafa1da093
  121. wallet: Replace max descendantsize with clustersize
    With the descendant size limits removed, replace the concept of "max number of
    descendants of any ancestor of a given tx" with the cluster count of the cluster
    that the transaction belongs to.
    cf1bcbbb8e
  122. mempool: Remove unused function CalculateDescendantMaximum b6992f9b48
  123. Eliminate use of cached ancestor data in miniminer_tests and truc_policy bd8164be1f
  124. mempool: eliminate accessors to mempool entry ancestor/descendant cached state 97126ca605
  125. Remove unused members from CTxMemPoolEntry 4f3c72da1c
  126. Remove mempool logic designed to maintain ancestor/descendant state 9b28e7b935
  127. mempool: addUnchecked no longer needs ancestors 44d70449d2
  128. Remove unused limits from CalculateMemPoolAncestors b0a0348947
  129. Make removeConflicts private 4a8b38b9cd
  130. Simplify ancestor calculation functions
    Now that ancestor calculation never fails (due to ancestor/descendant limits
    being eliminated), we can eliminate the error handling from
    CalculateMemPoolAncestors.
    658a6a817d
  131. Use txgraph to calculate ancestors fa428af0bc
  132. Use txgraph to calculate descendants 828a758324
  133. Rework truc_policy to use descendants, not children d6ffca78ee
  134. Rework RBF and TRUC validation
    Calculating mempool ancestors for a new transaction should not be done until
    after cluster size limits have been enforced, to limit CPU DoS potential.
    
    Achieve this by reworking TRUC and RBF validation logic:
    
    - TRUC policy enforcement is now done using only mempool parents of
      new transactions, not all mempool ancestors (note that it's fine to calculate
      ancestors of in-mempool transactions, if the number of such calls is
      reasonably bounded).
    - RBF replacement checks are performed earlier (which allows for checking
      cluster size limits earlier, because cluster size checks cannot happen until
      after all conflicts are staged for removal).
    - Verifying that a new transaction doesn't conflict with an ancestor now
      happens later, in AcceptSingleTransaction() rather than in PreChecks(). This
      means that the test is not performed at all in AcceptMultipleTransactions(),
      but in package acceptance we already disallow RBF in situations where a
      package transaction has in-mempool parents.
    
    Also to ensure that all RBF validation logic is applied in both the single
    transaction and multiple transaction cases, remove the optimization that skips
    the PackageMempoolChecks() in the case of a single transaction being validated
    in AcceptMultipleTransactions().
    5be8478dd2
  135. Make getting parents/children a function of the mempool, not a mempool entry e9e2690100
  136. Eliminate CheckPackageLimits, which no longer does anything e8aea13471
  137. Fix miniminer_tests to work with cluster limits d2cae03866
  138. Rewrite GatherClusters to use the txgraph implementation e55c92e3cf
  139. Stop tracking parents/children outside of txgraph a9bc6bb292
  140. Avoid violating mempool policy limits in tests
    Changes AddToMempool() helper to only apply changes if the mempool limits are
    respected.
    
    Fix package_rbf fuzz target to handle mempool policy violations
    ca2514c685
  141. bench: add more mempool benchmarks
    Add benchmarks for:
    
      - adding a transaction
      - calculating mempool ancestors/descendants
    9203a3774e
  142. fuzz: try to add more code coverage for mempool fuzzing
    Including test coverage for mempool eviction and expiry
    04a1f0d225
  143. Expose cluster information via rpc
    Co-authored-by: glozow <gloriajzhao@gmail.com>
    32f01e75c6
  144. doc: Update mempool_replacements.md to reflect feerate diagram checks b1d8c5f395
  145. test: add functional test for new cluster mempool RPCs
    Co-authored-by: glozow <gloriajzhao@gmail.com>
    9ff41c34b9
  146. fuzz: remove comparison between mini_miner block construction and miner
    This is in preparation for eliminating the block template building happening in
    mini_miner, in favor of directly using the linearizations done in the mempool.
    97e03bec46
  147. Invoke TxGraph::DoWork() at appropriate times 3024337513
  148. Update comments for CTxMemPool class ca5c96d728
  149. Add check that GetSortedScoreWithTopology() agrees with CompareMiningScoreWithTopology()
    We use CompareMiningScoreWithTopology() for sorting transaction announcements
    during tx relay, and we use GetSortedScoreWithTopology() in
    CTxMemPool::check().
    6362bf8e23
  150. doc: update policy/packages.md for new package acceptance logic 36c94d2569
  151. test: extend package rbf functional test to larger clusters
    Co-Authored-By: Gregory Sanders <gsanders87@gmail.com>
    241d9096e9
  152. Sanity check `GetFeerateDiagram()` in CTxMemPool::check() 02ac4a1e7c
  153. sdaftuar force-pushed on Nov 1, 2025
  154. sdaftuar commented at 5:59 pm on November 1, 2025: member

    Implemented a fix for the locking issue in 51d213d1449b61665628ce9b4e0209cff3c4f818. Left it unsquashed for easier review; if this approach seems ok I’ll squash it with the prior commit.

    FYI for reviewers: the insertion of the AssertLockHeld() in txmempool.h in that commit is what made the bug easy to reproduce in our tests (using a debug build).

  155. instagibbs commented at 1:37 pm on November 4, 2025: member
    https://github.com/bitcoin/bitcoin/commit/51d213d1449b61665628ce9b4e0209cff3c4f818#r169601405 left a comment in the non-PR review mode, just making sure it gets noticed
  156. glozow commented at 0:04 am on November 7, 2025: member

    it looks like in AcceptSingleTransaction() we acquire the lock and release it before the MemPoolAccept object is destroyed, causing a change to txgraph (when the changeset is destroyed) while the lock is not held. Will fix.

    Implemented a fix for the locking issue in https://github.com/bitcoin/bitcoin/commit/51d213d1449b61665628ce9b4e0209cff3c4f818.

    IIUC we need to ensure we don’t release the mempool lock until after the changeset (i.e. SubPackageState) is destroyed.

    The approach in 51d213d1449b61665628ce9b4e0209cff3c4f818 is to introduce wrappers that grab locks and do cleanup. Makes sense to me but I think we can just use the existing wrapper, AcceptSubPackage? https://github.com/glozow/bitcoin/tree/2025-10-mempoolaccept-layers

    It gives us these layers of MemPoolAccept functions:

    • 1 narrow public-facing interface for validating any number of transactions, AcceptPackage. It handles subpackaging and dispatches to the other functions.
      • On master, this function is already able to handle a package of size 1.
      • This is the only function that acquires any locks; everything below needs to already have it held.
    • The AcceptSubPackage wrapper, which handles cleaning up SubPackageState. It requires locks to already be held. Everything goes through this wrapper to be submitted.
    • AcceptSingleTransaction and AcceptMultipleTransactions, which each run a set of comprehensive checks for their respective categories. Sometimes they are given “subpackages” of a larger package, so they can be using a dirty m_view. However, they can expect a clean SubPackageState.
    • The internal helpers that apply some subset of validation rules: PreChecks, ReplacementChecks, PackageMempoolChecks, *ScriptChecks, and the submission helpersFinalizeSubpackageandSubmitPackage. They usually work on Workspaces and SubPackageState`s that have been modified by each other.

    It also consolidates the public interface. There are currently 3 ways to use MemPoolAccept: (1) Single tx, test_accept or not (2) Multi-tx test_accept (3) Package, not test_accept, restricted to child-with-parents package

    (3) also accepts packages of size 1, but the api is slightly different and should be made consistent. (2) and (3) are separate right now because they have different requirements and results (we can’t just reuse it with test_accept=true). But after cluster mempool, we can unify them by improving the subchunking in AcceptPackage. TLDR I think they should all route through 1 function.

  157. ajtowns commented at 8:59 pm on November 8, 2025: contributor

    FYI for reviewers: the insertion of the AssertLockHeld() in txmempool.h in that commit is what made the bug easy to reproduce in our tests (using a debug build).

    AIUI, constructors and destructors ignore lock annotations ~ChangeSet() EXCLUSIVE_LOCKS_REQUIRED(m_pool->cs) { AssertLockHeld(m_pool->cs); – so a runtime assertion makes sense. (Short of changing the thread safety model entirely, I guess)


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-09 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me