[test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee

glozow · glozow · commit c4554fe894d7 · 2023-04-17T09:52:25.000+01:00
diff --git a/src/test/txpackage_tests.cpp b/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,9 +664,9 @@ 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,
@@ -675,8 +682,8 @@ BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
     // 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 +696,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 +716,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 +748,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 +760,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);
diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py
@@ -7,15 +7,21 @@
 from decimal import Decimal
 
 from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.p2p import P2PTxInvStore
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_fee_amount,
     assert_greater_than,
     assert_raises_rpc_error,
     create_lots_of_big_transactions,
     gen_return_txouts,
 )
-from test_framework.wallet import MiniWallet
+from test_framework.wallet import (
+    COIN,
+    DEFAULT_FEE,
+    MiniWallet,
+)
 
 
 class MempoolLimitTest(BitcoinTestFramework):
@@ -44,7 +50,8 @@ def run_test(self):
         # 1 to create a tx initially that will be evicted from the mempool later
         # 3 batches of multiple transactions with a fee rate much higher than the previous UTXO
         # And 1 more to verify that this tx does not get added to the mempool with a fee rate less than the mempoolminfee
-        self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size) + 1)
+        # And 2 more for the package cpfp test
+        self.generate(miniwallet, 1 + (num_of_batches * tx_batch_size) + 1 + 2)
 
         # Mine 99 blocks so that the UTXOs are allowed to be spent
         self.generate(node, COINBASE_MATURITY - 1)
@@ -76,6 +83,44 @@ def run_test(self):
         self.log.info('Create a mempool tx that will not pass mempoolminfee')
         assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee)
 
+        self.log.info("Check that submitpackage allows cpfp of a parent below mempool min feerate")
+        node = self.nodes[0]
+        peer = node.add_p2p_connection(P2PTxInvStore())
+
+        # Package with 2 parents and 1 child. One parent has a high feerate due to modified fees,
+        # another is below the mempool minimum feerate but bumped by the child.
+        tx_poor = miniwallet.create_self_transfer(fee_rate=relayfee)
+        tx_rich = miniwallet.create_self_transfer(fee=0, fee_rate=0)
+        node.prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN))
+        package_txns = [tx_rich, tx_poor]
+        coins = [tx["new_utxo"] for tx in package_txns]
+        tx_child = miniwallet.create_self_transfer_multi(utxos_to_spend=coins, fee_per_output=10000) #DEFAULT_FEE
+        package_txns.append(tx_child)
+
+        submitpackage_result = node.submitpackage([tx["hex"] for tx in package_txns])
+
+        rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]]
+        poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]]
+        child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()]
+        assert_fee_amount(poor_parent_result["fees"]["base"], tx_poor["tx"].get_vsize(), relayfee)
+        assert_equal(rich_parent_result["fees"]["base"], 0)
+        assert_equal(child_result["fees"]["base"], DEFAULT_FEE)
+        # The "rich" parent does not require CPFP so its effective feerate is just its individual feerate.
+        assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"])
+        assert_equal(rich_parent_result["fees"]["effective-includes"], [tx_rich["wtxid"]])
+        # The "poor" parent and child's effective feerates are the same, composed of their total
+        # fees divided by their combined vsize.
+        package_fees = poor_parent_result["fees"]["base"] + child_result["fees"]["base"]
+        package_vsize = tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize()
+        assert_fee_amount(package_fees, package_vsize, poor_parent_result["fees"]["effective-feerate"])
+        assert_fee_amount(package_fees, package_vsize, child_result["fees"]["effective-feerate"])
+        assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"])
+        assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"])
+
+        # The node will broadcast each transaction, still abiding by its peer's fee filter
+        peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns])
+        self.generate(node, 1)
+
         self.log.info('Test passing a value below the minimum (5 MB) to -maxmempool throws an error')
         self.stop_node(0)
         self.nodes[0].assert_start_raises_init_error(["-maxmempool=4"], "Error: -maxmempool must be at least 5 MB")
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
@@ -20,7 +20,6 @@
     assert_raises_rpc_error,
 )
 from test_framework.wallet import (
-    COIN,
     DEFAULT_FEE,
     MiniWallet,
 )
@@ -325,42 +324,6 @@ def test_submit_child_with_parents(self, num_parents, partial_submit):
         peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns])
         self.generate(node, 1)
 
-    def test_submit_cpfp(self):
-        node = self.nodes[0]
-        peer = node.add_p2p_connection(P2PTxInvStore())
-
-        # Package with 2 parents and 1 child. One parent pays for itself using modified fees, and
-        # another has 0 fees but is bumped by child.
-        tx_poor = self.wallet.create_self_transfer(fee=0, fee_rate=0)
-        tx_rich = self.wallet.create_self_transfer(fee=0, fee_rate=0)
-        node.prioritisetransaction(tx_rich["txid"], 0, int(DEFAULT_FEE * COIN))
-        package_txns = [tx_rich, tx_poor]
-        coins = [tx["new_utxo"] for tx in package_txns]
-        tx_child = self.wallet.create_self_transfer_multi(utxos_to_spend=coins, fee_per_output=10000) #DEFAULT_FEE
-        package_txns.append(tx_child)
-
-        submitpackage_result = node.submitpackage([tx["hex"] for tx in package_txns])
-
-        rich_parent_result = submitpackage_result["tx-results"][tx_rich["wtxid"]]
-        poor_parent_result = submitpackage_result["tx-results"][tx_poor["wtxid"]]
-        child_result = submitpackage_result["tx-results"][tx_child["tx"].getwtxid()]
-        assert_equal(rich_parent_result["fees"]["base"], 0)
-        assert_equal(poor_parent_result["fees"]["base"], 0)
-        assert_equal(child_result["fees"]["base"], DEFAULT_FEE)
-        # The "rich" parent does not require CPFP so its effective feerate.
-        assert_fee_amount(DEFAULT_FEE, tx_rich["tx"].get_vsize(), rich_parent_result["fees"]["effective-feerate"])
-        assert_equal(rich_parent_result["fees"]["effective-includes"], [tx_rich["wtxid"]])
-        # The "poor" parent and child's effective feerates are the same, composed of the child's fee
-        # divided by their combined vsize.
-        assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), poor_parent_result["fees"]["effective-feerate"])
-        assert_fee_amount(DEFAULT_FEE, tx_poor["tx"].get_vsize() + tx_child["tx"].get_vsize(), child_result["fees"]["effective-feerate"])
-        assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], poor_parent_result["fees"]["effective-includes"])
-        assert_equal([tx_poor["wtxid"], tx_child["tx"].getwtxid()], child_result["fees"]["effective-includes"])
-
-        # The node will broadcast each transaction, still abiding by its peer's fee filter
-        peer.wait_for_broadcast([tx["tx"].getwtxid() for tx in package_txns])
-        self.generate(node, 1)
-
     def test_submitpackage(self):
         node = self.nodes[0]
 
@@ -369,9 +332,6 @@ def test_submitpackage(self):
             self.test_submit_child_with_parents(num_parents, False)
             self.test_submit_child_with_parents(num_parents, True)
 
-        self.log.info("Submitpackage valid packages with CPFP")
-        self.test_submit_cpfp()
-
         self.log.info("Submitpackage only allows packages of 1 child with its parents")
         # Chain of 3 transactions has too many generations
         chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)]
