rpc: refactor: use string_view in Arg/MaybeArg

stickies-v commented at 11:23 pm on July 15, 2025: contributor

The RPCHelpMan::{Arg,MaybeArg} helpers avoid copying (potentially) large strings by returning them as const std::string* (MaybeArg) or const std::string& (Arg). For MaybeArg, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. EnsureUniqueWalletName ) with raw pointers being implemented.

This PR aims to improve on this by returning a trivially copyable std::string_view (Arg) or std::optional<std::string_view> (MaybeArg), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using std::is_trivially_copyable_v instead of defining the types manually.

In cases where functions currently take a const std::string& and it would be too much work / touching consensus logic to update them (signmessage.cpp), a std::string copy is made (which was already happening anyway).

The last 2 commits increase usage of the {Arg,MaybeArg}<std::string_view> helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it’s a nice little cleanup.

DrahtBot added the label RPC/REST/ZMQ on Jul 15, 2025

DrahtBot commented at 11:23 pm on July 15, 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/32983.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	w0xlt, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32958 (wallet/refactor: change PSBTError to PSBTResult and remove std::optionalcommon::PSBTResult and return common::PSBTResult by kevkevinpal)
#32896 (wallet, rpc: add v3 transaction creation and wallet support by ishaanam)
#32394 (net: make m_nodes_mutex non-recursive by vasild)
#32326 (net: improve the interface around FindNode() and avoid a recursive mutex lock by vasild)
#32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
#31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
#31560 (rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script by theStack)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)

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 commented at 8:20 pm on July 16, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32983/commits/319abd609b3d2194e7a05454b6af55b6f16adbf1

pablomartin4btc commented at 3:45 am on July 24, 2025: member

ACK 319abd609b3d2194e7a05454b6af55b6f16adbf1

I like this refactoring — the general idea of reducing unnecessary string allocations via string_view makes sense.

I’m a bit unsure about the changes in mining.cpp and signmessage.cpp. It seems like we’re going from string → string_view → string, which introduces a bit of unnecessary churn and could lead to confusion down the line. I understand the intention may be to eventually update those functions to accept string_view(?), but without that broader change in place, it’s hard to see a clear benefit to doing it this way for now.

stickies-v force-pushed on Jul 24, 2025

stickies-v commented at 11:49 am on July 24, 2025: contributor

Force-pushed from 319abd6 to b8772ef to remove a std::string copy in rpc/mining.cpp, addressing (part of) @pablomartin4btc’s feedback.

It seems like we’re going from string → string_view → string

string_view is a non-owning read-only reference, so by definition it has to point to some other string-like type. string → string_view doesn’t incur any allocations, is trivial, and by definition happens anytime we have a string_view. The string_view → string operation indeed involves a new std::string allocation, and should be avoided where possible.

In signmessage.cpp, we were already coping the 3 strings, so 91a8ed27f3f9c58a210f02640371f274bdfcddbf does not add any overhead to what we currently have. We could optimize for performance and revert to using the request.params[{0,1,2}}.get_str(); syntax which would eliminate the copies, but I don’t think the cost of these copies is meaningful to the overall cost of this cryptographically heavy function. I’d personally favour readability and phasing out the old interface, but I’m happy to change that if reviewers prefer to optimize for performance here. Another alternative would be to keep the Arg<std::string> specialization, but I think it is much better to get rid of it now so it doesn’t get used in new places.

In mining.cpp, 91a8ed27f3f9c58a210f02640371f274bdfcddbf indeed introduced a new allocation. I thought avoiding it would touch too much code change, but I just had another look and it’s actually pretty trivial (and enables one more use case of the Arg helper in the last commit), so I force-pushed to improve that in 122b432b71e148cd85edcd74df741f99ac7f187c. Thanks for your review!

stickies-v force-pushed on Jul 24, 2025

DrahtBot added the label CI failed on Jul 24, 2025

DrahtBot commented at 12:11 pm on July 24, 2025: contributor

🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/46641144470 LLM reason (✨ experimental): The CI failure is caused by a compilation error in descriptors.cpp due to a type mismatch error involving ‘const char*’ being used where a ‘std::span’ is expected.

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.

pablomartin4btc commented at 1:57 pm on July 24, 2025: member

re-ACK 122b432b71e148cd85edcd74df741f99ac7f187c

Not blocking: I’d personally prefer keeping Arg<std::string> in signmessage.cpp if we’re not planning to update MessageVerify to accept string_view instead of const std::string&. Is updating MessageVerify something you’d prefer to avoid at this stage?

DrahtBot requested review from w0xlt on Jul 24, 2025

stickies-v commented at 2:10 pm on July 24, 2025: contributor

Is updating MessageVerify something you’d prefer to avoid at this stage?

Yes. It involves a cascade of changes, many touching consensus code, that require significant review and have mostly nothing to do with RPC, so it would blow up the scope of this PR.

DrahtBot removed the label CI failed on Jul 24, 2025

rpc: refactor: use string_view in Arg/MaybeArg

Modernizes interface by not forcing users to deal with raw pointers,
without adding copying overhead. Generalizes the logic of whether
we return by value or by optional/pointer.

In cases where functions take a `const std::string&` and it would
be too much work to update them, a string copy is made (which was
already happening anyway).

6d0796b809

refactor: increase string_view usage

Update select functions that take a const std::string& to take a
std::string_view instead. In a next commit, this allows us to use
the {Arg,MaybeArg}<std::string_view> helper.

bbdeafd9f4

rpc: refactor: use more (Maybe)Arg<std::string_view>

Use the {Arg,MaybeArg}<std::string_view> helper in all places where
it is a trivial change. In many places, this simplifies the logic
and reduces duplication of default values.

137319b5d6

stickies-v force-pushed on Jul 25, 2025

stickies-v commented at 8:33 pm on July 25, 2025: contributor

Force-pushed from 122b432 to 137319b to address silent merge conflict from #32845:

replaced self.MaybeArg<std::string> with self.MaybeArg<std::string_view> in wallet/rpc/wallet.cpp
updated EnsureUniqueWalletName signature to take std::optional<std::string_view> instead of const std::string*.

Otherwise no changes.

DrahtBot added the label CI failed on Jul 26, 2025

DrahtBot removed the label CI failed on Jul 26, 2025

rpc: refactor: use string_view in Arg/MaybeArg #32983

Code Coverage & Benchmarks

Reviews

Conflicts