Merge b14e1a8688 into 66aa6a47bd

7c123c08dd  miner: add package feerate vector to CBlockTemplate (ismaelsadeeq)

Pull request description:

  This PR enables `BlockAssembler` to add all selected packages' fee and virtual size to a vector, and then return the vector as a member of `CBlockTemplate` struct.

  This PR is the first step in the https://github.com/bitcoin/bitcoin/issues/30392 project.

  The packages' vsize and fee are used in #30157 to select a percentile fee rate of the top block in the mempool.

ACKs for top commit:
  rkrux:
    tACK 7c123c08dd
  ryanofsky:
    Code review ACK 7c123c08dd. Changes since last review are rebasing due to a test conflict, giving the new field a better name and description, resolving the test conflict, and renaming a lot of test variables. The actual code change is still one-line change.
  glozow:
    reACK 7c123c08dd

Tree-SHA512: 767b0b3d4273cf1589fd2068d729a66c7414c0f9574b15989fbe293f8c85cd6c641dd783cde55bfabab32cd047d7d8a071d6897b06ed4295c0d071e588de0861

doc/policy/packages.md

@@ -8,12 +8,6 @@ Graph (a directed edge exists between a transaction that spends the output of an
 For every transaction `t` in a **topologically sorted** package, if any of its parents are present
 in the package, they appear somewhere in the list before `t`.
 
-A **child-with-unconfirmed-parents** package is a topologically sorted package that consists of
-exactly one child and all of its unconfirmed parents (no other transactions may be present).
-The last transaction in the package is the child, and its package can be canonically defined based
-on the current state: each of its inputs must be available in the UTXO set as of the current chain
-tip or some preceding transaction in the package.
-
 ## Package Mempool Acceptance Rules
 
 The following rules are enforced for all packages:
@@ -73,7 +67,7 @@ The following rules are enforced for all packages:
 The following rules are only enforced for packages to be submitted to the mempool (not
 enforced for test accepts):
 
-* Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at
+* Packages must be child-with-parents packages. This also means packages must contain at
   least 1 transaction. (#31096)
 
    - *Rationale*: This allows for fee-bumping by CPFP. Allowing multiple parents makes it possible

src/node/miner.cpp

@@ -421,6 +421,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
         }
 
         ++nPackagesSelected;
+        pblocktemplate->m_package_feerates.emplace_back(packageFees, static_cast<int32_t>(packageSize));
 
         // Update transactions that depend on each of these
         nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx);

src/node/miner.h

@@ -10,6 +10,7 @@
 #include <policy/policy.h>
 #include <primitives/block.h>
 #include <txmempool.h>
+#include <util/feefrac.h>
 
 #include <memory>
 #include <optional>
@@ -39,6 +40,9 @@ struct CBlockTemplate
     std::vector<CAmount> vTxFees;
     std::vector<int64_t> vTxSigOpsCost;
     std::vector<unsigned char> vchCoinbaseCommitment;
+    /* A vector of package fee rates, ordered by the sequence in which
+     * packages are selected for inclusion in the block template.*/
+    std::vector<FeeFrac> m_package_feerates;
 };
 
 // Container for tracking updates to ancestor feerate as we include (parent)

src/policy/packages.h

@@ -26,7 +26,7 @@ static_assert(MAX_PACKAGE_WEIGHT >= MAX_STANDARD_TX_WEIGHT);
 
 // If a package is to be evaluated, it must be at least as large as the mempool's ancestor/descendant limits,
 // otherwise transactions that would be individually accepted may be rejected in a package erroneously.
-// Since a submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
+// Since a submitted package must be child-with-parents (all of the transactions are an ancestor
 // of the child), package limits are ultimately bounded by mempool package limits. Ensure that the
 // defaults reflect this constraint.
 static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT);

src/test/miner_tests.cpp

@@ -16,6 +16,7 @@
 #include <txmempool.h>
 #include <uint256.h>
 #include <util/check.h>
