Skip to content

fix(p2p): correlate NOTIFY responses with prior requests; cap flash heights #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raw391 wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

fix(p2p): correlate NOTIFY responses with prior requests; cap flash heights #198

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27d94d9
fix(p2p): correlate NOTIFY responses with prior requests; cap flash h…
tunacanfinder Jun 5, 2026
bf8c16d
fix(p2p): cap outbound flash-height request to the object limit
tunacanfinder Jul 15, 2026
0bad311
fix(p2p): serialize flash-height requests to avoid dropping honest peers
tunacanfinder Aug 4, 2026
1217c16
Merge branch 'dev' into fix/p2p-response-correlation-gate
victor-tucci Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/cryptonote_basic/connection_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ namespace cryptonote
state m_state{state_before_handshake};
std::vector<crypto::hash> m_needed_objects;
std::unordered_set<crypto::hash> m_requested_objects;
std::unordered_set<uint64_t> m_requested_flash_heights;
std::map<uint64_t, std::pair<crypto::hash, bool>> m_flash_state; // HEIGHT => {CHECKSUM, NEEDED}
bool m_need_flash_sync{false};
uint32_t m_drop_count{0}; // How many times we've wanted to drop
Expand Down
44 changes: 43 additions & 1 deletion src/cryptonote_protocol/cryptonote_protocol_handler.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ namespace cryptonote
}


if (context.m_need_flash_sync)
if (context.m_need_flash_sync && context.m_requested_flash_heights.empty())

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Re-arm flash synchronization after a capped request completes.

The cap leaves overflow heights flagged in m_flash_state, but the current path does not guarantee a follow-up request. After handle_response_block_flashes() clears m_requested_flash_heights, an unchanged advertisement reaches the else continue path in process_payload_sync_data() and does not set m_need_flash_sync again. Heights discovered while another request is pending can be stranded in the same way.

Preserve deferred work and schedule a callback after the current response, or re-arm m_need_flash_sync for needed heights that are not part of the completed request.

Also applies to: 192-200, 2551-2558

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cryptonote_protocol/cryptonote_protocol_handler.inl` at line 159, Update
process_payload_sync_data() and the related flash-sync handling around
handle_response_block_flashes() so deferred or capped heights in m_flash_state
re-arm m_need_flash_sync after the current request completes. Ensure the
callback or re-arm occurs after m_requested_flash_heights is cleared, including
heights discovered while another request is pending, while preserving pending
work for subsequent requests.

{
NOTIFY_REQUEST_BLOCK_FLASHES::request r{};
auto curr_height = m_core.get_current_blockchain_height();
Expand Down Expand Up @@ -189,7 +189,15 @@ namespace cryptonote
context.m_need_flash_sync = false;
if (!r.heights.empty())
{
// Cap the outbound request to the protocol object limit. A peer that enforces
// CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT (see handle_request_block_flashes) drops the
// connection on an oversized list, so never send more than the limit in one request. Heights
// beyond the cap stay flagged in m_flash_state and are re-requested when the peer next
// advertises a changed flash set.
if (r.heights.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT)
r.heights.resize(CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT);
MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_BLOCK_FLASHES: requesting flash tx lists for " << r.heights.size() << " blocks");
context.m_requested_flash_heights.insert(r.heights.begin(), r.heights.end());

@coderabbitai coderabbitai Bot Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Cap outbound flash-height requests to the same 500-item protocol limit.

process_payload_sync_data() still accepts up to 1000 advertised flash heights (Lines 391-394), but this path sends every needed height in a single NOTIFY_REQUEST_BLOCK_FLASHES. Once r.heights.size() exceeds CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT, a patched peer will reject our request in handle_request_block_flashes() and disconnect us. Batch the request here or leave the overflow heights pending for a follow-up request.

Suggested fix 
-      context.m_need_flash_sync = false;
+      bool more_flash_heights_pending = false;
+      context.m_need_flash_sync = false;
       if (!r.heights.empty())
       {
+        if (r.heights.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT)
+        {
+          r.heights.resize(CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT);
+          more_flash_heights_pending = true;
+        }
         MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_BLOCK_FLASHES: requesting flash tx lists for " << r.heights.size() << " blocks");
         context.m_requested_flash_heights.insert(r.heights.begin(), r.heights.end());
+        context.m_need_flash_sync = more_flash_heights_pending;
         post_notify<NOTIFY_REQUEST_BLOCK_FLASHES>(r, context);
         MLOG_PEER_STATE("requesting block flashes");
       }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
context.m_requested_flash_heights.insert(r.heights.begin(), r.heights.end());
bool more_flash_heights_pending = false;
context.m_need_flash_sync = false;
if (!r.heights.empty())
{
if (r.heights.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT)
{
r.heights.resize(CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT);
more_flash_heights_pending = true;
}
MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_BLOCK_FLASHES: requesting flash tx lists for " << r.heights.size() << " blocks");
context.m_requested_flash_heights.insert(r.heights.begin(), r.heights.end());
context.m_need_flash_sync = more_flash_heights_pending;
post_notify<NOTIFY_REQUEST_BLOCK_FLASHES>(r, context);
MLOG_PEER_STATE("requesting block flashes");
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cryptonote_protocol/cryptonote_protocol_handler.inl` at line 193, The
outbound flash-height request in `process_payload_sync_data()` can exceed the
protocol’s 500-item limit, causing `handle_request_block_flashes()` on patched
peers to reject and disconnect us. Update the request-building path around
`context.m_requested_flash_heights.insert(...)` and the
`NOTIFY_REQUEST_BLOCK_FLASHES` send logic to cap each batch at
`CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT`, sending only that many heights at
a time and leaving any remaining heights queued for a later follow-up request.

