Skip to content

Commit c9a7418

Browse files
committed
Merge bitcoin#31096: Package validation: accept packages of size 1
32fc597 rpc: Allow single transaction through submitpackage (glozow) Pull request description: There's no particular reason to restrict single transaction submissions with submitpackage. This change relaxes the RPC checks as enables the `AcceptPackage` flow to accept packages of a single transaction. Resolves bitcoin#31085 ACKs for top commit: naumenkogs: ACK bitcoin@32fc597 achow101: ACK 32fc597 glozow: ACK 32fc597 Tree-SHA512: ffed353bfdca610ffcfd53b40b76da05ffc26df6bac4b0421492e067bede930380e03399d2e2d1d17f0e88fb91cd8eb376e3aabebbabcc724590bf068d09807c
2 parents 6f24662 + 32fc597 commit c9a7418

File tree

5 files changed

+156
-55
lines changed

5 files changed

+156
-55
lines changed

doc/policy/packages.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ The following rules are only enforced for packages to be submitted to the mempoo
7474
enforced for test accepts):
7575

7676
* Packages must be child-with-unconfirmed-parents packages. This also means packages must contain at
77-
least 2 transactions. (#22674)
77+
least 1 transaction. (#31096)
7878

7979
- *Rationale*: This allows for fee-bumping by CPFP. Allowing multiple parents makes it possible
8080
to fee-bump a batch of transactions. Restricting packages to a defined topology is easier to

src/rpc/mempool.cpp

+7-6
Original file line numberDiff line numberDiff line change
@@ -926,7 +926,7 @@ static RPCHelpMan submitpackage()
926926
,
927927
{
928928
{"package", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of raw transactions.\n"
929-
"The package must solely consist of a child and its parents. None of the parents may depend on each other.\n"
929+
"The package must solely consist of a child transaction and all of its unconfirmed parents, if any. None of the parents may depend on each other.\n"
930930
"The package must be topologically sorted, with the child being the last element in the array.",
931931
{
932932
{"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""},
@@ -968,15 +968,15 @@ static RPCHelpMan submitpackage()
968968
},
969969
},
970970
RPCExamples{
971-
HelpExampleRpc("submitpackage", R"(["rawtx1", "rawtx2"])") +
972-
HelpExampleCli("submitpackage", R"('["rawtx1", "rawtx2"]')")
971+
HelpExampleRpc("submitpackage", R"(["raw-parent-tx-1", "raw-parent-tx-2", "raw-child-tx"])") +
972+
HelpExampleCli("submitpackage", R"('["raw-tx-without-unconfirmed-parents"]')")
973973
},
974974
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
975975
{
976976
const UniValue raw_transactions = request.params[0].get_array();
977-
if (raw_transactions.size() < 2 || raw_transactions.size() > MAX_PACKAGE_COUNT) {
977+
if (raw_transactions.empty() || raw_transactions.size() > MAX_PACKAGE_COUNT) {
978978
throw JSONRPCError(RPC_INVALID_PARAMETER,
979-
"Array must contain between 2 and " + ToString(MAX_PACKAGE_COUNT) + " transactions.");
979+
"Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions.");
980980
}
981981

982982
// Fee check needs to be run with chainstate and package context
@@ -1007,7 +1007,8 @@ static RPCHelpMan submitpackage()
10071007

10081008
txns.emplace_back(MakeTransactionRef(std::move(mtx)));
10091009
}
1010-
if (!IsChildWithParentsTree(txns)) {
1010+
CHECK_NONFATAL(!txns.empty());
1011+
if (txns.size() > 1 && !IsChildWithParentsTree(txns)) {
10111012
throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, "package topology disallowed. not child-with-parents or parents depend on each other.");
10121013
}
10131014

src/test/txpackage_tests.cpp

+93
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,8 @@ BOOST_AUTO_TEST_CASE(noncontextual_package_tests)
283283
BOOST_CHECK(GetPackageHash({tx_parent}) != GetPackageHash({tx_child}));
284284
BOOST_CHECK(GetPackageHash({tx_child, tx_child}) != GetPackageHash({tx_child}));
285285
BOOST_CHECK(GetPackageHash({tx_child, tx_parent}) != GetPackageHash({tx_child, tx_child}));
286+
BOOST_CHECK(!IsChildWithParents({}));
287+
BOOST_CHECK(!IsChildWithParentsTree({}));
286288
}
287289

288290
// 24 Parents and 1 Child
@@ -492,6 +494,97 @@ BOOST_AUTO_TEST_CASE(package_submission_tests)
492494
}
493495
}
494496

497+
// Tests for packages containing a single transaction
498+
BOOST_AUTO_TEST_CASE(package_single_tx)
499+
{
500+
// Mine blocks to mature coinbases.
501+
mineBlocks(3);
502+
LOCK(cs_main);
503+
auto expected_pool_size{m_node.mempool->size()};
504+
505+
const CAmount high_fee{1000};
506+
507+
// No unconfirmed parents
508+
CKey single_key = GenerateRandomKey();
509+
CScript single_locking_script = GetScriptForDestination(PKHash(single_key.GetPubKey()));
510+
auto mtx_single = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0,
511+
/*input_height=*/0, /*input_signing_key=*/coinbaseKey,
512+
/*output_destination=*/single_locking_script,
513+
/*output_amount=*/CAmount(49 * COIN), /*submit=*/false);
514+
CTransactionRef tx_single = MakeTransactionRef(mtx_single);
515+
Package package_tx_single{tx_single};
516+
const auto result_single_tx = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
517+
package_tx_single, /*test_accept=*/false, /*client_maxfeerate=*/{});
518+
expected_pool_size += 1;
519+
BOOST_CHECK_MESSAGE(result_single_tx.m_state.IsValid(),
520+
"Package validation unexpectedly failed: " << result_single_tx.m_state.ToString());
521+
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
522+
523+
// Parent and Child. Both submitted by themselves through the ProcessNewPackage interface.
524+
CKey parent_key = GenerateRandomKey();
525+
CScript parent_locking_script = GetScriptForDestination(WitnessV0KeyHash(parent_key.GetPubKey()));
526+
auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[1], /*input_vout=*/0,
527+
/*input_height=*/0, /*input_signing_key=*/coinbaseKey,
528+
/*output_destination=*/parent_locking_script,
529+
/*output_amount=*/CAmount(50 * COIN) - high_fee, /*submit=*/false);
530+
CTransactionRef tx_parent = MakeTransactionRef(mtx_parent);
531+
Package package_just_parent{tx_parent};
532+
const auto result_just_parent = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_just_parent, /*test_accept=*/false, /*client_maxfeerate=*/{});
533+
if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_just_parent, result_just_parent, /*expect_valid=*/true, nullptr)}) {
534+
BOOST_ERROR(err_parent_child.value());
535+
} else {
536+
auto it_parent = result_just_parent.m_tx_results.find(tx_parent->GetWitnessHash());
537+
BOOST_CHECK_MESSAGE(it_parent->second.m_state.IsValid(), it_parent->second.m_state.ToString());
538+
BOOST_CHECK(it_parent->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_parent)) == high_fee);
539+
BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().size(), 1);
540+
BOOST_CHECK_EQUAL(it_parent->second.m_wtxids_fee_calculations.value().front(), tx_parent->GetWitnessHash());
541+
}
542+
expected_pool_size += 1;
543+
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
544+
545+
CKey child_key = GenerateRandomKey();
546+
CScript child_locking_script = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey()));
547+
auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*input_vout=*/0,
548+
/*input_height=*/101, /*input_signing_key=*/parent_key,
549+
/*output_destination=*/child_locking_script,
550+
/*output_amount=*/CAmount(50 * COIN) - 2 * high_fee, /*submit=*/false);
551+
CTransactionRef tx_child = MakeTransactionRef(mtx_child);
552+
Package package_just_child{tx_child};
553+
const auto result_just_child = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_just_child, /*test_accept=*/false, /*client_maxfeerate=*/{});
554+
if (auto err_parent_child{CheckPackageMempoolAcceptResult(package_just_child, result_just_child, /*expect_valid=*/true, nullptr)}) {
555+
BOOST_ERROR(err_parent_child.value());
556+
} else {
557+
auto it_child = result_just_child.m_tx_results.find(tx_child->GetWitnessHash());
558+
BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(), it_child->second.m_state.ToString());
559+
BOOST_CHECK(it_child->second.m_effective_feerate.value().GetFee(GetVirtualTransactionSize(*tx_child)) == high_fee);
560+
BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().size(), 1);
561+
BOOST_CHECK_EQUAL(it_child->second.m_wtxids_fee_calculations.value().front(), tx_child->GetWitnessHash());
562+
}
563+
expected_pool_size += 1;
564+
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
565+
566+
// Too-low fee to RBF tx_single
567+
auto mtx_single_low_fee = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0,
568+
/*input_height=*/0, /*input_signing_key=*/coinbaseKey,
569+
/*output_destination=*/single_locking_script,
570+
/*output_amount=*/CAmount(49 * COIN - 1), /*submit=*/false);
571+
CTransactionRef tx_single_low_fee = MakeTransactionRef(mtx_single_low_fee);
572+
Package package_tx_single_low_fee{tx_single_low_fee};
573+
const auto result_single_tx_low_fee = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
574+
package_tx_single_low_fee, /*test_accept=*/false, /*client_maxfeerate=*/{});
575+
576+
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
577+
578+
BOOST_CHECK(!result_single_tx_low_fee.m_state.IsValid());
579+
BOOST_CHECK_EQUAL(result_single_tx_low_fee.m_state.GetResult(), PackageValidationResult::PCKG_TX);
580+
auto it_low_fee = result_single_tx_low_fee.m_tx_results.find(tx_single_low_fee->GetWitnessHash());
581+
BOOST_CHECK_EQUAL(it_low_fee->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);
582+
if (auto err_single{CheckPackageMempoolAcceptResult(package_tx_single_low_fee, result_single_tx_low_fee, /*expect_valid=*/false, m_node.mempool.get())}) {
583+
BOOST_ERROR(err_single.value());
584+
}
585+
BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
586+
}
587+
495588
// Tests for packages containing transactions that have same-txid-different-witness equivalents in
496589
// the mempool.
497590
BOOST_AUTO_TEST_CASE(package_witness_swap_tests)

