diff --git a/doc/policy/packages.md b/doc/policy/packages.md index febdbbf13c..4c45470e24 100644 --- a/doc/policy/packages.md +++ b/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 diff --git a/src/policy/packages.h b/src/policy/packages.h index 4b2350edac..5ed7f32a43 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 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); diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 9b6d22dde3..e8976fcf42 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 7992b85712..7a87ba4845 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -522,7 +522,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& coins_to_uncache, const std::optional& 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 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 fbf242a06c..4d0f11ea99 100755 --- a/test/functional/p2p_1p1c_network.py +++ b/test/functional/p2p_1p1c_network.py @@ -101,7 +101,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. diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index e841cf0806..6652442c36 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/test/functional/p2p_opportunistic_1p1c.py @@ -78,14 +78,14 @@ class PackageRelayTest(BitcoinTestFramework): "-maxmempool=5", ]] - 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): @@ -353,11 +353,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( @@ -367,27 +370,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 create_small_orphan(self): """Create small orphan transaction""" diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index 3947053c41..efb08784f5 100755 --- a/test/functional/rpc_packages.py +++ b/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()