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
ACK	maflcko, pablomartin4btc
Stale ACK	w0xlt

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)
#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)
#32015 (net: replace manual reference counting of CNode with shared_ptr 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

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

DrahtBot added the label Needs rebase on Aug 19, 2025

stickies-v force-pushed on Aug 19, 2025

stickies-v commented at 11:39 am on August 19, 2025: contributor

Rebased to address merge conflict from #32896, no other changes.

DrahtBot removed the label Needs rebase on Aug 19, 2025

in src/rpc/net.cpp:352 in 7b9a5d53c8 outdated

348@@ -349,13 +349,13 @@ static RPCHelpMan addnode()
349     if (command == "onetry")
350     {
351         CAddress addr;
352-        connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grant_outbound=*/{}, node_arg.c_str(), ConnectionType::MANUAL, use_v2transport);
353+        connman.OpenNetworkConnection(addr, /*fCountFailure=*/false, /*grant_outbound=*/{}, node_arg.data(), ConnectionType::MANUAL, use_v2transport);

maflcko commented at 8:26 am on August 29, 2025:

7b9a5d53c8ae47d2789e31306d035843d870f9a8: This happens to work, but generally it is not fine to replace c_str() with string_view::data(), because it is lacking the null-terminator. To restore the null-terminator in all cases, you’ll have to create a copy again:

0std::string{node_arg}.c_str()

stickies-v commented at 1:17 pm on August 29, 2025:

I agree. Even though there are no issues now, it’s safer to avoid using .data() altogether. I’ve adopted your suggestion. I’ll probably open a separate PR later today to change the necessary net code to allow us to string_view here. It’s a relatively straightforward change, but I don’t want to scope creep this PR more.

in src/rpc/util.h:449 in 7b9a5d53c8 outdated

