Transaction pool documentation (and some cleanup)

tx_pool.h doxygen documentation completed.
Many notes made on areas for improvement, be that functionality or
code clarity.
Commented code and unused code removed.

src/cryptonote_core/tx_pool.cpp

@@ -54,6 +54,9 @@ namespace cryptonote
 {
   namespace
   {
+    //FIXME: constants such as these should at least be in the header,
+    //       but probably somewhere more accessible to the rest of the
+    //       codebase.
     size_t const TRANSACTION_SIZE_LIMIT_V1 = (((CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V1 * 125) / 100) - CRYPTONOTE_COINBASE_BLOB_RESERVED_SIZE);
     size_t const TRANSACTION_SIZE_LIMIT_V2 = (((CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V2 * 125) / 100) - CRYPTONOTE_COINBASE_BLOB_RESERVED_SIZE);
     time_t const MIN_RELAY_TIME = (60 * 5); // only start re-relaying transactions after that many seconds
@@ -116,11 +119,12 @@ namespace cryptonote
       return false;
     }
 
+    // fee per kilobyte, size rounded up.
     uint64_t fee = inputs_amount - outputs_amount;
     uint64_t needed_fee = blob_size / 1024;
     needed_fee += (blob_size % 1024) ? 1 : 0;
     needed_fee *= FEE_PER_KB;
-    if (!kept_by_block && fee < needed_fee /*&& fee < MINING_ALLOWED_LEGACY_FEE*/)
+    if (!kept_by_block && fee < needed_fee)
     {
       LOG_PRINT_L1("transaction fee is not enough: " << print_money(fee) << ", minimum fee: " << print_money(needed_fee));
       tvc.m_verifivation_failed = true;
@@ -135,7 +139,9 @@ namespace cryptonote
       return false;
     }
 
-    //check key images for transaction if it is not kept by block
+    // if the transaction came from a block popped from the chain,
+    // don't check if we have its key images as spent.
+    // TODO: Investigate why not?
     if(!kept_by_block)
     {
       if(have_tx_keyimges_as_spent(tx))
@@ -163,9 +169,10 @@ namespace cryptonote
     CRITICAL_REGION_LOCAL(m_transactions_lock);
     if(!ch_inp_res)
     {
+      // if the transaction was valid before (kept_by_block), then it
+      // may become valid again, so ignore the failed inputs check.
       if(kept_by_block)
       {
-        //anyway add this transaction to pool, because it related to block
         auto txd_p = m_transactions.insert(transactions_container::value_type(id, tx_details()));
         CHECK_AND_ASSERT_MES(txd_p.second, false, "transaction already exists at inserting in memory pool");
         txd_p.first->second.blob_size = blob_size;
@@ -207,13 +214,14 @@ namespace cryptonote
         tvc.m_should_be_relayed = true;
     }
 
+    // assume failure during verification steps until success is certain
     tvc.m_verifivation_failed = true;
-    //update image_keys container, here should everything goes ok.
+
     BOOST_FOREACH(const auto& in, tx.vin)
     {
       CHECKED_GET_SPECIFIC_VARIANT(in, const txin_to_key, txin, false);
       std::unordered_set<crypto::hash>& kei_image_set = m_spent_key_images[txin.k_image];
-      CHECK_AND_ASSERT_MES(kept_by_block || kei_image_set.size() == 0, false, "internal error: keeped_by_block=" << kept_by_block
+      CHECK_AND_ASSERT_MES(kept_by_block || kei_image_set.size() == 0, false, "internal error: kept_by_block=" << kept_by_block
                                           << ",  kei_image_set.size()=" << kei_image_set.size() << ENDL << "txin.k_image=" << txin.k_image << ENDL
                                           << "tx_id=" << id );
       auto ins_res = kei_image_set.insert(id);
@@ -223,7 +231,7 @@ namespace cryptonote
     tvc.m_verifivation_failed = false;
 
     m_txs_by_fee.emplace((double)blob_size / fee, id);
-    //succeed
+
     return true;
   }
   //---------------------------------------------------------------------------------
@@ -235,6 +243,9 @@ namespace cryptonote
     return add_tx(tx, h, blob_size, tvc, keeped_by_block, relayed, version);
   }
   //---------------------------------------------------------------------------------
+  //FIXME: Can return early before removal of all of the key images.
+  //       At the least, need to make sure that a false return here
+  //       is treated properly.  Should probably not return early, however.
   bool tx_memory_pool::remove_transaction_keyimages(const transaction& tx)
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
@@ -301,7 +312,7 @@ namespace cryptonote
     );
   }
   //---------------------------------------------------------------------------------
-  //proper tx_pool handling courtesy of CryptoZoidberg and Boolberry
+  //TODO: investigate whether boolean return is appropriate
   bool tx_memory_pool::remove_stuck_transactions()
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
@@ -332,6 +343,7 @@ namespace cryptonote
     return true;
   }
   //---------------------------------------------------------------------------------
+  //TODO: investigate whether boolean return is appropriate
   bool tx_memory_pool::get_relayable_transactions(std::list<std::pair<crypto::hash, cryptonote::transaction>> &txs) const
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
@@ -380,6 +392,7 @@ namespace cryptonote
       txs.push_back(tx_vt.second.tx);
   }
   //------------------------------------------------------------------
+  //TODO: investigate whether boolean return is appropriate
   bool tx_memory_pool::get_transactions_and_spent_keys_info(std::vector<tx_info>& tx_infos, std::vector<spent_key_image_info>& key_image_infos) const
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
@@ -556,6 +569,7 @@ namespace cryptonote
     return ss.str();
   }
   //---------------------------------------------------------------------------------
+  //TODO: investigate whether boolean return is appropriate
   bool tx_memory_pool::fill_block_template(block &bl, size_t median_size, uint64_t already_generated_coins, size_t &total_size, uint64_t &fee)
   {
     // Warning: This function takes already_generated_
@@ -646,6 +660,7 @@ namespace cryptonote
     return n_removed;
   }
   //---------------------------------------------------------------------------------
+  //TODO: investigate whether only ever returning true is correct
   bool tx_memory_pool::init(const std::string& config_folder)
   {
     CRITICAL_REGION_LOCAL(m_transactions_lock);
@@ -679,6 +694,7 @@ namespace cryptonote
   }
 
   //---------------------------------------------------------------------------------
+  //TODO: investigate whether only ever returning true is correct
   bool tx_memory_pool::deinit()
   {
     if (m_config_folder.empty())