Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor prep for package rbf #30072

Merged
merged 5 commits into from
May 24, 2024
Merged

refactor prep for package rbf #30072

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57ee302
Add description for m_test_accept
instagibbs May 20, 2024
69f7ab0
Add m_allow_sibling_eviction as separate ATMPArgs flag
instagibbs May 20, 2024
cbbfe71
cpfp carveout is excluded in packages
glozow Nov 3, 2022
20d8936
[refactor] make some members MemPoolAccept-wide
glozow Aug 3, 2021
2fd34ba
Add sanity checks for various ATMPArgs booleans
instagibbs May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,10 +457,12 @@ class MemPoolAccept
std::vector<COutPoint>& m_coins_to_uncache;
/** When true, the transaction or package will not be submitted to the mempool. */
const bool m_test_accept;
/** Whether we allow transactions to replace mempool transactions by BIP125 rules. If false,
/** Whether we allow transactions to replace mempool transactions. If false,
* any transaction spending the same inputs as a transaction in the mempool is considered
* a conflict. */
const bool m_allow_replacement;
/** When true, allow sibling eviction. This only occurs in single transaction package settings. */
const bool m_allow_sibling_eviction;
Comment on lines +464 to +465
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"single transaction subpackage"?

Copy link
Member

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

/** When true, the mempool will not be trimmed when any transactions are submitted in
* Finalize(). Instead, limits should be enforced at the end to ensure the package is not
* partially submitted.
Expand All @@ -486,6 +488,7 @@ class MemPoolAccept
/* m_coins_to_uncache */ coins_to_uncache,
/* m_test_accept */ test_accept,
/* m_allow_replacement */ true,
/* m_allow_sibling_eviction */ true,
/* m_package_submission */ false,
/* m_package_feerates */ false,
/* m_client_maxfeerate */ {}, // checked by caller
Expand All @@ -501,6 +504,7 @@ class MemPoolAccept
/* m_coins_to_uncache */ coins_to_uncache,
/* m_test_accept */ true,
/* m_allow_replacement */ false,
/* m_allow_sibling_eviction */ false,
/* m_package_submission */ false, // not submitting to mempool
/* m_package_feerates */ false,
/* m_client_maxfeerate */ {}, // checked by caller
Expand All @@ -516,6 +520,7 @@ class MemPoolAccept
/* m_coins_to_uncache */ coins_to_uncache,
/* m_test_accept */ false,
/* m_allow_replacement */ false,
/* m_allow_sibling_eviction */ false,
/* m_package_submission */ true,
/* m_package_feerates */ true,
/* m_client_maxfeerate */ client_maxfeerate,
Expand All @@ -530,6 +535,7 @@ class MemPoolAccept
/* m_coins_to_uncache */ package_args.m_coins_to_uncache,
/* m_test_accept */ package_args.m_test_accept,
/* m_allow_replacement */ true,
/* m_allow_sibling_eviction */ true,
/* m_package_submission */ true, // do not LimitMempoolSize in Finalize()
/* m_package_feerates */ false, // only 1 transaction
/* m_client_maxfeerate */ package_args.m_client_maxfeerate,
Expand All @@ -545,6 +551,7 @@ class MemPoolAccept
std::vector<COutPoint>& coins_to_uncache,
bool test_accept,
bool allow_replacement,
bool allow_sibling_eviction,
bool package_submission,
bool package_feerates,
std::optional<CFeeRate> client_maxfeerate)
Expand All @@ -554,6 +561,7 @@ class MemPoolAccept
m_coins_to_uncache{coins_to_uncache},
m_test_accept{test_accept},
m_allow_replacement{allow_replacement},
m_allow_sibling_eviction{allow_sibling_eviction},
m_package_submission{package_submission},
m_package_feerates{package_feerates},
m_client_maxfeerate{client_maxfeerate}
instagibbs marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -981,8 +989,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// check using the full ancestor set here because it's more convenient to use what we have
// already calculated.
if (const auto err{SingleV3Checks(ws.m_ptx, ws.m_ancestors, ws.m_conflicts, ws.m_vsize)}) {
// Disabled within package validation.
if (err->second != nullptr && args.m_allow_replacement) {
// 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);
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.


// Potential sibling eviction. Add the sibling to our list of mempool conflicts to be
// included in RBF checks.
ws.m_conflicts.insert(err->second->GetHash());
Expand Down