diff --git a/doc/policy/packages.md b/doc/policy/packages.md index cd705ca1995217..bf50e9c5df8e98 100644 --- a/doc/policy/packages.md +++ b/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: @@ -75,7 +69,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 diff --git a/src/policy/packages.h b/src/policy/packages.h index 30503201226efb..9e9959c88a4f44 100644 --- a/src/policy/packages.h +++ b/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); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 17c7c0cc92ceef..1d4497947caf4b 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -441,20 +441,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, diff --git a/src/validation.cpp b/src/validation.cpp index e668bd569622c8..8bc96366e9f6b2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -512,7 +512,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& coins_to_uncache, const std::optional& client_maxfeerate) { return ATMPArgs{/* m_chainparams */ chainparams, @@ -1678,7 +1678,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. @@ -1687,49 +1687,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 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); diff --git a/test/functional/p2p_1p1c_network.py b/test/functional/p2p_1p1c_network.py index cdc4e1691d6aa2..6a7774ded358f3 100755 --- a/test/functional/p2p_1p1c_network.py +++ b/test/functional/p2p_1p1c_network.py @@ -103,7 +103,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. diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index 4477046c8d6d14..00b94d1a717cd2 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/test/functional/p2p_opportunistic_1p1c.py @@ -340,7 +340,7 @@ def test_multiple_parents(self): @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] # This parent needs CPFP @@ -361,20 +361,15 @@ def test_other_parent_in_mempool(self): # 2. Send child. peer_sender.send_and_ping(msg_tx(child["tx"])) - # 3. Node requests parent_low. However, 1p1c fails because package-not-child-with-unconfirmed-parents + # 3. 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 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] diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index f5d42d0c343200..586d3d5b510549 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -267,9 +267,11 @@ def test_conflicting(self): 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()