From a3fba8442fdd62f429054c3367984fd4206bbbeb Mon Sep 17 00:00:00 2001 From: PhilWindle <60546371+PhilWindle@users.noreply.github.com> Date: Tue, 17 Dec 2024 19:20:05 +0000 Subject: [PATCH] feat: Don't store every block number in block indices DB (#10658) The block indices DB is what enables clients to query the block number for a given index. Where there were blocks of zero size (so multiple block numbers against an index) it was storing every block number against that index. This could result in an unbounded values size in the DB. We now just store the block range in this scenario. --- .../lmdb_store/lmdb_tree_store.cpp | 16 +--- .../lmdb_store/lmdb_tree_store.hpp | 85 +++++++++++++++---- .../lmdb_store/lmdb_tree_store.test.cpp | 67 +++++++++++---- 3 files changed, 124 insertions(+), 44 deletions(-) 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();