Merge pull request #5400

5e673c03 blockchain_db: fix db txn ending too early (moneromooo-monero)
2024-11-23 11:23:26 +01:00 · 2019-04-16 22:30:40 +02:00 · 2019-04-16 22:30:40 +02:00 · 46c477ab30
commit 46c477ab30
parent 1f809e7485 5e673c03fe
9 changed files with 144 additions and 95 deletions
--- a/src/blockchain_db/blockchain_db.cpp
+++ b/src/blockchain_db/blockchain_db.cpp
@ -211,8 +211,6 @@ uint64_t BlockchainDB::add_block( const std::pair<block, blobdata>& blck
  if (blk.tx_hashes.size() != txs.size())
    throw std::runtime_error("Inconsistent tx/hashes sizes");

-  block_txn_start(false);
-
  TIME_MEASURE_START(time1);
  crypto::hash blk_hash = get_block_hash(blk);
  TIME_MEASURE_FINISH(time1);
@ -252,8 +250,6 @@ uint64_t BlockchainDB::add_block( const std::pair<block, blobdata>& blck

  m_hardfork->add(blk, prev_height);

-  block_txn_stop();
-
  ++num_calls;

  return prev_height;
--- a/src/blockchain_db/blockchain_db.h
+++ b/src/blockchain_db/blockchain_db.h
@ -770,9 +770,12 @@ public:
   */
  virtual void set_batch_transactions(bool) = 0;

-  virtual void block_txn_start(bool readonly=false) = 0;
-  virtual void block_txn_stop() = 0;
-  virtual void block_txn_abort() = 0;
+  virtual void block_wtxn_start() = 0;
+  virtual void block_wtxn_stop() = 0;
+  virtual void block_wtxn_abort() = 0;
+  virtual bool block_rtxn_start() const = 0;
+  virtual void block_rtxn_stop() const = 0;
+  virtual void block_rtxn_abort() const = 0;

  virtual void set_hard_fork(HardFork* hf);

@ -1699,6 +1702,52 @@ public:

 };  // class BlockchainDB

+class db_txn_guard
+{
+public:
+  db_txn_guard(BlockchainDB *db, bool readonly): db(db), readonly(readonly), active(false)
+  {
+    if (readonly)
+    {
+      active = db->block_rtxn_start();
+    }
+    else
+    {
+      db->block_wtxn_start();
+      active = true;
+    }
+  }
+  virtual ~db_txn_guard()
+  {
+    if (active)
+      stop();
+  }
+  void stop()
+  {
+    if (readonly)
+      db->block_rtxn_stop();
+    else
+      db->block_wtxn_stop();
+    active = false;
+  }
+  void abort()
+  {
+    if (readonly)
+      db->block_rtxn_abort();
+    else
+      db->block_wtxn_abort();
+    active = false;
+  }
+
+private:
+  BlockchainDB *db;
+  bool readonly;
+  bool active;
+};
+
+class db_rtxn_guard: public db_txn_guard { public: db_rtxn_guard(BlockchainDB *db): db_txn_guard(db, true) {} };
+class db_wtxn_guard: public db_txn_guard { public: db_wtxn_guard(BlockchainDB *db): db_txn_guard(db, false) {} };
+
 BlockchainDB *new_db(const std::string& db_type);

 }  // namespace cryptonote
--- a/src/blockchain_db/lmdb/db_lmdb.cpp
+++ b/src/blockchain_db/lmdb/db_lmdb.cpp
@ -3611,16 +3611,15 @@ void BlockchainLMDB::block_rtxn_stop() const
  memset(&m_tinfo->m_ti_rflags, 0, sizeof(m_tinfo->m_ti_rflags));
 }

-void BlockchainLMDB::block_txn_start(bool readonly)
+bool BlockchainLMDB::block_rtxn_start() const
 {
-  if (readonly)
-  {
-    MDB_txn *mtxn;
-	mdb_txn_cursors *mcur;
-	block_rtxn_start(&mtxn, &mcur);
-    return;
-  }
+  MDB_txn *mtxn;
+  mdb_txn_cursors *mcur;
+  return block_rtxn_start(&mtxn, &mcur);
+}

