Merge pull request #749

bfd4a28 Update BlockchainDB documentation (Thomas Winget)
797357e Change Doxyfile, Blockchain not blockchain_storage (Thomas Winget)
c835215 remove defunct code from cryptonote::core (Thomas Winget)
50dba6d cryptonote::core doxygen documentation (Thomas Winget)
8ac329d doxygen documentation for difficulty functions (Thomas Winget)
540a76c Move checkpoint functions into checkpoints class (Thomas Winget)
1b0c98e doxygen documentation for checkpoints.{h,cpp} (Thomas Winget)
89c24ac Remove unnecessary or defunct code (Thomas Winget)
ab0ed14 doxygen include private and static members (Thomas Winget)
3a48449 Updated documentation for blockchain.* (Thomas Winget)

1 files changed, 39 insertions, 10 deletions

diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp
index 131f56a4d..2f42e1db5 100644
--- a/src/cryptonote_core/blockchain.cpp
+++ b/src/cryptonote_core/blockchain.cpp
@@ -49,7 +49,7 @@
 #include "common/boost_serialization_helper.h"
 #include "warnings.h"
 #include "crypto/hash.h"
-#include "cryptonote_core/checkpoints_create.h"
+#include "cryptonote_core/checkpoints.h"
 #include "cryptonote_core/cryptonote_core.h"
 #if defined(PER_BLOCK_CHECKPOINT)
 #include "blocks/blocks.h"
@@ -840,7 +840,7 @@ bool Blockchain::switch_to_alternative_blockchain(std::list<blocks_ext_by_hash::
     }
   }
 
