mempool, refactor: add MemPoolBypass #25577

pull w0xlt wants to merge 8 commits into bitcoin:master from w0xlt:add_mempoolbypass changing 20 files +256 −116
  1. w0xlt commented at 6:51 am on July 9, 2022: contributor

    #25434, #25532 and #25570 add some parameters to bypass some mempool checks. The motivations for each parameter can be found in these PRs.

    #25434 (review) suggested to allocate the existing mempool parameters bypass_limits and test_accept in the same structure.

    This PR makes that change without introducing new behaviors or new assumptions. It is basically a refactor and will be used as basis for the new parameters in the PRs mentioned above.

  2. DrahtBot added the label Mempool on Jul 9, 2022
  3. DrahtBot commented at 12:08 pm on July 9, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25757 (rpc: add a new parameter desriptor_file to importdescriptors RPC by w0xlt)
    • #25747 (rpc: add a new parameter save_to_file to listdescriptors RPC by w0xlt)
    • #25648 (refactor: Remove all policy globals by MarcoFalke)
    • #24007 ([mempool] allow tx replacement by smaller witness by LarryRuane)

    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.

  4. w0xlt force-pushed on Jul 9, 2022
  5. w0xlt force-pushed on Jul 9, 2022
  6. w0xlt force-pushed on Jul 9, 2022
  7. in src/rpc/mempool.cpp:175 in 26ee1a47e4 outdated
    154-                                                     CFeeRate(AmountFromValue(request.params[1]));
    155+            CFeeRate max_raw_tx_fee_rate = DEFAULT_MAX_RAW_TX_FEE_RATE;
    156+
    157+            if (!request.params[1].isNull()) {
    158+                const UniValue& options = request.params[1];
    159+                RPCTypeCheckObj(options,
    


    luke-jr commented at 3:48 pm on July 16, 2022:
    Should have backward compatibility for now

  8. in src/validation.cpp:523 in 26ee1a47e4 outdated
    515@@ -518,18 +516,16 @@ class MemPoolAccept
    516         // Private ctor to avoid exposing details to clients and allowing the possibility of
    517         // mixing up the order of the arguments. Use static functions above instead.
    518         ATMPArgs(const CChainParams& chainparams,
    519-                 int64_t accept_time,
    520-                 bool bypass_limits,
    521+                 const int64_t accept_time,
    522+                 const std::optional<MemPoolBypass>& mempool_bypass,
    


    luke-jr commented at 3:50 pm on July 16, 2022:
    Probably better to just have a constexpr noop MemPoolBypass

    w0xlt commented at 4:51 am on July 17, 2022:
    Could you elaborate more ? I don´t see how constexpr fits in this case.
  9. in src/validation.h:247 in 26ee1a47e4 outdated
    242+    //! This does not prevent mempool from accepting the transaction.
    243+    bool m_bypass_limits{false};
    244+
    245+    MemPoolBypass() {}
    246+
    247+    MemPoolBypass(bool test_accept, bool bypass_limits) :
    


    luke-jr commented at 3:55 pm on July 16, 2022:
    Prefer limiting this to designated initializers, or otherwise explicit assignment.

    w0xlt commented at 5:05 pm on July 16, 2022:

    I originally used only C++ 20 designated initializers in this PR, but it crashes on one (and only one) CI check (error C7555 on a Windows machine, if I remember correctly).

    Something like: error C7555: use of designated initializers requires at least '/std:c++20'


    fanquake commented at 5:39 pm on July 16, 2022:
    We now use C++20 on that CI, so they should work fine.

  10. luke-jr changes_requested
  11. w0xlt force-pushed on Jul 17, 2022
  12. w0xlt commented at 4:50 am on July 17, 2022: contributor
    @luke-jr Thanks for the review. #25577 (review) and #25577 (review) have been addressed in the new push.
  13. in src/rpc/mempool.cpp:108 in 151c4d2ab7 outdated
     97@@ -98,7 +98,15 @@ static RPCHelpMan testmempoolaccept()
     98                 },
     99             },
    100             {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
    101-             "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB\n"},
    102+             "(deprecated. Use \"options\" instead) Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT + "/kvB\n"},
    


    luke-jr commented at 4:55 am on July 17, 2022:

    Replacing the option is fine. Only need to tolerate the old param type.

    Might need to pull in 09964963a1f7748fc1abc9471cbae8dab1061eb1


    w0xlt commented at 8:36 pm on July 17, 2022:

    Are you suggesting something like the dummy argument in getbalance() ?

    0RPCHelpMan getbalance()
    1{
    2  ....
    3  {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},
    

    luke-jr commented at 8:51 pm on July 17, 2022:

    No, more like this:

    0{"options|maxfeerate", {RPCArg::Type::OBJ, RPCArg::Type::AMOUNT}, RPCArg::Optional::OMITTED_NAMED_ARG, "",
    

  14. DrahtBot added the label Needs rebase on Jul 18, 2022
  15. w0xlt force-pushed on Jul 24, 2022
  16. DrahtBot removed the label Needs rebase on Jul 24, 2022
  17. RPC: Support specifying different types for param aliases
    Author: Luke Dashjr <luke-jr+git@utopios.org>
    5d9322ca15
  18. mempool: change `maxfeerate` to be a members of an `options` argument
    `maxfeerate` becomes a member of an "options" object rather than
    a positional argument.
    
    The idea is that any new parameters in the future will also go into
    options.
    841c77b183
  19. mempool: make `test_accept` parameter explicit a6db2d6293
  20. validation: Add `MemPoolBypass` for bypassing mempool checks d2c829ca65
  21. mempool: add `MemPoolBypass` parameter to `ProcessNewPackage` d6b9831aad
  22. mempool: add `MemPoolBypass` parameter to `AcceptToMemoryPool` 139a58e84d
  23. mempool: add `MemPoolBypass` parameter to `ProcessTransaction` 4e030fa1a7
  24. mempool: add `MemPoolBypass` parameter to `ATMPArgs` 863fb9757f
  25. w0xlt force-pushed on Jul 24, 2022
  26. w0xlt commented at 6:51 am on July 24, 2022: contributor
    @luke-jr I added the suggested commit and changed the parameter to "options|maxfeerate.
  27. glozow commented at 10:24 am on August 1, 2022: member
    Unclear to me how beneficial this refactor is, see contrib guidelines on refactoring.
  28. w0xlt commented at 2:09 pm on August 1, 2022: contributor

    @glozow This refactoring adds the options parameter to testmempoolaccept and also keeps the original parameter. This is only useful if more parameters are added to this RPC, such as #25434.

    As there is no consensus for #25434 (or #25532 and #25570), I will close this PR for now.

  29. w0xlt closed this on Aug 1, 2022

  30. bitcoin locked this on Aug 1, 2023

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-07-03 10:13 UTC

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