rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage

jonatack commented at 9:33 am on April 22, 2022: contributor

The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.

Fix all issues.

test: add getblockfrompeer coverage of invalid inputs 734b9669ff

DrahtBot added the label RPC/REST/ZMQ on Apr 22, 2022

MarcoFalke commented at 11:19 am on April 22, 2022: member

Closes #24943.

Pretty sure the changes here have nothing to do with the issue. The issue report is about bitcoin-cli. Here, you are modifying the server code.

jonatack commented at 12:29 pm on April 22, 2022: contributor

Yes. Removed the mention in the description.

in src/rpc/blockchain.cpp:445 in a926025ca8 outdated

438@@ -439,6 +439,11 @@ static RPCHelpMan getblockfrompeer()
439         },
440         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
441 {
442+    RPCTypeCheck(request.params, {
443+        UniValue::VSTR, // blockhash
444+        UniValue::VNUM, // peer_id
445+    }, true);

MarcoFalke commented at 12:38 pm on April 22, 2022:

get_int64 should already throw, so maybe the error string of get_int64 can be adjusted instead? Otherwise, I’d suggest to do this in RPCHelpMan somehow automatically instead of forcing reviewers and devs to do it manually.

jonatack commented at 9:34 am on April 25, 2022:

It throws with the internal-style error seen in the previously referenced issue, but we do the same standard type check and user-friendly error message in many RPCs:

scantxoutset sendrawtransaction testmempoolaccept estimatesmartfee estimaterawfee getdescriptorinfo deriveaddresses setmocktime mockscheduler addconnection createrawtransaction decodescript signrawtransactionwithkey decodepsbt combinepsbt finalizepsbt createpsbt converttopsbt utxoupdatepsbt joinpsbts analyzepsbt upgradewallet importmulti importdescriptors lockunspent getunconfirmedbalance walletcreatefundedpsbt walletprocesspsbt send fundrawtransaction bumpfee psbtbumpfee

…and the new sendall RPC.

So it seems correct to do it here as well (and the change is trivial to review). If you disagree, can you clarify further? Alternatively, could just drop the second commit and add test coverage only.

MarcoFalke commented at 9:39 am on April 25, 2022:

Yeah, I think it should be removed in the other RPCs as well.

jonatack commented at 9:52 am on April 25, 2022:

Ok, that seems to be a discussion for another pull. This one adds test coverage and harmonizes getblockfrompeer with the other RPCs listed. LMK if you’d prefer the second commit be dropped.

MarcoFalke commented at 2:44 pm on May 9, 2022:

gettxout is a “real” RPC and isn’t checked either, so I don’t see why this one should be checked for “harmony” as well.

Recall that this is a test-only RPC, so any effort spent on it won’t be visible by users.

MarcoFalke commented at 2:44 pm on May 9, 2022:

Started my suggestion in #25093, which should be trivial to extend to the type checking you want to add here.

jonatack commented at 2:53 pm on May 9, 2022:

Thanks for the proposal. My understanding of your suggestion “the error string of get_int64 can be adjusted instead” is that the JSON value is not X message be changed to the Expected type X, got Y messages in many of our RPCs, but the former are in the external UniValue library. I’m not yet clear how your suggestion ties in with that, but can drop the second commit here (I still think it’s an improvement ATM though).

w0xlt commented at 3:16 pm on May 9, 2022:

nit: This RPC does not accept null params.

0    RPCTypeCheck(request.params, {
1        UniValue::VSTR, // blockhash
2        UniValue::VNUM, // peer_id
3    });

jonatack commented at 11:05 am on June 27, 2022:

thanks @w0xlt, done

luke-jr approved

luke-jr commented at 1:46 am on May 6, 2022: member

utACK

w0xlt approved

w0xlt commented at 3:18 pm on May 9, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/24944/commits/a926025ca822ae1fe480c6b1a2c480d1d4641c06

luke-jr referenced this in commit 93ad98c315 on May 21, 2022

luke-jr referenced this in commit 4fe12e6184 on May 21, 2022

rpc: add RPCTypeCheck for getblockfrompeer inputs 2ef5294a5b

jonatack force-pushed on Jun 27, 2022

jonatack commented at 11:07 am on June 27, 2022: contributor

Thanks @luke-jr and @w0xlt for the reviews, updated per git diff a926025 2ef5294 to take @w0xlt’s suggestion.

brunoerg approved

brunoerg commented at 7:37 pm on July 5, 2022: contributor

ACK 2ef5294a5bb68ceb3797d2638567a172cc21699f

MarcoFalke commented at 3:25 pm on July 12, 2022: member

edited comment (https://github.com/bitcoin/bitcoin/pull/24944#issuecomment-1167214427) due to a bug or feature (?) in github-merge.py:

 0Merge message contains an @!
 1---------------------------------------------------------------------------
 2Merge bitcoin/bitcoin#24944: rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage
 3
 42ef5294a5bb68ceb3797d2638567a172cc21699f rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
 5734b9669ff7b2f5e2820993443a6f868f6b0b20a test: add getblockfrompeer coverage of invalid inputs (Jon Atack)
 6
 7Pull request description:
 8
 9  The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
10  
11  Fix all issues.
12
13
14ACKs for top commit:
15  jonatack:
16    Thanks [@luke-jr](/bitcoin-bitcoin/contributor/luke-jr/) and [@w0xlt](/bitcoin-bitcoin/contributor/w0xlt/) for the ACKs, updated per `git diff a926025 2ef5294` to take [@w0xlt](/bitcoin-bitcoin/contributor/w0xlt/)'s suggestion.
17  brunoerg:
18    ACK 2ef5294a5bb68ceb3797d2638567a172cc21699f

MarcoFalke commented at 3:26 pm on July 12, 2022: member

My thinking is that we should make RPCTypeCheck to be run internally and consistently on all methods, not intermittently on only those that it was added to. See also #24944 (review)

MarcoFalke commented at 3:27 pm on July 12, 2022: member

So I am ~0, but happy to merge if reviewers think it is useful.

MarcoFalke merged this on Jul 12, 2022

MarcoFalke closed this on Jul 12, 2022

sidhujag referenced this in commit acba77090d on Jul 12, 2022

jonatack deleted the branch on Jul 19, 2022

bitcoin locked this on Oct 1, 2023

rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage #24944