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.
| Type | Reviewers |
|---|---|
| Concept ACK | fjahr |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
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.
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.
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.