rpc: Disallow captures in RPCMethodImpl

ajtowns commented at 3:25 am on December 11, 2025: contributor

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, and as part of RPCHelpMan check that the function we provide is convertible to a bare function pointer, so that any attempts to capture anything (even if it’s by-copy, which is safe) results in an error.

While we’re at it, rename RPCHelpMan to RPCMethod, reflecting it’s greater responsibility since #19386.

DrahtBot added the label RPC/REST/ZMQ on Dec 11, 2025

DrahtBot commented at 3:25 am on December 11, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34049.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33392 (wallet, rpc: add UTXO set check and incremental rescan to importdescriptors by musaHaruna)
#32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)
#29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)
#26201 (Remove Taproot activation height by Sjors)

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.

ajtowns requested review from maflcko on Dec 11, 2025

ajtowns force-pushed on Dec 11, 2025

DrahtBot added the label CI failed on Dec 11, 2025

DrahtBot commented at 3:57 am on December 11, 2025: contributor

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

DrahtBot removed the label CI failed on Dec 11, 2025

in src/wallet/rpc/spend.cpp:1080 in 48c3c41fa6 outdated

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";

maflcko commented at 7:11 am on December 11, 2025:

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:

drop the second commit, and let asan/valgrind disallow the dangling references, like they have done in the past
Clarify the comment around the static_assert which is hit at compile time to say that any captured variables may have to be re-constructed a second time inside the lambda body

ajtowns commented at 9:03 am on December 11, 2025:

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.

Ataraxia009 commented at 9:04 am on December 11, 2025:

I dont think the second commit is necessary either should be fine with just removal of the captures

maflcko commented at 10:01 am on December 11, 2025:

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.

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.

ajtowns commented at 2:04 pm on December 11, 2025:

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)

maflcko approved

maflcko commented at 7:13 am on December 11, 2025: member

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)?

scripted-diff: rpc: Rename RPCHelpMan to RPCMethod

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-

a5c40bb42c

ajtowns force-pushed on Dec 11, 2025

scripted-diff: rpc: Don't pointlessly capture in RPCMethod lambdas

-BEGIN VERIFY SCRIPT-
sed -i 's/\[[&]\][(]const RPCMethod[&]/[](const RPCMethod\&/' $(git grep -l '\[\&\](const RPCMethod')
-END VERIFY SCRIPT-

54c602bc4d

ajtowns force-pushed on Dec 11, 2025

DrahtBot added the label CI failed on Dec 11, 2025

DrahtBot commented at 3:19 pm on December 11, 2025: contributor

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

rpc: Provide a way to be very explicit about what is being captured in RPCMethod 2cb1daceec

rpc: Disallow capturing in RPCHelpMan lambdas

Capturing by reference in these functions is buggy (whatever it refers
to is gone by the time the lambda returns), so disallow it.

cc1249c155

ajtowns force-pushed on Dec 11, 2025

DrahtBot removed the label CI failed on Dec 12, 2025

rpc: Disallow captures in RPCMethodImpl #34049

Code Coverage & Benchmarks

Reviews

Conflicts