Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Empty blocks can now be unwound #11920

Merged
merged 6 commits into from
Feb 12, 2025
Merged

fix: Empty blocks can now be unwound #11920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe90f3f
Some refactoring and don't remove nodes if size == 0
PhilWindle Feb 11, 2025
2850e96
Comments
PhilWindle Feb 11, 2025
8a253ed
Merge branch 'master' into pw/unwind-empty-blocks-2
PhilWindle Feb 11, 2025
aace5b3
Review changes
PhilWindle Feb 11, 2025
83a028f
Re-enabled test
PhilWindle Feb 12, 2025
e06057d
Merge branch 'master' into pw/unwind-empty-blocks-2
PhilWindle Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 20 additions & 25 deletions ...c/barretenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ template <typename Store, typename HashingPolicy> class ContentAddressedAppendOn
// Only construct from provided store and thread pool, no copies or moves
ContentAddressedAppendOnlyTree(std::unique_ptr<Store> store,
std::shared_ptr<ThreadPool> workers,
const std::vector<fr>& initial_values = {});
const std::vector<fr>& initial_values = {},
bool commit_genesis_state = true);
ContentAddressedAppendOnlyTree(ContentAddressedAppendOnlyTree const& other) = delete;
ContentAddressedAppendOnlyTree(ContentAddressedAppendOnlyTree&& other) = delete;
ContentAddressedAppendOnlyTree& operator=(ContentAddressedAppendOnlyTree const& other) = delete;
Expand Down Expand Up @@ -263,16 +264,16 @@ template <typename Store, typename HashingPolicy> class ContentAddressedAppendOn

template <typename Store, typename HashingPolicy>
ContentAddressedAppendOnlyTree<Store, HashingPolicy>::ContentAddressedAppendOnlyTree(
std::unique_ptr<Store> store, std::shared_ptr<ThreadPool> workers, const std::vector<fr>& initial_values)
std::unique_ptr<Store> store,
std::shared_ptr<ThreadPool> workers,
const std::vector<fr>& initial_values,
bool commit_genesis_state)
: store_(std::move(store))
, workers_(workers)
{
TreeMeta meta;
{
// start by reading the meta data from the backing store
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, true);
}
// start by reading the meta data from the backing store
store_->get_meta(meta);
depth_ = meta.depth;
zero_hashes_.resize(depth_ + 1);

Expand All @@ -292,15 +293,10 @@ ContentAddressedAppendOnlyTree<Store, HashingPolicy>::ContentAddressedAppendOnly
return;
}

// if the tree is empty then we want to write some initial state
meta.initialRoot = meta.root = current;
meta.initialSize = meta.size = 0;
store_->put_meta(meta);
TreeDBStats stats;
store_->commit(meta, stats, false);

// if we were given initial values to insert then we do that now
if (!initial_values.empty()) {
if (initial_values.empty()) {
meta.initialRoot = meta.root = current;
meta.initialSize = meta.size = 0;
} else {
Signal signal(1);
TypedResponse<AddDataResponse> result;
add_values(initial_values, [&](const TypedResponse<AddDataResponse>& resp) {
Expand All @@ -313,16 +309,15 @@ ContentAddressedAppendOnlyTree<Store, HashingPolicy>::ContentAddressedAppendOnly
throw std::runtime_error(format("Failed to initialise tree: ", result.message));
}

{
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, true);
}
store_->get_meta(meta);

meta.initialRoot = meta.root = result.inner.root;
meta.initialSize = meta.size = result.inner.size;
}
store_->put_meta(meta);

store_->put_meta(meta);
store_->commit(meta, stats, false);
if (commit_genesis_state) {
store_->commit_genesis_state();
}
}

