protocol: fix reorgs while syncing

src/cryptonote_basic/connection_context.h

@@ -39,7 +39,8 @@ namespace cryptonote
 
   struct cryptonote_connection_context: public epee::net_utils::connection_context_base
   {
-    cryptonote_connection_context(): m_state(state_befor_handshake), m_remote_blockchain_height(0), m_last_response_height(0) {}
+    cryptonote_connection_context(): m_state(state_befor_handshake), m_remote_blockchain_height(0), m_last_response_height(0),
+        m_last_known_hash(cryptonote::null_hash) {}
 
     enum state
     {
@@ -56,6 +57,7 @@ namespace cryptonote
     uint64_t m_last_response_height;
     boost::posix_time::ptime m_last_request_time;
     epee::copyable_atomic m_callback_request_count; //in debug purpose: problem with double callback rise
+    crypto::hash m_last_known_hash;
     //size_t m_score;  TODO: add score calculations
   };
 

src/cryptonote_protocol/block_queue.cpp

@@ -118,25 +118,6 @@ void block_queue::remove_spans(const boost::uuids::uuid &connection_id, uint64_t
   }
 }
 
-void block_queue::mark_last_block(uint64_t last_block_height)
-{
-  boost::unique_lock<boost::recursive_mutex> lock(mutex);
-  if (!blocks.empty() && is_blockchain_placeholder(*blocks.begin()))
-    blocks.erase(*blocks.begin());
-  for (block_map::iterator i = blocks.begin(); i != blocks.end(); )
-  {
-    block_map::iterator j = i++;
-    if (j->start_block_height + j->nblocks - 1 <= last_block_height)
-    {
-      blocks.erase(j);
-    }
-  }
-
-  // mark the current state of the db (for a fresh db, it's just the genesis block)
-  add_blocks(0, last_block_height + 1, boost::uuids::nil_uuid());
-  MDEBUG("last blocked marked at " << last_block_height);
-}
-
 uint64_t block_queue::get_max_block_height() const
 {
   boost::unique_lock<boost::recursive_mutex> lock(mutex);
@@ -171,7 +152,19 @@ std::string block_queue::get_overview() const
   return s;
 }
 
