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

@@ -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;

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

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)

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

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, context.m_expect_height);
-      m_core.get_short_chain_history(r.block_ids);
       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, context.m_expect_height);
-      m_core.get_short_chain_history(r.block_ids);
       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, context.m_expect_height);
-      m_core.get_short_chain_history(r.block_ids);
       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)

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;}