fix: Don't store indices of zero leaves. (#10270)

This PR ensures we don't store indices of zero leaves as this could be
misleading. Requesting the index of a zero leaf is deemed invalid.

It also adds tests that ensure we can unwind blocks of zero leaves

Author: PhilWindle

cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.hpp

@@ -671,6 +671,9 @@ void ContentAddressedAppendOnlyTree<Store, HashingPolicy>::find_leaf_index_from(
     auto job = [=, this]() -> void {
         execute_and_report<FindLeafIndexResponse>(
             [=, this](TypedResponse<FindLeafIndexResponse>& response) {
+                if (leaf == fr::zero()) {
+                    throw std::runtime_error("Requesting indices for zero leaves is prohibited");
+                }
                 ReadTransactionPtr tx = store_->create_read_transaction();
                 RequestContext requestContext;
                 requestContext.includeUncommitted = includeUncommitted;
@@ -703,6 +706,9 @@ void ContentAddressedAppendOnlyTree<Store, HashingPolicy>::find_leaf_index_from(
                 if (blockNumber == 0) {
                     throw std::runtime_error("Unable to find leaf index for block number 0");
                 }
+                if (leaf == fr::zero()) {
+                    throw std::runtime_error("Requesting indices for zero leaves is prohibited");
+                }
                 ReadTransactionPtr tx = store_->create_read_transaction();
                 BlockPayload blockData;
                 if (!store_->get_block_data(blockNumber, blockData, *tx)) {
@@ -909,6 +915,10 @@ void ContentAddressedAppendOnlyTree<Store, HashingPolicy>::add_batch_internal(
     // If we have been told to add these leaves to the index then do so now
     if (update_index) {
         for (uint32_t i = 0; i < number_to_insert; ++i) {
+            // We don't store indices of zero leaves
+            if (hashes_local[i] == fr::zero()) {
+                continue;
+            }
             // std::cout << "Updating index " << index + i << " : " << hashes_local[i] << std::endl;
             store_->update_index(index + i, hashes_local[i]);
         }

cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.test.cpp

@@ -786,6 +786,57 @@ TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_add_multiple_values_in_a
     check_sibling_path(tree, 4 - 1, memdb.get_sibling_path(4 - 1));
 }
 
+TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_pad_with_zero_leaves)
+{
+    constexpr size_t depth = 10;
+    std::string name = random_string();
+    LMDBTreeStore::SharedPtr db = std::make_shared<LMDBTreeStore>(_directory, name, _mapSize, _maxReaders);
+    std::unique_ptr<Store> store = std::make_unique<Store>(name, depth, db);
+    ThreadPoolPtr pool = make_thread_pool(1);
+    TreeType tree(std::move(store), pool);
+    MemoryTree<Poseidon2HashPolicy> memdb(depth);
+
+    std::vector<fr> to_add(32, fr::zero());
+    to_add[0] = VALUES[0];
+
+    for (size_t i = 0; i < 32; ++i) {
+        memdb.update_element(i, to_add[i]);
+    }
+    add_values(tree, to_add);
+    check_size(tree, 32);
+    check_root(tree, memdb.root());
+
+    commit_tree(tree, true);
+
+    check_size(tree, 32);
+    check_root(tree, memdb.root());
+}
+
+TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_not_retrieve_zero_leaf_indices)
+{
+    constexpr size_t depth = 8;
+    std::string name = random_string();
+    LMDBTreeStore::SharedPtr db = std::make_shared<LMDBTreeStore>(_directory, name, _mapSize, _maxReaders);
+    std::unique_ptr<Store> store = std::make_unique<Store>(name, depth, db);
+    ThreadPoolPtr pool = make_thread_pool(1);
+    TreeType tree(std::move(store), pool);
+    MemoryTree<Poseidon2HashPolicy> memdb(depth);
+
+    std::vector<fr> to_add(32, fr::zero());
+    to_add[0] = VALUES[0];
+
+    for (size_t i = 0; i < 32; ++i) {
+        memdb.update_element(i, VALUES[i]);
+    }
+    add_values(tree, to_add);
+    commit_tree(tree);
+    fr leaf = fr::zero();
+    check_find_leaf_index(tree, leaf, 0, false);
+    check_find_historic_leaf_index(tree, 1, leaf, 0, false);
+    check_find_leaf_index_from(tree, leaf, 0, 0, false);
+    check_find_historic_leaf_index_from(tree, 1, leaf, 0, 0, false);
+}
+
 TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_commit_multiple_blocks)
 {
     constexpr size_t depth = 10;
@@ -1354,7 +1405,12 @@ void test_unwind(std::string directory,
         // attempting to unwind a block that is not the tip should fail
         unwind_block(tree, blockNumber + 1, false);
         unwind_block(tree, blockNumber);
-        check_block_and_root_data(db, blockNumber, roots[blockNumber - 1], false);
+
+        // the root should now only exist if there are other blocks with same root
+        const auto last = roots.begin() + long(blockNumber - 1);
+        const auto it =
+            std::find_if(roots.begin(), last, [=](const fr& r) -> bool { return r == roots[blockNumber - 1]; });
+        check_block_and_root_data(db, blockNumber, roots[blockNumber - 1], false, it != last);
 
         const index_t previousValidBlock = blockNumber - 1;
         index_t deletedBlockStartIndex = previousValidBlock * batchSize;
@@ -1384,9 +1440,19 @@ void test_unwind(std::string directory,
 
             const index_t leafIndex = 1;
             check_historic_leaf(tree, historicBlockNumber, values[leafIndex], leafIndex, expectedSuccess);
-            check_find_historic_leaf_index(tree, historicBlockNumber, values[leafIndex], leafIndex, expectedSuccess);
-            check_find_historic_leaf_index_from(
-                tree, historicBlockNumber, values[leafIndex], 0, leafIndex, expectedSuccess);
+
+            // find historic leaves, provided they are not zero leaves
+            check_find_historic_leaf_index(tree,
+                                           historicBlockNumber,
+                                           values[leafIndex],
+                                           leafIndex,
+                                           expectedSuccess && values[leafIndex] != fr::zero());
+            check_find_historic_leaf_index_from(tree,
+                                                historicBlockNumber,
+                                                values[leafIndex],
+                                                0,
+                                                leafIndex,
+                                                expectedSuccess && values[leafIndex] != fr::zero());
         }
     }
 }
@@ -1405,6 +1471,33 @@ TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_unwind_all_blocks)
     test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, 16, 16, 16, second);
 }
 
+TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_unwind_initial_blocks_that_are_empty)
+{
+    const size_t block_size = 16;
+    // First we add 16 blocks worth pf zero leaves and unwind them all
+    std::vector<fr> first(1024, fr::zero());
+    test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, 16, 16, first);
+    // now we add 1 block of zero leaves and the other blocks non-zero leaves and unwind them all
+    std::vector<fr> second = create_values(1024);
+    // set the first 16 values to be zeros
+    for (size_t i = 0; i < block_size; i++) {
+        second[i] = fr::zero();
+    }
+    test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, 16, 16, second);
+
+    // now we add 2 block of zero leaves in the middle and the other blocks non-zero leaves and unwind them all
+    std::vector<fr> third = create_values(1024);
+    size_t offset = block_size * 2;
+    for (size_t i = 0; i < block_size * 2; i++) {
+        third[i + offset] = fr::zero();
+    }
+    test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, 16, 16, third);
+
+    // Now we add a number of regular blocks and unwind
+    std::vector<fr> fourth = create_values(1024);
+    test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, 16, 16, fourth);
+}
+
 TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_sync_and_unwind_large_blocks)
 {
 

cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp

@@ -512,7 +512,6 @@ void ContentAddressedCachedTreeStore<LeafValueType>::put_cached_node_by_index(ui
             return;
         }
     }
