rpc: Fail to return undocumented return values #20459

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2011-rpcDoc changing 5 files +53 −7
  1. MarcoFalke commented at 10:51 AM on November 23, 2020: member

    Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476

    Fix this by treating it as an internal bug to return undocumented return values.

  2. MarcoFalke added the label RPC/REST/ZMQ on Nov 23, 2020
  3. MarcoFalke added the label Needs backport (0.21) on Nov 23, 2020
  4. laanwj commented at 11:14 AM on November 23, 2020: member

    Concept ACK but it would be much better if checks like this could be done at compile time instead of run time. This seems to me like a kind of a python-esque solution instead of a static language one.

  5. MarcoFalke commented at 11:44 AM on November 23, 2020: member

    UniValue::getType() is not a compile-time constant, so I don't think this works at compile time (without replacing UniValue with something else)

  6. MarcoFalke removed the label Needs backport (0.21) on Nov 23, 2020
  7. MarcoFalke force-pushed on Nov 23, 2020
  8. DrahtBot commented at 9:43 PM on November 23, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  9. amitiuttarwar commented at 11:31 PM on December 24, 2020: contributor

    concept ACK, very nice! having a programatic check that the docs are in sync with the actual output seems like a win for maintaining up-to-date docs & reducing review burden

  10. in src/rpc/mining.cpp:532 in fa6135b1cb outdated 
     552 | -                },
 553 | -                RPCResult{
 554 | -                    RPCResult::Type::OBJ, "", "",
 555 | +        },
 556 | +        {
 557 | +            RPCResult{"When mode=='proposal'", RPCResult::Type::STR, "", ""},
    sipa commented at 7:10 AM on December 27, 2020:

    Nit: using single quotes isn't valid JSON or C++. Maybe use double quotes?

    MarcoFalke commented at 9:06 AM on December 27, 2020:

    The existing docs use single quotes:

    https://github.com/bitcoin/bitcoin/blob/02cf20b9f571474c939d18a8b9d6b5d22479a222/src/rpc/mining.cpp#L508

    https://github.com/bitcoin/bitcoin/blob/02cf20b9f571474c939d18a8b9d6b5d22479a222/src/rpc/mining.cpp#L521

    So I used that. Also, a lot of RPC clients are implemented in python, which also allows single quotes.

  11. in src/rpc/blockchain.cpp:2181 in 9999759ecc outdated 
    2182 | -                },
2183 | -                RPCResult{
2184 | -                    RPCResult::Type::OBJ, "", "",
2185 | +        },
2186 | +        {
2187 | +            RPCResult{"When action=='abort'", RPCResult::Type::BOOL, "", ""},
    sipa commented at 7:11 AM on December 27, 2020:

    Same here: single quotes isn't valid JSON.

    MarcoFalke commented at 9:06 AM on December 27, 2020:

    #20459 (review)

  12. in src/rpc/util.cpp:754 in faca73c074 outdated 
     748 | +    case Type::STR:
 749 | +    case Type::STR_HEX: {
 750 | +        return UniValue::VSTR == result.getType();
 751 | +    }
 752 | +    case Type::NUM:
 753 | +    case Type::NUM_AMOUNT:
    sipa commented at 7:16 AM on December 27, 2020:

    Suggestion for a follow-up: for NUM_AMOUNT, you can do a stronger test (parse with AmountFromValue, convert back to JSON using ValueFromAmount, and the string representation must match exactly).

  13. sipa commented at 7:17 AM on December 27, 2020: member

    utACK faca73c07461f3344c7f88ea63727d9ac1c70b2a

  14. MarcoFalke marked this as a draft on Dec 27, 2020
  15. MarcoFalke marked this as ready for review on Mar 15, 2021
  16. MarcoFalke force-pushed on Mar 15, 2021
  17. MarcoFalke force-pushed on Mar 15, 2021
  18. MarcoFalke force-pushed on Mar 15, 2021
  19. MarcoFalke force-pushed on Mar 15, 2021
  20. in src/rpc/server.cpp:152 in fabf0515c6 outdated 
     148 | @@ -149,6 +149,7 @@ static RPCHelpMan help()
 149 |      }
 150 |      if (strCommand == "dump_all_command_conversions") {
 151 |          // Used for testing only, undocumented
 152 | +        const_cast<JSONRPCRequest&>(jsonRequest).mode = JSONRPCRequest::GET_ARGS;
    MarcoFalke commented at 9:57 AM on March 20, 2021:

    This is currently safe to do, because JSONRPCRequest isn't const where it is (copy-)constructed. I don't like the const_cast myself, but the only alternative I came up with was to change every rpc method with the new signature. Any other ideas? Maybe @ryanofsky

    ryanofsky commented at 2:19 AM on April 1, 2021:

    re: #20459 (review)

    This is currently safe to do, because JSONRPCRequest isn't const where it is (copy-)constructed. I don't like the const_cast myself, but the only alternative I came up with was to change every rpc method with the new signature. Any other ideas? Maybe @ryanofsky

    I think it would be better add ANY to list of acceptable result types: 4d692e282d404ce3d25a51261a1c6958ebd8b196 (tag). Advantages of this approach:

    • No need for const_cast
    • No need to change dumpArgMap implementation and make callers care how it works internally
    • GET_ARGS can still mean "get arguments" instead of "get arguments and ignore result type"
    MarcoFalke commented at 10:24 AM on April 1, 2021:

    Thanks, done

  21. DrahtBot added the label Needs rebase on Mar 31, 2021
  22. ryanofsky approved
  23. ryanofsky commented at 2:23 AM on April 1, 2021: member

    Code review ACK fabf0515c6c58096aafb6685daf1830a34bf5c75. Suggested a tweak to get rid of const_cast, but current implementation also looks good, so feel free to ignore suggestion.

  24. rpc: Fail to return undocumented return values fa8192f42e
  25. MarcoFalke force-pushed on Apr 1, 2021
  26. DrahtBot removed the label Needs rebase on Apr 1, 2021
  27. ryanofsky approved
  28. ryanofsky commented at 2:11 PM on April 1, 2021: member

    Code review ACK fa8192f42e1d24444f1d0433c96dbce1adf76967. Only changes: rebase, no const_cast suggestion, and tostring cleanups needed after suggestion

  29. MarcoFalke merged this on Apr 3, 2021
  30. MarcoFalke closed this on Apr 3, 2021
  31. MarcoFalke deleted the branch on Apr 3, 2021
  32. sidhujag referenced this in commit c38b140703 on Apr 3, 2021
  33. DrahtBot locked this on Aug 18, 2022
Contributors
MarcoFalkelaanwjDrahtBotamitiuttarwarsiparyanofsky
Labels
RPC/REST/ZMQ
Linked (view graph)
#18476 bitcoin-cli returns no error on unsuccessful gettxout call
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-05-02 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me