+#include <util/feefrac.h>
 #include <util/strencodings.h>
 #include <util/time.h>
 #include <util/translation.h>
@@ -25,6 +26,7 @@
 #include <test/util/setup_common.h>
 
 #include <memory>
+#include <vector>
 
 #include <boost/test/unit_test.hpp>
 
@@ -123,19 +125,22 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     tx.vout[0].nValue = 5000000000LL - 1000;
     // This tx has a low fee: 1000 satoshis
     Txid hashParentTx = tx.GetHash(); // save this txid for later use
-    AddToMempool(tx_mempool, entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
+    const auto parent_tx{entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
+    AddToMempool(tx_mempool, parent_tx);
 
     // This tx has a medium fee: 10000 satoshis
     tx.vin[0].prevout.hash = txFirst[1]->GetHash();
     tx.vout[0].nValue = 5000000000LL - 10000;
     Txid hashMediumFeeTx = tx.GetHash();
-    AddToMempool(tx_mempool, entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
+    const auto medium_fee_tx{entry.Fee(10000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
+    AddToMempool(tx_mempool, medium_fee_tx);
 
     // This tx has a high fee, but depends on the first transaction
     tx.vin[0].prevout.hash = hashParentTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee
     Txid hashHighFeeTx = tx.GetHash();
-    AddToMempool(tx_mempool, entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx));
+    const auto high_fee_tx{entry.Fee(50000).Time(Now<NodeSeconds>()).SpendsCoinbase(false).FromTx(tx)};
+    AddToMempool(tx_mempool, high_fee_tx);
 
     std::unique_ptr<BlockTemplate> block_template = mining->createNewBlock(options);
     BOOST_REQUIRE(block_template);
@@ -145,6 +150,21 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
     BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx);
     BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx);
 
+    // Test the inclusion of package feerates in the block template and ensure they are sequential.
+    const auto block_package_feerates = BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}.CreateNewBlock()->m_package_feerates;
+    BOOST_CHECK(block_package_feerates.size() == 2);
+
+    // parent_tx and high_fee_tx are added to the block as a package.
+    const auto combined_txs_fee = parent_tx.GetFee() + high_fee_tx.GetFee();
+    const auto combined_txs_size = parent_tx.GetTxSize() + high_fee_tx.GetTxSize();
+    FeeFrac package_feefrac{combined_txs_fee, combined_txs_size};
+    // The package should be added first.
+    BOOST_CHECK(block_package_feerates[0] == package_feefrac);
+
+    // The medium_fee_tx should be added next.
+    FeeFrac medium_tx_feefrac{medium_fee_tx.GetFee(), medium_fee_tx.GetTxSize()};
+    BOOST_CHECK(block_package_feerates[1] == medium_tx_feefrac);
+
     // Test that a package below the block min tx fee doesn't get included
     tx.vin[0].prevout.hash = hashHighFeeTx;
     tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 0 fee

src/test/txpackage_tests.cpp

@@ -355,6 +355,9 @@ BOOST_AUTO_TEST_CASE(noncontextual_package_tests)
 
 BOOST_AUTO_TEST_CASE(package_submission_tests)
 {
+    // Mine blocks to mature coinbases.
+    mineBlocks(3);
+    MockMempoolMinFee(CFeeRate(5000));
     LOCK(cs_main);
     unsigned int expected_pool_size = m_node.mempool->size();
     CKey parent_key = GenerateRandomKey();
@@ -441,20 +444,6 @@ BOOST_AUTO_TEST_CASE(package_submission_tests)
         BOOST_CHECK_EQUAL(result_quit_early.m_state.GetResult(), PackageValidationResult::PCKG_TX);
     }
 
-    // Child with missing parent.
-    mtx_child.vin.emplace_back(COutPoint(package_unrelated[0]->GetHash(), 0));
-    Package package_missing_parent;
-    package_missing_parent.push_back(tx_parent);
-    package_missing_parent.push_back(MakeTransactionRef(mtx_child));
-    {
-        const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
-                                                             package_missing_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
-        BOOST_CHECK(result_missing_parent.m_state.IsInvalid());
-        BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
-        BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "package-not-child-with-unconfirmed-parents");
-        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
-    }
-
     // Submit package with parent + child.
     {
         const auto submit_parent_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
@@ -492,6 +481,60 @@ BOOST_AUTO_TEST_CASE(package_submission_tests)
 
         BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
     }
