wallet, rpc: Use OUTPUT_TYPES to describe the output types instead of hardcoding them #32432

1. 
   w0xlt commented at 2:27 am on May 7, 2025: contributor

   Follow-up to #32429, built on top of it.

   This PR addresses the #32429 (review) that the RPC documentation does not use OUTPUT_TYPES, but rather hardcodes them, as is already the case for the getnewaddress command. So here the output types are changed from std::string to std::string_view so that the values are known at compile time or during the early stages of program startup, before main() execution.

   It also updates wallet/rpc/addresses.cpp to write the RPC docs according to OUTPUT_TYPES instead of using hardcoded version.

   It also updates the documentation in outputtypes.h, adding Doxygen comments,

2. 
   DrahtBot commented at 2:28 am on May 7, 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/32432.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, achow101
   Stale ACK	musaHaruna

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

   Conflicts

   No conflicts as of last run.

3. w0xlt force-pushed on May 7, 2025
4. DrahtBot added the label CI failed on May 7, 2025
5. 
   DrahtBot commented at 2:41 am on May 7, 2025: contributor

   🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/41767168365 LLM reason (✨ experimental): The CI failure is due to a “/*” within a comment, which is treated as an 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.

6. w0xlt force-pushed on May 7, 2025
7. DrahtBot removed the label CI failed on May 7, 2025
8. laanwj added the label Wallet on May 7, 2025
9. laanwj added the label RPC/REST/ZMQ on May 7, 2025
10. DrahtBot added the label Needs rebase on May 7, 2025
11. w0xlt force-pushed on May 8, 2025
12. DrahtBot removed the label Needs rebase on May 8, 2025
13. 
    musaHaruna commented at 6:53 pm on May 8, 2025: none

    Concept ACK 2fee8f4

    After reading the comment #32429

    The PR is addresses the comment very well.

    Overall code looks good to me. The additional documentation added to the outputtype.h file help makes the code clearer. A little suggestion which you can feel free to ignore. Since PR is also making updates to the documentation in outputtypes.h you can also add that to the PR’s description.

14. 
    w0xlt commented at 9:44 pm on May 8, 2025: contributor

    Thanks for the review and suggestion @musaHaruna . Added that text to the PR’s description.
15. 
    musaHaruna commented at 8:40 pm on May 13, 2025: none

    tACK 2fee8f4z

    Compiled Successfully without errors Test the code functionality using: build/bin/bitcoin-cli -regtest help getnewaddress Result: Arguments: ` …..

    2. address_type (string, optional, default=set by -addresstype) The address type to use. Options are "legacy", "p2sh-segwit", "bech32", "bech32m".

16. 
    maflcko commented at 10:00 am on May 14, 2025: member

    So here the output types are changed from std::string to std::string_view so that the values are known at compile time or during the early stages of program startup, before main() execution.

    Can you explain why this is needed? Seems a lot of shuffling and code changes for a trivial change that will later cast everything to std::string anyway?

17. 
    w0xlt commented at 8:12 pm on May 15, 2025: contributor

    std::string_view is known (and usable) at compile time as a view over some static data (it can be constexpr), so it can be concatenated to RPC docs. [1] std::string no, it’s always a runtime, owning, heap‐allocated string, so it won´t work to produce the RPC docs.

    [1] https://stackoverflow.com/a/77358740 [2] https://en.cppreference.com/w/cpp/string/basic_string_view

18. 
    maflcko commented at 6:58 am on May 16, 2025: member

    std::string no, it’s always a runtime, owning, heap‐allocated string, so it won´t work to produce the RPC docs.

    Not sure what you mean by “won’t work”. What is the exact diff to reproduce the compile failure or error?

19. 
    maflcko commented at 7:07 am on May 16, 2025: member

    std::string_view is known (and usable) at compile time as a view over some static data (it can be constexpr), so it can be concatenated to RPC docs. [1]

    Also, I don’t think this is true. I don’t think it is possible in C++20 to concatenate two string_view into a single string_view at compile time.

20. DrahtBot added the label Needs rebase on May 16, 2025
21. 
    maflcko commented at 12:15 pm on June 4, 2025: member

    Are you still working on this?
22. 
    w0xlt commented at 6:21 pm on June 4, 2025: contributor

    Not sure what you mean by “won’t work”. What is the exact diff to reproduce the compile failure or error?

    If you try to concatenate the RPC description with any of the output type string variables, you will get an empty output type string. The reason seems to be that they are not yet initialized when this operation occurs.

    Using string_view solves this, as it ensures that the output types are already initialized at this point.

    But other suggestions on how we can use output type variables in the RPC description instead of hardcoding them are welcome.

23. 
    maflcko commented at 12:18 pm on June 5, 2025: member

    Not sure what you mean by “won’t work”. What is the exact diff to reproduce the compile failure or error?

    If you try to concatenate the RPC description with any of the output type string variables, you will get an empty output type string. The reason seems to be that they are not yet initialized when this operation occurs.

    Again, it would be good to have the exact code to reproduce. I guess you are talking about SIOF: https://en.cppreference.com/w/cpp/language/siof.html, which has solutions.

    No objection against doing stuff at compile time, but if there is no real reason and a lot of shuffling, I am not sure it provides value.

24. w0xlt force-pushed on Jun 18, 2025
25. w0xlt force-pushed on Jun 18, 2025
26. DrahtBot added the label CI failed on Jun 18, 2025
27. 
    DrahtBot commented at 6:50 pm on June 18, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/44365959476 LLM reason (✨ experimental): The CI failure is caused by a trailing whitespace issue flagged during the lint check.

    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.

28. w0xlt renamed this:
    wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`
    wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them
    on Jun 18, 2025
29. DrahtBot removed the label Needs rebase on Jun 18, 2025
30. in src/outputtype.cpp:60 in 76e0001015 outdated

    55+        result += "\"" + FormatOutputType(OUTPUT_TYPES[i]) + "\"";
    56+
    57+        if (i < count - 2) {
    58+            result += ", ";
    59+        } else if (i == count - 2) {
    60+            result += ", and ";
    

    ---

    ---

    

    maflcko commented at 8:06 pm on June 18, 2025:

    I’d say it is fine to just use Join(OUTPUT_TYPES, ", ", ...)

    ---

    

    w0xlt commented at 9:21 pm on June 18, 2025:

    Definitely better. Done in https://github.com/bitcoin/bitcoin/pull/32432/commits/8cc9845b8ddf4f93a02c622e7df8d1095dc1a640
31. 
    maflcko commented at 8:06 pm on June 18, 2025: member

    lgtm
32. DrahtBot removed the label CI failed on Jun 18, 2025
33. wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them 8cc9845b8d
34. w0xlt force-pushed on Jun 18, 2025
35. 
    maflcko commented at 7:23 am on June 19, 2025: member

    lgtm ACK 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640
36. DrahtBot requested review from musaHaruna on Jun 19, 2025
37. 
    achow101 commented at 8:43 pm on July 1, 2025: member

    ACK 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640
38. achow101 merged this on Jul 1, 2025
39. achow101 closed this on Jul 1, 2025

    ---

Contributors
w0xlt DrahtBot musaHaruna maflcko achow101

Review Requested
musaHaruna

Labels
Wallet RPC/REST/ZMQ