Merge pull request #1924

6d315459 core: avoid possible reordering bugs wth tx/bloch hash cache (moneromooo-monero)

src/cryptonote_basic/cryptonote_basic.h

@@ -35,6 +35,7 @@
 #include <vector>
 #include <cstring>  // memcmp
 #include <sstream>
+#include <atomic>
 #include "serialization/serialization.h"
 #include "serialization/variant.h"
 #include "serialization/vector.h"
@@ -186,6 +187,11 @@ namespace cryptonote
 
   class transaction: public transaction_prefix
   {
+  private:
+    // hash cash
+    mutable std::atomic<bool> hash_valid;
+    mutable std::atomic<bool> blob_size_valid;
+
   public:
     std::vector<std::vector<crypto::signature> > signatures; //count signatures  always the same as inputs count
     rct::rctSig rct_signatures;
@@ -193,19 +199,23 @@ namespace cryptonote
     // hash cash
     mutable crypto::hash hash;
     mutable size_t blob_size;
-    mutable bool hash_valid;
-    mutable bool blob_size_valid;
 
     transaction();
+    transaction(const transaction &t): transaction_prefix(t), hash_valid(false), blob_size_valid(false), signatures(t.signatures), rct_signatures(t.rct_signatures) { if (t.is_hash_valid()) { hash = t.hash; set_hash_valid(true); } if (t.is_blob_size_valid()) { blob_size = t.blob_size; set_blob_size_valid(true); } }
+    transaction &operator=(const transaction &t) { transaction_prefix::operator=(t); set_hash_valid(false); set_blob_size_valid(false); signatures = t.signatures; rct_signatures = t.rct_signatures; if (t.is_hash_valid()) { hash = t.hash; set_hash_valid(true); } if (t.is_blob_size_valid()) { blob_size = t.blob_size; set_blob_size_valid(true); } return *this; }
     virtual ~transaction();
     void set_null();
     void invalidate_hashes();
+    bool is_hash_valid() const { return hash_valid.load(std::memory_order_acquire); }
+    void set_hash_valid(bool v) const { hash_valid.store(v,std::memory_order_release); }
+    bool is_blob_size_valid() const { return hash_valid.load(std::memory_order_acquire); }
+    void set_blob_size_valid(bool v) const { blob_size_valid.store(v,std::memory_order_release); }
 
     BEGIN_SERIALIZE_OBJECT()
       if (!typename Archive<W>::is_saving())
       {
-        hash_valid = false;
+        set_hash_valid(false);
-        blob_size_valid = false;
+        set_blob_size_valid(false);
       }
 
       FIELDS(*static_cast<transaction_prefix *>(this))
@@ -312,15 +322,15 @@ namespace cryptonote
     extra.clear();
     signatures.clear();
     rct_signatures.type = rct::RCTTypeNull;
-    hash_valid = false;
+    set_hash_valid(false);
-    blob_size_valid = false;
+    set_blob_size_valid(false);
   }
 
   inline
   void transaction::invalidate_hashes()
   {
-    hash_valid = false;
+    set_hash_valid(false);
-    blob_size_valid = false;
+    set_blob_size_valid(false);
   }
 
   inline
