Match output unlock time (fix off by 1)

src/blockchain_db/blockchain_db.cpp

@@ -288,11 +288,11 @@ uint64_t BlockchainDB::add_block( const std::pair<block, blobdata>& blck
   time1 = epee::misc_utils::get_tick_count();
 
   std::vector<std::vector<uint64_t>> output_ids;
-  output_ids.reserve(1 + txs.size());
+  output_ids.reserve(txs.size());
 
   uint64_t num_rct_outs = 0;
   blobdata miner_bd = tx_to_blob(blk.miner_tx);
-  output_ids.push_back(add_transaction(blk_hash, std::make_pair(blk.miner_tx, blobdata_ref(miner_bd))));
+  std::vector<uint64_t> miner_output_ids = add_transaction(blk_hash, std::make_pair(blk.miner_tx, blobdata_ref(miner_bd)));
   if (blk.miner_tx.version == 2)
     num_rct_outs += blk.miner_tx.vout.size();
   int tx_i = 0;
@@ -314,30 +314,30 @@ uint64_t BlockchainDB::add_block( const std::pair<block, blobdata>& blck
   // When adding a block, we also need to add all the leaf tuples included in
   // the block to a table keeping track of locked leaf tuples. Once those leaf
   // tuples unlock, we use them to grow the tree.
-  std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext> leaf_tuples_by_unlock_height;
+  std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext> leaf_tuples_by_unlock_block;
 
   // Get miner tx's leaf tuples
   fcmp::curve_trees::curve_trees_v1.tx_outs_to_leaf_tuples(
     blk.miner_tx,
-    output_ids[0],
+    miner_output_ids,
     prev_height,
     true/*miner_tx*/,
-    leaf_tuples_by_unlock_height);
+    leaf_tuples_by_unlock_block);
 
   // Get all other txs' leaf tuples
   for (std::size_t i = 0; i < txs.size(); ++i)
   {
     fcmp::curve_trees::curve_trees_v1.tx_outs_to_leaf_tuples(
       txs[i].first,
-      output_ids[i+1],
+      output_ids[i],
       prev_height,
       false/*miner_tx*/,
-      leaf_tuples_by_unlock_height);
+      leaf_tuples_by_unlock_block);
   }
 
   // call out to subclass implementation to add the block & metadata
   time1 = epee::misc_utils::get_tick_count();
-  add_block(blk, block_weight, long_term_block_weight, cumulative_difficulty, coins_generated, num_rct_outs, blk_hash, leaf_tuples_by_unlock_height);
+  add_block(blk, block_weight, long_term_block_weight, cumulative_difficulty, coins_generated, num_rct_outs, blk_hash, leaf_tuples_by_unlock_block);
   TIME_MEASURE_FINISH(time1);
   time_add_block1 += time1;
 

src/blockchain_db/blockchain_db.h

@@ -407,7 +407,7 @@ private:
    * @param cumulative_difficulty the accumulated difficulty after this block
    * @param coins_generated the number of coins generated total after this block
    * @param blk_hash the hash of the block