+void BlockchainLMDB::block_wtxn_start()
+{
  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
  // Distinguish the exceptions here from exceptions that would be thrown while
  // using the txn and committing it.
@ -3652,10 +3651,13 @@ void BlockchainLMDB::block_txn_start(bool readonly)
    throw0(DB_ERROR_TXN_START((std::string("Attempted to start new write txn when batch txn already exists in ")+__FUNCTION__).c_str()));
 }

-void BlockchainLMDB::block_txn_stop()
+void BlockchainLMDB::block_wtxn_stop()
 {
  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
-  if (m_write_txn && m_writer == boost::this_thread::get_id())
+  if (!m_write_txn)
+    throw0(DB_ERROR_TXN_START((std::string("Attempted to stop write txn when no such txn exists in ")+__FUNCTION__).c_str()));
+  if (m_writer != boost::this_thread::get_id())
+    throw0(DB_ERROR_TXN_START((std::string("Attempted to stop write txn from the wrong thread in ")+__FUNCTION__).c_str()));
  {
    if (! m_batch_active)
 	{
@ -3669,38 +3671,29 @@ void BlockchainLMDB::block_txn_stop()
      memset(&m_wcursors, 0, sizeof(m_wcursors));
 	}
  }
-  else if (m_tinfo->m_ti_rtxn)
+}
+
+void BlockchainLMDB::block_wtxn_abort()
+{
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
+  if (!m_write_txn)
+    throw0(DB_ERROR_TXN_START((std::string("Attempted to abort write txn when no such txn exists in ")+__FUNCTION__).c_str()));
+  if (m_writer != boost::this_thread::get_id())
+    throw0(DB_ERROR_TXN_START((std::string("Attempted to abort write txn from the wrong thread in ")+__FUNCTION__).c_str()));
+
+  if (! m_batch_active)
  {
-    mdb_txn_reset(m_tinfo->m_ti_rtxn);
-    memset(&m_tinfo->m_ti_rflags, 0, sizeof(m_tinfo->m_ti_rflags));
+    delete m_write_txn;
+    m_write_txn = nullptr;
+    memset(&m_wcursors, 0, sizeof(m_wcursors));
  }
 }

-void BlockchainLMDB::block_txn_abort()
+void BlockchainLMDB::block_rtxn_abort() const
 {
  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
-  if (m_write_txn && m_writer == boost::this_thread::get_id())
-  {
-    if (! m_batch_active)
-    {
-      delete m_write_txn;
-      m_write_txn = nullptr;
-      memset(&m_wcursors, 0, sizeof(m_wcursors));
-    }
-  }
-  else if (m_tinfo->m_ti_rtxn)
-  {
-    mdb_txn_reset(m_tinfo->m_ti_rtxn);
-    memset(&m_tinfo->m_ti_rflags, 0, sizeof(m_tinfo->m_ti_rflags));
-  }
-  else
-  {
-    // This would probably mean an earlier exception was caught, but then we
-    // proceeded further than we should have.
-    throw0(DB_ERROR((std::string("BlockchainLMDB::") + __func__ +
-                     std::string(": block-level DB transaction abort called when write txn doesn't exist")
-                    ).c_str()));
-  }
+  mdb_txn_reset(m_tinfo->m_ti_rtxn);
+  memset(&m_tinfo->m_ti_rflags, 0, sizeof(m_tinfo->m_ti_rflags));
 }

 uint64_t BlockchainLMDB::add_block(const std::pair<block, blobdata>& blk, size_t block_weight, uint64_t long_term_block_weight, const difficulty_type& cumulative_difficulty, const uint64_t& coins_generated,
@ -3728,11 +3721,6 @@ uint64_t BlockchainLMDB::add_block(const std::pair<block, blobdata>& blk, size_t
  {
    throw;
  }
-  catch (...)
-  {
-    block_txn_abort();
-    throw;
-  }

  return ++m_height;
 }
@ -3742,16 +3730,16 @@ void BlockchainLMDB::pop_block(block& blk, std::vector<transaction>& txs)
  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
  check_open();

-  block_txn_start(false);
+  block_wtxn_start();

  try
  {
    BlockchainDB::pop_block(blk, txs);
-	block_txn_stop();
+    block_wtxn_stop();
  }
  catch (...)
  {
-	block_txn_abort();
+    block_wtxn_abort();
    throw;
  }
 }
