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

furszy · 2022-07-29T13:39:01Z

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 · 2022-07-29T13:49:28Z

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

 test  2022-07-29T13:45:02.711000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     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
                                       fun(*args, **kwds)
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept.py", line 68, in <lambda>
                                       assert_raises_rpc_error(-3, 'JSON value of type string is not of expected type array', lambda: node.testmempoolaccept(rawtxs='ff00baar'))
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 49, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 144, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: Expected type array, got string (-3)
                                   During handling of the above exception, another exception occurred:
                                   Traceback (most recent call last):
                                     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
                                       self.run_test()
                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept.py", line 68, in run_test
                                       assert_raises_rpc_error(-3, 'JSON value of type string is not of expected type array', lambda: node.testmempoolaccept(rawtxs='ff00baar'))
                                     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
                                       assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
                                     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
                                       message, e.error['message']))
                                   AssertionError: Expected substring not found in error message:
                                   substring: 'JSON value of type string is not of expected type array'
                                   error message: 'Expected type array, got string'.

furszy · 2022-07-29T13:51:04Z

thx @fanquake, pushed 👍🏼.

jonatack

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 · 2022-07-29T17:30:57Z

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.

fanquake · 2022-07-29T21:32:08Z

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 · 2022-07-29T21:53:20Z

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.

src/univalue/lib/univalue.cpp

furszy · 2022-07-30T15:15:07Z

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.

jonatack · 2022-07-30T16:47:55Z

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 · 2022-07-30T16:51:08Z

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

furszy · 2022-07-31T13:29:40Z

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.

aureleoules

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

src/rpc/util.cpp

jarolrod · 2022-08-17T02:33:37Z

concept ack

maflcko · 2022-08-18T18:24:42Z

src/rpc/mempool.cpp

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


why is this redundant?

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

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

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.

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

maflcko

.

maflcko · 2022-09-13T13:33:56Z

I think both can be done independently

2870a97 RPC: unify arg type error message (furszy) Pull request description: Decoupled from bitcoin#25737 per request. We are throwing two different error descriptions for the same problematic: 1) "Expected type {expected], got {type}" --> `RPCTypeCheckArgument()` 2) "JSON value of type {type} is not of expected type {expected}" --> `UniValue::checkType()` ACKs for top commit: MarcoFalke: review ACK 2870a97 fanquake: ACK 2870a97 Tree-SHA512: 9ac863243b6b7687986c0394611b5cad8b35424ec49d82195d536f2a5e64c60327b25f0dc7336189f86fd71122689c7309da49adfa93805d2e345693fa8efa9b

maflcko

left two nits

src/univalue/include/univalue.h

maflcko · 2022-09-14T15:27:06Z

src/rpc/util.cpp

        }
        i++;
    }
 }

-void RPCTypeCheckArgument(const UniValue& value, const UniValueType& typeExpected)


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

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

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.

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

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 · 2022-09-14T15:29:02Z

Also, the tests fail

maflcko · 2022-09-15T06:32:56Z

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 · 2022-09-15T13:17:17Z

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.

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

No-behavior change. Since bitcoin#25629, we check the univalue type internally.

furszy · 2022-09-15T13:47:12Z

Updated per feedback.

…_ERROR, 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 bitcoin#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 bitcoin#25629. Top commit has no ACKs. Tree-SHA512: 4e4c41851fd4e2b01a2d8b94e71513f9831f810768ebd89684caca4901e87d3677980003949bcce441f9ca607a1b38a5894839b6c492f5947b8bab8cd9423ba6

2dede9f Adjust RPCTypeCheckObj error string (Leonardo Araujo) Pull request description: Unifies the JSON type error strings as mentioned in bitcoin#26214. Also refer to bitcoin#25737. ACKs for top commit: furszy: ACK 2dede9f Tree-SHA512: c918889e347ba32cb6d0e33c0de5956c2077dd40c996151e16741b0c4983ff098c60258206ded76ad7bbec4876c780c6abb494a97e4f1e05717d28a59b9167a6

fanquake requested a review from maflcko July 29, 2022 13:39

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

furszy force-pushed the 2022_rpc_remove_type_check branch from 8a8583d to a3e5674 Compare July 29, 2022 13:50

jonatack reviewed Jul 29, 2022

View reviewed changes

furszy force-pushed the 2022_rpc_remove_type_check branch 2 times, most recently from c2cc1db to 0e5e63c Compare July 29, 2022 17:27

furszy force-pushed the 2022_rpc_remove_type_check branch 5 times, most recently from 1fbf520 to 53ce3b8 Compare July 29, 2022 20:00

DrahtBot mentioned this pull request Jul 30, 2022

refactor: Avoid UniValue copies #25429

Closed

maflcko reviewed Jul 30, 2022

View reviewed changes

src/univalue/lib/univalue.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_rpc_remove_type_check branch from 53ce3b8 to d48f5d1 Compare July 30, 2022 15:17

