Fixes #34263
A CHECK_NONFATAL assertion failure could occur in gettxoutsetinfo when a new block was connected between capturing pindex and validating it in GetUTXOStats().
The fix removes the early pindex capture since ComputeUTXOStats() independently fetches the current best block under lock. The response now uses stats.hashBlock and stats.nHeight (the actual computed values) instead of the potentially stale pindex.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
BlockMap and CChain Concurrency Improvement by alexanderwiederin)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.
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
2026-03-11 00:56:26
Concept ACK
Do you have a hint on how to reproduce the failure?
968@@ -969,12 +969,6 @@ static std::optional<kernel::CCoinsStats> GetUTXOStats(CCoinsView* view, node::B
969 }
970 }
971
972- // If the coinstats index isn't requested or is otherwise not usable, the
973- // pindex should either be null or equal to the view's best block. This is
974- // because without the coinstats index we can only get coinstats about the
975- // best block.
976- CHECK_NONFATAL(!pindex || pindex->GetBlockHash() == view->GetBestBlock());
I removed it because it seemed redundant given the current gettxoutsetinfo flow (normally pindex is nullptr unless querying a specific block, which already requires the index).
However, it may be better to keep it to prevent future misuse of the function with a stale pindex.
Reintroduced in 0672e1c
I believe there is still a race, just not properly handled with this PR.
ComputeUTXOStats sets nHeight and hashBlock without any lock held, and before/separately from acquiring the view’s cursor.
Do you have a hint on how to reproduce the failure?
To make a test that reliably reproduces the race condition detected by the Antithesis tool, we would need a way to force CoinsDB()->GetBestBlock() to change between the initial capture of pindex and the subsequent GetUTXOStats() call. There is currently no deterministic “pause point” in the code to do this.
Given that, the PR instead removes the early capture of pindex, eliminating this scenario.
ComputeUTXOStats sets nHeight and hashBlock without holding any lock, and before (or separately from) acquiring the view’s cursor.
Done in 8305f29. Thanks.
There is currently no deterministic “pause point” in the code to do this.
It would be possible to add an UninterruptibleSleep(100ms); (or so) in the right place to make this deterministic? Having such a patch would make testing easier.
The below commit adds a test that fails on master with the following AssertionError:
gettxoutsetinfo failed: Internal bug detected: !pindex || pindex->GetBlockHash() == view->GetBestBlock()
The error appears in the logs on master, but the test passes on this PR.
https://github.com/w0xlt/bitcoin/commit/beca478400c47a646f11ed4a057ff637592dbc67
Not sure if it should be added here because since it introduces test code in gettxoutsetinfo.
152+ std::unique_ptr<CCoinsViewCursor> pcursor;
153+ CBlockIndex* pindex;
154+ {
155+ LOCK(::cs_main);
156+ pindex = blockman.LookupBlockIndex(view->GetBestBlock());
157+ pcursor = view->Cursor();
It doesn’t really make a difference, since we are holding cs_main here, but might it be a bit clearer, if we’d read GetBestBlock from the cursor?
0diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
1index e1d3fff5ea..c9049ea7a3 100644
2--- a/src/kernel/coinstats.cpp
3+++ b/src/kernel/coinstats.cpp
4@@ -151,7 +151,3 @@ std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV
5- std::unique_ptr<CCoinsViewCursor> pcursor;
6- CBlockIndex* pindex;
7- {
8- LOCK(::cs_main);
9- pindex = blockman.LookupBlockIndex(view->GetBestBlock());
10- pcursor = view->Cursor();
11- }
12+ std::unique_ptr<CCoinsViewCursor> pcursor = view->Cursor();
13+ CBlockIndex* pindex = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(pcursor->GetBestBlock()));
Thanks for the suggestion! Adopted the pcursor->GetBestBlock() approach - it does make the intent clearer since pindex is explicitly tied to the cursor’s snapshot.
One thing to note: cs_main is still held during Cursor() because internally CCoinsViewDB::Cursor() calls GetBestBlock() and NewIterator() as two separate operations.
So if I understand correctly, without the lock, a concurrent FlushStateToDisk could slip between them, causing the cursor’s hashBlock to be from a different state than its iterator data - the same kind of mismatch this PR is fixing. I changed the diff to:
0diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
1index e1d3fff5ea..79da8ea4d4 100644
2--- a/src/kernel/coinstats.cpp
3+++ b/src/kernel/coinstats.cpp
4@@ -152,8 +152,8 @@ std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV
5 CBlockIndex* pindex;
6 {
7 LOCK(::cs_main);
8- pindex = blockman.LookupBlockIndex(view->GetBestBlock());
9 pcursor = view->Cursor();
10+ pindex = blockman.LookupBlockIndex(pcursor->GetBestBlock());
11 }
12 CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
Fix an assertion failure in gettxoutsetinfo (issue #34263) caused by
capturing the best block before releasing cs_main, then checking it
against a potentially newer best block in GetUTXOStats().
Remove the early pindex capture since ComputeUTXOStats() independently
fetches the current best block under lock. Use stats.hashBlock and
stats.nHeight (the actual computed values) instead of the potentially
stale pindex when building the response.
Acquire the cursor and block index under the same cs_main lock to
eliminate a potential race where a new block could be connected
between capturing the block info and acquiring the cursor, causing
the reported stats to reference a different block than the one
being iterated.
utACK f3bf63ec4f028cf9ee0820226f44fcbe26d358c9
Code changes look good to me. I didn’t try to reproduce the race.
1102@@ -1104,7 +1103,7 @@ static RPCHelpMan gettxoutsetinfo()
1103
1104 // If a specific block was requested and the index has already synced past that height, we can return the
1105 // data already even though the index is not fully synced yet.
1106- if (pindex->nHeight > summary.best_block_height) {
1107+ if (pindex && pindex->nHeight > summary.best_block_height) {
In 5e77072fa60110a00d2bff31798d58b6c10bd3da “rpc: fix race condition in gettxoutsetinfo”
Can move the pindex declaration below now that it is not used earlier.
0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
1index c631a93674..aea8521a02 100644
2--- a/src/rpc/blockchain.cpp
3+++ b/src/rpc/blockchain.cpp
4@@ -1065,7 +1065,6 @@ static RPCHelpMan gettxoutsetinfo()
5 {
6 UniValue ret(UniValue::VOBJ);
7
8- const CBlockIndex* pindex{nullptr};
9 const CoinStatsHashType hash_type{ParseHashType(self.Arg<std::string_view>("hash_type"))};
10 bool index_requested = request.params[2].isNull() || request.params[2].get_bool();
11
12@@ -1082,6 +1081,7 @@ static RPCHelpMan gettxoutsetinfo()
13 blockman = &active_chainstate.m_blockman;
14 }
15
16+ const CBlockIndex* pindex{nullptr};
17 if (!request.params[1].isNull()) {
18 if (!g_coin_stats_index) {
19 throw JSONRPCError(RPC_INVALID_PARAMETER, "Querying specific block heights requires coinstatsindex");
20(END)
108@@ -109,9 +109,8 @@ static void ApplyStats(CCoinsStats& stats, const std::map<uint32_t, Coin>& outpu
109
110 //! Calculate statistics about the unspent transaction output set
111 template <typename T>
112-static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point)
113+static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point, std::unique_ptr<CCoinsViewCursor> pcursor)
In f3bf63ec4f028cf9ee0820226f44fcbe26d358c9 “kernel: acquire coinstats cursor and block info atomically”
stats was being generated partially in both these ComputeUTXOStats functions, which reads oddly to me. Now that the pcursor is also moved and passed to this function, which reads oddly as well, I believe we can refactor this function in this PR to completely build the stats inside this function.
A side benefit is that by removing the stats and pcursor arguments, the function signature becomes quite similar to its namesake, which in turn becomes a straightforward wrapper of this function.
Builds fine, unit tests pass.
0diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
1index 49b51d64f3..69c4b44f2f 100644
2--- a/src/kernel/coinstats.cpp
3+++ b/src/kernel/coinstats.cpp
4@@ -109,9 +109,17 @@ static void ApplyStats(CCoinsStats& stats, const std::map<uint32_t, Coin>& outpu
5
6 //! Calculate statistics about the unspent transaction output set
7 template <typename T>
8-static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point, std::unique_ptr<CCoinsViewCursor> pcursor)
9+static std::optional<CCoinsStats> ComputeUTXOStats(T hash_obj, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point)
10 {
11+ std::unique_ptr<CCoinsViewCursor> pcursor;
12+ CBlockIndex* pindex;
13+ {
14+ LOCK(::cs_main);
15+ pcursor = view->Cursor();
16+ pindex = blockman.LookupBlockIndex(pcursor->GetBestBlock());
17+ }
18 assert(pcursor);
19+ CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
20
21 Txid prevkey;
22 std::map<uint32_t, Coin> outputs;
23@@ -130,7 +138,7 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
24 stats.coins_count++;
25 } else {
26 LogError("%s: unable to read value\n", __func__);
27- return false;
28+ return std::nullopt;
29 }
30 pcursor->Next();
31 }
32@@ -142,42 +150,27 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
33 FinalizeHash(hash_obj, stats);
34
35 stats.nDiskSize = view->EstimateSize();
36-
37- return true;
38+ return stats;
39 }
40
41 std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point)
42 {
43- std::unique_ptr<CCoinsViewCursor> pcursor;
44- CBlockIndex* pindex;
45- {
46- LOCK(::cs_main);
47- pcursor = view->Cursor();
48- pindex = blockman.LookupBlockIndex(pcursor->GetBestBlock());
49- }
50- CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
51-
52- bool success = [&]() -> bool {
53+ return [&]() -> std::optional<CCoinsStats> {
54 switch (hash_type) {
55 case(CoinStatsHashType::HASH_SERIALIZED): {
56 HashWriter ss{};
57- return ComputeUTXOStats(view, stats, ss, interruption_point, std::move(pcursor));
58+ return ComputeUTXOStats(ss, view, blockman, interruption_point);
59 }
60 case(CoinStatsHashType::MUHASH): {
61 MuHash3072 muhash;
62- return ComputeUTXOStats(view, stats, muhash, interruption_point, std::move(pcursor));
63+ return ComputeUTXOStats(muhash, view, blockman, interruption_point);
64 }
65 case(CoinStatsHashType::NONE): {
66- return ComputeUTXOStats(view, stats, nullptr, interruption_point, std::move(pcursor));
67+ return ComputeUTXOStats(nullptr, view, blockman, interruption_point);
68 }
69 } // no default case, so the compiler can warn about missing cases
70 assert(false);
71 }();
72-
73- if (!success) {
74- return std::nullopt;
75- }
76- return stats;
77 }
78
79 static void FinalizeHash(HashWriter& ss, CCoinsStats& stats)
Concept ACK f3bf63ec4f028cf9ee0820226f44fcbe26d358c9 for removal of the race condition.
Suggested some refactoring.
if you want to re-run through antithesis.
Looks like this fixes it