rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC

furszy commented at 1:39 pm on July 29, 2022: member

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.

fanquake requested review from maflcko on Jul 29, 2022

fanquake added the label RPC/REST/ZMQ on Jul 29, 2022

fanquake commented at 1:49 pm on July 29, 2022: member

In https://cirrus-ci.com/task/5952817063002112 (mempool_accept.py):

 0 test  2022-07-29T13:45:02.711000Z TestFramework (ERROR): Assertion failed 
 1                                   Traceback (most recent call last):
 2                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 135, in try_rpc
 3                                       fun(*args, **kwds)
 4                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept.py", line 68, in <lambda>
 5                                       assert_raises_rpc_error(-3, 'JSON value of type string is not of expected type array', lambda: node.testmempoolaccept(rawtxs='ff00baar'))
 6                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 49, in __call__
 7                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 8                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 144, in __call__
 9                                       raise JSONRPCException(response['error'], status)
10                                   test_framework.authproxy.JSONRPCException: Expected type array, got string (-3)
11                                   During handling of the above exception, another exception occurred:
12                                   Traceback (most recent call last):
13                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
14                                       self.run_test()
15                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept.py", line 68, in run_test
16                                       assert_raises_rpc_error(-3, 'JSON value of type string is not of expected type array', lambda: node.testmempoolaccept(rawtxs='ff00baar'))
17                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 126, in assert_raises_rpc_error
18                                       assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
19                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 143, in try_rpc
20                                       message, e.error['message']))
21                                   AssertionError: Expected substring not found in error message:
22                                   substring: 'JSON value of type string is not of expected type array'
23                                   error message: 'Expected type array, got string'.

furszy force-pushed on Jul 29, 2022

furszy commented at 1:51 pm on July 29, 2022: member

thx @fanquake, pushed 👍🏼.

jonatack commented at 2:03 pm on July 29, 2022: contributor

Would it make sense to first adapt the UniValue error messages so that these remain the same for client software? Otherwise this seems like it would be needlessly changing user space.

furszy force-pushed on Jul 29, 2022

furszy commented at 5:30 pm on July 29, 2022: member

Would it make sense to first adapt the UniValue error messages so that these remain the same for client software? Otherwise this seems like it would be needlessly changing user space.

good point @jonatack. Will move into that direction.

still, the new error introduced in #25629 is much more descriptive, and helpful for the user, than the current one. Might be good to deprecate the old one and move to the new format.

furszy force-pushed on Jul 29, 2022

fanquake commented at 9:32 pm on July 29, 2022: member

good point @jonatack. Will move into that direction. still, the new error introduced in #25629 is much more descriptive, and helpful for the user, than the current one. Might be good to deprecate the old one and move to the new format.

I really don’t think we need to revert these changes (£25629), or have any sort of “deprecation” for the older messages.

DrahtBot commented at 9:53 pm on July 29, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26039 (rpc: Return RPC_TYPE_ERROR, not RPC_MISC_ERROR on type mismatch (1/2) by MarcoFalke)
#25429 (refactor: Avoid UniValue copies by MarcoFalke)
#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

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.

in src/univalue/lib/univalue.cpp:220 in a758ba62e9 outdated

