Merge pull request #21 from warptangent/fix_block_reorg

Fixes and additions for block reorganization

5 files changed, 137 insertions, 35 deletions

diff --git a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp
index 835b9248f..4fa16b23e 100644
--- a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp
+++ b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.cpp
@@ -262,7 +262,7 @@ void BlockchainLMDB::add_transaction_data(const crypto::hash& blk_hash, const tr
     throw0(DB_ERROR("Failed to add tx unlock time to db transaction"));
 }
 
-void BlockchainLMDB::remove_transaction_data(const crypto::hash& tx_hash)
+void BlockchainLMDB::remove_transaction_data(const crypto::hash& tx_hash, const transaction& tx)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   check_open();
@@ -279,7 +279,7 @@ void BlockchainLMDB::remove_transaction_data(const crypto::hash& tx_hash)
   if (mdb_del(*m_write_txn, m_tx_heights, &val_h, NULL))
       throw1(DB_ERROR("Failed to add removal of tx block height to db transaction"));
 
-  remove_tx_outputs(tx_hash);
+  remove_tx_outputs(tx_hash, tx);
 
   if (mdb_del(*m_write_txn, m_tx_outputs, &val_h, NULL))
       throw1(DB_ERROR("Failed to add removal of tx outputs to db transaction"));
@@ -330,8 +330,9 @@ void BlockchainLMDB::add_output(const crypto::hash& tx_hash, const tx_out& tx_ou
   m_num_outputs++;
 }
 
-void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash)
+void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash, const transaction& tx)
 {
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
 
   lmdb_cur cur(*m_write_txn, m_tx_outputs);
 
@@ -356,7 +357,8 @@ void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash)
 
     for (uint64_t i = 0; i < num_elems; ++i)
     {
-      remove_output(*(const uint64_t*)v.mv_data);
+      const tx_out tx_output = tx.vout[i];
+      remove_output(*(const uint64_t*)v.mv_data, tx_output.amount);
       if (i < num_elems - 1)
       {
         mdb_cursor_get(cur, &k, &v, MDB_NEXT_DUP);
@@ -367,17 +369,19 @@ void BlockchainLMDB::remove_tx_outputs(const crypto::hash& tx_hash)
   cur.close();
 }
 
+// TODO: probably remove this function
 void BlockchainLMDB::remove_output(const tx_out& tx_output)
 {
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__ << " (unused version - does nothing)");
   return;
 }
 
-void BlockchainLMDB::remove_output(const uint64_t& out_index)
+void BlockchainLMDB::remove_output(const uint64_t& out_index, const uint64_t amount)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   check_open();
 
-  MDB_val k;
+  MDB_val_copy<uint64_t> k(out_index);
   MDB_val v;
 
 /****** Uncomment if ever outputs actually need to be stored in this manner
@@ -400,29 +404,94 @@ void BlockchainLMDB::remove_output(const uint64_t& out_index)
       throw1(DB_ERROR("Error adding removal of output to db transaction"));
 *********************************************************************/
 
-  auto result = mdb_del(*m_write_txn, m_output_indices, &v, NULL);
-  if (result != 0 && result != MDB_NOTFOUND)
-      throw1(DB_ERROR("Error adding removal of output tx index to db transaction"));
-
-  result = mdb_del(*m_write_txn, m_output_txs, &v, NULL);
-  if (result != 0 && result != MDB_NOTFOUND)
-      throw1(DB_ERROR("Error adding removal of output tx hash to db transaction"));
+  auto result = mdb_del(*m_write_txn, m_output_indices, &k, NULL);
+  if (result == MDB_NOTFOUND)
+  {
+    LOG_PRINT_L0("Unexpected: global output index not found in m_output_indices");
+  }
+  else if (result)
+  {
+    throw1(DB_ERROR("Error adding removal of output tx index to db transaction"));
+  }
 
-  result = mdb_del(*m_write_txn, m_output_amounts, &v, NULL);
-  if (result != 0 && result != MDB_NOTFOUND)
-      throw1(DB_ERROR("Error adding removal of output amount to db transaction"));
+  result = mdb_del(*m_write_txn, m_output_txs, &k, NULL);
+  // if (result != 0 && result != MDB_NOTFOUND)
+  //    throw1(DB_ERROR("Error adding removal of output tx hash to db transaction"));
+  if (result == MDB_NOTFOUND)
+  {
+    LOG_PRINT_L0("Unexpected: global output index not found in m_output_txs");
+  }
+  else if (result)
+  {
+    throw1(DB_ERROR("Error adding removal of output tx hash to db transaction"));
+  }
 
