package testmempoolaccept followups #22084

pull glozow wants to merge 5 commits into bitcoin:master from glozow:20833-followups changing 10 files +120 −84
  1. glozow commented at 8:40 am on May 27, 2021: member
    various followups from #20833
  2. DrahtBot added the label Build system on May 27, 2021
  3. DrahtBot added the label Mempool on May 27, 2021
  4. DrahtBot added the label RPC/REST/ZMQ on May 27, 2021
  5. DrahtBot added the label TX fees and policy on May 27, 2021
  6. DrahtBot added the label Validation on May 27, 2021
  7. DrahtBot commented at 3:11 pm on May 27, 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:

    • #22097 (validation: Move package acceptance size limit from KvB to WU by ariard)
    • #21866 ([Bundle 7/7] validation: Farewell, global Chainstate! by dongcarl)
    • #21800 (mempool/validation: mempool ancestor/descendant limits for packages by glozow)
    • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

    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.

  8. ariard commented at 5:20 am on May 28, 2021: member
    I think this is ready for rebase and review now :)
  9. glozow force-pushed on May 28, 2021
  10. glozow renamed this:
    [WIP] package testmempoolaccept followups
    package testmempoolaccept followups
    on May 28, 2021
  11. glozow marked this as ready for review on May 28, 2021
  12. glozow commented at 8:51 am on May 28, 2021: member
    yes! CC @ariard and @jnewbery :)
  13. fanquake removed the label Build system on Jun 1, 2021
  14. fanquake removed the label Mempool on Jun 1, 2021
  15. fanquake removed the label TX fees and policy on Jun 1, 2021
  16. fanquake added the label Refactoring on Jun 1, 2021
  17. jnewbery commented at 12:59 pm on June 1, 2021: member

    Code review ACK 94f4946b9b42b3d7c7c87ffc98a2b5248e91cc46.

    Thanks for addressing the outstanding review comments!

  18. in src/policy/packages.h:18 in 94f4946b9b outdated
    14@@ -14,6 +15,7 @@
    15 static constexpr uint32_t MAX_PACKAGE_COUNT{25};
    16 /** Default maximum total virtual size of transactions in a package in KvB. */
    17 static constexpr uint32_t MAX_PACKAGE_SIZE{101};
    18+static_assert(MAX_PACKAGE_SIZE * 4 * 1000 >= MAX_STANDARD_TX_WEIGHT);
    


    ariard commented at 5:30 pm on June 1, 2021:

    Can you switch the magic constant number of 4 by WITNESS_SCALE_FACTOR ?

    It won’t be required if #22097 gets first, but I don’t care about the order, I can take the change there.


    glozow commented at 9:03 am on June 2, 2021:
    yeah that makes sense, done
  19. in src/validation.cpp:626 in 94f4946b9b outdated
    620@@ -619,6 +621,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
    621     {
    622         const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout);
    623         if (ptxConflicting) {
    624+            if (!args.m_allow_bip125_replacement) {
    625+                // Transaction conflicts with a mempool tx, but we're not allowing replacements.
    626+                return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
    


    ariard commented at 5:32 pm on June 1, 2021:
    What do you think about updating the reject reason to “bip125-replacement-disallowed” ?

    glozow commented at 9:03 am on June 2, 2021:
    sure, updated
  20. ariard commented at 5:33 pm on June 1, 2021: member
    Otherwise, looks good to me
  21. [package] static_assert max package size >= max tx size 6c5f19d9c4
  22. [rpc] reserve space in txns 7d91442461
  23. [refactor] comment/naming improvements e8ecc621be
  24. disallow_mempool_conflicts -> allow_bip125_replacement and check earlier 5cac95cd15
  25. glozow force-pushed on Jun 2, 2021
  26. glozow commented at 9:42 am on June 2, 2021: member
    Addressed @ariard’s comments
  27. jnewbery commented at 10:16 am on June 2, 2021: member
    reACK 5cac95cd15da04b83afa1d31a43be9f5b30a1827
  28. glozow commented at 3:50 pm on June 2, 2021: member
    @ariard I looked back at the PR and just remembered your #20833 (review) about encapsulating the package policies, so I’ve added that commit and co-authored you.
  29. in src/validation.cpp:1087 in 0d7bbcd9f0 outdated
    1084@@ -1085,63 +1085,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
    1085 
    1086     PackageValidationState package_state;
    1087     const unsigned int package_count = txns.size();
    


    jnewbery commented at 4:19 pm on June 2, 2021:

    This local variable is no longer needed:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index fef880152a..a5d7966b09 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -1083,13 +1083,12 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
     5 {
     6     AssertLockHeld(cs_main);
     7 
     8-    PackageValidationState package_state;
     9-    const unsigned int package_count = txns.size();
    10     // These context-free checks can be done before grabbing the mempool lock.
    11+    PackageValidationState package_state;
    12     if (!CheckPackage(txns, package_state)) return PackageMempoolAcceptResult(package_state, {});
    13 
    14     std::vector<Workspace> workspaces{};
    15-    workspaces.reserve(package_count);
    16+    workspaces.reserve(txns.size());
    17     std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces),
    18                    [](const auto& tx) { return Workspace(tx); });
    19     std::map<const uint256, const MempoolAcceptResult> results;
    

    glozow commented at 4:29 pm on June 2, 2021:
    done
  30. MOVEONLY: context-free package policies
    Co-authored-by: ariard <antoine.riard@gmail.com>
    ee862d6efb
  31. glozow force-pushed on Jun 2, 2021
  32. jnewbery commented at 3:28 pm on June 3, 2021: member

    utACK ee862d6efb4c3c01e55f0d5d7a82cce75323cf40

    Checked that final commit is move-only

  33. in src/rpc/rawtransaction.cpp:908 in e8ecc621be outdated
    904@@ -905,7 +905,7 @@ static RPCHelpMan testmempoolaccept()
    905                 RPCResult{
    906                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
    907                         "Returns results for each transaction in the same order they were passed in.\n"
    908-                        "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n",
    909+                        "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n",
    


    harding commented at 7:20 pm on June 7, 2021:

    In [refactor] comment/naming improvements

    I think this is a bit confusing: the reader hasn’t be introduced to the allowed field yet, so they don’t know it’s part of the results. When I read this, I thought it was an input parameter that I had missed. I don’t think this is important, but if you edit, maybe this would be clearer:

    "Transactions that cannot be fully validated due to failures in other transactions will not contain an 'allowed' result.\n"

    glozow commented at 9:28 am on June 9, 2021:
    Good point, I’ll do this if I need to retouch or in a followup

    glozow commented at 10:47 am on June 10, 2021:
    Added to #21800
  34. in src/validation.h:243 in e8ecc621be outdated
    242-*                                   parent-child dependencies. The transactions must not conflict, i.e.
    243-*                                   must not spend the same inputs, even if it would be a valid BIP125
    244-*                                   replace-by-fee. Parents must appear before children.
    245+*                                   parent-child dependencies. The transactions must not conflict
    246+*                                   with each other, i.e., must not spend the same inputs. If any
    247+*                                   dependencies exist, parents must appear before children.
    


    harding commented at 7:24 pm on June 7, 2021:

    In [refactor] comment/naming improvements

    µ-nit: maybe “parents must appear anywhere in the list before their children”. Otherwise someone could mistakenly infer that a parent needed to appear immediately before its children.


    glozow commented at 9:29 am on June 9, 2021:
    Agree, will do this if I need to retouch or in a followup

    glozow commented at 10:47 am on June 10, 2021:
    Added to #21800
  35. harding commented at 7:39 pm on June 7, 2021: contributor
    A couple language nits in case this gets updated, otherwise LGTM. I didn’t review the move-only commit.
  36. in src/validation.cpp:1109 in ee862d6efb
    1108-        // impossible, we don't need to track the coins spent. Note that this logic will need to be
    1109-        // updated if RBFs in packages are allowed in the future.
    1110-        assert(args.disallow_mempool_conflicts);
    1111+        // package to spend. Since we already checked conflicts in the package and we don't allow
    1112+        // replacements, we don't need to track the coins spent. Note that this logic will need to be
    1113+        // updated if package replace-by-fee is allowed in the future.
    


    ariard commented at 9:24 pm on June 8, 2021:
    plot twist: in fact we may need package replace-by-feerate :)
  37. ariard commented at 9:26 pm on June 8, 2021: member

    Code Review ACK ee862d6

    Thanks for taking suggestions.

  38. fanquake merged this on Jun 10, 2021
  39. fanquake closed this on Jun 10, 2021

  40. glozow deleted the branch on Jun 10, 2021
  41. sidhujag referenced this in commit e21cd3b9fe on Jun 10, 2021
  42. gwillen referenced this in commit b413362d11 on Jun 1, 2022
  43. DrahtBot locked this on Aug 16, 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-11-17 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me