New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Run type check against RPCArgs (1/2) #26039
The head ref may contain hidden characters: "2209-rpc-type-\u{1F524}"
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I gather this is a large refactor with the removal of the RPCTypeCheck functions all over the place?
Concept ACK |
Concept ACK, but this feels like it's doing too much additional. It might be nice to have something minimal we can backport? |
Not sure if we want to backport this, but this should already be minimal. A fragile alternative would be to add |
Concept ACK. |
43ea30c
to
f03877f
Compare
Shouldn't we be able to just adjust the return code? |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff ACK faa7522
faa7522
to
fa35405
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff utACK fa35405
fa35405
to
fa9f6d7
Compare
rebased due to one-line conflict, should be trivial to re-ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK.
@@ -162,7 +162,7 @@ static std::vector<RPCArg> CreateTxDoc() | |||
}, | |||
}, | |||
}, | |||
}, | |||
RPCArgOptions{.skip_type_check = true}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to spell out RPCArgOptions
rather than just writing {.skip_type_check = true}
on its own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this is needed due to a compiler bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -217,6 +211,9 @@ struct RPCArg { | |||
|
|||
bool IsOptional() const; | |||
|
|||
/** Check whether the request JSON type matches. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document that it throws JSONRPCError
if there's a mismatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked in #26929
{"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{ | ||
.skip_type_check = true, | ||
.type_str = {"", "string or numeric"}, | ||
}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
ACK fa9f6d7 Added some debugging code to verify that the removed RPCTypeCheck calls seem to match what the new logic will enforce. |
Is there a (2/2) somewhere? |
@MarcoFalke Address the comments in the 2/2 ? |
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. |
Done in #26929 |
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 toRPCTypeCheck
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.