Expand Down Expand Up @@ -834,7 +829,7 @@ void ContentAddressedAppendOnlyTree<Store, HashingPolicy>::commit(const CommitCa
auto job = [=, this]() {
execute_and_report<CommitResponse>(
[=, this](TypedResponse<CommitResponse>& response) {
store_->commit(response.inner.meta, response.inner.stats);
store_->commit_block(response.inner.meta, response.inner.stats);
},
on_completion);
};
Expand Down Expand Up @@ -927,7 +922,7 @@ void ContentAddressedAppendOnlyTree<Store, HashingPolicy>::add_values_internal(s
{
ReadTransactionPtr tx = store_->create_read_transaction();
TreeMeta meta;
store_->get_meta(meta, *tx, true);
store_->get_meta(meta);
index_t sizeToAppend = values->size();
new_size = meta.size;
index_t batchIndex = 0;
Expand All @@ -952,7 +947,7 @@ void ContentAddressedAppendOnlyTree<Store, HashingPolicy>::add_batch_internal(
auto number_to_insert = static_cast<uint32_t>(hashes_local.size());

TreeMeta meta;
store_->get_meta(meta, tx, true);
store_->get_meta(meta);
index_t index = meta.size;
new_size = meta.size + number_to_insert;

Expand Down
138 changes: 131 additions & 7 deletions ...retenberg/crypto/merkle_tree/append_only_tree/content_addressed_append_only_tree.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,10 +425,9 @@ TEST_F(PersistedContentAddressedAppendOnlyTreeTest, errors_are_caught_and_handle
std::string name = random_string();
std::string directory = random_temp_directory();
std::filesystem::create_directories(directory);
auto& random_engine = numeric::get_randomness();

{
LMDBTreeStore::SharedPtr db = std::make_shared<LMDBTreeStore>(_directory, name, 50, _maxReaders);
LMDBTreeStore::SharedPtr db = std::make_shared<LMDBTreeStore>(_directory, name, 100, _maxReaders);
std::unique_ptr<Store> store = std::make_unique<Store>(name, depth, db);

ThreadPoolPtr pool = make_thread_pool(1);
Expand Down Expand Up @@ -1423,31 +1422,32 @@ 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)
TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_unwind_initial_blocks_that_are_full_of_zeros)
{
const size_t block_size = 16;
const uint32_t num_blocks = 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);
test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, num_blocks, num_blocks, 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);
test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, num_blocks, num_blocks, 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);
test_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, num_blocks, num_blocks, 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_unwind(_directory, "DB", _mapSize, _maxReaders, 10, block_size, num_blocks, num_blocks, fourth);
}

TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_sync_and_unwind_large_blocks)
Expand All @@ -1465,6 +1465,130 @@ TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_sync_and_unwind_large_bl
}
}

TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_commit_and_unwind_empty_blocks)
{
std::string name = random_string();
constexpr uint32_t depth = 10;
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);

// The first path stored is the genesis state. This effectively makes everything 1 based.
std::vector<fr_sibling_path> paths(1, memdb.get_sibling_path(0));
index_t blockNumber = 0;
uint32_t batchSize = 64;

std::vector<std::vector<fr>> values;
// commit an empty block
values.emplace_back();
// and another one
values.emplace_back();
// then a non-empty block
values.push_back(create_values(batchSize));
// then 2 more empty blocks
values.emplace_back();
values.emplace_back();
// then a non-empty block
values.push_back(create_values(batchSize));

index_t index = 0;

for (auto& blockValues : values) {
add_values(tree, blockValues);
commit_tree(tree);

for (const auto& blockValue : blockValues) {
memdb.update_element(index++, blockValue);
}

++blockNumber;

paths.push_back(memdb.get_sibling_path(0));

check_sibling_path(tree, 0, memdb.get_sibling_path(0));
check_historic_sibling_path(tree, 0, paths[1], 1);
check_block_height(tree, blockNumber);
}

while (blockNumber > 0) {
// Unwind the next block
unwind_block(tree, blockNumber);

--blockNumber;

check_sibling_path(tree, 0, paths[blockNumber]);

if (blockNumber > 0) {
check_historic_sibling_path(tree, 0, paths[1], 1);
check_block_height(tree, blockNumber);
}
}
}

TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_commit_and_remove_historic_empty_blocks)
{
std::string name = random_string();
constexpr uint32_t depth = 10;
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);

// The first path stored is the genesis state. This effectively makes everything 1 based.
std::vector<fr_sibling_path> paths(1, memdb.get_sibling_path(0));
index_t blockNumber = 0;
uint32_t batchSize = 64;

