validation/refactor: refactoring for package submission #23381

glozow · 2021-10-28T12:48:29Z

This contains the refactors and moves within #22674. There are no behavior changes, so it should be simpler to review.

jnewbery

I'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?

src/validation.cpp

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.

We want to be able to re-use the precomputed transaction data between PolicyScriptChecks and ConsensusScriptChecks in AcceptMultipleTransactions.

This bool was originally part of Workspace and was removed in bitcoin#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.

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 · 2021-10-28T15:17:33Z

Thanks for the review @jnewbery, took your suggestions

jnewbery

utACK 4d262e0

One suggestion inline.

src/validation.cpp

DrahtBot · 2021-10-28T18:37:46Z

The 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.

src/validation.cpp

ariard

Overall good. See the comment around the newer descendant limits members in Workspace.

src/validation.cpp

ariard · 2021-11-02T00:43:42Z

src/validation.cpp

-                                 "replacement-adds-unconfirmed", *err_string);
-        }
+    const CTransaction& tx = *ws.m_ptx;
+    const uint256& hash = ws.m_hash;


Maybe good timing to rename m_hash to m_txid_hash or anything else to break ambiguity in prevision of wtxid acceptance.

Good point, though I would prefer to just remove m_hash from Workspace entirely. The hashes are already cached in the CTransaction object, so we can just call ptx->GetHash() and ptx->GetWitnessHash() directly.

Yes that direction is even better!

src/validation.cpp

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-

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.

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 · 2021-11-05T19:50:15Z

Thanks for the reviews @ariard and @jnewbery, took suggestions

ariard · 2021-11-09T01:55:19Z

Code Review ACK 14cd7bf

Changes since last review have been removing of Workspace-level descendant limits, introducing of fd92b0c, stripping off the setAncestors alias, improving PackageMempoolChecks's comment, drying up PackageMempoolChecks from its Workspace parameter.

jnewbery · 2021-11-09T11:44:43Z

Code review ACK 14cd7bf

jnewbery · 2021-11-09T11:47:45Z

I think this might be RFM now since it's a simple refactor.

Perhaps @t-bast also wants to review this? It's basically the same commits from #22674 that you've already reviewed.

t-bast

Code Review ACK 14cd7bf

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 👍

laanwj · 2021-11-09T15:46:14Z

Code review ACK 14cd7bf, thanks for adding documentation and clarifying the code

…mission

maflcko · 2021-11-09T17:47:56Z

src/validation.cpp

-        }
+bool MemPoolAccept::ReplacementChecks(Workspace& ws)
+{
+    AssertLockHeld(cs_main);


Would it make sense to add Assume/Assert(ws.m_rbf);?

Sure, there's no harm in adding one. If safety is a concern, note that this is a no-op if m_iters_conflicting is empty:PaysMoreThanConflicts, GetEntriesForConflicts, HasNoNewUnconfirmed, and PaysForRBF should all simply return std::nullopt if they're called with empty or 0 inputs. So if we accidentally called this function with m_rbf false or no conflicts, it would be some small wasted effort, but we should be okay.

Summary: > 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. This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@0a79eab Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12346

Summary: > We want to be able to re-use the precomputed transaction data between > PolicyScriptChecks and ConsensusScriptChecks in > AcceptMultipleTransactions. For Bitcoin ABC, the `PolicyScriptChecks` code is part of `PreChecks`. D8203 duplicated the initialization of the `PrecomputedTransactionData`. This patch changes it to be computed only once, and only if the cheap pre-checks are successful. This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@cbb3598 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12347

Summary: > 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). This is a b)rtial ackport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@36a8441 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12351

Summary: This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@fd92b0c Test Plan: read new documentation Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12352

Summary: > 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. This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@9e910d8 Depends on D12352 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12359

Summary: > No change in behavior This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@c9b1439 Depends in D12359 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12360

Summary: I also removed all the includes that are now flagged as unused by my IDE in the source file. And a unit test was renamed during the move. Otherwise, there is no change in the code. This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@6876378 Depends on D12360 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12361

Summary: 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. This is a partial backport of [[bitcoin/bitcoin#23381 | core#23381]] bitcoin/bitcoin@14cd7bf Depends on D12361 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12362

glozow mentioned this pull request Oct 28, 2021

validation: mempool validation and submission for packages of 1 child + parents #22674

Merged

glozow added the Refactoring label Oct 28, 2021

jnewbery reviewed Oct 28, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

glozow added 4 commits October 28, 2021 15:57

[validation/refactor] store precomputed txdata in workspace

cbb3598

We want to be able to re-use the precomputed transaction data between PolicyScriptChecks and ConsensusScriptChecks in AcceptMultipleTransactions.

[validation/rpc] cache + use vsize calculated in PreChecks

36a8441

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 the 2021-10-validation-refactors branch from f7d8be9 to 4d262e0 Compare October 28, 2021 15:15

jnewbery reviewed Oct 28, 2021

View reviewed changes

src/validation.cpp Show resolved Hide resolved

This was referenced Oct 28, 2021

[policy] check ancestor feerate in RBF, remove BIP125 Rule2 #23121

Closed

doc: Fix incorrect C++ named args #22981

Merged

validation: Move package acceptance size limit from KvB to WU #22097

Closed

jnewbery reviewed Oct 29, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

ariard reviewed Nov 2, 2021

View reviewed changes

glozow force-pushed the 2021-10-validation-refactors branch from 4d262e0 to f185420 Compare November 4, 2021 16:32

glozow added 2 commits November 4, 2021 12:38

[validation] cache iterators to mempool conflicts

3d3e459

document workspace members

fd92b0c

glozow force-pushed the 2021-10-validation-refactors branch from f185420 to 7b8c8bf Compare November 4, 2021 16:41

glozow added 4 commits November 4, 2021 14:54

MOVEONLY: move package unit tests to their own file

6876378

glozow force-pushed the 2021-10-validation-refactors branch from 7b8c8bf to 14cd7bf Compare November 4, 2021 18:56

DrahtBot mentioned this pull request Nov 5, 2021

refactor: AcceptToMemoryPool #23437

Merged

This was referenced Nov 6, 2021

refactor, consensus: remove calls to global Params() in validation layer #23448

Closed

Remove CTxMemPool params from ATMP #23465

Merged

t-bast approved these changes Nov 9, 2021

View reviewed changes

laanwj merged commit e70fb87 into bitcoin:master Nov 9, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 9, 2021

Merge bitcoin#23381: validation/refactor: refactoring for package sub…

625cdea

…mission

maflcko reviewed Nov 9, 2021

View reviewed changes

glozow deleted the 2021-10-validation-refactors branch November 9, 2021 18:24

glozow mentioned this pull request Feb 11, 2022

Package Mempool Submission with Package Fee-Bumping #22290

Closed

10 tasks

bitcoin locked and limited conversation to collaborators Nov 9, 2022

validation/refactor: refactoring for package submission #23381

validation/refactor: refactoring for package submission #23381

Uh oh!

Conversation

glozow commented Oct 28, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented Oct 28, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ariard Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

ariard Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow commented Nov 5, 2021

Uh oh!

ariard commented Nov 9, 2021

Uh oh!

jnewbery commented Nov 9, 2021

Uh oh!

jnewbery commented Nov 9, 2021

Uh oh!

t-bast left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 9, 2021

Uh oh!

maflcko Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 28, 2021 •

edited

Loading

ariard Nov 9, 2021 •

edited

Loading

t-bast left a comment •

edited

Loading