`scantxoutset` should output the block hash instead of block height

luisschwab commented at 2:13 pm on July 18, 2024: contributor

Please describe the feature you’d like to see added.

When using the scantxoutset RPC, the current behaviour is to show the block height of the UTXO. This is not optimal, as block height is ambiguous, especially in the case of a block reorganization happening at the same instant of the query. In this case, an UTXO that does not exist would be assumed to exist, unless the chain’s tip hash is recorded before the scan, and make sure it still exists after, as per this comment by @evanlinjin.

The optimal behaviour should be instead to show block hash:

 0~$ bitcoin-cli scantxoutset start "[\"addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)\"]"
 1{
 2  "success": true,
 3  "txouts": 185261159,
 4  "height": 852742,
 5  "bestblock": "000000000000000000004bf6339c59b5c789c8bfd00efc1a4d77d948f1ea328a",
 6  "unspents": [
 7    {
 8      "txid": "fae435084345fe26e464994aebc6544875bca0b897bf4ce52a65901ae28ace92",
 9      "vout": 0,
10      "scriptPubKey": "0014a00b1ad58d24ad8433c56662bfb45596cd5fa7d6",
11      "desc": "addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)#smk4xmt7",
12      "amount": 0.00091190,
13      "coinbase": false,
14      "height": 852741
15+     "blockhash": 000000000000000000004bf6339c59b5c789c8bfd00efc1a4d77d948f1ea328a
16    }
17  ],
18  "total_amount": 0.00091190
19}

No response

Describe the solution you’d like

No response

Describe any alternatives you’ve considered

No response

Please leave any additional context

No response

luisschwab added the label Feature on Jul 18, 2024

maflcko removed the label Feature on Jul 18, 2024

maflcko added the label Bug on Jul 18, 2024

maflcko added the label RPC/REST/ZMQ on Jul 18, 2024

maflcko commented at 2:19 pm on July 18, 2024: member

This is not optimal, as block height is ambiguous, especially in the case of a block reorganization happening at the same instant of the query.

Sounds like a bug. The code:

 0        {
 1            ChainstateManager& chainman = EnsureChainman(node);
 2            LOCK(cs_main);
 3            Chainstate& active_chainstate = chainman.ActiveChainstate();
 4            active_chainstate.ForceFlushStateToDisk();
 5            pcursor = CHECK_NONFATAL(active_chainstate.CoinsDB().Cursor());
 6            tip = CHECK_NONFATAL(active_chainstate.m_chain.Tip());
 7        }
 8        bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins, node.rpc_interruption_point);
 9        result.pushKV("success", res);
10        result.pushKV("txouts", count);
11        result.pushKV("height", tip->nHeight);
12        result.pushKV("bestblock", tip->GetBlockHash().GetHex());

maflcko added the label Feature on Jul 18, 2024

maflcko removed the label Bug on Jul 18, 2024

maflcko commented at 2:42 pm on July 18, 2024: member

Sorry, no. The code is fine, because the cursor and the tip are taken in the same lock.

And the height and hash are already returned. Not sure what is left to be done here.

I don’t see the value of removing an RPC field and breaking clients for no clear reason.

I understand that the current docs for the fields may not be ideal. So pull requests with improvements are welcome :)

maflcko removed the label Feature on Jul 18, 2024

maflcko added the label Docs on Jul 18, 2024

sipa commented at 2:44 pm on July 18, 2024: member

@luisschwab Just to be clear, I believe that the block hash you’re asking for is already returned, under the name "bestblock". I don’t think we should be removing the height, but you’re of course free to ignore it.

sipa commented at 2:48 pm on July 18, 2024: member

Actually, I think there is some confusion here. @luisschwab is talking about the height/blockhash of the UTXO found, but the suggested diff in RPC output is about the height/blockhash of the chain tip up to where the scan occurred (which are both already reported).

I don’t think it would be hard to add a block hash to the individual UTXO records returned by scantxoutset (for the block they were created in), in addition to the "height" field returned for them.

luisschwab commented at 3:07 pm on July 18, 2024: contributor

Yes, I took the example out of the best block at the time. Indeed, it may be best to add the extra field instead (and perhaps mark height as deprecated?).

edit: fixed the diff to reflect height in UTXO and not the response as a whole.

maflcko commented at 3:20 pm on July 18, 2024: member

(and perhaps mark height aa deprecated?).

Not sure. It already exists and is convenient. If a user wants to shoot themselves in the foot, they can continue to do so by turning the block hash into a block height themselves. So I don’t think there is a strong benefit in removing it?

maflcko removed the label Docs on Jul 18, 2024

luisschwab commented at 6:35 pm on July 18, 2024: contributor

So I don’t think there is a strong benefit in removing it?

Yes, I agree.

maflcko commented at 7:08 pm on July 18, 2024: member

While touching this, another thing to add could be the number of confirmations? I understand that this wouldn’t help machine consumers of the interface, but human callers may find it useful?

luisschwab commented at 7:11 pm on July 18, 2024: contributor

Sure, should also be trivial to add.

jbesraa commented at 5:47 pm on July 21, 2024: none

I am interested in taking a look at this issue if nobody else is working on it already

luisschwab commented at 2:27 pm on July 23, 2024: contributor

@jbesraa I’m currently working on a PR for this issue.

luisschwab commented at 9:40 pm on July 23, 2024: contributor

PR #30515 is up, reviews are appreciated.

fanquake closed this on Jul 28, 2024

fanquake referenced this in commit 38c30a4b50 on Jul 28, 2024

scantxoutset should output the block hash instead of block height #30478

Please describe the feature you’d like to see added.

Is your feature related to a problem, if so please describe it.

Describe the solution you’d like

Describe any alternatives you’ve considered

Please leave any additional context

`scantxoutset` should output the block hash instead of block height #30478