rpc: Catch listsinceblock target_confirmations exceeding block count #19655
Conversation
8bb7c52 to
88ce8c9
Compare
|
I think technically -1 is correct? |
|
The -1 is from a |
|
But |
|
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 |
There was a problem hiding this comment.
Code review ACK 88ce8c9. 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.
|
@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. |
There was a problem hiding this comment.
Code review ACK c0e57fe. Thanks for the fix and updates! Suggest squashing commits
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.
c0e57fe to
c133cdc
Compare
|
Code review ACK c133cdc |
Summary: 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. This is a backport of [[bitcoin/bitcoin#19655 | core#19655]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10073
This addresses an issue brought up in #19587.
Currently, the
target_confirmationsparameter tolistsinceblockis not checked for being too large. Whentarget_confirmationsis greater than one more than the current number of blocks,listsinceblockfails with error code -1. In comparison, whentarget_confirmationsis 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_confirmationsvalue corresponds to a block with more confirmations than the genesis block. This happens iftarget_confirmationsexceeds one more than the number of blocks.