std::vector<std::vector<fr>> values;
// commit an empty block
values.emplace_back();
// and another one
values.emplace_back();
// then a non-empty block
values.push_back(create_values(batchSize));
// then 2 more empty blocks
values.emplace_back();
values.emplace_back();
// then a non-empty block
values.push_back(create_values(batchSize));

index_t index = 0;

for (auto& blockValues : values) {
add_values(tree, blockValues);
commit_tree(tree);

for (const auto& blockValue : blockValues) {
memdb.update_element(index++, blockValue);
}

++blockNumber;

paths.push_back(memdb.get_sibling_path(0));

check_sibling_path(tree, 0, memdb.get_sibling_path(0));
check_historic_sibling_path(tree, 0, paths[1], 1);
check_block_height(tree, blockNumber);
}

index_t blockToRemove = 1;

while (blockToRemove < blockNumber) {
finalise_block(tree, blockToRemove + 1);
// Remove the historic next block
remove_historic_block(tree, blockToRemove);

check_sibling_path(tree, 0, paths[blockNumber]);

check_historic_sibling_path(tree, 0, paths[blockToRemove + 1], blockToRemove + 1);
check_block_height(tree, blockNumber);
blockToRemove++;
}
}

TEST_F(PersistedContentAddressedAppendOnlyTreeTest, can_retrieve_block_numbers_by_index)
{
std::string name = random_string();
Expand Down
21 changes: 7 additions & 14 deletions ...g/cpp/src/barretenberg/crypto/merkle_tree/indexed_tree/content_addressed_indexed_tree.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ template <typename Store, typename HashingPolicy>
ContentAddressedIndexedTree<Store, HashingPolicy>::ContentAddressedIndexedTree(std::unique_ptr<Store> store,
std::shared_ptr<ThreadPool> workers,
const index_t& initial_size)
: ContentAddressedAppendOnlyTree<Store, HashingPolicy>(std::move(store), workers)
: ContentAddressedAppendOnlyTree<Store, HashingPolicy>(std::move(store), workers, {}, false)
{
if (initial_size < 2) {
throw std::runtime_error("Indexed trees must have initial size > 1");
Expand All @@ -284,10 +284,7 @@ ContentAddressedIndexedTree<Store, HashingPolicy>::ContentAddressedIndexedTree(s
zero_hashes_[0] = current;

TreeMeta meta;
{
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, false);
}
store_->get_meta(meta);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will see this in a few places. I got rid of the need to pass a transaction just to read uncommitted meta data. The transaction was unused.


// if the tree already contains leaves then it's been initialised in the past
if (meta.size > 0) {
Expand Down Expand Up @@ -321,15 +318,11 @@ ContentAddressedIndexedTree<Store, HashingPolicy>::ContentAddressedIndexedTree(s
if (!result.success) {
throw std::runtime_error(format("Failed to initialise tree: ", result.message));
}
{
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, true);
}
store_->get_meta(meta);
meta.initialRoot = result.inner.root;
meta.initialSize = result.inner.size;
store_->put_meta(meta);
TreeDBStats stats;
store_->commit(meta, stats, false);
store_->commit_genesis_state();
}

template <typename Store, typename HashingPolicy>
Expand Down Expand Up @@ -918,7 +911,7 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::generate_insertions(
{
ReadTransactionPtr tx = store_->create_read_transaction();
TreeMeta meta;
store_->get_meta(meta, *tx, true);
store_->get_meta(meta);
RequestContext requestContext;
requestContext.includeUncommitted = true;
// Ensure that the tree is not going to be overfilled
Expand Down Expand Up @@ -1326,7 +1319,7 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::add_or_update_values_seq
{
TreeMeta meta;
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, true);
store_->get_meta(meta);

index_t new_total_size = results->appended_leaves + meta.size;
meta.size = new_total_size;
Expand Down Expand Up @@ -1417,7 +1410,7 @@ void ContentAddressedIndexedTree<Store, HashingPolicy>::generate_sequential_inse
[=, this](TypedResponse<SequentialInsertionGenerationResponse>& response) {
TreeMeta meta;
ReadTransactionPtr tx = store_->create_read_transaction();
store_->get_meta(meta, *tx, true);
store_->get_meta(meta);

RequestContext requestContext;
requestContext.includeUncommitted = true;
Expand Down
Loading