Merge bitcoin/bitcoin#26933: mempool: disallow txns under min relay fee, even in packages

bf77fc9 [test] mempool full in package accept (glozow)
b51ebcc [validation] set PackageValidationState when mempool full (glozow)
563a2ee [policy] disallow transactions under min relay fee, even in packages (glozow)
c4554fe [test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee (glozow)
ac463e8 [test util] mock mempool minimum feerate (glozow)

Pull request description:

  Part of package relay, see #27463.

  Note that this still allows packages to bump transactions that are below the dynamic mempool minimum feerate, which means this still solves the "mempool is congested and my presigned 1sat/vB tx is screwed" problem for all transactions.

  On master, the package policy (only accessible through regtest-only RPC submitpackage) allows 0-fee (or otherwise below min relay feerate) transactions if they are bumped by a child. However, with default package limits, we don't yet have a DoS-resistant way of ensuring these transactions remain bumped throughout their time in the mempool. Primarily, the fee-bumping child may later be replaced by another transaction that doesn't bump the parent(s). The parent(s) could potentially stay bumped by other transactions, but not enough to ever be selected by the `BlockAssembler` (due to `blockmintxfee`).

  For example, (tested [here](https://github.com/glozow/bitcoin/commits/26933-motivation)):
  - The mempool accepts 24 below-minrelayfeerate transactions ("0-fee parents"), all bumped by a single high-fee transaction ("the fee-bumping child"). The fee-bumping child also spends a confirmed UTXO.
  - Two additional children are added to each 0-fee parent. These children each pay a feerate slightly above the minimum relay feerate (e.g. 1.9sat/vB) such that, for each 0-fee parent, the total fees of its two children divided by the total size of the children and parent is above the minimum relay feerate.
  - If a block template is built now, all transactions would be selected.
  - A transaction replaces the the fee-bumping child, spending only the confirmed UTXO and not any of the outputs from the 0-fee parents.
   - The 0-fee parents now each have 2 children. Their descendant feerates are above minrelayfeerate, which means that they remain in the mempool, even if the mempool evicts all below-minrelayfeerate packages.
   - If a block template is built now, none of the 0-fee parents or their children would be selected.
   - Even more low-feerate descendants can be added to these below-minrelayfeerate packages and they will not be evicted until they expire or the mempool reaches capacity.

  Unless we have a DoS-resistant way of ensuring package CPFP-bumped transactions are always bumped, allowing package CPFP to bump below-minrelayfeerate transactions can result in these problematic situations. See #27018 which proposes a partial solution with some limitations, and contains discussion about potential improvements to eviction strategy. While no adequate solution exists, for now, avoid these situations by requiring all transactions to meet min relay feerate.

ACKs for top commit:
  ajtowns:
    reACK bf77fc9
  instagibbs:
    re-ACK bitcoin/bitcoin@bf77fc9

Tree-SHA512: 28940f41493a9e280b010284316fb8caf1ed7b2090ba9a4ef8a3b2eafc5933601074b142f4f7d4e3c6c4cce99d3146f5c8e1393d9406c6f2070dd41c817985c9

Author: glozow

doc/policy/packages.md

@@ -80,24 +80,37 @@ test accepts):
 If any transactions in the package are already in the mempool, they are not submitted again
 ("deduplicated") and are thus excluded from this calculation.
 
-To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate
-(`-minrelaytxfee`) and the dynamic mempool minimum feerate, the total package feerate is used instead
-of the individual feerate. The individual transactions are allowed to be below the feerate
-requirements if the package meets the feerate requirements. For example, the parent(s) in the
-package can pay no fees but be paid for by the child.
-
-*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not
-meeting minimum fees on its own. This would allow contracting applications to adjust their fees at
-broadcast time instead of overshooting or risking becoming stuck or pinned.
-
-*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as
-we do not want a transaction's fees to be double-counted.
+To meet the dynamic mempool minimum feerate, i.e., the feerate determined by the transactions
+evicted when the mempool reaches capacity (not the static minimum relay feerate), the total package
+feerate instead of individual feerate can be used. For example, if the mempool minimum feerate is
+5sat/vB and a 1sat/vB parent transaction has a high-feerate child, it may be accepted if
+submitted as a package.
+
+*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a presigned
+transaction (i.e. in which a replacement transaction with a higher fee cannot be signed) being
+rejected from the mempool when transaction volume is high and the mempool minimum feerate rises.
+
+Note: Package feerate cannot be used to meet the minimum relay feerate (`-minrelaytxfee`)
+requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is
+set to 5satvB, a 1sat/vB parent transaction with a high-feerate child will not be accepted, even if
+submitted as a package.
+
+*Rationale*: Avoid situations in which the mempool contains non-bumped transactions below min relay
+feerate (which we consider to have pay 0 fees and thus receiving free relay). While package
+submission would ensure these transactions are bumped at the time of entry, it is not guaranteed
+that the transaction will always be bumped. For example, a later transaction could replace the
+fee-bumping child without still bumping the parent. These no-longer-bumped transactions should be
+removed during a replacement, but we do not have a DoS-resistant way of removing them or enforcing a
+limit on their quantity. Instead, prevent their entry into the mempool.
 
 Implementation Note: Transactions within a package are always validated individually first, and
 package validation is used for the transactions that failed. Since package feerate is only
 calculated using transactions that are not in the mempool, this implementation detail affects the
 outcome of package validation.
 
+*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as
+we do not want a transaction's fees to be double-counted.
+
 *Rationale*: Packages are intended for incentive-compatible fee-bumping: transaction B is a
 "legitimate" fee-bump for transaction A only if B is a descendant of A and has a *higher* feerate
 than A. We want to prevent "parents pay for children" behavior; fees of parents should not help

src/test/txpackage_tests.cpp

@@ -16,6 +16,9 @@
 #include <boost/test/unit_test.hpp>
 
 BOOST_AUTO_TEST_SUITE(txpackage_tests)
+// A fee amount that is above 1sat/vB but below 5sat/vB for most transactions created within these
+// unit tests.
+static const CAmount low_fee_amt{200};
 
 // Create placeholder transactions that have no meaning.
 inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outputs)
@@ -373,6 +376,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
 {
     // Mine blocks to mature coinbases.
     mineBlocks(5);
+    MockMempoolMinFee(CFeeRate(5000));
     LOCK(cs_main);
 
     // Transactions with a same-txid-different-witness transaction in the mempool should be ignored,
@@ -560,13 +564,15 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
     BOOST_CHECK(parent2_v2_result.m_result_type == MempoolAcceptResult::ResultType::VALID);
     package_mixed.push_back(ptx_parent2_v1);
 
-    // parent3 will be a new transaction. Put 0 fees on it to make it invalid on its own.
+    // parent3 will be a new transaction. Put a low feerate to make it invalid on its own.
     auto mtx_parent3 = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[3], /*input_vout=*/0,
                                                      /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
                                                      /*output_destination=*/acs_spk,
-                                                     /*output_amount=*/CAmount(50 * COIN), /*submit=*/false);
+                                                     /*output_amount=*/CAmount(50 * COIN - low_fee_amt), /*submit=*/false);
     CTransactionRef ptx_parent3 = MakeTransactionRef(mtx_parent3);
     package_mixed.push_back(ptx_parent3);