furszy force-pushed the 2022_rpc_remove_type_check branch from d48f5d1 to 133fa8a Compare July 31, 2022 13:27

furszy force-pushed the 2022_rpc_remove_type_check branch from 133fa8a to d54f65c Compare July 31, 2022 13:48

aureleoules reviewed Aug 15, 2022

View reviewed changes

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

maflcko reviewed Aug 18, 2022

View reviewed changes

maflcko suggested changes Aug 18, 2022

View reviewed changes

furszy force-pushed the 2022_rpc_remove_type_check branch from d54f65c to f1ee517 Compare September 2, 2022 02:17

furszy force-pushed the 2022_rpc_remove_type_check branch 2 times, most recently from 8d56cf9 to 9b63460 Compare September 14, 2022 15:18

furszy changed the title ~~rpc: remove redundant univalue type checks~~ rpc: custom univalue type exception + remove redundant univalue type checks Sep 14, 2022

maflcko reviewed Sep 14, 2022

View reviewed changes

DrahtBot removed the Needs rebase label Sep 14, 2022

furszy force-pushed the 2022_rpc_remove_type_check branch 2 times, most recently from 5032c39 to 153f465 Compare September 14, 2022 22:05

furszy changed the title ~~rpc: custom univalue type exception + remove redundant univalue type checks~~ rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR Sep 15, 2022

furszy force-pushed the 2022_rpc_remove_type_check branch from 153f465 to 5d9ea95 Compare September 15, 2022 13:43

rpc: remove unneeded RPCTypeCheckArgument checks

e68d380

No-behavior change. Since bitcoin#25629, we check the univalue type internally.

furszy force-pushed the 2022_rpc_remove_type_check branch from 5d9ea95 to e68d380 Compare September 15, 2022 13:45

maflcko approved these changes Sep 15, 2022

View reviewed changes

fanquake merged commit b1f44ec into bitcoin:master Sep 21, 2022

maflcko mentioned this pull request Sep 21, 2022

[24.x] rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR #26147

Closed

ryanofsky mentioned this pull request Sep 26, 2022

Multiprocess bitcoin #10102

Draft

maflcko mentioned this pull request Sep 30, 2022

Adjust RPCTypeCheckObj error string #26214

Closed

araujo88 mentioned this pull request Oct 3, 2022

rpc: Adjust RPCTypeCheckObj error string #26240

Merged

ryanofsky mentioned this pull request Feb 13, 2023

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

furszy deleted the 2022_rpc_remove_type_check branch May 27, 2023 01:48

bitcoin locked and limited conversation to collaborators May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

furszy commented Jul 29, 2022 •

edited

Loading

fanquake commented Jul 29, 2022

furszy commented Jul 29, 2022

jonatack left a comment

furszy commented Jul 29, 2022 •

edited

Loading

fanquake commented Jul 29, 2022

DrahtBot commented Jul 29, 2022 •

edited

Loading

furszy commented Jul 30, 2022

jonatack commented Jul 30, 2022

jonatack commented Jul 30, 2022

furszy commented Jul 31, 2022 •

edited

Loading

aureleoules left a comment

jarolrod commented Aug 17, 2022

maflcko Aug 18, 2022

furszy Sep 2, 2022

maflcko Sep 10, 2022

furszy Sep 10, 2022

maflcko Sep 15, 2022

maflcko left a comment

maflcko commented Sep 13, 2022

maflcko left a comment

maflcko Sep 14, 2022

furszy Sep 14, 2022

maflcko Sep 14, 2022

maflcko Sep 14, 2022

furszy Sep 14, 2022 •

edited

Loading

maflcko commented Sep 14, 2022

maflcko commented Sep 15, 2022

furszy commented Sep 15, 2022 •

edited

Loading

furszy commented Sep 15, 2022

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

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

Conversation

furszy commented Jul 29, 2022 • edited Loading

Context

Proposed Changes

fanquake commented Jul 29, 2022

furszy commented Jul 29, 2022

jonatack left a comment

Choose a reason for hiding this comment

furszy commented Jul 29, 2022 • edited Loading

fanquake commented Jul 29, 2022

DrahtBot commented Jul 29, 2022 • edited Loading

Conflicts

furszy commented Jul 30, 2022

jonatack commented Jul 30, 2022

jonatack commented Jul 30, 2022

furszy commented Jul 31, 2022 • edited Loading

aureleoules left a comment

Choose a reason for hiding this comment

jarolrod commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko commented Sep 13, 2022

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

maflcko commented Sep 14, 2022

maflcko commented Sep 15, 2022

furszy commented Sep 15, 2022 • edited Loading

furszy commented Sep 15, 2022

furszy commented Jul 29, 2022 •

edited

Loading

furszy commented Jul 29, 2022 •

edited

Loading

DrahtBot commented Jul 29, 2022 •

edited

Loading

furszy commented Jul 31, 2022 •

edited

Loading

furszy Sep 14, 2022 •

edited

Loading

furszy commented Sep 15, 2022 •

edited

Loading