blockchain: fix a few block addition bugs

If the block reward was too high, the verification failed flag
was set, but the function continued. The code which was supposed
to trap this flag and return failure failed to trap it, and,
while the block was not added to the chain, the function would
return success.
The reason for avoiding returning when the block reward problem
was detected was to be able to return any transactions to the
pool if needed. This is now mooted by moving the transaction
return code to a separate function, which is now called at all
appropriate points, making the logic much simpler, and hopefully
correct now.
We also move the hard fork version check after the prev_id check,
as block which does not go on the top of the chain might not
have the expected version there, without being invalid just for
this reason.
Last, we trap the case where a block fails to be added due to
using already spent key images, to set the verification failed
flag.

src/cryptonote_core/blockchain.cpp

@@ -2363,6 +2363,23 @@ bool Blockchain::check_block_timestamp(const block& b) const
   return check_block_timestamp(timestamps, b);
 }
 //------------------------------------------------------------------
+void Blockchain::return_tx_to_pool(const std::vector<transaction> &txs)
+{
+  for (auto& tx : txs)
+  {
+    cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc);
+    // We assume that if they were in a block, the transactions are already
+    // known to the network as a whole. However, if we had mined that block,
+    // that might not be always true. Unlikely though, and always relaying
+    // these again might cause a spike of traffic as many nodes re-relay
+    // all the transactions in a popped block when a reorg happens.
+    if (!m_tx_pool.add_tx(tx, tvc, true, true))
+    {
+      LOG_PRINT_L0("Failed to return taken transaction with hash: " << get_transaction_hash(tx) << " to tx_pool");
+    }
+  }
+}
+//------------------------------------------------------------------
 //      Needs to validate the block and acquire each transaction from the
 //      transaction mem_pool, then pass the block and transactions to
 //      m_db->add_block()
@@ -2374,16 +2391,17 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
   CRITICAL_REGION_LOCAL(m_blockchain_lock);
   TIME_MEASURE_START(t1);
 
+  if(bl.prev_id != get_tail_id())
+  {
+    LOG_PRINT_L1("Block with id: " << id << std::endl << "has wrong prev_id: " << bl.prev_id << std::endl << "expected: " << get_tail_id());
+    return false;
+  }
+
   // this is a cheap test
   if (!m_hardfork->check(bl))
   {
     LOG_PRINT_L1("Block with id: " << id << std::endl << "has old version: " << (unsigned)bl.major_version << std::endl << "current: " << (unsigned)m_hardfork->get_current_version());
-    return false;
+    bvc.m_verifivation_failed = true;
-  }
-
-  if(bl.prev_id != get_tail_id())
-  {
-    LOG_PRINT_L1("Block with id: " << id << std::endl << "has wrong prev_id: " << bl.prev_id << std::endl << "expected: " << get_tail_id());
     return false;
   }
 
@@ -2498,7 +2516,6 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
   uint64_t t_exists = 0;
   uint64_t t_pool = 0;
   uint64_t t_dblspnd = 0;
-  bool add_tx_to_pool = false;
   TIME_MEASURE_FINISH(t3);
 
 // XXX old code adds miner tx here
@@ -2520,7 +2537,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
     {
       LOG_PRINT_L1("Block with id: " << id << " attempting to add transaction already in blockchain with id: " << tx_id);
       bvc.m_verifivation_failed = true;
-      break;
+      return_tx_to_pool(txs);
+      return false;
     }
 
     TIME_MEASURE_FINISH(aa);
@@ -2532,7 +2550,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
     {
       LOG_PRINT_L1("Block with id: " << id  << " has at least one unknown transaction with id: " << tx_id);
       bvc.m_verifivation_failed = true;
-      break;
+      return_tx_to_pool(txs);
+      return false;
     }
 
     TIME_MEASURE_FINISH(bb);
@@ -2568,8 +2587,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
         add_block_as_invalid(bl, id);
         LOG_PRINT_L1("Block with id " << id << " added as invalid because of wrong inputs in transactions");
         bvc.m_verifivation_failed = true;
