Merge bitcoin/bitcoin#28984: Cluster size 2 package rbf

94ed4fb Add release note for size 2 package rbf (Greg Sanders)
afd52d8 doc: update package RBF comment (Greg Sanders)
6e3c439 mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b6 Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf [test] package rbf (glozow)
dc21f61 [policy] package rbf (Suhas Daftuar)
5da3967 PackageV3Checks: Relax assumptions (Greg Sanders)

Pull request description:

  Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.

  Proposed validation steps:
  1) If the transaction package is of size 1, legacy rbf rules apply.
  2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
  3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
  4) The package is a single chunk
  5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
  6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
  7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)

  Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.

ACKs for top commit:
  achow101:
    ACK 94ed4fb
  glozow:
    reACK 94ed4fb via range-diff
  ismaelsadeeq:
    re-ACK 94ed4fb
  theStack:
    Code-review ACK 94ed4fb
  murchandamus:
    utACK 94ed4fb

Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992

Author: achow101

doc/policy/packages.md

@@ -36,10 +36,29 @@ The following rules are enforced for all packages:
 * Packages cannot have conflicting transactions, i.e. no two transactions in a package can spend
    the same inputs. Packages cannot have duplicate transactions. (#20833)
 
-* No transaction in a package can conflict with a mempool transaction. Replace By Fee is
-  currently disabled for packages. (#20833)
+* Only limited package replacements are currently considered. (#28984)
 
-   - Package RBF may be enabled in the future.
+   - All direct conflicts must signal replacement (or have `-mempoolfullrbf=1` set).
+
+   - Packages are 1-parent-1-child, with no in-mempool ancestors of the package.
+
+   - All conflicting clusters(connected components of mempool transactions) must be clusters of up to size 2.
+
+   - No more than MAX_REPLACEMENT_CANDIDATES transactions can be replaced, analogous to
+     regular [replacement rule](./mempool-replacements.md) 5).
+
+   - Replacements must pay more total total fees at the incremental relay fee (analogous to
+     regular [replacement rules](./mempool-replacements.md) 3 and 4).
+
+   - Parent feerate must be lower than package feerate.
+
+   - Must improve [feerate diagram](https://delvingbitcoin.org/t/mempool-incentive-compatibility/553). (#29242)
+
+   - *Rationale*: Basic support for package RBF can be used by wallets
+     by making chains of no longer than two, then directly conflicting
+     those chains when needed. Combined with V3 transactions this can
+     result in more robust fee bumping. More general package RBF may be
+     enabled in the future.
 
 * When packages are evaluated against ancestor/descendant limits, the union of all transactions'
   descendants and ancestors is considered. (#21800)

doc/release-28984.md

@@ -0,0 +1,6 @@
+P2P and network changes
+-----------------------
+
+- Limited package RBF is now enabled, where the proposed conflicting package would result in
+  a connected component, aka cluster, of size 2 in the mempool. All clusters being conflicted
+  against must be of size 2 or lower.

src/policy/v3_policy.cpp

@@ -91,7 +91,6 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
             const auto parent_info = [&] {
                 if (mempool_ancestors.size() > 0) {
                     auto& mempool_parent = *mempool_ancestors.begin();
-                    Assume(mempool_parent->GetCountWithDescendants() == 1);
                     return ParentInfo{mempool_parent->GetTx().GetHash(),
                                       mempool_parent->GetTx().GetWitnessHash(),
                                       mempool_parent->GetTx().version,
@@ -135,10 +134,7 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
                 }
             }
 
-            // It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks
-            // catches mempool siblings and sibling eviction is not extended to packages. Also, if the package consists of connected transactions,
-            // any tx having a mempool ancestor would mean the package exceeds ancestor limits.
-            if (!Assume(!parent_info.m_has_mempool_descendant)) {
+            if (parent_info.m_has_mempool_descendant) {
                 return strprintf("tx %s (wtxid=%s) would exceed descendant count limit",
                                 parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString());
             }

src/test/fuzz/package_eval.cpp

@@ -314,7 +314,7 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
             // just use result_package.m_state here. This makes the expect_valid check meaningless, but
             // we can still verify that the contents of m_tx_results are consistent with m_state.
             const bool expect_valid{result_package.m_state.IsValid()};
-            Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, nullptr));
+            Assert(!CheckPackageMempoolAcceptResult(txs, result_package, expect_valid, &tx_pool));
         } else {
             // This is empty if it fails early checks, or "full" if transactions are looked at deeper
             Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty());

src/test/txpackage_tests.cpp

@@ -6,6 +6,7 @@
 #include <key_io.h>
 #include <policy/packages.h>
 #include <policy/policy.h>
+#include <policy/rbf.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
 #include <serialize.h>
@@ -938,4 +939,147 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
     }
 }
+
+BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)
+{
+    mineBlocks(5);
+    LOCK(::cs_main);
+    size_t expected_pool_size = m_node.mempool->size();
+    CKey child_key{GenerateRandomKey()};
+    CScript parent_spk = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey()));
+    CKey grandchild_key{GenerateRandomKey()};
+    CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey()));
+
+    const CAmount coinbase_value{50 * COIN};
+    // Test that de-duplication works. This is not actually package rbf.
+    {
+        // 1 parent paying 200sat, 1 child paying 300sat
+        Package package1;
+        // 1 parent paying 200sat, 1 child paying 500sat
+        Package package2;
+        // Package1 and package2 have the same parent. The children conflict.
+        auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0,
+                                                        /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
+                                                        /*output_destination=*/parent_spk,
+                                                        /*output_amount=*/coinbase_value - low_fee_amt, /*submit=*/false);
+        CTransactionRef tx_parent = MakeTransactionRef(mtx_parent);
+        package1.push_back(tx_parent);
+        package2.push_back(tx_parent);
+
+        CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - low_fee_amt - 300, false));
+        package1.push_back(tx_child_1);
+        CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(tx_parent, 0, 101, child_key, child_spk, coinbase_value - low_fee_amt - 500, false));
+        package2.push_back(tx_child_2);
+
+        LOCK(m_node.mempool->cs);
+        const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, /*test_accept=*/false, std::nullopt);
+        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_1.value());
+        }
+
+        // Check precise ResultTypes and mempool size. We know it_parent_1 and it_child_1 exist from above call
+        auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
+        auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
+        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        expected_pool_size += 2;
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+        const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, /*test_accept=*/false, std::nullopt);
+        if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_2.value());
+        }
+
+        // Check precise ResultTypes and mempool size. We know it_parent_2 and it_child_2 exist from above call
+        auto it_parent_2 = submit2.m_tx_results.find(tx_parent->GetWitnessHash());
+        auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
+        BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+        BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+        // child1 has been replaced
+        BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash())));
+    }
+
+    // Test package rbf.
+    {
+        CTransactionRef tx_parent_1 = MakeTransactionRef(CreateValidMempoolTransaction(
+            m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
+            coinbaseKey, parent_spk, coinbase_value - 200, /*submit=*/false));
+        CTransactionRef tx_child_1 = MakeTransactionRef(CreateValidMempoolTransaction(
+            tx_parent_1, /*input_vout=*/0, /*input_height=*/101,
+            child_key, child_spk, coinbase_value - 400, /*submit=*/false));
+
+        CTransactionRef tx_parent_2 = MakeTransactionRef(CreateValidMempoolTransaction(
+            m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
+            coinbaseKey, parent_spk, coinbase_value - 800, /*submit=*/false));
+        CTransactionRef tx_child_2 = MakeTransactionRef(CreateValidMempoolTransaction(
+            tx_parent_2, /*input_vout=*/0, /*input_height=*/101,
+            child_key, child_spk, coinbase_value - 800 - 200, /*submit=*/false));
+
+        CTransactionRef tx_parent_3 = MakeTransactionRef(CreateValidMempoolTransaction(
+            m_coinbase_txns[1], /*input_vout=*/0, /*input_height=*/0,
+            coinbaseKey, parent_spk, coinbase_value - 199, /*submit=*/false));
+        CTransactionRef tx_child_3 = MakeTransactionRef(CreateValidMempoolTransaction(
+            tx_parent_3, /*input_vout=*/0, /*input_height=*/101,
+            child_key, child_spk, coinbase_value - 199 - 1300, /*submit=*/false));
+
+        // In all packages, the parents conflict with each other
+        BOOST_CHECK(tx_parent_1->GetHash() != tx_parent_2->GetHash() && tx_parent_2->GetHash() != tx_parent_3->GetHash());
+
+        // 1 parent paying 200sat, 1 child paying 200sat.
+        Package package1{tx_parent_1, tx_child_1};
+        // 1 parent paying 800sat, 1 child paying 200sat.
+        Package package2{tx_parent_2, tx_child_2};
+        // 1 parent paying 199sat, 1 child paying 1300sat.
+        Package package3{tx_parent_3, tx_child_3};
+
+        const auto submit1 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package1, false, std::nullopt);
+        if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_1.value());
+        }
+        auto it_parent_1 = submit1.m_tx_results.find(tx_parent_1->GetWitnessHash());
+        auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnessHash());
+        BOOST_CHECK_EQUAL(it_parent_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(it_child_1->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        expected_pool_size += 2;
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+        // This replacement is actually not package rbf; the parent carries enough fees
+        // to replace the entire package on its own.
+        const auto submit2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package2, false, std::nullopt);
+        if (auto err_2{CheckPackageMempoolAcceptResult(package2, submit2, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_2.value());
+        }
+        auto it_parent_2 = submit2.m_tx_results.find(tx_parent_2->GetWitnessHash());
+        auto it_child_2 = submit2.m_tx_results.find(tx_child_2->GetWitnessHash());
+        BOOST_CHECK_EQUAL(it_parent_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(it_child_2->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+        // Package RBF, in which the replacement transaction's child sponsors the fees to meet RBF feerate rules
+        const auto submit3 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package3, false, std::nullopt);
+        if (auto err_3{CheckPackageMempoolAcceptResult(package3, submit3, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_3.value());
+        }
+        auto it_parent_3 = submit3.m_tx_results.find(tx_parent_3->GetWitnessHash());
+        auto it_child_3 = submit3.m_tx_results.find(tx_child_3->GetWitnessHash());
+        BOOST_CHECK_EQUAL(it_parent_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+        BOOST_CHECK_EQUAL(it_child_3->second.m_result_type, MempoolAcceptResult::ResultType::VALID);
+
+        // package3 was considered as a package to replace both package2 transactions
+        BOOST_CHECK(it_parent_3->second.m_replaced_transactions.size() == 2);
+        BOOST_CHECK(it_child_3->second.m_replaced_transactions.empty());
+
+        std::vector<Wtxid> expected_package3_wtxids({tx_parent_3->GetWitnessHash(), tx_child_3->GetWitnessHash()});
+        const auto package3_total_vsize{GetVirtualTransactionSize(*tx_parent_3) + GetVirtualTransactionSize(*tx_child_3)};
+        BOOST_CHECK(it_parent_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
+        BOOST_CHECK(it_child_3->second.m_wtxids_fee_calculations.value() == expected_package3_wtxids);
+        BOOST_CHECK_EQUAL(it_parent_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300);
+        BOOST_CHECK_EQUAL(it_child_3->second.m_effective_feerate.value().GetFee(package3_total_vsize), 199 + 1300);
+
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+    }
+
+}
 BOOST_AUTO_TEST_SUITE_END()

src/test/util/txmempool.cpp

@@ -7,6 +7,7 @@
 #include <chainparams.h>
 #include <node/context.h>
 #include <node/mempool_args.h>
+#include <policy/rbf.h>
 #include <policy/v3_policy.h>
 #include <txmempool.h>
 #include <util/check.h>
@@ -68,6 +69,28 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
             return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString());
         }
 
+        // Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)
+        if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {
+            return strprintf("tx %s result replaced too many transactions",
+                                wtxid.ToString());
+        }
+
+        // Replacements can't happen for subpackages larger than 2
+        if (!atmp_result.m_replaced_transactions.empty() &&
+            atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() > 2) {
+             return strprintf("tx %s was part of a too-large package RBF subpackage",
+                                wtxid.ToString());
+        }
+
+        if (!atmp_result.m_replaced_transactions.empty() && mempool) {
+            LOCK(mempool->cs);
+            // If replacements occurred and it used 2 transactions, this is a package RBF and should result in a cluster of size 2
+            if (atmp_result.m_wtxids_fee_calculations.has_value() && atmp_result.m_wtxids_fee_calculations.value().size() == 2) {
+                const auto cluster = mempool->GatherClusters({tx->GetHash()});
+                if (cluster.size() != 2) return strprintf("tx %s has too many ancestors or descendants for a package rbf", wtxid.ToString());
+            }
+        }
+
         // m_vsize and m_base_fees should exist iff the result was VALID or MEMPOOL_ENTRY
         const bool mempool_entry{atmp_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY};
         if (atmp_result.m_base_fees.has_value() != (valid || mempool_entry)) {
@@ -108,6 +131,11 @@ std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,
                     return strprintf("wtxid %s should not be in mempool", wtxid.ToString());
                 }
             }
+            for (const auto& tx_ref : atmp_result.m_replaced_transactions) {
+                if (mempool->exists(GenTxid::Txid(tx_ref->GetHash()))) {
+                    return strprintf("tx %s should not be in mempool as it was replaced", tx_ref->GetWitnessHash().ToString());
+                }
+            }
         }
     }
     return std::nullopt;