Some cleanup in span/connection_context + few more checks

src/cryptonote_basic/connection_context.cpp

@@ -29,6 +29,7 @@
 
 #include "connection_context.h"
 
+#include <boost/optional/optional.hpp>
 #include "cryptonote_protocol/cryptonote_protocol_defs.h"
 #include "p2p/p2p_protocol_defs.h"
 
@@ -69,4 +70,23 @@ namespace cryptonote
     };
     return std::numeric_limits<size_t>::max();
   }
+
+  void cryptonote_connection_context::set_state_normal()
+  {
+    m_state = state_normal;
+    m_expected_heights_start = 0;
+    m_needed_objects.clear();
+    m_needed_objects.shrink_to_fit();
+    m_expected_heights.clear();
+    m_expected_heights.shrink_to_fit();
+    m_requested_objects.clear();
+  }
+
+  boost::optional<crypto::hash> cryptonote_connection_context::get_expected_hash(const uint64_t height) const
+  {
+    const auto difference = height - m_expected_heights_start;
+    if (height < m_expected_heights_start || m_expected_heights.size() < difference)
+      return boost::none;
+    return m_expected_heights[difference];
+  }
 } // cryptonote

src/cryptonote_basic/connection_context.h

@@ -34,6 +34,7 @@
 #include <atomic>
 #include <algorithm>
 #include <boost/date_time/posix_time/posix_time.hpp>
+#include <boost/optional/optional_fwd.hpp>
 #include "net/net_utils_base.h"
 #include "crypto/hash.h"
 
@@ -42,7 +43,7 @@ namespace cryptonote
   struct cryptonote_connection_context: public epee::net_utils::connection_context_base
   {
     cryptonote_connection_context(): m_state(state_before_handshake), m_remote_blockchain_height(0), m_last_response_height(0),
-        m_last_request_time(boost::date_time::not_a_date_time), m_callback_request_count(0),
+        m_expected_heights_start(0), m_last_request_time(boost::date_time::not_a_date_time), m_callback_request_count(0),
         m_last_known_hash(crypto::null_hash), m_pruning_seed(0), m_rpc_port(0), m_rpc_credits_per_hash(0), m_anchor(false), m_score(0),
         m_expect_response(0), m_expect_height(0), m_num_requested(0) {}
 
@@ -92,11 +93,18 @@ namespace cryptonote
     //! \return Maximum number of bytes permissible for `command`.
     static size_t get_max_bytes(int command) noexcept;
 
+    //! Use this instead of `m_state = state_normal`.
+    void set_state_normal();
+
+    boost::optional<crypto::hash> get_expected_hash(uint64_t height) const;
+
     state m_state;
     std::vector<std::pair<crypto::hash, uint64_t>> m_needed_objects;
+    std::vector<crypto::hash> m_expected_heights;
     std::unordered_set<crypto::hash> m_requested_objects;
     uint64_t m_remote_blockchain_height;
     uint64_t m_last_response_height;
+    uint64_t m_expected_heights_start;
     boost::posix_time::ptime m_last_request_time;
     copyable_atomic m_callback_request_count; //in debug purpose: problem with double callback rise
     crypto::hash m_last_known_hash;

src/cryptonote_protocol/block_queue.cpp

@@ -51,10 +51,10 @@ void block_queue::add_blocks(uint64_t height, std::vector<cryptonote::block_comp
   blocks.insert(span(height, std::move(bcel), connection_id, addr, rate, size));
   if (has_hashes)
   {
-    for (const crypto::hash &h: hashes)
+    for (std::size_t i = 0; i < hashes.size(); ++i)
     {
-      requested_hashes.insert(h);
+      requested_hashes.insert(hashes[i]);
-      have_blocks.insert(h);
+      have_blocks.emplace(hashes[i], height + i);
     }
     set_span_hashes(height, connection_id, hashes);
   }
@@ -219,6 +219,16 @@ bool block_queue::have(const crypto::hash &hash) const
   return have_blocks.find(hash) != have_blocks.end();
 }
 
+std::uint64_t block_queue::have_height(const crypto::hash &hash) const
+{
+  boost::unique_lock<boost::recursive_mutex> lock(mutex);
+  const auto elem = have_blocks.find(hash);
+  if (elem == have_blocks.end())
+    return std::numeric_limits<std::uint64_t>::max();
+  return elem->second;
+}
+
+
 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 epee::net_utils::network_address &addr, bool sync_pruned_blocks, uint32_t local_pruning_seed, uint32_t pruning_seed, uint64_t blockchain_height, const std::vector<std::pair<crypto::hash, uint64_t>> &block_hashes, boost::posix_time::ptime time)
 {
   boost::unique_lock<boost::recursive_mutex> lock(mutex);

src/cryptonote_protocol/block_queue.h

@@ -98,6 +98,7 @@ namespace cryptonote
     bool foreach(std::function<bool(const span&)> f) const;
     bool requested(const crypto::hash &hash) const;
     bool have(const crypto::hash &hash) const;
+    std::uint64_t have_height(const crypto::hash &hash) const;
 
   private:
     void erase_block(block_map::iterator j);
@@ -107,6 +108,6 @@ namespace cryptonote
     block_map blocks;
     mutable boost::recursive_mutex mutex;
     std::unordered_set<crypto::hash> requested_hashes;
-    std::unordered_set<crypto::hash> have_blocks;
+    std::unordered_map<crypto::hash, std::uint64_t> have_blocks;
   };
 }

