mempool, refactor: add MemPoolBypass#25577
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
w0xlt
force-pushed
the
add_mempoolbypass
branch
3 times, most recently
from
a5a17cf to
26ee1a4
Compare
|
|
||
| if (!request.params[1].isNull()) { | ||
| const UniValue& options = request.params[1]; | ||
| RPCTypeCheckObj(options, |
There was a problem hiding this comment.
Should have backward compatibility for now
| int64_t accept_time, | ||
| bool bypass_limits, | ||
| const int64_t accept_time, | ||
| const std::optional<MemPoolBypass>& mempool_bypass, |
There was a problem hiding this comment.
Probably better to just have a constexpr noop MemPoolBypass
There was a problem hiding this comment.
Could you elaborate more ? I don´t see how constexpr fits in this case.
src/validation.h
Outdated
|
|
||
| MemPoolBypass() {} | ||
|
|
||
| MemPoolBypass(bool test_accept, bool bypass_limits) : |
There was a problem hiding this comment.
Prefer limiting this to designated initializers, or otherwise explicit assignment.
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
We now use C++20 on that CI, so they should work fine.
|
@luke-jr Thanks for the review. #25577 (comment) and #25577 (comment) have been addressed in the new push. |
src/rpc/mempool.cpp
Outdated
There was a problem hiding this comment.
Replacing the option is fine. Only need to tolerate the old param type.
Might need to pull in 0996496
There was a problem hiding this comment.
Are you suggesting something like the dummy argument in getbalance() ?
RPCHelpMan getbalance()
{
....
{"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."},There was a problem hiding this comment.
No, more like this:
{"options|maxfeerate", {RPCArg::Type::OBJ, RPCArg::Type::AMOUNT}, RPCArg::Optional::OMITTED_NAMED_ARG, "",Author: Luke Dashjr <[email protected]>
`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.
|
@luke-jr I added the suggested commit and changed the parameter to |
|
Unclear to me how beneficial this refactor is, see contrib guidelines on refactoring. |
5 participants
#25434, #25532 and #25570 add some parameters to bypass some mempool checks. The motivations for each parameter can be found in these PRs.
#25434 (comment) suggested to allocate the existing mempool parameters
bypass_limitsandtest_acceptin 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.