There’s no particular reason to restrict single transaction submissions with submitpackage. This change relaxes the RPC checks as well as the IsChildWith* checks to allow these cases, as well as return false rather than segfault with an empty package.
Resolves #31085
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31096.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | glozow, kevkevinpal, theStack |
| Stale ACK | rkrux, stickies-v, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
1694@@ -1695,15 +1695,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1695 return PackageMempoolAcceptResult(package_state_quit_early, {});
1696 }
1697
1698- // IsChildWithParents() guarantees the package is > 1 transactions.
1699- assert(package.size() > 1);
I wouldn’t remove this, but rather update to:
0 // IsChildWithParents() guarantees the package is not empty.
1 assert(!package.empty());
After all, if the package is empty, we’d have UB a few lines down.
IsChildWithParents can guarantee. Perhaps it’s safer to change the RPC code or ProcessNewPackage to direct a single transaction to ATMP instead.
AcceptSubPackage internally, I’m not convinced this is more risky than adding more complexity on top.
I think I agree with instagibbs that adding complexity on top should be avoided if we can, and it seems to me like modifying IsChildWithParents() to better align with the Package definition (i.e. child and all unconfirmed parents, as per AcceptPackage()) is preferable for long-term robustness, over special-casing a transaction that has 0 unconfirmed parents as is currently the case.
I looked at the callstacks that make use of IsChildWithParents(), and since the scope is fairly limited, I feel quite confident that this new behaviour is safe? But of course, my knowledge on this part of the codebase comes nowhere near what you both have.
My summary of the call hierarchy of IsChildWithParents() and how this PR affects it:
IsChildWithParentsTree(), which is a RPC-only function and looks unaffected by this PRMemPoolAccept::PackageMempoolChecks(), which is unaffected by this PR because the callsite guarantees the package is of size 2.MemPoolAccept::AcceptPackage(), which looks safe, but affects the return value for callsites:
ProcessNewPackage(), which looks safe, but affects the return value for callsites:
net_processing.cpp (2x), which is unaffected by this PR, because ProcessNewPackage() is guaranteed to be called with a package of size 2 (from Find1P1CPackage())testmempoolaccept(), which is unaffected by this PR because ProcessNewPackage() is only called when txns.size() > 1submitpackage(), which is the goal of this PR, and seems safe to me.Potentially, we could increase test coverage for a single-tx package? There are probably better ways to write these, but e.g. these tests on ProcessNewPackage() (and as a bonus, but perhaps not needed here on IsWellFormedPackage()) seem like they could be useful?
0diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
1index 9954ad6b7c..3a90a81ea2 100644
2--- a/src/test/txpackage_tests.cpp
3+++ b/src/test/txpackage_tests.cpp
4@@ -357,6 +357,17 @@ BOOST_AUTO_TEST_CASE(noncontextual_package_tests)
5 BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY);
6 BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
7 }
8+
9+ // Transaction without unconfirmed parents
10+ {
11+ CMutableTransaction mtx;
12+ mtx.vin.emplace_back(COutPoint(m_coinbase_txns[0]->GetHash(), 0));
13+ mtx.vout.emplace_back(20 * COIN, spk);
14+ CTransactionRef tx = MakeTransactionRef(mtx);
15+
16+ PackageValidationState state;
17+ BOOST_CHECK(IsWellFormedPackage({tx}, state, /*require_sorted=*/true));
18+ }
19 }
20
21 BOOST_AUTO_TEST_CASE(package_submission_tests)
22@@ -500,6 +511,29 @@ BOOST_AUTO_TEST_CASE(package_submission_tests)
23 }
24 }
25
26+// Tests for packages containing a single transaction that doesn't have
27+// any unconfirmed parents.
28+BOOST_AUTO_TEST_CASE(package_single_tx)
29+{
30+ LOCK(cs_main);
31+ auto expected_pool_size{m_node.mempool->size()};
32+
33+ CKey single_key = GenerateRandomKey();
34+ CScript single_locking_script = GetScriptForDestination(PKHash(single_key.GetPubKey()));
35+ auto mtx_single = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*input_vout=*/0,
36+ /*input_height=*/0, /*input_signing_key=*/coinbaseKey,
37+ /*output_destination=*/single_locking_script,
38+ /*output_amount=*/CAmount(49 * COIN), /*submit=*/false);
39+ CTransactionRef tx_single = MakeTransactionRef(mtx_single);
40+ Package package_tx_single{tx_single};
41+ const auto result_single_tx = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
42+ package_tx_single, /*test_accept=*/false, /*client_maxfeerate=*/{});
43+ expected_pool_size += 1;
44+ BOOST_CHECK_MESSAGE(result_single_tx.m_state.IsValid(),
45+ "Package validation unexpectedly failed: " << result_single_tx.m_state.GetRejectReason());
46+ BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
47+}
48+
49 // Tests for packages containing transactions that have same-txid-different-witness equivalents in
50 // the mempool.
51 BOOST_AUTO_TEST_CASE(package_witness_swap_tests)
hmm, it’s a fair bit of code difference because we essentially need to get BroadcastTransaction grafted into place, then also return the proper PackageMempoolAcceptResult details vs the coarse details returned by it. My attempt required quite a bit of churn and additional testing required for a small feature like this, so I’m keeping as-is.
I’m also thinking now that gating the call via package-not-child-with-unconfirmed-parents check is the right thing to do. The further we stray from 1P1C ergonomically I think the harder it’s going to be to explain what will propagate and what will not. If anything we should be tightening the RPC call further to match.
1702 const auto& child = package.back();
1703 std::unordered_set<uint256, SaltedTxidHasher> unconfirmed_parent_txids;
1704- std::transform(package.cbegin(), package.cend() - 1,
1705- std::inserter(unconfirmed_parent_txids, unconfirmed_parent_txids.end()),
1706- [](const auto& tx) { return tx->GetHash(); });
1707+ if (package.size() > 1) {
390 node.submitpackage, [chain_hex[0]] * (MAX_PACKAGE_COUNT + 1)
391 )
392
393+ # Singleton is ok
394+ node.submitpackage([chain_hex[0]] * 1)
395+
No need to special-case this:
0diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
1index d4da0f9b06..ce42bf810e 100755
2--- a/test/functional/rpc_packages.py
3+++ b/test/functional/rpc_packages.py
4@@ -370,7 +370,7 @@ class RPCPackagesTest(BitcoinTestFramework):
5 node = self.nodes[0]
6
7 self.log.info("Submitpackage valid packages with 1 child and some number of parents")
8- for num_parents in [1, 2, 24]:
9+ for num_parents in [0, 1, 2, 24]:
10 self.test_submit_child_with_parents(num_parents, False)
11 self.test_submit_child_with_parents(num_parents, True)
12
13@@ -387,9 +387,6 @@ class RPCPackagesTest(BitcoinTestFramework):
14 node.submitpackage, [chain_hex[0]] * (MAX_PACKAGE_COUNT + 1)
15 )
16
17- # Singleton is ok
18- node.submitpackage([chain_hex[0]] * 1)
19-
20 # Create a transaction chain such as only the parent gets accepted (by making the child's
21 # version non-standard). Make sure the parent does get broadcast.
22 self.log.info("If a package is partially submitted, transactions included in mempool get broadcast")
282@@ -283,6 +283,14 @@ BOOST_AUTO_TEST_CASE(noncontextual_package_tests)
283 BOOST_CHECK(GetPackageHash({tx_parent}) != GetPackageHash({tx_child}));
284 BOOST_CHECK(GetPackageHash({tx_child, tx_child}) != GetPackageHash({tx_child}));
285 BOOST_CHECK(GetPackageHash({tx_child, tx_parent}) != GetPackageHash({tx_child, tx_child}));
286+
287+ // IsChildWithParents* can also be a singleton
288+ BOOST_CHECK(IsChildWithParents({tx_parent}));
289+ BOOST_CHECK(IsChildWithParentsTree({tx_parent}));
nit: we require exactly one child, so I think using tx_parent is unnecessarily confusing
0 BOOST_CHECK(IsChildWithParents({tx_child}));
1 BOOST_CHECK(IsChildWithParentsTree({tx_child}));
117@@ -118,8 +118,9 @@ bool IsWellFormedPackage(const Package& txns, PackageValidationState& state, boo
118
119 bool IsChildWithParents(const Package& package)
120 {
121+ if (package.empty()) return false;
122 assert(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}));
123- if (package.size() < 2) return false;
124+ if (package.size() < 2) return true;
Worth updating RPC doc as well here? Informing that only child is also allowed.
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L926
I had similar thoughts, yes. I think the current description is correct but less helpful than it could be (even without this PR).
Suggested alternative:
“The package must solely consist of a child transaction and all, some or none of its unconfirmed parents. None of the parents may depend on each other.\n”
You must include unconfirmed parents, if any exist.
Thanks, you’re right. I was confused by the IsChildWithParents documentation stating:
…not all parents need to be present…
But that is of course just an incomplete, context-free check. The MemPoolAccept::AcceptPackage documentation and implementation indeed require all unconfirmed parents to be included. Your suggestion seems better.
Perhaps including the no-parents case in the RPC example would be helpful too:
0diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
1index 09a9d87c2c..3074b3994a 100644
2--- a/src/rpc/mempool.cpp
3+++ b/src/rpc/mempool.cpp
4@@ -965,8 +965,8 @@ static RPCHelpMan submitpackage()
5 },
6 },
7 RPCExamples{
8- HelpExampleRpc("submitpackage", R"(["rawtx1", "rawtx2"])") +
9- HelpExampleCli("submitpackage", R"('["rawtx1", "rawtx2"]')")
10+ HelpExampleRpc("submitpackage", R"(["raw-parent-tx-1", "raw-parent-tx-2", "raw-child-tx"])") +
11+ HelpExampleCli("submitpackage", R"('["raw-tx-without-unconfirmed-parents"]')")
12 },
13 [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
14 {
tACK https://github.com/bitcoin/bitcoin/commit/72872c7073e99da0828e788b25520dcecf721c1b Would be great to have the docs updated as well.
Make and functional tests successful.
In regtest I manually created a PSBT, processed it and submitted a single transaction package with successful response.
0➜ bitcoin git:(2024-10-submitpackage-singleton) ✗ bitcoincli submitpackage "[\"020000000001018e61beaa1f85daba48a46e6bef4e58d12f32768d347185cb077c988cc5af976b0000000000fdffffff02e60f1024010000001600148d6b22f6e4c0c5dc96529ca5ebc6ad0cde42966800e1f5050000000016001468c93e48eeed6fda96d6d234da5c3ccb5e052856024730440220292c5b776a8d1280bced38c1ef463ff204bd815801798e16909bd9dfd20e495a02206a0d2c1a55bad94b69411ca9ed4656d506066a5aa88265bfd7a08a644bc015840121039814ceadf23fc32f60fa4f607c4882f20e5cb12f766c66407dbc61f0cc51e30d00000000\"]"
1{
2 "package_msg": "success",
3 "tx-results": {
4 "6f3a4de5d782679609abd424dcfd3711472c8fdb5270695a3c0636bad4717e09": {
5 "txid": "108cd63d68165c7af07cf410a6055eade488cb3d049935c79832f8b96f817e69",
6 "vsize": 141,
7 "fees": {
8 "base": 0.00000282,
9 "effective-feerate": 0.00002000,
10 "effective-includes": [
11 "6f3a4de5d782679609abd424dcfd3711472c8fdb5270695a3c0636bad4717e09"
12 ]
13 }
14 }
15 },
16 "replaced-transactions": [
17 ]
18}
package-not-child-with-unconfirmed-parents to allow people to submit just the cpfp alone? Should be done in a separate PR regardless.
@glozow we may want to consider relaxing
package-not-child-with-unconfirmed-parentsto allow people to submit just the cpfp alone? Should be done in a separate PR regardless.
That works without validation changes if you make the split happen within RPC or ProcessNewPackage.
lgtm ACK 92ed1bd586f7048f75b38024521696ff04e5f69c
Though @stickies-v’s suggestion would be a nice addition. This was non-obvious to me.
1694@@ -1695,8 +1695,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1695 return PackageMempoolAcceptResult(package_state_quit_early, {});
1696 }
1697
1698- // IsChildWithParents() guarantees the package is > 1 transactions.
1699- assert(package.size() > 1);
1700+ // IsChildWithParents() guarantees the package is not empty.
1701+ assert(!package.empty());
Orthogonal to the changes here, but related and I think it would be good to add this assert + doc to IsChildWithParentsTree() too, if you force-push.
0diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp
1index 38f26a9c4b..e34f94cbb1 100644
2--- a/src/policy/packages.cpp
3+++ b/src/policy/packages.cpp
4@@ -138,6 +138,7 @@ bool IsChildWithParentsTree(const Package& package)
5 {
6 if (!IsChildWithParents(package)) return false;
7 std::unordered_set<uint256, SaltedTxidHasher> parent_txids;
8+ assert(!package.empty()); // IsChildWithParents() guarantees the package is not empty.
9 std::transform(package.cbegin(), package.cend() - 1, std::inserter(parent_txids, parent_txids.end()),
10 [](const auto& ptx) { return ptx->GetHash(); });
11 // Each parent must not have an input who is one of the other parents.
ACK 303661871debadee5f67bd7e4cd0cccc85344ef2
I think reducing the special (and imo unexpected) treatment of packages of transactions without unconfirmed parents by this PR is an improvement. I looked at the call hierarchy of IsChildWithParents(), and the changes look safe to me, but I am not super familiar with this part of the code.
IsWellFormedPackage coverage already looks good to me, added our unit test for single tx package processing, thanks
1694@@ -1695,8 +1695,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1695 return PackageMempoolAcceptResult(package_state_quit_early, {});
1696 }
1697
1698- // IsChildWithParents() guarantees the package is > 1 transactions.
1699- assert(package.size() > 1);
AcceptSubPackage twice for a single transaction, if individual_results_nonfinal.emplace(wtxid, single_res); stuff is triggered?
you’re right, relatively harmless, but pushed a change which means it won’t try package evaluation unless txns_package_eval is larger than size 1.
edit: causes more issues actually, thinking on this
I’ve failed to suggest a simple code-change to avoid double-validation of a single tx.
The code is really about multi-tx. Even if we add a bunch of conditions around auto multi_submission_result, stuff below (individual_results_nonfinal) makes you think about multi-tx again.
The least thing I’m worried about is double-validation on itself. I’m worried about code comprehension complexity: reading multi-tx code against a single-tx scenario.
At this point I agree with Gloria to handle this case earlier. Or substantially moving code around inside this function to make reasoning about 1-tx more natural to it.
I’ve already made the attempt to completel split out the code paths and it was far more work to understand as I have to use the BroadcastTransaction interface which is quite different than testmempoolaccept’s requirements.
(edit, wrong paste!) This is the most minimal fix I can devise:
0diff --git a/src/validation.cpp b/src/validation.cpp
1index 3e48335255..f4c5b1bf9b 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -1775,12 +1775,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
5 if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) {
6 // The transaction succeeded on its own and is now in the mempool. Don't include it
7 // in package validation, because its fees should only be "used" once.
8 assert(m_pool.exists(GenTxid::Wtxid(wtxid)));
9 results_final.emplace(wtxid, single_res);
10- } else if (single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE &&
11- single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
12+ } else if (package.size() == 1 || // Package validation cannot save this single tx
13+ (single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE &&
14+ single_res.m_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS)) {
15 // Package validation policy only differs from individual policy in its evaluation
16 // of feerate. For example, if a transaction fails here due to violation of a
17 // consensus rule, the result will not change when it is submitted as part of a
18 // package. To minimize the amount of repeated work, unless the transaction fails
19 // due to feerate or missing inputs (its parent is a previous transaction in the
The fix for double-validation seems good.
Do you mind sharing the code of your attempt (assuming it compiles :))? I wanna take a look, but would be useful for archival reasons anyway.
Do you mind sharing the code of your attempt (assuming it compiles :))? I wanna take a look, but would be useful for archival reasons anyway.
It compiled, but it won’t pass tests because I would have had to heavily modify the BroadcastTransaction interface to report the various types of failures and return conditions. Not sure I kept the branch sorry.
else if (package.size() == 1 || // still? I think it’s an improvement for code comprehension if we end up handling single-tx here.
Expanding on this thought, I sketched a two-step improvement in handling single-package tx wrt code readability. Wanna take a look?
I certainly might be missing something.
Left a comment, I think my one-liner is the cleanest correct solution?
pushed it as a separate commit
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32484608582
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Users may want to entirely switch to submitpackage
even for loose transactions. We should let them.
re-ACK d120154d55, no changes except for addressing merge conflict from #31139
edit: updated hash, still had the old range-diff hash in my clipboard
Re-ACK d120154d55451b6c90b709a4a093cbb6cabe48d8
Added improved rpc help and unit test case since last review.
Labels
TX fees and policy
Mempool
Milestone
29.0