Clean lmbd impl

- Reverted back to storing output_id in locked_outputs table; it's
required to make sure outputs enter the tree in chain order I see
no other simple way.
- Removed unnecessary comments and db flags (MDB_APPENDDUP already
makes sure key/value doesn't already exist, and when inserting,
every global output id should be unique, so should never get that
error)

src/blockchain_db/blockchain_db.cpp

@@ -74,17 +74,22 @@ static void get_outs_by_unlock_block(const cryptonote::transaction &tx,
 
     auto output_pair = fcmp_pp::curve_trees::OutputPair{
         .output_pubkey = std::move(output_public_key),
-        .commitment =    std::move(commitment)
+        .commitment    = std::move(commitment)
+      };
+
+    auto output_context = fcmp_pp::curve_trees::OutputContext{
+        .output_id   = output_ids[i],
+        .output_pair = std::move(output_pair)
       };
 
     if (outs_by_unlock_block_inout.find(unlock_block) == outs_by_unlock_block_inout.end())
     {
-      auto new_vec = std::vector<fcmp_pp::curve_trees::OutputPair>{std::move(output_pair)};
+      auto new_vec = std::vector<fcmp_pp::curve_trees::OutputContext>{std::move(output_context)};
       outs_by_unlock_block_inout[unlock_block] = std::move(new_vec);
     }
     else
     {
-      outs_by_unlock_block_inout[unlock_block].emplace_back(std::move(output_pair));
+      outs_by_unlock_block_inout[unlock_block].emplace_back(std::move(output_context));
     }
   }
 }

src/blockchain_db/blockchain_db.h

@@ -1783,7 +1783,7 @@ class BlockchainDB
   virtual bool for_all_alt_blocks(std::function<bool(const crypto::hash &blkid, const alt_block_data_t &data, const cryptonote::blobdata_ref *blob)> f, bool include_blob = false) const = 0;
 
   // TODO: description and make private
-  virtual void grow_tree(std::vector<fcmp_pp::curve_trees::OutputPair> &&new_leaves) = 0;
+  virtual void grow_tree(std::vector<fcmp_pp::curve_trees::OutputContext> &&new_leaves) = 0;
 
   virtual void trim_tree(const uint64_t trim_n_leaf_tuples) = 0;
 

src/blockchain_db/lmdb/db_lmdb.cpp

@@ -199,7 +199,7 @@ namespace
  *
  * spent_keys       input hash   -
  *
- * locked_outputs   block ID     [{output pubkey, commitment}...]
+ * locked_outputs   block ID     [{output ID, output pubkey, commitment}...]
  * leaves           leaf_idx     {output pubkey, commitment}
  * layers           layer_idx    [{child_chunk_idx, child_chunk_hash}...]
  *
@@ -887,9 +887,8 @@ void BlockchainLMDB::add_block(const block& blk, size_t block_weight, uint64_t l
     MDB_val_set(k_block_id, locked_output.first);
     MDB_val_set(v_output, locked_output.second);
 
-    // MDB_NODUPDATA because no benefit to having duplicate outputs in the tree, only 1 can be spent
-    result = mdb_cursor_put(m_cur_locked_outputs, &k_block_id, &v_output, MDB_APPENDDUP | MDB_NODUPDATA);
-    if (result != MDB_SUCCESS && result != MDB_KEYEXIST)
+    result = mdb_cursor_put(m_cur_locked_outputs, &k_block_id, &v_output, MDB_APPENDDUP);
+    if (result != MDB_SUCCESS)
       throw0(DB_ERROR(lmdb_error("Failed to add locked output: ", result).c_str()));
   }
 
@@ -1344,7 +1343,7 @@ void BlockchainLMDB::remove_spent_key(const crypto::key_image& k_image)
   }
 }
 
-void BlockchainLMDB::grow_tree(std::vector<fcmp_pp::curve_trees::OutputPair> &&new_leaves)
+void BlockchainLMDB::grow_tree(std::vector<fcmp_pp::curve_trees::OutputContext> &&new_leaves)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
   if (new_leaves.empty())
@@ -1375,10 +1374,7 @@ void BlockchainLMDB::grow_tree(std::vector<fcmp_pp::curve_trees::OutputPair> &&n
     MDB_val_copy<uint64_t> k(i + leaves.start_leaf_tuple_idx);
     MDB_val_set(v, leaves.tuples[i]);
 
-    // TODO: according to the docs, MDB_APPEND isn't supposed to perform any key comparisons to maximize efficiency.
-    // Adding MDB_NOOVERWRITE I assume re-introduces a key comparison. Benchmark NOOVERWRITE here
-    // MDB_NOOVERWRITE makes sure key doesn't already exist
-    int result = mdb_cursor_put(m_cur_leaves, &k, &v, MDB_APPEND | MDB_NOOVERWRITE);
+    int result = mdb_cursor_put(m_cur_leaves, &k, &v, MDB_APPEND);
     if (result != MDB_SUCCESS)
       throw0(DB_ERROR(lmdb_error("Failed to add leaf: ", result).c_str()));
   }