-std::pair<uint64_t, uint64_t> block_queue::reserve_span(uint64_t first_block_height, uint64_t last_block_height, uint64_t max_blocks, const boost::uuids::uuid &connection_id, boost::posix_time::ptime time)
+bool block_queue::requested(const crypto::hash &hash) const
+{
+  boost::unique_lock<boost::recursive_mutex> lock(mutex);
+  for (const auto &span: blocks)
+  {
+    for (const auto &h: span.hashes)
+      if (h == hash)
+        return true;
+  }
+  return false;
+}
+
+std::pair<uint64_t, uint64_t> block_queue::reserve_span(uint64_t first_block_height, uint64_t last_block_height, uint64_t max_blocks, const boost::uuids::uuid &connection_id, const std::list<crypto::hash> &block_hashes, boost::posix_time::ptime time)
 {
   boost::unique_lock<boost::recursive_mutex> lock(mutex);
 
@@ -181,71 +174,25 @@ std::pair<uint64_t, uint64_t> block_queue::reserve_span(uint64_t first_block_hei
     return std::make_pair(0, 0);
   }
 
-  uint64_t max_block_height = get_max_block_height();
+  uint64_t span_start_height = last_block_height - block_hashes.size() + 1;
-  if (last_block_height > max_block_height)
+  std::list<crypto::hash>::const_iterator i = block_hashes.begin();
-    max_block_height = last_block_height;
+  while (i != block_hashes.end() && requested(*i))
-  if (max_block_height == 0)
   {
-    MDEBUG("reserve_span: max_block_height is 0");
-    return std::make_pair(first_block_height, std::min(last_block_height - first_block_height + 1, max_blocks));
-  }
-
-  uint64_t base = 0, last_placeholder_block = 0;
-  bool has_placeholder = false;
-  block_map::const_iterator i = blocks.begin();
-  if (i != blocks.end() && is_blockchain_placeholder(*i))
-  {
-    base = i->start_block_height + i->nblocks;
-    last_placeholder_block = base - 1;
-    has_placeholder = true;
     ++i;
-    for (block_map::const_iterator j = i; j != blocks.end(); ++j)
+    ++span_start_height;
-    {
-      if (j->start_block_height < base)
-        base = j->start_block_height;
-    }
   }
-  if (base > first_block_height)
+  uint64_t span_length = 0;
-    base = first_block_height;
+  std::list<crypto::hash> hashes;
-
+  while (i != block_hashes.end() && span_length < max_blocks)
-  CHECK_AND_ASSERT_MES (base <= max_block_height + 1, std::make_pair(0, 0), "Blockchain placeholder larger than max block height");
-  std::vector<uint8_t> bitmap(max_block_height + 1 - base, 0);
-  MDEBUG("base " << base << ", last_placeholder_block " << (has_placeholder ? std::to_string(last_placeholder_block) : "none") << ", first_block_height " << first_block_height);
-  if (has_placeholder && last_placeholder_block >= base)
-    memset(bitmap.data(), 1, last_placeholder_block + 1 - base);
-  while (i != blocks.end())
   {
-    CHECK_AND_ASSERT_MES (i->start_block_height >= base, std::make_pair(0, 0), "Span starts before blochckain placeholder");
+    hashes.push_back(*i);
-    memset(bitmap.data() + i->start_block_height - base, 1, i->nblocks);
     ++i;
+    ++span_length;
   }
-
+  MDEBUG("Reserving span " << span_start_height << " - " << (span_start_height + span_length - 1) << " for " << connection_id);
-  const uint8_t *ptr = (const uint8_t*)memchr(bitmap.data() + first_block_height - base, 0, bitmap.size() - (first_block_height - base));
+  add_blocks(span_start_height, span_length, connection_id, time);
-  if (!ptr)
+  set_span_hashes(span_start_height, connection_id, hashes);
-  {
+  return std::make_pair(span_start_height, span_length);
-    MDEBUG("reserve_span: 0 not found in bitmap: " << first_block_height << " " << bitmap.size());
-    print();
-    return std::make_pair(0, 0);
-  }
-  uint64_t start_block_height = ptr - bitmap.data() + base;
-  if (start_block_height > last_block_height)
-  {
-    MDEBUG("reserve_span: start_block_height > last_block_height: " << start_block_height << " < " << last_block_height);
-    return std::make_pair(0, 0);
-  }
-  if (start_block_height + max_blocks - 1 < first_block_height)
-  {
-    MDEBUG("reserve_span: start_block_height + max_blocks - 1 < first_block_height: " << start_block_height << " + " << max_blocks << " - 1 < " << first_block_height);
-    return std::make_pair(0, 0);
-  }
-
-  uint64_t nblocks = 1;
-  while (start_block_height + nblocks <= last_block_height && nblocks < max_blocks && bitmap[start_block_height + nblocks - base] == 0)
-    ++nblocks;
-
-  MDEBUG("Reserving span " << start_block_height << " - " << (start_block_height + nblocks - 1) << " for " << connection_id);
-  add_blocks(start_block_height, nblocks, connection_id, time);
-  return std::make_pair(start_block_height, nblocks);
 }
 
 bool block_queue::is_blockchain_placeholder(const span &span) const
@@ -366,13 +313,15 @@ size_t block_queue::get_num_filled_spans() const
   return size;
 }
 