src/validation.cpp

+51-43
Original file line numberDiff line numberDiff line change
@@ -1685,10 +1685,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr
16851685

16861686
PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, ATMPArgs& args)
16871687
{
1688+
Assert(!package.empty());
16881689
AssertLockHeld(cs_main);
16891690
// Used if returning a PackageMempoolAcceptResult directly from this function.
16901691
PackageValidationState package_state_quit_early;
16911692

1693+
// There are two topologies we are able to handle through this function:
1694+
// (1) A single transaction
1695+
// (2) A child-with-unconfirmed-parents package.
16921696
// Check that the package is well-formed. If it isn't, we won't try to validate any of the
16931697
// transactions and thus won't return any MempoolAcceptResults, just a package-wide error.
16941698

@@ -1697,48 +1701,50 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
16971701
return PackageMempoolAcceptResult(package_state_quit_early, {});
16981702
}
16991703

1700-
// All transactions in the package must be a parent of the last transaction. This is just an
1701-
// opportunity for us to fail fast on a context-free check without taking the mempool lock.
1702-
if (!IsChildWithParents(package)) {
1703-
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents");
1704-
return PackageMempoolAcceptResult(package_state_quit_early, {});
1705-
}
1706-
1707-
// IsChildWithParents() guarantees the package is > 1 transactions.
1708-
assert(package.size() > 1);
1709-
// The package must be 1 child with all of its unconfirmed parents. The package is expected to
1710-
// be sorted, so the last transaction is the child.
1711-
const auto& child = package.back();
1712-
std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids;
1713-
std::transform(package.cbegin(), package.cend() - 1,
1714-
std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
1715-
[](const auto& tx) { return tx->GetHash(); });
1716-
1717-
// All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only
1718-
// way to verify this is to look up the child's inputs in our current coins view (not including
1719-
// mempool), and enforce that all parents not present in the package be available at chain tip.
1720-
// Since this check can bring new coins into the coins cache, keep track of these coins and
1721-
// uncache them if we don't end up submitting this package to the mempool.
1722-
const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip();
1723-
for (const auto& input : child->vin) {
1724-
if (!coins_tip_cache.HaveCoinInCache(input.prevout)) {
1725-
args.m_coins_to_uncache.push_back(input.prevout);
1726-
}
1727-
}
1728-
// Using the MemPoolAccept m_view cache allows us to look up these same coins faster later.
1729-
// This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically
1730-
// require inputs to be confirmed if they aren't in the package.
1731-
m_view.SetBackend(m_active_chainstate.CoinsTip());
1732-
const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) {
1733-
return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout);
1734-
};
1735-
if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) {
1736-
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents");
1737-
return PackageMempoolAcceptResult(package_state_quit_early, {});
1704+
if (package.size() > 1) {
1705+
// All transactions in the package must be a parent of the last transaction. This is just an
1706+
// opportunity for us to fail fast on a context-free check without taking the mempool lock.
1707+
if (!IsChildWithParents(package)) {
1708+
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-parents");
1709+
return PackageMempoolAcceptResult(package_state_quit_early, {});
1710+
}
1711+
1712+
// IsChildWithParents() guarantees the package is > 1 transactions.
1713+
assert(package.size() > 1);
1714+
// The package must be 1 child with all of its unconfirmed parents. The package is expected to
1715+
// be sorted, so the last transaction is the child.
1716+
const auto& child = package.back();
1717+
std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids;
1718+
std::transform(package.cbegin(), package.cend() - 1,
1719+
std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
1720+
[](const auto& tx) { return tx->GetHash(); });
1721+
1722+
// All child inputs must refer to a preceding package transaction or a confirmed UTXO. The only
1723+
// way to verify this is to look up the child's inputs in our current coins view (not including
1724+
// mempool), and enforce that all parents not present in the package be available at chain tip.
1725+
// Since this check can bring new coins into the coins cache, keep track of these coins and
1726+
// uncache them if we don't end up submitting this package to the mempool.
1727+
const CCoinsViewCache& coins_tip_cache = m_active_chainstate.CoinsTip();
1728+
for (const auto& input : child->vin) {
1729+
if (!coins_tip_cache.HaveCoinInCache(input.prevout)) {
1730+
args.m_coins_to_uncache.push_back(input.prevout);
1731+
}
1732+
}
1733+
// Using the MemPoolAccept m_view cache allows us to look up these same coins faster later.
1734+
// This should be connecting directly to CoinsTip, not to m_viewmempool, because we specifically
1735+
// require inputs to be confirmed if they aren't in the package.
1736+
m_view.SetBackend(m_active_chainstate.CoinsTip());
1737+
const auto package_or_confirmed = [this, &unconfirmed_parent_txids](const auto& input) {
1738+
return unconfirmed_parent_txids.count(input.prevout.hash) > 0 || m_view.HaveCoin(input.prevout);
1739+
};
1740+
if (!std::all_of(child->vin.cbegin(), child->vin.cend(), package_or_confirmed)) {
1741+
package_state_quit_early.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-child-with-unconfirmed-parents");
1742+
return PackageMempoolAcceptResult(package_state_quit_early, {});
1743+
}
1744+
// Protect against bugs where we pull more inputs from disk that miss being added to
1745+
// coins_to_uncache. The backend will be connected again when needed in PreChecks.
1746+
m_view.SetBackend(m_dummy);
17381747
}
1739-
// Protect against bugs where we pull more inputs from disk that miss being added to
1740-
// coins_to_uncache. The backend will be connected again when needed in PreChecks.
1741-
m_view.SetBackend(m_dummy);
17421748

17431749
LOCK(m_pool.cs);
17441750
// Stores results from which we will create the returned PackageMempoolAcceptResult.
@@ -1748,6 +1754,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
17481754
// this transaction. "Nonfinal" because if a transaction fails by itself but succeeds later
17491755
// (i.e. when evaluated with a fee-bumping child), the result in this map may be discarded.
17501756
std::map<uint256, MempoolAcceptResult> individual_results_nonfinal;
1757+
// Tracks whether we think package submission could result in successful entry to the mempool
17511758
bool quit_early{false};
17521759
std::vector<CTransactionRef> txns_package_eval;
17531760
for (const auto& tx : package) {
@@ -1789,8 +1796,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
17891796
// in package validation, because its fees should only be "used" once.
17901797
assert(m_pool.exists(GenTxid::Wtxid(wtxid)));
17911798
results_final.emplace(wtxid, single_res);
1792-
} else if (single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE &&
1793-
single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
1799+
} else if (package.size() == 1 || // If there is only one transaction, no need to retry it "as a package"
1800+
(single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE &&
1801+
single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS)) {
17941802
// Package validation policy only differs from individual policy in its evaluation
17951803
// of feerate. For example, if a transaction fails here due to violation of a
17961804
// consensus rule, the result will not change when it is submitted as part of a

test/functional/rpc_packages.py

+4-5
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,8 @@ def test_submitpackage(self):
377377
assert txid_list[0] not in node.getrawmempool()
378378
assert txid_list[1] not in node.getrawmempool()
379379

380-
self.log.info("Submitpackage valid packages with 1 child and some number of parents")
381-
for num_parents in [1, 2, 24]:
380+
self.log.info("Submitpackage valid packages with 1 child and some number of parents (or none)")
381+
for num_parents in [0, 1, 2, 24]:
382382
self.test_submit_child_with_parents(num_parents, False)
383383
self.test_submit_child_with_parents(num_parents, True)
384384

@@ -389,10 +389,9 @@ def test_submitpackage(self):
389389
assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex)
390390
assert_equal(legacy_pool, node.getrawmempool())
391391

392-
assert_raises_rpc_error(-8, f"Array must contain between 2 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [])
393-
assert_raises_rpc_error(-8, f"Array must contain between 2 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [chain_hex[0]] * 1)
392+
assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [])
394393
assert_raises_rpc_error(
395-
-8, f"Array must contain between 2 and {MAX_PACKAGE_COUNT} transactions.",
394+
-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.",
396395
node.submitpackage, [chain_hex[0]] * (MAX_PACKAGE_COUNT + 1)
397396
)
398397

0 commit comments

Comments
 (0)