diff --git a/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.cpp b/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.cpp index 09f87cae606..45802caf358 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.cpp +++ b/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.cpp @@ -161,15 +161,7 @@ void LMDBTreeStore::write_block_index_data(const block_number_t& blockNumber, msgpack::unpack((const char*)data.data(), data.size()).get().convert(payload); } - // Double check it's not already present (it shouldn't be) - // We then add the block number and sort - // Sorting shouldn't be necessary as we add blocks in ascending order, but we will make sure - // Sorting here and when we unwind blocks means that looking up the block number for an index becomes O(1) - // These lookups are much more frequent than adds or deletes so we take the hit here - if (!payload.contains(blockNumber)) { - payload.blockNumbers.emplace_back(blockNumber); - payload.sort(); - } + payload.add_block(blockNumber); // Write the new payload back down msgpack::sbuffer buffer; @@ -189,11 +181,11 @@ bool LMDBTreeStore::find_block_for_index(const index_t& index, block_number_t& b } BlockIndexPayload payload; msgpack::unpack((const char*)data.data(), data.size()).get().convert(payload); - if (payload.blockNumbers.empty()) { + if (payload.is_empty()) { return false; } // The block numbers are sorted so we simply return the lowest - blockNumber = payload.blockNumbers[0]; + blockNumber = payload.get_min_block_number(); return true; } @@ -217,7 +209,7 @@ void LMDBTreeStore::delete_block_index(const index_t& sizeAtBlock, payload.delete_block(blockNumber); // if it's now empty, delete it - if (payload.blockNumbers.empty()) { + if (payload.is_empty()) { tx.delete_value(key, *_indexToBlockDatabase); return; } diff --git a/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.hpp b/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.hpp index a28ce245967..c67e13f5130 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.hpp +++ b/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.hpp @@ -1,4 +1,5 @@ #pragma once +#include "barretenberg/common/log.hpp" #include "barretenberg/common/serialize.hpp" #include "barretenberg/crypto/merkle_tree/indexed_tree/indexed_leaf.hpp" #include "barretenberg/crypto/merkle_tree/lmdb_store/callbacks.hpp" @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -63,39 +65,90 @@ struct BlockIndexPayload { bool operator==(const BlockIndexPayload& other) const { return blockNumbers == other.blockNumbers; } - void sort() { std::sort(blockNumbers.begin(), blockNumbers.end()); } + bool is_empty() const { return blockNumbers.empty(); } + + block_number_t get_min_block_number() { return blockNumbers[0]; } bool contains(const block_number_t& blockNumber) { - auto it = std::lower_bound(blockNumbers.begin(), blockNumbers.end(), blockNumber); - if (it == blockNumbers.end()) { - // The block was not found, we can return + if (is_empty()) { return false; } - return *it == blockNumber; + if (blockNumbers.size() == 1) { + return blockNumbers[0] == blockNumber; + } + return blockNumber >= blockNumbers[0] && blockNumber <= blockNumbers[1]; } void delete_block(const block_number_t& blockNumber) { + // Shouldn't be possible, but no need to do anything here if (blockNumbers.empty()) { return; } - // shuffle the block number down, removing the one we want to remove and then pop the end item - auto it = std::lower_bound(blockNumbers.begin(), blockNumbers.end(), blockNumber); - if (it == blockNumbers.end()) { - // The block was not found, we can return + + // If the size is 1, the blocknumber must match that in index 0, if it does remove it + if (blockNumbers.size() == 1) { + if (blockNumbers[0] == blockNumber) { + blockNumbers.pop_back(); + } return; } - // It could be a block higher than the one we are looking for - if (*it != blockNumber) { + + // we have 2 entries, we must verify that the block number is equal to the item in index 1 + if (blockNumbers[1] != blockNumber) { + throw std::runtime_error(format("Unable to delete block number ", + blockNumber, + " for retrieval by index, current max block number at that index: ", + blockNumbers[1])); + } + // It is equal, decrement it, we know that the new block number must have been added previously + --blockNumbers[1]; + + // We have modified the high block. If it is now equal to the low block then pop it + if (blockNumbers[0] == blockNumbers[1]) { + blockNumbers.pop_back(); + } + } + + void add_block(const block_number_t& blockNumber) + { + // If empty, just add the block number + if (blockNumbers.empty()) { + blockNumbers.emplace_back(blockNumber); + return; + } + + // If the size is 1, then we must be adding the block 1 larger than that in index 0 + if (blockNumbers.size() == 1) { + if (blockNumber != blockNumbers[0] + 1) { + // We can't accept a block number for this index that does not immediately follow the block before + throw std::runtime_error(format("Unable to store block number ", + blockNumber, + " for retrieval by index, current max block number at that index: ", + blockNumbers[0])); + } + blockNumbers.emplace_back(blockNumber); return; } - // we have found our block, shuffle blocks after this one down - auto readIt = it + 1; - while (readIt != blockNumbers.end()) { - *it++ = *readIt++; + + // Size must be 2 here, if larger, this is an error + if (blockNumbers.size() != 2) { + throw std::runtime_error(format("Unable to store block number ", + blockNumber, + " for retrieval by index, block numbers is of invalid size: ", + blockNumbers.size())); + } + + // If the size is 2, then we must be adding the block 1 larger than that in index 1 + if (blockNumber != blockNumbers[1] + 1) { + // We can't accept a block number for this index that does not immediately follow the block before + throw std::runtime_error(format("Unable to store block number ", + blockNumber, + " for retrieval by index, current max block number at that index: ", + blockNumbers[1])); } - blockNumbers.pop_back(); + blockNumbers[1] = blockNumber; } }; /** diff --git a/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.test.cpp b/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.test.cpp index e9b85aa5bbc..c33eb42bc23 100644 --- a/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.test.cpp +++ b/barretenberg/cpp/src/barretenberg/crypto/merkle_tree/lmdb_store/lmdb_tree_store.test.cpp @@ -273,11 +273,10 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_by_index) BlockAndIndex{ .blockNumber = 5, .index = 130 } }; LMDBTreeStore store(_directory, "DB1", _mapSize, _maxReaders); { - // write all of the blocks. we will write them in reverse order + // write all of the blocks. LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); - for (int i = int(blocks.size()) - 1; i >= 0; i--) { + for (auto block : blocks) { // the arg is block size so add 1 - const BlockAndIndex& block = blocks[size_t(i)]; store.write_block_index_data(block.blockNumber, block.index + 1, *transaction); } transaction->commit(); @@ -409,14 +408,24 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in { // write all of the blocks. we will write them in reverse order LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); - for (int i = int(blocks.size()) - 1; i >= 0; i--) { + for (auto block : blocks) { // the arg is block size so add 1 - const BlockAndIndex& block = blocks[size_t(i)]; store.write_block_index_data(block.blockNumber, block.index + 1, *transaction); } transaction->commit(); } + { + // we can't add a duplicate block at an index if it is not the next block number + LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); + // the arg is block size so add 1 + EXPECT_THROW(store.write_block_index_data(3, 60 + 1, *transaction), std::runtime_error); + EXPECT_THROW(store.write_block_index_data(6, 60 + 1, *transaction), std::runtime_error); + EXPECT_THROW(store.write_block_index_data(1, 25 + 1, *transaction), std::runtime_error); + EXPECT_THROW(store.write_block_index_data(3, 25 + 1, *transaction), std::runtime_error); + transaction->abort(); + } + { // read back some blocks and check them LMDBTreeReadTransaction::Ptr transaction = store.create_read_transaction(); @@ -435,11 +444,12 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in } { - // delete block 2 at index 60 + // attempting to delete block 2 at index 60 should fail as it is not the last block in the series at index 60 LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); // the arg is block size so add 1 - store.delete_block_index(blocks[1].index + 1, blocks[1].blockNumber, *transaction); - transaction->commit(); + EXPECT_THROW(store.delete_block_index(blocks[1].index + 1, blocks[1].blockNumber, *transaction), + std::runtime_error); + transaction->abort(); } { @@ -449,9 +459,9 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in EXPECT_TRUE(store.find_block_for_index(5, readBack, *transaction)); EXPECT_EQ(readBack, 1); - // should be the new lowest block at this index + // should still be the lowest block at this index EXPECT_TRUE(store.find_block_for_index(30, readBack, *transaction)); - EXPECT_EQ(readBack, 3); + EXPECT_EQ(readBack, 2); EXPECT_TRUE(store.find_block_for_index(82, readBack, *transaction)); EXPECT_EQ(readBack, 5); @@ -463,9 +473,9 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in // try and delete blocks that don't exist at index 60 LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); // the arg is block size so add 1 - store.delete_block_index(blocks[1].index + 1, 2, *transaction); - store.delete_block_index(blocks[1].index + 1, 5, *transaction); - transaction->commit(); + EXPECT_THROW(store.delete_block_index(blocks[1].index + 1, 2, *transaction), std::runtime_error); + EXPECT_THROW(store.delete_block_index(blocks[1].index + 1, 5, *transaction), std::runtime_error); + transaction->abort(); } { @@ -475,9 +485,8 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in EXPECT_TRUE(store.find_block_for_index(5, readBack, *transaction)); EXPECT_EQ(readBack, 1); - // should be the new lowest block at this index EXPECT_TRUE(store.find_block_for_index(30, readBack, *transaction)); - EXPECT_EQ(readBack, 3); + EXPECT_EQ(readBack, 2); EXPECT_TRUE(store.find_block_for_index(82, readBack, *transaction)); EXPECT_EQ(readBack, 5); @@ -486,7 +495,7 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in } { - // delete 2 more blocks + // delete the last 2 blocks at index 60 LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); // the arg is block size so add 1 store.delete_block_index(blocks[3].index + 1, blocks[3].blockNumber, *transaction); @@ -494,6 +503,32 @@ TEST_F(LMDBTreeStoreTest, can_write_and_retrieve_block_numbers_with_duplicate_in transaction->commit(); } + { + // check the blocks again + LMDBTreeReadTransaction::Ptr transaction = store.create_read_transaction(); + block_number_t readBack = 0; + EXPECT_TRUE(store.find_block_for_index(5, readBack, *transaction)); + EXPECT_EQ(readBack, 1); + + EXPECT_TRUE(store.find_block_for_index(30, readBack, *transaction)); + EXPECT_EQ(readBack, 2); + + EXPECT_TRUE(store.find_block_for_index(82, readBack, *transaction)); + EXPECT_EQ(readBack, 5); + } + + { + // delete the last final block at index 60 + LMDBTreeWriteTransaction::Ptr transaction = store.create_write_transaction(); + // the arg is block size so add 1 + // Only one block remains at index 60, try and delete one that doesn't exist, it should do nothing + store.delete_block_index(blocks[3].index + 1, blocks[3].blockNumber, *transaction); + + // Now delete the last block + store.delete_block_index(blocks[1].index + 1, blocks[1].blockNumber, *transaction); + transaction->commit(); + } + { // check the blocks again LMDBTreeReadTransaction::Ptr transaction = store.create_read_transaction();