Package validation: accept packages of size 1

instagibbs commented at 9:39 am on October 16, 2024: member

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 #31085

DrahtBot commented at 9:39 am on October 16, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31096.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, naumenkogs, achow101
Concept ACK	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.

Conflicts

No conflicts as of last run.

TheCharlatan commented at 9:42 am on October 16, 2024: contributor

Concept ACK

in src/validation.cpp:1695 in 6915ddb669 outdated

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);

stickies-v commented at 10:00 am on October 16, 2024:

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.

glozow commented at 10:27 am on October 16, 2024:

Given that this assert exists, I’m worried we have other places with very strict assumptions on what IsChildWithParents can guarantee. Perhaps it’s safer to change the RPC code or ProcessNewPackage to direct a single transaction to ATMP instead.

instagibbs commented at 10:33 am on October 16, 2024:

I’d rather not; we almost immediately handle singletons via AcceptSubPackage internally, I’m not convinced this is more risky than adding more complexity on top.

glozow commented at 10:47 am on October 16, 2024:

This is what testmempoolaccept does

stickies-v commented at 1:29 pm on October 22, 2024:

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 PR
MemPoolAccept::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() > 1
  - submitpackage(), 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)

instagibbs commented at 7:26 pm on October 22, 2024:

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.

in src/validation.cpp:1691 in 6915ddb669 outdated

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) {

stickies-v commented at 10:02 am on October 16, 2024:

I think this check can be dropped, operation should be safe and no-op if package is empty? I’m fine with it either way, just simplifies the code a bit.

instagibbs commented at 10:24 am on October 16, 2024:

I’m bad at reading the spec, is that defined behavior? https://en.cppreference.com/w/cpp/algorithm/transform

instagibbs commented at 10:28 am on October 16, 2024:

ok I’m convinced, removed

stickies-v commented at 10:03 am on October 16, 2024: contributor

Concept ACK

instagibbs force-pushed on Oct 16, 2024

glozow commented at 10:28 am on October 16, 2024: member

concept ACK

instagibbs force-pushed on Oct 16, 2024

in test/functional/rpc_packages.py:392 in 72872c7073 outdated

390             node.submitpackage, [chain_hex[0]] * (MAX_PACKAGE_COUNT + 1)
391         )
392 
393+        # Singleton is ok
394+        node.submitpackage([chain_hex[0]] * 1)
395+

stickies-v commented at 11:26 am on October 16, 2024:

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")

instagibbs commented at 1:54 pm on October 21, 2024:

done

in src/test/txpackage_tests.cpp:289 in 72872c7073 outdated

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}));

stickies-v commented at 11:27 am on October 16, 2024:

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}));

instagibbs commented at 1:54 pm on October 21, 2024:

sure done

in src/policy/packages.cpp:123 in 72872c7073 outdated

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;

stickies-v commented at 11:41 am on October 16, 2024:

Note: this also requires updating this documentation: https://github.com/bitcoin/bitcoin/blob/538ccaed004ff89ec8b2b71d8711feed624ffccc/doc/policy/packages.md?plain=1#L78-L79

rkrux commented at 6:55 am on October 21, 2024:

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

stickies-v commented at 10:21 am on October 21, 2024:

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”

instagibbs commented at 1:54 pm on October 21, 2024:

took a slightly different tack, I think this is more correct. You must include unconfirmed parents, if any exist.

stickies-v commented at 4:20 pm on October 21, 2024:

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         {

instagibbs commented at 4:24 pm on October 21, 2024:

can do when touched again

instagibbs commented at 7:25 pm on October 22, 2024:

done

stickies-v commented at 11:43 am on October 16, 2024: contributor

Approach ACK, code LGTM 72872c7073e99da0828e788b25520dcecf721c1b and change looks safe at first sight but want to take some more time to think about it as I’m not super familiar with this code.

kevkevinpal commented at 1:23 pm on October 18, 2024: contributor

Concept ACK

laanwj added the label Mempool on Oct 20, 2024

laanwj added the label TX fees and policy on Oct 20, 2024

rkrux approved

rkrux commented at 6:59 am on October 21, 2024: none

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}

DrahtBot requested review from glozow on Oct 21, 2024

DrahtBot requested review from TheCharlatan on Oct 21, 2024

DrahtBot requested review from stickies-v on Oct 21, 2024

fanquake added this to the milestone 29.0 on Oct 21, 2024

instagibbs force-pushed on Oct 21, 2024

instagibbs commented at 1:57 pm on October 21, 2024: member

@glozow we may want to consider relaxing package-not-child-with-unconfirmed-parents to allow people to submit just the cpfp alone? Should be done in a separate PR regardless.

glozow commented at 0:31 am on October 22, 2024: member

@glozow we may want to consider relaxing package-not-child-with-unconfirmed-parents to 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.

TheCharlatan approved

TheCharlatan commented at 9:33 am on October 22, 2024: contributor