-   * @param leaf_tuples_by_unlock_height the leaves from this block to add to the merkle tree
+   * @param leaf_tuples_by_unlock_block the leaves from this block to add to the merkle tree
    */
   virtual void add_block( const block& blk
                 , size_t block_weight
@@ -416,7 +416,7 @@ private:
                 , const uint64_t& coins_generated
                 , uint64_t num_rct_outs
                 , const crypto::hash& blk_hash
-                , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+                , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
                 ) = 0;
 
   /**

src/blockchain_db/lmdb/db_lmdb.cpp

@@ -817,7 +817,7 @@ estim:
 }
 
 void BlockchainLMDB::add_block(const block& blk, size_t block_weight, uint64_t long_term_block_weight, const difficulty_type& cumulative_difficulty, const uint64_t& coins_generated,
-    uint64_t num_rct_outs, const crypto::hash& blk_hash, const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext> &leaf_tuples_by_unlock_height)
+    uint64_t num_rct_outs, const crypto::hash& blk_hash, const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext> &leaf_tuples_by_unlock_block)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   check_open();
@@ -846,7 +846,7 @@ void BlockchainLMDB::add_block(const block& blk, size_t block_weight, uint64_t l
   }
 
   // Grow the tree with outputs that unlock at this block height
-  const auto unlocked_leaf_tuples = this->get_locked_leaf_tuples_at_height(m_height);
+  const auto unlocked_leaf_tuples = this->get_locked_leaf_tuples_at_block_id(m_height);
 
   this->grow_tree(fcmp::curve_trees::curve_trees_v1, unlocked_leaf_tuples);
 
@@ -900,14 +900,14 @@ void BlockchainLMDB::add_block(const block& blk, size_t block_weight, uint64_t l
   CURSOR(locked_leaves)
 
   // Add the locked leaf tuples from this block to the locked outputs table
-  for (const auto &locked_tuple : leaf_tuples_by_unlock_height)
+  for (const auto &locked_tuple : leaf_tuples_by_unlock_block)
   {
-    MDB_val_set(k_height, locked_tuple.first);
+    MDB_val_set(k_block_id, locked_tuple.first);
     MDB_val_set(v_tuple, locked_tuple.second);
 
     // MDB_NODUPDATA because no benefit to having duplicate outputs in the tree, only 1 can be spent
     // Can't use MDB_APPENDDUP because outputs aren't inserted in order sorted by unlock height
-    result = mdb_cursor_put(m_cur_locked_leaves, &k_height, &v_tuple, MDB_NODUPDATA);
+    result = mdb_cursor_put(m_cur_locked_leaves, &k_block_id, &v_tuple, MDB_NODUPDATA);
     if (result != MDB_SUCCESS && result != MDB_KEYEXIST)
       throw0(DB_ERROR(lmdb_error("Failed to add locked output: ", result).c_str()));
   }
@@ -2206,8 +2206,8 @@ bool BlockchainLMDB::audit_layer(const C_CHILD &c_child,
     chunk_width);
 }
 
-std::vector<fcmp::curve_trees::CurveTreesV1::LeafTupleContext> BlockchainLMDB::get_locked_leaf_tuples_at_height(
+std::vector<fcmp::curve_trees::CurveTreesV1::LeafTupleContext> BlockchainLMDB::get_locked_leaf_tuples_at_block_id(
-  const uint64_t height)
+  uint64_t block_id)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   check_open();
@@ -2215,7 +2215,7 @@ std::vector<fcmp::curve_trees::CurveTreesV1::LeafTupleContext> BlockchainLMDB::g
   TXN_PREFIX_RDONLY();
   RCURSOR(locked_leaves)
 
-  MDB_val_set(k_height, height);
+  MDB_val_set(k_block_id, block_id);
   MDB_val v_tuple;
 
   // Get all the locked outputs at that height
@@ -2225,16 +2225,16 @@ std::vector<fcmp::curve_trees::CurveTreesV1::LeafTupleContext> BlockchainLMDB::g
   MDB_cursor_op op = MDB_SET;
   while (1)
   {
-    int result = mdb_cursor_get(m_cur_locked_leaves, &k_height, &v_tuple, op);
+    int result = mdb_cursor_get(m_cur_locked_leaves, &k_block_id, &v_tuple, op);
     if (result == MDB_NOTFOUND)
       break;
     if (result != MDB_SUCCESS)
       throw0(DB_ERROR(lmdb_error("Failed to get next locked outputs: ", result).c_str()));
     op = MDB_NEXT_MULTIPLE;
 
-    const uint64_t h = *(const uint64_t*)k_height.mv_data;
+    const uint64_t blk_id = *(const uint64_t*)k_block_id.mv_data;
-    if (h != height)
+    if (blk_id != block_id)
-      throw0(DB_ERROR(("Height " + std::to_string(h) + " not the expected" + std::to_string(height)).c_str()));
+      throw0(DB_ERROR(("Height " + std::to_string(blk_id) + " not the expected" + std::to_string(block_id)).c_str()));
 
     const auto range_begin = ((const fcmp::curve_trees::CurveTreesV1::LeafTupleContext*)v_tuple.mv_data);
     const auto range_end = range_begin + v_tuple.mv_size / sizeof(fcmp::curve_trees::CurveTreesV1::LeafTupleContext);
@@ -6777,15 +6777,15 @@ void BlockchainLMDB::migrate_5_6()
         }
 
         // Get the block in which the output will unlock
-        const uint64_t unlock_height = cryptonote::get_unlock_height(output_data.unlock_time, output_data.height);
+        const uint64_t unlock_block = cryptonote::get_unlock_block_index(output_data.unlock_time, output_data.height);
 
         // Now add the leaf tuple to the locked leaves table
-        MDB_val_set(k_height, unlock_height);
+        MDB_val_set(k_block_id, unlock_block);
         MDB_val_set(v_tuple, tuple_context);
 
         // MDB_NODUPDATA because no benefit to having duplicate outputs in the tree, only 1 can be spent
         // Can't use MDB_APPENDDUP because outputs aren't inserted in order sorted by unlock height
-        result = mdb_cursor_put(c_locked_leaves, &k_height, &v_tuple, MDB_NODUPDATA);
+        result = mdb_cursor_put(c_locked_leaves, &k_block_id, &v_tuple, MDB_NODUPDATA);
         if (result != MDB_SUCCESS && result != MDB_KEYEXIST)
           throw0(DB_ERROR(lmdb_error("Failed to add locked output: ", result).c_str()));
         if (result == MDB_KEYEXIST)
@@ -6866,7 +6866,7 @@ void BlockchainLMDB::migrate_5_6()
         }
 
         // Get all the locked outputs at that height
-        const auto leaf_tuples = this->get_locked_leaf_tuples_at_height(i);
+        const auto leaf_tuples = this->get_locked_leaf_tuples_at_block_id(i);
         this->grow_tree(fcmp::curve_trees::curve_trees_v1, leaf_tuples);
 
         // TODO: Remove locked outputs from the locked outputs table after adding them to tree

src/blockchain_db/lmdb/db_lmdb.h

@@ -390,7 +390,7 @@ private:
                 , const uint64_t& coins_generated
                 , uint64_t num_rct_outs
                 , const crypto::hash& block_hash
-                , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+                , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
                 );
 
   virtual void remove_block();
@@ -450,7 +450,7 @@ private:
     const uint64_t child_chunk_idx,
     const uint64_t chunk_width) const;
 
-  std::vector<fcmp::curve_trees::CurveTreesV1::LeafTupleContext> get_locked_leaf_tuples_at_height(const uint64_t height);
+  std::vector<fcmp::curve_trees::CurveTreesV1::LeafTupleContext> get_locked_leaf_tuples_at_block_id(uint64_t block_id);
 
   uint64_t num_outputs() const;
 

src/blockchain_db/testdb.h

@@ -148,7 +148,7 @@ public:
                         , const uint64_t& coins_generated
                         , uint64_t num_rct_outs
                         , const crypto::hash& blk_hash
-                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
                         ) override { }
   virtual cryptonote::block get_block_from_height(const uint64_t& height) const override { return cryptonote::block(); }
   virtual void set_hard_fork_version(uint64_t height, uint8_t version) override {}

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -1645,20 +1645,23 @@ namespace cryptonote
     return key;
   }
   //---------------------------------------------------------------
-  // TODO: write tests for this func
+  // TODO: write tests for this func that match with current daemon logic
-  uint64_t get_unlock_height(uint64_t unlock_time, uint64_t height_included_in_chain)
+  uint64_t get_unlock_block_index(uint64_t unlock_time, uint64_t block_included_in_chain)
   {
-    uint64_t unlock_height = 0;
+    uint64_t unlock_block_index = 0;
-    const uint64_t default_unlock_height = height_included_in_chain + CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE;
+
+    static_assert(CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE > 0, "unexpected default spendable age");
+    const uint64_t default_block_index = block_included_in_chain + (CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE - 1);
 
-    // TODO: double triple check off by 1
     if (unlock_time == 0)
     {
-      unlock_height = default_unlock_height;
+      unlock_block_index = default_block_index;
     }
     else if (unlock_time < CRYPTONOTE_MAX_BLOCK_NUMBER)
     {
-      unlock_height = unlock_time;
+      // The unlock_time in this case is supposed to be the chain height at which the output unlocks
+      // The chain height is 1 higher than the highest block index, so we subtract 1 for this delta
+      unlock_block_index = unlock_time > 1 ? (unlock_time - 1) : 0;
     }
     else
     {
@@ -1674,29 +1677,29 @@ namespace cryptonote
         const auto seconds_since_unlock = hf_v15_time - unlock_time;
         const auto blocks_since_unlock = seconds_since_unlock / DIFFICULTY_TARGET_V2;
 
-        unlock_height = hf_v15_height >= blocks_since_unlock
+        unlock_block_index = hf_v15_height > blocks_since_unlock
           ? (hf_v15_height - blocks_since_unlock)
-          : default_unlock_height;
+          : default_block_index;
       }
       else
       {
         const auto seconds_until_unlock = unlock_time - hf_v15_time;
         const auto blocks_until_unlock = seconds_until_unlock / DIFFICULTY_TARGET_V2;
-        unlock_height = hf_v15_height + blocks_until_unlock;
+        unlock_block_index = hf_v15_height + blocks_until_unlock;
       }
 
       /* Note: since this function was introduced for the hf that included fcmp's, it's possible for an output to be
-          spent before it reaches the unlock_height going by the old rules; this is ok. It can't be spent again because
+          spent before it reaches the unlock_block_index going by the old rules; this is ok. It can't be spent again b/c
           it'll have a duplicate key image. It's also possible for an output to unlock by old rules, and then re-lock
-          again at the fork. This is also ok, we just need to be sure that the new hf rules use this unlock_height
+          again at the fork. This is also ok, we just need to be sure that the new hf rules use this unlock_block_index
           starting at the fork for fcmp's.
       */
 
       // TODO: double check the accuracy of this calculation
