Make listsinceblock refuse unknown block hash #11565

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/since changing 3 files +47 −10

ryanofsky commented at 11:12 AM on October 26, 2017: member

Change suggested by @theuni who noticed listsinceblock would ignore invalid block hashes causing it to return a completely unfiltered list of transactions.

Make listsinceblock refuse unknown block hash

Change suggested by Cory Fields <cory-nospam-@coryfields.com> who noticed
listsinceblock would ignore invalid block hashes causing it to return a
completely unfiltered list of transactions.

659b2061c4

fanquake added the label RPC/REST/ZMQ on Oct 26, 2017
promag commented at 1:22 PM on October 26, 2017: member
With this change there is a different behaviour with these calls:
```
bitcoin-cli -named listsinceblock target_confirmations=1
bitcoin-cli listsinceblock "" 1
```
To make both consistent deal with empty string?
```
    if (!request.params[0].isNull() && !request.params[0].get_str().empty()) {
```
Otherwise tested ACK.
promag commented at 1:24 PM on October 26, 2017: member
Maybe we should also raise
```
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter");
```
if an argument is not a valid block hash?
theuni commented at 2:13 PM on October 26, 2017: member

Concept ACK. Agree with @promag's points. Also, because a zero hash is returned for lastblock in some cases (though maybe not anymore after this?), hash.IsNull() should probably throw an error also.
jnewbery commented at 3:02 PM on October 26, 2017: member

Tested ACK 3f4318c962d398ab375039aaf70beed2c4323fe4
ryanofsky force-pushed on Oct 26, 2017
ryanofsky commented at 3:54 PM on October 26, 2017: member

Updated 3f4318c962d398ab375039aaf70beed2c4323fe4 -> 659b2061c4329472a45e913c5d45e6ab180600a3 (pr/since.1 -> pr/since.2)

Changed to accept empty string, and added tests for empty, invalid hex, and zero strings.
promag commented at 4:33 PM on October 26, 2017: member

Invalid blockhash is not the same as block not found. The first should throw RPC_INVALID_PARAMETER. So for now rename test_invalid_blockhash to test_block_notfound` or something similar?
ryanofsky commented at 4:58 PM on October 26, 2017: member

Invalid blockhash is not the same as block not found

I don't know what this means. Feel free to send a patch or open a pr with whatever change you would like to see.
theuni commented at 5:23 PM on October 26, 2017: member

I generally agree with @promag that an invalid hash (zero, too long, too short) should throw a different error than a valid hash but missing block, but I suspect that we don't do that in most places already, so there's not much use in differentiating here.

utACK 659b2061c4329472a45e913c5d45e6ab180600a3
promag commented at 5:38 PM on October 26, 2017: member

We do have partial argument validation, mostly type validation, but in some cases value validation. It can be improved in a different PR for sure, I can take that.
jnewbery commented at 7:18 PM on October 26, 2017: member

Tested ACK 659b2061c4329472a45e913c5d45e6ab180600a3

Agree that invalid block hash validation for this and other RPC methods can be added in a future PR.
promag commented at 7:42 PM on October 26, 2017: member

ACK 659b206.
gmaxwell commented at 9:02 PM on October 26, 2017: contributor

utACK
ryanofsky commented at 2:08 AM on October 27, 2017: member

Should this be backported?
jonasschnelli added the label Needs backport on Oct 27, 2017
theuni commented at 5:52 PM on October 27, 2017: member

@ryanofsky I'd say so. I've been trying to come up with a scenario where this would be a regression from a user's pov, but can't come up with anything realistic.

All I've got is "maybe some software isn't prepared to cope with error/no output on a call", but that doesn't seem like an issue since everyone has to deal with warmup failures already.
MarcoFalke added this to the milestone 0.15.1 on Oct 28, 2017
MarcoFalke commented at 6:04 PM on October 28, 2017: member

utACK 659b2061c4329472a45e913c5d45e6ab180600a3
sdaftuar commented at 12:33 PM on October 31, 2017: member

utACK
laanwj merged this on Nov 1, 2017
laanwj closed this on Nov 1, 2017
laanwj referenced this in commit e1f6a2a801 on Nov 1, 2017
laanwj referenced this in commit 7d4546f17d on Nov 1, 2017
fanquake commented at 1:49 PM on November 1, 2017: member

Backported in #11550
fanquake removed the label Needs backport on Nov 1, 2017
codablock referenced this in commit 0dafd3c71a on Sep 26, 2019
codablock referenced this in commit df54da263b on Sep 26, 2019
codablock referenced this in commit 46373f5ee2 on Sep 30, 2019
codablock referenced this in commit e2b4b205ea on Sep 30, 2019
barrystyle referenced this in commit e891e32691 on Jan 22, 2020
barrystyle referenced this in commit 4a57e5723b on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.15.2