lgtm ACK 92ed1bd586f7048f75b38024521696ff04e5f69c

Though @stickies-v’s suggestion would be a nice addition. This was non-obvious to me.

DrahtBot requested review from rkrux on Oct 22, 2024

in src/validation.cpp:1695 in 92ed1bd586 outdated

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());

stickies-v commented at 1:22 pm on October 22, 2024:

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.

instagibbs force-pushed on Oct 22, 2024

stickies-v approved

stickies-v commented at 1:25 pm on October 24, 2024: contributor

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.

DrahtBot requested review from TheCharlatan on Oct 24, 2024

instagibbs force-pushed on Oct 25, 2024

instagibbs commented at 1:47 pm on October 25, 2024: member

@stickies-v IsWellFormedPackage coverage already looks good to me, added our unit test for single tx package processing, thanks

stickies-v commented at 11:44 am on October 28, 2024: contributor

re-ACK 5e4df9f67179e9cc284cb2ad2264de6b8bb6c606, thanks for adding the test.

DrahtBot added the label Needs rebase on Nov 1, 2024

in src/validation.cpp:1695 in 5e4df9f671 outdated

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);

naumenkogs commented at 12:10 pm on November 4, 2024:

Won’t we redundantly call AcceptSubPackage twice for a single transaction, if individual_results_nonfinal.emplace(wtxid, single_res); stuff is triggered?

instagibbs commented at 3:17 pm on November 4, 2024:

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

instagibbs commented at 5:26 pm on November 4, 2024:

It also causes double-validation for single transaction package that is missing inputs.

naumenkogs commented at 9:02 am on November 6, 2024:

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.

instagibbs commented at 3:13 pm on November 6, 2024:

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

naumenkogs commented at 11:28 am on November 7, 2024:

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.

instagibbs commented at 2:22 pm on November 12, 2024:

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.

naumenkogs commented at 6:58 am on November 13, 2024:

