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.