rpc, refactor: gettxoutsetinfo race condition fix follow-ups
What the developer wrote
Authored by rkrux
rpc, refactor: gettxoutsetinfo race condition fix follow-ups
This patch addresses my own review comments from the review of PR 34451.
If these are found helpful, it makes sense to do them now after the previous
PR was merged and backported.
Pasting the comments below that also explains the changes:
- Move the pindex declaration below now that it is not used earlier.
- 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
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.
What changed, and why it matters
This is a small internal cleanup change for a Bitcoin Core function that calculates statistics about the unspent coin set (used by the gettxoutsetinfo RPC). It moves where a variable is declared and refactors how the statistics object is built so the code is easier to read. There is no direct security fix in this commit itself; it is described as a follow-up to a prior pull request that addressed a race condition.
No immediate action required. Treat as routine code cleanup. Review PR 34451 separately if assessing the original race condition fix.
Security signals we found
Commit title references a prior race-condition fix, but this commit is explicitly a follow-up refactor
No new locks, assertions, or validation logic added
No change to RPC inputs/outputs or network behavior
Code movement only: variable declaration moved, stats construction relocated
Evidence from the diff
The commit refactors ComputeUTXOStats in src/kernel/coinstats.cpp so that the CCoinsStats object and the CCoinsViewCursor are created inside the templated helper rather than in the public wrapper. It also moves a pindex declaration in src/rpc/blockchain.cpp closer to where it is used. The commit message explicitly frames these as non-functional review follow-ups to PR 34451, which fixed a gettxoutsetinfo race condition. The diff shows no new locking, no validation changes, and no behavior change beyond construction location.
Changed components
src/kernel/coinstats.cppsrc/rpc/blockchain.cppgettxoutsetinfo RPC (indirectly, via prior PR)Inspect captured patch +16 / −23
diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp
index 4e30a876..8af546db 100644
--- a/src/kernel/coinstats.cpp
+++ b/src/kernel/coinstats.cpp
@@ -108,9 +108,17 @@ static void ApplyStats(CCoinsStats& stats, const std::map<uint32_t, Coin>& outpu
//! Calculate statistics about the unspent transaction output set
template <typename T>
-static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point, std::unique_ptr<CCoinsViewCursor> pcursor)
+static std::optional<CCoinsStats> ComputeUTXOStats(T hash_obj, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point)
{
+ std::unique_ptr<CCoinsViewCursor> pcursor;
+ CBlockIndex* pindex;
+ {
+ LOCK(::cs_main);
+ pcursor = view->Cursor();
+ pindex = blockman.LookupBlockIndex(pcursor->GetBestBlock());
+ }
assert(pcursor);
+ CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
Txid prevkey;
std::map<uint32_t, Coin> outputs;
@@ -129,7 +137,7 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
stats.coins_count++;
} else {
LogError("%s: unable to read value\n", __func__);
- return false;
+ return std::nullopt;
}
pcursor->Next();
}
@@ -141,42 +149,27 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
FinalizeHash(hash_obj, stats);
stats.nDiskSize = view->EstimateSize();
-
- return true;
+ return stats;
}
std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point)
{
- std::unique_ptr<CCoinsViewCursor> pcursor;
- CBlockIndex* pindex;
- {
- LOCK(::cs_main);
- pcursor = view->Cursor();
- pindex = blockman.LookupBlockIndex(pcursor->GetBestBlock());
- }
- CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
-
- bool success = [&]() -> bool {
+ return [&]() -> std::optional<CCoinsStats> {
switch (hash_type) {
case(CoinStatsHashType::HASH_SERIALIZED): {
HashWriter ss{};
- return ComputeUTXOStats(view, stats, ss, interruption_point, std::move(pcursor));
+ return ComputeUTXOStats(ss, view, blockman, interruption_point);
}
case(CoinStatsHashType::MUHASH): {
MuHash3072 muhash;
- return ComputeUTXOStats(view, stats, muhash, interruption_point, std::move(pcursor));
+ return ComputeUTXOStats(muhash, view, blockman, interruption_point);
}
case(CoinStatsHashType::NONE): {
- return ComputeUTXOStats(view, stats, nullptr, interruption_point, std::move(pcursor));
+ return ComputeUTXOStats(nullptr, view, blockman, interruption_point);
}
} // no default case, so the compiler can warn about missing cases
assert(false);
}();
-
- if (!success) {
- return std::nullopt;
- }
- return stats;
}
static void FinalizeHash(HashWriter& ss, CCoinsStats& stats)
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 64ca01e8..dd243900 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1065,7 +1065,6 @@ static RPCHelpMan gettxoutsetinfo()
{
UniValue ret(UniValue::VOBJ);
- const CBlockIndex* pindex{nullptr};
const CoinStatsHashType hash_type{ParseHashType(self.Arg<std::string_view>("hash_type"))};
bool index_requested = request.params[2].isNull() || request.params[2].get_bool();
@@ -1082,6 +1081,7 @@ static RPCHelpMan gettxoutsetinfo()
blockman = &active_chainstate.m_blockman;
}
+ const CBlockIndex* pindex{nullptr};
if (!request.params[1].isNull()) {
if (!g_coin_stats_index) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Querying specific block heights requires coinstatsindex");
Why this scored 11/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.