+    BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*ptx_parent3)) > low_fee_amt);
+    BOOST_CHECK(m_node.mempool->m_min_relay_feerate.GetFee(GetVirtualTransactionSize(*ptx_parent3)) <= low_fee_amt);
 
     // child spends parent1, parent2, and parent3
     CKey mixed_grandchild_key;
@@ -627,6 +633,7 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
 BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
 {
     mineBlocks(5);
+    MockMempoolMinFee(CFeeRate(5000));
     LOCK(::cs_main);
     size_t expected_pool_size = m_node.mempool->size();
     CKey child_key;
@@ -636,9 +643,9 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     grandchild_key.MakeNewKey(true);
     CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey()));
 
-    // zero-fee parent and high-fee child package
+    // low-fee parent and high-fee child package
     const CAmount coinbase_value{50 * COIN};
-    const CAmount parent_value{coinbase_value - 0};
+    const CAmount parent_value{coinbase_value - low_fee_amt};
     const CAmount child_value{parent_value - COIN};
 
     Package package_cpfp;
@@ -657,26 +664,29 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     package_cpfp.push_back(tx_child);
 
     // Package feerate is calculated using modified fees, and prioritisetransaction accepts negative
-    // fee deltas. This should be taken into account. De-prioritise the parent transaction by -1BTC,
-    // bringing the package feerate to 0.
-    m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), -1 * COIN);
+    // fee deltas. This should be taken into account. De-prioritise the parent transaction
+    // to bring the package feerate to 0.
+    m_node.mempool->PrioritiseTransaction(tx_parent->GetHash(), child_value - coinbase_value);
     {
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
         const auto submit_cpfp_deprio = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
                                                    package_cpfp, /*test_accept=*/ false);
