rpc: fix race condition in gettxoutsetinfo#34451
rpc: fix race condition in gettxoutsetinfo#34451w0xlt wants to merge 2 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2026-03-11 00:56:26 |
|
Concept ACK Do you have a hint on how to reproduce the failure? |
| // If the coinstats index isn't requested or is otherwise not usable, the | ||
| // pindex should either be null or equal to the view's best block. This is | ||
| // because without the coinstats index we can only get coinstats about the | ||
| // best block. | ||
| CHECK_NONFATAL(!pindex || pindex->GetBlockHash() == view->GetBestBlock()); |
There was a problem hiding this comment.
Why remove this? Now that pindex is null, it should pass...
There was a problem hiding this comment.
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. |
To make a test that reliably reproduces the race condition detected by the Antithesis tool, we would need a way to force Given that, the PR instead removes the early capture of pindex, eliminating this scenario.
Done in 8305f29. Thanks. |
|
CI error is unrelated. |
It would be possible to add an |
|
The below commit adds a test that fails on master with the following AssertionError: Not sure if it should be added here because since it introduces test code in |
| { | ||
| LOCK(::cs_main); | ||
| pindex = blockman.LookupBlockIndex(view->GetBestBlock()); | ||
| pcursor = view->Cursor(); |
There was a problem hiding this comment.
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?
diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
index e1d3fff5ea..c9049ea7a3 100644
--- a/src/kernel/coinstats.cpp
+++ b/src/kernel/coinstats.cpp
@@ -151,7 +151,3 @@ std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV
- std::unique_ptr<CCoinsViewCursor> pcursor;
- CBlockIndex* pindex;
- {
- LOCK(::cs_main);
- pindex = blockman.LookupBlockIndex(view->GetBestBlock());
- pcursor = view->Cursor();
- }
+ std::unique_ptr<CCoinsViewCursor> pcursor = view->Cursor();
+ CBlockIndex* pindex = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(pcursor->GetBestBlock()));There was a problem hiding this comment.
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:
diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
index e1d3fff5ea..79da8ea4d4 100644
--- a/src/kernel/coinstats.cpp
+++ b/src/kernel/coinstats.cpp
@@ -152,8 +152,8 @@ std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV
CBlockIndex* pindex;
{
LOCK(::cs_main);
- pindex = blockman.LookupBlockIndex(view->GetBestBlock());
pcursor = view->Cursor();
+ pindex = blockman.LookupBlockIndex(pcursor->GetBestBlock());
}
CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};Fix an assertion failure in gettxoutsetinfo (issue bitcoin#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.
Fixes #34263
A
CHECK_NONFATALassertion failure could occur ingettxoutsetinfowhen a new block was connected between capturingpindexand validating it inGetUTXOStats().The fix removes the early
pindexcapture sinceComputeUTXOStats()independently fetches the current best block under lock. The response now usesstats.hashBlockandstats.nHeight(the actual computed values) instead of the potentially stalepindex.