p2p: fix race causing dropped connections during sync
What the developer wrote
Authored by j-berman
p2p: fix race causing dropped connections during sync
Without this commit:
1) read height from DB
2) add block to chain in separate thread
3) read chain for block id's and request them from peer
4) ERR in handle_response_chain_entry, peer's first block is the
one that was added to the chain, which has block idx=height from
step 1.
This commit reads the chain for height and highest block id's
in one go while holding the m_blockchain_lock to avoid the race.
What changed, and why it matters
This patch fixes a timing bug in Monero's peer-to-peer syncing. Previously, a node could read its own blockchain height, then a new block could be added in another thread, and then the node would ask a peer for older blocks using an out-of-date height. This mismatch could cause the node to think the peer's first returned block was wrong and drop the connection, making sync slower or less reliable. The fix reads the height and the list of block hashes together under the same lock so they cannot get out of step.
Treat as a reliability/DoS-hardening fix. Nodes should upgrade to avoid unnecessary sync failures and dropped peer connections. No immediate emergency response is warranted, but the fix should be included in the next release.
Security signals we found
Race condition between blockchain height read and short-chain history read
P2P connection drop during synchronization
Missing atomic snapshot across height and chain history
Potential denial-of-service via sync disruption
Evidence from the diff
The commit resolves a race condition in get_short_chain_history. Before, cryptonote_protocol_handler called m_core.get_current_blockchain_height() and then m_core.get_short_chain_history() as two separate operations. Because get_short_chain_history takes m_blockchain_lock only internally, another thread could add a block between the two calls, so the requested block_ids would reflect a chain one block longer than m_expect_height. In handle_response_chain_entry, the peer’s first block would then have an index equal to the stale expected height, triggering an error and dropping the connection. The patch changes get_short_chain_history to return both the chain history and the exact height at which it was captured, both under m_blockchain_lock, and updates callers to use that single snapshot.
Changed components
src/cryptonote_core/blockchain.cppsrc/cryptonote_core/blockchain.hsrc/cryptonote_core/cryptonote_core.cppsrc/cryptonote_core/cryptonote_core.hsrc/cryptonote_protocol/cryptonote_protocol_handler.inltests/unit_tests/node_server.cppInspect captured patch +12 / −14
diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index 868e410..789e961 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -726,13 +726,13 @@ crypto::hash Blockchain::get_tail_id() const
* powers of 2 less recent from there, so 13, 17, 25, etc...
*
*/
-bool Blockchain::get_short_chain_history(std::list<crypto::hash>& ids) const
+bool Blockchain::get_short_chain_history(std::list<crypto::hash>& ids, uint64_t& current_height) const
{
LOG_PRINT_L3("Blockchain::" << __func__);
CRITICAL_REGION_LOCAL(m_blockchain_lock);
uint64_t i = 0;
uint64_t current_multiplier = 1;
- uint64_t sz = m_db->height();
+ uint64_t sz = current_height = m_db->height();
if(!sz)
return true;
diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h
index 03a0d21..703c45e 100644
--- a/src/cryptonote_core/blockchain.h
+++ b/src/cryptonote_core/blockchain.h
@@ -438,10 +438,11 @@ namespace cryptonote
* powers of 2 less recent from there, so 13, 17, 25, etc...
*
* @param ids return-by-reference list to put the resulting hashes in
+ * @param current_height the current blockchain height, return-by-reference
*
* @return true
*/
- bool get_short_chain_history(std::list<crypto::hash>& ids) const;
+ bool get_short_chain_history(std::list<crypto::hash>& ids, uint64_t& current_height) const;
/**
* @brief get recent block hashes for a foreign chain
diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp
index e636014..a537d8b 100644
--- a/src/cryptonote_core/cryptonote_core.cpp
+++ b/src/cryptonote_core/cryptonote_core.cpp
@@ -1546,9 +1546,9 @@ namespace cryptonote
return m_mempool.get_pool_for_rpc(tx_infos, key_image_infos);
}
//-----------------------------------------------------------------------------------------------
- bool core::get_short_chain_history(std::list<crypto::hash>& ids) const
+ bool core::get_short_chain_history(std::list<crypto::hash>& ids, uint64_t& current_height) const
{
- return m_blockchain_storage.get_short_chain_history(ids);
+ return m_blockchain_storage.get_short_chain_history(ids, current_height);
}
//-----------------------------------------------------------------------------------------------
bool core::handle_get_objects(NOTIFY_REQUEST_GET_OBJECTS::request& arg, NOTIFY_RESPONSE_GET_OBJECTS::request& rsp, cryptonote_connection_context& context)
diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h
index 763b7e1..41b004a 100644
--- a/src/cryptonote_core/cryptonote_core.h
+++ b/src/cryptonote_core/cryptonote_core.h
@@ -579,7 +579,7 @@ namespace cryptonote
*
* @note see Blockchain::get_short_chain_history
*/
- bool get_short_chain_history(std::list<crypto::hash>& ids) const;
+ bool get_short_chain_history(std::list<crypto::hash>& ids, uint64_t& current_height) const;
/**
* @copydoc Blockchain::find_blockchain_supplement(const std::list<crypto::hash>&, NOTIFY_RESPONSE_CHAIN_ENTRY::request&) const
diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
index 8b88cb7..aba2b33 100644
--- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl
+++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
@@ -278,8 +278,7 @@ namespace cryptonote
{
NOTIFY_REQUEST_CHAIN::request r = {};
context.m_needed_objects.clear();
- context.m_expect_height = m_core.get_current_blockchain_height();
- m_core.get_short_chain_history(r.block_ids);
+ m_core.get_short_chain_history(r.block_ids, context.m_expect_height);
handler_request_blocks_history( r.block_ids ); // change the limit(?), sleep(?)
r.prune = m_sync_pruned_blocks;
context.m_last_request_time = boost::posix_time::microsec_clock::universal_time();
@@ -739,8 +738,7 @@ namespace cryptonote
context.m_needed_objects.clear();
context.m_state = cryptonote_connection_context::state_synchronizing;
NOTIFY_REQUEST_CHAIN::request r = {};
- context.m_expect_height = m_core.get_current_blockchain_height();
- m_core.get_short_chain_history(r.block_ids);
+ m_core.get_short_chain_history(r.block_ids, context.m_expect_height);
handler_request_blocks_history( r.block_ids ); // change the limit(?), sleep(?)
r.prune = m_sync_pruned_blocks;
context.m_last_request_time = boost::posix_time::microsec_clock::universal_time();
@@ -2342,8 +2340,7 @@ skip:
{//we have to fetch more objects ids, request blockchain entry
NOTIFY_REQUEST_CHAIN::request r = {};
- context.m_expect_height = m_core.get_current_blockchain_height();
- m_core.get_short_chain_history(r.block_ids);
+ m_core.get_short_chain_history(r.block_ids, context.m_expect_height);
CHECK_AND_ASSERT_MES(!r.block_ids.empty(), false, "Short chain history is empty");
// we'll want to start off from where we are on that peer, which may not be added yet
@@ -2479,7 +2476,7 @@ skip:
int t_cryptonote_protocol_handler<t_core>::handle_response_chain_entry(int command, NOTIFY_RESPONSE_CHAIN_ENTRY::request& arg, cryptonote_connection_context& context)
{
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);
+ << ", m_start_height=" << arg.start_height << ", m_total_height=" << arg.total_height << ", expect height=" << context.m_expect_height);
MLOG_PEER_STATE("received chain");
if (context.m_expect_response != NOTIFY_RESPONSE_CHAIN_ENTRY::ID)
diff --git a/tests/unit_tests/node_server.cpp b/tests/unit_tests/node_server.cpp
index a5bb09e..3f84f01 100644
--- a/tests/unit_tests/node_server.cpp
+++ b/tests/unit_tests/node_server.cpp
@@ -57,7 +57,7 @@ public:
void set_target_blockchain_height(uint64_t) {}
bool init(const boost::program_options::variables_map& vm) {return true ;}
bool deinit(){return true;}
- bool get_short_chain_history(std::list<crypto::hash>& ids) const { return true; }
+ bool get_short_chain_history(std::list<crypto::hash>& ids, uint64_t& current_height) const { return true; }
bool have_block(const crypto::hash& id, int *where = NULL) const {return false;}
bool have_block_unlocked(const crypto::hash& id, int *where = NULL) const {return false;}
void get_blockchain_top(uint64_t& height, crypto::hash& top_id)const{height=0;top_id=crypto::null_hash;}
Why this scored 29/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.