--- a/src/blockchain_db/lmdb/db_lmdb.h
+++ b/src/blockchain_db/lmdb/db_lmdb.h
@ -310,11 +310,14 @@ public:
  virtual void batch_stop();
  virtual void batch_abort();

-  virtual void block_txn_start(bool readonly);
-  virtual void block_txn_stop();
-  virtual void block_txn_abort();
-  virtual bool block_rtxn_start(MDB_txn **mtxn, mdb_txn_cursors **mcur) const;
+  virtual void block_wtxn_start();
+  virtual void block_wtxn_stop();
+  virtual void block_wtxn_abort();
+  virtual bool block_rtxn_start() const;
  virtual void block_rtxn_stop() const;
+  virtual void block_rtxn_abort() const;
+
+  bool block_rtxn_start(MDB_txn **mtxn, mdb_txn_cursors **mcur) const;

  virtual void pop_block(block& blk, std::vector<transaction>& txs);

--- a/src/blockchain_db/testdb.h
+++ b/src/blockchain_db/testdb.h
@ -55,9 +55,13 @@ public:
  virtual bool batch_start(uint64_t batch_num_blocks=0, uint64_t batch_bytes=0) { return true; }
  virtual void batch_stop() {}
  virtual void set_batch_transactions(bool) {}
-  virtual void block_txn_start(bool readonly=false) {}
-  virtual void block_txn_stop() {}
-  virtual void block_txn_abort() {}
+  virtual void block_wtxn_start() {}
+  virtual void block_wtxn_stop() {}
+  virtual void block_wtxn_abort() {}
+  virtual bool block_rtxn_start() const { return true; }
+  virtual void block_rtxn_stop() const {}
+  virtual void block_rtxn_abort() const {}
+
  virtual void drop_hard_fork_info() {}
  virtual bool block_exists(const crypto::hash& h, uint64_t *height) const { return false; }
  virtual cryptonote::blobdata get_block_blob_from_height(const uint64_t& height) const { return cryptonote::t_serializable_object_to_blob(get_block_from_height(height)); }
--- a/src/cryptonote_basic/hardfork.cpp
+++ b/src/cryptonote_basic/hardfork.cpp
@ -266,11 +266,9 @@ bool HardFork::reorganize_from_chain_height(uint64_t height)
 bool HardFork::rescan_from_block_height(uint64_t height)
 {
  CRITICAL_REGION_LOCAL(lock);
-  db.block_txn_start(true);
-  if (height >= db.height()) {
-    db.block_txn_stop();
+  db_rtxn_guard rtxn_guard(&db);
+  if (height >= db.height())
    return false;
-  }

  versions.clear();

@ -293,8 +291,6 @@ bool HardFork::rescan_from_block_height(uint64_t height)
    current_fork_index = voted;
  }

-  db.block_txn_stop();
-
  return true;
 }

--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@ -428,6 +428,7 @@ bool Blockchain::init(BlockchainDB* db, const network_type nettype, bool offline
    block bl;
    block_verification_context bvc = boost::value_initialized<block_verification_context>();
    generate_genesis_block(bl, get_config(m_nettype).GENESIS_TX, get_config(m_nettype).GENESIS_NONCE);
+    db_wtxn_guard wtxn_guard(m_db);
    add_new_block(bl, bvc);
    CHECK_AND_ASSERT_MES(!bvc.m_verifivation_failed, false, "Failed to add genesis block to blockchain");
  }