-        add_tx_to_pool = true;
+        return_tx_to_pool(txs);
-        break;
+        return false;
       }
     }
 #if defined(PER_BLOCK_CHECKPOINT)
@@ -2584,8 +2603,8 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
         add_block_as_invalid(bl, id);
         LOG_PRINT_L1("Block with id " << id << " added as invalid because of wrong inputs in transactions");
         bvc.m_verifivation_failed = true;
-        add_tx_to_pool = true;
+        return_tx_to_pool(txs);
-        break;
+        return false;
       }
     }
 #endif
@@ -2600,10 +2619,12 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
   TIME_MEASURE_START(vmt);
   uint64_t base_reward = 0;
   uint64_t already_generated_coins = m_db->height() ? m_db->get_block_already_generated_coins(m_db->height() - 1) : 0;
-  if(!validate_miner_transaction(bl, cumulative_block_size, fee_summary, base_reward, already_generated_coins))
+  if(!validate_miner_transaction(bl, cumulative_block_size, fee_summary, base_reward, already_generated_coins, bvc.m_partial_block_reward))
   {
     LOG_PRINT_L1("Block with id: " << id << " has incorrect miner transaction");
     bvc.m_verifivation_failed = true;
+    return_tx_to_pool(txs);
+    return false;
   }
 
   TIME_MEASURE_FINISH(vmt);
@@ -2623,41 +2644,31 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
 
   TIME_MEASURE_START(addblock);
   uint64_t new_height = 0;
-  bool add_success = true;
   if (!bvc.m_verifivation_failed)
   {
     try
     {
       new_height = m_db->add_block(bl, block_size, cumulative_difficulty, already_generated_coins, txs);
     }
+    catch (const KEY_IMAGE_EXISTS& e)
+    {
+      LOG_ERROR("Error adding block with hash: " << id << " to blockchain, what = " << e.what());
+      bvc.m_verifivation_failed = true;
+      return_tx_to_pool(txs);
+      return false;
+    }
     catch (const std::exception& e)
     {
       //TODO: figure out the best way to deal with this failure
       LOG_ERROR("Error adding block with hash: " << id << " to blockchain, what = " << e.what());
-      add_success = false;
+      return_tx_to_pool(txs);
-    }
-  }
-
-  // if we failed for any reason to verify the block, return taken
-  // transactions to the tx_pool.
-  if ((bvc.m_verifivation_failed && add_tx_to_pool) || !add_success)
-  {
-    // return taken transactions to transaction pool
-    for (auto& tx : txs)
-    {
-      cryptonote::tx_verification_context tvc = AUTO_VAL_INIT(tvc);
-      // We assume that if they were in a block, the transactions are already
-      // known to the network as a whole. However, if we had mined that block,
-      // that might not be always true. Unlikely though, and always relaying
-      // these again might cause a spike of traffic as many nodes re-relay
-      // all the transactions in a popped block when a reorg happens.
-      if (!m_tx_pool.add_tx(tx, tvc, true, true))
-      {
-        LOG_PRINT_L0("Failed to return taken transaction with hash: " << get_transaction_hash(tx) << " to tx_pool");
-      }
-    }
       return false;
     }
+  }
+  else
+  {
+    LOG_ERROR("Blocks that failed verification should not reach here");
+  }
 
   TIME_MEASURE_FINISH(addblock);
 

src/cryptonote_core/blockchain.h

@@ -269,6 +269,7 @@ namespace cryptonote
     uint64_t get_adjusted_time() const;
     bool complete_timestamps_vector(uint64_t start_height, std::vector<uint64_t>& timestamps);
     bool update_next_cumulative_size_limit();
+    void return_tx_to_pool(const std::vector<transaction> &txs);
 
     bool check_for_double_spend(const transaction& tx, key_images_container& keys_this_block) const;
     void get_timestamp_and_difficulty(uint64_t &timestamp, difficulty_type &difficulty, const int offset) const;