@@ -1471,10 +1467,7 @@ void BlockchainLMDB::grow_layer(const std::unique_ptr<C> &curve,
     lv.child_chunk_hash = curve->to_bytes(ext.hashes[i]);
     MDB_val_set(v, lv);
 
-    // TODO: according to the docs, MDB_APPENDDUP isn't supposed to perform any key comparisons to maximize efficiency.
-    // Adding MDB_NODUPDATA I assume re-introduces a key comparison. Benchmark MDB_NODUPDATA here
-    // MDB_NODUPDATA makes sure key/data pair doesn't already exist
-    int result = mdb_cursor_put(m_cur_layers, &k, &v, MDB_APPENDDUP | MDB_NODUPDATA);
+    int result = mdb_cursor_put(m_cur_layers, &k, &v, MDB_APPENDDUP);
     if (result != MDB_SUCCESS)
       throw0(DB_ERROR(lmdb_error("Failed to add hash: ", result).c_str()));
   }
@@ -2225,7 +2218,7 @@ bool BlockchainLMDB::audit_layer(const std::unique_ptr<C_CHILD> &c_child,
   return audit_complete;
 }
 
-std::vector<fcmp_pp::curve_trees::OutputPair> BlockchainLMDB::get_outs_at_unlock_block_id(
+std::vector<fcmp_pp::curve_trees::OutputContext> BlockchainLMDB::get_outs_at_unlock_block_id(
   uint64_t block_id)
 {
   LOG_PRINT_L3("BlockchainLMDB::" << __func__);
@@ -2238,7 +2231,7 @@ std::vector<fcmp_pp::curve_trees::OutputPair> BlockchainLMDB::get_outs_at_unlock
   MDB_val v_output;
 
   // Get all the locked outputs at the provided block id
-  std::vector<fcmp_pp::curve_trees::OutputPair> outs;
+  std::vector<fcmp_pp::curve_trees::OutputContext> outs;
 
   MDB_cursor_op op = MDB_SET;
   while (1)
@@ -2254,8 +2247,8 @@ std::vector<fcmp_pp::curve_trees::OutputPair> BlockchainLMDB::get_outs_at_unlock
     if (blk_id != block_id)
       throw0(DB_ERROR(("Blk id " + std::to_string(blk_id) + " not the expected" + std::to_string(block_id)).c_str()));
 
-    const auto range_begin = ((const fcmp_pp::curve_trees::OutputPair*)v_output.mv_data);
-    const auto range_end = range_begin + v_output.mv_size / sizeof(fcmp_pp::curve_trees::OutputPair);
+    const auto range_begin = ((const fcmp_pp::curve_trees::OutputContext*)v_output.mv_data);
+    const auto range_end = range_begin + v_output.mv_size / sizeof(fcmp_pp::curve_trees::OutputContext);
 
     auto it = range_begin;
 
@@ -2474,7 +2467,6 @@ void BlockchainLMDB::open(const std::string& filename, const int db_flags)
   mdb_set_dupsort(txn, m_tx_indices, compare_hash32);
   mdb_set_dupsort(txn, m_output_amounts, compare_uint64);
   mdb_set_dupsort(txn, m_locked_outputs, compare_uint64);
-  mdb_set_dupsort(txn, m_leaves, compare_uint64);
   mdb_set_dupsort(txn, m_layers, compare_uint64);
   mdb_set_dupsort(txn, m_output_txs, compare_uint64);
   mdb_set_dupsort(txn, m_block_info, compare_uint64);
@@ -6867,25 +6859,27 @@ void BlockchainLMDB::migrate_5_6()
         }
 
         // Prepare the output for insertion to the tree
-        const auto output_pair = fcmp_pp::curve_trees::OutputPair{
+        auto output_pair = fcmp_pp::curve_trees::OutputPair{
             .output_pubkey = std::move(output_data.pubkey),
-            .commitment =    std::move(output_data.commitment)
+            .commitment    = std::move(output_data.commitment)
+          };
+
+        auto output_context = fcmp_pp::curve_trees::OutputContext{
+            .output_id   = output_id,
+            .output_pair = std::move(output_pair)
           };
 
         // Get the block in which the output will unlock
         const uint64_t unlock_block = cryptonote::get_unlock_block_index(output_data.unlock_time, output_data.height);
 
         // Now add the output to the locked outputs table
         MDB_val_set(k_block_id, unlock_block);
-        MDB_val_set(v_output, output_pair);
+        MDB_val_set(v_output, output_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_outputs, &k_block_id, &v_output, MDB_NODUPDATA);
-        if (result != MDB_SUCCESS && result != MDB_KEYEXIST)
+        result = mdb_cursor_put(c_locked_outputs, &k_block_id, &v_output, 0);
+        if (result != MDB_SUCCESS)
           throw0(DB_ERROR(lmdb_error("Failed to add locked output: ", result).c_str()));
-        if (result == MDB_KEYEXIST)
-          MDEBUG("Duplicate output pubkey: " << output_pair.output_pubkey << " , output_id: " << output_id);
       }
     }
 

