rpc: add utxo's blockhash and number of confirmations to scantxoutset output

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

This PR resolves #30478 by adding two fields to the scantxoutset RPC:

blockhash: the blockhash that an UTXO was created
confirmations: the number of confirmations an UTXO has relative to the chaintip.

The rationale for the first field is that a blockhash is a much more reliable identifier than the height:

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 https://github.com/bitcoindevkit/bdk/issues/895#issuecomment-1475766797 comment by evanlinjin.

The second one was suggested by maflcko, and I agree it’s useful for human users:

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?

This will yield an RPC output like so:

 0bitcoin-cli scantxoutset start "[\"addr(bc1q5q9344vdyjkcgv79ve3tldz4jmx4lf7knmnx6r)\"]"
 1{
 2  "success": true,
 3  "txouts": 185259116,
 4  "height": 853622,
 5  "bestblock": "00000000000000000002e97d9be8f0ddf31829cf873061b938c10b0f80f708b2",
 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": "00000000000000000002eefe7e7db44d5619c3dace4c65f3fdcd2913d4945c13",
16+     "confirmations": 882
17    }
18  ],
19  "total_amount": 0.00091190
20}

DrahtBot commented at 9:39 pm on July 23, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Eunovo, sipa, tdb3
Stale ACK	maflcko, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on Jul 23, 2024

in src/rpc/blockchain.cpp:1 in b86f4a66a5

maflcko commented at 5:40 am on July 24, 2024:

