rpc: Disallow gettxoutsetinfo queries for a specific block with `use_index=false` #25471

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202206_gettxoutsetinfo_check changing 1 files +5 −1

mzumsande commented at 10:39 PM on June 24, 2022: contributor
In the gettxoutsetinfo RPC, if we set use_index to false but specify hash_or_height, we currently hit a nonfatal error, e.g. gettxoutsetinfo "muhash" "1" "false" results in:
```
Internal bug detected: "!pindex || pindex->GetBlockHash() == view->GetBestBlock()"
rpc/blockchain.cpp:836 (GetUTXOStats) 
```
The failing check was added in #24410, but the previous behavior, returning the specified height together with data corresponding to the tip's height, was very confusing too in my opinion. Fix this by disallowing the interaction of use_index=false and hash_or_height and add a RPC help example with -named because users might ask themselves how to use the use_index flag witout hitting an error.

An alternative way would be to allow the interaction if the specified hash_or_height happens to correspond to the tip (which should then also be applied to the HASH_SERIALIZED check before). If reviewers would prefer that, please say so.
fanquake added the label RPC/REST/ZMQ on Jun 24, 2022
DrahtBot commented at 3:38 AM on June 25, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19949 (cli: Parse and allow hash value by fjahr)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
shaavan commented at 9:40 AM on June 25, 2022: contributor
Concept ACK
- I agree with allowing displaying an RPC error instead of an Internal bug error because:
  
  AFAIK. This will enable the incorrect command to fail faster
  
  This will allow users to understand better the problem's cause and how to solve it.
Now, as for the alternative that you suggested:

An alternative way would be to allow the interaction if the specified hash_or_height happens to correspond to the tip...

I want to explain why I think it is better not to switch to the alternative.
1. This will potentially increase the code complexity.
2. Running the command bitcoin-cli -named gettxoutsetinfo hash_type='muhash' use_index='false' prints the info for the current best block. So technically, the alternative is not needed.
3. Since it is explicitly mentioned in the gettxoutsetinfo help message, the hash_or_height option is available only with the coinstatsindex. So ideally, it shouldn't be allowed to provide hash_or_height value with use_index=0.
```
Arguments:
1. hash_type         (string, optional, default="hash_serialized_2") Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'.
2. hash_or_height    (string or numeric, optional) The block hash or height of the target height (only available with coinstatsindex).
3. use_index         (boolean, optional, default=true) Use coinstatsindex, if available.
```
However, not using the alternative makes the gettxoutsetinfo command quite verbose, as there is no way to make use_index = false without using the --named option.

Screenshot:

So I would suggest reordering the arguments of this RPC to
```
1. `hash_type`
2. `use_index`
3. `hash_or_height`
```
Because, first, it would eliminate the need to use --named for specific cases, and second, this makes the ordering of argument logically sounder.

And I would also suggest mentioning in the description of hash_or_height, that not using this command would print info for the best block.
mzumsande commented at 3:11 PM on June 25, 2022: contributor

Thanks for the review!

So I would suggest reordering the arguments of this RPC

That would be nice, but it would break compatibility with older versions, which I think should really be avoided unless absolutely necessary. For other RPCs, we sometimes even keep dysfunctional dummy-arguments around to avoid this. (prioritisetransaction)

And I would also suggest mentioning in the description of hash_or_height, that not using this command would print info for the best block.

I'll do this with my next push!
Riahiamirreza commented at 5:59 PM on June 26, 2022: contributor

I tried to produce the same error and have difficulty with it. Where the use_index and hash_or_height are given to the program? Are they api args? or should be set while running bitcoind? I checked the api docs but not found something useful.
mzumsande commented at 6:26 PM on June 26, 2022: contributor

Where the use_index and hash_or_height are given to the program? Are they api args? or should be set while running bitcoind? I checked the api docs but not found something useful.

You need to start bitcoind with the coinstatsindex to reproduce this, e.g. ./bitcoind -signet -coinstatsindex, and wait until the index is synced. After that, you can query ./bitcoin-cli -signet gettxoutsetinfo "muhash" "1" "false", which should give you this internal error on master.

Doing the same locally with -regtest instead of -signet works too, but then you need to mine a couple of blocks yourself (./bitcoin-cli -regtest -generate 10 before querying gettxoutsetinfo ).
rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=false
by returning an RPC error where previously a NonFatalError would be thrown.
27c8056885
mzumsande force-pushed on Jun 28, 2022
mzumsande commented at 10:36 PM on June 28, 2022: contributor

2e9c043 to 27c8056: added explanation for the default of hash_or_height parameter (thanks @shaavan)
shaavan approved
shaavan commented at 11:40 AM on June 29, 2022: contributor
ACK 27c8056885b05bd25f540dbf6ede89230d43c7b9

Changes since my last review:
- The help message for the default behavior of the hash_or_height argument has been added.
I was able to successfully run the ./bitcoin-cli -signet help gettxoutsetinfo command, and verify the help message has been displayed correctly.

Screenshot:
maflcko requested review from dongcarl on Jun 29, 2022
maflcko requested review from fjahr on Jun 29, 2022
fjahr commented at 11:31 PM on June 29, 2022: contributor

utACK 27c8056885b05bd25f540dbf6ede89230d43c7b9
fjahr approved
dongcarl commented at 7:12 PM on June 30, 2022: contributor

Concept ACK for fixing the internal error that's exposed to RPC

In the longer term, I'd like to see CreateUTXOSnapshot use kernel::ComputeUTXOStats directly, and GetUTXOStats inlined into its only user gettxoutsetinfo and a lot of the if else checking can be removed.
maflcko merged this on Jul 1, 2022
maflcko closed this on Jul 1, 2022
sidhujag referenced this in commit 077c58e94f on Jul 1, 2022
mzumsande deleted the branch on Nov 3, 2022
bitcoin locked this on Nov 3, 2023