@raw391 raw391 Jul 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capped the sender to match the receiver limit. Overflow heights stay flagged in m_flash_state rather than re-arming m_need_flash_sync here, since the flags aren't cleared until a response lands and re-arming would re-request the in-flight batch.

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raw391, the cap itself is correct, and I agree that immediately re-arming m_need_flash_sync would risk re-requesting the in-flight batch.

However, the overflow is not currently guaranteed a follow-up: on the next unchanged flash advertisement, an existing m_flash_state entry takes the else continue path, so it does not set m_need_flash_sync again. Those overflow heights can therefore remain flagged indefinitely unless that peer advertises a different checksum.

A follow-up should be scheduled only after the current batch is resolved, while ensuring the next request excludes heights already in m_requested_flash_heights (or otherwise tracks completed/requested batches separately).

post_notify<NOTIFY_REQUEST_BLOCK_FLASHES>(r, context);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
MLOG_PEER_STATE("requesting block flashes");
}
Expand Down Expand Up @@ -1218,6 +1226,15 @@ namespace cryptonote
MLOG_P2P_MESSAGE("Received NOTIFY_RESPONSE_GET_BLOCKS (" << arg.blocks.size() << " blocks)");
MLOG_PEER_STATE("received blocks");

if (context.m_state != cryptonote_connection_context::state_synchronizing
|| !context.m_last_request_time
|| context.m_requested_objects.empty())
{
LOG_ERROR_CCONTEXT("Received NOTIFY_RESPONSE_GET_BLOCKS without a pending block request, dropping connection");
drop_connection(context, false, false);
return 1;
}

auto request_time = *context.m_last_request_time;
context.m_last_request_time.reset();

Expand Down Expand Up @@ -2427,6 +2444,15 @@ skip:
{
MLOG_P2P_MESSAGE("Received NOTIFY_RESPONSE_CHAIN_ENTRY: m_block_ids.size()=" << arg.m_block_ids.size()
<< ", m_start_height=" << arg.start_height << ", m_total_height=" << arg.total_height);

if (context.m_state != cryptonote_connection_context::state_synchronizing
|| !context.m_last_request_time
|| !context.m_requested_objects.empty())
{
LOG_ERROR_CCONTEXT("Received NOTIFY_RESPONSE_CHAIN_ENTRY without a pending chain-entry request, dropping connection");
drop_connection(context, false, false);
return 1;
}
MLOG_PEER_STATE("received chain");

context.m_last_request_time.reset();
Expand Down Expand Up @@ -2500,6 +2526,14 @@ skip:
int t_cryptonote_protocol_handler<t_core>::handle_request_block_flashes(int command, NOTIFY_REQUEST_BLOCK_FLASHES::request& arg, cryptonote_connection_context& context)
{
MLOG_P2P_MESSAGE("Received NOTIFY_REQUEST_BLOCK_FLASHES: heights.size()=" << arg.heights.size());

if (arg.heights.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT)
{
LOG_ERROR_CCONTEXT("Too many heights (" << arg.heights.size() << ") in NOTIFY_REQUEST_BLOCK_FLASHES, dropping connection");
drop_connection(context, false, false);
return 1;
}

NOTIFY_RESPONSE_BLOCK_FLASHES::request r;

r.txs = m_core.get_pool().get_mined_flashes({arg.heights.begin(), arg.heights.end()});
Expand All @@ -2514,6 +2548,14 @@ skip:
{
MLOG_P2P_MESSAGE("Received NOTIFY_RESPONSE_BLOCK_FLASHES: txs.size()=" << arg.txs.size());

if (context.m_requested_flash_heights.empty())
{
LOG_ERROR_CCONTEXT("Received NOTIFY_RESPONSE_BLOCK_FLASHES without a pending request, dropping connection");
drop_connection(context, false, false);
return 1;
}
context.m_requested_flash_heights.clear();

m_core.get_pool().keep_missing_flashes(arg.txs);
if (arg.txs.empty())
{
Expand Down
Loading