src/blockchain_db/lmdb/db_lmdb.h

@@ -368,7 +368,7 @@ class BlockchainLMDB : public BlockchainDB
   static int compare_string(const MDB_val *a, const MDB_val *b);
 
   // make private
-  virtual void grow_tree(std::vector<fcmp_pp::curve_trees::OutputPair> &&new_leaves);
+  virtual void grow_tree(std::vector<fcmp_pp::curve_trees::OutputContext> &&new_leaves);
 
   virtual void trim_tree(const uint64_t trim_n_leaf_tuples);
 
@@ -447,7 +447,7 @@ class BlockchainLMDB : public BlockchainDB
     const uint64_t child_layer_idx,
     const uint64_t chunk_width) const;
 
-  std::vector<fcmp_pp::curve_trees::OutputPair> get_outs_at_unlock_block_id(uint64_t block_id);
+  std::vector<fcmp_pp::curve_trees::OutputContext> get_outs_at_unlock_block_id(uint64_t block_id);
 
   void del_locked_outs_at_block_id(uint64_t block_id);
 

src/blockchain_db/testdb.h

@@ -116,7 +116,7 @@ class BaseTestDB: public cryptonote::BlockchainDB {
   virtual void add_tx_amount_output_indices(const uint64_t tx_index, const std::vector<uint64_t>& amount_output_indices) override {}
   virtual void add_spent_key(const crypto::key_image& k_image) override {}
   virtual void remove_spent_key(const crypto::key_image& k_image) override {}
-  virtual void grow_tree(std::vector<fcmp_pp::curve_trees::OutputPair> &&new_leaves) override {};
+  virtual void grow_tree(std::vector<fcmp_pp::curve_trees::OutputContext> &&new_leaves) override {};
   virtual void trim_tree(const uint64_t trim_n_leaf_tuples) override {};
   virtual bool audit_tree(const uint64_t expected_n_leaf_tuples) const override { return false; };
   virtual std::array<uint8_t, 32UL> get_tree_root() const override { return {}; };

src/fcmp_pp/curve_trees.cpp

@@ -706,25 +706,29 @@ template<typename C1, typename C2>
 typename CurveTrees<C1, C2>::TreeExtension CurveTrees<C1, C2>::get_tree_extension(
     const uint64_t old_n_leaf_tuples,
     const LastHashes &existing_last_hashes,
-    std::vector<OutputPair> &&new_leaf_tuples) const
+    std::vector<OutputContext> &&new_leaf_tuples) const
 {
     TreeExtension tree_extension;
     tree_extension.leaves.start_leaf_tuple_idx = old_n_leaf_tuples;
 
     if (new_leaf_tuples.empty())
         return tree_extension;
 
-    // Convert outputs into leaf tuples, place each element of each leaf tuple in a flat vector to be hashed, and place
-    // the outputs in a tree extension struct for insertion into the db. We ignore invalid outputs, since they cannot be
-    // inserted to the tree.
+    // Sort the leaves by order they appear in the chain
+    const auto sort_fn = [](const OutputContext &a, const OutputContext &b) { return a.output_id < b.output_id; };
+    std::sort(new_leaf_tuples.begin(), new_leaf_tuples.end(), sort_fn);
+
+    // Convert sorted outputs into leaf tuples, place each element of each leaf tuple in a flat vector to be hashed,
+    // and place the outputs in a tree extension struct for insertion into the db. We ignore invalid outputs, since
+    // they cannot be inserted to the tree.
     std::vector<typename C2::Scalar> flattened_leaves;
     flattened_leaves.reserve(new_leaf_tuples.size() * LEAF_TUPLE_SIZE);
     tree_extension.leaves.tuples.reserve(new_leaf_tuples.size());
     for (auto &o : new_leaf_tuples)
     {
         // TODO: this loop can be parallelized
         LeafTuple leaf;
-        try { leaf = leaf_tuple(o); }
+        try { leaf = leaf_tuple(o.output_pair); }
         catch(...)
         {
           // Invalid outputs can't be added to the tree
@@ -737,7 +741,7 @@ typename CurveTrees<C1, C2>::TreeExtension CurveTrees<C1, C2>::get_tree_extensio
         flattened_leaves.emplace_back(std::move(leaf.C_x));
 
         // We can derive {O.x,I.x,C.x} from output pairs, so we store just the output pair in the db to save 32 bytes
-        tree_extension.leaves.tuples.emplace_back(std::move(o));
+        tree_extension.leaves.tuples.emplace_back(std::move(o.output_pair));
     }
 
     if (flattened_leaves.empty())
@@ -802,7 +806,7 @@ typename CurveTrees<C1, C2>::TreeExtension CurveTrees<C1, C2>::get_tree_extensio
 template CurveTrees<Helios, Selene>::TreeExtension CurveTrees<Helios, Selene>::get_tree_extension(
     const uint64_t old_n_leaf_tuples,
     const LastHashes &existing_last_hashes,
-    std::vector<OutputPair> &&new_leaf_tuples) const;
+    std::vector<OutputContext> &&new_leaf_tuples) const;
 //----------------------------------------------------------------------------------------------------------------------
 template<typename C1, typename C2>
 std::vector<TrimLayerInstructions> CurveTrees<C1, C2>::get_trim_instructions(

src/fcmp_pp/curve_trees.h

@@ -134,14 +134,26 @@ struct TrimLayerInstructions final
 // - Output pairs do NOT necessarily have torsion cleared. We need the output pubkey as it exists in the chain in order
 //   to derive the correct I (when deriving {O.x, I.x, C.x}). Torsion clearing O before deriving I from O would enable
 //   spending a torsioned output once before the fcmp++ fork and again with a different key image via fcmp++.
+#pragma pack(push, 1)
 struct OutputPair final
 {
     crypto::public_key output_pubkey;
     rct::key           commitment;
 };
-static_assert(sizeof(OutputPair) == (32+32), "db expects 64 bytes for output pairs");
 
-using OutputsByUnlockBlock = std::map<uint64_t, std::vector<OutputPair>>;
+// Contextual wrapper for the output
+struct OutputContext final
+{
+    // Output's global id in the chain, used to insert the output in the tree in the order it entered the chain
+    uint64_t output_id;
+    OutputPair output_pair;
+};
+#pragma pack(pop)
+
+static_assert(sizeof(OutputPair)    == (32+32),   "db expects 64 bytes for output pairs");
+static_assert(sizeof(OutputContext) == (8+32+32), "db expects 72 bytes for output context");
+
+using OutputsByUnlockBlock = std::map<uint64_t, std::vector<OutputContext>>;
 
 //----------------------------------------------------------------------------------------------------------------------
 //----------------------------------------------------------------------------------------------------------------------
@@ -237,7 +249,7 @@ class CurveTrees
     // leaves to add to the tree, and return a tree extension struct that can be used to extend a tree
     TreeExtension get_tree_extension(const uint64_t old_n_leaf_tuples,
         const LastHashes &existing_last_hashes,
-        std::vector<OutputPair> &&new_leaf_tuples) const;
+        std::vector<OutputContext> &&new_leaf_tuples) const;
 
     // Get instructions useful for trimming all existing layers in the tree
     std::vector<TrimLayerInstructions> get_trim_instructions(

tests/unit_tests/curve_trees.cpp

@@ -744,15 +744,17 @@ void CurveTreesGlobalTree::log_tree()
 //----------------------------------------------------------------------------------------------------------------------
 // Test helpers
 //----------------------------------------------------------------------------------------------------------------------
-static const std::vector<fcmp_pp::curve_trees::OutputPair> generate_random_leaves(const CurveTreesV1 &curve_trees,
+static const std::vector<fcmp_pp::curve_trees::OutputContext> generate_random_leaves(const CurveTreesV1 &curve_trees,
     const std::size_t old_n_leaf_tuples,
     const std::size_t new_n_leaf_tuples)
 {
-    std::vector<fcmp_pp::curve_trees::OutputPair> output_pairs;
-    output_pairs.reserve(new_n_leaf_tuples);
+    std::vector<fcmp_pp::curve_trees::OutputContext> outs;
+    outs.reserve(new_n_leaf_tuples);
 
     for (std::size_t i = 0; i < new_n_leaf_tuples; ++i)
     {
+        const std::uint64_t output_id = old_n_leaf_tuples + i;
+
         // Generate random output tuple
         crypto::secret_key o,c;
         crypto::public_key O,C;
@@ -765,10 +767,15 @@ static const std::vector<fcmp_pp::curve_trees::OutputPair> generate_random_leave
                 .commitment    = std::move(C_key)
             };
 
-        output_pairs.emplace_back(std::move(output_pair));
+        auto output_context = fcmp_pp::curve_trees::OutputContext{
+                .output_id   = output_id,
+                .output_pair = std::move(output_pair)
+            };
+
+        outs.emplace_back(std::move(output_context));
     }
 
-    return output_pairs;
+    return outs;
 }
 //----------------------------------------------------------------------------------------------------------------------
 static const Selene::Scalar generate_random_selene_scalar()