New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor prep for package rbf #30072
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
suggested by @glozow , can close if this ends up not being helpful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ff7c71f. Code review + checked that the members get cleared properly in between subpackage evals.
ed7e6cc
to
6a39183
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK 6a39183
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reACK 6a39183
6a39183
to
186a00e
Compare
rebased due to merge conflict |
reACK 186a00e via range-diff |
re-ACK 186a00e |
186a00e
to
6fcd3cc
Compare
// because it's unnecessary. Also, CPFP carve out can increase the limit for individual | ||
// transactions, but this exemption is not extended to packages in CheckPackageLimits(). | ||
std::string err_string; | ||
// because it's unnecessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sr-gi I reverted the logic change here to reduce review surface, just shortening the comment here instead.
win64 test failure is minisketch https://github.com/bitcoin/bitcoin/pull/30072/checks?check_run_id=25181774728 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm ACK 6fcd3cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK 6fcd3cc
Out of curiosity, I verified that the extended carve-out functional test in 868c3e7 (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"
):
diff
diff --git a/test/functional/mempool_package_onemore.py b/test/functional/mempool_package_onemore.py
index 76e5ad2ff8..a6883f09fe 100755
--- a/test/functional/mempool_package_onemore.py
+++ b/test/functional/mempool_package_onemore.py
@@ -59,10 +59,10 @@ class MempoolPackagesTest(BitcoinTestFramework):
replaceable_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=[chain[0]])
txns = [replaceable_tx["tx"], self.wallet.create_self_transfer_multi(utxos_to_spend=replaceable_tx["new_utxos"])["tx"]]
txns_hex = [tx.serialize().hex() for tx in txns]
- assert_equal(self.nodes[0].testmempoolaccept(txns_hex)[0]["reject-reason"], "too-long-mempool-chain")
+ assert "package-mempool-limits" in self.nodes[0].testmempoolaccept(txns_hex)[0]["package-error"]
pkg_result = self.nodes[0].submitpackage(txns_hex)
- assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[0].getwtxid()]["error"]
- assert_equal(pkg_result["tx-results"][txns[1].getwtxid()]["error"], "bad-txns-inputs-missingorspent")
+ assert "error" not in pkg_result["tx-results"][txns[0].getwtxid()]
+ assert "too-long-mempool-chain" in pkg_result["tx-results"][txns[1].getwtxid()]["error"]
# But not if it chains directly off the first transaction
self.nodes[0].sendrawtransaction(replaceable_tx["hex"])
# and the second chain should work just fine
6fcd3cc
to
a84b574
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK a84b574 📦
// If we are using package feerates, we must be doing package submission. | ||
// It also means carveouts and sibling eviction are not permitted. | ||
if (m_package_feerates) { | ||
Assume(m_package_submission); | ||
Assume(!m_allow_carveouts); | ||
Assume(!m_allow_sibling_eviction); | ||
} | ||
if (m_allow_sibling_eviction) Assume(m_allow_replacement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of unrelated, and more like a "I wish my c++ skills were better" nit: I wonder if there is a way to do the introduced sanity checks already at compile-time, since these flags are all constant for each of the ATMPArgs
instances (probably not, or at least not without significant changes in structure...).
utACK a84b574 Left one nit/question for you, but looks good to me regardless. |
7689438
to
63cfa4c
Compare
ACK 63cfa4c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 63cfa4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 63cfa4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 63cfa4c
size_t m_conflicting_size{0}; | ||
}; | ||
|
||
struct SubPackageState m_subpackage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for struct
(seems like a C thing?)
struct SubPackageState m_subpackage; | |
SubPackageState m_subpackage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old habits!
@@ -1380,6 +1438,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: | |||
// replacements, we don't need to track the coins spent. Note that this logic will need to be | |||
// updated if package replace-by-fee is allowed in the future. | |||
assert(!args.m_allow_replacement); | |||
assert(!m_subpackage.m_rbf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this will need to be deleted immediately in the next PR?
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.
63cfa4c
to
2fd34ba
Compare
sadly, forced to rebased post #29873 Should be pretty trivial to review with something like: |
reACK 2fd34ba via range-diff |
/** When true, allow sibling eviction. This only occurs in single transaction package settings. */ | ||
const bool m_allow_sibling_eviction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"single transaction subpackage"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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 😅 . I agree "single transaction package" doesn't mean much to me in this context, perhaps "disallowed when evaluating a multi-transaction subpackage or package" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 2fd34ba
Sorry it took me a while to get familiar with the validation logic. I left some non-blocking comments.
@@ -476,6 +476,9 @@ class MemPoolAccept | |||
*/ | |||
const std::optional<CFeeRate> m_client_maxfeerate; | |||
|
|||
/** Whether CPFP carveout and RBF carveout are granted. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 90b60a1
nit (not-blocking):
/** Whether CPFP carveout and RBF carveout are granted. */ | |
/** Whether CPFP and RBF carveouts are granted. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will touch if I have to retouch the PR
@@ -948,6 +957,13 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) | |||
ws.m_ancestors = std::move(*ancestors); | |||
} else { | |||
// If CalculateMemPoolAncestors fails second time, we want the original error string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 90b60a1
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think it makes it worse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably from when 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.
src/validation.cpp
Outdated
std::string err_string; | ||
if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) { | ||
// Apply package mempool ancestor/descendant limits. | ||
if (!PackageMempoolChecks(txns, m_total_vsize, package_state)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 90b60a1
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted that change based on your feedback already :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, this was left on my first pass, which I never submitted 😆
// Single transaction contexts only. | ||
if (args.m_allow_sibling_eviction && err->second != nullptr) { | ||
// We should only be considering where replacement is considered valid as well. | ||
Assume(args.m_allow_replacement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In package RBF this changes: f93b602#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R528
We don't want sibling eviction attempts to happen during package rbf logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add that imo it's much clearer to have distinct flags each controlling small pieces of logic within validation (even if one flag could cover multiple things) while the dedicated ATMPArgs
constructors make decisions on what combinations of the flags are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 2fd34ba
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.