Revert "feat: Avoid inserting an empty leaf in indexed trees on update (#10281)"

This reverts commit 5a04ca8.

Author: ludamad

barretenberg/cpp/src/barretenberg/crypto/merkle_tree/indexed_tree/content_addressed_indexed_tree.hpp

@@ -252,20 +252,18 @@ class ContentAddressedIndexedTree : public ContentAddressedAppendOnlyTree<Store,
                              const InsertionGenerationCallback& completion);
 
     struct InsertionUpdates {
-        // On insertion, we always update a low leaf. If it's creating a new leaf, we need to update the pointer to
-        // point to the new one, if it's an update to an existing leaf, we need to change its payload.
         LeafUpdate low_leaf_update;
-        // We don't create new leaves on update
-        std::optional<std::pair<IndexedLeafValueType, index_t>> new_leaf;
+        IndexedLeafValueType new_leaf;
+        index_t new_leaf_index;
     };
 
     struct SequentialInsertionGenerationResponse {
-        std::vector<InsertionUpdates> updates_to_perform;
+        std::shared_ptr<std::vector<InsertionUpdates>> updates_to_perform;
         index_t highest_index;
     };
 
     using SequentialInsertionGenerationCallback =
-        std::function<void(TypedResponse<SequentialInsertionGenerationResponse>&)>;
+        std::function<void(const TypedResponse<SequentialInsertionGenerationResponse>&)>;
     void generate_sequential_insertions(const std::vector<LeafValueType>& values,
                                         const SequentialInsertionGenerationCallback& completion);
 
