rpc: Improve error message for getblock invalid datatype. #21718

pull klementtan wants to merge 1 commits into bitcoin:master from klementtan:master changing 2 files +7 −3

klementtan commented at 10:54 AM on April 17, 2021: contributor

Improve error messages for getblock invalid datatype.

fixes: #21717
klementtan force-pushed on Apr 17, 2021
in src/rpc/blockchain.cpp:939 in 5417741d75 outdated
```
 937 | @@ -938,8 +938,10 @@ static RPCHelpMan getblock()
 938 |      if (!request.params[1].isNull()) {
 939 |          if(request.params[1].isNum())
```
MarcoFalke commented at 10:57 AM on April 17, 2021:

might be easier to swap the if branches and make this branch bool?

klementtan commented at 11:10 AM on April 17, 2021:

Yup you're right. Updated the PR according to your comments.
klementtan force-pushed on Apr 17, 2021
klementtan force-pushed on Apr 17, 2021
klementtan closed this on Apr 17, 2021
klementtan force-pushed on Apr 17, 2021
klementtan reopened this on Apr 17, 2021

in src/rpc/blockchain.cpp:939 in f6704323d1 outdated

 935 | @@ -936,10 +936,10 @@ static RPCHelpMan getblock()
 936 |  
 937 |      int verbosity = 1;
 938 |      if (!request.params[1].isNull()) {
 939 | -        if(request.params[1].isNum())
 940 | -            verbosity = request.params[1].get_int();
 941 | -        else
 942 | +        if(request.params[1].isBool())

MarcoFalke commented at 11:16 AM on April 17, 2021:

        if (request.params[1].isBool()) {

please use the new style per the dev notes

klementtan commented at 12:43 PM on April 17, 2021:

Added tests and updated the code to the new style

MarcoFalke approved
MarcoFalke commented at 11:17 AM on April 17, 2021: member

Looks good. A test would be nice, if possible.
DrahtBot added the label RPC/REST/ZMQ on Apr 17, 2021
klementtan force-pushed on Apr 17, 2021
klementtan force-pushed on Apr 17, 2021

in test/functional/rpc_blockchain.py:416 in b446f49f78 outdated

 410 | @@ -410,6 +411,13 @@ def _test_getblock(self):
 411 |          self.log.info("Test that getblock with verbosity 2 still works with pruned Undo data")
 412 |          datadir = get_datadir_path(self.options.tmpdir, 0)
 413 |  
 414 | +        try:
 415 | +            self.log.info("Test that getblock with invlaid verbosity type returns proper error message")
 416 | +            node.getblock(blockhash, "2")

MarcoFalke commented at 2:54 PM on April 17, 2021:

You can use the assert_raises_rpc_error helper

klementtan commented at 3:04 PM on April 17, 2021:

Okie updated to use assert_raises_rpc_error instead.

MarcoFalke approved
klementtan force-pushed on Apr 17, 2021
MarcoFalke commented at 3:10 PM on April 17, 2021: member

review ACK 89cbab16139d91c4bcf2a7308b8b21afb0efe00e

in test/functional/rpc_blockchain.py:413 in 89cbab1613 outdated

 409 | @@ -410,6 +410,9 @@ def _test_getblock(self):
 410 |          self.log.info("Test that getblock with verbosity 2 still works with pruned Undo data")
 411 |          datadir = get_datadir_path(self.options.tmpdir, 0)
 412 |  
 413 | +        self.log.info("Test that getblock with invlaid verbosity type returns proper error message")

theStack commented at 11:23 PM on April 17, 2021:

        self.log.info("Test that getblock with invalid verbosity type returns proper error message")

klementtan commented at 4:32 AM on April 18, 2021:

My apologies for that. Have updated the PR to fix the typos. Thanks!

theStack commented at 11:29 PM on April 17, 2021: member

Thanks for fixing, welcome as a new contributor! 🎉

Almost-ACK, there are two typos: one in the commit subject (s/getbloack/getblock/), and one in the test log message (s/invlaid/invalid/).
rpc: Improve getblock error message for invalid data type. a411494261
klementtan force-pushed on Apr 18, 2021
theStack approved
theStack commented at 7:22 AM on April 18, 2021: member

ACK a41149426168b8ea96099f10576022c6a09033d1
promag commented at 8:36 PM on April 18, 2021: member

Code review ACK a41149426168b8ea96099f10576022c6a09033d1.
instagibbs commented at 6:00 AM on April 19, 2021: member

utACK https://github.com/bitcoin/bitcoin/pull/21718/commits/a41149426168b8ea96099f10576022c6a09033d1
fanquake merged this on Apr 19, 2021
fanquake closed this on Apr 19, 2021
sidhujag referenced this in commit d3e4ee012b on Apr 19, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#21717 Invalid error message for getblock