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

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.

glozow added the label RPC/REST/ZMQ on Sep 8, 2022

DrahtBot commented at 5:27 pm on September 8, 2022: contributor

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

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.

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.

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.

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?

fanquake commented at 9:33 am on September 10, 2022: member

Concept ACK

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?

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.

w0xlt commented at 8:33 am on September 13, 2022: contributor

Concept ACK.

DrahtBot added the label Needs rebase on Sep 13, 2022

maflcko force-pushed on Sep 13, 2022

DrahtBot removed the label Needs rebase on Sep 13, 2022

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:

 0void RPCArg::MatchesType(const UniValue& request) const
 1{
 2    if(m_skip_type_check) return;
 3    if (IsOptional() && request.isNull()) return;
 4    // Special cases
 5    if (m_type == Type::AMOUNT || m_type == Type::RANGE) {
 6        // Type::AMOUNT: VNUM or VSTR, checked inside AmountFromValue()
 7        // Type::RANGE: VNUM or VARR, checked inside ParseRange()
 8        return;
 9    }
10    // Check and throw exception if type is invalid
11    UniValue::VType type = ToUnivalueType(m_type);
12    request.checkType(type);
13}

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.

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?

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)

fanquake referenced this in commit b1f44ecdcd on Sep 21, 2022

DrahtBot added the label Needs rebase on Sep 21, 2022

sidhujag referenced this in commit c366c50c21 on Sep 23, 2022

maflcko force-pushed on Sep 29, 2022

DrahtBot removed the label Needs rebase on Sep 29, 2022

DrahtBot added the label Needs rebase on Sep 30, 2022

maflcko force-pushed on Sep 30, 2022

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

maflcko force-pushed on Sep 30, 2022

maflcko marked this as ready for review on Sep 30, 2022

maflcko commented at 11:02 am on September 30, 2022: member

Rebased, changed title, rewritten description

DrahtBot removed the label Needs rebase on Sep 30, 2022

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.

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…

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.

maflcko force-pushed on Nov 14, 2022

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.

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.

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

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.

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:

0    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.

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.

0const UniValue& recipients{request.params[0].get_array()};

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

Why? Also, seems unrelated?

furszy approved

furszy commented at 10:17 pm on November 15, 2022: member

Code ACK faa8b760, only few non-blocking nits.

maflcko force-pushed on Dec 12, 2022

DrahtBot renamed this:
~~rpc: Run type check against RPCArgs (1/2)~~
rpc: Run type check against RPCArgs (1/2)
on Dec 12, 2022

maflcko renamed this:
~~rpc: Run type check against RPCArgs (1/2)~~
refactor: Run type check against RPCArgs (1/2)
on Dec 12, 2022

maflcko removed the label RPC/REST/ZMQ on Dec 12, 2022

DrahtBot added the label Refactoring on Dec 12, 2022

maflcko added the label RPC/REST/ZMQ on Dec 12, 2022

maflcko removed the label RPC/REST/ZMQ on Dec 12, 2022

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.

maflcko requested review from furszy on Dec 12, 2022

furszy approved

furszy commented at 10:12 pm on December 12, 2022: member

diff ACK faa7522

DrahtBot added the label Needs rebase on Dec 13, 2022

maflcko force-pushed on Dec 14, 2022

DrahtBot removed the label Needs rebase on Dec 14, 2022

furszy approved

furszy commented at 1:22 pm on December 14, 2022: member

diff utACK fa35405

fanquake requested review from ryanofsky on Dec 14, 2022

fanquake requested review from stickies-v on Jan 4, 2023

DrahtBot added the label Needs rebase on Jan 11, 2023

test: Fix wrong types passed to RPCs faf96721a6

rpc: Run type check against RPCArgs fa9f6d7bcd

maflcko force-pushed on Jan 11, 2023

maflcko commented at 4:43 pm on January 11, 2023: member

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

DrahtBot removed the label Needs rebase on Jan 11, 2023

fanquake requested review from ajtowns on Jan 13, 2023

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:

 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
 1rpc/blockchain.cpp: In function 'RPCHelpMan getblock()':
 2rpc/blockchain.cpp:760:5: error: conversion from '<brace-enclosed initializer list>' to 'RPCArg' is ambiguous
 3  760 |     };
 4      |     ^
 5In file included from ./rpc/server.h:10,
 6                 from rpc/blockchain.cpp:31:
 7./rpc/util.h:195:5: note: candidate: 'RPCArg::RPCArg(std::string, RPCArg::Type, RPCArg::Fallback, std::string, std::vector<RPCArg>, RPCArgOptions)'
 8  195 |     RPCArg(
 9      |     ^~~~~~
10./rpc/util.h:180:5: note: candidate: 'RPCArg::RPCArg(std::string, RPCArg::Type, RPCArg::Fallback, std::string, RPCArgOptions)'
11  180 |     RPCArg(
12      |     ^~~~~~
13In file included from /usr/include/c++/9/vector:67,
14                 from ./core_io.h:11,
15                 from ./rpc/blockchain.h:9,
16                 from rpc/blockchain.cpp:6:
17/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>]'
18  622 |       vector(initializer_list<value_type> __l,
19      |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
20make[2]: *** [Makefile:10271: rpc/libbitcoin_node_a-blockchain.o] Error 1

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

ajtowns commented at 2:13 pm on January 13, 2023: contributor

Approach ACK.

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.

ajtowns commented at 3:01 pm on January 13, 2023: contributor

Is there a (2/2) somewhere?

fanquake merged this on Jan 17, 2023

fanquake closed this on Jan 17, 2023

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 ?

maflcko deleted the branch on Jan 17, 2023

sidhujag referenced this in commit bfee29ccf9 on Jan 17, 2023

Fri3ndHacker approved

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.

maflcko commented at 11:13 am on January 20, 2023: member

1.5

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

bitcoin locked this on Jan 20, 2024

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

Reviews

Conflicts