216@@ -217,8 +217,7 @@ const UniValue& UniValue::operator[](size_t index) const
217 void UniValue::checkType(const VType& expected) const
218 {
219     if (typ != expected) {
220-        throw std::runtime_error{"JSON value of type " + std::string{uvTypeName(typ)} + " is not of expected type " +
221-                                 std::string{uvTypeName(expected)}};
222+        throw std::runtime_error{"Expected type " + std::string{uvTypeName(expected)} +  ", got " + std::string{uvTypeName(typ)}};

maflcko commented at 7:20 am on July 30, 2022:

ACK, previously this was "JSON value is not an {} as expected". Then, changed in fae5ce8795080018875227aee8613677f92e99ce.

I think I wanted to use your wording, so mind to split this up into a separate pull?

furszy commented at 3:16 pm on July 30, 2022:

will just drop the revert (rationale: #25737 (comment)).

maflcko commented at 3:54 pm on July 30, 2022:

Sure, no strong opinion. Just to clarify, you haven’t “dropped the revert”: It is not a revert, but a change to the string. Also, the string is still changed in this pull request.

furszy commented at 4:40 pm on July 30, 2022:

Just to clarify, you haven’t “dropped the revert”: It is not a revert, but a change to the string.

yeah ok. I thought that the string before fae5ce87 was the “Expected type {expected], got {type}” thus why mentioned the revert wording but seems that not. We actually have two different errors for the same problematic. Depends on the RPC command, we either throw a “JSON value of type {type} is not of expected type {expected}” or the “Expected type {expected], got {type}”.

Also, the string is still changed in this pull request.

Yeah, that is intended.

Unless someone has a strong opinion about it, will actually move the sources to only use the “JSON value of type {type} is not of expected type {expected}” error description everywhere and drop the other, less informative, one.

I think that It doesn’t make sense to build any client based on a json parsing error (if the command is expecting an int and you provide a string, the command will fail anyway..), so no one should be affected by this change.

furszy commented at 3:15 pm on July 30, 2022: member

good point @jonatack. Will move into that direction. still, the new error introduced in #25629 is much more descriptive, and helpful for the user, than the current one. Might be good to deprecate the old one and move to the new format.

I really don’t think we need to revert these changes (£25629), or have any sort of “deprecation” for the older messages.

Thinking about this for a second time, and.. agree. It doesn’t make sense to build any client based on a json parsing error.. if the RPC command is expecting an int and receives a string, it will always fail and never execute the command. So the new error description shouldn’t affect anyone. Will just drop the first commit.

furszy force-pushed on Jul 30, 2022

jonatack commented at 4:47 pm on July 30, 2022: contributor

If we really want to change user space, would it make sense to opt for the most frequently used of the error message formats currently, and the shortest and most straightforward one?

jonatack commented at 4:51 pm on July 30, 2022: contributor

(See #24944 (review) for a list of RPCs.)

furszy force-pushed on Jul 31, 2022

furszy commented at 1:29 pm on July 31, 2022: member

If we really want to change user space, would it make sense to opt for the most frequently used of the error message formats currently, and the shortest and most straightforward one?

Right now, we are throwing two different errors for the same problematic.

“Expected type {expected], got {type}”
“JSON value of type {type} is not of expected type {expected}”

This PR aims to drop the first one as it is less informative than the second one.

Note: aside from this changes, we still have the first error in RPCTypeCheckObj.

furszy force-pushed on Jul 31, 2022

in src/rpc/util.cpp:47 in d54f65c5d1 outdated

40@@ -41,19 +41,15 @@ void RPCTypeCheck(const UniValue& params,
41 
42         const UniValue& v = params[i];
43         if (!(fAllowNull && v.isNull())) {
44-            RPCTypeCheckArgument(v, t);
45+            if (!t.typeAny && v.type() != t.type) {
46+                throw JSONRPCError(RPC_TYPE_ERROR,
47+                                   strprintf("JSON value of type %s is not of expected type %s", uvTypeName(v.type()), uvTypeName(t.type)));
48+            }

aureleoules commented at 1:39 pm on August 15, 2022:

maybe merge if statements?

furszy commented at 2:23 am on September 2, 2022:

I’m unsure about chaining all those statements together. It’s hard to read already.

What about splitting the statements? e.g.

0if (fAllowNull && v.isNull()) continue;
1if (!t.typeAny && v.type() != t.type) {
2    throw JSONRPCError(RPC_TYPE_ERROR,
3          strprintf("JSON value of type %s is not of expected type %s", uvTypeName(v.type()), uvTypeName(t.type)));
4}

aureleoules commented at 10:04 am on September 13, 2022:

looks good :+1:

aureleoules commented at 1:43 pm on August 15, 2022: member

ACK d54f65c5d10983b97c8d33273a62b29b70b3853a. Verified that RPCTypeCheckArgument calls are redundant. +1 for unifying error messages.

jarolrod commented at 2:33 am on August 17, 2022: member

concept ack

in src/rpc/mempool.cpp:608 in d54f65c5d1 outdated

604@@ -605,7 +605,6 @@ static RPCHelpMan gettxspendingprevout()
605         },
606         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
607         {
608-            RPCTypeCheckArgument(request.params[0], UniValue::VARR);

maflcko commented at 6:24 pm on August 18, 2022:

why is this redundant?

furszy commented at 2:17 am on September 2, 2022:

oops, great catch. There is another one in coins.cpp. Pushing..

maflcko commented at 9:31 am on September 10, 2022:

Now you changed the error code from -3 to -1, effectively the opposite of what #26039 is doing

furszy commented at 1:08 pm on September 10, 2022:

true.. general exceptions are -1, while this is a type error.

We could throw a custom exception on the UniValue type check to catch all of them as type errors.

But.. I’m more inclined to close this PR and directly go ahead with moving the type checks responsibility to the request handler (which seems that is what you are doing in #26039). Haven’t checked if there is any restriction there but we do have each arg type on the help info object, so.. we could use that to verify every arg type.

maflcko commented at 6:28 am on September 15, 2022:

now you are changing the behavior in commit 8bdd192d4f897cc872f33cd6d15c36449dc345e5 and then reverting the behavior change in the next commit?

maflcko changes_requested

maflcko commented at 6:24 pm on August 18, 2022: member

.

furszy force-pushed on Sep 2, 2022

furszy commented at 12:41 pm on September 2, 2022: member

pushed per @MarcoFalke feedback. Thanks 👌🏼.

in src/rpc/util.cpp:46 in 5ea10772a1 outdated

39@@ -40,20 +40,15 @@ void RPCTypeCheck(const UniValue& params,
40             break;
41 
42         const UniValue& v = params[i];
43-        if (!(fAllowNull && v.isNull())) {
44-            RPCTypeCheckArgument(v, t);
45+        if (fAllowNull && v.isNull()) continue;
46+        if (!t.typeAny && v.type() != t.type) {
47+            throw JSONRPCError(RPC_TYPE_ERROR,
48+                               strprintf("JSON value of type %s is not of expected type %s", uvTypeName(v.type()), uvTypeName(t.type)));

maflcko commented at 1:16 pm on September 7, 2022:

Would be nice to split this behaviour change of unifying the string into a separate pull request from the refactoring changes

furszy commented at 4:43 pm on September 8, 2022:

ok, one sec.

furszy force-pushed on Sep 8, 2022

furszy commented at 6:47 pm on September 8, 2022: member

Based on Marko feedback, have removed the string unification from this PR. Will push it in a subsequent work. Ready to go.

maflcko commented at 6:47 am on September 12, 2022: member

have removed the string unification from this PR. Will push it in a subsequent work.

Are you still working on the string unification?

furszy commented at 1:21 pm on September 12, 2022: member

have removed the string unification from this PR. Will push it in a subsequent work.

Are you still working on the string unification?

As this PR touches the same code, was expecting to get it merged prior to create the follow-up PR. But.. based on #25737 (review) could create it now.

fanquake referenced this in commit e9e943cfb7 on Sep 13, 2022

DrahtBot added the label Needs rebase on Sep 13, 2022

fanquake commented at 12:35 pm on September 13, 2022: member

Might be worth closing / combining this into #26039? That PR is a more comprehensive version of the same kind of change.

furszy commented at 12:47 pm on September 13, 2022: member

Might be worth closing / combining this into #26039? That PR is a more comprehensive version of the same kind of change.

yeah, I wrote the same few days ago to MarkoFalke here #25737 (review).

Still, another proposal would be to do something like this: https://github.com/furszy/bitcoin/commit/e6720177b768d79b7f59f5dc6f77d1527b23fc05 Essentially, throw a custom exception for the type error instead of a bare runtime_error.

maflcko commented at 12:59 pm on September 13, 2022: member

throw a custom exception

Yeah, this shouldn’t hurt. Though I think there is still a benefit to checking the types in the rpc help manager:

In the future more complicated types can be checked, for example hex strings or amounts
Checking the types in the beginning may avoid a costly abort (when calculations have been done) or brittle abort (when resources need to be freed) when the type error is found later

furszy commented at 1:26 pm on September 13, 2022: member

Checking the types in the beginning may avoid a costly abort (when calculations have been done) or brittle abort (when resources need to be freed) when the type error is found later

Absolutely. The RPC framework should at least verify that the univalue types are the expected ones.

In the future more complicated types can be checked, for example hex strings or amounts

The only nano-concern that have about this is that we could end up downgrading performance by parsing the arguments twice (first at the framework level, then at the RPC command). So, we would need to store the parsed values somewhere in the request context.

So, options for this PR: (1) I could close it in favor of #26039 or.. (2) could push the univalue type error custom exception (this would make #26039 smaller).

Agree on going with (2) or do you prefer (1)? (I’m fine either way)

maflcko commented at 1:33 pm on September 13, 2022: member

I think both can be done independently

sidhujag referenced this in commit 6bc24d594c on Sep 13, 2022

furszy force-pushed on Sep 14, 2022

furszy renamed this:
~~rpc: remove redundant univalue type checks~~
rpc: custom univalue type exception + remove redundant univalue type checks
on Sep 14, 2022

in src/univalue/include/univalue.h:26 in 9b634608d5 outdated

25+        const std::string msg;
26+    public:
27+        explicit type_error(const std::string& _err) : msg(_err) {}
28+        const char* what() const noexcept override { return msg.c_str(); }
29+    };
30+

maflcko commented at 3:25 pm on September 14, 2022:

0    class type_error : : public std::runtime_error
1{
2    using std::runtime_error::runtime_error;
3};

furszy commented at 4:12 pm on September 14, 2022:

k

in src/rpc/util.cpp:50 in 9b634608d5 outdated

49         }
50         i++;
51     }
52 }
53 
54-void RPCTypeCheckArgument(const UniValue& value, const UniValueType& typeExpected)

maflcko commented at 3:27 pm on September 14, 2022:

not really a fan of removing this, given that this is needed for #26039

furszy commented at 4:12 pm on September 14, 2022:

Left you a comment there, #26039 (review). I think that we shouldn’t continue having the type error string defined in two different places.

maflcko commented at 4:42 pm on September 14, 2022:

I am removing RPCTypeCheck and you are adding the type error string to RPCTypeCheck. So I don’t see how the changes here require removing it or help in any way to reducing it to one place.

maflcko commented at 4:46 pm on September 14, 2022:

Also, unrelated: RPCTypeCheckObj has a third and different type error string?

furszy commented at 7:42 pm on September 14, 2022:

I am removing RPCTypeCheck and you are adding the type error string to RPCTypeCheck. So I don’t see how the changes here require removing it or help in any way to reducing it to one place.

Yeah, it’s not really needed here. I was pointing out that, as you are removing RPCTypeCheck, and I’m removing RPCTypeCheckArgument here, we could also reduce the error string duplication there as well.

But.. I could also implement the changes here too.. So yeah, probably better to leave it up for another PR and done.

Also, unrelated: RPCTypeCheckObj has a third and different type error string?

Yeah, I mentioned it in one of the first comments here #25737 (comment) and then forgot about it. Could fix it on the follow-up Univalue::checkType unification PR.

maflcko commented at 3:28 pm on September 14, 2022: member

left two nits

maflcko commented at 3:29 pm on September 14, 2022: member

Also, the tests fail

DrahtBot removed the label Needs rebase on Sep 14, 2022

furszy force-pushed on Sep 14, 2022

maflcko commented at 6:32 am on September 15, 2022: member

It would be good to explain the behavior change in the title, not summarize the detailed code changes. If someone was interested in the detailed code changes they could look at them, or you could summarize them in the commit/pull description.

Also, it would help reviewers if you explained for each commit whether it is a refactor or behavior change. This helps to document whether a change was intentional or not.

furszy commented at 1:17 pm on September 15, 2022: member

It would be good to explain the behavior change in the title, not summarize the detailed code changes. If someone was interested in the detailed code changes they could look at them, or you could summarize them in the commit/pull description.

Sure, the lack of scope in the PR description is mainly because this started as a simple redundant code removal (which didn’t require much text) and then we realized that for making the cleanup, needed to properly return RPC_TYPE_ERROR instead of RPC_MISC_ERROR in the inner univalue type check.

Also, it would help reviewers if you explained for each commit whether it is a refactor or behavior change. This helps to document whether a change was intentional or not.

Sure.

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

By throwing a custom exception from `Univalue::checkType` (instead of a plain
std::runtime_error) and catching it on the RPC server request handler.

So we properly return RPC_TYPE_ERROR (-3) on arg type errors and
not the general RPC_MISC_ERROR (-1).

55566630c6

furszy renamed this:
~~rpc: custom univalue type exception + remove redundant univalue type checks~~
rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR
on Sep 15, 2022

furszy force-pushed on Sep 15, 2022

rpc: remove unneeded RPCTypeCheckArgument checks

No-behavior change.

Since #25629, we check the univalue type internally.

e68d380797

furszy force-pushed on Sep 15, 2022

furszy commented at 1:47 pm on September 15, 2022: member

Updated per feedback.

maflcko approved

fanquake merged this on Sep 21, 2022

fanquake closed this on Sep 21, 2022

sidhujag referenced this in commit c366c50c21 on Sep 23, 2022

maflcko referenced this in commit 48174c0f28 on Nov 14, 2022

sidhujag referenced this in commit f165af6302 on Nov 14, 2022

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

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

Context

Proposed Changes

Conflicts