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

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:

~$ bitcoin-cli scantxoutset start "[\"addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)\"]"
{
  "success": true,
  "txouts": 185261159,
  "height": 852742,
  "bestblock": "000000000000000000004bf6339c59b5c789c8bfd00efc1a4d77d948f1ea328a",
  "unspents": [
    {
      "txid": "fae435084345fe26e464994aebc6544875bca0b897bf4ce52a65901ae28ace92",
      "vout": 0,
      "scriptPubKey": "0014a00b1ad58d24ad8433c56662bfb45596cd5fa7d6",
      "desc": "addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)#smk4xmt7",
      "amount": 0.00091190,
      "coinbase": false,
      "height": 852741
+     "blockhash": 000000000000000000004bf6339c59b5c789c8bfd00efc1a4d77d948f1ea328a
    }
  ],
  "total_amount": 0.00091190
}

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

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

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:

        {
            ChainstateManager& chainman = EnsureChainman(node);
            LOCK(cs_main);
            Chainstate& active_chainstate = chainman.ActiveChainstate();
            active_chainstate.ForceFlushStateToDisk();
            pcursor = CHECK_NONFATAL(active_chainstate.CoinsDB().Cursor());
            tip = CHECK_NONFATAL(active_chainstate.m_chain.Tip());
        }
        bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins, node.rpc_interruption_point);
        result.pushKV("success", res);
        result.pushKV("txouts", count);
        result.pushKV("height", tip->nHeight);
        result.pushKV("bestblock", tip->GetBlockHash().GetHex());

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

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

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.

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.

(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?

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

Yes, I agree.

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?

Sure, should also be trivial to add.

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

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

PR #30515 is up, reviews are appreciated.