-crypto::hash block_queue::get_last_known_hash() const
+crypto::hash block_queue::get_last_known_hash(const boost::uuids::uuid &connection_id) const
 {
   boost::unique_lock<boost::recursive_mutex> lock(mutex);
   crypto::hash hash = cryptonote::null_hash;
   uint64_t highest_height = 0;
   for (const auto &span: blocks)
   {
+    if (span.connection_id != connection_id)
+      continue;
     uint64_t h = span.start_block_height + span.nblocks - 1;
     if (h > highest_height && span.hashes.size() == span.nblocks)
     {

src/cryptonote_protocol/block_queue.h

@@ -73,11 +73,10 @@ namespace cryptonote
     void flush_stale_spans(const std::set<boost::uuids::uuid> &live_connections);
     void remove_span(uint64_t start_block_height);
     void remove_spans(const boost::uuids::uuid &connection_id, uint64_t start_block_height);
-    void mark_last_block(uint64_t last_block_height);
     uint64_t get_max_block_height() const;
     void print() const;
     std::string get_overview() const;
-    std::pair<uint64_t, uint64_t> reserve_span(uint64_t first_block_height, uint64_t last_block_height, uint64_t max_blocks, const boost::uuids::uuid &connection_id, boost::posix_time::ptime time = boost::posix_time::microsec_clock::universal_time());
+    std::pair<uint64_t, uint64_t> reserve_span(uint64_t first_block_height, uint64_t last_block_height, uint64_t max_blocks, const boost::uuids::uuid &connection_id, const std::list<crypto::hash> &block_hashes, boost::posix_time::ptime time = boost::posix_time::microsec_clock::universal_time());
     bool is_blockchain_placeholder(const span &span) const;
     std::pair<uint64_t, uint64_t> get_start_gap_span() const;
     std::pair<uint64_t, uint64_t> get_next_span_if_scheduled(std::list<crypto::hash> &hashes, boost::uuids::uuid &connection_id, boost::posix_time::ptime &time) const;
@@ -86,9 +85,10 @@ namespace cryptonote
     size_t get_data_size() const;
     size_t get_num_filled_spans_prefix() const;
     size_t get_num_filled_spans() const;
-    crypto::hash get_last_known_hash() const;
+    crypto::hash get_last_known_hash(const boost::uuids::uuid &connection_id) const;
     float get_speed(const boost::uuids::uuid &connection_id) const;
     bool foreach(std::function<bool(const span&)> f, bool include_blockchain_placeholder = false) const;
+    bool requested(const crypto::hash &hash) const;
 
   private:
     block_map blocks;

src/cryptonote_protocol/cryptonote_protocol_handler.inl

@@ -947,7 +947,8 @@ namespace cryptonote
     }
 
     {
-      MLOG_YELLOW(el::Level::Debug, "Got NEW BLOCKS inside of " << __FUNCTION__ << ": size: " << arg.blocks.size());
+      MLOG_YELLOW(el::Level::Debug, context << " Got NEW BLOCKS inside of " << __FUNCTION__ << ": size: " << arg.blocks.size()
+          << ", blocks: " << start_height << " - " << (start_height + arg.blocks.size() - 1));
 
       // add that new span to the block queue
       const boost::posix_time::time_duration dt = now - context.m_last_request_time;
@@ -978,7 +979,6 @@ namespace cryptonote
           uint64_t start_height;
           std::list<cryptonote::block_complete_entry> blocks;
           boost::uuids::uuid span_connection_id;
-          m_block_queue.mark_last_block(previous_height - 1);
           if (!m_block_queue.get_next_span(start_height, blocks, span_connection_id))
           {
             MDEBUG(context << " no next span found, going back to download");
@@ -986,11 +986,6 @@ namespace cryptonote
           }
           MDEBUG(context << " next span in the queue has blocks " << start_height << "-" << (start_height + blocks.size() - 1)
               << ", we need " << previous_height);
-          if (previous_height < start_height || previous_height >= start_height + blocks.size())
-          {
-            MDEBUG(context << " this span is not what we need, going back to download");
-            break;
-          }
 
           const boost::posix_time::ptime start = boost::posix_time::microsec_clock::universal_time();
 
@@ -1083,7 +1078,7 @@ namespace cryptonote
 
           m_core.cleanup_handle_incoming_blocks();
 
-          m_block_queue.mark_last_block(m_core.get_current_blockchain_height() - 1);
+          m_block_queue.remove_spans(span_connection_id, start_height);
 
           if (m_core.get_current_blockchain_height() > previous_height)
           {
@@ -1201,8 +1196,6 @@ skip:
   template<class t_core>
   bool t_cryptonote_protocol_handler<t_core>::request_missing_objects(cryptonote_connection_context& context, bool check_having_blocks, bool force_next_span)
   {
-    m_block_queue.mark_last_block(m_core.get_current_blockchain_height() - 1);
-
     // flush stale spans
     std::set<boost::uuids::uuid> live_connections;
     m_p2p->for_each_connection([&](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t support_flags)->bool{
@@ -1308,8 +1301,13 @@ skip:
           context.m_last_response_height = 0;
           goto skip;
         }
+        // take out blocks we already have
+        while (!context.m_needed_objects.empty() && m_core.have_block(context.m_needed_objects.front()))
+        {
+          context.m_needed_objects.pop_front();
+        }
         const uint64_t first_block_height = context.m_last_response_height - context.m_needed_objects.size() + 1;
-        span = m_block_queue.reserve_span(first_block_height, context.m_last_response_height, count_limit, context.m_connection_id);
+        span = m_block_queue.reserve_span(first_block_height, context.m_last_response_height, count_limit, context.m_connection_id, context.m_needed_objects);
         MDEBUG(context << " span from " << first_block_height << ": " << span.first << "/" << span.second);
       }
       if (span.second == 0 && !force_next_span)
@@ -1360,8 +1358,6 @@ skip:
             auto j = it++;
             context.m_needed_objects.erase(j);
           }
-
-          m_block_queue.set_span_hashes(span.first, context.m_connection_id, hashes);
         }
 
         context.m_last_request_time = boost::posix_time::microsec_clock::universal_time();
@@ -1384,10 +1380,9 @@ skip:
 
       if (!start_from_current_chain)
       {
-        // we'll want to start off from where we are on the download side, which may not be added yet
+        // we'll want to start off from where we are on that peer, which may not be added yet
-        crypto::hash last_known_hash = m_block_queue.get_last_known_hash();
+        if (context.m_last_known_hash != cryptonote::null_hash && r.block_ids.front() != context.m_last_known_hash)
-        if (last_known_hash != cryptonote::null_hash && r.block_ids.front() != last_known_hash)
+          r.block_ids.push_front(context.m_last_known_hash);
-          r.block_ids.push_front(last_known_hash);
       }
 
       handler_request_blocks_history( r.block_ids ); // change the limit(?), sleep(?)
@@ -1486,6 +1481,7 @@ skip:
     {
       context.m_needed_objects.push_back(bl_id);
     }
+    context.m_last_known_hash = context.m_needed_objects.back();
 
     if (!request_missing_objects(context, false))
     {

tests/unit_tests/block_queue.cpp

@@ -85,191 +85,3 @@ TEST(block_queue, flush_uuid)
   bq.add_blocks(0, 200, uuid1());
   ASSERT_EQ(bq.get_max_block_height(), 399);
 }
-
-TEST(block_queue, reserve_overlaps_both)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(0, 100, uuid1());
-  bq.add_blocks(200, 100, uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-
-  span = bq.reserve_span(50, 250, 250, uuid2());
-  ASSERT_EQ(span.first, 100);
-  ASSERT_EQ(span.second, 100);
-}
-
-TEST(block_queue, reserve_overlaps_none)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(0, 100, uuid1());
-  bq.add_blocks(200, 100, uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-
-  span = bq.reserve_span(120, 180, 250, uuid2());
-  ASSERT_EQ(span.first, 120);
-  ASSERT_EQ(span.second, 61);
-}
-
-TEST(block_queue, reserve_overlaps_none_max_hit)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(0, 100, uuid1());
-  bq.add_blocks(200, 100, uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-
-  span = bq.reserve_span(120, 500, 50, uuid2());
-  ASSERT_EQ(span.first, 120);
-  ASSERT_EQ(span.second, 50);
-}
-
-TEST(block_queue, reserve_overlaps_start)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(0, 100, uuid1());
-  bq.add_blocks(200, 100, uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-
-  span = bq.reserve_span(50, 150, 250, uuid2());
-  ASSERT_EQ(span.first, 100);
-  ASSERT_EQ(span.second, 51);
-}
-
-TEST(block_queue, reserve_overlaps_start_max_hit)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(0, 100, uuid1());
-  bq.add_blocks(200, 100, uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-
-  span = bq.reserve_span(50, 300, 75, uuid2());
-  ASSERT_EQ(span.first, 100);
-  ASSERT_EQ(span.second, 75);
-}
-
-TEST(block_queue, reserve_overlaps_stop)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(0, 100, uuid1());
-  bq.add_blocks(200, 100, uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-
-  span = bq.reserve_span(150, 300, 250, uuid2());
-  ASSERT_EQ(span.first, 150);
-  ASSERT_EQ(span.second, 50);
-}
-
-TEST(block_queue, reserve_start_is_empty_after)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(100, 100, uuid1());
-  span = bq.reserve_span(150, 250, 100, uuid1());
-  ASSERT_EQ(span.first, 200);
-  ASSERT_EQ(span.second, 51);
-}
-
-TEST(block_queue, reserve_start_is_empty_start_fits)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(100, 100, uuid1());
-  span = bq.reserve_span(0, 250, 50, uuid1());
-  ASSERT_EQ(span.first, 0);
-  ASSERT_EQ(span.second, 50);
-}
-
-TEST(block_queue, reserve_start_is_empty_start_overflows)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(100, 100, uuid1());
-  span = bq.reserve_span(0, 250, 150, uuid1());
-  ASSERT_EQ(span.first, 0);
-  ASSERT_EQ(span.second, 100);
-}
-
-TEST(block_queue, flush_spans)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-
-  bq.add_blocks(100, 100, uuid2());
-  bq.add_blocks(200, 100, uuid1());
-  bq.add_blocks(300, 100, uuid2());
-  ASSERT_EQ(bq.get_max_block_height(), 399);
-  bq.flush_spans(uuid2());
-  ASSERT_EQ(bq.get_max_block_height(), 299);
-  span = bq.reserve_span(0, 500, 500, uuid1());
-  ASSERT_EQ(span.first, 0);
-  ASSERT_EQ(span.second, 200);
-  bq.flush_spans(uuid1());
-  ASSERT_EQ(bq.get_max_block_height(), 0);
-}
-
-TEST(block_queue, get_next_span)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-  uint64_t height;
-  std::list<cryptonote::block_complete_entry> blocks;
-  boost::uuids::uuid uuid;
-
-  bq.add_blocks(100, std::list<cryptonote::block_complete_entry>(100), uuid2(), 0, 0);
-  bq.add_blocks(200, std::list<cryptonote::block_complete_entry>(101), uuid1(), 0, 0);
-  bq.add_blocks(300, std::list<cryptonote::block_complete_entry>(102), uuid2(), 0, 0);
-
-  ASSERT_TRUE(bq.get_next_span(height, blocks, uuid));
-  ASSERT_EQ(height, 100);
-  ASSERT_EQ(blocks.size(), 100);
-  ASSERT_EQ(uuid, uuid2());
-  bq.remove_span(height);
-
-  ASSERT_TRUE(bq.get_next_span(height, blocks, uuid));
-  ASSERT_EQ(height, 200);
-  ASSERT_EQ(blocks.size(), 101);
-  ASSERT_EQ(uuid, uuid1());
-  bq.remove_span(height);
-
-  ASSERT_TRUE(bq.get_next_span(height, blocks, uuid));
-  ASSERT_EQ(height, 300);
-  ASSERT_EQ(blocks.size(), 102);
-  ASSERT_EQ(uuid, uuid2());
-  bq.remove_span(height);
-
-  ASSERT_FALSE(bq.get_next_span(height, blocks, uuid));
-}
-
-TEST(block_queue, get_next_span_if_scheduled)
-{
-  cryptonote::block_queue bq;
-  std::pair<uint64_t, uint64_t> span;
-  uint64_t height;
-  std::list<cryptonote::block_complete_entry> blocks;
-  boost::uuids::uuid uuid;
-  std::list<crypto::hash> hashes;
-  boost::posix_time::ptime time;
-
-  bq.reserve_span(0, 100, 100, uuid1());
-  span = bq.get_next_span_if_scheduled(hashes, uuid, time);
-  ASSERT_EQ(span.first, 0);
-  ASSERT_EQ(span.second, 100);
-  ASSERT_EQ(uuid, uuid1());
-  bq.add_blocks(0, std::list<cryptonote::block_complete_entry>(100), uuid1(), 0, 0);
-  span = bq.get_next_span_if_scheduled(hashes, uuid, time);
-  ASSERT_EQ(span.second, 0);
-}