-      MDEBUG("unlock time: " << unlock_time << " , unlock_height: " << unlock_height);
+      MDEBUG("unlock time: " << unlock_time << " , unlock_block_index: " << unlock_block_index);
     }
 
-    // Can't unlock earlier than the default unlock height
+    // Can't unlock earlier than the default unlock block
-    return std::max(unlock_height, default_unlock_height);
+    return std::max(unlock_block_index, default_block_index);
   }
 }

src/cryptonote_basic/cryptonote_format_utils.h

@@ -266,7 +266,8 @@ namespace cryptonote
   crypto::secret_key encrypt_key(crypto::secret_key key, const epee::wipeable_string &passphrase);
   crypto::secret_key decrypt_key(crypto::secret_key key, const epee::wipeable_string &passphrase);
 
-  uint64_t get_unlock_height(uint64_t unlock_time, uint64_t height_included_in_chain);
+  // Returns the block index in which the provided unlock_time unlocks
+  uint64_t get_unlock_block_index(uint64_t unlock_time, uint64_t block_included_in_chain);
 
 #define CHECKED_GET_SPECIFIC_VARIANT(variant_var, specific_type, variable_name, fail_return_val) \
   CHECK_AND_ASSERT_MES(variant_var.type() == typeid(specific_type), fail_return_val, "wrong variant type: " << variant_var.type().name() << ", expected " << typeid(specific_type).name()); \