@ -443,7 +444,8 @@ bool Blockchain::init(BlockchainDB* db, const network_type nettype, bool offline
    m_db->fixup();
  }

-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
+
  // check how far behind we are
  uint64_t top_block_timestamp = m_db->get_top_block_timestamp();
  uint64_t timestamp_diff = time(NULL) - top_block_timestamp;
@ -464,7 +466,8 @@ bool Blockchain::init(BlockchainDB* db, const network_type nettype, bool offline
 #endif

  MINFO("Blockchain initialized. last block: " << m_db->height() - 1 << ", " << epee::misc_utils::get_time_interval_string(timestamp_diff) << " time ago, current difficulty: " << get_difficulty_for_next_block());
-  m_db->block_txn_stop();
+
+  rtxn_guard.stop();

  uint64_t num_popped_blocks = 0;
  while (!m_db->is_read_only())
@ -518,8 +521,11 @@ bool Blockchain::init(BlockchainDB* db, const network_type nettype, bool offline
  if (test_options && test_options->long_term_block_weight_window)
    m_long_term_block_weights_window = test_options->long_term_block_weight_window;

-  if (!update_next_cumulative_weight_limit())
-    return false;
+  {
+    db_txn_guard txn_guard(m_db, m_db->is_read_only());
+    if (!update_next_cumulative_weight_limit())
+      return false;
+  }
  return true;
 }
 //------------------------------------------------------------------
@ -725,6 +731,7 @@ bool Blockchain::reset_and_set_genesis_block(const block& b)
  m_db->reset();
  m_hardfork->init();

+  db_wtxn_guard wtxn_guard(m_db);
  block_verification_context bvc = boost::value_initialized<block_verification_context>();
  add_new_block(b, bvc);
  if (!update_next_cumulative_weight_limit())
@ -772,7 +779,7 @@ bool Blockchain::get_short_chain_history(std::list<crypto::hash>& ids) const
  if(!sz)
    return true;

-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
  bool genesis_included = false;
  uint64_t current_back_offset = 1;
  while(current_back_offset < sz)
@ -799,7 +806,6 @@ bool Blockchain::get_short_chain_history(std::list<crypto::hash>& ids) const
  {
    ids.push_back(m_db->get_block_hash_from_height(0));
  }
-  m_db->block_txn_stop();

  return true;
 }
@ -1866,7 +1872,7 @@ bool Blockchain::handle_get_objects(NOTIFY_REQUEST_GET_OBJECTS::request& arg, NO
 {
  LOG_PRINT_L3("Blockchain::" << __func__);
  CRITICAL_REGION_LOCAL(m_blockchain_lock);
-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard (m_db);
  rsp.current_blockchain_height = get_current_blockchain_height();
  std::vector<std::pair<cryptonote::blobdata,block>> blocks;
  get_blocks(arg.blocks, blocks, rsp.missed_ids);
@ -1893,7 +1899,6 @@ bool Blockchain::handle_get_objects(NOTIFY_REQUEST_GET_OBJECTS::request& arg, NO
      // as done below if any standalone transactions were requested
      // and missed.
      rsp.missed_ids.insert(rsp.missed_ids.end(), missed_tx_ids.begin(), missed_tx_ids.end());
-      m_db->block_txn_stop();
      return false;
    }

@ -1903,7 +1908,6 @@ bool Blockchain::handle_get_objects(NOTIFY_REQUEST_GET_OBJECTS::request& arg, NO
  //get and pack other transactions, if needed
  get_transactions_blobs(arg.txs, rsp.txs, rsp.missed_ids);

-  m_db->block_txn_stop();
  return true;
 }
 //------------------------------------------------------------------
@ -2075,14 +2079,13 @@ bool Blockchain::find_blockchain_supplement(const std::list<crypto::hash>& qbloc
    return false;
  }

-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
  // make sure that the last block in the request's block list matches
  // the genesis block
  auto gen_hash = m_db->get_block_hash_from_height(0);
  if(qblock_ids.back() != gen_hash)
  {
    MCERROR("net.p2p", "Client sent wrong NOTIFY_REQUEST_CHAIN: genesis block mismatch: " << std::endl << "id: " << qblock_ids.back() << ", " << std::endl << "expected: " << gen_hash << "," << std::endl << " dropping connection");
-	m_db->block_txn_abort();
    return false;
  }

@ -2100,11 +2103,9 @@ bool Blockchain::find_blockchain_supplement(const std::list<crypto::hash>& qbloc
    catch (const std::exception& e)
    {
      MWARNING("Non-critical error trying to find block by hash in BlockchainDB, hash: " << *bl_it);
-	  m_db->block_txn_abort();
      return false;
    }
  }
-  m_db->block_txn_stop();

  // this should be impossible, as we checked that we share the genesis block,
  // but just in case...
@ -2295,7 +2296,7 @@ bool Blockchain::find_blockchain_supplement(const std::list<crypto::hash>& qbloc
    return false;
  }

-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
  current_height = get_current_blockchain_height();
  const uint32_t pruning_seed = get_blockchain_pruning_seed();
  start_height = tools::get_next_unpruned_block_height(start_height, current_height, pruning_seed);
@ -2307,7 +2308,6 @@ bool Blockchain::find_blockchain_supplement(const std::list<crypto::hash>& qbloc
    hashes.push_back(m_db->get_block_hash_from_height(i));
  }

-  m_db->block_txn_stop();
  return true;
 }

@ -2354,7 +2354,7 @@ bool Blockchain::find_blockchain_supplement(const uint64_t req_start_block, cons
    }
  }

