refactor: Run type check against RPCArgs (1/2) by maflcko · Pull Request #26039 · bitcoin/bitcoin

maflcko · 2022-09-07T18:00:06Z

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 3b5fb6e.

DrahtBot · 2022-09-08T17:27:59Z

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 · 2022-09-09T01:28:42Z

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

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

fanquake · 2022-09-10T09:33:41Z

Concept ACK

luke-jr · 2022-09-10T23:34:07Z

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

maflcko · 2022-09-12T06:45:57Z

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 · 2022-09-13T08:33:10Z

Concept ACK.

src/rpc/util.cpp

luke-jr · 2022-09-21T03:21:53Z

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 · 2022-09-21T06:43:29Z

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

… not RPC_MISC_ERROR e68d380 rpc: remove unneeded RPCTypeCheckArgument checks (furszy) 5556663 rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR (furszy) Pull request description: Same rationale as #26039, tackling another angle of the problem. #### Context We have the same univalue type error checking code spread/duplicated few times: `RPCTypeCheckObj`, `RPCTypeCheckArgument`, `UniValue::checkType`. In the first two functions, we are properly returning an `RPC_TYPE_ERROR` while in `UniValue::checkType` we are throwing an `std::runtime_error` which is caught by the RPC server request handler, who invalidly treats it as `RPC_MISC_ERROR` (which is a generic error return code that provides no information to the user). #### Proposed Changes Throw a custom exception from `Univalue::checkType` (instead of a plain `std::runtime_error`) and catch it on the RPC server request handler. So we properly return `RPC_TYPE_ERROR` (-3) on every arg type error and not the general `RPC_MISC_ERROR` (-1). This will allow us to remove all the `RPCTypeCheckArgument` calls. As them are redundant since #25629. Top commit has no ACKs. Tree-SHA512: 4e4c41851fd4e2b01a2d8b94e71513f9831f810768ebd89684caca4901e87d3677980003949bcce441f9ca607a1b38a5894839b6c492f5947b8bab8cd9423ba6

furszy

diff ACK faa7522

furszy

diff utACK fa35405

maflcko · 2023-01-11T16:43:09Z

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

ajtowns

Approach ACK.

ajtowns · 2023-01-13T13:27:08Z

src/rpc/rawtransaction.cpp

                },
            },
-        },
+         RPCArgOptions{.skip_type_check = true}},


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

Pretty sure this is needed due to a compiler bug

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

ajtowns · 2023-01-13T13:29:25Z

src/rpc/util.h


    bool IsOptional() const;

+    /** Check whether the request JSON type matches. */


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

Reworked in #26929

ajtowns · 2023-01-13T14:08:42Z

src/rpc/blockchain.cpp

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

ajtowns · 2023-01-13T15:01:27Z

ACK fa9f6d7

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

ajtowns · 2023-01-13T15:01:49Z

Is there a (2/2) somewhere?

fanquake · 2023-01-17T09:40:16Z

Is there a (2/2) somewhere?

@MarcoFalke Address the comments in the 2/2 ?

maflcko · 2023-01-20T11:10:30Z

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 · 2023-01-20T11:13:35Z

1.5

Done in #26929

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

This was referenced Sep 8, 2022

rpc: Add a parameter to sendrawtransaction which sets a maximum value for unspendable outputs. #25943

Merged

rpc: In utxoupdatepsbt also look for the tx in the txindex #25939

Merged

amovfx reviewed Sep 9, 2022

View reviewed changes

DrahtBot mentioned this pull request Sep 10, 2022

rpc: add descriptorprocesspsbt rpc #25796

Merged

maflcko mentioned this pull request Sep 10, 2022

rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR #25737

Merged

amovfx mentioned this pull request Sep 10, 2022

Wallet: Add foreign_outputs metadata to support CoinJoin transactions #25991

Closed

DrahtBot mentioned this pull request Sep 12, 2022

rpc: unify arg type error message #26069

Merged

DrahtBot added the Needs rebase label Sep 13, 2022

maflcko force-pushed the 2209-rpc-type- branch from 43ea30c to f03877f Compare September 13, 2022 12:21

DrahtBot removed the Needs rebase label Sep 13, 2022

furszy reviewed Sep 14, 2022

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

furszy approved these changes Dec 12, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 13, 2022

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

Closed

DrahtBot added the Needs rebase label Dec 13, 2022

maflcko force-pushed the 2209-rpc-type- branch from faa7522 to fa35405 Compare December 14, 2022 10:04

DrahtBot removed the Needs rebase label Dec 14, 2022

furszy approved these changes Dec 14, 2022

View reviewed changes

fanquake requested a review from ryanofsky December 14, 2022 14:58

DrahtBot mentioned this pull request Dec 28, 2022

refactor: Add performance-no-automatic-move clang-tidy check #26758

Closed

maflcko mentioned this pull request Jan 3, 2023

rpc: Correct RPCHelpMan for fundrawtransaction's input_weights field #26795

Merged

fanquake requested a review from stickies-v January 4, 2023 20:03

DrahtBot mentioned this pull request Jan 10, 2023

test: merge banning test from p2p_disconnect_ban to rpc_setban #26863

Closed

DrahtBot added the Needs rebase label Jan 11, 2023

MarcoFalke added 2 commits January 11, 2023 17:41

test: Fix wrong types passed to RPCs

faf9672

rpc: Run type check against RPCArgs

fa9f6d7

maflcko force-pushed the 2209-rpc-type- branch from fa35405 to fa9f6d7 Compare January 11, 2023 16:42

DrahtBot removed the Needs rebase label Jan 11, 2023

fanquake requested a review from ajtowns January 13, 2023 12:12

ajtowns reviewed Jan 13, 2023

View reviewed changes

fanquake merged commit 7799f53 into bitcoin:master Jan 17, 2023

maflcko deleted the 2209-rpc-type-🔤 branch January 17, 2023 12:53

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 17, 2023

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

bfee29c

Fri3ndHacker approved these changes Jan 19, 2023

View reviewed changes

ryanofsky mentioned this pull request Feb 13, 2023

RPC: Accept options as named-only parameters #26485

Merged

bitcoin locked and limited conversation to collaborators Jan 20, 2024


		bool IsOptional() const;

		/** Check whether the request JSON type matches. */

Conversation

maflcko commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

amovfx commented Sep 9, 2022

Uh oh!

amovfx left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 10, 2022

Uh oh!

luke-jr commented Sep 10, 2022

Uh oh!

maflcko commented Sep 12, 2022

Uh oh!

w0xlt commented Sep 13, 2022

Uh oh!

Uh oh!

luke-jr commented Sep 21, 2022

Uh oh!

maflcko commented Sep 21, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 11, 2023

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Jan 13, 2023

Uh oh!

ajtowns commented Jan 13, 2023

Uh oh!

fanquake commented Jan 17, 2023

Uh oh!

maflcko commented Jan 20, 2023

Uh oh!

maflcko commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

maflcko commented Sep 7, 2022 •

edited

Loading

DrahtBot commented Sep 8, 2022 •

edited

Loading