-
     nodes_by_index_[level][index] = data;
 }
 
@@ -695,7 +694,6 @@ void ContentAddressedCachedTreeStore<LeafValueType>::persist_leaf_pre_image(cons
     if (leafPreImageIter == leaves_.end()) {
         return;
     }
-    // std::cout << "Persisting leaf preimage " << leafPreImageIter->second << std::endl;
     dataStore_->write_leaf_by_hash(hash, leafPreImageIter->second, tx);
 }
 

cpp/src/barretenberg/world_state/world_state.cpp

@@ -505,7 +505,15 @@ WorldStateStatusFull WorldState::sync_block(const StateReference& block_state_re
 
     {
         auto& wrapper = std::get<TreeWithStore<NullifierTree>>(fork->_trees.at(MerkleTreeId::NULLIFIER_TREE));
-        NullifierTree::AddCompletionCallback completion = [&](const auto&) -> void { signal.signal_decrement(); };
+        NullifierTree::AddCompletionCallback completion = [&](const auto& resp) -> void {
+            // take the first error
+            bool expected = true;
+            if (!resp.success && success.compare_exchange_strong(expected, false)) {
+                err_message = resp.message;
+            }
+
+            signal.signal_decrement();
+        };
         wrapper.tree->add_or_update_values(nullifiers, 0, completion);
     }