Merge pull request #10196

e89922a ledger: throw on secret view key export rejection (tobtoht)

README.md

@@ -137,8 +137,8 @@ Dates are provided in the format YYYY-MM-DD. The "Minimum" is the software versi
 | 1978433                        | 2019-11-30 | v12               | v0.15.0.0              | v0.16.0.0                  | New PoW based on RandomX, only allow >= 2 outputs, change to the block median used to calculate penalty, v1 coinbases are forbidden, rct sigs in coinbase forbidden, 10 block lock time for incoming outputs
 | 2210000                        | 2020-10-17 | v13               | v0.17.0.0              | v0.17.3.2                  | New CLSAG transaction format
 | 2210720                        | 2020-10-18 | v14               | v0.17.1.1              | v0.17.3.2                  | forbid old MLSAG transaction format
-| 2688888                        | 2022-08-13 | v15               | v0.18.0.0              | v0.18.4.3                  | ringsize = 16, bulletproofs+, view tags, adjusted dynamic block weight algorithm
-| 2689608                        | 2022-08-14 | v16               | v0.18.0.0              | v0.18.4.3                  | forbid old v14 transaction format
+| 2688888                        | 2022-08-13 | v15               | v0.18.0.0              | v0.18.4.4                  | ringsize = 16, bulletproofs+, view tags, adjusted dynamic block weight algorithm
+| 2689608                        | 2022-08-14 | v16               | v0.18.0.0              | v0.18.4.4                  | forbid old v14 transaction format
 | XXXXXXX                        | XXX-XX-XX | XXX                | vX.XX.X.X              | vX.XX.X.X                  | XXX |
 
 X's indicate that these details have not been determined as of commit date.

contrib/epee/include/misc_log_ex.h

@@ -30,6 +30,7 @@
 
 #ifdef __cplusplus
 
+#include <sstream>
 #include <string>
 
 #include "easylogging++.h"
@@ -40,9 +41,15 @@
 #define MAX_LOG_FILE_SIZE 104850000 // 100 MB - 7600 bytes
 #define MAX_LOG_FILES 10
 
+#define LOG_TO_STRING(x) \
+    std::stringstream ss; \
+    ss << x; \
+    const std::string str = ss.str();
+
 #define MCLOG_TYPE(level, cat, color, type, x) do { \
     if (el::Loggers::allowed(level, cat)) { \
-      el::base::Writer(level, color, __FILE__, __LINE__, ELPP_FUNC, type).construct(cat) << x; \
+      LOG_TO_STRING(x); \
+      el::base::Writer(level, color, __FILE__, __LINE__, ELPP_FUNC, type).construct(cat) << str; \
     } \
   } while (0)
 
@@ -91,7 +98,8 @@
   do { \
     if (el::Loggers::allowed(level, cat)) { \
       init; \
-      el::base::Writer(level, color, __FILE__, __LINE__, ELPP_FUNC, type).construct(cat) << x; \
+      LOG_TO_STRING(x); \
+      el::base::Writer(level, color, __FILE__, __LINE__, ELPP_FUNC, type).construct(cat) << str; \
     } \
   } while(0)
 #define MIDEBUG(init, x) IFLOG(el::Level::Debug, MONERO_DEFAULT_LOG_CATEGORY, el::Color::Default, el::base::DispatchAction::NormalLog, init, x)

external/easylogging++/easylogging++.cc

