Merge pull request #7247

57f7c2e53 protocol: more sanity checks in new chain block hashes (moneromooo-monero)

8 files changed, 90 insertions, 21 deletions

diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index 594aa1e9c..cd5b4f455 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -2758,32 +2758,44 @@ void Blockchain::flush_invalid_blocks()
   m_invalid_blocks.clear();
 }
 //------------------------------------------------------------------
-bool Blockchain::have_block(const crypto::hash& id) const
+bool Blockchain::have_block_unlocked(const crypto::hash& id, int *where) const
 {
+  // WARNING: this function does not take m_blockchain_lock, and thus should only call read only
+  // m_db functions which do not depend on one another (ie, no getheight + gethash(height-1), as
+  // well as not accessing class members, even read only (ie, m_invalid_blocks). The caller must
+  // lock if it is otherwise needed.
   LOG_PRINT_L3("Blockchain::" << __func__);
-  CRITICAL_REGION_LOCAL(m_blockchain_lock);
 
   if(m_db->block_exists(id))
   {
     LOG_PRINT_L2("block " << id << " found in main chain");
+    if (where) *where = HAVE_BLOCK_MAIN_CHAIN;
     return true;
   }
 
   if(m_db->get_alt_block(id, NULL, NULL))
   {
     LOG_PRINT_L2("block " << id << " found in alternative chains");
+    if (where) *where = HAVE_BLOCK_ALT_CHAIN;
     return true;
   }
 
   if(m_invalid_blocks.count(id))
   {
     LOG_PRINT_L2("block " << id << " found in m_invalid_blocks");
+    if (where) *where = HAVE_BLOCK_INVALID;
     return true;
   }
 
   return false;
 }
 //------------------------------------------------------------------
+bool Blockchain::have_block(const crypto::hash& id, int *where) const
+{
+  CRITICAL_REGION_LOCAL(m_blockchain_lock);
+  return have_block_unlocked(id, where);
+}
+//------------------------------------------------------------------
 bool Blockchain::handle_block_to_main_chain(const block& bl, block_verification_context& bvc, bool notify/* = true*/)
 {
     LOG_PRINT_L3("Blockchain::" << __func__);
diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h
index 66c3bc395..3529255e1 100644
--- a/src/cryptonote_core/blockchain.h
+++ b/src/cryptonote_core/blockchain.h
@@ -376,10 +376,12 @@ namespace cryptonote
      * for a block with the given hash
      *
      * @param id the hash to search for
+     * @param where the type of block, if non NULL
      *
      * @return true if the block is known, else false
      */
-    bool have_block(const crypto::hash& id) const;
+    bool have_block_unlocked(const crypto::hash& id, int *where = NULL) const;
+    bool have_block(const crypto::hash& id, int *where = NULL) const;
 
     /**
      * @brief gets the total number of transactions on the main chain
diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp
index 507c47a51..12125fb9d 100644
--- a/src/cryptonote_core/cryptonote_core.cpp
+++ b/src/cryptonote_core/cryptonote_core.cpp
@@ -1625,9 +1625,14 @@ namespace cryptonote
     return m_mempool.get_transactions_count(include_sensitive_txes);
   }
   //-----------------------------------------------------------------------------------------------
-  bool core::have_block(const crypto::hash& id) const
+  bool core::have_block_unlocked(const crypto::hash& id, int *where) const
   {
-    return m_blockchain_storage.have_block(id);
+    return m_blockchain_storage.have_block_unlocked(id, where);
+  }
+  //-----------------------------------------------------------------------------------------------
+  bool core::have_block(const crypto::hash& id, int *where) const
+  {
+    return m_blockchain_storage.have_block(id, where);
   }
   //-----------------------------------------------------------------------------------------------
   bool core::parse_tx_from_blob(transaction& tx, crypto::hash& tx_hash, const blobdata& blob) const
diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h
index fe55d76bf..a6cce8617 100644
--- a/src/cryptonote_core/cryptonote_core.h
+++ b/src/cryptonote_core/cryptonote_core.h
@@ -55,6 +55,8 @@
 PUSH_WARNINGS
 DISABLE_VS_WARNINGS(4355)
 
+enum { HAVE_BLOCK_MAIN_CHAIN, HAVE_BLOCK_ALT_CHAIN, HAVE_BLOCK_INVALID };
+
 namespace cryptonote
 {
    struct test_options {
@@ -543,7 +545,8 @@ namespace cryptonote
       *
       * @note see Blockchain::have_block
       */
-     bool have_block(const crypto::hash& id) const;
+     bool have_block_unlocked(const crypto::hash& id, int *where = NULL) const;
+     bool have_block(const crypto::hash& id, int *where = NULL) const;
 
      /**
       * @copydoc Blockchain::get_short_chain_history
diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
index e0f856f1b..5dbd02da8 100644
--- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl
+++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl
@@ -2570,17 +2570,6 @@ skip:
       return 1;
     }
 
-    std::unordered_set<crypto::hash> hashes;
-    for (const auto &h: arg.m_block_ids)
-    {
-      if (!hashes.insert(h).second)
-      {
-        LOG_ERROR_CCONTEXT("sent duplicate block, dropping connection");
-        drop_connection(context, true, false);
-        return 1;
-      }
-    }
-
     uint64_t n_use_blocks = m_core.prevalidate_block_hashes(arg.start_height, arg.m_block_ids, arg.m_block_weights);
     if (n_use_blocks == 0 || n_use_blocks + HASH_OF_HASHES_STEP <= arg.m_block_ids.size())
     {
@@ -2592,18 +2581,69 @@ skip:
     context.m_needed_objects.clear();
     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)
     {
       if (!blocks_found.insert(arg.m_block_ids[i]).second)
       {
         LOG_ERROR_CCONTEXT("Duplicate blocks in chain entry response, dropping connection");
-        drop_connection(context, true, false);
+        drop_connection_with_score(context, 5, false);
+        return 1;
+      }
+      int where;
+      const bool have_block = m_core.have_block_unlocked(arg.m_block_ids[i], &where);
+      if (first && !have_block)
+      {
+        LOG_ERROR_CCONTEXT("First block hash is unknown, dropping connection");
+        drop_connection_with_score(context, 5, false);
         return 1;
       }
+      if (!first)
+      {
+        // 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
+        if (have_block)
+        {
+          switch (where)
+          {
+            default:
+            case HAVE_BLOCK_INVALID:
+              LOG_ERROR_CCONTEXT("Block is invalid or known without known type, dropping connection");
+              drop_connection(context, true, false);
+              return 1;
+            case HAVE_BLOCK_MAIN_CHAIN:
+              if (expect_unknown)
+              {
+                LOG_ERROR_CCONTEXT("Block is on the main chain, but we did not expect a known block, dropping connection");
+                drop_connection_with_score(context, 5, false);
+                return 1;
+              }
+              if (m_core.get_block_id_by_height(arg.start_height + i) != arg.m_block_ids[i])
+              {
+                LOG_ERROR_CCONTEXT("Block is on the main chain, but not at the expected height, dropping connection");
+                drop_connection_with_score(context, 5, false);
+                return 1;
+              }
+              break;
+            case HAVE_BLOCK_ALT_CHAIN:
+              if (expect_unknown)
+              {
+                LOG_ERROR_CCONTEXT("Block is on the main chain, but we did not expect a known block, dropping connection");
+                drop_connection_with_score(context, 5, false);
+                return 1;
+              }
+              break;
+          }
+        }
+        else
+          expect_unknown = true;
+      }
       const uint64_t block_weight = arg.m_block_weights.empty() ? 0 : arg.m_block_weights[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;
 
diff --git a/tests/core_proxy/core_proxy.cpp b/tests/core_proxy/core_proxy.cpp
index 8095f03e3..09be8758b 100644
--- a/tests/core_proxy/core_proxy.cpp
+++ b/tests/core_proxy/core_proxy.cpp
@@ -245,12 +245,17 @@ bool tests::proxy_core::init(const boost::program_options::variables_map& /*vm*/
     return true;
 }
 