So you’d rather not take else if (package.size() == 1 || // still? I think it’s an improvement for code comprehension if we end up handling single-tx here.

naumenkogs commented at 11:41 am on November 13, 2024:

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.

instagibbs commented at 2:10 pm on November 13, 2024:

Left a comment, I think my one-liner is the cleanest correct solution?

pushed it as a separate commit

naumenkogs commented at 7:36 am on November 14, 2024:

Have you left a comment on my branch? I can’t see it for some reason.

instagibbs commented at 2:03 pm on November 14, 2024:

https://github.com/naumenkogs/bitcoin/commit/88c10c5789bfae1865d03966d1ce523e870ff0bc#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1811 weird comment UI, here is a direct link hopefully

naumenkogs commented at 8:08 am on November 18, 2024:

Weird UI indeed, i left a reply and I’ll try to monitor that thread now.

instagibbs force-pushed on Nov 4, 2024

instagibbs commented at 3:30 pm on November 4, 2024: member

rebased on master due to test conflict

DrahtBot added the label CI failed on Nov 4, 2024

DrahtBot commented at 4:56 pm on November 4, 2024: contributor

🚧 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.

DrahtBot removed the label Needs rebase on Nov 4, 2024

instagibbs force-pushed on Nov 4, 2024

DrahtBot removed the label CI failed on Nov 4, 2024

stickies-v commented at 5:27 pm on November 5, 2024: contributor

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

naumenkogs commented at 8:14 am on November 6, 2024: member

@stickies-v i think you might be looking at the wrong commit. The hash you acked is outdated.

TheCharlatan approved

TheCharlatan commented at 9:21 pm on November 7, 2024: contributor

Re-ACK d120154d55451b6c90b709a4a093cbb6cabe48d8

Added improved rpc help and unit test case since last review.

theStack commented at 1:58 pm on November 9, 2024: contributor

Concept ACK

DrahtBot added the label CI failed on Nov 13, 2024

DrahtBot removed the label CI failed on Nov 13, 2024

glozow commented at 1:03 pm on November 22, 2024: member

I still don’t really like that the IsChildWithParents() function returns true for a tx that isn’t a child / doesn’t have any parents. Also dislike that we run the whole input-loading thing for the child-with-unconfirmed-parents check even for a singleton tx. I guess one way around this is to rename the 2 IsChildWithParents functions to something along the lines of “is topology we can handle” and to add a package.size() > 1 gate to the child-with-unconfirmed-parents check.

I spent some time implementing the redirection at submitpackage and at ProcessNewPackage, and I now agree with you that it’s not as clean. Considering AcceptSubPackage is already smoothing out those joints, I like the idea of getting rid of ATMP in the future, just throwing things at ProcessNewPackage, and enumerating what we can handle at the start of AcceptPackage.

Here is my counteroffer branch which is fairly similar to this. I’ve also implemented a removal of the is-child-with-unconfirmed-parents rule on top of it, which I think we’re ready for after this PR. There’s also an extra unit test in between. Lmk what you think?

instagibbs commented at 5:04 pm on November 22, 2024: member

@glozow I think I agree your branch at 4737df3f6512d2d9c7f8aa95e0635b9d03031402 is superior to this.

Considering AcceptSubPackage is already smoothing out those joints, I like the idea of getting rid of ATMP in the future, just throwing things at ProcessNewPackage, and enumerating what we can handle at the start of AcceptPackage.

Yes, I would love to get rid of the separate paths over time.

’ve also implemented a removal of the is-child-with-unconfirmed-parents rule on top of it, which I think we’re ready for after this PR

I’ll have to do a bunch more thinking and clearly requires its own PR.

glozow commented at 5:08 pm on November 22, 2024: member

thanks for looking!

I’ll have to do a bunch more thinking and clearly requires its own PR.

In case it wasn’t clear, I definitely meant that as a separate PR. I only implemented it to sketch out what it’d look like as the next step.

instagibbs force-pushed on Nov 22, 2024

instagibbs commented at 5:19 pm on November 22, 2024: member

Took @glozow first two commits with a couple additional sanity check lines.

edit: Turns out it doesn’t correctly mark package as failed if single tx fails. Might have to go half-back to my solution here and add unit test coverage?

DrahtBot added the label CI failed on Nov 22, 2024

DrahtBot commented at 6:26 pm on November 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33394400492

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.

instagibbs force-pushed on Nov 22, 2024

DrahtBot removed the label CI failed on Nov 22, 2024

glozow commented at 11:03 pm on November 22, 2024: member

edit: Turns out it doesn’t correctly mark package as failed if single tx fails.

Sorry about that! I’m happy with the package.size() == 1 gate for the retry logic. Nice that the fuzzer caught that. Can add a low fee test case to package_single_tx to hit it I think.

ACK 278cb41e8785a4948972a4b4f635c2654edd0e3e

DrahtBot requested review from stickies-v on Nov 22, 2024

DrahtBot requested review from TheCharlatan on Nov 22, 2024

DrahtBot requested review from theStack on Nov 22, 2024

naumenkogs commented at 6:45 am on November 25, 2024: member

ACK 278cb41e8785a4948972a4b4f635c2654edd0e3e

instagibbs force-pushed on Nov 25, 2024

instagibbs commented at 3:42 pm on November 25, 2024: member

Would have been remiss to not push a regression test for the issue the fuzzer caught. Pushed test case for coverage of a reconsiderable tx.

rpc: Allow single transaction through submitpackage

And under the hood suppoert single transactions
in AcceptPackage. This simplifies user experience
and paves the way for reducing number of codepaths
for transaction acceptance in the future.

Co-Authored-By: instagibbs <gsanders87@gmail.com>

32fc59796f

in src/test/txpackage_tests.cpp:584 in 3595e7ce9c outdated

575+
576+    BOOST_CHECK(!result_single_tx_low_fee.m_state.IsValid());
577+    BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
578+    if (auto err_single{CheckPackageMempoolAcceptResult(package_tx_single_low_fee, result_single_tx_low_fee, /*expect_valid=*/false, m_node.mempool.get())}) {
579+        BOOST_ERROR(err_single.value());
580+    }

glozow commented at 7:11 pm on November 25, 2024:

Check the error?

0BOOST_CHECK_EQUAL(result_single_tx_low_fee.m_state.GetResult(), PackageValidationResult::PCKG_TX);
1auto it_low_fee = result_single_tx_low_fee.m_tx_results.find(tx_single_low_fee->GetWitnessHash());
2BOOST_CHECK_EQUAL(it_low_fee->second.m_state.GetResult(), TxValidationResult::TX_RECONSIDERABLE);

instagibbs commented at 7:26 pm on November 25, 2024:

done

instagibbs force-pushed on Nov 25, 2024

in src/test/txpackage_tests.cpp:582 in 32fc59796f

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())}) {

glozow commented at 9:00 pm on November 25, 2024:

nit: in the future, best to call CheckPackageMempoolAcceptResult beforehand because the errors would be more helpful if they happened (e.g. “error this value should exist” instead of bad optional access)

glozow commented at 9:01 pm on November 25, 2024: member

ACK 32fc59796f74a2941772b5ec2755b1319132cd9c

DrahtBot requested review from naumenkogs on Nov 25, 2024

naumenkogs commented at 8:34 am on November 27, 2024: member

ACK https://github.com/bitcoin/bitcoin/commit/32fc59796f74a2941772b5ec2755b1319132cd9c

achow101 commented at 10:39 pm on December 3, 2024: member

ACK 32fc59796f74a2941772b5ec2755b1319132cd9c

achow101 merged this on Dec 3, 2024

achow101 closed this on Dec 3, 2024

Package validation: accept packages of size 1 #31096

Code Coverage & Benchmarks

Reviews

Conflicts