rpc: Put undocumented JSON failure mode behind a runtime flag #25161

pull suhailsaqan wants to merge 1 commits into bitcoin:master from suhailsaqan:master changing 4 files +8 −1
  1. suhailsaqan commented at 9:49 PM on May 17, 2022: contributor

    Fixes #24695 (Put undocumented JSON failure mode behind a runtime flag)

  2. in src/init.cpp:573 in 5a90593462 outdated 
     569 | @@ -570,6 +570,7 @@ void SetupServerArgs(ArgsManager& argsman)
 570 |      argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 571 |      argsman.AddArg("-rpcauth=<userpw>", "Username and HMAC-SHA-256 hashed password for JSON-RPC connections. The field <userpw> comes in the format: <USERNAME>:<SALT>$<HASH>. A canonical python script is included in share/rpcauth. The client then connects normally using the rpcuser=<USERNAME>/rpcpassword=<PASSWORD> pair of arguments. This option can be specified multiple times", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 572 |      argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 573 | +    argsman.AddArg("-rpcshowerror", "Whether or not to throw a non-fatal error at runtime if the documentation for an RPC is incorrect. Default is false.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
    luke-jr commented at 11:09 PM on May 17, 2022:

    ArgsManager::DEBUG_ONLY

    luke-jr commented at 11:17 PM on May 17, 2022:

    Maybe -rpcdoccheck would be better?

  3. in src/wallet/rpc/wallet.cpp:59 in 5a90593462 outdated 
      55 | @@ -56,7 +56,7 @@ static RPCHelpMan getwalletinfo()
  56 |                          {
  57 |                              {RPCResult::Type::NUM, "duration", "elapsed seconds since scan start"},
  58 |                              {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"},
  59 | -                        }, /*skip_type_check=*/true},
    luke-jr commented at 11:18 PM on May 17, 2022:

    I think we still need this?

    suhailsaqan commented at 11:34 PM on May 17, 2022:

    I think we still need this?

    Wouldn't it be invalid because I took out the skip_type_check in src/rpc/util.h?

    luke-jr commented at 12:11 AM on May 18, 2022:

    Yeah, you'll have to not do that ;)

    suhailsaqan commented at 12:40 AM on May 18, 2022:

    Gotchu, I think that works now?

  4. luke-jr changes_requested
  5. DrahtBot added the label RPC/REST/ZMQ on May 17, 2022
  6. DrahtBot added the label Wallet on May 17, 2022
  7. in src/init.cpp:573 in 8c7b684420 outdated 
     569 | @@ -570,6 +570,7 @@ void SetupServerArgs(ArgsManager& argsman)
 570 |      argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 571 |      argsman.AddArg("-rpcauth=<userpw>", "Username and HMAC-SHA-256 hashed password for JSON-RPC connections. The field <userpw> comes in the format: <USERNAME>:<SALT>$<HASH>. A canonical python script is included in share/rpcauth. The client then connects normally using the rpcuser=<USERNAME>/rpcpassword=<PASSWORD> pair of arguments. This option can be specified multiple times", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 572 |      argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 573 | +    argsman.AddArg("-rpcdoccheck", "Whether or not to throw a non-fatal error at runtime if the documentation for an RPC is incorrect. Default is false.", ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
    luke-jr commented at 12:46 AM on May 18, 2022:

    Usually the description is written as a positive (what happens if you set it). Also, default should match the format of other boolean options.

    suhailsaqan commented at 1:14 AM on May 18, 2022:

    Usually the description is written as a positive (what happens if you set it). Also, default should match the format of other boolean options.

    Fixed and squashed previous commits.

  8. suhailsaqan force-pushed on May 18, 2022
  9. in src/init.cpp:573 in 0d4bdec0d4 outdated 
     569 | @@ -570,6 +570,7 @@ void SetupServerArgs(ArgsManager& argsman)
 570 |      argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 571 |      argsman.AddArg("-rpcauth=<userpw>", "Username and HMAC-SHA-256 hashed password for JSON-RPC connections. The field <userpw> comes in the format: <USERNAME>:<SALT>$<HASH>. A canonical python script is included in share/rpcauth. The client then connects normally using the rpcuser=<USERNAME>/rpcpassword=<PASSWORD> pair of arguments. This option can be specified multiple times", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 572 |      argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 573 | +    argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
    luke-jr commented at 1:54 AM on May 18, 2022:

    I think you still need ALLOW_ANY

  10. in src/rpc/util.h:25 in 0d4bdec0d4 outdated 
      21 | @@ -22,6 +22,8 @@
  22 |  #include <variant>
  23 |  #include <vector>
  24 |  
  25 | +static const bool DEFAULT_RPC_DOC_CHECK=false;
    luke-jr commented at 1:55 AM on May 18, 2022: 
    static constexpr bool DEFAULT_RPC_DOC_CHECK{false};
  11. luke-jr changes_requested
  12. in src/init.cpp:573 in 910e787545 outdated 
     569 | @@ -570,7 +570,7 @@ void SetupServerArgs(ArgsManager& argsman)
 570 |      argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24). This option can be specified multiple times", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
 571 |      argsman.AddArg("-rpcauth=<userpw>", "Username and HMAC-SHA-256 hashed password for JSON-RPC connections. The field <userpw> comes in the format: <USERNAME>:<SALT>$<HASH>. A canonical python script is included in share/rpcauth. The client then connects normally using the rpcuser=<USERNAME>/rpcpassword=<PASSWORD> pair of arguments. This option can be specified multiple times", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 572 |      argsman.AddArg("-rpcbind=<addr>[:port]", "Bind to given address to listen for JSON-RPC connections. Do not expose the RPC server to untrusted networks such as the public internet! This option is ignored unless -rpcallowip is also passed. Port is optional and overrides -rpcport. Use [host]:port notation for IPv6. This option can be specified multiple times (default: 127.0.0.1 and ::1 i.e., localhost)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
 573 | -    argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
 574 | +    argsman.AddArg("-rpcdoccheck", strprintf("Throw a non-fatal error at runtime if the documentation for an RPC is incorrect (default: %u)", DEFAULT_RPC_DOC_CHECK), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
    luke-jr commented at 2:23 AM on May 18, 2022:

    both, look at the other debug options

  13. MarcoFalke commented at 5:47 AM on May 18, 2022: member

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

  14. luke-jr approved
  15. luke-jr commented at 5:51 AM on May 18, 2022: member

    utACK after squashing

  16. w0xlt commented at 6:02 AM on May 18, 2022: contributor

    Approach ACK

  17. suhailsaqan commented at 6:23 AM on May 18, 2022: contributor

    It says I can't squash because "A merge commit cannot exist among those commits". Would anyone know how to fix that?

  18. MarcoFalke commented at 7:15 AM on May 18, 2022: member

    See https://github.com/bitcoin/bitcoin/pull/25165

  19. suhailsaqan commented at 7:51 AM on May 18, 2022: contributor

    When I do git merge master it says Already up to date.. Am I not following it correctly?

    So the current situation I'm in is that the changes are committed on my master branch. So I did git checkout master and git fetch -all but then not sure if git merge master is the correct thing to do next?

  20. vincenzopalazzo commented at 8:41 AM on May 18, 2022: none

    Concept ACK

  21. in src/rpc/util.cpp:865 in 162346de57 outdated 
     861 | @@ -861,7 +862,7 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
 862 |  
 863 |  bool RPCResult::MatchesType(const UniValue& result) const
 864 |  {
 865 | -    if (m_skip_type_check) {
 866 | +    if (!gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK) || m_skip_type_check) {
    MarcoFalke commented at 12:43 PM on May 18, 2022:

    nit: It might be better to put this into RPCHelpMan::HandleRequest, so that iteration over the results can be skipped.

    suhailsaqan commented at 4:30 PM on May 18, 2022:

    Just pushed changes. I believe it works but not entirely sure.

  22. in src/rpc/util.h:25 in 162346de57 outdated 
      21 | @@ -22,6 +22,8 @@
  22 |  #include <variant>
  23 |  #include <vector>
  24 |  
  25 | +static constexpr bool DEFAULT_RPC_DOC_CHECK{false};
    MarcoFalke commented at 12:45 PM on May 18, 2022:

    Now that this is off by default, it needs to be enabled in the tests.

    See def write_config.

  23. MarcoFalke commented at 4:52 PM on May 18, 2022: member

    I've reworked #25165 . Hopefully it should be clearer now?

  24. suhailsaqan force-pushed on May 18, 2022
  25. MarcoFalke removed the label Wallet on May 18, 2022
  26. rpc: Put undocumented JSON failure mode behind a runtime flag
    rpc: Put undocumented JSON failure mode behind a runtime flag
    b953ea6cc6
  27. suhailsaqan force-pushed on May 18, 2022
  28. luke-jr approved
  29. luke-jr commented at 6:00 PM on May 18, 2022: member

    utACK b953ea6cc691ba61bf08eb186e76e7e8b7ba8120

  30. vincenzopalazzo approved
  32. MarcoFalke merged this on May 19, 2022
  33. MarcoFalke closed this on May 19, 2022
  34. luke-jr referenced this in commit f83f753e0c on May 21, 2022
  35. sidhujag referenced this in commit bb830284e8 on May 28, 2022
  36. DrahtBot locked this on May 19, 2023
Contributors
suhailsaqanluke-jrMarcoFalkew0xltvincenzopalazzo
Labels
RPC/REST/ZMQ
Linked (view graph)
#24695 Put undocumented JSON failure mode behind a runtime flag
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: 2026-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me