+
+    // In-mempool parent and child with missing parent.
+    {
+        auto tx_parent_1 = MakeTransactionRef(CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], /*input_vout=*/0,
+                                                                            /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
+                                                                            /*output_destination=*/parent_locking_script,
+                                                                            /*output_amount=*/CAmount(50 * COIN - low_fee_amt), /*submit=*/false));
+        auto tx_parent_2 = MakeTransactionRef(CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[2], /*input_vout=*/0,
+                                                                            /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
+                                                                            /*output_destination=*/parent_locking_script,
+                                                                            /*output_amount=*/CAmount(50 * COIN - 800), /*submit=*/false));
+
+        auto tx_child_missing_parent = MakeTransactionRef(CreateValidMempoolTransaction({tx_parent_1, tx_parent_2},
+                                                                                        {{tx_parent_1->GetHash(), 0}, {tx_parent_2->GetHash(), 0}},
+                                                                                        /*input_height=*/0, {parent_key},
+                                                                                        {{49 * COIN, child_locking_script}}, /*submit=*/false));
+
+        Package package_missing_parent{tx_parent_1, tx_child_missing_parent};
+
+        const auto result_missing_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                             package_missing_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
+        if (auto err_missing_parent{CheckPackageMempoolAcceptResult(package_missing_parent, result_missing_parent, /*expect_valid=*/false, m_node.mempool.get())}) {
+            BOOST_ERROR(err_missing_parent.value());
+        } else {
+            auto it_parent = result_missing_parent.m_tx_results.find(tx_parent_1->GetWitnessHash());
+            auto it_child = result_missing_parent.m_tx_results.find(tx_child_missing_parent->GetWitnessHash());
+
+            BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetResult(), PackageValidationResult::PCKG_TX);
+            BOOST_CHECK_EQUAL(result_missing_parent.m_state.GetRejectReason(), "transaction failed");
+
+            BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);
+            BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS);
+            BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent");
+            BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+        }
+
+        // Submit parent2 ahead of time, should become ok.
+        Package package_just_parent2{tx_parent_2};
+        expected_pool_size += 1;
+        const auto result_just_parent2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                           package_just_parent2, /*test_accept=*/false, /*client_maxfeerate=*/{});
+        if (auto err_parent2{CheckPackageMempoolAcceptResult(package_just_parent2, result_just_parent2, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_parent2.value());
+        }
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+
+        const auto result_parent_already_in = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                                package_missing_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
+        expected_pool_size += 2;
+        if (auto err_parent_already_in{CheckPackageMempoolAcceptResult(package_missing_parent, result_parent_already_in, /*expect_valid=*/true, m_node.mempool.get())}) {
+            BOOST_ERROR(err_parent_already_in.value());
+        }
+        BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
+    }
 }
 
 // Tests for packages containing a single transaction

src/validation.cpp

@@ -520,7 +520,7 @@ public:
             };
         }
 
