refactor: Run type check against RPCArgs (1/2) #26039

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2209-rpc-type-🔤 changing 14 files +85 −155
  1. maflcko commented at 6:00 PM on September 7, 2022: member

    It seems brittle to require RPCTypeCheck being called inside the code logic. Without compile-time enforcement this will lead to places where it is forgotten and thus to inconsistencies and bugs. Fix this by removing the calls to RPCTypeCheck and doing the check internally.

    The changes should be reviewed as refactoring changes. However, if they change behavior, it will be a bugfix. For example the changes here happen to also detect/fix bugs like the one fixed in commit 3b5fb6e77a93f58b3d03b1eec3595f5c45e633a9.

  2. glozow added the label RPC/REST/ZMQ on Sep 8, 2022
  3. DrahtBot commented at 5:27 PM on September 8, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ajtowns
    Concept ACK fanquake, luke-jr, w0xlt
    Stale ACK furszy

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)
    • #26469 (rpc: getblock: implement with block height as input parameter. by russeree)
    • #25943 (rpc: Add a parameter to sendrawtransaction which sets a maximum burned output for OP_RETURN transactions. by davidgumberg)
    • #25939 (rpc: In utxoupdatepsbt also look for the tx in the txindex by ishaanam)
    • #25796 (rpc: add descriptorprocesspsbt rpc by ishaanam)
    • #25429 (refactor: Avoid UniValue copies by MarcoFalke)
    • #25344 (New outputs argument for bumpfee/psbtbumpfee by rodentrabies)
    • #25093 (rpc: Check for omitted, but required parameters by MarcoFalke)
    • #21283 (Implement BIP 370 PSBTv2 by achow101)

    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. amovfx commented at 1:28 AM on September 9, 2022: none

    Is this something that I can help out on? I'm looking for another task. I don't know of the etiquette of hoping on a drafted pull request.

  5. amovfx commented at 1:41 AM on September 9, 2022: none

    From what I gather this is a large refactor with the removal of the RPCTypeCheck functions all over the place?

  6. fanquake commented at 9:33 AM on September 10, 2022: member

    Concept ACK

  7. luke-jr commented at 11:34 PM on September 10, 2022: member

    Concept ACK, but this feels like it's doing too much additional. It might be nice to have something minimal we can backport?

  8. maflcko commented at 6:45 AM on September 12, 2022: member

    Not sure if we want to backport this, but this should already be minimal. A fragile alternative would be to add RPCTypeCheck to every RPC that is missing it, with no compile-time guarantee that all have been covered.

  9. w0xlt commented at 8:33 AM on September 13, 2022: contributor

    Concept ACK.

  10. DrahtBot added the label Needs rebase on Sep 13, 2022
  11. maflcko force-pushed on Sep 13, 2022
  12. DrahtBot removed the label Needs rebase on Sep 13, 2022
  13. in src/rpc/util.cpp:700 in f03877ff32 outdated 
     702 | +        return;
 703 | +    }
 704 | +    case Type::ARR: {
 705 | +        RPCTypeCheckArgument(request, UniValue::VARR);
 706 | +        return;
 707 | +    }
    furszy commented at 4:09 PM on September 14, 2022:

    What about doing something like this:

    void RPCArg::MatchesType(const UniValue& request) const
{
    if(m_skip_type_check) return;
    if (IsOptional() && request.isNull()) return;
    // Special cases
    if (m_type == Type::AMOUNT || m_type == Type::RANGE) {
        // Type::AMOUNT: VNUM or VSTR, checked inside AmountFromValue()
        // Type::RANGE: VNUM or VARR, checked inside ParseRange()
        return;
    }
    // Check and throw exception if type is invalid
    UniValue::VType type = ToUnivalueType(m_type);
    request.checkType(type);
}

    So we define the univalue type error string only once (inside Univalue::checkType) and not twice (Univalue ::checkType and RPCTypeCheckArgument). Which will let us not have to worry anymore about stuff like #26069 and remove RPCTypeCheckArgument.

    maflcko commented at 4:38 PM on September 14, 2022:

    If we decide in the future to also check for hex strings, that wouldn't work, because json (and univalue) only knowns strings, not hex string.

  14. luke-jr commented at 3:21 AM on September 21, 2022: member

    Not sure if we want to backport this, but this should already be minimal. A fragile alternative would be to add RPCTypeCheck to every RPC that is missing it, with no compile-time guarantee that all have been covered.

    Shouldn't we be able to just adjust the return code?

  15. maflcko commented at 6:43 AM on September 21, 2022: member

    Yes, see #25737 (rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR by furszy)

  16. fanquake referenced this in commit b1f44ecdcd on Sep 21, 2022
  17. DrahtBot added the label Needs rebase on Sep 21, 2022
  18. sidhujag referenced this in commit c366c50c21 on Sep 23, 2022
  19. maflcko force-pushed on Sep 29, 2022
  20. DrahtBot removed the label Needs rebase on Sep 29, 2022
  21. DrahtBot added the label Needs rebase on Sep 30, 2022
  22. maflcko force-pushed on Sep 30, 2022
  23. maflcko renamed this:
    rpc: Return RPC_TYPE_ERROR, not RPC_MISC_ERROR on type mismatch (1/2)
    rpc: Run type check against RPCArgs (1/2)
    on Sep 30, 2022
  24. maflcko force-pushed on Sep 30, 2022
  25. maflcko marked this as ready for review on Sep 30, 2022
  26. maflcko commented at 11:02 AM on September 30, 2022: member

    Rebased, changed title, rewritten description

  27. DrahtBot removed the label Needs rebase on Sep 30, 2022
  28. in src/rpc/blockchain.cpp:653 in fa3a7b6fff outdated 
     631 | @@ -637,7 +632,8 @@ static RPCHelpMan getblock()
 632 |                  "If verbosity is 3, returns an Object with information about block <hash> and information about each transaction, including prevout information for inputs (only for unpruned blocks in the current best chain).\n",
 633 |                  {
 634 |                      {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
 635 | -                    {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a JSON object, 2 for JSON object with transaction data, and 3 for JSON object with transaction data including prevout information for inputs"},
 636 | +                    {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a JSON object, 2 for JSON object with transaction data, and 3 for JSON object with transaction data including prevout information for inputs",
 637 | +                     RPCArgOptions{.skip_type_check = true}},
    furszy commented at 1:19 PM on November 4, 2022:

    Not for this PR, but since the verbosity field was changed from a bool to an int in #8704 (2017) and the bool behavior is not mentioned anywhere in the command help description. What if we cleanup the bool stuff and enable the arg type check?

    maflcko commented at 11:18 AM on November 14, 2022:

    No opinion, but as you mention, seems best to not touch here.

  29. in src/rpc/blockchain.cpp:875 in fa3a7b6fff outdated 
     851 | @@ -856,7 +852,11 @@ static RPCHelpMan gettxoutsetinfo()
 852 |                  "Note this call may take some time if you are not using coinstatsindex.\n",
 853 |                  {
 854 |                      {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
 855 | -                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the current best block"}, "The block hash or height of the target height (only available with coinstatsindex).", RPCArgOptions{.type_str={"", "string or numeric"}}},
 856 | +                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the current best block"}, "The block hash or height of the target height (only available with coinstatsindex).",
 857 | +                     RPCArgOptions{
 858 | +                         .skip_type_check = true,
 859 | +                         .type_str = {"", "string or numeric"},
 860 | +                     }},
    furszy commented at 1:45 PM on November 4, 2022:

    we could make the Type field in RPCArg a bitfield, then be able to verify if the type is either a string or a num.

    maflcko commented at 11:37 AM on November 14, 2022:

    Seems unrelated to the changes here, but my preferences would be:

    • Introduce a specific type for block heights
    • Make the type a full class to be able to build any type on demand
    ajtowns commented at 2:08 PM on January 13, 2023:

    Keeping the RPC API simpler seems better than adding too much cleverness/complexity -- you can always convert height to a hash on the client side in advance...

  30. furszy commented at 1:48 PM on November 4, 2022: member

    Code ACK fa3a7b6f

    Note for reviewers: Even when this PR removes the RPCTypeCheck calls from each of the RPC commands and scopes the check inside the the -rpcdoccheck flag, the user will still receive the type mismatch error message from the inner univalue checkType function.

  31. maflcko force-pushed on Nov 14, 2022
  32. maflcko commented at 11:45 AM on November 14, 2022: member

    Even when this PR removes the RPCTypeCheck calls from each of the RPC commands and scopes the check inside the the -rpcdoccheck flag

    Oh, this is actually a bug, as the check should run unconditionally. Fixed and thanks for the prompt.

  33. in src/rpc/util.cpp:673 in faa8b760b8 outdated 
     671 | +    if (m_opts.skip_type_check) return;
 672 | +    if (IsOptional() && request.isNull()) return;
 673 | +    switch (m_type) {
 674 | +    case Type::STR_HEX:
 675 | +    case Type::STR: {
 676 | +        RPCTypeCheckArgument(request, UniValue::VSTR);
    furszy commented at 1:30 PM on November 14, 2022:

    Small cleanup idea: what if we remove RPCTypeCheckArgument and just call request.checkType(UniValue::VSTR)? (would just need to make checkType public)

    maflcko commented at 1:36 PM on November 14, 2022:

    checkType is used for json type checks. As mentioned previously you wouldn't be able to check for other types, such as hex strings (https://github.com/bitcoin/bitcoin/pull/26039#discussion_r971058812) in the future. So I'd like to leave them copied for now. If you feel strongly, it might be better to do the replacement in RPCTypeCheckArgument, which can be done separate form this pull request?

    furszy commented at 1:59 PM on November 14, 2022:

    ha, I completely forgot that I mentioned it before. Sorry for that. nvm then.

  34. in src/rpc/util.cpp:567 in faa8b760b8 outdated 
     561 | @@ -579,6 +562,12 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
 562 |      if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
 563 |          throw std::runtime_error(ToString());
 564 |      }
 565 | +    {
 566 | +        size_t i{0};
 567 | +        for (const auto& arg : m_args) {
 568 | +            arg.MatchesType(request.params[i++]);
 569 | +        }
 570 | +    }
    furszy commented at 10:03 PM on November 15, 2022:

    In faa8b760: nit: could remove the extra brackets.

    maflcko commented at 9:04 AM on November 16, 2022:

    They are needed to scope i

    maflcko commented at 12:57 PM on December 12, 2022:

    Thx, removed

  35. in src/rpc/fees.cpp:158 in faa8b760b8 outdated 
     152 | @@ -155,8 +153,6 @@ static RPCHelpMan estimaterawfee()
 153 |          },
 154 |          [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 155 |          {
 156 | -            RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VNUM}, true);
    furszy commented at 10:06 PM on November 15, 2022:

    Small topic about this: The last arg from RPCTypeCheck was fAllowNull, which was true here (so the two args were nullable). Now.. if we go to the arg definition, conf_target has RPCArg::Optional::NO. Which means the opposite.

    maflcko commented at 1:36 PM on December 12, 2022:

    fAllowNull must be true if any arg is optional. MatchesType added in this pull request is calling IsOptional on itself (the arg).

    Even though fAllowNull is set, conf_target could not be null previously, as it is checked when read. This behavior is preserved in this refactoring commit.

  36. in src/wallet/rpc/backup.cpp:1335 in faa8b760b8 outdated 
    1328 | @@ -1329,8 +1329,6 @@ RPCHelpMan importmulti()
1329 |      // the user could have gotten from another RPC command prior to now
1330 |      wallet.BlockUntilSyncedToCurrentChain();
1331 |  
1332 | -    RPCTypeCheck(mainRequest.params, {UniValue::VARR, UniValue::VOBJ});
1333 | -
1334 |      EnsureLegacyScriptPubKeyMan(*pwallet, true);
1335 |  
1336 |      const UniValue& requests = mainRequest.params[0];
    furszy commented at 10:11 PM on November 15, 2022:

    could add:

        const UniValue& requests = mainRequest.params[0].get_array();
    maflcko commented at 12:57 PM on December 12, 2022:

    Why? Also, seems unrelated?

    furszy commented at 1:27 PM on December 12, 2022:

    IIRC, it is because we access requests without checking if it's an array or not. The requests .size() call and requests[i] access do not throw an exception if it is not.

    still, not a big deal.

    maflcko commented at 1:45 PM on December 12, 2022:

    without checking if it's an array or not

    Checking the type is the goal of this pull request. However, the type was usually already checked before, so this pull request is a refactor. Are you saying that this pull is accidentally fixing a bug?

    furszy commented at 1:59 PM on December 12, 2022:

    Not really. We were checking the type here before with the RPCTypeCheck that you removed and now we are checking it at the server's dispatcher function in a general manner.

    I was just suggesting to unify the code because there was a bug in the past due a missing get_array call in other command. E.g. if you provided another type instead of an array, and there was no RPCTypeCheck call before it, the usual for loop using request.size() and the brackets operator was just skipping the argument, not throwing the invalid type exception.

    Still, not a big deal at all because we are now checking the type for all the commands in a general manner anyway. So, feel free to ignore this and the other comment.

    maflcko commented at 2:07 PM on December 12, 2022:

    Anyone is free to change the code in a separate pull request. I can be pinged for review, but I want to keep this pull minimal for now.

  37. in src/wallet/rpc/spend.cpp:1325 in faa8b760b8 outdated 
    1306 | -                UniValue::VSTR, // estimate_mode
1307 | -                UniValueType(), // fee_rate, will be checked by AmountFromValue() in SetFeeEstimateMode()
1308 | -                UniValue::VOBJ, // options
1309 | -                }, true
1310 | -            );
1311 | -
    furszy commented at 10:16 PM on November 15, 2022:

    nit: could add the get_array at line 1316.

    const UniValue& recipients{request.params[0].get_array()};
    maflcko commented at 12:57 PM on December 12, 2022:

    Why? Also, seems unrelated?

  38. furszy approved
  39. furszy commented at 10:17 PM on November 15, 2022: member

    Code ACK faa8b760, only few non-blocking nits.

  40. maflcko force-pushed on Dec 12, 2022
  41. maflcko force-pushed on Dec 12, 2022
  42. DrahtBot renamed this:
    rpc: Run type check against RPCArgs (1/2)
    rpc: Run type check against RPCArgs (1/2)
    on Dec 12, 2022
  43. maflcko renamed this:
    rpc: Run type check against RPCArgs (1/2)
    refactor: Run type check against RPCArgs (1/2)
    on Dec 12, 2022
  44. maflcko removed the label RPC/REST/ZMQ on Dec 12, 2022
  45. DrahtBot added the label Refactoring on Dec 12, 2022
  46. maflcko added the label RPC/REST/ZMQ on Dec 12, 2022
  47. maflcko removed the label RPC/REST/ZMQ on Dec 12, 2022
  48. maflcko commented at 1:47 PM on December 12, 2022: member

    Changed title and description to clarify that this should be seen as a refactor, not a behaviour change. If any behaviour changes, it is likely a bugfix.

  49. maflcko requested review from furszy on Dec 12, 2022
  50. furszy approved
  51. furszy commented at 10:12 PM on December 12, 2022: member

    diff ACK faa7522

  52. DrahtBot added the label Needs rebase on Dec 13, 2022
  53. maflcko force-pushed on Dec 14, 2022
  54. DrahtBot removed the label Needs rebase on Dec 14, 2022
  55. furszy approved
  56. furszy commented at 1:22 PM on December 14, 2022: member

    diff utACK fa35405

  57. fanquake requested review from ryanofsky on Dec 14, 2022
  58. fanquake requested review from stickies-v on Jan 4, 2023
  59. DrahtBot added the label Needs rebase on Jan 11, 2023
  60. test: Fix wrong types passed to RPCs faf96721a6
  61. rpc: Run type check against RPCArgs fa9f6d7bcd
  62. maflcko force-pushed on Jan 11, 2023
  63. maflcko commented at 4:43 PM on January 11, 2023: member

    rebased due to one-line conflict, should be trivial to re-ACK

  64. DrahtBot removed the label Needs rebase on Jan 11, 2023
  65. fanquake requested review from ajtowns on Jan 13, 2023
  66. in src/rpc/rawtransaction.cpp:165 in fa9f6d7bcd 
     161 | @@ -162,7 +162,7 @@ static std::vector<RPCArg> CreateTxDoc()
 162 |                      },
 163 |                  },
 164 |              },
 165 | -        },
 166 | +         RPCArgOptions{.skip_type_check = true}},
    ajtowns commented at 1:27 PM on January 13, 2023:

    Any reason to spell out RPCArgOptions rather than just writing {.skip_type_check = true} on its own?

    maflcko commented at 10:07 AM on January 17, 2023:

    Pretty sure this is needed due to a compiler bug

    maflcko commented at 11:07 AM on January 17, 2023:

    gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0:

    /usr/bin/ccache g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config  -fmacro-prefix-map=/bitcoin-core=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -I/usr/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE    -pthread   -fdebug-prefix-map=/bitcoin-core=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wno-unused-parameter -Wno-deprecated-copy     -fno-extended-identifiers -fPIE -g -O2 -MT rpc/libbitcoin_node_a-blockchain.o -MD -MP -MF rpc/.deps/libbitcoin_node_a-blockchain.Tpo -c -o rpc/libbitcoin_node_a-blockchain.o `test -f 'rpc/blockchain.cpp' || echo './'`rpc/blockchain.cpp
