rpc: Use type-safe exception to pass RPC help #32660

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2506-rpc-help changing 3 files +11 −11
  1. maflcko commented at 12:10 pm on June 2, 2025: member

    The current “catch-all” catch (const std::exception& e) in CRPCTable::help is problematic, because it could catch exceptions unrelated to passing the help string up.

    Fix this by using a dedicated exception type.

  2. DrahtBot commented at 12:10 pm on June 2, 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/32660.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, rkrux

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

  3. DrahtBot added the label RPC/REST/ZMQ on Jun 2, 2025
  4. maflcko commented at 12:13 pm on June 2, 2025: member

    Can be tested by adding a bug, like

     0diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp
 1index 5597f8d237..d62893bf0c 100644
 2--- a/src/rpc/signmessage.cpp
 3+++ b/src/rpc/signmessage.cpp
 4@@ -19,7 +19,7 @@ static RPCHelpMan verifymessage()
 5     return RPCHelpMan{"verifymessage",
 6         "Verify a signed message.",
 7         {
 8-            {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
 9+            {"addres\ns", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
10             {"signature", RPCArg::Type::STR, RPCArg::Optional::NO, "The signature provided by the signer in base 64 encoding (see signmessage)."},
11             {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message that was signed."},
12         },

    On master, the bug is hidden. On this pull, it is more obvious. For example, ./test/functional/rpc_help.py or the help RPC fail.

  5. in src/rpc/util.h:72 in fa0cf42380 outdated 
    66@@ -67,6 +67,10 @@ class FillableSigningProvider;
67 class CScript;
68 struct Sections;
69 
70+struct HelpException : std::runtime_error {
71+    explicit HelpException(const std::string& msg) : std::runtime_error{msg} {}
72+};
    l0rinc commented at 2:31 pm on June 2, 2025:

    nits:

    • formatting is usually different for classes/structs - but maybe this style was chosen to align with the rest of the file
    • can be final
    0struct HelpException final : std::runtime_error
1{
2    explicit HelpException(const std::string& msg) : std::runtime_error{msg} {}
3};
    maflcko commented at 3:34 pm on June 2, 2025:
    I am just using clang-format, so if you want to change that, please do so in https://github.com/bitcoin/bitcoin/blob/1c6602399be6de0148938a3fd7b51e6c48b985d2/src/.clang-format#L17-L20
    l0rinc commented at 9:14 am on June 3, 2025:
    weird, I thought formatter falls back to AfterClass (my IDE delegates to clang-format and does that) - see #32414 (review)
    maflcko commented at 9:37 am on June 3, 2025:
    I am just using vanilla clang-format, but if there is an interaction issue with an IDE, it could make sense to set AfterStruct to false or true explicitly. Happy to review such a pull.
    l0rinc commented at 9:44 am on June 3, 2025:
    I see there isn’t a clear winner in the source code for where struct braces should be placed - do you think we should format them differently than classes? If we want to unify the formatting with classes, are you willing to review a scripted diff formatting the existing struct braces (whitespace only)?
    maflcko commented at 9:47 am on June 3, 2025:
    No opinion from my side. I think anything is fine here
  6. in src/rpc/server.cpp:89 in fa0cf42380 outdated 
    88     jreq.params = UniValue();
89 
90-    for (const std::pair<std::string, const CRPCCommand*>& command : vCommands)
91-    {
92-        const CRPCCommand *pcmd = command.second;
93+    for (const auto& [_, pcmd] : vCommands) {
    l0rinc commented at 2:34 pm on June 2, 2025:

    👍, alternatively, if you think it’s clearer:

    0    for (const auto& pcmd : vCommands | std::views::values) {
    maflcko commented at 7:34 am on June 3, 2025:
    Heh, wasn’t aware of it, but leaving as-is for now.
    rkrux commented at 2:36 pm on June 3, 2025:
    TIL, ty!
  7. l0rinc approved
  8. l0rinc commented at 2:35 pm on June 2, 2025: contributor
    code review ACK fa0cf42380dec17e73d38aa69dd70662a0bc344a
  9. achow101 commented at 8:40 pm on June 2, 2025: member
    It doesn’t seem like this is catching the issue in #29136 (review)? I tried making a similar change to a RPC in this PR and the test still passed.
  10. maflcko commented at 5:46 am on June 3, 2025: member

    It doesn’t seem like this is catching the issue in #29136 (comment)? I tried making a similar change to a RPC in this PR and the test still passed.

    Are you sure? What are the exact steps to reproduce?

    Locally such a bug passes for me on current master:

     0$ git checkout bitcoin-core/master && git diff && cmakeb && ./bld-cmake/test/functional/rpc_help.py 
 1M	src/rpc/signmessage.cpp
 2HEAD is now at 88b22acc3d Merge bitcoin/bitcoin#32528: rpc: Round verificationprogress to 1 for a recent tip
 3
 4
 5diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp
 6index 5597f8d237..18af6d110e 100644
 7--- a/src/rpc/signmessage.cpp
 8+++ b/src/rpc/signmessage.cpp
 9@@ -17,7 +17,7 @@
10 static RPCHelpMan verifymessage()
11 {
12     return RPCHelpMan{"verifymessage",
13-        "Verify a signed message.",
14+        "\nVerify a signed message.",
15         {
16             {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
17             {"signature", RPCArg::Type::STR, RPCArg::Optional::NO, "The signature provided by the signer in base 64 encoding (see signmessage)."},

    whereas on this pull, the same diff fails.

  11. fanquake commented at 7:23 am on June 3, 2025: member

    whereas on this pull, the same diff fails.

    Work for me:

     0./build/test/functional/rpc_help.py                        
 12025-06-03T07:21:05.244000Z TestFramework (INFO): PRNG seed is: 8715551814237508387
 22025-06-03T07:21:05.244000Z TestFramework (INFO): Initializing test directory /var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/bitcoin_func_test_ks0xf8o9
 32025-06-03T07:21:06.291000Z TestFramework (ERROR): JSONRPC error
 4Traceback (most recent call last):
 5  File "/bitcoin/test/functional/test_framework/test_framework.py", line 189, in main
 6    self.run_test()
 7    ~~~~~~~~~~~~~^^
 8  File "/bitcoin/./build/test/functional/rpc_help.py", line 53, in run_test
 9    self.test_categories()
10    ~~~~~~~~~~~~~~~~~~~~^^
11  File "/bitcoin/./build/test/functional/rpc_help.py", line 102, in test_categories
12    titles = [line[3:-3] for line in node.help().splitlines() if line.startswith('==')]
13                                     ~~~~~~~~~^^
14  File "/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
15    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
16  File "/bitcoin/test/functional/test_framework/authproxy.py", line 151, in __call__
17    raise JSONRPCException(response['error'], status)
18test_framework.authproxy.JSONRPCException: Internal bug detected: !m_description.starts_with('\n')
19../../src/rpc/util.cpp:796 (ToString)
20Bitcoin Core v29.99.0-6497cef9bdc8-dirty
21Please report this issue here: https://github.com/bitcoin/bitcoin/issues
22 (-1)
232025-06-03T07:21:06.343000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.

    0./build/bin/bitcoin-cli help    
1error code: -1
2error message:
3Internal bug detected: !m_description.starts_with('\n')
4../../src/rpc/util.cpp:796 (ToString)
5Bitcoin Core v29.99.0-6497cef9bdc8-dirty
6Please report this issue here: https://github.com/bitcoin/bitcoin/issues
  12. in src/rpc/util.cpp:648 in fa0cf42380 outdated 
    644@@ -645,7 +645,7 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
645      * the user is asking for help information, and throw help when appropriate.
646      */
647     if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
648-        throw std::runtime_error(ToString());
649+        throw HelpException{ToString()};
    rkrux commented at 10:18 am on June 3, 2025:
    Ah! Took me some time to understand how this fix worked. In case of erroneous help text, a NonFatalCheckError exception is thrown from here that was earlier caught by the catch-all handler in server.cpp. Now only HelpException is caught and the rest, including NonFatalCheckError, is bubbled up all the way to the RPC response. https://github.com/bitcoin/bitcoin/blob/e872a566f251c73908de8b6d243c94a6679c2eac/src/rpc/util.cpp#L795-L796
    rkrux commented at 10:26 am on June 3, 2025:

    Nit: Maybe returning the result by throwing an exception is obvious to others but it wasn’t to me. Does it necessarily need to be called HelpException? Calling it HelpResult might make it more explicit or HelpResultException (not my preference).

     0diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
 1index d5c790e89b..bd99615bbf 100644
 2--- a/src/rpc/server.cpp
 3+++ b/src/rpc/server.cpp
 4@@ -96,7 +96,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
 5             UniValue unused_result;
 6             if (setDone.insert(pcmd->unique_id).second)
 7                 pcmd->actor(jreq, unused_result, /*last_handler=*/true);
 8-        } catch (const HelpException& e) {
 9+        } catch (const HelpResult& e) {
10             std::string strHelp{e.what()};
11             if (strCommand == "")
12             {
13diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
14index 75b801f2bf..5da02b4df4 100644
15--- a/src/rpc/util.cpp
16+++ b/src/rpc/util.cpp
17@@ -645,7 +645,7 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
18      * the user is asking for help information, and throw help when appropriate.
19      */
20     if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
21-        throw HelpException{ToString()};
22+        throw HelpResult{ToString()};
23     }
24     UniValue arg_mismatch{UniValue::VOBJ};
25     for (size_t i{0}; i < m_args.size(); ++i) {
26diff --git a/src/rpc/util.h b/src/rpc/util.h
27index 0a86768814..1650d2da1f 100644
28--- a/src/rpc/util.h
29+++ b/src/rpc/util.h
30@@ -67,8 +67,8 @@ class FillableSigningProvider;
31 class CScript;
32 struct Sections;
33 
34-struct HelpException : std::runtime_error {
35-    explicit HelpException(const std::string& msg) : std::runtime_error{msg} {}
36+struct HelpResult : std::runtime_error {
37+    explicit HelpResult(const std::string& msg) : std::runtime_error{msg} {}
38 };
    maflcko commented at 10:38 am on June 3, 2025:
    thx, I may change to HelpResult, if I have to re-touch for other reasons.
    maflcko commented at 9:40 am on June 12, 2025:
    done style-nit. Should be easy to re-review via git range-diff bitcoin-core/master fa0cf42380 fa946520d2.
  13. rkrux commented at 10:32 am on June 3, 2025: contributor

    ACK fa0cf42380dec17e73d38aa69dd70662a0bc344a

     0➜  bitcoin git:(2506-rpc-help) ✗ git diff --color | cat
 1diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp
 2index 5597f8d237..18af6d110e 100644
 3--- a/src/rpc/signmessage.cpp
 4+++ b/src/rpc/signmessage.cpp
 5@@ -17,7 +17,7 @@
 6 static RPCHelpMan verifymessage()
 7 {
 8     return RPCHelpMan{"verifymessage",
 9-        "Verify a signed message.",
10+        "\nVerify a signed message.",
11         {
12             {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
13             {"signature", RPCArg::Type::STR, RPCArg::Optional::NO, "The signature provided by the signer in base 64 encoding (see signmessage)."},

     0➜  bitcoin git:(2506-rpc-help) ✗ bitcoinclireg help verifymessage  
 1error code: -1
 2error message:
 3Internal bug detected: !m_description.starts_with('\n')
 4rpc/util.cpp:796 (ToString)
 5Bitcoin Core v29.99.0-fa0cf42380de-dirty
 6Please report this issue here: https://github.com/bitcoin/bitcoin/issues
 7
 8➜  bitcoin git:(2506-rpc-help) ✗ bitcoinclireg help
 9error code: -1
10error message:
11Internal bug detected: !m_description.starts_with('\n')
12rpc/util.cpp:796 (ToString)
13Bitcoin Core v29.99.0-fa0cf42380de-dirty
14Please report this issue here: https://github.com/bitcoin/bitcoin/issues

     0➜  bitcoin git:(master) ✗ bitcoinclireg help verifymessage
 1Internal bug detected: !m_description.starts_with('\n')
 2rpc/util.cpp:796 (ToString)
 3Bitcoin Core v29.99.0-4b1d48a6866b-dirty
 4Please report this issue here: https://github.com/bitcoin/bitcoin/issues
 5
 6➜  bitcoin git:(master) ✗ bitcoinclireg help
 7...
 8...
 9signmessagewithprivkey "privkey" "message"
10validateaddress "address"
11Internal bug detected: !m_description.starts_with('\n')
12
13== Wallet ==
14abandontransaction "txid"

    The rpc_help functional test also fails with the diff in the branch but not in master.

  14. achow101 commented at 9:43 pm on June 3, 2025: member

    Are you sure? What are the exact steps to reproduce?

    Nope.

    Not sure what I did wrong yesterday, but tried again and it does seem to work.

  15. l0rinc commented at 8:24 am on June 4, 2025: contributor

    I retested the scenario with "address\n;)", running

    0cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug && ninja -C build && build/test/functional/test_runner.py  rpc_help

    and got:

    0Internal bug detected: s.m_left.find('\n') == std::string::npos

    on master it’s:

    0rpc_help.py | ✓ Passed

    👍

  16. maflcko renamed this:
    rpc: Use type-safe HelpException
    rpc: Use type-safe exception to pass RPC help
    on Jun 12, 2025
  17. rpc: Use type-safe exception to pass RPC help eeeec1579e
  18. refactor: Use structured binding for-loop fa946520d2
  19. maflcko force-pushed on Jun 12, 2025
  20. l0rinc commented at 10:05 am on June 12, 2025: contributor

    tested ACK fa946520d229ae45b30519bccc9eaa2c47b4a093 (edited)

    Reapplied the tests from #32660 (comment) - passes without the changes, fails with the change as expected!

    git fetch origin fa0cf42380dec17e73d38aa69dd70662a0bc344a fa946520d229ae45b30519bccc9eaa2c47b4a093 git range-diff master fa946520d229ae45b30519bccc9eaa2c47b4a093 fa0cf42380dec17e73d38aa69dd70662a0bc344a

     01:  fa97f9b78e ! 1:  eeeec1579e rpc: Use type-safe HelpException
 1@@ Metadata
 2 Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
 3 
 4## Commit message ##
 5-    rpc: Use type-safe HelpException
 6+    rpc: Use type-safe exception to pass RPC help
 7 
 8## src/rpc/server.cpp ##
 9@@ src/rpc/server.cpp: std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
10@@ src/rpc/server.cpp: std::string CRPCTable::help(const std::string& strCommand, c
11 -        {
12 -            // Help text is returned in an exception
13 -            std::string strHelp = std::string(e.what());
14-+        } catch (const HelpException& e) {
15++        } catch (const HelpResult& e) {
16 +            std::string strHelp{e.what()};
17              if (strCommand == "")
18              {
19@@ src/rpc/util.cpp: UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& reque
20       */
21      if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
22 -        throw std::runtime_error(ToString());
23-+        throw HelpException{ToString()};
24++        throw HelpResult{ToString()};
25      }
26      UniValue arg_mismatch{UniValue::VOBJ};
27      for (size_t i{0}; i < m_args.size(); ++i) {
28@@ src/rpc/util.h: class FillableSigningProvider;
29  class CScript;
30  struct Sections;
31  
32-+struct HelpException : std::runtime_error {
33-+    explicit HelpException(const std::string& msg) : std::runtime_error{msg} {}
34++struct HelpResult : std::runtime_error {
35++    explicit HelpResult(const std::string& msg) : std::runtime_error{msg} {}
36 +};
37 +
38  /**
392:  fa0cf42380 = 2:  fa946520d2 refactor: Use structured binding for-loop
  21. DrahtBot requested review from rkrux on Jun 12, 2025
  22. rkrux approved
  23. rkrux commented at 11:07 am on June 12, 2025: contributor

    re-ACK fa946520d229ae45b30519bccc9eaa2c47b4a093

    0git range-diff fa0cf42...fa94652

    Thanks for accepting the naming suggestion.


maflcko DrahtBot l0rinc rkrux achow101 fanquake

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-06-15 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me