@@ -361,19 +371,27 @@ namespace cryptonote
 
   struct block: public block_header
   {
+  private:
+    // hash cash
+    mutable std::atomic<bool> hash_valid;
+
+  public:
     block(): block_header(), hash_valid(false) {}
-    void invalidate_hashes() { hash_valid = false; }
+    block(const block &b): block_header(b), hash_valid(false), miner_tx(b.miner_tx), tx_hashes(b.tx_hashes) { if (b.is_hash_valid()) { hash = b.hash; set_hash_valid(true); } }
+    block &operator=(const block &b) { block_header::operator=(b); hash_valid = false; miner_tx = b.miner_tx; tx_hashes = b.tx_hashes; if (b.is_hash_valid()) { hash = b.hash; set_hash_valid(true); } return *this; }
+    void invalidate_hashes() { set_hash_valid(false); }
+    bool is_hash_valid() const { return hash_valid.load(std::memory_order_acquire); }
+    void set_hash_valid(bool v) const { hash_valid.store(v,std::memory_order_release); }
 
     transaction miner_tx;
     std::vector<crypto::hash> tx_hashes;
 
     // hash cash
     mutable crypto::hash hash;
-    mutable bool hash_valid;
 
     BEGIN_SERIALIZE_OBJECT()
       if (!typename Archive<W>::is_saving())
-        hash_valid = false;
+        set_hash_valid(false);
 
       FIELDS(*static_cast<block_header *>(this))
       FIELD(miner_tx)

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -100,8 +100,7 @@ namespace cryptonote
     binary_archive<false> ba(ss);
     bool r = ::serialization::serialize(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
-    tx.hash_valid = false;
+    tx.invalidate_hashes();
-    tx.blob_size_valid = false;
     return true;
   }
   //---------------------------------------------------------------
@@ -122,8 +121,7 @@ namespace cryptonote
     binary_archive<false> ba(ss);
     bool r = ::serialization::serialize(ba, tx);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse transaction from blob");
-    tx.hash_valid = false;
+    tx.invalidate_hashes();
-    tx.blob_size_valid = false;
     //TODO: validate tx
 
     get_transaction_hash(tx, tx_hash);
@@ -660,7 +658,7 @@ namespace cryptonote
   //---------------------------------------------------------------
   bool get_transaction_hash(const transaction& t, crypto::hash& res, size_t* blob_size)
   {
-    if (t.hash_valid)
+    if (t.is_hash_valid())
     {
 #ifdef ENABLE_HASH_CASH_INTEGRITY_CHECK
       CHECK_AND_ASSERT_THROW_MES(!calculate_transaction_hash(t, res, blob_size) || t.hash == res, "tx hash cash integrity failure");
@@ -668,10 +666,10 @@ namespace cryptonote
       res = t.hash;
       if (blob_size)
       {
-        if (!t.blob_size_valid)
+        if (!t.is_blob_size_valid())
         {
           t.blob_size = get_object_blobsize(t);
-          t.blob_size_valid = true;
+          t.set_blob_size_valid(true);
         }
         *blob_size = t.blob_size;
       }
@@ -683,11 +681,11 @@ namespace cryptonote
     if (!ret)
       return false;
     t.hash = res;
-    t.hash_valid = true;
+    t.set_hash_valid(true);
     if (blob_size)
     {
       t.blob_size = *blob_size;
-      t.blob_size_valid = true;
+      t.set_blob_size_valid(true);
     }
     return true;
   }
@@ -735,7 +733,7 @@ namespace cryptonote
   //---------------------------------------------------------------
   bool get_block_hash(const block& b, crypto::hash& res)
   {
-    if (b.hash_valid)
+    if (b.is_hash_valid())
     {
 #ifdef ENABLE_HASH_CASH_INTEGRITY_CHECK
       CHECK_AND_ASSERT_THROW_MES(!calculate_block_hash(b, res) || b.hash == res, "block hash cash integrity failure");
@@ -749,7 +747,7 @@ namespace cryptonote
     if (!ret)
       return false;
     b.hash = res;
-    b.hash_valid = true;
+    b.set_hash_valid(true);
     return true;
   }
   //---------------------------------------------------------------
@@ -769,7 +767,6 @@ namespace cryptonote
       string_tools::hex_to_pod(longhash_202612, res);
       return true;
     }
-    block b_local = b; //workaround to avoid const errors with do_serialize
     blobdata bd = get_block_hashing_blob(b);
     crypto::cn_slow_hash(bd.data(), bd.size(), res);
     return true;
@@ -809,9 +806,8 @@ namespace cryptonote
     binary_archive<false> ba(ss);
     bool r = ::serialization::serialize(ba, b);
     CHECK_AND_ASSERT_MES(r, false, "Failed to parse block from blob");
-    b.hash_valid = false;
+    b.invalidate_hashes();
-    b.miner_tx.hash_valid = false;
+    b.miner_tx.invalidate_hashes();
-    b.miner_tx.blob_size_valid = false;
     return true;
   }
   //---------------------------------------------------------------

src/cryptonote_basic/miner.cpp

@@ -355,11 +355,11 @@ namespace cryptonote
 
       if(check_hash(h, diffic))
       {
-        bl.hash_valid = false;
+        bl.invalidate_hashes();
         return true;
       }
     }
-    bl.hash_valid = false;
+    bl.invalidate_hashes();
     return false;
   }
   //-----------------------------------------------------------------------------------------------------

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -133,7 +133,7 @@ namespace cryptonote
     tx.unlock_time = height + CRYPTONOTE_MINED_MONEY_UNLOCK_WINDOW;
     tx.vin.push_back(in);
 
-    tx.hash_valid = tx.blob_size_valid = false;
+    tx.invalidate_hashes();
 
     //LOG_PRINT("MINER_TX generated ok, block_reward=" << print_money(block_reward) << "("  << print_money(block_reward - fee) << "+" << print_money(fee)
     //  << "), current_block_size=" << current_block_size << ", already_generated_coins=" << already_generated_coins << ", tx_id=" << get_transaction_hash(tx), LOG_LEVEL_2);
@@ -454,7 +454,7 @@ namespace cryptonote
       MCINFO("construct_tx", "transaction_created: " << get_transaction_hash(tx) << ENDL << obj_to_json_str(tx) << ENDL);
     }
 
-    tx.hash_valid = tx.blob_size_valid = false;
+    tx.invalidate_hashes();
 
     return true;
   }
@@ -492,7 +492,7 @@ namespace cryptonote
     bl.timestamp = 0;
     bl.nonce = nonce;
     miner::find_nonce_for_given_block(bl, 1, 0);
-    bl.hash_valid = false;
+    bl.invalidate_hashes();
     return true;
   }
   //---------------------------------------------------------------