rpc/blockchain.cpp: In function 'RPCHelpMan getblock()':
rpc/blockchain.cpp:760:5: error: conversion from '<brace-enclosed initializer list>' to 'RPCArg' is ambiguous
  760 |     };
      |     ^
In file included from ./rpc/server.h:10,
                 from rpc/blockchain.cpp:31:
./rpc/util.h:195:5: note: candidate: 'RPCArg::RPCArg(std::string, RPCArg::Type, RPCArg::Fallback, std::string, std::vector<RPCArg>, RPCArgOptions)'
  195 |     RPCArg(
      |     ^~~~~~
./rpc/util.h:180:5: note: candidate: 'RPCArg::RPCArg(std::string, RPCArg::Type, RPCArg::Fallback, std::string, RPCArgOptions)'
  180 |     RPCArg(
      |     ^~~~~~
In file included from /usr/include/c++/9/vector:67,
                 from ./core_io.h:11,
                 from ./rpc/blockchain.h:9,
                 from rpc/blockchain.cpp:6:
/usr/include/c++/9/bits/stl_vector.h:622:43: note:   initializing argument 1 of 'std::vector<_Tp, _Alloc>::vector(std::initializer_list<_Tp>, const allocator_type&) [with _Tp = RPCArg; _Alloc = std::allocator<RPCArg>; std::vector<_Tp, _Alloc>::allocator_type = std::allocator<RPCArg>]'
  622 |       vector(initializer_list<value_type> __l,
      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
make[2]: *** [Makefile:10271: rpc/libbitcoin_node_a-blockchain.o] Error 1
  67. in src/rpc/util.h:214 in fa9f6d7bcd 
     210 | @@ -217,6 +211,9 @@ struct RPCArg {
 211 |  
 212 |      bool IsOptional() const;
 213 |  
 214 | +    /** Check whether the request JSON type matches. */
    ajtowns commented at 1:29 PM on January 13, 2023:

    Document that it throws JSONRPCError if there's a mismatch?

    maflcko commented at 11:13 AM on January 20, 2023:

    Reworked in https://github.com/bitcoin/bitcoin/pull/26929

  68. ajtowns commented at 2:13 PM on January 13, 2023: contributor

    Approach ACK.

  69. ajtowns commented at 3:01 PM on January 13, 2023: contributor

    ACK fa9f6d7bcdba5f18c46fff1dcc0ac6d3dd8db75d

    Added some debugging code to verify that the removed RPCTypeCheck calls seem to match what the new logic will enforce.

  70. ajtowns commented at 3:01 PM on January 13, 2023: contributor

    Is there a (2/2) somewhere?

  71. fanquake merged this on Jan 17, 2023
  72. fanquake closed this on Jan 17, 2023
  73. fanquake commented at 9:40 AM on January 17, 2023: member

    Is there a (2/2) somewhere? @MarcoFalke Address the comments in the 2/2 ?

  74. maflcko deleted the branch on Jan 17, 2023
  75. sidhujag referenced this in commit bfee29ccf9 on Jan 17, 2023
  76. Fri3ndHacker approved
  77. maflcko commented at 11:10 AM on January 20, 2023: member

    Is there a (2/2) somewhere?

    I haven't written it, because code that isn't written doesn't need to be rebased by me.

    I can do a 1.5/2, unless you really want me to write 2/2 now.

  78. maflcko commented at 11:13 AM on January 20, 2023: member

    1.5

    Done in https://github.com/bitcoin/bitcoin/pull/26929

  79. bitcoin locked this on Jan 20, 2024
Contributors
maflckoDrahtBotamovfxfanquakeluke-jrw0xltfurszyajtownsFri3ndHacker


ryanofskystickies-v

Labels
Refactoring
Linked (view graph)
#25737 rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR
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-22 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me