Part of #22290, aka Package Mempool Accept.
This enables CPFP fee bumping in child-with-unconfirmed-parents packages by introducing package feerate (total modified fees divided by total virtual size) and using it in place of individual feerate. We also always validate individual transactions first to avoid incentive-incompatible policies like “parents pay for children” or “siblings pay for siblings” behavior.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
80+
81+Transactions within a package are always validated individually first, and package validation is
82+used for the transactions that failed.
83+
84+*Rationale*: Packages are intended for incentive-compatible fee-bumping. That is, transaction A is a
85+"legitimate" fee-bump transaction B only if A is a descendant of B and has a higher feerate.
56@@ -57,3 +57,40 @@ test accepts):
57
58 - Warning: Batched fee-bumping may be unsafe for some use cases. Users and application developers
59 should take caution if utilizing multi-parent packages.
60+
61+### Package Fees and Feerate
62+
63+*Package Feerate* is the total modified fees divided by the total virtual size of all transactions
64+in the package after deduplication.
1221+ const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);
1222+ TxValidationState placeholder_state;
1223+ if (args.m_package_feerates &&
1224+ !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) {
1225+ package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low");
1226+ return PackageMempoolAcceptResult(package_state, package_feerate, {});
82+used for the transactions that failed.
83+
84+*Rationale*: Packages are intended for incentive-compatible fee-bumping. That is, transaction A is a
85+"legitimate" fee-bump transaction B only if A is a descendant of B and has a higher feerate.
86+Package feerate, as defined above, does not consider the fees and relationships between all
87+transactions in the package. We want to prevent "parents pay for children" behavior; fees of
85+"legitimate" fee-bump transaction B only if A is a descendant of B and has a higher feerate.
86+Package feerate, as defined above, does not consider the fees and relationships between all
87+transactions in the package. We want to prevent "parents pay for children" behavior; fees of
88+children should have no impact on the consideration of their parents, since they are not necessary
89+for the parents to be mined. More generally, if transaction B is not needed in order for transaction
90+A to be mined, B's fees cannot help A. In a child-with-parents package, simply excluding any parent
655@@ -649,6 +656,10 @@ class MemPoolAccept
656
657 CChainState& m_active_chainstate;
658
659+ // Used to calculate package feerate.
660+ CAmount m_total_modified_fees;
m_total_modified_fees and m_total_vsize just be local variables? They seem to be used only in MemPoolAccept::AcceptMultipleTransactions.
PackageRBFChecks() in the future, but we’ll cross that bridge when we get there.
Concept ACK
left a few comments below
56@@ -57,3 +57,40 @@ test accepts):
57
58 - Warning: Batched fee-bumping may be unsafe for some use cases. Users and application developers
59 should take caution if utilizing multi-parent packages.
60+
61+### Package Fees and Feerate
62+
63+*Package Feerate* is the total modified fees divided by the total virtual size of all transactions
prioritisetransaction RPC does the modifying); perhaps “modified” can be dropped? I think it’s implicit. Or maybe briefly mention the RPC here.
705+ strprintf("Expected package feerate %s, got %s", expected_feerate.ToString(),
706+ submit_prioritised_package.m_package_feerate.value().ToString()));
707+ }
708+
709+ // Package feerate is calculated without topology in mind; it's just aggregating fees and sizes.
710+ // However, this should not allow parents to pay for children. Each transaction should be
“However, this should not allow parents to pay for children”
This reads to me like “parents must not pay for children”. When it is more (to my understanding), parents must be validated first before considering children. If the parent(s) is accepted into the mempool first a low fee child can later join them as part of a sufficient fee package but a low fee child must not prevent a sufficient fee parent from being accepted into the mempool. That’s right yeah?
Our goals are:
(1) If the child’s feerate is too low to be accepted, the parent’s fees should not be able to pay for it. A parent should not pay for a child, because the child is not necessary for the parent to be mined.
(2) If the child’s feerate is too low to be accepted, that should not prevent the parent from being accepted. A child’s fees should not harm its parent, because the child is not necessary for the parent to be mined.
Validating the parents individually first is how we achieve these goals. It is not the goal itself.
This reads to me like “parents must not pay for children”. When it is more (to my understanding), parents must be validated first before considering children.
This is a reversal of the goal/implementation relationship. We install seatbelts in the car (implementation) so that drivers are protected in accidents (goal). The goal is not to install seatbelts for no reason, it’s to protect drivers.
Concept ACK
Built on MacOS Big Sur, ran unit tests (diagrams from PR review club are helpful here illustrating each unit test). Ideally I’d play around with these but just read through and ran them today. Didn’t run the fuzz tests but the fuzzer seems to be failing on the CI.
I asked about DoS vectors for CPFP in today’s PR review club as that seems the greatest hurdle to get over for updated RBF rules and package RBF but seem to be less of a concern with CPFP (?). Not sure whether the CPFP carve out rule needs to be tweaked when CPFPing within a package?
Great to see this series of PRs progress.
88+children should have no impact on the consideration of their parents, since they are not necessary
89+for the parents to be mined. More generally, if transaction B is not needed in order for transaction
90+A to be mined, B's fees cannot help A. In a child-with-parents package, simply excluding any parent
91+transactions that meet feerate requirements individually is sufficient to ensure this.
92+
93+*Rationale*: For backwards-compatibility of transaction relay policy, it's important that package
i agree as a design principle that backwards-compatibility of transaction relay policy is worthwhile.
That said, we might have in the future to make exceptions to that principle, where a transaction/package might be relayed across non-upgraded nodes though rejected by upgraded nodes. E.g I’m thinking if replace-by-feerate is deployed instead of replace-by-fee, you might have a (2000 sats; 10 sat/vB) transaction replacing a (5000 sats; 5 sat/vB) already-in-mempool one (even if we can envision some transition period with dual-support).
Thus I’m suggesting to enlighten the wording here, “As a principle, backwards-compatibility of transaction relay policy is prioritized”, “we prevent non-programmed restriction of policy”. Especially as all those questions of policy loosening/tightening and how the project operates them are relatively young, I think it’s cautious to be more formal in the phrasing. Thereby avoiding in the future ecosystems users to claim an adamant stability of Core policy rules, like in the zero-conf/full-rbf past polemics.
688+ submit_package_too_low.m_package_feerate.value().ToString()));
689+ }
690+
691+ // Package feerate includes the modified fees of the transactions.
692+ // This means a child with its fee delta from prioritisetransaction can pay for a parent.
693+ m_node.mempool->PrioritiseTransaction(tx_child_cheap->GetHash(), 1 * COIN);
prioritisetransaction documentation, I think it’s a valid behavior to subtract fees from a transaction, thus altering ProcessNewPackage result. If you think it’s a case worthy to cover.
111+to be mined. More generally, if transaction B is not needed in order for transaction A to be mined,
112+B's fees cannot harm A. In a child-with-parents package, simply validating parents individually
113+first is sufficient to ensure this.
114+
115+*Rationale*: As a principle, we want to avoid accidentally restricting policy in order to be
116+backwards-compatibile for users and applications that rely on p2p transaction relay. Concretely,
1375@@ -1338,18 +1376,33 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
1376 results.emplace(wtxid, MempoolAcceptResult::MempoolTxDifferentWitness(iter.value()->GetTx().GetWitnessHash()));
1377 } else {
1378 // Transaction does not already exist in the mempool.
1379- txns_new.push_back(tx);
1380+ // Try submitting the transaction on its own.
1381+ const auto single_res = AcceptSingleTransaction(tx, single_args);
I think the TxValidationState could be inspected here. If the failure cannot be assigned to the feerate check, the package validation could fail early ? Otherwise if you have a package of 25th component and the latest is faultive, we might validate twice the first 24 ones. Once here, the other one in AcceptMultipleTransactions and this could be avoid.
Am I correct ?
If yes, I think it’s always good to minimize our DoS surface. Though I’m unsure if the difference would be significant between the two implementations.
1390 }
1391 }
1392
1393 // Nothing to do if the entire package has already been submitted.
1394- if (txns_new.empty()) return PackageMempoolAcceptResult(package_state, std::move(results));
1395+ // Also quit now if any of the transactions was consensus-invalid because package validation
txns_new is empty only if all submitted package transactions are accepted. Not because transactions are consensus-invalid, I’m unsure if the comment is accurate with the latest version of the code ?
Code Review ACK a7017507
Modifications since last review are typying arguments in AcceptMultipleTransactions and quit_early new a7017507 commit.
76+### Package Fees and Feerate
77+
78+*Package Feerate* is the total modified fees (base fees + any fee delta from
79+`prioritisetransaction`) divided by the total virtual size of all transactions in the package.
80+If any transactions in the package are already in the mempool, they are not submitted again
81+("deduplicated"), and thus excluded from this calculation.
drop the comma, as the dependent clause has no subject
0("deduplicated") and are thus excluded from this calculation.
79+`prioritisetransaction`) divided by the total virtual size of all transactions in the package.
80+If any transactions in the package are already in the mempool, they are not submitted again
81+("deduplicated"), and thus excluded from this calculation.
82+
83+To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate
84+(`minRelayTxFee`) and dynamic mempool minimum feerate, the total package feerate is used instead of
0(`minRelayTxFee`) and the dynamic mempool minimum feerate, the total package feerate is used instead of
80+If any transactions in the package are already in the mempool, they are not submitted again
81+("deduplicated"), and thus excluded from this calculation.
82+
83+To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate
84+(`minRelayTxFee`) and dynamic mempool minimum feerate, the total package feerate is used instead of
85+the individual feerate. The individual transactions are allowed to be below feerate requirements if
0the individual feerate. The individual transactions are allowed to be below the feerate requirements if
81+("deduplicated"), and thus excluded from this calculation.
82+
83+To meet the two feerate requirements of a mempool, i.e., the pre-configured minimum relay feerate
84+(`minRelayTxFee`) and dynamic mempool minimum feerate, the total package feerate is used instead of
85+the individual feerate. The individual transactions are allowed to be below feerate requirements if
86+the package meets the feerate requirements. For example, the parent(s) in the package can have 0
“Two” is written out in the same paragraph (instead of 2), so s/0/zero/.
0the package meets the feerate requirements. For example, the parent(s) in the package can have zero
85+the individual feerate. The individual transactions are allowed to be below feerate requirements if
86+the package meets the feerate requirements. For example, the parent(s) in the package can have 0
87+fees but be paid for by the child.
88+
89+*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not
90+meeting minimum fees on its own. This allows L2 applications to adjust their fees at broadcast time
L2 (layer two) isn’t previously defined in this document, so perhaps something like
0meeting minimum fees on its own. This would allow applications building on Bitcoin Core to adjust their fees at broadcast time
Also, both sentences begin with “This…”; it may be good to replace the first one with “The package feerate can be thought of…”
86+the package meets the feerate requirements. For example, the parent(s) in the package can have 0
87+fees but be paid for by the child.
88+
89+*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a parent not
90+meeting minimum fees on its own. This allows L2 applications to adjust their fees at broadcast time
91+instead of overshooting or risking getting stuck/pinned.
112+B's fees cannot harm A. In a child-with-parents package, simply validating parents individually
113+first is sufficient to ensure this.
114+
115+*Rationale*: As a principle, we want to avoid accidentally restricting policy in order to be
116+backwards-compatibile for users and applications that rely on p2p transaction relay. Concretely,
117+it's important that package validation does not prevent the acceptance of a transaction that would
599+
600+BOOST_FIXTURE_TEST_CASE(package_cpfp_tests, TestChain100Setup)
601+{
602+ mineBlocks(5);
603+ LOCK(cs_main);
604+ unsigned int expected_pool_size = m_node.mempool->size();
22bf054
size_t type for size() (or auto), same for line 2160- LOCK(cs_main);
1- unsigned int expected_pool_size = m_node.mempool->size();
2+ LOCK(::cs_main);
3+ size_t expected_pool_size{m_node.mempool->size()};
586@@ -568,6 +587,205 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
587 BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_parent2_v1->GetWitnessHash())));
588 BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent3->GetHash())));
589 BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_mixed_child->GetHash())));
590+
591+ // package feerate should include parent3 and child. It should not include parent1 or parent2_v1.
592+ BOOST_CHECK(mixed_result.m_package_feerate != std::nullopt);
593+ CFeeRate expected_feerate(1 * COIN, GetVirtualTransactionSize(*ptx_parent3) + GetVirtualTransactionSize(*ptx_mixed_child));
22bf054eee6bfc7148bb40150d648b0dcf0db44b nit, this is the only non-const expected_feerate in the test and it can be const too
0 const CFeeRate expected_feerate(1 * COIN, GetVirtualTransactionSize(*ptx_parent3) + GetVirtualTransactionSize(*ptx_mixed_child));
91+instead of overshooting or risking getting stuck/pinned.
92+
93+*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as
94+we do not want a transaction's fees to be double-counted.
95+
96+Implementation Note: Transactions within a package are always validated individually first, and
0Implementation Note: Transactions within a package are always feerate-validated individually first, and
“feerate-validated” may prevent readers from thinking you’re referring to transaction consensus validation.
92+
93+*Rationale*: It would be incorrect to use the fees of transactions that are already in the mempool, as
94+we do not want a transaction's fees to be double-counted.
95+
96+Implementation Note: Transactions within a package are always validated individually first, and
97+package validation is used for the transactions that failed. Since package feerate is only
“… used for the transactions that failed.” This is a bit vague for me, and maybe that’s okay since this is a high-level document. But let me try to describe what this actually means at a high level (without looking at the code), Pythonically:
0# Create a new package (t-sorted list of transactions) from the incoming package
1new_package = []
2for tx in package:
3 if has_parent(new_package, tx) or not feerate_okay(tx):
4 new_package.append(tx)
5fee_validate(new_package)
Consider the most trivial (and useful) case, child pays for parent. Here, parent A has a low feerate, and its child B has a high feerate, package is [A, B]. The first loop iteration looks at A, and asks: does A have a parent (within new_package)? No – new_package is empty. So then we check A’s feerate. It’s not okay, so we add A to new_package. The second loop iteration considers B. Does it have a parent in new_package? Yes, so add it to new_package (no need to check its feerate). So new_package ends up [A, B], so we evaluate that and it likely passes.
Let’s consider the reverse case, parent pays for child. Here, parent A has a high feerate, and B has a low feerate. Again, package is [A, B]. The first loop iteration finds that A does not have a parent (in new_package), so we check A’s feerate. It is okay, so we don’t add A to the result list. The second loop checks B; it does not have a parent in new_package, so we check its feerate. It’s not okay, so we add it to new_package, which becomes [B]. We check this package feerate and it fails. So parent A was not able to pay for child B, which is what we want.
Note that any package transactions that are not in new_package (in this second example, A) have passed their individual feerate test, so there is no need to check them again.
I think this algorithm generalizes to an arbitrary package DAG but I’m not completely sure.
Your description is correct at a high level, and is what I intend to communicate to the reader. To be nitpicky, we’ll add transactions to the package for any policy reason, not just feerate, since TxValidationResult doesn’t specify whether it’s a fee-related or other policy issue.
I think this algorithm generalizes to an arbitrary package DAG but I’m not completely sure.
While I wish this were true, there are cases where it doesn’t work - see example Q2 in the package mempool accept gist. This is why the package-child-with-unconfirmed-parents restriction is so important.
One thing I had to think really hard about (maybe it’s just me) is the effect of deduplication. I pictured a package like A <– B <– C (A is B’s parent, B is C’s parent). What happens if (only) B is already in the mempool, so we deduplicate it out of the package? Then the package would consist of A <– ??? <– C, which is no longer a valid package.
I think the answer to this is that if B is already in the mempool, then A must also already be in the mempool, so it will be deduplicated out of the package too. (This wasn’t obvious to me at first.) Or I guess A could be mined, so the package would end up being just C, which is not actually a package (which is okay, evaluate C on its own). In any event, deduplication only removes from the “left” (if you view the transactions payments as flowing left to right as I’ve done here) – it can’t remove from the middle of this big DAG (well, it can’t be a big DAG yet, but will in the future).
One other comment, and again, it may just be me, but it may not be obvious to the uninitiated why package relay is needed at all. Taking the most basic single parent and single child case, if the parent’s fee is so low that it drops out of the mempool (or at least many nodes’ mempools), then why not just reconstitute the parent with a higher fee and broadcast this new transaction? Why is a child even needed?
I think the answer (and correct me if I’m wrong) is that the parent transaction’s broadcaster may not be able to create a new version of the parent transaction. It may have been signed by other parties, and they’re not around (or aren’t willing) to sign a replacement transaction. In L2 applications especially, this transaction may have been formed months ago.
I’m not sure if I’m suggesting any changes to this document, but you may want to consider trying to get this concept across (if my understanding is correct).
Thanks @LarryRuane for the thoughtful review.
One thing I had to think really hard about (maybe it’s just me) is the effect of deduplication. I pictured a package like A <– B <– C (A is B’s parent, B is C’s parent)…
Note that A<-B<-C is already not a valid package with these rules; it would fail IsChildWithParents. I’ll continue with this example considering just A<-B.
You are correct that if B is already in the mempool, A (and any other ancestors or transactions to the “left”) must either be in the mempool or already confirmed. Unfortunately, we cannot query with absolute certainty if a transaction has already confirmed, which is why the rule is it must be a child with all unconfirmed parents. The only possibilities for B already being in the mempool are:
package-not-child-with-unconfirmed-parents because A’s inputs were not available. This is correct, since a confirmed parent of B was included in the package.why not just reconstitute the parent with a higher fee and broadcast this new transaction? Why is a child even needed? I think the answer (and correct me if I’m wrong) is that the parent transaction’s broadcaster may not be able to…
You are correct.
1418 auto submission_result = AcceptMultipleTransactions(txns_new, args);
1419 // Include already-in-mempool transaction results in the final result.
1420 for (const auto& [wtxid, mempoolaccept_res] : results) {
1421 submission_result.m_tx_results.emplace(wtxid, mempoolaccept_res);
1422 }
1423+ if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate != std::nullopt);
nit, preferable?
0if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value());
has_value()
97@@ -98,7 +98,9 @@ BOOST_FIXTURE_TEST_CASE(package_validation_tests, TestChain100Setup)
98 BOOST_CHECK(it_child != result_parent_child.m_tx_results.end());
99 BOOST_CHECK_MESSAGE(it_child->second.m_state.IsValid(),
100 "Package validation unexpectedly failed: " << it_child->second.m_state.GetRejectReason());
101-
102+ BOOST_CHECK(result_parent_child.m_package_feerate != std::nullopt);
22bf054eee6bfc7148bb40150d648b0dcf0db44b nit, and similar elsewhere
0 BOOST_CHECK(result_parent_child.m_package_feerate.has_value());
607+ CScript parent_spk = GetScriptForDestination(WitnessV0KeyHash(child_key.GetPubKey()));
608+ CKey grandchild_key;
609+ grandchild_key.MakeNewKey(true);
610+ CScript child_spk = GetScriptForDestination(WitnessV0KeyHash(grandchild_key.GetPubKey()));
611+
612+ // zero-fee parent and child package
0 // zero-fee parent and fee=1 child package
71@@ -72,3 +72,48 @@ test accepts):
72 a competing package or transaction with a mutated witness, even though the two
73 same-txid-different-witness transactions are conflicting and cannot replace each other, the
74 honest package should still be considered for acceptance.
75+
76+### Package Fees and Feerate
77+
78+*Package Feerate* is the total modified fees (base fees + any fee delta from
79+`prioritisetransaction`) divided by the total virtual size of all transactions in the package.
0[`prioritisetransaction`](https://developer.bitcoin.org/reference/rpc/prioritisetransaction.html)) divided by the total virtual size of all transactions in the package.
410@@ -399,6 +411,9 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
411 BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_parent->GetHash())));
412 BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child1->GetHash())));
413
414+ // Child2 would have been validated individually.
415+ BOOST_CHECK(submit_witness1.m_package_feerate == std::nullopt);
416+
417 const auto submit_witness2 = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
418 {ptx_parent, ptx_child2}, /*test_accept=*/ false);
Should we check the feerate for this package? I think this is the only one missing in this file.
0BOOST_CHECK(submit_witness2.m_package_feerate == std::nullopt);
1253@@ -1231,20 +1254,35 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1254 m_viewmempool.PackageAddTransaction(ws.m_ptx);
1255 }
1256
1257+ // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee.
1258+ // For transactions consisting of exactly one child and its parents, it suffices to use the
1259+ // package feerate (total modified fees / total virtual size) to check this requirement.
1260+ const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0},
1261+ [](int64_t sum, auto& ws) { return sum + ws.m_vsize; });
nit, maybe just (I’m more old-school) … but it does lose the const, and I don’t know if our style guide allows one-line loops (but it should!)
0 int64_t m_total_vsize{0};
1 for (const auto& ws : workspaces) m_total_vsize += ws.m_vsize;
std::accumulate.
Code review, tested ACK dc0b2d3fa375ec0641bd2075a99be0383d1129ee
Suggestions are optional.
Regarding your reply to my earlier comment:
there are cases where it [my little strawman algorithm] doesn’t work [for the general case]
Thanks, you’re right, I see now that this trivial algorithm is definitely wrong. This made me think about the general case more carefully, and I ended up writing a little Python demo to help with my understanding. This in no way affects the current PR (which is excellent as-is), but anyone interested in this topic may find this useful, and it may be helpful in writing a future PR if we want to generalize the allowed package forms:
Code Review ACK a7017507
Modifications since last review are extending ATMPArgs private ctor extension with m_package_feerates and changes in the unit tests.
Out of scope for now but dropped a message on the gist itself https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a
I see no reason for this(possibly wrongly perceived) shortfall to stop incremental improvements
Out of scope for now but dropped a message on the gist itself
Will respond on the gist. I don’t think it’s relevant to packages actually.
Rebased and addressed @jonatack and @LarryRuane’s comments
Thanks, suggestion for future, make the rebase and address-review-comments two separate force-pushes for easier review.
code review ACK 157a7bb547abc90a8bbb595cdee8eaac9c3300c9
docs are clear re:design, tests look fairly comprehensive, nice step forward
507@@ -508,6 +508,19 @@ class MemPoolAccept
508 };
509 }
510
511+ /** Parameters for a single transaction within a package. */
512+ static ATMPArgs SingleInPackageAccept(const ATMPArgs& package_args) {
As fas as I can see, the main difference to the SingleAccept parameters is that m_allow_bip125_replacement is false here, because it is taken from the PackageTestAccept or PackageChildWithParents parameters.
If a transaction is no longer subject to package validation because it is valid on its own, is it still necessary to disallow RBF? This seems to slightly conflict with the philosophy that “package validation should not prevent the acceptance of a transaction that would otherwise be policy-valid on its own”.
SingleInPackageAccept
SingleInPackageAccept function at all, instead of just creating a SingleAccept() ATMPArgs set?
SingleInPackageAccept should never have bypass_limits=true, so I don’t want the caller to have the option to set it.
1222+ [](int64_t sum, auto& ws) { return sum + ws.m_vsize; });
1223+ const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0},
1224+ [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; });
1225+ const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize);
1226+ TxValidationState placeholder_state;
1227+ if (args.m_package_feerates &&
AcceptMultipleTransactions() and get to this point without having m_package_feerates set (at least no test breaks if I add an assert for it) - is there a future use case for that or should there be an assert or Assume?
Right, we always have m_package_feerates on when we call AcceptMultipleTransactions. I imagined turning it off for testmempoolaccept with multiple transactions ~but didn’t actually come up with a good reason to do that. I think i will delete this condition from here~
EDIT: I now remember why I wanted to turn off package feerates for testmempoolaccept; it’s because those are not restricted by child-with-parents. Sorry for the confusion. Yes, this should be here; the correct fix was to set m_package_feerates=false for ATMPArgs::PackageTestAccept.
ATMPArgs::PackageTestAccept.m_package_feerates=true in the original code
Code Review ACK https://github.com/bitcoin/bitcoin/commit/a70175076a5f85391a089607f6a19a58f173d4a1 @ariard seems you ACKed an old commit? A rebase + small changes since then, should be pretty easy to review
git range-diff a701750...157a7bb
Reviewed again and it looks good to me, just two questions before I ACK:
Could this introduce some weird behavior for the testmempoolaccept RPC? That RPC bypasses AcceptPackage() and calls AcceptMultipleTransactions() directly, which now has logic to calculate a package feerate (and possibly return a related error) which only makes sense for the special case of a “exactly one child and its parents” package that AcceptPackage() ensures.
Another question about RBF below.
This allows CPFP within a package prior to submission to mempool.
This avoids "parents pay for children" and "siblings pay for siblings"
behavior, since package feerate is calculated with totals and is
topology-unaware.
It also ensures that package validation never causes us to reject a
transaction that we would have otherwise accepted in single-tx
validation.
Package validation policy only differs from individual policy in its
evaluation of feerate. Minimize DoS surface; don't validate all over
again if we know the result will be the same.
In response to @mzumsande’s (very good) questions, I have made these 2 changes:
ATMPArgs::PackageTestAccept.m_package_feerates=false because there is no child-with-parents restriction (it accepts completely independent transactions), which means package feerate isn’t very meaningful and we shouldn’t use it in CheckFeeRate().
ATMPArgs::SingleInPackageAccept.m_allow_bip125_replacement=true because RBF should be allowed for the individual transactions.
Also addressed @LarryRuane’s #24152 (review) which I forgot to address last time.
changes are the aforementioned ATMPArgs::PackageTestAccept.m_package_feerates=false
, ATMPArgs::SingleInPackageAccept.m_allow_bip125_replacement=true, and the has_value change.
reACK https://github.com/bitcoin/bitcoin/pull/24152/commits/9bebf35e269b2a918df27708565ecd0c5bd3f116
Post-Merge Review ACK 9bebf35e
Changes since last ACK are turning m_packages_feerates to false for PackageTestAccept and authorizing bip 125 replacement for SingleInPackageAccept.
643+ BOOST_CHECK(submit_cpfp_deprio.m_tx_results.empty());
644+ BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size);
645+ const CFeeRate expected_feerate(0, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child));
646+ BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.has_value());
647+ BOOST_CHECK(submit_cpfp_deprio.m_package_feerate.value() == CFeeRate{0});
648+ BOOST_CHECK_MESSAGE(submit_cpfp_deprio.m_package_feerate.value() == expected_feerate,
expected_feerate is already instantiated to 0
1214@@ -1205,20 +1215,35 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1215 m_viewmempool.PackageAddTransaction(ws.m_ptx);
1216 }
1217
1218+ // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee.
1219+ // For transactions consisting of exactly one child and its parents, it suffices to use the
1220+ // package feerate (total modified fees / total virtual size) to check this requirement.
1221+ const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0},
This assumes that AcceptMultipleTransactions always takes proper packages and that we can safely consider the aggregated feerate of the bunch of transactions. It’s not the case.
EDIT: i was reviewing an older version of this branch which was missing a fix…. (https://github.com/bitcoin/bitcoin/pull/24152#pullrequestreview-989689132)
This patch demonstrates how this makes testmempoolaccept accept invalid transactions (by considering the package feerate for unrelated transactions):
0diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
1index d3961e753d..f8dd380ee5 100755
2--- a/test/functional/mempool_accept.py
3+++ b/test/functional/mempool_accept.py
4@@ -333,6 +333,15 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
5 maxfeerate=0,
6 )
7
8+ self.log.info("Don't consider package fees between unrelated transactions")
9+ txs = [
10+ self.wallet.create_self_transfer(fee_rate=0, mempool_valid=False)['tx'],
11+ self.wallet.create_self_transfer()['tx'],
12+ ]
13+ res = self.nodes[0].testmempoolaccept([tx.serialize().hex() for tx in txs])
14+ # The first tx has a feerate of 0. No child pay for it. It should be rejected.
15+ assert not res[0]["allowed"]
16+
17
18 if __name__ == '__main__':
19 MempoolAcceptanceTest().main()
I think the package feerate check shouldn’t be in AcceptMultipleTransactions, since it doesn’t necessarily take a child-with-parents package.
(sorry, i know i’m late to the party)
EDIT: aarg. I was reviewing an older branch which didn’t have the fix for m_package_feerates in PackageTestAccept (https://github.com/bitcoin/bitcoin/pull/24152#discussion_r843317502)… Sorry for the noise.