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

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

2. DrahtBot added the label RPC/REST/ZMQ on Jul 15, 2025
3. 
   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)
   • #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)
   • #31560 (rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script by theStack)

   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.

4. 
   w0xlt commented at 8:20 pm on July 16, 2025: contributor

   ACK https://github.com/bitcoin/bitcoin/pull/32983/commits/319abd609b3d2194e7a05454b6af55b6f16adbf1
5. 
   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.

6. stickies-v force-pushed on Jul 24, 2025
7. 
   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!

8. stickies-v force-pushed on Jul 24, 2025
9. DrahtBot added the label CI failed on Jul 24, 2025
10. 
    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.

11. 
    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?

12. DrahtBot requested review from w0xlt on Jul 24, 2025
13. 
    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.

14. DrahtBot removed the label CI failed on Jul 24, 2025
15. stickies-v force-pushed on Jul 25, 2025
16. 
    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.

17. DrahtBot added the label CI failed on Jul 26, 2025
18. DrahtBot removed the label CI failed on Jul 26, 2025
19. DrahtBot added the label Needs rebase on Aug 19, 2025
20. 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).

    7b9a5d53c8
21. 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.

    a6072d8d0f
22. 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.

    24d4863879
23. stickies-v force-pushed on Aug 19, 2025
24. 
    stickies-v commented at 11:39 am on August 19, 2025: contributor

    Rebased to address merge conflict from #32896, no other changes.
25. DrahtBot removed the label Needs rebase on Aug 19, 2025

Contributors
stickies-v DrahtBot w0xlt pablomartin4btc

Review Requested
w0xlt

Labels
RPC/REST/ZMQ