@@ -1424,14 +1422,6 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::add_or_update_values_seq
     const AddSequentiallyCompletionCallbackWithWitness& completion,
     bool capture_witness)
 {
-
-    // This struct is used to collect some state from the asynchronous operations we are about to perform
-    struct IntermediateResults {
-        std::vector<InsertionUpdates> updates_to_perform;
-        size_t appended_leaves = 0;
-    };
-    auto results = std::make_shared<IntermediateResults>();
-
     auto on_error = [=](const std::string& message) {
         try {
             TypedResponse<AddIndexedDataSequentiallyResponse<LeafValueType>> response;
@@ -1453,7 +1443,7 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::add_or_update_values_seq
                 ReadTransactionPtr tx = store_->create_read_transaction();
                 store_->get_meta(meta, *tx, true);
 
-                index_t new_total_size = results->appended_leaves + meta.size;
+                index_t new_total_size = values.size() + meta.size;
                 meta.size = new_total_size;
                 meta.root = store_->get_current_root(*tx, true);
 
@@ -1462,29 +1452,23 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::add_or_update_values_seq
 
             if (capture_witness) {
                 // Split results->update_witnesses between low_leaf_witness_data and insertion_witness_data
+                // Currently we always insert an empty leaf, even if it's an update, so we can split based
+                // on the index of the witness data
                 response.inner.insertion_witness_data =
                     std::make_shared<std::vector<LeafUpdateWitnessData<LeafValueType>>>();
-                response.inner.insertion_witness_data->reserve(results->updates_to_perform.size());
-
+                ;
                 response.inner.low_leaf_witness_data =
                     std::make_shared<std::vector<LeafUpdateWitnessData<LeafValueType>>>();
-                response.inner.low_leaf_witness_data->reserve(results->updates_to_perform.size());
-
-                size_t current_witness_index = 0;
-                for (size_t i = 0; i < results->updates_to_perform.size(); ++i) {
-                    LeafUpdateWitnessData<LeafValueType> low_leaf_witness =
-                        updates_completion_response.inner.update_witnesses->at(current_witness_index++);
-                    response.inner.low_leaf_witness_data->push_back(low_leaf_witness);
-
-                    // If this update has an insertion, append the real witness
-                    if (results->updates_to_perform.at(i).new_leaf.has_value()) {
-                        LeafUpdateWitnessData<LeafValueType> insertion_witness =
-                            updates_completion_response.inner.update_witnesses->at(current_witness_index++);
-                        response.inner.insertion_witness_data->push_back(insertion_witness);
+                ;
+
+                for (size_t i = 0; i < updates_completion_response.inner.update_witnesses->size(); ++i) {
+                    LeafUpdateWitnessData<LeafValueType>& witness_data =
+                        updates_completion_response.inner.update_witnesses->at(i);
+                    // If even, it's a low leaf, if odd, it's an insertion witness
+                    if (i % 2 == 0) {
+                        response.inner.low_leaf_witness_data->push_back(witness_data);
                     } else {
-                        // If it's an update, append an empty witness
-                        response.inner.insertion_witness_data->push_back(LeafUpdateWitnessData<LeafValueType>{
-                            .leaf = IndexedLeafValueType::empty(), .index = 0, .path = std::vector<fr>(depth_) });
+                        response.inner.insertion_witness_data->push_back(witness_data);
                     }
                 }
             }
@@ -1496,33 +1480,23 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::add_or_update_values_seq
     // This signals the completion of the insertion data generation
     // Here we'll perform all updates to the tree
     SequentialInsertionGenerationCallback insertion_generation_completed =
-        [=, this](TypedResponse<SequentialInsertionGenerationResponse>& insertion_response) {
+        [=, this](const TypedResponse<SequentialInsertionGenerationResponse>& insertion_response) {
             if (!insertion_response.success) {
                 on_error(insertion_response.message);
                 return;
             }
 
             std::shared_ptr<std::vector<LeafUpdate>> flat_updates = std::make_shared<std::vector<LeafUpdate>>();
-            flat_updates->reserve(insertion_response.inner.updates_to_perform.size() * 2);
-
-            for (size_t i = 0; i < insertion_response.inner.updates_to_perform.size(); ++i) {
-                InsertionUpdates& insertion_update = insertion_response.inner.updates_to_perform.at(i);
+            for (size_t i = 0; i < insertion_response.inner.updates_to_perform->size(); ++i) {
+                InsertionUpdates& insertion_update = insertion_response.inner.updates_to_perform->at(i);
                 flat_updates->push_back(insertion_update.low_leaf_update);
-                if (insertion_update.new_leaf.has_value()) {
-                    results->appended_leaves++;
-                    IndexedLeafValueType new_leaf;
-                    index_t new_leaf_index = 0;
-                    std::tie(new_leaf, new_leaf_index) = insertion_update.new_leaf.value();
-                    flat_updates->push_back(LeafUpdate{
-                        .leaf_index = new_leaf_index,
-                        .updated_leaf = new_leaf,
-                        .original_leaf = IndexedLeafValueType::empty(),
-                    });
-                }
+                flat_updates->push_back(LeafUpdate{
+                    .leaf_index = insertion_update.new_leaf_index,
+                    .updated_leaf = insertion_update.new_leaf,
+                    .original_leaf = IndexedLeafValueType::empty(),
+                });
             }
-            // We won't use anymore updates_to_perform
-            results->updates_to_perform = std::move(insertion_response.inner.updates_to_perform);
-            assert(insertion_response.inner.updates_to_perform.size() == 0);
+
             if (capture_witness) {
                 perform_updates(flat_updates->size(), flat_updates, final_completion);
                 return;
@@ -1540,12 +1514,27 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::generate_sequential_inse
 {
     execute_and_report<SequentialInsertionGenerationResponse>(
         [=, this](TypedResponse<SequentialInsertionGenerationResponse>& response) {
+            response.inner.highest_index = 0;
+            response.inner.updates_to_perform = std::make_shared<std::vector<InsertionUpdates>>();
+
             TreeMeta meta;
             ReadTransactionPtr tx = store_->create_read_transaction();
             store_->get_meta(meta, *tx, true);
 
             RequestContext requestContext;
             requestContext.includeUncommitted = true;
+            //  Ensure that the tree is not going to be overfilled
+            index_t new_total_size = values.size() + meta.size;
+            if (new_total_size > max_size_) {
+                throw std::runtime_error(format("Unable to insert values into tree ",
+                                                meta.name,
+                                                " new size: ",
+                                                new_total_size,
+                                                " max size: ",
+                                                max_size_));
+            }
+            // The highest index touched will be the last leaf index, since we append a zero for updates
+            response.inner.highest_index = new_total_size - 1;
             requestContext.root = store_->get_current_root(*tx, true);
             // Fetch the frontier (non empty nodes to the right) of the tree. This will ensure that perform_updates or
             // perform_updates_without_witness has all the cached nodes it needs to perform the insertions. See comment
@@ -1554,15 +1543,12 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::generate_sequential_inse
                 find_leaf_hash(meta.size - 1, requestContext, *tx, true);
             }
 
-            index_t current_size = meta.size;
-
             for (size_t i = 0; i < values.size(); ++i) {
                 const LeafValueType& new_payload = values[i];
-                // TODO(Alvaro) - Rethink this. I think it's fine for us to interpret empty values as a regular update
-                // (it'd empty out the payload of the zero leaf)
                 if (new_payload.is_empty()) {
                     continue;
                 }
+                index_t index_of_new_leaf = i + meta.size;
                 fr value = new_payload.get_key();
 
                 // This gives us the leaf that need updating
@@ -1609,25 +1595,25 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::generate_sequential_inse
                             .updated_leaf = IndexedLeafValueType::empty(),
                             .original_leaf = low_leaf,
                         },
-                    .new_leaf = std::nullopt,
+                    .new_leaf = IndexedLeafValueType::empty(),
+                    .new_leaf_index = index_of_new_leaf,
                 };
 
                 if (!is_already_present) {
                     // Update the current leaf to point it to the new leaf
                     IndexedLeafValueType new_leaf =
                         IndexedLeafValueType(new_payload, low_leaf.nextIndex, low_leaf.nextValue);
-                    index_t index_of_new_leaf = current_size;
+
                     low_leaf.nextIndex = index_of_new_leaf;
                     low_leaf.nextValue = value;
-                    current_size++;
                     // Cache the new leaf
                     store_->set_leaf_key_at_index(index_of_new_leaf, new_leaf);
                     store_->put_cached_leaf_by_index(index_of_new_leaf, new_leaf);
                     // Update cached low leaf
                     store_->put_cached_leaf_by_index(low_leaf_index, low_leaf);
 
                     insertion_update.low_leaf_update.updated_leaf = low_leaf;
-                    insertion_update.new_leaf = std::pair(new_leaf, index_of_new_leaf);
+                    insertion_update.new_leaf = new_leaf;
                 } else if (IndexedLeafValueType::is_updateable()) {
                     // Update the current leaf's value, don't change it's link
                     IndexedLeafValueType replacement_leaf =
@@ -1645,20 +1631,8 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::generate_sequential_inse
                                                     " is already present"));
                 }
 
-                response.inner.updates_to_perform.push_back(insertion_update);
-            }
-
-            //  Ensure that the tree is not going to be overfilled
-            if (current_size > max_size_) {
-                throw std::runtime_error(format("Unable to insert values into tree ",
-                                                meta.name,
-                                                " new size: ",
-                                                current_size,
-                                                " max size: ",
-                                                max_size_));
+                response.inner.updates_to_perform->push_back(insertion_update);
             }
-            // The highest index touched will be current_size - 1
-            response.inner.highest_index = current_size - 1;
         },
         completion);
 }

barretenberg/cpp/src/barretenberg/crypto/merkle_tree/indexed_tree/indexed_leaf.hpp

@@ -107,7 +107,7 @@ struct PublicDataLeafValue {
 
     fr get_key() const { return slot; }
 
-    bool is_empty() const { return slot == fr::zero() && value == fr::zero(); }
+    bool is_empty() const { return slot == fr::zero(); }
 
     std::vector<fr> get_hash_inputs(fr nextValue, fr nextIndex) const
     {

barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/merkle_tree.cpp

@@ -102,8 +102,12 @@ FF AvmMerkleTreeTraceBuilder::perform_storage_write([[maybe_unused]] uint32_t cl
         //  We update the low value
         low_preimage.value = value;
         FF low_preimage_hash = unconstrained_hash_public_data_preimage(low_preimage);
-        // Update the low leaf
-        return unconstrained_update_leaf_index(low_preimage_hash, static_cast<uint64_t>(low_index), low_path);
+        // Update the low leaf - this will be returned in future
+        [[maybe_unused]] FF root =
+            unconstrained_update_leaf_index(low_preimage_hash, static_cast<uint64_t>(low_index), low_path);
+        // TEMPORARY UNTIL WE CHANGE HOW UPDATES WORK
+        // Insert a zero leaf at the insertion index
+        return unconstrained_update_leaf_index(FF::zero(), static_cast<uint64_t>(insertion_index), insertion_path);
     }
     // The new leaf for an insertion is
     PublicDataTreeLeafPreimage new_preimage{

noir-projects/noir-protocol-circuits/crates/rollup-lib/src/base/components/public_data_tree.nr

@@ -57,11 +57,16 @@ pub(crate) fn public_data_tree_insert(
             },
             |write: PublicDataTreeLeaf, low_preimage: PublicDataTreeLeafPreimage| {
                 // Build insertion leaf
-                PublicDataTreeLeafPreimage {
-                    slot: write.slot,
-                    value: write.value,
-                    next_slot: low_preimage.next_slot,
-                    next_index: low_preimage.next_index,
+                let is_update = low_preimage.slot == write.slot;
+                if is_update {
+                    PublicDataTreeLeafPreimage::empty()
+                } else {
+                    PublicDataTreeLeafPreimage {
+                        slot: write.slot,
+                        value: write.value,
+                        next_slot: low_preimage.next_slot,
+                        next_index: low_preimage.next_index,
+                    }
                 }
             },
         )

noir-projects/noir-protocol-circuits/crates/types/src/data/public_data_tree_leaf.nr

@@ -1,4 +1,4 @@
-use crate::{merkle_tree::leaf_preimage::IndexedTreeLeafValue, traits::Empty};
+use crate::traits::Empty;
 
 pub struct PublicDataTreeLeaf {
     pub slot: Field,
@@ -17,12 +17,6 @@ impl Empty for PublicDataTreeLeaf {
     }
 }
 
-impl IndexedTreeLeafValue for PublicDataTreeLeaf {
-    fn get_key(self) -> Field {
-        self.slot
-    }
-}
-
 impl PublicDataTreeLeaf {
     pub fn is_empty(self) -> bool {
         (self.slot == 0) & (self.value == 0)

noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/indexed_tree.nr

@@ -3,7 +3,6 @@ pub mod check_valid_low_leaf;
 use crate::{
     abis::append_only_tree_snapshot::AppendOnlyTreeSnapshot,
     merkle_tree::{
-        leaf_preimage::{IndexedTreeLeafPreimage, IndexedTreeLeafValue},
         membership::{assert_check_membership, MembershipWitness},
         root::{calculate_empty_tree_root, calculate_subtree_root, root_from_sibling_path},
     },
@@ -113,14 +112,14 @@ pub fn insert<Value, Leaf, let TreeHeight: u32>(
     build_insertion_leaf: fn(Value, Leaf) -> Leaf,
 ) -> AppendOnlyTreeSnapshot
 where
-    Value: IndexedTreeLeafValue,
-    Leaf: IndexedTreeLeafPreimage,
+    Value: Eq + Empty,
+    Leaf: Hash + Empty,
 {
     assert(is_valid_low_leaf(low_leaf_preimage, value), "Invalid low leaf");
 
     // perform membership check for the low leaf against the original root
     assert_check_membership(
-        low_leaf_preimage.as_leaf(),
+        low_leaf_preimage.hash(),
         low_leaf_membership_witness.leaf_index,
         low_leaf_membership_witness.sibling_path,
         snapshot.root,
@@ -130,34 +129,29 @@ where
     let updated_low_leaf =
         update_low_leaf(low_leaf_preimage, value, snapshot.next_available_leaf_index);
 
-    // Update low leaf
     snapshot.root = root_from_sibling_path(
-        updated_low_leaf.as_leaf(),
+        updated_low_leaf.hash(),
         low_leaf_membership_witness.leaf_index,
         low_leaf_membership_witness.sibling_path,
     );
 
-    if low_leaf_preimage.get_key() == value.get_key() {
-        // If it's an update, we don't need to insert the new leaf and advance the tree
-        snapshot
-    } else {
-        let insertion_leaf = build_insertion_leaf(value, low_leaf_preimage);
-        assert_check_membership(
-            0,
-            snapshot.next_available_leaf_index as Field,
-            insertion_sibling_path,
-            snapshot.root,
-        );
-
-        // Calculate the new root
-        snapshot.root = root_from_sibling_path(
-            insertion_leaf.as_leaf(),
-            snapshot.next_available_leaf_index as Field,
-            insertion_sibling_path,
-        );
-
-        snapshot.next_available_leaf_index += 1;
-
-        snapshot
-    }
+    let insertion_leaf = build_insertion_leaf(value, low_leaf_preimage);
+
+    assert_check_membership(
+        0,
+        snapshot.next_available_leaf_index as Field,
+        insertion_sibling_path,
+        snapshot.root,
+    );
+
+    // Calculate the new root
+    snapshot.root = root_from_sibling_path(
+        insertion_leaf.hash(),
+        snapshot.next_available_leaf_index as Field,
+        insertion_sibling_path,
+    );
+
+    snapshot.next_available_leaf_index += 1;
+
+    snapshot
 }

noir-projects/noir-protocol-circuits/crates/types/src/merkle_tree/indexed_tree/check_valid_low_leaf.nr

@@ -16,19 +16,12 @@ mod tests {
         indexed_tree::check_valid_low_leaf::assert_check_valid_low_leaf,
         leaf_preimage::IndexedTreeLeafPreimage,
     };
-    use crate::traits::Empty;
 
     struct TestLeafPreimage {
         value: Field,
         next_value: Field,
     }
 
-    impl Empty for TestLeafPreimage {
-        fn empty() -> Self {
-            Self { value: 0, next_value: 0 }
-        }
-    }
-
     impl IndexedTreeLeafPreimage for TestLeafPreimage {
         fn get_key(self) -> Field {
             self.value