-  //removing alt_chain entries from alternative chain
+  //removing alt_chain entries from alternative chains container
   for (auto ch_ent: alt_chain)
   {
     m_alternative_chains.erase(ch_ent);
@@ -991,8 +991,7 @@ bool Blockchain::validate_miner_transaction(const block& b, size_t cumulative_bl
   return true;
 }
 //------------------------------------------------------------------
-// get the block sizes of the last <count> blocks, starting at <from_height>
-// and return by reference <sz>.
+// get the block sizes of the last <count> blocks, and return by reference <sz>.
 void Blockchain::get_last_n_blocks_sizes(std::vector<size_t>& sz, size_t count) const
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
@@ -1408,6 +1407,10 @@ bool Blockchain::get_blocks(uint64_t start_offset, size_t count, std::list<block
 //TODO: This function *looks* like it won't need to be rewritten
 //      to use BlockchainDB, as it calls other functions that were,
 //      but it warrants some looking into later.
+//
+//FIXME: This function appears to want to return false if any transactions
+//       that belong with blocks are missing, but not if blocks themselves
+//       are missing.
 bool Blockchain::handle_get_objects(NOTIFY_REQUEST_GET_OBJECTS::request& arg, NOTIFY_RESPONSE_GET_OBJECTS::request& rsp)
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
@@ -1421,6 +1424,9 @@ bool Blockchain::handle_get_objects(NOTIFY_REQUEST_GET_OBJECTS::request& arg, NO
   {
     std::list<crypto::hash> missed_tx_ids;
     std::list<transaction> txs;
+
+    // FIXME: s/rsp.missed_ids/missed_tx_id/ ?  Seems like rsp.missed_ids
+    //        is for missed blocks, not missed transactions as well.
     get_transactions(bl.tx_hashes, txs, missed_tx_ids);
 
     if (missed_tx_ids.size() != 0)
@@ -1668,6 +1674,8 @@ uint64_t Blockchain::block_difficulty(uint64_t i) const
   return 0;
 }
 //------------------------------------------------------------------
+//TODO: return type should be void, throw on exception
+//       alternatively, return true only if no blocks missed
 template<class t_ids_container, class t_blocks_container, class t_missed_container>
 bool Blockchain::get_blocks(const t_ids_container& block_ids, t_blocks_container& blocks, t_missed_container& missed_bs) const
 {
@@ -1692,6 +1700,8 @@ bool Blockchain::get_blocks(const t_ids_container& block_ids, t_blocks_container
   return true;
 }
 //------------------------------------------------------------------
+//TODO: return type should be void, throw on exception
+//       alternatively, return true only if no transactions missed
 template<class t_ids_container, class t_tx_container, class t_missed_container>
 bool Blockchain::get_transactions(const t_ids_container& txs_ids, t_tx_container& txs, t_missed_container& missed_txs) const
 {
@@ -1708,7 +1718,6 @@ bool Blockchain::get_transactions(const t_ids_container& txs_ids, t_tx_container
     {
       missed_txs.push_back(tx_hash);
     }
-    //FIXME: is this the correct way to handle this?
     catch (const std::exception& e)
     {
       return false;
@@ -1965,6 +1974,11 @@ bool Blockchain::get_tx_outputs_gindexs(const crypto::hash& tx_id, std::vector<u
   return true;
 }
 //------------------------------------------------------------------
+//FIXME: it seems this function is meant to be merely a wrapper around
+//       another function of the same name, this one adding one bit of
+//       functionality.  Should probably move anything more than that
+//       (getting the hash of the block at height max_used_block_id)
+//       to the other function to keep everything in one place.
 // This function overloads its sister function with
 // an extra value (hash of highest block that holds an output used as input)
 // as a return-by-reference.
@@ -1975,6 +1989,7 @@ bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t& max_used_block
 
 #if defined(PER_BLOCK_CHECKPOINT)
   // check if we're doing per-block checkpointing
+  // FIXME: investigate why this block returns
   if (m_db->height() < m_blocks_hash_check.size() && kept_by_block)
   {
     TIME_MEASURE_START(a);
@@ -2038,6 +2053,10 @@ bool Blockchain::have_tx_keyimges_as_spent(const transaction &tx) const
 }
 //------------------------------------------------------------------
 // This function validates transaction inputs and their keys.
+// FIXME: consider moving functionality specific to one input into
+//        check_tx_input() rather than here, and use this function simply
+//        to iterate the inputs as necessary (splitting the task
+//        using threads, etc.)
 bool Blockchain::check_tx_inputs(const transaction& tx, uint64_t* pmax_used_block_height)
 {
   LOG_PRINT_L3("Blockchain::" << __func__);
@@ -2320,7 +2339,7 @@ bool Blockchain::check_tx_input(const txin_to_key& txin, const crypto::hash& tx_
 
   output_keys.clear();
 
-  //check ring signature
+  // collect output keys
   outputs_visitor vi(output_keys, *this);
   if (!scan_outputkeys_for_indexes(txin, vi, tx_prefix_hash, pmax_related_block_height))
   {
@@ -2617,6 +2636,11 @@ leave:
     txs.push_back(tx);
     TIME_MEASURE_START(dd);
 
+    // FIXME: the storage should not be responsible for validation.
+    //        If it does any, it is merely a sanity check.
+    //        Validation is the purview of the Blockchain class
+    //        - TW
+    //
     // ND: this is not needed, db->add_block() checks for duplicate k_images and fails accordingly.
     // if (!check_for_double_spend(tx, keys))
     // {
@@ -2797,6 +2821,8 @@ bool Blockchain::add_new_block(const block& bl_, block_verification_context& bvc
   return handle_block_to_main_chain(bl, id, bvc);
 }
 //------------------------------------------------------------------
+//TODO: Refactor, consider returning a failure height and letting
+//      caller decide course of action.
 void Blockchain::check_against_checkpoints(const checkpoints& points, bool enforce)
 {
   const auto& pts = points.get_points();
@@ -2834,16 +2860,16 @@ void Blockchain::check_against_checkpoints(const checkpoints& points, bool enfor
 // with an existing checkpoint.
 bool Blockchain::update_checkpoints(const std::string& file_path, bool check_dns)
 {
-  if (!cryptonote::load_checkpoints_from_json(m_checkpoints, file_path))
+  if (!m_checkpoints.load_checkpoints_from_json(file_path))
   {
-    return false;
+      return false;
   }
 
   // if we're checking both dns and json, load checkpoints from dns.
   // if we're not hard-enforcing dns checkpoints, handle accordingly
   if (m_enforce_dns_checkpoints && check_dns)
   {
-    if (!cryptonote::load_checkpoints_from_dns(m_checkpoints))
+    if (!m_checkpoints.load_checkpoints_from_dns())
     {
       return false;
     }
@@ -2851,7 +2877,7 @@ bool Blockchain::update_checkpoints(const std::string& file_path, bool check_dns
   else if (check_dns)
   {
     checkpoints dns_points;
-    cryptonote::load_checkpoints_from_dns(dns_points);
+    dns_points.load_checkpoints_from_dns();
     if (m_checkpoints.check_for_conflicts(dns_points))
     {
       check_against_checkpoints(dns_points, false);
@@ -2878,6 +2904,8 @@ void Blockchain::block_longhash_worker(const uint64_t height, const std::vector<
   TIME_MEASURE_START(t);
   slow_hash_allocate_state();
 
+  //FIXME: height should be changing here, as get_block_longhash expects
+  //       the height of the block passed to it
   for (const auto & block : blocks)
   {
     crypto::hash id = get_block_hash(block);
@@ -2933,6 +2961,7 @@ bool Blockchain::cleanup_handle_incoming_blocks(bool force_sync)
 }
 
 //------------------------------------------------------------------
+//FIXME: unused parameter txs
 void Blockchain::output_scan_worker(const uint64_t amount, const std::vector<uint64_t> &offsets, std::vector<output_data_t> &outputs, std::unordered_map<crypto::hash, cryptonote::transaction> &txs) const
 {
   try