-        /** Parameters for child-with-unconfirmed-parents package validation. */
+        /** Parameters for child-with-parents package validation. */
         static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time,
                                                 std::vector<COutPoint>& coins_to_uncache, const std::optional<CFeeRate>& client_maxfeerate) {
             return ATMPArgs{/* m_chainparams */ chainparams,
@@ -1694,7 +1694,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
 
     // There are two topologies we are able to handle through this function:
     // (1) A single transaction
-    // (2) A child-with-unconfirmed-parents package.
+    // (2) A child-with-parents package.
     // Check that the package is well-formed. If it isn't, we won't try to validate any of the
     // transactions and thus won't return any MempoolAcceptResults, just a package-wide error.
 
@@ -1703,49 +1703,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
         return PackageMempoolAcceptResult(package_state_quit_early, {});
     }
 
-    if (package.size() > 1) {
+    if (package.size() > 1 && !IsChildWithParents(package)) {
         // All transactions in the package must be a parent of the last transaction. This is just an
         // opportunity for us to fail fast on a context-free check without taking the mempool lock.
-        if (!IsChildWithParents(package)) {
-            package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents");
-            return PackageMempoolAcceptResult(package_state_quit_early, {});
-        }
-
-        // IsChildWithParents() guarantees the package is > 1 transactions.
-        assert(package.size() > 1);
-        // The package must be 1 child with all of its unconfirmed parents. The package is expected to
-        // be sorted, so the last transaction is the child.
-        const auto& child = package.back();
-        std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids;
-        std::transform(package.cbegin(), package.cend() - 1,
-                       std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
-                       [](const auto& tx) { return tx->GetHash(); });
-
-        // All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only
-        // way to verify this is to look up the child's inputs in our current coins view (not including
-        // mempool), and enforce that all parents not present in the package be available at chain tip.
-        // Since this check can bring new coins into the coins cache, keep track of these coins and
-        // uncache them if we don't end up submitting this package to the mempool.
-        const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip();
-        for (const auto& input : child->vin) {
-            if (!coins_tip_cache.HaveCoinInCache(input.prevout)) {
-                args.m_coins_to_uncache.push_back(input.prevout);
-            }
-        }
-        // Using the MemPoolAccept m_view cache allows us to look up these same coins faster later.
-        // This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically
-        // require inputs to be confirmed if they aren't in the package.
-        m_view.SetBackend(m_active_chainstate.CoinsTip());
-        const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) {
-             return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout);
-        };
-        if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) {
-            package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents");
-            return PackageMempoolAcceptResult(package_state_quit_early, {});
-        }
-        // Protect against bugs where we pull more inputs from disk that miss being added to
-        // coins_to_uncache. The backend will be connected again when needed in PreChecks.
-        m_view.SetBackend(m_dummy);
+        package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents");
+        return PackageMempoolAcceptResult(package_state_quit_early, {});
     }
 
     LOCK(m_pool.cs);

test/functional/p2p_1p1c_network.py

@@ -103,7 +103,7 @@ class PackageRelayTest(BitcoinTestFramework):
         package_hex_2, parent_2, child_2 = self.create_basic_1p1c(self.wallet_nonsegwit)
 
         # 3: 2-parent-1-child package. Both parents are above mempool min feerate. No package submission happens.
-        # We require packages to be child-with-unconfirmed-parents and only allow 1-parent-1-child packages.
+        # We require packages to be child-with-parents and only allow 1-parent-1-child packages.
         package_hex_3, parent_31, _parent_32, child_3 = self.create_package_2p1c(self.wallet)
 
         # 4: parent + child package where the child spends 2 different outputs from the parent.

test/functional/p2p_opportunistic_1p1c.py

@@ -25,7 +25,6 @@ from test_framework.p2p import (
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
-    assert_equal,
     assert_greater_than,
 )
 from test_framework.wallet import (
@@ -64,14 +63,14 @@ class PackageRelayTest(BitcoinTestFramework):
         ]]
         self.supports_cli = False
 
-    def create_tx_below_mempoolminfee(self, wallet):
+    def create_tx_below_mempoolminfee(self, wallet, utxo_to_spend=None):
         """Create a 1-input 1sat/vB transaction using a confirmed UTXO. Decrement and use
         self.sequence so that subsequent calls to this function result in unique transactions."""
 
         self.sequence -= 1
         assert_greater_than(self.nodes[0].getmempoolinfo()["mempoolminfee"], FEERATE_1SAT_VB)
 