-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
  total_height = get_current_blockchain_height();
  size_t count = 0, size = 0;
  blocks.reserve(std::min(std::min(max_count, (size_t)10000), (size_t)(total_height - start_height)));
@ -2380,7 +2380,6 @@ bool Blockchain::find_blockchain_supplement(const uint64_t req_start_block, cons
      blocks.back().second.push_back(std::make_pair(b.tx_hashes[i], std::move(txs[i])));
    }
  }
-  m_db->block_txn_stop();
  return true;
 }
 //------------------------------------------------------------------
@ -3535,7 +3534,7 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&

  static bool seen_future_version = false;

-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
  uint64_t blockchain_height;
  const crypto::hash top_hash = get_tail_id(blockchain_height);
  ++blockchain_height; // block height to chain height
@ -3544,7 +3543,6 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
    MERROR_VER("Block with id: " << id << std::endl << "has wrong prev_id: " << bl.prev_id << std::endl << "expected: " << top_hash);
    bvc.m_verifivation_failed = true;
 leave:
-    m_db->block_txn_stop();
    return false;
  }

@ -3827,7 +3825,7 @@ leave:
  if(precomputed)
    block_processing_time += m_fake_pow_calc_time;

-  m_db->block_txn_stop();
+  rtxn_guard.stop();
  TIME_MEASURE_START(addblock);
  uint64_t new_height = 0;
  if (!bvc.m_verifivation_failed)
