validation/refactor: refactoring for package submission

glozow commented at 12:48 pm on October 28, 2021: member

This 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 reorder coins_to_uncache, test_accept to test_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:

Done

in 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 the txdata variable has been removed. Perhaps just remove the first sentence?

glozow commented at 3:16 pm on October 28, 2021:

Removed first sentence

in 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:

Done

in 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: the tx_ is a bit redundant here. This is a member of the Workspace 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 the tx_

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 a m_iters_conflicting to the workspace struct

jnewbery commented at 2:44 pm on October 28, 2021: member

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?

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

0a79eaba72

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

cbb3598b5c

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

8fa2936b34

[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).

36a8441912

glozow force-pushed on Oct 28, 2021

glozow commented at 3:17 pm on October 28, 2021: member

Thanks for the review @jnewbery, took your suggestions

in 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 to m_direct_conflicts and m_all_conflicts to highlight that difference.

Also, the m_conflicts member above is only used in PreChecks(), so could be a local variable in that function rather than a member of Workspace.

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 the Workspace members

jnewbery commented at 3:59 pm on October 28, 2021: member

utACK 4d262e060e714042d747c4f0b2e0398234893d34

One suggestion inline.

jnewbery added this to the "Blockers" column in a project

DrahtBot commented at 6:37 pm on October 28, 2021: member

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.

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:

This parameter isn’t used in this PR or #22674. I suggest leaving it out until it’s used in #22290 (and I think there, you can just pass the vector of Workspaces and not the vector of CTransactionRefs, since the Workspaces contain the CTransactionRefs)

glozow commented at 4:34 pm on November 4, 2021:

Removed from this PR

in 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 in MemPoolAccept. AFAICT, the intermediate state, as modified in consequence to account for carve-out relaxation in PreChecks 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 rename m_hash to m_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 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.

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 in m_modified_fees member declaration in Workspace. Naming could be also clearer to indicate the source of the modification itself, like m_priority_fees

glozow commented at 4:35 pm on November 4, 2021:

Added comment to m_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 in PreChecks 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 limits

ariard commented at 1:00 am on November 2, 2021: member

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

glozow force-pushed on Nov 4, 2021

[validation] cache iterators to mempool conflicts 3d3e4598b6

document workspace members fd92b0c398

glozow force-pushed on Nov 4, 2021

scripted-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-

9e910d8152

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.

c9b1439ca9

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.

14cd7bf793

glozow force-pushed on Nov 4, 2021

glozow commented at 7:50 pm on November 5, 2021: member

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

ariard commented at 1:55 am on November 9, 2021: member

Code Review ACK 14cd7bf

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

jnewbery commented at 11:44 am on November 9, 2021: member

Code review ACK 14cd7bf793547fa5143acece564482271f5c30bc

jnewbery commented at 11:47 am on November 9, 2021: member

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 approved

t-bast commented at 2:32 pm on November 9, 2021: member

Code 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: member

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

laanwj merged this on Nov 9, 2021

laanwj closed this on Nov 9, 2021

sidhujag referenced this in commit 625cdea35d on Nov 9, 2021

in 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 add Assume/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 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.

glozow deleted the branch on Nov 9, 2021

laanwj removed this from the "Blockers" column in a project

Fabcien referenced this in commit 979f4e87a4 on Oct 21, 2022

Fabcien referenced this in commit 7afb349331 on Oct 21, 2022

Fabcien referenced this in commit 7597e8cfa3 on Oct 24, 2022

Fabcien referenced this in commit bd6e032568 on Oct 24, 2022

Fabcien referenced this in commit 9c01a0070a on Oct 24, 2022

Fabcien referenced this in commit f269b4d06a on Oct 24, 2022

Fabcien referenced this in commit 789c3ac241 on Oct 24, 2022

Fabcien referenced this in commit 9f8c2b711e on Oct 24, 2022

DrahtBot locked this on Nov 9, 2022

validation/refactor: refactoring for package submission #23381

Conflicts