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
  1. 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.
  2. glozow added the label Refactoring on Oct 28, 2021
  3. 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
  4. 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
  5. 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
  6. 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_
  7. 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
  8. 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?

  9. [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
  10. [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
  11. [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
  12. [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
  13. glozow force-pushed on Oct 28, 2021
  14. glozow commented at 3:17 pm on October 28, 2021: member
    Thanks for the review @jnewbery, took your suggestions
  15. 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
  16. jnewbery commented at 3:59 pm on October 28, 2021: member

    utACK 4d262e060e714042d747c4f0b2e0398234893d34

    One suggestion inline.

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

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

  19. 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
  20. 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.

  21. 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!
  22. 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 👍
  23. 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
  24. ariard commented at 1:00 am on November 2, 2021: member
    Overall good. See the comment around the newer descendant limits members in Workspace.
  25. glozow force-pushed on Nov 4, 2021
  26. [validation] cache iterators to mempool conflicts 3d3e4598b6
  27. document workspace members fd92b0c398
  28. glozow force-pushed on Nov 4, 2021
  29. 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
  30. 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
  31. MOVEONLY: move package unit tests to their own file 6876378365
  32. [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
  33. glozow force-pushed on Nov 4, 2021
  34. glozow commented at 7:50 pm on November 5, 2021: member
    Thanks for the reviews @ariard and @jnewbery, took suggestions
  35. 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.

  36. jnewbery commented at 11:44 am on November 9, 2021: member
    Code review ACK 14cd7bf793547fa5143acece564482271f5c30bc
  37. 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.

  38. t-bast approved
  39. 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:

  40. laanwj commented at 3:46 pm on November 9, 2021: member
    Code review ACK 14cd7bf793547fa5143acece564482271f5c30bc, thanks for adding documentation and clarifying the code
  41. laanwj merged this on Nov 9, 2021
  42. laanwj closed this on Nov 9, 2021

  43. sidhujag referenced this in commit 625cdea35d on Nov 9, 2021
  44. 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.
  45. glozow deleted the branch on Nov 9, 2021
  46. laanwj removed this from the "Blockers" column in a project

  47. Fabcien referenced this in commit 979f4e87a4 on Oct 21, 2022
  48. Fabcien referenced this in commit 7afb349331 on Oct 21, 2022
  49. Fabcien referenced this in commit 7597e8cfa3 on Oct 24, 2022
  50. Fabcien referenced this in commit bd6e032568 on Oct 24, 2022
  51. Fabcien referenced this in commit 9c01a0070a on Oct 24, 2022
  52. Fabcien referenced this in commit f269b4d06a on Oct 24, 2022
  53. Fabcien referenced this in commit 789c3ac241 on Oct 24, 2022
  54. Fabcien referenced this in commit 9f8c2b711e on Oct 24, 2022
  55. DrahtBot 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