Merge bitcoin/bitcoin#31385: package validation: relax the package-not-child-with-unconfirmed-parents rule

ea17a94 [doc] release note for relaxing requirement of all unconfirmed parents present (glozow)
12f48d5 test: add chained 1p1c propagation test (Greg Sanders)
525be56 [unit test] package submission 2p1c with 1 parent missing (glozow)
f24771a relax child-with-unconfirmed-parents rule (glozow)

Pull request description:

  Broadens the package validation interface, see #27463 for wider context.

  On master, package rules include that (1) the package topology must be child-wth-parents (2) all of the child's unconfirmed parents must be present. This PR relaxes the second rule, leaving the first rule untouched (there are plans to change that as well, but not here).

  Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We could potentially do this for ancestor packages (with a similar definition that all UTXOs to make the tx valid are available in this package), but it's also questionable whether it's useful to enforce this.

  This rule gets in the way of certain usage of 1p1c package relay currently. If a transaction has multiple parents, of which only 1 requires a package CPFP, this rule blocks the package from relaying. Even if all the non-low-feerate parents are already in mempool, when the p2p logic submits the 1p1c package, it gets rejected for not meeting this rule.

ACKs for top commit:
  ishaanam:
    re-utACK ea17a94
  instagibbs:
    ACK ea17a94

Tree-SHA512: c2231761ae7b2acea10a96735e7a36c646f517964d0acb59bacbae1c5a1950e0223458b84c6d5ce008f0c1d53c1605df0fb3cd0064ee535ead006eb7c0fa625b

Author: fanquake

doc/policy/packages.md

@@ -8,11 +8,9 @@ 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.
+A **child-with-parents** package is a topologically sorted package that consists of exactly one child and at least one
+of its unconfirmed parents.  Not all unconfirmed parents need to be present but no other transactions may be present; the
+parent of a parent should not be in this package (unless this "grandparent" is also a direct parent of the child).
 
 ## Package Mempool Acceptance Rules
 
@@ -73,7 +71,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

doc/release-notes-31385.md

@@ -0,0 +1,14 @@
+RPC
+
+The `submitpackage` RPC, which allows submissions of child-with-parents
+packages, no longer requires that all unconfirmed parents be present. The
+package may contain other in-mempool ancestors as well. (#31385)
+
+P2P
+
+Opportunistic 1-parent-1-child package relay has been improved to handle
+situations when the child already has unconfirmed parent(s) in the mempool.
+This means that 1p1c packages can be accepted and propagate, even if they are
+connected to broader topologies: multi-parent-1-child (where only 1 parent
+requires fee-bumping), grandparent-parent-child (where only parent requires
+fee-bumping) etc. (#31385)

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 a parent
 // 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/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 @@ class MemPoolAccept
             };
         }
 
-        /** 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,
@@ -1716,7 +1716,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.
 
@@ -1725,49 +1725,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

@@ -101,7 +101,7 @@ def create_packages(self):
         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.