cryptonote_protocol: cleanup_handle_incoming_blocks on scope exit
What the developer wrote
Authored by j-berman
cryptonote_protocol: cleanup_handle_incoming_blocks on scope exit
What changed, and why it matters
This change refactors how a Monero node cleans up after receiving blocks. Previously, every early-exit path had to remember to call a cleanup routine and remove a queued block span; now a single scope-exit handler does it automatically. The main risk is that any path that used to skip cleanup now runs it, or that the new handler runs in a different order with surrounding code. The patch appears intended to prevent resource leaks or stuck block queues when a peer misbehaves or the node is shutting down.
Review the cleanup_handle_incoming_blocks implementation for idempotency and thread safety when invoked from a scope-exit destructor, especially because destructors run during stack unwinding after exceptions. Verify that remove_spans is safe to call after cleanup failure and that the stopped flag prevents unwanted span removal only on the intended shutdown path. Consider whether cleanup_on_exit.reset() before the final height check could leave spans unremoved if that later code throws.
Security signals we found
Refactor of cleanup/error-handling paths
Scope-exit guard introduced to prevent missing cleanup on early return
Potential change in cleanup ordering and re-entrancy behavior
Block queue span removal moved to a single deferred path
Evidence from the diff
The commit replaces multiple explicit calls to m_core.cleanup_handle_incoming_blocks() and m_block_queue.remove_spans() with an epee::misc_utils::create_scope_leave_handler lambda. The lambda captures this, stopped, context, span_connection_id, and start_height. It runs cleanup, then, unless the node was stopping, removes spans. Early returns now rely on the destructor of the scope-leave object. The m_stopping branch sets a flag and returns instead of calling cleanup directly. At the normal exit the handler is manually reset (cleanup_on_exit.reset()) before the function continues.
Changed components
src/cryptonote_protocol/cryptonote_protocol_handler.inlcryptonote protocol block download/processing handlerm_core.cleanup_handle_incoming_blocksm_block_queue.remove_spansInspect captured patch +17 / −32
diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
index bcf4471..e171007 100644
--- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl
+++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
@@ -1493,14 +1493,28 @@ namespace cryptonote
return 1;
}
+ bool stopped = false;
+ auto cleanup_on_exit = epee::misc_utils::create_scope_leave_handler([this, &stopped, &context, span_connection_id, start_height]() {
+ if (!m_core.cleanup_handle_incoming_blocks())
+ {
+ LOG_PRINT_CCONTEXT_L0("Failure in cleanup_handle_incoming_blocks");
+ return;
+ }
+
+ if (stopped)
+ return;
+
+ m_block_queue.remove_spans(span_connection_id, start_height);
+ });
+
uint64_t block_process_time_full = 0, transactions_process_time_full = 0;
size_t num_txs = 0, blockidx = 0;
for(const block_complete_entry& block_entry: blocks)
{
if (m_stopping)
{
- m_core.cleanup_handle_incoming_blocks();
- return 1;
+ stopped = true;
+ return 1;
}
// process transactions
@@ -1519,13 +1533,6 @@ namespace cryptonote
}))
LOG_ERROR_CCONTEXT("span connection id not found");
- if (!m_core.cleanup_handle_incoming_blocks())
- {
- LOG_PRINT_CCONTEXT_L0("Failure in cleanup_handle_incoming_blocks");
- return 1;
- }
- // in case the peer had dropped beforehand, remove the span anyway so other threads can wake up and get it
- m_block_queue.remove_spans(span_connection_id, start_height);
return 1;
}
TIME_MEASURE_FINISH(transactions_process_time);
@@ -1552,14 +1559,6 @@ namespace cryptonote
}))
LOG_ERROR_CCONTEXT("span connection id not found");
- if (!m_core.cleanup_handle_incoming_blocks())
- {
- LOG_PRINT_CCONTEXT_L0("Failure in cleanup_handle_incoming_blocks");
- return 1;
- }
-
- // in case the peer had dropped beforehand, remove the span anyway so other threads can wake up and get it
- m_block_queue.remove_spans(span_connection_id, start_height);
return 1;
}
if(bvc.m_marked_as_orphaned)
@@ -1572,14 +1571,6 @@ namespace cryptonote
}))
LOG_ERROR_CCONTEXT("span connection id not found");
- if (!m_core.cleanup_handle_incoming_blocks())
- {
- LOG_PRINT_CCONTEXT_L0("Failure in cleanup_handle_incoming_blocks");
- return 1;
- }
-
- // in case the peer had dropped beforehand, remove the span anyway so other threads can wake up and get it
- m_block_queue.remove_spans(span_connection_id, start_height);
return 1;
}
@@ -1591,13 +1582,7 @@ namespace cryptonote
MDEBUG(context << "Block process time (" << blocks.size() << " blocks, " << num_txs << " txs): " << block_process_time_full + transactions_process_time_full << " (" << transactions_process_time_full << "/" << block_process_time_full << ") ms");
- if (!m_core.cleanup_handle_incoming_blocks())
- {
- LOG_PRINT_CCONTEXT_L0("Failure in cleanup_handle_incoming_blocks");
- return 1;
- }
-
- m_block_queue.remove_spans(span_connection_id, start_height);
+ cleanup_on_exit.reset();
const uint64_t current_blockchain_height = m_core.get_current_blockchain_height();
if (current_blockchain_height > previous_height)
Why this scored 42/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.