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

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:

#33117 (Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications by D33r-Gee)
#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)
#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)
#30988 (Split CConnman by vasild)
#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

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 outdated

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 != "*") {

stickies-v commented at 10:36 am on September 12, 2025:

Yeah you’re probably right, will change if I force push, but I feel like this is close to merge and the PR has been open for a while so gonna minimize nit force pushes. Thanks for your review!

maflcko commented at 9:58 am on September 26, 2025:

I actually like the value_or version better, because it has less branches (can be read left-to-right without jumps). Also, it is shorter. So my recommendation would be to merge this as-is. (or keep it in the rebase)

stickies-v commented at 11:58 am on October 2, 2025:

Keeping as value_or, no strong opinion either way.

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()) {

stickies-v commented at 11:57 am on October 2, 2025:

Done (but without negating the emptiness which seems wrong)

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

stickies-v commented at 10:32 am on September 12, 2025:

Will do if I force-push, but they are aliases so there should be no safety difference, and afaik the project also hasn’t stated a preference to phase one out, although I personally tend to use and prefer tfm::format.

stickies-v commented at 11:58 am on October 2, 2025:

Done.

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.

DrahtBot added the label Needs rebase on Oct 1, 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).

b3bf18f0ba

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.

037830ca0d

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.

b63428ac9c

stickies-v force-pushed on Oct 2, 2025

stickies-v commented at 12:01 pm on October 2, 2025: contributor

Force-pushed to address merge conflict from #32326, and addressed 2 review nits from @pablomartin4btc :

replaced strprintf usage with tfm::format
replaced descriptor.size() == 0 with descriptor.empty()

DrahtBot removed the label Needs rebase on Oct 2, 2025

pablomartin4btc approved

pablomartin4btc commented at 5:58 pm on October 2, 2025: member

re-ACK b63428a

(tiny nit - sorry, just realised this - only if you have to retouch or rebase again, it would be good to update the copyrights on the files changed that requires it)

DrahtBot requested review from maflcko on Oct 2, 2025

w0xlt commented at 11:12 am on October 21, 2025: contributor

reACK https://github.com/bitcoin/bitcoin/pull/32983/commits/b63428ac9ce2c903670409b3e47b9f6730917ae8

maflcko commented at 10:28 am on October 24, 2025: member

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

re-ACK b63428ac9ce2c903670409b3e47b9f6730917ae8 🎉

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 b63428ac9ce2c903670409b3e47b9f6730917ae8 🎉
301hezAbAsrlrdi4mpWuS//0Xux46CJ9DK+oK6Cc5/o5ajTTSoqllwMLFJZ8l95e7b/JyycTSig4pZ49xrkuVAA==

rfm?

achow101 commented at 5:17 pm on October 24, 2025: member

ACK b63428ac9ce2c903670409b3e47b9f6730917ae8

achow101 merged this on Oct 24, 2025

achow101 closed this on Oct 24, 2025

stickies-v deleted the branch on Oct 28, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts