From d8d3cf97300480b9d3942c667978dd4860b76feb Mon Sep 17 00:00:00 2001 From: j-berman Date: Thu, 11 Dec 2025 21:32:23 -0800 Subject: [PATCH] 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. --- src/cryptonote_core/blockchain.cpp | 4 ++-- src/cryptonote_core/blockchain.h | 3 ++- src/cryptonote_core/cryptonote_core.cpp | 4 ++-- src/cryptonote_core/cryptonote_core.h | 2 +- .../cryptonote_protocol_handler.inl | 11 ++++------- tests/unit_tests/node_server.cpp | 2 +- 6 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index a6c2e3faf..3a34089f5 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -728,13 +728,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& ids) const +bool Blockchain::get_short_chain_history(std::list& 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 f16bc7415..0fd81f4ba 100644 --- a/src/cryptonote_core/blockchain.h +++ b/src/cryptonote_core/blockchain.h @@ -445,10 +445,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& ids) const; + bool get_short_chain_history(std::list& 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 300fb2553..d30bc72a7 100644 --- a/src/cryptonote_core/cryptonote_core.cpp +++ b/src/cryptonote_core/cryptonote_core.cpp @@ -1536,9 +1536,9 @@ namespace cryptonote return m_mempool.get_pool_for_rpc(tx_infos, key_image_infos); } //----------------------------------------------------------------------------------------------- - bool core::get_short_chain_history(std::list& ids) const + bool core::get_short_chain_history(std::list& 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 362060b54..ab9a0ee12 100644 --- a/src/cryptonote_core/cryptonote_core.h +++ b/src/cryptonote_core/cryptonote_core.h @@ -580,7 +580,7 @@ namespace cryptonote * * @note see Blockchain::get_short_chain_history */ - bool get_short_chain_history(std::list& ids) const; + bool get_short_chain_history(std::list& ids, uint64_t& current_height) const; /** * @copydoc Blockchain::find_blockchain_supplement(const std::list&, 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 1686394c5..fb195b390 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -274,8 +274,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(); @@ -730,8 +729,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(); @@ -2340,8 +2338,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 @@ -2477,7 +2474,7 @@ skip: int t_cryptonote_protocol_handler::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 58a6fbc4a..e55b43911 100644 --- a/tests/unit_tests/node_server.cpp +++ b/tests/unit_tests/node_server.cpp @@ -56,7 +56,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& ids) const { return true; } + bool get_short_chain_history(std::list& 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;}