src/fcmp/curve_trees.cpp

@@ -684,9 +684,9 @@ void CurveTrees<Helios, Selene>::tx_outs_to_leaf_tuples(const cryptonote::transa
     const std::vector<uint64_t> &output_ids,
     const uint64_t tx_height,
     const bool miner_tx,
-    std::multimap<uint64_t, CurveTrees<Helios, Selene>::LeafTupleContext> &leaf_tuples_by_unlock_height_inout) const
+    std::multimap<uint64_t, CurveTrees<Helios, Selene>::LeafTupleContext> &leaf_tuples_by_unlock_block_inout) const
 {
-    const uint64_t unlock_height = cryptonote::get_unlock_height(tx.unlock_time, tx_height);
+    const uint64_t unlock_block = cryptonote::get_unlock_block_index(tx.unlock_time, tx_height);
 
     CHECK_AND_ASSERT_THROW_MES(tx.vout.size() == output_ids.size(), "unexpected size of output ids");
 
@@ -708,22 +708,23 @@ void CurveTrees<Helios, Selene>::tx_outs_to_leaf_tuples(const cryptonote::transa
             ? rct::zeroCommit(out.amount)
             : tx.rct_signatures.outPk[i].mask;
 
+        CurveTrees<Helios, Selene>::LeafTupleContext tuple_context;
+        tuple_context.output_id = output_ids[i];
+    
         try
         {
-            // Throws an error if output is invalid; we don't want leaf tuples from invalid outputs
+            // Convert output to leaf tuple; throws if output is invalid
-            auto leaf_tuple = output_to_leaf_tuple(
+            tuple_context.leaf_tuple = output_to_leaf_tuple(
                 output_public_key,
                 rct::rct2pk(commitment));
-
-            auto tuple_context = CurveTrees<Helios, Selene>::LeafTupleContext{
-                    .output_id  = output_ids[i],
-                    .leaf_tuple = std::move(leaf_tuple),
-                };
-
-            leaf_tuples_by_unlock_height_inout.emplace(unlock_height, std::move(tuple_context));
         }
         catch (...)
-        { /*continue*/ };
+        {
+            // We don't want leaf tuples from invalid outputs in the tree
+            continue;
+        };
+
+        leaf_tuples_by_unlock_block_inout.emplace(unlock_block, std::move(tuple_context));
     }
 }
 //----------------------------------------------------------------------------------------------------------------------

src/fcmp/curve_trees.h

@@ -232,7 +232,7 @@ public:
         const std::vector<uint64_t> &output_ids,
         const uint64_t tx_height,
         const bool miner_tx,
-        std::multimap<uint64_t, LeafTupleContext> &leaf_tuples_by_unlock_height_inout) const;
+        std::multimap<uint64_t, LeafTupleContext> &leaf_tuples_by_unlock_block_inout) const;
 
     // Take in the existing number of leaf tuples and the existing last hashes of each layer in the tree, as well as new
     // leaves to add to the tree, and return a tree extension struct that can be used to extend a tree