@ -3945,8 +3943,6 @@ bool Blockchain::update_next_cumulative_weight_limit(uint64_t *long_term_effecti

  LOG_PRINT_L3("Blockchain::" << __func__);

-  m_db->block_txn_start(false);
-
  // when we reach this, the last hf version is not yet written to the db
  const uint64_t db_height = m_db->height();
  const uint8_t hf_version = get_current_hard_fork_version();
@ -4009,9 +4005,8 @@ bool Blockchain::update_next_cumulative_weight_limit(uint64_t *long_term_effecti
  if (long_term_effective_median_block_weight)
    *long_term_effective_median_block_weight = m_long_term_effective_median_block_weight;

-  m_db->add_max_block_size(m_current_block_cumul_weight_limit);
-
-  m_db->block_txn_stop();
+  if (!m_db->is_read_only())
+    m_db->add_max_block_size(m_current_block_cumul_weight_limit);

  return true;
 }
@ -4022,12 +4017,11 @@ bool Blockchain::add_new_block(const block& bl, block_verification_context& bvc)
  crypto::hash id = get_block_hash(bl);
  CRITICAL_REGION_LOCAL(m_tx_pool);//to avoid deadlock lets lock tx_pool for whole add/reorganize process
  CRITICAL_REGION_LOCAL1(m_blockchain_lock);
-  m_db->block_txn_start(true);
+  db_rtxn_guard rtxn_guard(m_db);
  if(have_block(id))
  {
    LOG_PRINT_L3("block with id = " << id << " already exists");
    bvc.m_already_exists = true;
-    m_db->block_txn_stop();
    m_blocks_txs_check.clear();
    return false;
  }
@ -4037,14 +4031,14 @@ bool Blockchain::add_new_block(const block& bl, block_verification_context& bvc)
  {
    //chain switching or wrong block
    bvc.m_added_to_main_chain = false;
-    m_db->block_txn_stop();
+    rtxn_guard.stop();
    bool r = handle_alternative_block(bl, id, bvc);
    m_blocks_txs_check.clear();
    return r;
    //never relay alternative blocks
  }

-  m_db->block_txn_stop();
+  rtxn_guard.stop();
  return handle_block_to_main_chain(bl, id, bvc);
 }
 //------------------------------------------------------------------
--- a/tests/core_tests/chaingen.h
+++ b/tests/core_tests/chaingen.h
@ -643,7 +643,15 @@ public:
    log_event("cryptonote::block");

    cryptonote::block_verification_context bvc = AUTO_VAL_INIT(bvc);
-    m_c.handle_incoming_block(t_serializable_object_to_blob(b), &b, bvc);
+    cryptonote::blobdata bd = t_serializable_object_to_blob(b);
+    std::vector<cryptonote::block> pblocks;
+    if (m_c.prepare_handle_incoming_blocks(std::vector<cryptonote::block_complete_entry>(1, {bd, {}}), pblocks))
+    {
+      m_c.handle_incoming_block(bd, &b, bvc);
+      m_c.cleanup_handle_incoming_blocks();
+    }
+    else
+      bvc.m_verifivation_failed = true;
    bool r = check_block_verification_context(bvc, m_ev_index, b, m_validator);
    CHECK_AND_NO_ASSERT_MES(r, false, "block verification context check failed");
    return r;
@ -666,7 +674,14 @@ public:
    log_event("serialized_block");

    cryptonote::block_verification_context bvc = AUTO_VAL_INIT(bvc);
-    m_c.handle_incoming_block(sr_block.data, NULL, bvc);
+    std::vector<cryptonote::block> pblocks;
+    if (m_c.prepare_handle_incoming_blocks(std::vector<cryptonote::block_complete_entry>(1, {sr_block.data, {}}), pblocks))
+    {
+      m_c.handle_incoming_block(sr_block.data, NULL, bvc);
+      m_c.cleanup_handle_incoming_blocks();
+    }
+    else
+      bvc.m_verifivation_failed = true;

    cryptonote::block blk;
    std::stringstream ss;
--- a/tests/unit_tests/blockchain_db.cpp
+++ b/tests/unit_tests/blockchain_db.cpp
@ -271,6 +271,8 @@ TYPED_TEST(BlockchainDBTest, AddBlock)
  this->get_filenames();
  this->init_hard_fork();

+  db_wtxn_guard guard(this->m_db);
+
  // adding a block with no parent in the blockchain should throw.
  // note: this shouldn't be possible, but is a good (and cheap) failsafe.
  //
@ -317,6 +319,8 @@ TYPED_TEST(BlockchainDBTest, RetrieveBlockData)
  this->get_filenames();
  this->init_hard_fork();

+  db_wtxn_guard guard(this->m_db);
+
  ASSERT_NO_THROW(this->m_db->add_block(this->m_blocks[0], t_sizes[0], t_sizes[0],  t_diffs[0], t_coins[0], this->m_txs[0]));

  ASSERT_EQ(t_sizes[0], this->m_db->get_block_weight(0));