@@ -3065,8 +3065,9 @@ void Writer::triggerDispatch(void) {
   }
   if (m_proceed && m_level == Level::Fatal
       && !ELPP->hasFlag(LoggingFlag::DisableApplicationAbortOnFatalLog)) {
+    const std::string str = "Aborting application. Reason: Fatal log at [" + std::string(m_file) + ":" + std::to_string(m_line) + "]";
     base::Writer(Level::Warning, Color::Default, m_file, m_line, m_func).construct(1, base::consts::kDefaultLoggerId)
-        << "Aborting application. Reason: Fatal log at [" << m_file << ":" << m_line << "]";
+        << str;
     std::stringstream reasonStream;
     reasonStream << "Fatal log at [" << m_file << ":" << m_line << "]"
                  << " If you wish to disable 'abort on fatal log' please use "

external/easylogging++/easylogging++.h

@@ -3275,9 +3275,7 @@ class Writer : base::NoCopy {
     processDispatch();
   }
 
-  template <typename T>
-  inline typename std::enable_if<std::is_integral<T>::value, Writer&>::type
-  operator<<(T log) {
+  Writer& operator<<(const std::string &log) {
 #if ELPP_LOGGING_ENABLED
   if (m_proceed) {
     m_messageBuilder << log;
@@ -3286,26 +3284,6 @@ class Writer : base::NoCopy {
   return *this;
   }
 
-  template <typename T>
-  inline typename std::enable_if<!std::is_integral<T>::value, Writer&>::type
-  operator<<(const T& log) {
-#if ELPP_LOGGING_ENABLED
-    if (m_proceed) {
-      m_messageBuilder << log;
-    }
-#endif  // ELPP_LOGGING_ENABLED
-    return *this;
-  }
-
-  inline Writer& operator<<(std::ostream& (*log)(std::ostream&)) {
-#if ELPP_LOGGING_ENABLED
-    if (m_proceed) {
-      m_messageBuilder << log;
-    }
-#endif  // ELPP_LOGGING_ENABLED
-    return *this;
-  }
-
   inline operator bool() {
     return true;
   }
@@ -3622,8 +3600,9 @@ class DefaultPerformanceTrackingCallback : public PerformanceTrackingCallback {
         ss << ELPP_LITERAL("]");
       }
     }
+    const std::string str = ss.str();
     el::base::Writer(m_data->performanceTracker()->level(), m_data->file(), m_data->line(), m_data->func()).construct(1,
-        m_data->loggerId().c_str()) << ss.str();
+        m_data->loggerId().c_str()) << str;
   }
  private:
   const PerformanceTrackingData* m_data;

src/checkpoints/checkpoints.cpp

@@ -268,6 +268,7 @@ namespace cryptonote
     ADD_CHECKPOINT2(3451000, "0cbc912e06e1adae11f6c9cb675d3159d225b4b04d4a6c61defe50ae1816dd60", "0x6aa5fc4226bab97");
     ADD_CHECKPOINT2(3482000, "629071b10ddad67bdc6156102aba8e008a754c91da252eede852fff9175a9f0a", "0x6f1063da7e70c0e");
     ADD_CHECKPOINT2(3516300, "fa08acbcda99fcc3cd94a749364a29fa6de9501a023cb6673d0c68fdf988b7c3", "0x738f0af4d65d459");
+    ADD_CHECKPOINT2(3541000, "74c457bed9ceef40f31f43bb8fab804077519d45c910dcad2acf4dd8556195c7", "0x76ff158c682d218");
     return true;
   }
 

src/cryptonote_core/blockchain.cpp

@@ -5497,7 +5497,7 @@ void Blockchain::cancel()
 }
 
 #if defined(PER_BLOCK_CHECKPOINT)
-static const char expected_block_hashes_hash[] = "4725ea463d1520b56ce7cd54dd478d44f976e65d73ad6fe01c427eff854ecf79";
+static const char expected_block_hashes_hash[] = "baf89ec87f4435fc782a8111b1c8817ba0f00c496e98af154a982818c495edea";
 void Blockchain::load_compiled_in_block_hashes(const GetCheckpointsCallback& get_checkpoints)
 {
   if (get_checkpoints == nullptr || !m_fast_sync)

src/cryptonote_protocol/block_queue.cpp

@@ -153,18 +153,26 @@ uint64_t block_queue::get_next_needed_height(uint64_t blockchain_height) const
   boost::unique_lock<boost::recursive_mutex> lock(mutex);
   if (blocks.empty())
     return blockchain_height;
-  uint64_t last_needed_height = blockchain_height;
-  bool first = true;
+
+  uint64_t covered_until = blockchain_height;
+
   for (const auto &span: blocks)
   {
-    if (span.start_block_height + span.nblocks - 1 < blockchain_height)
+    // Ignore spans entirely below current chain height
+    const uint64_t span_end = span.start_block_height + span.nblocks - 1;
+    if (span_end < blockchain_height)
       continue;
-    if (span.start_block_height != last_needed_height || (first && span.blocks.empty()))
-      return last_needed_height;
-    last_needed_height = span.start_block_height + span.nblocks;
-    first = false;
+
+    // If this span starts after what we already have/scheduled, we found the first gap
+    if (span.start_block_height > covered_until)
+      return covered_until;
+
+    // This span overlaps or is adjacent; extend coverage regardless of filled/scheduled
+    if (span.start_block_height <= covered_until)
+      covered_until = std::max(covered_until, span.start_block_height + span.nblocks);
   }
-  return last_needed_height;
+
+  return covered_until;
 }
 
 void block_queue::print() const

src/cryptonote_protocol/cryptonote_protocol_handler.inl

@@ -48,6 +48,7 @@
 #include "net/network_throttle-detail.hpp"
 #include "common/pruning.h"
 #include "common/util.h"
+#include "misc_log_ex.h"
 
 #undef MONERO_DEFAULT_LOG_CATEGORY
 #define MONERO_DEFAULT_LOG_CATEGORY "net.cn"