-        return wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, sequence=self.sequence, confirmed_only=True)
+        return wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, sequence=self.sequence, utxo_to_spend=utxo_to_spend, confirmed_only=True)
 
     @cleanup
     def test_basic_child_then_parent(self):
@@ -340,11 +339,14 @@ class PackageRelayTest(BitcoinTestFramework):
 
     @cleanup
     def test_other_parent_in_mempool(self):
-        self.log.info("Check opportunistic 1p1c fails if child already has another parent in mempool")
+        self.log.info("Check opportunistic 1p1c works even if child already has another parent in mempool")
         node = self.nodes[0]
 
+        # Grandparent will enter mempool by itself
+        grandparent_high = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB*10, confirmed_only=True)
+
         # This parent needs CPFP
-        parent_low = self.create_tx_below_mempoolminfee(self.wallet)
+        parent_low = self.create_tx_below_mempoolminfee(self.wallet, utxo_to_spend=grandparent_high["new_utxo"])
         # This parent does not need CPFP and can be submitted alone ahead of time
         parent_high = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB*10, confirmed_only=True)
         child = self.wallet.create_self_transfer_multi(
@@ -354,27 +356,27 @@ class PackageRelayTest(BitcoinTestFramework):
 
         peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="outbound-full-relay")
 
-        # 1. Send first parent which will be accepted.
+        # 1. Send grandparent which is accepted
+        peer_sender.send_and_ping(msg_tx(grandparent_high["tx"]))
+        assert grandparent_high["txid"] in node.getrawmempool()
+
+        # 2. Send first parent which is accepted.
         peer_sender.send_and_ping(msg_tx(parent_high["tx"]))
         assert parent_high["txid"] in node.getrawmempool()
 
-        # 2. Send child.
+        # 3. Send child which is handled as an orphan.
         peer_sender.send_and_ping(msg_tx(child["tx"]))
 
-        # 3. Node requests parent_low. However, 1p1c fails because package-not-child-with-unconfirmed-parents
+        # 4. Node requests parent_low.
         parent_low_txid_int = int(parent_low["txid"], 16)
         peer_sender.wait_for_getdata([parent_low_txid_int])
         peer_sender.send_and_ping(msg_tx(parent_low["tx"]))
 
         node_mempool = node.getrawmempool()
+        assert grandparent_high["txid"] in node_mempool
         assert parent_high["txid"] in node_mempool
-        assert parent_low["txid"] not in node_mempool
-        assert child["txid"] not in node_mempool
-
-        # Same error if submitted through submitpackage without parent_high
-        package_hex_missing_parent = [parent_low["hex"], child["hex"]]
-        result_missing_parent = node.submitpackage(package_hex_missing_parent)
-        assert_equal(result_missing_parent["package_msg"], "package-not-child-with-unconfirmed-parents")
+        assert parent_low["txid"] in node_mempool
+        assert child["txid"] in node_mempool
 
     def run_test(self):
         node = self.nodes[0]

test/functional/rpc_packages.py

@@ -271,9 +271,11 @@ class RPCPackagesTest(BitcoinTestFramework):
         assert_equal(submitres, {'package_msg': 'conflict-in-package', 'tx-results': {}, 'replaced-transactions': []})
         assert tx_child["txid"] not in node.getrawmempool()
 
-        # ... and without the in-mempool ancestor tx1 included in the call
+        # without the in-mempool ancestor tx1 included in the call, tx2 can be submitted, but
+        # tx_child is missing an input.
         submitres = node.submitpackage([tx2["hex"], tx_child["hex"]])
-        assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []})
+        assert_equal(submitres["tx-results"][tx_child["wtxid"]], {"txid": tx_child["txid"], "error": "bad-txns-inputs-missingorspent"})
+        assert tx2["txid"] in node.getrawmempool()
 
         # Regardless of error type, the child can never enter the mempool
         assert tx_child["txid"] not in node.getrawmempool()