wallet2: fix task lifetime during parsed block processing
What the developer wrote
Authored by selsta
wallet2: fix task lifetime during parsed block processing
What changed, and why it matters
This patch changes how a Monero wallet waits for background worker threads while scanning incoming blocks. Previously, one shared waiter object was used across three separate multi-threaded stages. Now each stage gets its own waiter, so the wallet correctly waits for each batch of tasks to finish before starting the next stage. The commit title says it fixes 'task lifetime' issues, which suggests the old code could have allowed threads to keep running while later code already read or overwrote their data. That kind of bug can lead to crashes or incorrect wallet balance/transaction detection, but the patch does not by itself prove remote theft of funds is possible.
Treat as a stability and probable security fix. Review whether the old shared-waiter behavior could allow a race exploitable with crafted blocks or RPC responses. Backport to maintained branches and monitor for related crash or balance-corruption reports. Consider requesting a security advisory from the Monero maintainers if a concrete exploit path is identified.
Security signals we found
Concurrency/lifetime fix in wallet block processing
Replacement of shared threadpool waiter with per-phase waiters
Potential data race or use-after-scope between parallelized phases
No explicit security disclosure or advisory text in commit
Evidence from the diff
wallet2::process_parsed_blocks() performs three parallelized phases: (1) cache_tx_data, (2) derivation key image/iod processing via gender(), and (3) geniod output scanning. Before the patch, a single tools::threadpool::waiter named ‘waiter’ was declared at function scope and reused for all three phases. The patch removes that shared waiter and introduces three separate waiters: cache_waiter, derivation_waiter, and output_waiter, each scoped to its phase and waited on before the next phase begins. The change ensures task objects and their referenced data (tx_cache_data, geniods, etc.) are not still in flight when subsequent phases access them. The commit message frames this as a ‘task lifetime’ fix, implying the prior waiter reuse could leave earlier tasks alive across phase boundaries, creating use-after-scope or data-race conditions. No explicit security advisory, CVE, or researcher attribution is present in the supplied materials.
Changed components
src/wallet/wallet2.cppwallet2::process_parsed_blocksthreadpool task scheduling and synchronizationInspect captured patch +12 / −10
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 834679f..97b9d71 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -3248,7 +3248,6 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
THROW_WALLET_EXCEPTION_IF(!m_blockchain.is_in_bounds(start_height), error::out_of_hashchain_bounds_error);
tools::threadpool& tpool = tools::threadpool::getInstanceForCompute();
- tools::threadpool::waiter waiter(tpool);
size_t num_txes = 0;
std::vector<tx_cache_data> tx_cache_data;
@@ -3258,6 +3257,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
size_t txidx = 0;
crypto::hash prev_block_id;
bool has_prev_block = m_blockchain.is_in_bounds(start_height - 1);
+ tools::threadpool::waiter cache_waiter(tpool);
if (has_prev_block) {
prev_block_id = m_blockchain[start_height - 1];
}
@@ -3280,16 +3280,16 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
continue;
}
if (m_refresh_type != RefreshNoCoinbase)
- tpool.submit(&waiter, [&, i, txidx](){ cache_tx_data(parsed_blocks[i].block.miner_tx, get_transaction_hash(parsed_blocks[i].block.miner_tx), tx_cache_data[txidx]); });
+ tpool.submit(&cache_waiter, [&, i, txidx](){ cache_tx_data(parsed_blocks[i].block.miner_tx, get_transaction_hash(parsed_blocks[i].block.miner_tx), tx_cache_data[txidx]); });
++txidx;
for (size_t idx = 0; idx < parsed_blocks[i].txes.size(); ++idx)
{
- tpool.submit(&waiter, [&, i, idx, txidx](){ cache_tx_data(parsed_blocks[i].txes[idx], parsed_blocks[i].block.tx_hashes[idx], tx_cache_data[txidx]); });
+ tpool.submit(&cache_waiter, [&, i, idx, txidx](){ cache_tx_data(parsed_blocks[i].txes[idx], parsed_blocks[i].block.tx_hashes[idx], tx_cache_data[txidx]); });
++txidx;
}
}
THROW_WALLET_EXCEPTION_IF(txidx != num_txes, error::wallet_internal_error, "txidx does not match tx_cache_data size");
- THROW_WALLET_EXCEPTION_IF(!waiter.wait(), error::wallet_internal_error, "Exception in thread pool");
+ THROW_WALLET_EXCEPTION_IF(!cache_waiter.wait(), error::wallet_internal_error, "Exception in thread pool");
hw::device &hwdev = m_account.get_device();
hw::reset_mode rst(hwdev);
@@ -3305,11 +3305,12 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
}
};
+ tools::threadpool::waiter derivation_waiter(tpool);
for (size_t i = 0; i < tx_cache_data.size(); ++i)
{
if (tx_cache_data[i].empty())
continue;
- tpool.submit(&waiter, [&gender, &tx_cache_data, i]() {
+ tpool.submit(&derivation_waiter, [&gender, &tx_cache_data, i]() {
auto &slot = tx_cache_data[i];
for (auto &iod: slot.primary)
gender(iod);
@@ -3317,7 +3318,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
gender(iod);
}, true);
}
- THROW_WALLET_EXCEPTION_IF(!waiter.wait(), error::wallet_internal_error, "Exception in thread pool");
+ THROW_WALLET_EXCEPTION_IF(!derivation_waiter.wait(), error::wallet_internal_error, "Exception in thread pool");
auto geniod = [&](const cryptonote::transaction &tx, size_t n_vouts, size_t txidx) {
for (size_t k = 0; k < n_vouts; ++k)
@@ -3349,6 +3350,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
std::vector<geniod_params> geniods;
geniods.reserve(num_txes);
+ tools::threadpool::waiter output_waiter(tpool);
txidx = 0;
uint8_t hf_version_view_tags = get_view_tag_fork();
for (size_t i = 0; i < blocks.size(); ++i)
@@ -3369,7 +3371,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
if (parsed_blocks[i].block.major_version >= hf_version_view_tags)
geniods.push_back(geniod_params{ tx, n_vouts, txidx });
else
- tpool.submit(&waiter, [&, n_vouts, txidx](){ geniod(tx, n_vouts, txidx); }, true);
+ tpool.submit(&output_waiter, [&, n_vouts, txidx](){ geniod(tx, n_vouts, txidx); }, true);
}
}
++txidx;
@@ -3379,7 +3381,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
if (parsed_blocks[i].block.major_version >= hf_version_view_tags)
geniods.push_back(geniod_params{ parsed_blocks[i].txes[j], parsed_blocks[i].txes[j].vout.size(), txidx });
else
- tpool.submit(&waiter, [&, i, j, txidx](){ geniod(parsed_blocks[i].txes[j], parsed_blocks[i].txes[j].vout.size(), txidx); }, true);
+ tpool.submit(&output_waiter, [&, i, j, txidx](){ geniod(parsed_blocks[i].txes[j], parsed_blocks[i].txes[j].vout.size(), txidx); }, true);
++txidx;
}
}
@@ -3398,7 +3400,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
{
size_t batch_end = std::min(batch_start + GENIOD_BATCH_SIZE, geniods.size());
THROW_WALLET_EXCEPTION_IF(batch_end < batch_start, error::wallet_internal_error, "Thread batch end overflow");
- tpool.submit(&waiter, [&geniods, &geniod, batch_start, batch_end]() {
+ tpool.submit(&output_waiter, [&geniods, &geniod, batch_start, batch_end]() {
for (size_t i = batch_start; i < batch_end; ++i)
{
const geniod_params &gp = geniods[i];
@@ -3410,7 +3412,7 @@ void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vect
}
THROW_WALLET_EXCEPTION_IF(num_batch_txes != geniods.size(), error::wallet_internal_error, "txes batched for thread pool did not reach expected value");
}
- THROW_WALLET_EXCEPTION_IF(!waiter.wait(), error::wallet_internal_error, "Exception in thread pool");
+ THROW_WALLET_EXCEPTION_IF(!output_waiter.wait(), error::wallet_internal_error, "Exception in thread pool");
hwdev.set_mode(hw::device::NONE);
Why this scored 51/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.