src/cryptonote_protocol/cryptonote_protocol_handler.h

@@ -158,6 +158,7 @@ namespace cryptonote
     bool should_ask_for_pruned_data(cryptonote_connection_context& context, uint64_t first_block_height, uint64_t nblocks, bool check_block_weights) const;
     void drop_connection(cryptonote_connection_context &context, bool add_fail, bool flush_all_spans);
     void drop_connection_with_score(cryptonote_connection_context &context, unsigned int score, bool flush_all_spans);
+    void drop_connection(const boost::uuids::uuid&);
     void drop_connections(const epee::net_utils::network_address address);
     bool kick_idle_peers();
     bool check_standby_peers();

src/cryptonote_protocol/cryptonote_protocol_handler.inl

@@ -35,6 +35,7 @@
 // (may contain code and/or modifications by other developers)
 // developer rfree: this code is caller of our new network code, and is modded; e.g. for rate limiting
 
+#include <boost/optional/optional.hpp>
 #include <list>
 #include <ctime>
 
@@ -385,7 +386,7 @@ namespace cryptonote
 
     if(m_core.have_block(hshd.top_id))
     {
-      context.m_state = cryptonote_connection_context::state_normal;
+      context.set_state_normal();
       if(is_inital  && hshd.current_height >= target && target == m_core.get_current_blockchain_height())
         on_connection_synchronized();
       return true;
@@ -394,7 +395,7 @@ namespace cryptonote
     // No chain synchronization over hidden networks (tor, i2p, etc.)
     if(context.m_remote_address.get_zone() != epee::net_utils::zone::public_)
     {
-      context.m_state = cryptonote_connection_context::state_normal;
+      context.set_state_normal();
       return true;
     }
 
@@ -435,7 +436,7 @@ namespace cryptonote
 
     if (m_no_sync)
     {
-      context.m_state = cryptonote_connection_context::state_normal;
+      context.set_state_normal();
       return true;
     }
 
@@ -1196,8 +1197,9 @@ namespace cryptonote
     block_hashes.reserve(arg.blocks.size());
     const boost::posix_time::ptime now = boost::posix_time::microsec_clock::universal_time();
     uint64_t start_height = std::numeric_limits<uint64_t>::max();
+    crypto::hash previous{};
     cryptonote::block b;
-    for(const block_complete_entry& block_entry: arg.blocks)
+    for(std::size_t i = 0; i < arg.blocks.size(); ++i)
     {
       if (m_stopping)
       {
@@ -1205,10 +1207,10 @@ namespace cryptonote
       }
 
       crypto::hash block_hash;
-      if(!parse_and_validate_block_from_blob(block_entry.block, b, block_hash))
+      if(!parse_and_validate_block_from_blob(arg.blocks[i].block, b, block_hash))
       {
         LOG_ERROR_CCONTEXT("sent wrong block: failed to parse and validate block: "
-          << epee::string_tools::buff_to_hex_nodelimer(block_entry.block) << ", dropping connection");
+          << 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;
@@ -1216,14 +1218,25 @@ namespace cryptonote
       if (b.miner_tx.vin.size() != 1 || b.miner_tx.vin.front().type() != typeid(txin_gen))
       {
         LOG_ERROR_CCONTEXT("sent wrong block: block: miner tx does not have exactly one txin_gen input"
-          << epee::string_tools::buff_to_hex_nodelimer(block_entry.block) << ", dropping connection");
+          << 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;
       }
+
+      const auto this_height = boost::get<txin_gen>(b.miner_tx.vin[0]).height;
+      if (context.get_expected_hash(this_height) != block_hash)
+      {
+        LOG_ERROR_CCONTEXT("Sent invalid chain");
+        drop_connection(context, false, false);
+        ++m_sync_bad_spans_downloaded;
+        return 1;
+      }
+
+      // if first block
       if (start_height == std::numeric_limits<uint64_t>::max())
       {
-        start_height = boost::get<txin_gen>(b.miner_tx.vin[0]).height;
+        start_height = this_height;
         if (start_height > context.m_expect_height)
         {
           LOG_ERROR_CCONTEXT("sent block ahead of expected height, dropping connection");
@@ -1231,21 +1244,45 @@ namespace cryptonote
           ++m_sync_bad_spans_downloaded;
           return 1;
         }
+
+        if (this_height == 0 || context.get_expected_hash(this_height - 1) != b.prev_id)
+        {
+          LOG_ERROR_CCONTEXT("Sent invalid chain");
+          drop_connection(context, false, false);
+          ++m_sync_bad_spans_downloaded;
+          return 1;
+        }
+      }
+      else if (b.prev_id != previous)
+      {
+        LOG_ERROR_CCONTEXT("Sent invalid chain");
+        drop_connection(context, false, false);
+        ++m_sync_bad_spans_downloaded;
+        return 1;
+      }
+      previous = block_hash;
+
+      if (start_height + i != this_height)
+      {
+        LOG_ERROR_CCONTEXT("Sent invalid chain");
+        drop_connection(context, false, false);
+        ++m_sync_bad_spans_downloaded;
+        return 1;
       }
 
       auto req_it = context.m_requested_objects.find(block_hash);
       if(req_it == context.m_requested_objects.end())
       {
-        LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(block_entry.block))
+        LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(arg.blocks[i].block))
           << " wasn't requested, dropping connection");
         drop_connection(context, false, false);
         ++m_sync_bad_spans_downloaded;
         return 1;
       }
