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

mzumsande · 2022-06-24T22:39:04Z

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.

DrahtBot · 2022-06-25T03:38:28Z

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

Concept ACK

I agree with allowing displaying an RPC error instead of an Internal bug error because:
1. AFAIK. This will enable the incorrect command to fail faster
2. 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.

This will potentially increase the code complexity.
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.
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 · 2022-06-25T15:11:41Z

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 · 2022-06-26T17:59:12Z

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 · 2022-06-26T18:26:59Z

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 ).

…ndex=false by returning an RPC error where previously a NonFatalError would be thrown.

mzumsande · 2022-06-28T22:36:13Z

2e9c043 to 27c8056:
added explanation for the default of hash_or_height parameter (thanks @shaavan)

shaavan

ACK 27c8056

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:

fjahr · 2022-06-29T23:31:57Z

utACK 27c8056

dongcarl · 2022-06-30T19:12:43Z

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.

…ific block with `use_index=false` 27c8056 rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=false (Martin Zumsande) Pull request description: 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 [bitcoin#24410](bitcoin@664a14b), 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. ACKs for top commit: fjahr: utACK 27c8056 shaavan: ACK 27c8056 Tree-SHA512: 1d81c34eaa48c86134a2cf7193246d5de6bfd819d413c3b3fae9cb9290e0297a336111eeaecede2f0f020b0f9a181d240de0da4493e1b387fe63b8189154442b

fanquake added the RPC/REST/ZMQ label Jun 24, 2022

shaavan reviewed Jun 25, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 25, 2022

cli: Parse and allow hash value #19949

Closed

rpc: Disallow gettxoutsetinfo queries for a specific block with use_i…

27c8056

…ndex=false by returning an RPC error where previously a NonFatalError would be thrown.

mzumsande force-pushed the 202206_gettxoutsetinfo_check branch from 2e9c043 to 27c8056 Compare June 28, 2022 22:33

shaavan approved these changes Jun 29, 2022

View reviewed changes

maflcko requested review from dongcarl and fjahr June 29, 2022 14:16

fjahr approved these changes Jun 29, 2022

View reviewed changes

maflcko merged commit 53b1a24 into bitcoin:master Jul 1, 2022

mzumsande deleted the 202206_gettxoutsetinfo_check branch November 3, 2022 01:11

bitcoin locked and limited conversation to collaborators Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

mzumsande commented Jun 24, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Jun 25, 2022 •

edited

Loading

Uh oh!

shaavan left a comment

Uh oh!

mzumsande commented Jun 25, 2022

Uh oh!

Riahiamirreza commented Jun 26, 2022 •

edited

Loading

Uh oh!

mzumsande commented Jun 26, 2022

Uh oh!

mzumsande commented Jun 28, 2022

Uh oh!

shaavan left a comment

Uh oh!

fjahr commented Jun 29, 2022

Uh oh!

dongcarl commented Jun 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

Uh oh!

Conversation

mzumsande commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jun 25, 2022

Uh oh!

Riahiamirreza commented Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Jun 26, 2022

Uh oh!

mzumsande commented Jun 28, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jun 29, 2022

Uh oh!

dongcarl commented Jun 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

mzumsande commented Jun 24, 2022 •

edited

Loading

DrahtBot commented Jun 25, 2022 •

edited

Loading

Riahiamirreza commented Jun 26, 2022 •

edited

Loading