-  result = mdb_del(*m_write_txn, m_output_keys, &v, NULL);
+  result = mdb_del(*m_write_txn, m_output_keys, &k, NULL);
   if (result == MDB_NOTFOUND)
   {
-      LOG_PRINT_L2("Removing output, no public key found.");
+      LOG_PRINT_L0("Unexpected: global output index not found in m_output_keys");
   }
   else if (result)
     throw1(DB_ERROR("Error adding removal of output pubkey to db transaction"));
 
+  remove_amount_output_index(amount, out_index);
+
   m_num_outputs--;
 }
 
+void BlockchainLMDB::remove_amount_output_index(const uint64_t amount, const uint64_t global_output_index)
+{
+  LOG_PRINT_L3("BlockchainLMDB::" << __func__);
+  check_open();
+
+  lmdb_cur cur(*m_write_txn, m_output_amounts);
+
+  MDB_val_copy<uint64_t> k(amount);
+  MDB_val v;
+
+  auto result = mdb_cursor_get(cur, &k, &v, MDB_SET);
+  if (result == MDB_NOTFOUND)
+    throw1(OUTPUT_DNE("Attempting to get an output index by amount and amount index, but amount not found"));
+  else if (result)
+    throw0(DB_ERROR("DB error attempting to get an output"));
+
+  size_t num_elems = 0;
+  mdb_cursor_count(cur, &num_elems);
+
+  mdb_cursor_get(cur, &k, &v, MDB_FIRST_DUP);
+
+  uint64_t amount_output_index = 0;
+  uint64_t goi = 0;
+  bool found_index = false;
+  for (uint64_t i = 0; i < num_elems; ++i)
+  {
+    mdb_cursor_get(cur, &k, &v, MDB_GET_CURRENT);
+    goi = *(const uint64_t *)v.mv_data;
+    if (goi == global_output_index)
+    {
+      amount_output_index = i;
+      found_index = true;
+      break;
+    }
+    mdb_cursor_get(cur, &k, &v, MDB_NEXT_DUP);
+  }
+  if (found_index)
+  {
+    // found the amount output index
+    // now delete it
+    result = mdb_cursor_del(cur, 0);
+    if (result)
+      throw0(DB_ERROR(std::string("Error deleting amount output index ").append(boost::lexical_cast<std::string>(amount_output_index)).c_str()));
+  }
+  else
+  {
+    // not found
+    cur.close();
+    throw1(OUTPUT_DNE("Failed to find amount output index"));
+  }
+  cur.close();
+}
+
 void BlockchainLMDB::add_spent_key(const crypto::key_image& k_image)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
diff --git a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h
index d95d19019..2513aa48a 100644
--- a/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h
+++ b/src/cryptonote_core/BlockchainDB_impl/db_lmdb.h
@@ -190,15 +190,16 @@ private:
 
   virtual void add_transaction_data(const crypto::hash& blk_hash, const transaction& tx);
 
-  virtual void remove_transaction_data(const crypto::hash& tx_hash);
+  virtual void remove_transaction_data(const crypto::hash& tx_hash, const transaction& tx);
 
   virtual void add_output(const crypto::hash& tx_hash, const tx_out& tx_output, const uint64_t& local_index);
 
   virtual void remove_output(const tx_out& tx_output);
 
-  void remove_tx_outputs(const crypto::hash& tx_hash);
+  void remove_tx_outputs(const crypto::hash& tx_hash, const transaction& tx);
 
-  void remove_output(const uint64_t& out_index);
+  void remove_output(const uint64_t& out_index, const uint64_t amount);
+  void remove_amount_output_index(const uint64_t amount, const uint64_t global_output_index);
 
   virtual void add_spent_key(const crypto::key_image& k_image);
 
diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index 48e6543ed..4fa868abe 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -1113,7 +1113,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
     if(alt_chain.size())
     {
       // make sure alt chain doesn't somehow start past the end of the main chain
-      CHECK_AND_ASSERT_MES(m_db->height() - 1 > alt_chain.front()->second.height, false, "main blockchain wrong height");
+      CHECK_AND_ASSERT_MES(m_db->height() > alt_chain.front()->second.height, false, "main blockchain wrong height");
 
       // make sure that the blockchain contains the block that should connect
       // this alternate chain with it.
@@ -1188,8 +1188,16 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
     // FIXME:
     // this brings up an interesting point: consider allowing to get block
     // difficulty both by height OR by hash, not just height.
-    auto main_chain_cumulative_difficulty = m_db->get_block_cumulative_difficulty(m_db->get_block_height(b.prev_id));
-    bei.cumulative_difficulty = alt_chain.size() ? it_prev->second.cumulative_difficulty : main_chain_cumulative_difficulty;
+    difficulty_type main_chain_cumulative_difficulty = m_db->get_block_cumulative_difficulty(m_db->height() - 1);
+    if (alt_chain.size())
+    {
+      bei.cumulative_difficulty = it_prev->second.cumulative_difficulty;
+    }
+    else
+    {
+      // passed-in block's previous block's cumulative difficulty, found on the main chain
+      bei.cumulative_difficulty = m_db->get_block_cumulative_difficulty(m_db->get_block_height(b.prev_id));
+    }
     bei.cumulative_difficulty += current_diff;
 
     // add block to alternate blocks storage,
@@ -1216,8 +1224,8 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
     {
       //do reorganize!
       LOG_PRINT_GREEN("###### REORGANIZE on height: "
-          << alt_chain.front()->second.height << " of " << m_db->height()
-          << " with cum_difficulty " << m_db->get_block_cumulative_difficulty(m_db->height())
+          << alt_chain.front()->second.height << " of " << m_db->height() - 1
+          << " with cum_difficulty " << m_db->get_block_cumulative_difficulty(m_db->height() - 1)
           << std::endl << " alternative blockchain size: " << alt_chain.size()
           << " with cum_difficulty " << bei.cumulative_difficulty, LOG_LEVEL_0
       );
@@ -1699,13 +1707,22 @@ bool Blockchain::have_block(const crypto::hash& id) const
   CRITICAL_REGION_LOCAL(m_blockchain_lock);
 
   if(m_db->block_exists(id))
+  {
+    LOG_PRINT_L3("block exists in main chain");
     return true;
+  }
 
   if(m_alternative_chains.count(id))
+  {
+    LOG_PRINT_L3("block found in m_alternative_chains");
     return true;
+  }
 
   if(m_invalid_blocks.count(id))
+  {
+    LOG_PRINT_L3("block found in m_invalid_blocks");
     return true;
+  }
 
   return false;
 }
@@ -2010,14 +2027,25 @@ bool Blockchain::check_block_timestamp(const block& b) const
 bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash& id, block_verification_context& bvc)
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
+
+  // NOTE: Omitting check below with have_block() It causes an error after a
+  // blockchain reorganize begins with error: "Attempting to add block to main
+  // chain, but it's already either there or in an alternate"
+  //
+  // A block in the alternative chain, desired to become the main chain, never
+  // makes it due to have_block finding it in he alternative chain.
+  //
+  // Original implementation didn't use it here, and it doesn't appear
+  // necessary to be called from here in this implementation either.
+
   // if we already have the block, return false
-  if (have_block(id))
-  {
-    LOG_PRINT_L0("Attempting to add block to main chain, but it's already either there or in an alternate chain.  hash: " << id);
-    bvc.m_verifivation_failed = true;
-    return false;
-  }
-  
+  // if (have_block(id))
+  // {
+  //   LOG_PRINT_L0("Attempting to add block to main chain, but it's already either there or in an alternate chain.  hash: " << id);
+  //   bvc.m_verifivation_failed = true;
+  //   return false;
+  // }
+
   TIME_MEASURE_START(block_processing_time);
   CRITICAL_REGION_LOCAL(m_blockchain_lock);
   if(bl.prev_id != get_tail_id())
diff --git a/src/cryptonote_core/blockchain_db.cpp b/src/cryptonote_core/blockchain_db.cpp
index 439cf5ded..5e781af79 100644
--- a/src/cryptonote_core/blockchain_db.cpp
+++ b/src/cryptonote_core/blockchain_db.cpp
@@ -107,6 +107,9 @@ void BlockchainDB::remove_transaction(const crypto::hash& tx_hash)
 {
   transaction tx = get_tx(tx_hash);
 
+  // TODO: This loop calling remove_output() should be removed. It doesn't do
+  // anything (function is empty), and the outputs were already intended to be
+  // removed later as part of remove_transaction_data().
   for (const tx_out& tx_output : tx.vout)
   {
     remove_output(tx_output);
@@ -120,7 +123,8 @@ void BlockchainDB::remove_transaction(const crypto::hash& tx_hash)
     }
   }
 
-  remove_transaction_data(tx_hash);
+  // need tx as tx.vout has the tx outputs, and the output amounts are needed
+  remove_transaction_data(tx_hash, tx);
 }
 
 }  // namespace cryptonote
diff --git a/src/cryptonote_core/blockchain_db.h b/src/cryptonote_core/blockchain_db.h
index b498320ae..db56c7c07 100644
--- a/src/cryptonote_core/blockchain_db.h
+++ b/src/cryptonote_core/blockchain_db.h
@@ -274,7 +274,7 @@ private:
   virtual void add_transaction_data(const crypto::hash& blk_hash, const transaction& tx) = 0;
 
   // tells the subclass to remove data about a transaction
-  virtual void remove_transaction_data(const crypto::hash& tx_hash) = 0;
+  virtual void remove_transaction_data(const crypto::hash& tx_hash, const transaction& tx) = 0;
 
   // tells the subclass to store an output
   virtual void add_output(const crypto::hash& tx_hash, const tx_out& tx_output, const uint64_t& local_index) = 0;