validation/refactor: refactoring for package submission #23381
pull glozow wants to merge 10 commits into bitcoin:master from glozow:2021-10-validation-refactors changing 6 files +285 −195-
glozow commented at 12:48 pm on October 28, 2021: memberThis contains the refactors and moves within #22674. There are no behavior changes, so it should be simpler to review.
-
glozow added the label Refactoring on Oct 28, 2021
-
in src/validation.cpp:1095 in f7d8be9b4b outdated
1091@@ -1019,9 +1092,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp 1092 EXCLUSIVE_LOCKS_REQUIRED(cs_main) 1093 { 1094 std::vector<COutPoint> coins_to_uncache; 1095- MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, 1096- test_accept, /* m_allow_bip125_replacement */ true }; 1097- 1098+ auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, nAcceptTime, bypass_limits, test_accept, coins_to_uncache);
jnewbery commented at 1:17 pm on October 28, 2021:No need to reordercoins_to_uncache, test_accept
totest_accept, coins_to_uncache
here. It makes your static ctors inconsistent in their argument ordering.
glozow commented at 3:15 pm on October 28, 2021:Donein src/validation.cpp:1005 in f7d8be9b4b outdated
1001 1002 if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(ws.m_state); 1003 1004+ if (m_rbf && !ReplacementChecks(ws)) return MempoolAcceptResult::Failure(ws.m_state); 1005+ 1006 // Only compute the precomputed transaction data if we need to verify
jnewbery commented at 1:23 pm on October 28, 2021:This comment is a little bit stranded now that thetxdata
variable has been removed. Perhaps just remove the first sentence?
glozow commented at 3:16 pm on October 28, 2021:Removed first sentencein src/validation.cpp:814 in f7d8be9b4b outdated
810@@ -760,60 +811,90 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) 811 // to be secure by simply only having two immediately-spendable 812 // outputs - one for each counterparty. For more info on the uses for 813 // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html 814- if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT || 815- !m_pool.CalculateMemPoolAncestors(*entry, setAncestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) { 816+ if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
jnewbery commented at 1:38 pm on October 28, 2021:0 if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
0 if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
glozow commented at 3:16 pm on October 28, 2021:Donein src/validation.cpp:529 in f7d8be9b4b outdated
524+ PrecomputedTransactionData m_precomputed_txdata; 525+ 526+ // Descendant limits passed in to CalculateMemPoolAncestors() for this transaction. 527+ // These may be modified while evaluating a transaction (eg to account for in-mempool 528+ // conflicts; see the comment in PreChecks). 529+ size_t m_tx_limit_descendants;
jnewbery commented at 1:48 pm on October 28, 2021:nit: thetx_
is a bit redundant here. This is a member of theWorkspace
object which is a collection of data relating to a single candidate transaction.
glozow commented at 3:16 pm on October 28, 2021:Hok, took out thetx_
in src/validation.cpp:844 in f7d8be9b4b outdated
864- "replacement-adds-unconfirmed", *err_string); 865- } 866+ const CTransaction& tx = *ws.m_ptx; 867+ const uint256& hash = ws.m_hash; 868+ TxValidationState& state = ws.m_state; 869+ const CTxMemPool::setEntries setIterConflicting = m_pool.GetIterSet(ws.m_conflicts);
jnewbery commented at 2:35 pm on October 28, 2021:Is there any performance penalty in doing this lookup twice? I suspect it’s not too bad since each of the lookups are into a boost::multi_index::hashed_unique map, which has an amortized complexity of O(1) (https://www.boost.org/doc/libs/1_56_0/libs/multi_index/doc/reference/hash_indices.html#lookup).
Even so, it seems a shame to have to query the mempool twice for the same data. Could this set be stored in the
Workspace
object to avoid the second lookup?
glozow commented at 3:17 pm on October 28, 2021:added am_iters_conflicting
to theworkspace
structjnewbery commented at 2:44 pm on October 28, 2021: memberI’d suggest removing the following sentences from the commit logs:
- “It also prevents a bug where we swap a package transaction for a same-txid-different-wtxid mempool transaction and put the wrong virtual size in the RPC result.”
- “Not doing this now would cause a bug where every package transaction could increase the descendant limit despite having unrelated conflicts, resulting in an inflated descendant limit.”
I don’t think writing about what bugs could be introduced in future is very informative. It’s enough to say that these values are specific to individual candidate transactions, so should be kept in the
Workspace
struct.In the scripted-diff: clean up MemPoolAccept aliases commit, there are various alias members that are not removed. Was that just because it was too difficult to target them with a scripted diff, or because you think those members are special in some way?
[validation] case-based constructors for ATMPArgs
No change in behavior. ATMPArgs can continue to have granular rules like switching BIP125 on/off while we create an interface for the different sets of rules for single transactions vs multiple-testmempoolaccept vs package validation. This is a cleaner interface than manually constructing the args, which makes it easy to mix up ordering, use the wrong default, etc. It also means we don't need to edit ATMP/single transaction validation code every time we update ATMPArgs for package validation.
[validation/refactor] store precomputed txdata in workspace
We want to be able to re-use the precomputed transaction data between PolicyScriptChecks and ConsensusScriptChecks in AcceptMultipleTransactions.
[validation] re-introduce bool for whether a transaction is RBF
This bool was originally part of Workspace and was removed in #22539 when it was no longer needed in Finalize(). Re-introducing it because, once again, multiple functions will need to know whether we're doing an RBF. Member of MemPoolAccept so that we can use this to inform package RBF in the future.
[validation/rpc] cache + use vsize calculated in PreChecks
This is not only cleaner but also helps make sure we are always using the virtual size measure that includes the sigop weight heuristic (which is the vsize the mempool would return).
glozow force-pushed on Oct 28, 2021in src/validation.cpp:502 in 4d262e060e outdated
500+ explicit Workspace(const CTransactionRef& ptx, size_t limit_descendants, size_t limit_descendant_size) : 501+ m_ptx(ptx), m_hash(ptx->GetHash()), 502+ m_limit_descendants{limit_descendants}, 503+ m_limit_descendant_size{limit_descendant_size} {} 504 std::set<uint256> m_conflicts; 505+ /** Iterators to mempool entries that this transaction directly conflicts with. */
jnewbery commented at 3:59 pm on October 28, 2021:nit: you could also comment the
m_all_conflicting
member below to emphasize that it contains the conflicts with all descendants. And perhaps rename the members tom_direct_conflicts
andm_all_conflicts
to highlight that difference.Also, the
m_conflicts
member above is only used inPreChecks()
, so could be a local variable in that function rather than a member ofWorkspace
.Perhaps it makes sense to clean all of that up in the [validation] cache iterators to mempool conflicts commit?
glozow commented at 4:33 pm on November 4, 2021:Added a commit to document theWorkspace
membersjnewbery commented at 3:59 pm on October 28, 2021: memberutACK 4d262e060e714042d747c4f0b2e0398234893d34
One suggestion inline.
jnewbery added this to the "Blockers" column in a project
DrahtBot commented at 6:37 pm on October 28, 2021: memberThe following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #23465 (Remove CChainParams and CTxMemPool params from ATMP by lsilva01)
- #23448 (refactor, consensus: remove calls to global
Params()
in validation layer by lsilva01) - #23437 (refactor, mempool: remove AcceptToMemoryPoolWithTime by lsilva01)
- #23121 ([policy] check ancestor feerate in RBF, remove BIP125 Rule2 by glozow)
- #22981 (doc: Fix incorrect C++ named args by MarcoFalke)
- #22674 (validation: mempool validation and submission for packages of 1 child + parents by glozow)
- #22097 (validation: Move package acceptance size limit from KvB to WU by ariard)
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.
in src/validation.cpp:882 in 4d262e060e outdated
912 return true; 913 } 914 915-bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata) 916+bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txns, 917+ std::vector<Workspace>& workspaces,
jnewbery commented at 11:01 am on October 29, 2021:
glozow commented at 4:34 pm on November 4, 2021:Removed from this PRin src/validation.cpp:532 in 4d262e060e outdated
527+ 528+ // Descendant limits passed in to CalculateMemPoolAncestors() for this transaction. 529+ // These may be modified while evaluating a transaction (eg to account for in-mempool 530+ // conflicts; see the comment in PreChecks). 531+ size_t m_limit_descendants; 532+ size_t m_limit_descendant_size;
ariard commented at 0:35 am on November 2, 2021:If you can backport the renaming to
m_tx_limit_descendants
from #22674, I think that’s good for review clarity.That said, I still don’t get why introducing a new pair of descendant limits at the
Workspace
-level, in addition of the one already present inMemPoolAccept
. AFAICT, the intermediate state, as modified in consequence to account for carve-out relaxation inPreChecks
isn’t re-used inside the other mempool checks helpers (PolicyScriptChecks
,Finalize
, …). Or is the rational about a future use, even ulterior to #22674 ?
glozow commented at 2:16 pm on November 2, 2021:You’re right that it’s not necessary yet in #22674 - I apologize, as the cut off for splitting it out of #22290 was a little bit arbitrary, so there are a few not-yet-relevant-until-later refactors in this PR.
To explain the problem: when we add package RBF, we don’t want to increase the
MemPoolAccept::m_limit_descendants
here: https://github.com/bitcoin/bitcoin/blob/04670ef81ea2300fcba4e1a492c4c6b0e0752848/src/validation.cpp#L765 For example, if we had a package of 5 parents and 1 child, and each of the parents had exactly 1 conflict, this would cause us to increase the descendant limit 5 times. That would be incorrectly inflating the limit.
glozow commented at 4:33 pm on November 4, 2021:Removed the commit from this PR - will add later when needed in package RBF
ariard commented at 1:06 am on November 9, 2021:To explain the problem: when we add package RBF, we don’t want to increase the > MemPoolAccept::m_limit_descendants here:
bitcoin/src/validation.cpp
Line 765 in 04670ef m_limit_descendants += 1;
For example, if we had a package of 5 parents and 1 child, and each of the parents had exactly 1 > conflict, this would cause us to increase the descendant limit 5 times. That would be incorrectly inflating the limit.
Thanks for the explanation, now I wonder if the carve-out relaxation makes really sense with package RBF. If your counterparty has broadcast a junk branch on the parent transaction and you’re RBF’ing one of your output with a package n>2, the one-more-carve-out-child relaxation won’t work, I think ?
Though I would say let’s defer the conversation for a future PR when package RBF is effectively introduced.
in src/validation.cpp:843 in 4d262e060e outdated
864- if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) { 865- return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, 866- "replacement-adds-unconfirmed", *err_string); 867- } 868+ const CTransaction& tx = *ws.m_ptx; 869+ const uint256& hash = ws.m_hash;
ariard commented at 0:43 am on November 2, 2021:Maybe good timing to renamem_hash
tom_txid_hash
or anything else to break ambiguity in prevision of wtxid acceptance.
glozow commented at 2:17 pm on November 2, 2021:Good point, though I would prefer to just removem_hash
fromWorkspace
entirely. The hashes are already cached in theCTransaction
object, so we can just callptx->GetHash()
andptx->GetWitnessHash()
directly.
ariard commented at 0:44 am on November 9, 2021:Yes that direction is even better!in src/validation.cpp:737 in 4d262e060e outdated
731@@ -680,9 +732,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) 732 733 int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS); 734 735- // nModifiedFees includes any fee deltas from PrioritiseTransaction 736- nModifiedFees = ws.m_base_fees; 737- m_pool.ApplyDelta(hash, nModifiedFees); 738+ // ws.m_modified_fees includes any fee deltas from PrioritiseTransaction
ariard commented at 0:51 am on November 2, 2021:nit: This comment could be replicated, at least in substance inm_modified_fees
member declaration inWorkspace
. Naming could be also clearer to indicate the source of the modification itself, likem_priority_fees
glozow commented at 4:35 pm on November 4, 2021:Added comment tom_modified_fees
👍in src/validation.cpp:547 in 4d262e060e outdated
537@@ -492,15 +538,23 @@ class MemPoolAccept 538 // only tests that are fast should be done here (to avoid CPU DoS). 539 bool PreChecks(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); 540 541+ // Run checks for mempool replace-by-fee. 542+ bool ReplacementChecks(Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs); 543+ 544+ // Enforce package mempool ancestor/descendant limits.
ariard commented at 1:00 am on November 2, 2021:Note the comment above
PreChecks
also mention evaluation package limits. I think both of statements are true, though they don’t refer to the same set of package limits. IIRC the first ones inPreChecks
are the in-mempool package limits while the second ones concern atomically accepted package, which is stricter under the union rule.I think we can improve documentation to establish clearly that we have two set of package limits to consider.
glozow commented at 6:56 pm on November 4, 2021:Edited the comment to clarify that there’s individual and package ancestor/desc limitsariard commented at 1:00 am on November 2, 2021: memberOverall good. See the comment around the newer descendant limits members inWorkspace
.glozow force-pushed on Nov 4, 2021[validation] cache iterators to mempool conflicts 3d3e4598b6document workspace members fd92b0c398glozow force-pushed on Nov 4, 2021scripted-diff: clean up MemPoolAccept aliases
The aliases are leftover from a previous MOVEONLY refactor - they are unnecessary and removing them reduces the diff for splitting out mempool Checks from PreChecks, making RBF variables MemPoolAccept-wide, etc. -BEGIN VERIFY SCRIPT- unalias() { sed -i "s:\<$1\>:$2:g" src/validation.cpp; sed -i "/$2 = $2/d" src/validation.cpp; } unalias nModifiedFees ws.m_modified_fees unalias nConflictingFees ws.m_conflicting_fees unalias nConflictingSize ws.m_conflicting_size unalias setConflicts ws.m_conflicts unalias allConflicting ws.m_all_conflicting unalias setAncestors ws.m_ancestors -END VERIFY SCRIPT-
MOVEONLY: mempool checks to their own functions
No change in behavior, because package transactions would not be going through the rbf logic in PreChecks anyway (BIP125 is currently disabled for package acceptance, see ATMPArgs). We draw the line here because each individual transaction in package validation still goes through all PreChecks. For example, checking that one's own conflicts and dependencies are disjoint (a consensus check) and individual transaction mempool ancestor/descendant limits.
MOVEONLY: move package unit tests to their own file 6876378365[test] call CheckPackage for package sanitization checks
Makes the test more minimal. We're just trying to test that our package sanitization logic is correct. Now that this code lives in its own function (rather than inside of AcceptMultipleTransactions), there's no need to call ProcessNewPackage to test this.
glozow force-pushed on Nov 4, 2021ariard commented at 1:55 am on November 9, 2021: memberCode Review ACK 14cd7bf
Changes since last review have been removing of
Workspace
-level descendant limits, introducing of fd92b0c39, stripping off thesetAncestors
alias, improvingPackageMempoolChecks
’s comment, drying upPackageMempoolChecks
from itsWorkspace
parameter.jnewbery commented at 11:44 am on November 9, 2021: memberCode review ACK 14cd7bf793547fa5143acece564482271f5c30bct-bast approvedt-bast commented at 2:32 pm on November 9, 2021: memberCode Review ACK https://github.com/bitcoin/bitcoin/pull/23381/commits/14cd7bf793547fa5143acece564482271f5c30bc
My ack shouldn’t weigh that much given my poor knowledge of the codebase, but the code does look cleaner and the added comments are helpful :+1:
laanwj commented at 3:46 pm on November 9, 2021: memberCode review ACK 14cd7bf793547fa5143acece564482271f5c30bc, thanks for adding documentation and clarifying the codelaanwj merged this on Nov 9, 2021laanwj closed this on Nov 9, 2021
sidhujag referenced this in commit 625cdea35d on Nov 9, 2021in src/validation.cpp:839 in c9b1439ca9 outdated
855- return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, 856- "replacement-adds-unconfirmed", *err_string); 857- } 858+bool MemPoolAccept::ReplacementChecks(Workspace& ws) 859+{ 860+ AssertLockHeld(cs_main);
MarcoFalke commented at 5:47 pm on November 9, 2021:Would it make sense to addAssume/Assert(ws.m_rbf);
?
glozow commented at 6:40 pm on November 9, 2021:Sure, there’s no harm in adding one. If safety is a concern, note that this is a no-op ifm_iters_conflicting
is empty:PaysMoreThanConflicts
,GetEntriesForConflicts
,HasNoNewUnconfirmed
, andPaysForRBF
should all simply returnstd::nullopt
if they’re called with empty or 0 inputs. So if we accidentally called this function withm_rbf
false or no conflicts, it would be some small wasted effort, but we should be okay.glozow deleted the branch on Nov 9, 2021laanwj removed this from the "Blockers" column in a project
Fabcien referenced this in commit 979f4e87a4 on Oct 21, 2022Fabcien referenced this in commit 7afb349331 on Oct 21, 2022Fabcien referenced this in commit 7597e8cfa3 on Oct 24, 2022Fabcien referenced this in commit bd6e032568 on Oct 24, 2022Fabcien referenced this in commit 9c01a0070a on Oct 24, 2022Fabcien referenced this in commit f269b4d06a on Oct 24, 2022Fabcien referenced this in commit 789c3ac241 on Oct 24, 2022Fabcien referenced this in commit 9f8c2b711e on Oct 24, 2022DrahtBot locked this on Nov 9, 2022
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 07:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me