nit: While touching this, you can also clarify the {RPCResult::Type::NUM, "height", "The current block height (index)" to say “The block height the scan was done at” (or similar), because the current block height may be different from the block height the scan was done at.

Same for "bestblock" (the next field)

in src/rpc/blockchain.cpp:2299 in b86f4a66a5 outdated

2295@@ -2292,6 +2296,7 @@ static RPCHelpMan scantxoutset()
2296             const COutPoint& outpoint = it.first;
2297             const Coin& coin = it.second;
2298             const CTxOut& txo = coin.out;
2299+            const CBlockIndex* pblockindex = (*active_chain)[coin.nHeight];

maflcko commented at 5:43 am on July 24, 2024:

Nit: Because you assume this always exists, may as well use a reference, rather than a pointer (which can be null)

0            const CBlockIndex& coinb_block{*(*active_chain)[coin.nHeight]};

or, if you want to belt-and-suspenders check against a nullptr deref (UB):

0            const CBlockIndex& coinb_block{*CHECK_NONFATAL((*active_chain)[coin.nHeight])};

in src/rpc/blockchain.cpp:2310 in b86f4a66a5 outdated

2307@@ -2303,6 +2308,8 @@ static RPCHelpMan scantxoutset()
2308             unspent.pushKV("amount", ValueFromAmount(txo.nValue));
2309             unspent.pushKV("coinbase", coin.IsCoinBase());
2310             unspent.pushKV("height", (int32_t)coin.nHeight);

maflcko commented at 5:46 am on July 24, 2024:

unrelated nit: coin.nHeight is already a 31-bit unsigned integer, so the cast to (int32_t) doesn’t change the value or the sign, and can be removed. (This will push the value as uint32_t instead, which is fine)

luisschwab commented at 4:48 pm on July 24, 2024:

while we’re at it, should I also remove this cast?

0unspent.pushKV("vout", (int32_t)outpoint.n);

outpoint.n is an uint32_t

maflcko commented at 5:12 am on July 25, 2024:

while we’re at it, should I also remove this cast?

Sure, the cast is completely harmless (because a value high enough for the sign or value to change can’t exist), but the cast is confusing at best, so best to remove.

in src/rpc/blockchain.cpp:2312 in b86f4a66a5 outdated

2307@@ -2303,6 +2308,8 @@ static RPCHelpMan scantxoutset()
2308             unspent.pushKV("amount", ValueFromAmount(txo.nValue));
2309             unspent.pushKV("coinbase", coin.IsCoinBase());
2310             unspent.pushKV("height", (int32_t)coin.nHeight);
2311+            unspent.pushKV("blockhash", pblockindex->GetBlockHash().GetHex());
2312+            unspent.pushKV("confirmations", (int32_t)(tip->nHeight - coin.nHeight + 1));

maflcko commented at 6:27 am on July 24, 2024:

nit: According to https://eel.is/c++draft/conv.prom#5 coin.nHeight in this context will already be promoted to int (int32_t in Bitcoin Core). Also, 1 and tip->nHeight are int, so no need to cast to int to int.

0            unspent.pushKV("confirmations", tip->nHeight - coin.nHeight + 1);

If you do want to explicitly cast, my recommendation would be to use int32_t{...}, which is compile-time safe and print a compile error if the conversion wasn’t value- and sign-preserving.

maflcko approved

maflcko commented at 6:29 am on July 24, 2024: member

Left some style nits, some unrelated to your changes. Also, you can add a test to test/functional/rpc_scantxoutset.py for the new fields, if you want, but this should be fine either way.

ACK b86f4a66a5056a2975902e6e84e324aa6f920ebf

in src/rpc/blockchain.cpp:2193 in 46b82bb628 outdated

2189@@ -2190,7 +2190,7 @@ static RPCHelpMan scantxoutset()
2190             RPCResult{"when action=='start'; only returns after scan completes", RPCResult::Type::OBJ, "", "", {
2191                 {RPCResult::Type::BOOL, "success", "Whether the scan was completed"},
2192                 {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs scanned"},
2193-                {RPCResult::Type::NUM, "height", "The current block height (index)"},
2194+                {RPCResult::Type::NUM, "height", "The block height the scan was done at"},

tdb3 commented at 0:01 am on July 25, 2024:

nitty nit: If you happen to change this file again, could adjust the wording to The block height at which the scan was performed

in src/rpc/blockchain.cpp:2278 in 46b82bb628 outdated

2274@@ -2273,6 +2275,7 @@ static RPCHelpMan scantxoutset()
2275         int64_t count = 0;
2276         std::unique_ptr<CCoinsViewCursor> pcursor;
2277         const CBlockIndex* tip;
2278+        CChain* active_chain = nullptr;

tdb3 commented at 0:21 am on July 25, 2024:

nit: If the file is touched again.

 0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 1index e964a65a05..5df8641f6d 100644
 2--- a/src/rpc/blockchain.cpp
 3+++ b/src/rpc/blockchain.cpp
 4@@ -2275,7 +2275,7 @@ static RPCHelpMan scantxoutset()
 5         int64_t count = 0;
 6         std::unique_ptr<CCoinsViewCursor> pcursor;
 7         const CBlockIndex* tip;
 8-        CChain* active_chain = nullptr;
 9+        CChain* active_chain{nullptr};
10         NodeContext& node = EnsureAnyNodeContext(request.context);
11         {
12             ChainstateManager& chainman = EnsureChainman(node);

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

in src/rpc/blockchain.cpp:2207 in 46b82bb628 outdated

2202@@ -2203,6 +2203,8 @@ static RPCHelpMan scantxoutset()
2203                         {RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " of the unspent output"},
2204                         {RPCResult::Type::BOOL, "coinbase", "Whether this is a coinbase output"},
2205                         {RPCResult::Type::NUM, "height", "Height of the unspent transaction output"},
2206+                        {RPCResult::Type::STR_HEX, "blockhash", "Blockhash of the unspent transaction output"},
2207+                        {RPCResult::Type::NUM, "confirmations", "Number of confirmations of the unspent transaction output"},

tdb3 commented at 1:41 am on July 25, 2024:

It would be great to add tests for these in test/functional/rpc_scantxoutset.py. Maybe something like:

 0diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py
 1index 078a24d577..8edc6d265a 100755
 2--- a/test/functional/rpc_scantxoutset.py
 3+++ b/test/functional/rpc_scantxoutset.py
 4@@ -120,6 +120,11 @@ class ScantxoutsetTest(BitcoinTestFramework):
 5         assert_equal(self.nodes[0].scantxoutset("status"), None)
 6         assert_equal(self.nodes[0].scantxoutset("abort"), False)
 7 
 8+        self.generate(self.nodes[0], 2)
 9+        unspent = self.nodes[0].scantxoutset("start", ["addr(mpQ8rokAhp1TAtJQR6F6TaUmjAWkAWYYBq)"])["unspents"][0]
10+        assert_equal(unspent["height"], info["height"])
11+        assert_equal(unspent["confirmations"], 3)
12+
13         # check that first arg is needed
14         assert_raises_rpc_error(-1, "scantxoutset \"action\" ( [scanobjects,...] )", self.nodes[0].scantxoutset)

maflcko commented at 5:26 am on July 25, 2024:

0                        {RPCResult::Type::NUM, "confirmations", "Number of confirmations of the unspent transaction output when the scan was done"},

nit: (Same wording here, because the confirmations refers in the calculation to the same block)

tdb3 commented at 1:43 am on July 25, 2024: contributor

Approach ACK

Nice addition. Seems to work well.

Ran a manual test where 102 blocks were generated, a spend to bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5 was done, and two more blocks were generated. The scan height was 104, the height of the UTXO was 103, and it’s reported that there are two confirmations.

 0$ src/bitcoin-cli scantxoutset start '["addr(bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5)"]'
 1{
 2  "success": true,
 3  "txouts": 105,
 4  "height": 104,
 5  "bestblock": "4534eca3f55657bfd57620bc0c7f16e0716c97986e31eba8934dd08e5e9970de",
 6  "unspents": [
 7    {
 8      "txid": "f8c8772da1c5a520223c2d8afa0f4c6c7d9876019d27e1797fc8fa74054272db",
 9      "vout": 0,
10      "scriptPubKey": "00141ecfee53fa4d694353f6a24bfdacddbb53128876",
11      "desc": "addr(bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5)#zwy3a86t",
12      "amount": 10.99999577,
13      "coinbase": false,
14      "height": 103,
15      "blockhash": "10f7bc951dac6bca18eca1caf471033230e91caf6086906f40283c7b206ee73c",
16      "confirmations": 2
17    }
18  ],
19  "total_amount": 10.99999577
20}

luisschwab commented at 2:46 am on July 25, 2024: contributor

Fixed nits and added the test

luisschwab requested review from tdb3 on Jul 25, 2024

luisschwab requested review from maflcko on Jul 25, 2024

in src/rpc/blockchain.cpp:2287 in c46b6b0fe6 outdated

2283@@ -2281,6 +2284,7 @@ static RPCHelpMan scantxoutset()
2284             active_chainstate.ForceFlushStateToDisk();
2285             pcursor = CHECK_NONFATAL(active_chainstate.CoinsDB().Cursor());
2286             tip = CHECK_NONFATAL(active_chainstate.m_chain.Tip());
2287+            active_chain = &chainman.ActiveChain();

sipa commented at 3:00 am on July 25, 2024:

I don’t think this is safe. When this scope closes, cs_main is released, and after that point, active_chain points to CChain which may be concurrently modified by another thread (which on itself would be undefined behavior already). Worse, it’s possible that a reorg occurs (however unlikely) which shortens the chain, and at that point, the (*active_chain)[coin.nHeight] call below would be accessing the CChain::vChain vector out of bounds.

But I don’t think you need this at all. tip already captures the CBlockIndex* pointer to the tip of the chain, and all found UTXOs are necessarily created in ancestors of that tip. So instead of (*active_chain)[coin.nHeight] below, you can use tip->GetAncestor(coin.nHeight).

luisschwab commented at 3:27 am on July 25, 2024:

You’re right, it is unsafe. Your solution is a lot cleaner as well.

luisschwab requested review from sipa on Jul 25, 2024

maflcko commented at 5:13 am on July 25, 2024: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

in src/rpc/blockchain.cpp:2194 in 8776b0796a outdated

2189@@ -2190,7 +2190,7 @@ static RPCHelpMan scantxoutset()
2190             RPCResult{"when action=='start'; only returns after scan completes", RPCResult::Type::OBJ, "", "", {
2191                 {RPCResult::Type::BOOL, "success", "Whether the scan was completed"},
2192                 {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs scanned"},
2193-                {RPCResult::Type::NUM, "height", "The current block height (index)"},
2194+                {RPCResult::Type::NUM, "height", "The block height at which the scan was done"},
2195                 {RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at the tip of the chain"},

maflcko commented at 5:15 am on July 25, 2024:

nit: The two fields should have the same wording, as they refer to the same block

maflcko commented at 4:08 pm on July 25, 2024:

0                {RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at which the scan was done"},

maflcko commented at 9:58 am on July 26, 2024:

Just a nit, but I don’t think this was resolved?

luisschwab requested review from maflcko on Jul 25, 2024

tdb3 commented at 0:53 am on July 26, 2024: contributor

This is looking much cleaner. Some commit squashing would help button things up nicely.

luisschwab force-pushed on Jul 26, 2024

luisschwab requested review from tdb3 on Jul 26, 2024

maflcko commented at 10:01 am on July 26, 2024: member

Please squash the commits. No need to leave the buggy commit in, to add those lines in one commit, and then delete them in the next commit. It seems better to squash everything here, so that there are less bugs and less code churn.

luisschwab force-pushed on Jul 26, 2024

in test/functional/rpc_scantxoutset.py:129 in 9c7e1ef8fe outdated

119@@ -120,7 +120,13 @@ def run_test(self):
120         assert_equal(self.nodes[0].scantxoutset("status"), None)
121         assert_equal(self.nodes[0].scantxoutset("abort"), False)
122 
123-        # check that first arg is needed
124+        # Check that confirmations field is correct
125+        self.generate(self.nodes[0], 2)
126+        unspent = self.nodes[0].scantxoutset("start", ["addr(mpQ8rokAhp1TAtJQR6F6TaUmjAWkAWYYBq)"])["unspents"][0]
127+        assert_equal(unspent["height"], info["height"])
128+        assert_equal(unspent["confirmations"], 3)

Eunovo commented at 3:49 pm on July 26, 2024:

Can you also add an assert checking that the blockhash is correct?

luisschwab commented at 8:56 pm on July 27, 2024:

thoughts @Eunovo?

0        # Check that the blockhash and confirmations fields are correct
1        self.generate(self.nodes[0], 2)
2        unspent = self.nodes[0].scantxoutset("start", ["addr(mpQ8rokAhp1TAtJQR6F6TaUmjAWkAWYYBq)"])["unspents"][0]
3        blockhash = self.nodes[0].getblockhash(info["height"])
4        assert_equal(unspent["height"], info["height"])
5        assert_equal(unspent["blockhash"], blockhash)
6        assert_equal(unspent["confirmations"], 3)

Eunovo commented at 9:45 pm on July 27, 2024:

This works but you can also get the blockhash from the first call to generate in https://github.com/bitcoin/bitcoin/blob/9c7e1ef8feafd468b35da52c1c2839d4d33d2138/test/functional/rpc_scantxoutset.py#L60 blockhash = self.generate(self.nodes[0], 1)[0]

Either way is fine to me

luisschwab commented at 9:54 pm on July 27, 2024:

True, but I think my way leaves it more explicit.

luisschwab commented at 4:30 pm on July 26, 2024: contributor

Done, commits squashed into one.

BrandonOdiwuor approved

BrandonOdiwuor commented at 7:16 pm on July 26, 2024: contributor

Tested ACK 9c7e1ef8feafd468b35da52c1c2839d4d33d2138

Screenshot from 2024-07-26 21-57-15

rpc: add utxo's blockhash and number of confirmations to scantxoutset output 17845e7f21

luisschwab force-pushed on Jul 27, 2024

Eunovo approved

Eunovo commented at 10:05 pm on July 27, 2024: none

ACK https://github.com/bitcoin/bitcoin/pull/30515/commits/17845e7f219e2281cd7a51d2cfe67b22eb40c4ba

DrahtBot requested review from BrandonOdiwuor on Jul 27, 2024

sipa commented at 10:10 pm on July 27, 2024: member

utACK 17845e7f219e2281cd7a51d2cfe67b22eb40c4ba

tdb3 approved

tdb3 commented at 10:45 pm on July 27, 2024: contributor

ACK 17845e7f219e2281cd7a51d2cfe67b22eb40c4ba Diff showed just the test modification. Ran unit and functional tests locally (passed).

fanquake merged this on Jul 28, 2024

fanquake closed this on Jul 28, 2024

rpc: add utxo’s blockhash and number of confirmations to scantxoutset output #30515

Code Coverage

Reviews