-        BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
-        BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_state.IsInvalid(),
-                            "Package validation unexpectedly succeeded: " << submit_cpfp_deprio.m_state.GetRejectReason());
-        BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty());
+        BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_state.GetResult(), PackageValidationResult::PCKG_TX);
+        BOOST_CHECK(submit_cpfp_deprio.m_state.IsInvalid());
+        BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetResult(),
+                          TxValidationResult::TX_MEMPOOL_POLICY);
+        BOOST_CHECK_EQUAL(submit_cpfp_deprio.m_tx_results.find(tx_child->GetWitnessHash())->second.m_state.GetResult(),
+                          TxValidationResult::TX_MISSING_INPUTS);
+        BOOST_CHECK(submit_cpfp_deprio.m_tx_results.find(tx_parent->GetWitnessHash())->second.m_state.GetRejectReason() == "min relay fee not met");
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
         const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child));
     }
 
     // Clear the prioritisation of the parent transaction.
     WITH_LOCK(m_node.mempool->cs, m_node.mempool->ClearPrioritisation(tx_parent->GetHash()));
 
-    // Package CPFP: Even though the parent pays 0 absolute fees, the child pays 1 BTC which is
-    // enough for the package feerate to meet the threshold.
+    // Package CPFP: Even though the parent's feerate is below the mempool minimum feerate, the
+    // child pays enough for the package feerate to meet the threshold.
     {
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
         const auto submit_cpfp = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
@@ -689,7 +699,7 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
         auto it_child = submit_cpfp.m_tx_results.find(tx_child->GetWitnessHash());
         BOOST_CHECK(it_parent != submit_cpfp.m_tx_results.end());
         BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
-        BOOST_CHECK(it_parent->second.m_base_fees.value() == 0);
+        BOOST_CHECK(it_parent->second.m_base_fees.value() == coinbase_value - parent_value);
         BOOST_CHECK(it_child != submit_cpfp.m_tx_results.end());
         BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
         BOOST_CHECK(it_child->second.m_base_fees.value() == COIN);
@@ -709,22 +719,28 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     }
 
     // Just because we allow low-fee parents doesn't mean we allow low-feerate packages.
-    // This package just pays 200 satoshis total. This would be enough to pay for the child alone,
-    // but isn't enough for the entire package to meet the 1sat/vbyte minimum.
+    // The mempool minimum feerate is 5sat/vB, but this package just pays 800 satoshis total.
+    // The child fees would be able to pay for itself, but isn't enough for the entire package.
     Package package_still_too_low;
+    const CAmount parent_fee{200};
+    const CAmount child_fee{600};
     auto mtx_parent_cheap = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], /*input_vout=*/0,
                                                           /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
                                                           /*output_destination=*/parent_spk,
-                                                          /*output_amount=*/coinbase_value, /*submit=*/false);
+                                                          /*output_amount=*/coinbase_value - parent_fee, /*submit=*/false);
     CTransactionRef tx_parent_cheap = MakeTransactionRef(mtx_parent_cheap);
     package_still_too_low.push_back(tx_parent_cheap);
+    BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent_cheap)) > parent_fee);
+    BOOST_CHECK(m_node.mempool->m_min_relay_feerate.GetFee(GetVirtualTransactionSize(*tx_parent_cheap)) <= parent_fee);
 
     auto mtx_child_cheap = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent_cheap, /*input_vout=*/0,
                                                          /*input_height=*/101, /*input_signing_key=*/child_key,
                                                          /*output_destination=*/child_spk,
-                                                         /*output_amount=*/coinbase_value - 200, /*submit=*/false);
+                                                         /*output_amount=*/coinbase_value - parent_fee - child_fee, /*submit=*/false);
     CTransactionRef tx_child_cheap = MakeTransactionRef(mtx_child_cheap);
     package_still_too_low.push_back(tx_child_cheap);
+    BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_child_cheap)) <= child_fee);
+    BOOST_CHECK(m_node.mempool->GetMinFee().GetFee(GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap)) > parent_fee + child_fee);
 
     // Cheap package should fail with package-fee-too-low.
     {
@@ -735,11 +751,6 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
         BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
         BOOST_CHECK_EQUAL(submit_package_too_low.m_state.GetRejectReason(), "package-fee-too-low");
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-        const CFeeRate child_feerate(200, GetVirtualTransactionSize(*tx_child_cheap));
-        BOOST_CHECK(child_feerate.GetFeePerK() > 1000);
-        const CFeeRate expected_feerate(200,
-            GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap));
-        BOOST_CHECK(expected_feerate.GetFeePerK() < 1000);
     }
 
     // Package feerate includes the modified fees of the transactions.
@@ -752,18 +763,18 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
         expected_pool_size += 2;
         BOOST_CHECK_MESSAGE(submit_prioritised_package.m_state.IsValid(),
                 "Package validation unexpectedly failed" << submit_prioritised_package.m_state.GetRejectReason());