444@@ -445,7 +445,7 @@ class RPCHelpMan
445     {
446         auto i{GetParamIndex(key)};
447         // Return argument (required or with default value).
448-        if constexpr (std::is_integral_v<R> || std::is_floating_point_v<R>) {
449+        if constexpr (std::is_trivially_copyable_v<R>) {
450             // Return numbers by value.

maflcko commented at 8:32 am on August 29, 2025:

7b9a5d53c8ae47d2789e31306d035843d870f9a8: update docstring?

stickies-v commented at 1:17 pm on August 29, 2025:

Done!

in src/rpc/net.cpp:761 in 24d4863879 outdated

764     CNetAddr netAddr;
765-    bool isSubnet = false;
766-
767-    if (request.params[0].get_str().find('/') != std::string::npos)
768-        isSubnet = true;
769+    auto subnet_arg{request.params[0].get_str()};

maflcko commented at 9:31 am on August 29, 2025:

0    std::string subnet_arg{help.Arg<std::string_view>("subnet"};

nit in the last commit: Use the named Arg helper for new code?

stickies-v commented at 1:18 pm on August 29, 2025:

Done. Kept the get_str() because we need a std::string, but your suggestion is clean.

maflcko approved

maflcko commented at 9:48 am on August 29, 2025: member

Looks like a nice change using more named Arg and MaybeArg helpers, removing the manual params[idx].isNull and params[idx].get_str legacy handling.

Not a blocker, but it would be nice to fix the theoretical c_str/data issue.

lgtm ACK 24d48638795ba61e8a420df4b831281455e208a7 🏉

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: lgtm ACK 24d48638795ba61e8a420df4b831281455e208a7 🏉
35H7u/Rl/t/WytDb3+AuCk6ztGaKQRbxTrL0FpxHFBTYItwNTR1lUNNpUXBYo9owTgz4R28dgoi482SWydKgABw==

DrahtBot requested review from pablomartin4btc on Aug 29, 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).

d290cb7b55

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.

2893a545c4

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.

b49a4f17ab

stickies-v force-pushed on Aug 29, 2025

stickies-v commented at 1:19 pm on August 29, 2025: contributor

Force-pushed to address review comments from @maflcko:

fixed docstring
removed usage of std::string_view::data() to avoid null termination issues in the future
added one more usage of Arg<string_view> helper

maflcko commented at 5:31 am on September 1, 2025: member

re-ACK b49a4f17aba76d2f2d7f1109d7e68b02303947bf 📦

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: re-ACK b49a4f17aba76d2f2d7f1109d7e68b02303947bf 📦
3JW4EvwaR4XLQhcR6T9hGyLgtYsINoIScF38nceTExXcoMNow8XF1qF09JmqP0GHa9MG74bnoJu39PvfdvemcAg==

in src/wallet/rpc/coins.cpp:199 in b49a4f17ab

195@@ -196,8 +196,7 @@ RPCHelpMan getbalance()
196 
197     LOCK(pwallet->cs_wallet);
198 
199-    const auto dummy_value{self.MaybeArg<std::string>("dummy")};
200-    if (dummy_value && *dummy_value != "*") {
201+    if (self.MaybeArg<std::string_view>("dummy").value_or("*") != "*") {

pablomartin4btc commented at 2:18 am on September 12, 2025:

not sure about the clarity of this one-liner change… perhaps could be:

0    if (auto dummy = self.MaybeArg<std::string_view>("dummy"); dummy && *dummy != "*") {

DrahtBot requested review from pablomartin4btc on Sep 12, 2025

in src/script/descriptor.cpp:2743 in b49a4f17ab

2743+    if (!CheckChecksum(descriptor, require_checksum, error)) return {};
2744     uint32_t key_exp_index = 0;
2745-    auto ret = ParseScript(key_exp_index, sp, ParseScriptContext::TOP, out, error);
2746-    if (sp.size() == 0 && !ret.empty()) {
2747+    auto ret = ParseScript(key_exp_index, descriptor, ParseScriptContext::TOP, out, error);
2748+    if (descriptor.size() == 0 && !ret.empty()) {

pablomartin4btc commented at 2:19 am on September 12, 2025:

nit: (not a blocker) if you need to re-touch…

0    if (!descriptor.empty() && !ret.empty()) {

DrahtBot requested review from pablomartin4btc on Sep 12, 2025

in src/rpc/output_script.cpp:137 in b49a4f17ab

141-                }
142-                output_type = parsed.value();
143+            auto address_type{self.Arg<std::string_view>("address_type")};
144+            auto output_type{ParseOutputType(address_type)};
145+            if (!output_type) {
146+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", address_type));

pablomartin4btc commented at 2:19 am on September 12, 2025:

nit (for safety & consistency):

0                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, tfm::format"Unknown address type '%s'", address_type));

Already done in other places like:

https://github.com/bitcoin/bitcoin/blob/b49a4f17aba76d2f2d7f1109d7e68b02303947bf/src/rpc/node.cpp#L193

DrahtBot requested review from pablomartin4btc on Sep 12, 2025

in src/rpc/blockchain.cpp:2641 in b49a4f17ab

2636@@ -2633,9 +2637,8 @@ static RPCHelpMan scanblocks()
2637         ret.pushKV("to_height", start_index->nHeight); // start_index is always the last scanned block here
2638         ret.pushKV("relevant_blocks", std::move(blocks));
2639         ret.pushKV("completed", completed);
2640-    }
2641-    else {
2642-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", request.params[0].get_str()));
2643+    } else {
2644+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", action));

pablomartin4btc commented at 2:19 am on September 12, 2025:

nit (same observation here regarding tfm::format):

0        throw JSONRPCError(RPC_INVALID_PARAMETER, tfm::format("Invalid action '%s'", action));

DrahtBot requested review from pablomartin4btc on Sep 12, 2025

pablomartin4btc approved

pablomartin4btc commented at 2:19 am on September 12, 2025: member

crACK & tACK b49a4f17aba76d2f2d7f1109d7e68b02303947bf

Verified addressed comments & suggestions from @maflcko since my last review.

Performed some manual testing on RPC calls (getblock, addnode, getdescriptoractivity, signmessage, verifymessage). Also tested them passing very long strings (>10MB).

Left a couple of tiny nits, none blocker.

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

Code Coverage & Benchmarks

Reviews

Conflicts