When defining RPCHelpMan objects, we usually return a lambda, and mostly we define those via [&](...) { ... } which explicitly captures any parameters or local variables by reference. If we were to actually use any of those captures (we don’t), we would invoke undefined behaviour. So instead, convert all the [&] to [] to avoid capturing.
While we’re at it, rename RPCHelpMan to RPCMethod, reflecting its greater responsibility since #19386.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34049.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, maflcko, rkrux |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, 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.
Possible typos and grammar issues:
2026-03-25 09:24:11
🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20120913076/job/57740673204
LLM reason (✨ experimental): clang-tidy failed due to a bugprone-move-forwarding-reference error (forwarding reference passed to std::move()), treated as a warning-as-error.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
1074@@ -1075,8 +1075,9 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1075 "\nBump the fee, get the new transaction\'s " + std::string(want_psbt ? "psbt" : "txid") + "\n" +
1076 HelpExampleCli(method_name, "<txid>")
1077 },
1078- [want_psbt](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1079+ [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1080 {
1081+ const bool want_psbt = request.strMethod == "psbtbumpfee";
nit in 48c3c41fa604b8634c509bb38d67de9bea2ab59d: I think the first commit makes a lot of sense and should be done. However, the second commit seems overly restrictive. Here, it is easy to re-create the calculation, but will it always be the case? I think it could make sense to either:
Here, it is easy to re-create the calculation, but will it always be the case?
I think so – the RpcMethodFnType functions don’t take any params, so if you’re doing a calculation (like a deprecatedrpc test) you can just do the same thing inside the RPCMethodImpl, and if you’re providing something hardcoded, you can pass a boolean or enum via a template (eg template <bool wants_enum> static RPCHelpMan bumpfee_filter) and make your logic conditional on that.
Here, it is easy to re-create the calculation, but will it always be the case?
I think so – the
RpcMethodFnTypefunctions don’t take any params, so if you’re doing a calculation (like a deprecatedrpc test) you can just do the same thing inside theRPCMethodImpl, and if you’re providing something hardcoded, you can pass a boolean or enum via a template (egtemplate <bool wants_enum> static RPCHelpMan bumpfee_filter) and make your logic conditional on that.
Oh, I meant “succinct” instead of “easy”. Imagine there was another (maybe even more verbose) const std::string incremental_fee{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE).ToString(FeeEstimateMode::SAT_VB)};, which was used in both places (doc and impl). At some point it becomes tedious to keep the two in sync. Sure, it is possible to add a global function for it, but that also seems tedious.
We are somewhat stuck with C++ and can’t use Rust, so it will be normal to use sanitizers. In this case, I think it is fine to defer to them and prefer the cleaner code. But no strong opinion, this is just my preference.
Hmm; personally, I think if we can get compile time errors without too much hassle, that’s much better than leaving it for CI to uncover.
Bumped the PR with a way of capturing succintly, but being very explicit about it when you’re doing it.
(In that model, if you wanted to capture incremental_fee as well, you’d put both items of data into a struct Data { bool want_psbt; std::string incremental_fee_desc; } and write data.want_psbt instead of just want_psbt, etc)
lgtm. Seems fine to remove this silent footgun. I left a nit in the second commit, because I think it is fine to relax it a bit.
Also, while doing a global scripted-diff here, might as well do the rename RPCHelpMan to RPCMethod from #19386 (review)?
🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20135737627/job/57788454545
LLM reason (✨ experimental): clang-tidy failed the CI due to a performance-move-const-arg error in spend.cpp (std::move on a const bool).
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Since this class defines the functionality of the RPC method, not
just its help text, this better reflects reality.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/\bRPCHelpMan\b/RPCMethod/g' $(git grep -l RPCHelpMan src/)
-END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT-
sed -i 's/\[[&]\][(]const RPCMethod[&]/[](const RPCMethod\&/' $(git grep -l '\[\&\](const RPCMethod')
-END VERIFY SCRIPT-
review ACK 5a81d73a810d1fe6f20832bc6d9962da8dd2008a 🏣
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 5a81d73a810d1fe6f20832bc6d9962da8dd2008a 🏣
3QpEc9mvDgy3IqAVaYnsa/pOyg4LN91Hmm2a0tCiJTHns9JOSnF+2cnVYRR7sxjOsTHjnThMKXKXsaRV+Zt34Bg==
code review ACK 5a81d73a810d1fe6f20832bc6d9962da8dd2008a
It (the renaming) makes the RPC definitions easier to parse mentally for me. Also, I prefer removal of unnecessary capturing.
Removing unneeded captures (especially by reference) is definitely an improvement. But note that std::function’s whole purpose is to support captures. If captures are not needed in any instance, then std::function is the wrong tool and a simple function pointer would suffice.
0+ using RPCMethodImpl = UniValue (*)(const RPCHelpMan&, const JSONRPCRequest&);
1- using RPCMethodImpl = std::function<UniValue(const RPCHelpMan&, const JSONRPCRequest&)>;
Nitpick:
we usually return a lambda
A lambda is not something that can be returned, just like one cannot return an addition or a multiplication. A lambda is an expression that yields to a closure object.
Removing unneeded captures (especially by reference) is definitely an improvement. But note that
std::function’s whole purpose is to support captures. If captures are not needed in any instance, thenstd::functionis the wrong tool and a simple function pointer would suffice.
I think this pull is referring to captures by reference, which indeed are not used and obviously can not be used anyway. However captures by value are used, and work just fine, see for example:
0src/wallet/rpc/spend.cpp:1030: [want_psbt](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
Labels
RPC/REST/ZMQ
Milestone
32.0