First few commits of #28984 to set the stage for the package RBF logic.
These refactors are preparation for evaluating an RBF in a multi-proposed-transaction context instead of only a single proposed transaction. Also, carveouts and sibling evictions only should work in single RBF cases so add logic to preclude multi-tx cases in the future.
No behavior changes aside from bailing earlier from failed carve-outs.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | glozow, sr-gi, theStack |
| Stale ACK | sdaftuar, ismaelsadeeq |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
710+ // AcceptSingleTransaction and AcceptMultipleTransactions invocations
711+
712+ /** Aggregated modified fees of all transactions, used to calculate package feerate. */
713+ CAmount m_total_modified_fees;
714+ /** Aggregated virtual size of all transactions, used to calculate package feerate. */
715+ int64_t m_total_vsize;
0 CAmount m_total_modified_fees{0};
1 /** Aggregated virtual size of all transactions, used to calculate package feerate. */
2 int64_t m_total_vsize{0};
1216@@ -1179,9 +1217,10 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
1217 entry->GetTxSize(),
1218 entry->GetFee()
1219 );
1220- ws.m_replaced_transactions.push_back(it->GetSharedTx());
1221+ m_replaced_transactions.push_back(it->GetSharedTx());
1222 }
1223- m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED);
1224+ m_pool.RemoveStaged(m_all_conflicts, false, MemPoolRemovalReason::REPLACED);
1225+ m_all_conflicts.clear();
Worth commenting that even though clearing happens in ClearSubPackageState, this is cleared so that we don’t process the vector over and over again for each tx in the package.
fwiw here’s a test for it, doing an RBF (but not package RBF) within package eval: https://github.com/glozow/bitcoin/blob/2024-05-review-30072/src/test/txpackage_tests.cpp#L940-L996
fwiw that test is a single tx rbf, so even without this line it should pass. With package RBF it might result in multiple RemoveStaged calls on same conflicts, though I’m unsure what exactly that would do.
edit: I suppose it would result in every tx in subpackage reporting the whole set of m_all_conflicts during package rbf multiple times
Finalize() is called in a subpackage, the child tx, it will try and enumerate the m_all_conflicts list again, but those transactions were already removed, so we would crash.
54@@ -55,14 +55,23 @@ def run_test(self):
55 assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0], second_chain])
56 # ...especially if its > 40k weight
57 assert_raises_rpc_error(-26, "too-long-mempool-chain, too many descendants", self.chain_tx, [chain[0]], num_outputs=350)
58+ # ...even if it's submitted with other transactions
Don’t think the other transactions help
0 # ...or if it's submitted with other transactions
In 90b60a16731ac756dce44c29f57d111f4dffba75
I see :%s/even/or was applied a few lines above, but this suggestion seems not to have been covered. Was it applied to the above line by mistake?
1216@@ -1179,9 +1217,12 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
1217 entry->GetTxSize(),
1218 entry->GetFee()
1219 );
1220- ws.m_replaced_transactions.push_back(it->GetSharedTx());
1221+ m_replaced_transactions.push_back(it->GetSharedTx());
1222 }
1223- m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED);
1224+ m_pool.RemoveStaged(m_all_conflicts, false, MemPoolRemovalReason::REPLACED);
1225+ // Don't attempt process same conflicts repeatedly during subpackage evaluation:
nitty nit (nonblocking)
0 // Don't attempt to process the same conflicts repeatedly during subpackage evaluation:
995@@ -996,7 +996,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
996 // already calculated.
997 if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
998 // Disabled within package validation.
999- if (err->second != nullptr && args.m_allow_replacement) {
1000+ if (err->second != nullptr && args.m_allow_replacement && !args.m_package_feerates) {
m_package_feerates is experiencing mission creep here. Really this is asking “was I called in AcceptSingleTransaction”?
Should we bikeshed this to make it clearer @glozow ?
Agree there’s room for improvement clarity-wise.
Perhaps a good approach is to have each flag control 1 part of the validation logic (even if 1 flag implies another flag)
LimitMempoolSize() within Finalize()CheckFeeRate should be skipped in PreChecksThe ATMPArgs constructors should decide the flags based on where they are created and we can add a sanity checking function within each ctor to assert that package RBF implies package feerates, package feerates implies package submission, etc.
m_allow_sibling_eviction ATMPArg and had it being true imply m_allow_replacements.
475@@ -476,6 +476,9 @@ class MemPoolAccept
476 */
477 const std::optional<CFeeRate> m_client_maxfeerate;
478
479+ /** Whether CPFP carveout and RBF carveout are granted. */
In 90b60a16731ac756dce44c29f57d111f4dffba75
nit (not-blocking):
0 /** Whether CPFP and RBF carveouts are granted. */
956@@ -948,6 +957,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
957 ws.m_ancestors = std::move(*ancestors);
958 } else {
959 // If CalculateMemPoolAncestors fails second time, we want the original error string.
In 90b60a16731ac756dce44c29f57d111f4dffba75
I think this comment is not accurate (it was not all that accurate before that PR either). At this point, CalculateMemPoolAncestors has only been called once. The only case in which this caching applies is in the last else case of this context. I think this should be reworded
My reading of it is if it fails below for a second time, we will want to have cached the first error string.
either way, this PR hopefully doesn’t make this confusion worse than status quo?
CalculateMemPoolAncestors took an in-out string param and we wanted to make a copy of the string before it got mutated. Probably best to just move the comment to the code where stuff is actually returned.
1437- // because it's unnecessary. Also, CPFP carve out can increase the limit for individual
1438- // transactions, but this exemption is not extended to packages in CheckPackageLimits().
1439- std::string err_string;
1440- if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) {
1441+ // Apply package mempool ancestor/descendant limits.
1442+ if (!PackageMempoolChecks(txns, m_total_vsize, package_state)) {
In 90b60a16731ac756dce44c29f57d111f4dffba75
If we are removing txns.size() > 1 because it is a truism, we should at least assume it to make sure we are not overlooking it
1445@@ -1431,9 +1446,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1446 }
1447
1448 // Apply package mempool ancestor/descendant limits. Skip if there is only one transaction,
1449- // because it's unnecessary. Also, CPFP carve out can increase the limit for individual
1450- // transactions, but this exemption is not extended to packages in CheckPackageLimits().
1451- std::string err_string;
1452+ // because it's unnecessary.
561@@ -555,6 +562,7 @@ class MemPoolAccept
562 m_coins_to_uncache{coins_to_uncache},
563 m_test_accept{test_accept},
564 m_allow_replacement{allow_replacement},
565+ m_allow_sibling_eviction{allow_sibling_eviction},
566 m_package_submission{package_submission},
567 m_package_feerates{package_feerates},
568 m_client_maxfeerate{client_maxfeerate}
07bb2736c17dd50b6ec3d4770001f8e6f0c8710a: would be good to put the sanity checks here
0 // If we are using package feerates, we must be doing package submission.
1 // It also means carveouts and sibling eviction are not permitted.
2 if (m_package_feerates) {
3 Assume(m_package_submission);
4 Assume(!m_allow_carveouts);
5 Assume(!m_allow_sibling_eviction);
6 }
7 if (m_allow_sibling_evction) Assume(m_allow_replacement);
455@@ -456,6 +456,7 @@ class MemPoolAccept
456 * the mempool.
457 */
458 std::vector<COutPoint>& m_coins_to_uncache;
459+ /** When true, the transaction or package will not be submited to the mempool. */
spelling nit:
0 /** When true, the transaction or package will not be submitted to the mempool. */
Code-review ACK 6fcd3cc016eaf07e47c5c448482ff844855a247b
Out of curiosity, I verified that the extended carve-out functional test in 868c3e77e01df8cfdc00a7599f16d0adf019aee7 (checking testmempoolaccept and submitpackage results) would also fail on master, but in a slightly different way, as the carve out leads to a failure later (in CheckPackageLimits() vs the earlier PreChecks() for testmempoolaccept; for submitpackage the first tx succeeds, and only the second fails with "too-long-mempool-chain"):
0diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py
1index 76e5ad2ff8..a6883f09fe 100755
2--- a/test/functional/mempool_package_onemore.py
3+++ b/test/functional/mempool_package_onemore.py
4@@ -59,10 +59,10 @@ class MempoolPackagesTest(BitcoinTestFramework):
5 replaceable_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]])
6 txns = [replaceable_tx["tx"], self.wallet.create_self_transfer_multi(utxos_to_spend=replaceable_tx["new_utxos"])["tx"]]
7 txns_hex = [tx.serialize().hex() for tx in txns]
8- assert_equal(self.nodes[0].testmempoolaccept(txns_hex)[0]["reject-reason"], "too-long-mempool-chain")
9+ assert "package-mempool-limits" in self.nodes[0].testmempoolaccept(txns_hex)[0]["package-error"]
10 pkg_result = self.nodes[0].submitpackage(txns_hex)
11- assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[0].getwtxid()]["error"]
12- assert_equal(pkg_result["tx-results"][txns[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent")
13+ assert "error" not in pkg_result["tx-results"][txns[0].getwtxid()]
14+ assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[1].getwtxid()]["error"]
15 # But not if it chains directly off the first transaction
16 self.nodes[0].sendrawtransaction(replaceable_tx["hex"])
17 # and the second chain should work just fine
581+ if (m_package_feerates) {
582+ Assume(m_package_submission);
583+ Assume(!m_allow_carveouts);
584+ Assume(!m_allow_sibling_eviction);
585+ }
586+ if (m_allow_sibling_eviction) Assume(m_allow_replacement);
ATMPArgs instances (probably not, or at least not without significant changes in structure…).
736+ CAmount m_conflicting_fees{0};
737+ /** Total size (in virtual bytes) of mempool transactions being replaced. */
738+ size_t m_conflicting_size{0};
739+
740+ /** Re-set sub-package state to not leak between evaluations */
741+ void ClearSubPackageState() EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs)
utACK a84b57462cd3dfcd059783696aacd796ef1793d4
Left one nit/question for you, but looks good to me regardless.
747+ CAmount m_conflicting_fees{0};
748+ /** Total size (in virtual bytes) of mempool transactions being replaced. */
749+ size_t m_conflicting_size{0};
750+ };
751+
752+ struct SubPackageState m_subpackage;
Is there a reason for struct (seems like a C thing?)
0 SubPackageState m_subpackage;
1437@@ -1380,6 +1438,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1438 // replacements, we don't need to track the coins spent. Note that this logic will need to be
1439 // updated if package replace-by-fee is allowed in the future.
1440 assert(!args.m_allow_replacement);
1441+ assert(!m_subpackage.m_rbf);
The behavior is not new, but this rule exits earlier than before.
Previously, a carve out could have been granted in PreChecks() but then
nullified in PackageMempoolChecks() when CheckPackageLimits() is called
with the default limits.
No change in behavior.
For single transaction acceptance, this is a simple refactor:
Workspace::m_all_conflicting
Workspace::m_conflicting_fees
Workspace::m_conflicting_size
Workspace::m_replaced_transactions
are now grouped under a new SubPackageState struct that is
a member of MemPoolAccept.
And local variables m_total_vsize and m_total_modified_fees are now
SubpackageState members so they can be accessed from
PackageMempoolChecks.
We want these to be package-wide variables because
- Transactions could conflict with the same tx (just not the same
prevout), or their conflicts could share descendants.
- We want to compare conflicts with the package fee rather than
individual transaction fee.
We reset these MemPoolAccept-wide fields for each subpackage
evaluation to not cause state leaking, similar to temporary
coins.
sadly, forced to rebased post #29873
Should be pretty trivial to review with something like:
git range-diff master 63cfa4c 2fd34ba504957331f5a08614b6e1f8317260f04d
461+ /** Whether we allow transactions to replace mempool transactions. If false,
462 * any transaction spending the same inputs as a transaction in the mempool is considered
463 * a conflict. */
464 const bool m_allow_replacement;
465+ /** When true, allow sibling eviction. This only occurs in single transaction package settings. */
466+ const bool m_allow_sibling_eviction;
I thought sibling eviction was relevant in a TRUC Transaction constellation when there already exist an unconfirmed parent and unconfirmed child, and now a newly submitted transaction is a child of the same parent, but spends a different output than the original child, i.e. not actually in conflict with the original child except for the topological restrictions affecting TRUC Transactions.
If I got all that right, I would posit that “This only occurs in single transaction package settings.” is a bit confusing, unless I’m the only person for whom it’s not obvious that “single transaction package settings” is exclusively describing the replacement transaction.
“single transaction package settings” is exclusively describing the replacement transaction.
The replacement transaction is always going to be the one we’re currently validating - maybe I’m misunderstanding what you’re saying :sweat_smile: . I agree “single transaction package” doesn’t mean much to me in this context, perhaps “disallowed when evaluating a multi-transaction subpackage or package” ?
988@@ -981,8 +989,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
989 // check using the full ancestor set here because it's more convenient to use what we have
990 // already calculated.
991 if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
992- // Disabled within package validation.
993- if (err->second != nullptr && args.m_allow_replacement) {
994+ // Single transaction contexts only.
995+ if (args.m_allow_sibling_eviction && err->second != nullptr) {
996+ // We should only be considering where replacement is considered valid as well.
997+ Assume(args.m_allow_replacement);
Is there any case in which m_allow_sibling_eviction is not shadowing m_allow_replacement? All the uses I see, they have the same value, or if one is set, the other is assumed.
Is that just the case now, and may it be different in a follow-up? Otherwise, what is the point of having two flags?
In package RBF this changes: https://github.com/bitcoin/bitcoin/pull/28984/commits/f93b602e74e101f4fed51f6d43b4930fcd7b0530#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R528
We don’t want sibling eviction attempts to happen during package rbf logic
ATMPArgs constructors make decisions on what combinations of the flags are allowed.