rpc: Catch listsinceblock target_confirmations exceeding block count #19655

pull adaminsky wants to merge 1 commits into bitcoin:master from adaminsky:listsinceblock_rpc changing 2 files +24 −1

adaminsky commented at 7:49 AM on August 4, 2020: contributor

This addresses an issue brought up in #19587.

Currently, the target_confirmations parameter to listsinceblock is not checked for being too large. When target_confirmations is greater than one more than the current number of blocks, listsinceblock fails with error code -1. In comparison, when target_confirmations is less than 1, a -8 "Invalid parameter" error code is thrown.

This PR fixes the issue by returning a -8 "Invalid parameter" error if the target_confirmations value corresponds to a block with more confirmations than the genesis block. This happens if target_confirmations exceeds one more than the number of blocks.
fanquake added the label RPC/REST/ZMQ on Aug 4, 2020
adaminsky force-pushed on Aug 8, 2020
luke-jr commented at 9:19 PM on August 11, 2020: member

I think technically -1 is correct?
adaminsky commented at 10:13 PM on August 11, 2020: contributor

The -1 is from a CHECK_NONFATAL so the error says an internal bug is detected. I was thinking that the -8 error is more helpful since it is consistent with the result when target_confirmations = 0.
luke-jr commented at 10:18 PM on August 11, 2020: member

But target_confirmations 0 is in fact an invalid parameter. A too-large height just doesn't yield a useful result yet.
adaminsky commented at 10:57 PM on August 11, 2020: contributor

Yeah, so I guess the problem mentioned in the issue has been fixed by returning a -1 error. Maybe giving the genesis block hash when target_confirmations is too large makes sense.
ryanofsky approved
ryanofsky commented at 3:40 PM on August 12, 2020: member

Code review ACK 88ce8c9d3c6c97e50172980e75d11d3099b85dca. Simple fix and nice new tests.

I think returning genesis block hash in this case as suggested would be an improvement, and maybe invalid parameter error code doesn't give enough hope for the future, but either way new behavior is probably better than returning invalid 0 hash.
adaminsky commented at 4:38 PM on August 12, 2020: contributor

@ryanofsky thank you for the review. My commit message is actually incorrect because the problem with the 0 hash was already fixed from when it was reported by returning the -1 error. I'll update this to return genesis block hash instead.

in src/wallet/rpcwallet.cpp:1450 in c0e57fe0d3 outdated

1446 | @@ -1447,7 +1447,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1447 |                  "Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the \"removed\" array.\n",
1448 |                  {
1449 |                      {"blockhash", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, the block hash to list transactions since, otherwise list all transactions."},
1450 | -                    {"target_confirmations", RPCArg::Type::NUM, /* default */ "1", "Return the nth block hash from the main chain. e.g. 1 would mean the best block hash. Note: this is not used as a filter, but only affects [lastblock] in the return value"},
1451 | +                    {"target_confirmations", RPCArg::Type::NUM, /* default */ "1", "Return the nth block hash from the main chain or the last block hash in case the nth block does not exist. e.g. 1 would mean the best block hash. Note: this is not used as a filter, but only affects [lastblock] in the return value"},

ryanofsky commented at 8:37 PM on August 12, 2020:

In commit "Return genesis hash" (c0e57fe0d399235f3f2612c4972c597b34107509)

I think this description is more confusing than before and there's no need to mention the special case here, since it's explained in lastblock documentation below. If you think it should be mentioned both places, maybe put the inserted text after "e.g. 1 would mean the best block hash" instead of before to avoid jumping topics.

in test/functional/wallet_listsinceblock.py:95 in c0e57fe0d3 outdated

  90 | +        assert_equal(
  91 | +            self.nodes[0].getblockhash(0),
  92 | +            self.nodes[0].listsinceblock(blockhash, blockheight + 1)['lastblock'])
  93 | +        assert_equal(
  94 | +            self.nodes[0].getblockhash(0),
  95 | +            self.nodes[0].listsinceblock(blockhash, blockheight + 2)['lastblock'])

ryanofsky commented at 8:43 PM on August 12, 2020:

In commit "Return genesis hash" (c0e57fe0d399235f3f2612c4972c597b34107509)

Maybe test with a larger value like 1000 to be sure this isn't an edge case

ryanofsky approved
ryanofsky commented at 8:45 PM on August 12, 2020: member

Code review ACK c0e57fe0d399235f3f2612c4972c597b34107509. Thanks for the fix and updates! Suggest squashing commits

Cap listsinceblock target_confirmations param

Previously, listsinceblock would fail with error code -1 when the
target_confirmations exceeded the number of confirmations of the genesis
block. This commit allows target_confirmations to refer to a lastblock
hash with more confirmations than exist in the chain by setting the
lastblock hash to the genesis hash in this case. This allows for
`listsinceblock "" 6` to not fail if the block count is less than 5
which may happen on regtest.

Includes update to the functional test for listsinceblock to test for
this case.

c133cdcdc3

adaminsky force-pushed on Aug 12, 2020
ryanofsky approved
ryanofsky commented at 10:54 PM on August 12, 2020: member

Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15. Just suggested changes since last review. Thanks!
laanwj commented at 10:12 AM on August 13, 2020: member

Code review ACK c133cdcdc3397a734d57e05494682bf9bf6f4c15 Test looks good too.
laanwj merged this on Aug 13, 2020
laanwj closed this on Aug 13, 2020
Fabcien referenced this in commit ae4f7c1674 on Sep 8, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19587 property lastblock on listsinceblock cannot be used on subsequent calls if target_confirmations exceeds current block count