@@ -59,8 +60,10 @@
     const char *cat = "net.p2p.msg"; \
     if (ELPP->vRegistry()->allowed(level, cat)) { \
       init; \
-      if (test) \
-        el::base::Writer(level, el::Color::Default, __FILE__, __LINE__, ELPP_FUNC, el::base::DispatchAction::NormalLog).construct(cat) << x; \
+      if (test) { \
+        LOG_TO_STRING(x); \
+        el::base::Writer(level, el::Color::Default, __FILE__, __LINE__, ELPP_FUNC, el::base::DispatchAction::NormalLog).construct(cat) << str; \
+      } \
     } \
   } while(0)
 

src/device/device_ledger.cpp

@@ -410,7 +410,7 @@ namespace hw {
       this->length_send = set_command_header_noopt(ins, p1);
       if (ins == INS_GET_KEY && p1 == IO_SECRET_KEY) {
         // export view key user input
-        this->exchange_wait_on_input();
+        CHECK_AND_ASSERT_THROW_MES(this->exchange_wait_on_input() == 0, "Key export rejected on device.");
       } else {
         this->exchange();
       }
@@ -619,15 +619,14 @@ namespace hw {
         send_simple(INS_GET_KEY, 0x02);
 
         //View key is retrievied, if allowed, to speed up blockchain parsing
-        memmove(this->viewkey.data,  this->buffer_recv+0,  32);
-        if (is_fake_view_key(this->viewkey)) {
-          MDEBUG("Have Not view key");
-          this->has_view_key = false;
-        } else {
-          MDEBUG("Have view key");
-          this->has_view_key = true;
-        }
-      
+        crypto::secret_key view_secret_key;
+        memmove(view_secret_key.data, this->buffer_recv+0, 32);
+
+        CHECK_AND_ASSERT_THROW_MES(!is_fake_view_key(view_secret_key), "Key export rejected on device.");
+
+        this->viewkey = view_secret_key;
+        this->has_view_key = true;
+
         #ifdef DEBUG_HWDEVICE
         send_simple(INS_GET_KEY, 0x04);
         memmove(dbg_viewkey.data, this->buffer_recv+0, 32);

src/device/device_ledger.hpp

@@ -172,8 +172,8 @@ namespace hw {
         HMACmap hmac_map;
 
         // To speed up blockchain parsing the view key maybe handle here.
-        crypto::secret_key viewkey;
-        bool has_view_key;
+        crypto::secret_key viewkey = crypto::null_skey;
+        bool has_view_key = false;
 
         device *controle_device;
 

tests/unit_tests/logging.cpp

@@ -28,6 +28,10 @@
 //
 // Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers
 
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
 #include <boost/filesystem.hpp>
 #include "gtest/gtest.h"
 #include "file_io_utils.h"
@@ -251,3 +255,55 @@ TEST(logging, empty_configuration)
     EXPECT_NO_THROW(log1.configure(cfg));
     EXPECT_EQ(log1.typedConfigurations()->filename(el::Level::Info), "");
 }
+
+TEST(logging, deadlock)
+{
+  std::mutex inner_mutex;
+
+  // 1. Thread 1 starts logger
+  // 2. Thread 2 grabs inner mutex shared across threads
+  // 3. Thread 2 logs
+  // 4. Thread 1 grabs inner mutex shared across threads
+  // 5. Thread 1 finishes logging
+  std::condition_variable cv1, cv2;
+  std::mutex mutex1, mutex2;
+  std::unique_lock<std::mutex> lock_until_t1_starts_logger(mutex1);
+  std::unique_lock<std::mutex> lock_until_t2_finishes_logging(mutex2);
+  bool t1_started_logger = false;
+  bool t2_finished_logging = false;
+
+  const auto thread1_func = [&]
+  {
+    const auto thread1_inner_func = [&]() -> std::string
+    {
+      t1_started_logger = true;
+      lock_until_t1_starts_logger.unlock();
+      cv1.notify_one();
+      cv2.wait(lock_until_t2_finishes_logging, [&]{return t2_finished_logging;});
+
+      std::lock_guard<std::mutex> guard(inner_mutex);
+      return "world!";
+    };
+    MGINFO("Hello, " << thread1_inner_func() << " - Sincerely, thread 1");
+  };
+
+  const auto thread2_func = [&]
+  {
+    cv1.wait(lock_until_t1_starts_logger, [&]{return t1_started_logger;});
+
+    {
+      std::lock_guard<std::mutex> guard(inner_mutex);
+      MGINFO("Hello, world! - Sincerely, thread 2");
+    }
+
+    t2_finished_logging = true;
+    lock_until_t2_finishes_logging.unlock();
+    cv2.notify_one();
+  };
+
+  std::thread t1(thread1_func);
+  std::thread t2(thread2_func);
+
+  t1.join();
+  t2.join();
+}