diff --git a/doc/policy/packages.md b/doc/policy/packages.md index febdbbf13ca..259f9e37893 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: @@ -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 diff --git a/src/policy/packages.h b/src/policy/packages.h index 4b2350edac7..09dfd888c1e 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 3c36b782604..173a4ea4c52 100644 --- a/src/test/txpackage_tests.cpp +++ b/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 diff --git a/src/validation.cpp b/src/validation.cpp index bf370d171a5..827a901f8a3 100644 --- a/src/validation.cpp +++ b/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& 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 e48e5b88b67..5e55c6fd10c 100755 --- a/test/functional/p2p_1p1c_network.py +++ b/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. diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index addb5027f37..31bd2e369f6 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/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): @@ -339,11 +338,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( @@ -353,27 +355,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] diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index a2f9210f94d..41f2c901a69 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()