-bool tests::proxy_core::have_block(const crypto::hash& id) {
+bool tests::proxy_core::have_block_unlocked(const crypto::hash& id, int *where) {
     if (m_hash2blkidx.end() == m_hash2blkidx.find(id))
         return false;
+    if (where) *where = HAVE_BLOCK_MAIN_CHAIN;
     return true;
 }
 
+bool tests::proxy_core::have_block(const crypto::hash& id, int *where) {
+    return have_block_unlocked(id, where);
+}
+
 void tests::proxy_core::build_short_history(std::list<crypto::hash> &m_history, const crypto::hash &m_start) {
     m_history.push_front(get_block_hash(m_genesis));
     /*std::unordered_map<crypto::hash, tests::block_index>::const_iterator cit = m_hash2blkidx.find(m_lastblk);
diff --git a/tests/core_proxy/core_proxy.h b/tests/core_proxy/core_proxy.h
index ebc3a89c2..94f148e8c 100644
--- a/tests/core_proxy/core_proxy.h
+++ b/tests/core_proxy/core_proxy.h
@@ -74,7 +74,8 @@ namespace tests
     bool init(const boost::program_options::variables_map& vm);
     bool deinit(){return true;}
     bool get_short_chain_history(std::list<crypto::hash>& ids);
-    bool have_block(const crypto::hash& id);
+    bool have_block(const crypto::hash& id, int *where = NULL);
+    bool have_block_unlocked(const crypto::hash& id, int *where = NULL);
     void get_blockchain_top(uint64_t& height, crypto::hash& top_id);
     bool handle_incoming_tx(const cryptonote::tx_blob_entry& tx_blob, cryptonote::tx_verification_context& tvc, cryptonote::relay_method tx_relay, bool relayed);
     bool handle_incoming_txs(const std::vector<cryptonote::tx_blob_entry>& tx_blobs, std::vector<cryptonote::tx_verification_context>& tvc, cryptonote::relay_method tx_relay, bool relayed);
diff --git a/tests/unit_tests/node_server.cpp b/tests/unit_tests/node_server.cpp
index 0569d3748..4d6f09e69 100644
--- a/tests/unit_tests/node_server.cpp
+++ b/tests/unit_tests/node_server.cpp
@@ -55,7 +55,8 @@ public:
   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 have_block(const crypto::hash& id) 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;}
   bool handle_incoming_tx(const cryptonote::tx_blob_entry& tx_blob, cryptonote::tx_verification_context& tvc, cryptonote::relay_method tx_relay, bool relayed) { return true; }
   bool handle_incoming_txs(const std::vector<cryptonote::tx_blob_entry>& tx_blob, std::vector<cryptonote::tx_verification_context>& tvc, cryptonote::relay_method tx_relay, bool relayed) { return true; }