Merge pull request #650

e298b14 Blockchain: Update comments on removing block (warptangent)
b368e29 blockchain_import: Get hard fork version from HardFork (warptangent)
e02577f Move HardFork DB update to BlockchainDB::add_block() (warptangent)
3800875 Make HardFork object available to BlockchainDB and derived DB implementations (warptangent)
fd46c96 BlockchainDB/LMDB: Refactor block-scope DB txn handling for add block (warptangent)
f3a6000 BlockchainDB/LMDB/BDB: Extract DB txn functions for block add/remove (warptangent)
f47d5b0 BlockchainLMDB: Allow two HardFork functions to update DB during block add (warptangent)
15ee0be BlockchainLMDB: extract txn macros used during block add/remove (warptangent)

8 files changed, 130 insertions, 34 deletions

diff --git a/src/blockchain_db/berkeleydb/db_bdb.cpp b/src/blockchain_db/berkeleydb/db_bdb.cpp
index 2c18ae5df..cdbca52f9 100644
--- a/src/blockchain_db/berkeleydb/db_bdb.cpp
+++ b/src/blockchain_db/berkeleydb/db_bdb.cpp
@@ -781,6 +781,8 @@ BlockchainBDB::BlockchainBDB(bool batch_transactions) :
     m_batch_transactions = batch_transactions;
     m_write_txn = nullptr;
     m_height = 0;
+
+    m_hardfork = nullptr;
 }
 
 void BlockchainBDB::open(const std::string& filename, const int db_flags)
@@ -1831,6 +1833,21 @@ void BlockchainBDB::set_batch_transactions(bool batch_transactions)
     LOG_PRINT_L3("batch transactions " << (m_batch_transactions ? "enabled" : "disabled"));
 }
 