-      if(b.tx_hashes.size() != block_entry.txs.size())
+      if(b.tx_hashes.size() != arg.blocks[i].txs.size())
       {
-        LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(block_entry.block))
+        LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(arg.blocks[i].block))
-          << ", tx_hashes.size()=" << b.tx_hashes.size() << " mismatch with block_complete_entry.m_txs.size()=" << block_entry.txs.size() << ", dropping connection");
+          << ", 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;
@@ -1463,6 +1500,14 @@ namespace cryptonote
           bool parent_known = m_core.have_block(new_block.prev_id);
           if (!parent_known)
           {
+            const std::uint64_t confirmed_height = m_block_queue.have_height(new_block.prev_id);
+            if (confirmed_height != std::numeric_limits<std::uint64_t>::max() && confirmed_height + 1 != start_height)
+            {
+              MERROR(context << "Found incorrect height for " << new_block.prev_id << " provided by " << span_connection_id);
+              drop_connection(span_connection_id);
+              return 1;
+            }
+
             // it could be:
             //  - later in the current chain
             //  - later in an alt chain
@@ -2091,7 +2136,6 @@ skip:
     m_block_queue.flush_stale_spans(live_connections);
 
     // if we don't need to get next span, and the block queue is full enough, wait a bit
-    bool start_from_current_chain = false;
     if (!force_next_span)
     {
       do
@@ -2114,7 +2158,7 @@ skip:
             return false;
           }
           MDEBUG(context << "Nothing to get from this peer, and it's not ahead of us, all done");
-          context.m_state = cryptonote_connection_context::state_normal;
+          context.set_state_normal();
           if (m_core.get_current_blockchain_height() >= m_core.get_target_blockchain_height())
             on_connection_synchronized();
           return true;
@@ -2263,7 +2307,7 @@ skip:
             return false;
           }
           MDEBUG(context << "Nothing to get from this peer, and it's not ahead of us, all done");
-          context.m_state = cryptonote_connection_context::state_normal;
+          context.set_state_normal();
           if (m_core.get_current_blockchain_height() >= m_core.get_target_blockchain_height())
             on_connection_synchronized();
           return true;
@@ -2376,7 +2420,7 @@ skip:
       const uint64_t blockchain_height = m_core.get_current_blockchain_height();
       if (std::max(blockchain_height, m_block_queue.get_next_needed_height(blockchain_height)) >= m_core.get_target_blockchain_height())
       {
-        context.m_state = cryptonote_connection_context::state_normal;
+        context.set_state_normal();
         MLOG_PEER_STATE("Nothing to do for now, switching to normal state");
         return true;
       }
@@ -2419,15 +2463,12 @@ skip:
       m_core.get_short_chain_history(r.block_ids);
       CHECK_AND_ASSERT_MES(!r.block_ids.empty(), false, "Short chain history is empty");
 
