What the developer wrote
Authored by tobtoht
Merge pull request #11366
109b58b wallet2: bound the timestamp-to-height search (Thomas)
ACKs: jpk68, SNeedlewoods
What changed, and why it matters
This change fixes a binary search in the Monero wallet that previously could loop forever or behave incorrectly if something went wrong. The old code used an unbounded 'while (true)' loop and a midpoint calculation that could overflow. The patch limits the loop to 64 iterations (enough for any blockchain height) and switches to a safer midpoint formula. In the worst case, the old loop might never terminate, causing the wallet to hang when looking up a blockchain height from a timestamp.
Treat as a low-severity hardening fix. No urgent action required unless the wallet is observed hanging during timestamp-to-height lookups. Reviewers should verify that 64 iterations is sufficient for current and foreseeable blockchain heights and that the new midpoint arithmetic behaves correctly near boundary conditions.
Security signals we found
Unbounded loop replaced with bounded iteration
Integer overflow mitigation in midpoint calculation
Defensive error handling added for search failure
Evidence from the diff
The function wallet2::get_blockchain_height_by_timestamp() performs a binary search over blockchain heights to find the height closest to a target timestamp. The patch makes two defensive changes: (1) replaces ‘while (true)’ with a bounded ‘for’ loop of 64 iterations, since the search range halves each time and 2^64 exceeds any realistic height; (2) replaces ‘(height_min + height_max) / 2’ with ‘height_min + (height_max - height_min) / 2’ to avoid potential unsigned 64-bit integer overflow in the midpoint calculation. It also adds a final throw if the loop exits without returning. These are robustness fixes against infinite loops and arithmetic overflow, not a confirmed remotely exploitable vulnerability.
Changed components
src/wallet/wallet2.cppwallet2::get_blockchain_height_by_timestampInspect captured patch +4 / −2
### src/wallet/wallet2.cpp
@@ -15397,11 +15397,12 @@ uint64_t wallet2::get_blockchain_height_by_timestamp(uint64_t timestamp_target)
throw std::runtime_error("failed to get blockchain height");
}
height_max--;
- while (true)
+ // the range halves every iteration, so 64 steps always suffice
+ for (unsigned iterations = 0; iterations < 64; ++iterations)
{
COMMAND_RPC_GET_BLOCKS_BY_HEIGHT::request req;
COMMAND_RPC_GET_BLOCKS_BY_HEIGHT::response res;
- uint64_t height_mid = (height_min + height_max) / 2;
+ uint64_t height_mid = height_min + (height_max - height_min) / 2;
req.heights =
{
height_min,
@@ -15461,6 +15462,7 @@ uint64_t wallet2::get_blockchain_height_by_timestamp(uint64_t timestamp_target)
return height_min;
}
}
+ throw std::runtime_error("failed to find the blockchain height for the given timestamp");
}
//----------------------------------------------------------------------------------------------------
bool wallet2::is_synced()Why this scored 34/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.