← Watch feed
p2p: stop buffered dispatch after fatal notifications
Public commit record
50/100 · Thin
What the developer wrote
Authored by selsta
p2p: stop buffered dispatch after fatal notifications
✓ Specific, descriptive subject✓ Names a concrete action or component! No meaningful explanatory body
What changed, and why it matters
This patch changes how Monero's peer-to-peer networking layer handles bad or rejected messages. Previously, when a message handler decided to drop a peer, it often returned a generic success-like code (1) or a 'handler not defined' error. The patch makes these handlers return a specific connection error code, and makes the lower-level protocol stop processing further buffered messages from that peer when it sees a fatal error. This prevents a misbehaving or malicious peer from forcing the node to keep handling queued messages after the node has already decided to disconnect. The change also ensures notifications (one-way messages) return OK instead of a handler-not-defined error when a command is filtered, avoiding spurious errors.
Recommended action
Treat as a security-relevant hardening patch. Review whether the change fully covers all notify handlers that can drop connections, verify that LEVIN_ERROR_CONNECTION is consistently negative and recognized as fatal by the async handler, and consider backporting to maintained release branches. No immediate emergency response is indicated from the diff alone, but nodes should upgrade in due course.
Security signals we found
01
P2P protocol error-handling consistency fix
02
Prevents continued processing of buffered messages from peers marked for disconnection
03
Replaces magic return value 1 with explicit LEVIN_ERROR_CONNECTION error code
04
Filtered commands now return correct status for notifications vs invocations
05
Peer scoring threshold now triggers immediate connection termination return
Evidence from the diff
The commit modifies the Levin async protocol handler, the abstract invoke dispatcher, the cryptonote protocol handler, and the P2P node server. Key changes: (1) levin_protocol_handler_async.h now checks the return value of notify() and returns false (terminating dispatch) when the result is negative, stopping buffered message dispatch after fatal notifications. (2) levin_abstract_invoke2.h’s END_INVOKE_MAP2 macro returns LEVIN_OK for unknown notify commands and LEVIN_ERROR_CONNECTION_HANDLER_NOT_DEFINED only for invoke commands, matching semantics. (3) cryptonote_protocol_handler.inl replaces many bare ‘return 1;’ statements after drop_connection() with ‘return LEVIN_ERROR_CONNECTION;’, and adds early returns when peer score drops below DROP_PEERS_ON_SCORE. (4) net_node.h returns LEVIN_OK for filtered notify commands instead of LEVIN_ERROR_CONNECTION_HANDLER_NOT_DEFINED. Together these ensure fatal conditions propagate correctly so the connection is closed promptly and queued messages are not processed.
Changed components
contrib/epee/include/net/levin_protocol_handler_async.hcontrib/epee/include/storages/levin_abstract_invoke2.hsrc/cryptonote_protocol/cryptonote_protocol_handler.inlsrc/p2p/net_node.hInspect captured patch +61 / −51
diff --git a/contrib/epee/include/net/levin_protocol_handler_async.h b/contrib/epee/include/net/levin_protocol_handler_async.h
index 805ab4d..8ad40ae 100644
--- a/contrib/epee/include/net/levin_protocol_handler_async.h
+++ b/contrib/epee/include/net/levin_protocol_handler_async.h
@@ -534,7 +534,13 @@ public:
return false;
}
else
- m_config.m_pcommands_handler->notify(m_current_head.m_command, buff_to_invoke, m_connection_context);
+ {
+ const int notify_result = m_config.m_pcommands_handler->notify(
+ m_current_head.m_command, buff_to_invoke, m_connection_context
+ );
+ if(notify_result < 0)
+ return false;
+ }
}
// reuse small buffer
if (!temp.empty() && temp.capacity() <= 64 * 1024)
diff --git a/contrib/epee/include/storages/levin_abstract_invoke2.h b/contrib/epee/include/storages/levin_abstract_invoke2.h
index 8dfc169..7fd52ea 100644
--- a/contrib/epee/include/storages/levin_abstract_invoke2.h
+++ b/contrib/epee/include/storages/levin_abstract_invoke2.h
@@ -212,7 +212,7 @@ namespace epee
#define END_INVOKE_MAP2() \
LOG_ERROR("Unknown command:" << command); \
on_levin_traffic(context, false, false, true, in_buff.size(), "invalid-command"); \
- return LEVIN_ERROR_CONNECTION_HANDLER_NOT_DEFINED; \
+ return is_notify ? LEVIN_OK : LEVIN_ERROR_CONNECTION_HANDLER_NOT_DEFINED; \
} \
catch (const std::exception &e) { \
MERROR("Error in handle_invoke_map: " << e.what()); \
diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
index 7e0fe0b..fbdb2cc 100644
--- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl
+++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
@@ -617,7 +617,7 @@ namespace cryptonote
if (!m_core.check_incoming_block_size(arg.b.block))
{
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
// Parse and quick hash incoming block, dropping the connection on failure
@@ -633,7 +633,7 @@ namespace cryptonote
);
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
// Log block info
@@ -661,7 +661,7 @@ namespace cryptonote
MERROR("sent bad block entry: there are duplicate tx hashes in parsed block: "
<< epee::string_tools::buff_to_hex_nodelimer(arg.b.block));
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
// Keeping a map of the full transactions provided in this payload allows us to pass them
@@ -678,7 +678,7 @@ namespace cryptonote
);
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
// try adding block to the blockchain
@@ -739,7 +739,7 @@ namespace cryptonote
// drop connection and punish peer
LOG_PRINT_CCONTEXT_L0("Block verification failed, dropping connection");
drop_connection_with_score(context, bvc.m_bad_pow ? P2P_IP_FAILS_BEFORE_BLOCK : 1, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
}
else if( bvc.m_added_to_main_chain )
@@ -778,7 +778,7 @@ namespace cryptonote
{
LOG_ERROR_CCONTEXT("Requested fluffy tx before handshake, dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
std::vector<std::pair<cryptonote::blobdata, block>> local_blocks;
@@ -789,7 +789,7 @@ namespace cryptonote
{
LOG_ERROR_CCONTEXT("failed to find block: " << arg.block_hash << ", dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
std::vector<crypto::hash> txids;
@@ -814,7 +814,7 @@ namespace cryptonote
<< ", dropping connection"
);
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
txids.push_back(b.tx_hashes[tx_idx]);
seen[tx_idx] = true;
@@ -831,7 +831,7 @@ namespace cryptonote
);
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
}
@@ -842,14 +842,14 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("Failed to handle request NOTIFY_REQUEST_FLUFFY_MISSING_TX, "
<< "failed to get requested transactions");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (!missed.empty() || txs.size() != txids.size())
{
LOG_ERROR_CCONTEXT("Failed to handle request NOTIFY_REQUEST_FLUFFY_MISSING_TX, "
<< missed.size() << " requested transactions not found" << ", dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
for(auto& tx: txs)
@@ -930,7 +930,7 @@ namespace cryptonote
{
LOG_PRINT_CCONTEXT_L1("Duplicate transaction in notification, dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
}
@@ -964,7 +964,7 @@ namespace cryptonote
{
LOG_PRINT_CCONTEXT_L1("Tx verification failed, dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
switch (tvc.m_relay)
@@ -1008,7 +1008,7 @@ namespace cryptonote
{
LOG_ERROR_CCONTEXT("Requested objects before handshake, dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
MLOG_P2P_MESSAGE("Received NOTIFY_REQUEST_GET_OBJECTS (" << arg.blocks.size() << " blocks)");
if (arg.blocks.size() > CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT)
@@ -1018,7 +1018,7 @@ namespace cryptonote
<< arg.blocks.size() << ") expected not more then "
<< CURRENCY_PROTOCOL_MAX_OBJECT_REQUEST_COUNT);
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
NOTIFY_RESPONSE_GET_OBJECTS::request rsp;
@@ -1026,7 +1026,7 @@ namespace cryptonote
{
LOG_ERROR_CCONTEXT("failed to handle request NOTIFY_REQUEST_GET_OBJECTS, dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
context.m_last_request_time = boost::posix_time::microsec_clock::universal_time();
MLOG_P2P_MESSAGE("-->>NOTIFY_RESPONSE_GET_OBJECTS: blocks.size()="
@@ -1066,7 +1066,7 @@ namespace cryptonote
{
LOG_ERROR_CCONTEXT("Got NOTIFY_RESPONSE_GET_OBJECTS out of the blue, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
context.m_expect_response = 0;
@@ -1097,7 +1097,7 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("sent wrong NOTIFY_HAVE_OBJECTS: no blocks");
drop_connection(context, true, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if(context.m_last_response_height > arg.current_blockchain_height)
{
@@ -1105,13 +1105,15 @@ namespace cryptonote
<< " < m_last_response_height=" << context.m_last_response_height << ", dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (arg.current_blockchain_height < context.m_remote_blockchain_height)
{
MINFO(context << "Claims " << arg.current_blockchain_height << ", claimed " << context.m_remote_blockchain_height << " before");
hit_score(context, 1);
+ if (context.m_score <= DROP_PEERS_ON_SCORE)
+ return LEVIN_ERROR_CONNECTION;
}
context.m_remote_blockchain_height = arg.current_blockchain_height;
if (context.m_remote_blockchain_height > m_core.get_target_blockchain_height())
@@ -1137,7 +1139,7 @@ namespace cryptonote
<< epee::string_tools::buff_to_hex_nodelimer(arg.blocks[i].block) << ", dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (b.miner_tx.vin.size() != 1 || b.miner_tx.vin.front().type() != typeid(txin_gen))
{
@@ -1145,7 +1147,7 @@ namespace cryptonote
<< epee::string_tools::buff_to_hex_nodelimer(arg.blocks[i].block) << ", dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
const auto this_height = boost::get<txin_gen>(b.miner_tx.vin[0]).height;
@@ -1154,7 +1156,7 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("Sent invalid chain");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
// if first block
@@ -1166,7 +1168,7 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("sent block ahead of expected height, dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (this_height == 0 || context.get_expected_hash(this_height - 1) != b.prev_id)
@@ -1174,7 +1176,7 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("Sent invalid chain");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
}
else if (b.prev_id != previous)
@@ -1182,7 +1184,7 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("Sent invalid chain");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
previous = block_hash;
@@ -1191,7 +1193,7 @@ namespace cryptonote
LOG_ERROR_CCONTEXT("Sent invalid chain");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
auto req_it = context.m_requested_objects.find(block_hash);
@@ -1201,7 +1203,7 @@ namespace cryptonote
<< " wasn't requested, dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if(b.tx_hashes.size() != arg.blocks[i].txs.size())
{
@@ -1209,7 +1211,7 @@ namespace cryptonote
<< ", tx_hashes.size()=" << b.tx_hashes.size() << " mismatch with block_complete_entry.m_txs.size()=" << arg.blocks[i].txs.size() << ", dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
context.m_requested_objects.erase(req_it);
@@ -1222,7 +1224,7 @@ namespace cryptonote
<< context.m_requested_objects.size() << "), dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
const bool pruned_ok = should_ask_for_pruned_data(context, start_height, arg.blocks.size(), true);
@@ -1236,14 +1238,14 @@ namespace cryptonote
MERROR(context << "returned a pruned block, dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (block_entry.block_weight)
{
MERROR(context << "returned a block weight for a non pruned block, dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
for (const tx_blob_entry &tx_entry: block_entry.txs)
{
@@ -1252,7 +1254,7 @@ namespace cryptonote
MERROR(context << "returned at least one pruned object which we did not expect, dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
}
}
@@ -1267,7 +1269,7 @@ namespace cryptonote
MERROR(context << "returned at least one pruned block with 0 weight, dropping connection");
drop_connection(context, false, false);
++m_sync_bad_spans_downloaded;
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
}
}
@@ -1795,7 +1797,7 @@ skip:
{
LOG_ERROR_CCONTEXT("Requested chain before handshake, dropping connection");
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
NOTIFY_RESPONSE_CHAIN_ENTRY::request r;
if(!m_core.find_blockchain_supplement(arg.block_ids, !arg.prune, r))
@@ -2498,14 +2500,14 @@ skip:
{
LOG_ERROR_CCONTEXT("Got NOTIFY_RESPONSE_CHAIN_ENTRY out of the blue, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
context.m_expect_response = 0;
if (arg.start_height + 1 > context.m_expect_height) // we expect an overlapping block
{
LOG_ERROR_CCONTEXT("Got NOTIFY_RESPONSE_CHAIN_ENTRY past expected height, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
context.m_last_request_time = boost::date_time::not_a_date_time;
@@ -2516,19 +2518,19 @@ skip:
{
LOG_ERROR_CCONTEXT("sent empty m_block_ids, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (arg.total_height < arg.m_block_ids.size() || arg.start_height > arg.total_height - arg.m_block_ids.size())
{
LOG_ERROR_CCONTEXT("sent invalid start/nblocks/height, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (!arg.m_block_weights.empty() && arg.m_block_weights.size() != arg.m_block_ids.size())
{
LOG_ERROR_CCONTEXT("sent invalid block weight array, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
MDEBUG(context << "first block hash " << arg.m_block_ids.front() << ", last " << arg.m_block_ids.back());
@@ -2536,12 +2538,14 @@ skip:
{
LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_CHAIN_ENTRY, with total_height=" << arg.total_height << " and block_ids=" << arg.m_block_ids.size());
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (arg.total_height < context.m_remote_blockchain_height)
{
MINFO(context << "Claims " << arg.total_height << ", claimed " << context.m_remote_blockchain_height << " before");
hit_score(context, 1);
+ if (context.m_score <= DROP_PEERS_ON_SCORE)
+ return LEVIN_ERROR_CONNECTION;
}
context.m_remote_blockchain_height = arg.total_height;
context.m_last_response_height = arg.start_height + arg.m_block_ids.size()-1;
@@ -2551,7 +2555,7 @@ skip:
<< ", m_start_height=" << arg.start_height
<< ", m_block_ids.size()=" << arg.m_block_ids.size());
drop_connection(context, false, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
uint64_t n_use_blocks = m_core.prevalidate_block_hashes(arg.start_height, arg.m_block_ids, arg.m_block_weights);
@@ -2559,7 +2563,7 @@ skip:
{
LOG_ERROR_CCONTEXT("Most blocks are invalid, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
context.m_expected_heights_start = arg.start_height;
@@ -2577,7 +2581,7 @@ skip:
{
LOG_ERROR_CCONTEXT("Duplicate blocks in chain entry response, dropping connection");
drop_connection_with_score(context, 5, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
int where;
const bool have_block = m_core.have_block_unlocked(arg.m_block_ids[i], &where);
@@ -2588,7 +2592,7 @@ skip:
{
LOG_ERROR_CCONTEXT("First block hash is unknown, dropping connection");
drop_connection_with_score(context, 5, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (!have_block)
expect_unknown = true;
@@ -2605,19 +2609,19 @@ skip:
case HAVE_BLOCK_INVALID:
LOG_ERROR_CCONTEXT("Block is invalid or known without known type, dropping connection");
drop_connection(context, true, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
case HAVE_BLOCK_MAIN_CHAIN:
if (expect_unknown)
{
LOG_ERROR_CCONTEXT("Block is on the main chain, but we did not expect a known block, dropping connection");
drop_connection_with_score(context, 5, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
if (m_core.get_block_id_by_height(arg.start_height + i) != arg.m_block_ids[i])
{
LOG_ERROR_CCONTEXT("Block is on the main chain, but not at the expected height, dropping connection");
drop_connection_with_score(context, 5, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
break;
case HAVE_BLOCK_ALT_CHAIN:
@@ -2625,7 +2629,7 @@ skip:
{
LOG_ERROR_CCONTEXT("Block is on the main chain, but we did not expect a known block, dropping connection");
drop_connection_with_score(context, 5, false);
- return 1;
+ return LEVIN_ERROR_CONNECTION;
}
break;
}
diff --git a/src/p2p/net_node.h b/src/p2p/net_node.h
index b32f79b..36759c3 100644
--- a/src/p2p/net_node.h
+++ b/src/p2p/net_node.h
@@ -308,7 +308,7 @@ namespace nodetool
BEGIN_INVOKE_MAP2(node_server)
if (is_filtered_command(context.m_remote_address, command))
- return LEVIN_ERROR_CONNECTION_HANDLER_NOT_DEFINED;
+ return is_notify ? LEVIN_OK : LEVIN_ERROR_CONNECTION_HANDLER_NOT_DEFINED;
HANDLE_INVOKE_T2(COMMAND_HANDSHAKE, &node_server::handle_handshake)
HANDLE_INVOKE_T2(COMMAND_TIMED_SYNC, &node_server::handle_timed_sync)
Risk score
Our methodology →Why this scored 59/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.