-      if (!start_from_current_chain)
-      {
       // we'll want to start off from where we are on that peer, which may not be added yet
       if (context.m_last_known_hash != crypto::null_hash && r.block_ids.front() != context.m_last_known_hash)
       {
         context.m_expect_height = std::numeric_limits<uint64_t>::max();
         r.block_ids.push_front(context.m_last_known_hash);
       }
-      }
 
       handler_request_blocks_history( r.block_ids ); // change the limit(?), sleep(?)
       r.prune = m_sync_pruned_blocks;
@@ -2439,7 +2480,7 @@ skip:
 
       context.m_last_request_time = boost::posix_time::microsec_clock::universal_time();
       context.m_expect_response = NOTIFY_RESPONSE_CHAIN_ENTRY::ID;
-      MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_CHAIN: m_block_ids.size()=" << r.block_ids.size() << ", start_from_current_chain " << start_from_current_chain);
+      MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_CHAIN: m_block_ids.size()=" << r.block_ids.size());
       post_notify<NOTIFY_REQUEST_CHAIN>(r, context);
       MLOG_PEER_STATE("requesting chain");
     }else
@@ -2453,7 +2494,7 @@ skip:
                            << "\r\nm_requested_objects.size()=" << context.m_requested_objects.size()
                            << "\r\non connection [" << epee::net_utils::print_connection_context_short(context)<< "]");
 
-      context.m_state = cryptonote_connection_context::state_normal;
+      context.set_state_normal();
       if (context.m_remote_blockchain_height >= m_core.get_target_blockchain_height())
       {
         if (m_core.get_current_blockchain_height() >= m_core.get_target_blockchain_height())
@@ -2626,11 +2667,14 @@ skip:
       return 1;
     }
 
+    context.m_expected_heights_start = arg.start_height;
+
+    context.m_expected_heights.clear();
+    context.m_expected_heights.reserve(arg.m_block_ids.size());
     context.m_needed_objects.clear();
     context.m_needed_objects.reserve(arg.m_block_ids.size());
     uint64_t added = 0;
     std::unordered_set<crypto::hash> blocks_found;
-    bool first = true;
     bool expect_unknown = false;
     for (size_t i = 0; i < arg.m_block_ids.size(); ++i)
     {
@@ -2642,9 +2686,10 @@ skip:
       }
       int where;
       const bool have_block = m_core.have_block_unlocked(arg.m_block_ids[i], &where);
-      if (first)
+      if (i == 0)
       {
-        if (!have_block && !m_block_queue.requested(arg.m_block_ids[i]) && !m_block_queue.have(arg.m_block_ids[i]))
+        // our outgoing chainlist only has proven blocks (i.e. downloaded)
+        if (!have_block && m_block_queue.have_height(arg.m_block_ids[i]) != arg.start_height)
         {
           LOG_ERROR_CCONTEXT("First block hash is unknown, dropping connection");
           drop_connection_with_score(context, 5, false);
@@ -2653,7 +2698,7 @@ skip:
         if (!have_block)
           expect_unknown = true;
       }
-      if (!first)
+      if (0 < i)
       {
         // after the first, blocks may be known or unknown, but if they are known,
         // they should be at the same height if on the main chain
@@ -2694,10 +2739,10 @@ skip:
           expect_unknown = true;
       }
       const uint64_t block_weight = arg.m_block_weights.empty() ? 0 : arg.m_block_weights[i];
+      context.m_expected_heights.push_back(arg.m_block_ids[i]);
       context.m_needed_objects.push_back(std::make_pair(arg.m_block_ids[i], block_weight));
       if (++added == n_use_blocks)
         break;
-      first = false;
     }
     context.m_last_response_height -= arg.m_block_ids.size() - n_use_blocks;
 
@@ -2906,6 +2951,16 @@ skip:
   }
   //------------------------------------------------------------------------------------------------------------------------
   template<class t_core>
+  void t_cryptonote_protocol_handler<t_core>::drop_connection(const boost::uuids::uuid& id)
+  {
+    m_p2p->for_connection(id, [this](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t f)->bool{
+      // This _could be_ outside of strand, so careful on actions
+      drop_connection(context, true, false);
+      return true;
+    });
+  }
+  //------------------------------------------------------------------------------------------------------------------------
+  template<class t_core>
   void t_cryptonote_protocol_handler<t_core>::drop_connections(const epee::net_utils::network_address address)
   {
     MWARNING("dropping connections to " << address.str());
@@ -2922,6 +2977,7 @@ skip:
     {
       m_block_queue.flush_spans(id, true);
       m_p2p->for_connection(id, [&](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t f)->bool{
+        // This _could be_ outside of strand, so careful on actions
         drop_connection(context, true, false);
         return true;
       });