+void BlockchainBDB::block_txn_start()
+{
+  // TODO
+}
+
+void BlockchainBDB::block_txn_stop()
+{
+  // TODO
+}
+
+void BlockchainBDB::block_txn_abort()
+{
+  // TODO
+}
+
 uint64_t BlockchainBDB::add_block(const block& blk, const size_t& block_size, const difficulty_type& cumulative_difficulty, const uint64_t& coins_generated, const std::vector<transaction>& txs)
 {
     LOG_PRINT_L3("BlockchainBDB::" << __func__);
diff --git a/src/blockchain_db/berkeleydb/db_bdb.h b/src/blockchain_db/berkeleydb/db_bdb.h
index 6db91aa1e..42119da93 100644
--- a/src/blockchain_db/berkeleydb/db_bdb.h
+++ b/src/blockchain_db/berkeleydb/db_bdb.h
@@ -328,6 +328,10 @@ public:
   virtual void batch_stop();
   virtual void batch_abort();
 
+  virtual void block_txn_start();
+  virtual void block_txn_stop();
+  virtual void block_txn_abort();
+
   virtual void pop_block(block& blk, std::vector<transaction>& txs);
 
 #if defined(BDB_BULK_CAN_THREAD)
diff --git a/src/blockchain_db/blockchain_db.cpp b/src/blockchain_db/blockchain_db.cpp
index 3737dfc4f..270b5399e 100644
--- a/src/blockchain_db/blockchain_db.cpp
+++ b/src/blockchain_db/blockchain_db.cpp
@@ -99,6 +99,8 @@ uint64_t BlockchainDB::add_block( const block& blk
                                 , const std::vector<transaction>& txs
                                 )
 {
+  block_txn_start();
+
   TIME_MEASURE_START(time1);
   crypto::hash blk_hash = get_block_hash(blk);
   TIME_MEASURE_FINISH(time1);
@@ -125,9 +127,21 @@ uint64_t BlockchainDB::add_block( const block& blk
   TIME_MEASURE_FINISH(time1);
   time_add_transaction += time1;
 
+  // DB's new height based on this added block is only incremented after this
+  // function returns, so height() here returns the new previous height.
+  uint64_t prev_height = height();
+  m_hardfork->add(blk, prev_height);
+
+  block_txn_stop();
+
   ++num_calls;
 
-  return height();
+  return prev_height;
+}
+
+void BlockchainDB::set_hard_fork(HardFork*& hf)
+{
+  m_hardfork = hf;
 }
 
 void BlockchainDB::pop_block(block& blk, std::vector<transaction>& txs)
diff --git a/src/blockchain_db/blockchain_db.h b/src/blockchain_db/blockchain_db.h
index 4a140e5f6..5926f34a5 100644
--- a/src/blockchain_db/blockchain_db.h
+++ b/src/blockchain_db/blockchain_db.h
@@ -28,12 +28,15 @@
 #ifndef BLOCKCHAIN_DB_H
 #define BLOCKCHAIN_DB_H
 
+#pragma once
+
 #include <list>
 #include <string>
 #include <exception>
 #include "crypto/hash.h"
 #include "cryptonote_core/cryptonote_basic.h"
 #include "cryptonote_core/difficulty.h"
+#include "cryptonote_core/hardfork.h"
 
 /* DB Driver Interface
  *
@@ -322,6 +325,8 @@ protected:
   uint64_t time_commit1 = 0;
   bool m_auto_remove_logs = true;
 
+  HardFork* m_hardfork;
+
 public:
 
   // virtual dtor
@@ -368,6 +373,12 @@ public:
   virtual void batch_stop() = 0;
   virtual void set_batch_transactions(bool) = 0;
 
+  virtual void block_txn_start() = 0;
+  virtual void block_txn_stop() = 0;
+  virtual void block_txn_abort() = 0;
+
+  virtual void set_hard_fork(HardFork*& hf);
+
   // adds a block with the given metadata to the top of the blockchain, returns the new height
   // NOTE: subclass implementations of this (or the functions it calls) need
   // to handle undoing any partially-added blocks in the event of a failure.
diff --git a/src/blockchain_db/lmdb/db_lmdb.cpp b/src/blockchain_db/lmdb/db_lmdb.cpp
index 5982b9d9a..28e6f5525 100644
--- a/src/blockchain_db/lmdb/db_lmdb.cpp
+++ b/src/blockchain_db/lmdb/db_lmdb.cpp
@@ -945,6 +945,8 @@ BlockchainLMDB::BlockchainLMDB(bool batch_transactions)
   m_write_batch_txn = nullptr;
   m_batch_active = false;
   m_height = 0;
+
+  m_hardfork = nullptr;
 }
 
 void BlockchainLMDB::open(const std::string& filename, const int mdb_flags)
@@ -1265,6 +1267,32 @@ void BlockchainLMDB::unlock()
       auto_txn.commit(); \
   } while(0)
 
+
+// The below two macros are for DB access within block add/remove, whether
+// regular batch txn is in use or not. m_write_txn is used as a batch txn, even
+// if it's only within block add/remove.
+//
+// DB access functions that may be called both within block add/remove and
+// without should use these. If the function will be called ONLY within block
+// add/remove, m_write_txn alone may be used instead of these macros.
+
+#define TXN_BLOCK_PREFIX(flags); \
+  mdb_txn_safe auto_txn; \
+  mdb_txn_safe* txn_ptr = &auto_txn; \
+  if (m_batch_active || m_write_txn) \
+    txn_ptr = m_write_txn; \
+  else \
+  { \
+    if (auto mdb_res = mdb_txn_begin(m_env, NULL, flags, auto_txn)) \
+      throw0(DB_ERROR(lmdb_error(std::string("Failed to create a transaction for the db in ")+__FUNCTION__+": ", mdb_res).c_str())); \
+  } \
+
+#define TXN_BLOCK_POSTFIX_SUCCESS() \
+  do { \
+    if (! m_batch_active && ! m_write_txn) \
+      auto_txn.commit(); \
+  } while(0)
+
 bool BlockchainLMDB::block_exists(const crypto::hash& h) const
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
@@ -2164,6 +2192,44 @@ void BlockchainLMDB::set_batch_transactions(bool batch_transactions)
   LOG_PRINT_L3("batch transactions " << (m_batch_transactions ? "enabled" : "disabled"));
 }
 
+void BlockchainLMDB::block_txn_start()
+{
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
+  if (! m_batch_active && m_write_txn)
+    throw0(DB_ERROR((std::string("Attempted to start new write txn when write txn already exists in ")+__FUNCTION__).c_str()));
+  if (! m_batch_active)
+  {
+    m_write_txn = new mdb_txn_safe();
+    if (auto mdb_res = mdb_txn_begin(m_env, NULL, 0, *m_write_txn))
+      throw0(DB_ERROR(lmdb_error("Failed to create a transaction for the db: ", mdb_res).c_str()));
+  }
+}
+
+void BlockchainLMDB::block_txn_stop()
+{
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
+  if (! m_batch_active)
+  {
+    TIME_MEASURE_START(time1);
+    m_write_txn->commit();
+    TIME_MEASURE_FINISH(time1);
+    time_commit1 += time1;
+
+    delete m_write_txn;
+    m_write_txn = NULL;
+  }
+}
+
+void BlockchainLMDB::block_txn_abort()
+{
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
+  if (! m_batch_active)
+  {
+    delete m_write_txn;
+    m_write_txn = NULL;
+  }
+}
+
 uint64_t BlockchainLMDB::add_block(const block& blk, const size_t& block_size, const difficulty_type& cumulative_difficulty, const uint64_t& coins_generated,
     const std::vector<transaction>& txs)
 {
@@ -2180,33 +2246,15 @@ uint64_t BlockchainLMDB::add_block(const block& blk, const size_t& block_size, c
     }
   }
 
-  mdb_txn_safe txn;
-  if (! m_batch_active)
-  {
-    if (auto mdb_res = mdb_txn_begin(m_env, NULL, 0, txn))
-      throw0(DB_ERROR(lmdb_error("Failed to create a transaction for the db: ", mdb_res).c_str()));
-    m_write_txn = &txn;
-  }
-
   uint64_t num_outputs = m_num_outputs;
   try
   {
     BlockchainDB::add_block(blk, block_size, cumulative_difficulty, coins_generated, txs);
-    if (! m_batch_active)
-    {
-      m_write_txn = NULL;
-
-      TIME_MEASURE_START(time1);
-      txn.commit();
-      TIME_MEASURE_FINISH(time1);
-      time_commit1 += time1;
-    }
   }
   catch (...)
   {
     m_num_outputs = num_outputs;
-    if (! m_batch_active)
-      m_write_txn = NULL;
+    block_txn_abort();
     throw;
   }
 
@@ -2481,14 +2529,14 @@ void BlockchainLMDB::set_hard_fork_starting_height(uint8_t version, uint64_t hei
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   check_open();
 
-  TXN_PREFIX(0);
+  TXN_BLOCK_PREFIX(0);
 
   MDB_val_copy<uint8_t> val_key(version);
   MDB_val_copy<uint64_t> val_value(height);
   if (auto result = mdb_put(*txn_ptr, m_hf_starting_heights, &val_key, &val_value, 0))
     throw1(DB_ERROR(std::string("Error adding hard fork starting height to db transaction: ").append(mdb_strerror(result)).c_str()));
 
-  TXN_POSTFIX_SUCCESS();
+  TXN_BLOCK_POSTFIX_SUCCESS();
 }
 
 uint64_t BlockchainLMDB::get_hard_fork_starting_height(uint8_t version) const
@@ -2516,14 +2564,14 @@ void BlockchainLMDB::set_hard_fork_version(uint64_t height, uint8_t version)
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   check_open();
 
-  TXN_PREFIX(0);
+  TXN_BLOCK_PREFIX(0);
 
   MDB_val_copy<uint64_t> val_key(height);
   MDB_val_copy<uint8_t> val_value(version);
   if (auto result = mdb_put(*txn_ptr, m_hf_versions, &val_key, &val_value, 0))
     throw1(DB_ERROR(std::string("Error adding hard fork version to db transaction: ").append(mdb_strerror(result)).c_str()));
 
-  TXN_POSTFIX_SUCCESS();
+  TXN_BLOCK_POSTFIX_SUCCESS();
 }
 
 uint8_t BlockchainLMDB::get_hard_fork_version(uint64_t height) const
diff --git a/src/blockchain_db/lmdb/db_lmdb.h b/src/blockchain_db/lmdb/db_lmdb.h
index eb0704ab2..e88bcd01b 100644
--- a/src/blockchain_db/lmdb/db_lmdb.h
+++ b/src/blockchain_db/lmdb/db_lmdb.h
@@ -194,6 +194,10 @@ public:
   virtual void batch_stop();
   virtual void batch_abort();
 
+  virtual void block_txn_start();
+  virtual void block_txn_stop();
+  virtual void block_txn_abort();
+
   virtual void pop_block(block& blk, std::vector<transaction>& txs);
 
   virtual bool can_thread_bulk_indices() const { return true; }
diff --git a/src/blockchain_utilities/blockchain_import.cpp b/src/blockchain_utilities/blockchain_import.cpp
index 7eb493b6d..d3045a229 100644
--- a/src/blockchain_utilities/blockchain_import.cpp
+++ b/src/blockchain_utilities/blockchain_import.cpp
@@ -400,11 +400,8 @@ int import_from_file(FakeCore& simple_core, const std::string& import_file_path,
             // size_t blob_size = 0;
             // get_transaction_hash(tx, hsh, blob_size);
 
-            // we'd need to get the starting heights from the daemon
-            // to be correct once voting kicks in
-            uint64_t v2height = opt_testnet ? 624634 : 1009827;
 
-            uint8_t version = h < v2height ? 1 : 2;
+            uint8_t version = simple_core.m_storage.get_current_hard_fork_version();
             tx_verification_context tvc = AUTO_VAL_INIT(tvc);
             bool r = true;
             r = simple_core.m_pool.add_tx(tx, tvc, true, true, version);
@@ -469,9 +466,6 @@ int import_from_file(FakeCore& simple_core, const std::string& import_file_path,
           try
           {
             simple_core.add_block(b, block_size, cumulative_difficulty, coins_generated, txs);
-            #if !defined(BLOCKCHAIN_DB) || (BLOCKCHAIN_DB == DB_LMDB)
-            simple_core.m_hardfork->add(b, h-1);
-            #endif
           }
           catch (const std::exception& e)
           {
diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index e43875bdc..f9247ae27 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -290,6 +290,8 @@ bool Blockchain::init(BlockchainDB* db, const bool testnet, const bool fakechain
   }
   m_hardfork->init();
 
+  m_db->set_hard_fork(m_hardfork);
+
   // if the blockchain is new, add the genesis block
   // this feels kinda kludgy to do it this way, but can be looked at later.
   // TODO: add function to create and store genesis block,
@@ -459,6 +461,11 @@ block Blockchain::pop_block_from_blockchain()
     {
       cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc);
 
+      // FIXME: HardFork
+      // Besides the below, popping a block should also remove the last entry
+      // in hf_versions.
+      //
+      // FIXME: HardFork
       // This is not quite correct, as we really want to add the txes
       // to the pool based on the version determined after all blocks
       // are popped.
@@ -2690,9 +2697,6 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
 
   TIME_MEASURE_FINISH(addblock);
 
-  // this will not fail since check succeeded above
-  m_hardfork->add(bl, new_height - 1);
-
   // do this after updating the hard fork state since the size limit may change due to fork
   update_next_cumulative_size_limit();