-        const CFeeRate expected_feerate(1 * COIN + 200,
+        const CFeeRate expected_feerate(1 * COIN + parent_fee + child_fee,
             GetVirtualTransactionSize(*tx_parent_cheap) + GetVirtualTransactionSize(*tx_child_cheap));
         BOOST_CHECK_EQUAL(submit_prioritised_package.m_tx_results.size(), package_still_too_low.size());
         auto it_parent = submit_prioritised_package.m_tx_results.find(tx_parent_cheap->GetWitnessHash());
         auto it_child = submit_prioritised_package.m_tx_results.find(tx_child_cheap->GetWitnessHash());
         BOOST_CHECK(it_parent != submit_prioritised_package.m_tx_results.end());
         BOOST_CHECK(it_parent->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
-        BOOST_CHECK(it_parent->second.m_base_fees.value() == 0);
+        BOOST_CHECK(it_parent->second.m_base_fees.value() == parent_fee);
         BOOST_CHECK(it_parent->second.m_effective_feerate.value() == expected_feerate);
         BOOST_CHECK(it_child != submit_prioritised_package.m_tx_results.end());
         BOOST_CHECK(it_child->second.m_result_type == MempoolAcceptResult::ResultType::VALID);
-        BOOST_CHECK(it_child->second.m_base_fees.value() == 200);
+        BOOST_CHECK(it_child->second.m_base_fees.value() == child_fee);
         BOOST_CHECK(it_child->second.m_effective_feerate.value() == expected_feerate);
         std::vector<uint256> expected_wtxids({tx_parent_cheap->GetWitnessHash(), tx_child_cheap->GetWitnessHash()});
         BOOST_CHECK(it_parent->second.m_wtxids_fee_calculations.value() == expected_wtxids);
@@ -800,8 +811,8 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
         BOOST_CHECK_MESSAGE(submit_rich_parent.m_state.IsInvalid(), "Package validation unexpectedly succeeded");
 
         // The child would have been validated on its own and failed, then submitted as a "package" of 1.
-        BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
-        BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "package-fee-too-low");
+        BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetResult(), PackageValidationResult::PCKG_TX);
+        BOOST_CHECK_EQUAL(submit_rich_parent.m_state.GetRejectReason(), "transaction failed");
 
         auto it_parent = submit_rich_parent.m_tx_results.find(tx_parent_rich->GetWitnessHash());
         BOOST_CHECK(it_parent != submit_rich_parent.m_tx_results.end());
@@ -810,6 +821,11 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
         BOOST_CHECK_MESSAGE(it_parent->second.m_base_fees.value() == high_parent_fee,
                 strprintf("rich parent: expected fee %s, got %s", high_parent_fee, it_parent->second.m_base_fees.value()));
         BOOST_CHECK(it_parent->second.m_effective_feerate == CFeeRate(high_parent_fee, GetVirtualTransactionSize(*tx_parent_rich)));
+        auto it_child = submit_rich_parent.m_tx_results.find(tx_child_poor->GetWitnessHash());
+        BOOST_CHECK(it_child != submit_rich_parent.m_tx_results.end());
+        BOOST_CHECK_EQUAL(it_child->second.m_result_type, MempoolAcceptResult::ResultType::INVALID);
+        BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MEMPOOL_POLICY);
+        BOOST_CHECK(it_child->second.m_state.GetRejectReason() == "min relay fee not met");
 
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
         BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(tx_parent_rich->GetHash())));

src/test/util/setup_common.cpp

@@ -433,6 +433,33 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
     return mempool_transactions;
 }
 
+void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate)
+{
+    LOCK2(cs_main, m_node.mempool->cs);
+    // Transactions in the mempool will affect the new minimum feerate.
+    assert(m_node.mempool->size() == 0);
+    // The target feerate cannot be too low...
+    // ...otherwise the transaction's feerate will need to be negative.
+    assert(target_feerate > m_node.mempool->m_incremental_relay_feerate);
+    // ...otherwise this is not meaningful. The feerate policy uses the maximum of both feerates.
+    assert(target_feerate > m_node.mempool->m_min_relay_feerate);
+
+    // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to
+    // achieve the exact target feerate.
+    CMutableTransaction mtx = CMutableTransaction();
+    mtx.vin.push_back(CTxIn{COutPoint{g_insecure_rand_ctx.rand256(), 0}});
+    mtx.vout.push_back(CTxOut(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE))));
+    const auto tx{MakeTransactionRef(mtx)};
+    LockPoints lp;
+    // The new mempool min feerate is equal to the removed package's feerate + incremental feerate.
+    const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) -
+        m_node.mempool->m_incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx));
+    m_node.mempool->addUnchecked(CTxMemPoolEntry(tx, /*fee=*/tx_fee,
+                                                 /*time=*/0, /*entry_height=*/1,
+                                                 /*spends_coinbase=*/true, /*sigops_cost=*/1, lp));
+    m_node.mempool->TrimToSize(0);
+    assert(m_node.mempool->GetMinFee() == target_feerate);
+}
 /**
  * @returns a real block (0000000000013b8ab2cd513b0261a14096412195a72a0c4827d229dcc7e0f7af)
  *      with 9 txs.