From 3476dc352ef40f3cbd30e3b64e9f9d6846eb1183 Mon Sep 17 00:00:00 2001 From: PhilWindle <60546371+PhilWindle@users.noreply.github.com> Date: Thu, 28 Nov 2024 14:22:54 +0000 Subject: [PATCH] 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 --- .../content_addressed_append_only_tree.hpp | 10 ++ ...ontent_addressed_append_only_tree.test.cpp | 101 +++++++++++++++++- .../cached_content_addressed_tree_store.hpp | 2 - .../barretenberg/world_state/world_state.cpp | 10 +- 4 files changed, 116 insertions(+), 7 deletions(-) diff --git a/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.hpp b/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.hpp index f1ceef8ee..7dc41b0ab 100644 --- a/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.hpp +++ b/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.hpp @@ -671,6 +671,9 @@ void ContentAddressedAppendOnlyTree::find_leaf_index_from( auto job = [=, this]() -> void { execute_and_report( [=, this](TypedResponse& 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::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::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]); } diff --git a/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.test.cpp b/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.test.cpp index 52fc12daa..5ac60d919 100644 --- a/cpp/src/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.test.cpp +++ b/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(_directory, name, _mapSize, _maxReaders); + std::unique_ptr store = std::make_unique(name, depth, db); + ThreadPoolPtr pool = make_thread_pool(1); + TreeType tree(std::move(store), pool); + MemoryTree memdb(depth); + + std::vector 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(_directory, name, _mapSize, _maxReaders); + std::unique_ptr store = std::make_unique(name, depth, db); + ThreadPoolPtr pool = make_thread_pool(1); + TreeType tree(std::move(store), pool); + MemoryTree memdb(depth); + + std::vector 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 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 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 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 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) { diff --git a/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp b/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp index 85e5cef2b..013f1cc84 100644 --- a/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp +++ b/cpp/src/barretenberg/crypto/merkle_tree/node_store/cached_content_addressed_tree_store.hpp @@ -512,7 +512,6 @@ void ContentAddressedCachedTreeStore::put_cached_node_by_index(ui return; } } - nodes_by_index_[level][index] = data; } @@ -695,7 +694,6 @@ void ContentAddressedCachedTreeStore::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); } diff --git a/cpp/src/barretenberg/world_state/world_state.cpp b/cpp/src/barretenberg/world_state/world_state.cpp index e2efc72bb..9cb3b36bf 100644 --- a/cpp/src/barretenberg/world_state/world_state.cpp +++ b/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>(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); }