Repository: bitcoin/bitcoin (language: cpp)

Issue:
`scantxoutset` should output the block hash instead of block height
### 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](https://github.com/bitcoindevkit/bdk/issues/895#issuecomment-1475766797) comment by @evanlinjin.

The optimal behaviour should be instead to show block **hash**:

```diff
~$ 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_


Discussion / hints:
> 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:


```cpp
        {
            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.

Fix patch:
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 9899a13a1e3bc..217f2fd0b90fd 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2190,7 +2190,7 @@ static RPCHelpMan scantxoutset()
             RPCResult{"when action=='start'; only returns after scan completes", RPCResult::Type::OBJ, "", "", {
                 {RPCResult::Type::BOOL, "success", "Whether the scan was completed"},
                 {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs scanned"},
-                {RPCResult::Type::NUM, "height", "The current block height (index)"},
+                {RPCResult::Type::NUM, "height", "The block height at which the scan was done"},
                 {RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at the tip of the chain"},
                 {RPCResult::Type::ARR, "unspents", "",
                 {
@@ -2203,6 +2203,8 @@ static RPCHelpMan scantxoutset()
                         {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the unspent output"},
                         {RPCResult::Type::BOOL, "coinbase", "Whether this is a coinbase output"},
                         {RPCResult::Type::NUM, "height", "Height of the unspent transaction output"},
+                        {RPCResult::Type::STR_HEX, "blockhash", "Blockhash of the unspent transaction output"},
+                        {RPCResult::Type::NUM, "confirmations", "Number of confirmations of the unspent transaction output when the scan was done"},
                     }},
                 }},
                 {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount of all found unspent outputs in " + CURRENCY_UNIT},
@@ -2292,17 +2294,20 @@ static RPCHelpMan scantxoutset()
             const COutPoint& outpoint = it.first;
             const Coin& coin = it.second;
             const CTxOut& txo = coin.out;
+            const CBlockIndex& coinb_block{*CHECK_NONFATAL(tip->GetAncestor(coin.nHeight))};
             input_txos.push_back(txo);
             total_in += txo.nValue;
 
             UniValue unspent(UniValue::VOBJ);
             unspent.pushKV("txid", outpoint.hash.GetHex());
-            unspent.pushKV("vout", (int32_t)outpoint.n);
+            unspent.pushKV("vout", outpoint.n);
             unspent.pushKV("scriptPubKey", HexStr(txo.scriptPubKey));
             unspent.pushKV("desc", descriptors[txo.scriptPubKey]);
             unspent.pushKV("amount", ValueFromAmount(txo.nValue));
             unspent.pushKV("coinbase", coin.IsCoinBase());
-            unspent.pushKV("height", (int32_t)coin.nHeight);
+            unspent.pushKV("height", coin.nHeight);
+            unspent.pushKV("blockhash", coinb_block.GetBlockHash().GetHex());
+            unspent.pushKV("confirmations", tip->nHeight - coin.nHeight + 1);
 
             unspents.push_back(std::move(unspent));
         }


Test patch:
diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py
index 078a24d577fda..e53e2fa910740 100755
--- a/test/functional/rpc_scantxoutset.py
+++ b/test/functional/rpc_scantxoutset.py
@@ -120,7 +120,15 @@ def run_test(self):
         assert_equal(self.nodes[0].scantxoutset("status"), None)
         assert_equal(self.nodes[0].scantxoutset("abort"), False)
 
-        # check that first arg is needed
+        # Check that the blockhash and confirmations fields are correct
+        self.generate(self.nodes[0], 2)
+        unspent = self.nodes[0].scantxoutset("start", ["addr(mpQ8rokAhp1TAtJQR6F6TaUmjAWkAWYYBq)"])["unspents"][0]
+        blockhash = self.nodes[0].getblockhash(info["height"])
+        assert_equal(unspent["height"], info["height"])
+        assert_equal(unspent["blockhash"], blockhash)
+        assert_equal(unspent["confirmations"], 3)
+
+        # Check that first arg is needed
         assert_raises_rpc_error(-1, "scantxoutset \"action\" ( [scanobjects,...] )", self.nodes[0].scantxoutset)
 
         # Check that second arg is needed for start