tests/block_weight/block_weight.cpp

@@ -65,7 +65,7 @@ public:
                         , const uint64_t& coins_generated
                         , uint64_t num_rct_outs
                         , const crypto::hash& blk_hash
-                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
                         ) override {
     blocks.push_back({block_weight, long_term_block_weight});
   }

tests/core_tests/chaingen.cpp

@@ -88,7 +88,7 @@ namespace
         , const uint64_t& coins_generated
         , uint64_t num_rct_outs
         , const crypto::hash& blk_hash
-        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
     ) override
     {
       blocks.push_back({blk, blk_hash});

tests/unit_tests/curve_trees.cpp

@@ -828,7 +828,8 @@ static bool trim_tree_in_memory(const std::size_t trim_n_leaf_tuples,
     CHECK_AND_ASSERT_THROW_MES(trim_n_leaf_tuples > 0, "must be trimming some leaves");
 
     // Trim the global tree by `trim_n_leaf_tuples`
-    LOG_PRINT_L1("Trimming " << trim_n_leaf_tuples << " leaf tuples from tree");
+    LOG_PRINT_L1("Trimming " << trim_n_leaf_tuples << " leaf tuples from tree with "
+        << old_n_leaf_tuples << " leaves in memory");
 
     global_tree.trim_tree(trim_n_leaf_tuples);
 

tests/unit_tests/hardfork.cpp

@@ -54,7 +54,7 @@ public:
                         , const uint64_t& coins_generated
                         , uint64_t num_rct_outs
                         , const crypto::hash& blk_hash
-                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
                         ) override {
     blocks.push_back(blk);
   }

tests/unit_tests/long_term_block_weight.cpp

@@ -58,7 +58,7 @@ public:
                         , const uint64_t& coins_generated
                         , uint64_t num_rct_outs
                         , const crypto::hash& blk_hash
-                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_height
+                        , const std::multimap<uint64_t, fcmp::curve_trees::CurveTreesV1::LeafTupleContext>& leaf_tuples_by_unlock_block
                         ) override {
     blocks.push_back({block_weight, long_term_block_weight});
   }