From 97868ed2ae7d541b17ad231964683d117260f349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Fri, 15 Nov 2024 18:59:32 +0100 Subject: [PATCH 1/3] Rep crawler special handling of unit tests --- nano/node/repcrawler.cpp | 20 +++++--------------- nano/node/repcrawler.hpp | 2 +- nano/secure/ledger.cpp | 4 ++-- nano/secure/ledger.hpp | 2 +- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index b54eae4a..e8caeb29 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -269,13 +269,13 @@ std::vector> nano::rep_crawler::prepar return { random_peers.begin (), random_peers.end () }; } -auto nano::rep_crawler::prepare_query_target () -> std::optional +auto nano::rep_crawler::prepare_query_target () const -> std::optional { constexpr int max_attempts = 4; auto transaction = node.ledger.tx_begin_read (); - std::optional> hash_root; + std::optional> hash_root; // Randomly select a block from ledger to request votes for for (auto i = 0; i < max_attempts && !hash_root; ++i) @@ -289,20 +289,10 @@ auto nano::rep_crawler::prepare_query_target () -> std::optional } } - if (!hash_root) + // Special case for dev network where number of blocks might be very low: if we can't find a block to query, just pick genesis + if (node.network_params.network.is_dev_network () && !hash_root) { - return std::nullopt; - } - - // Don't send same block multiple times in tests - if (node.network_params.network.is_dev_network ()) - { - nano::lock_guard lock{ mutex }; - - for (auto i = 0; queries.get ().count (hash_root->first) != 0 && i < max_attempts; ++i) - { - hash_root = node.ledger.hash_root_random (transaction); - } + hash_root = std::make_pair (node.network_params.ledger.genesis->hash (), node.network_params.ledger.genesis->root ()); } return hash_root; diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index cd9d6800..d99bae55 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -107,7 +107,7 @@ private: /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ std::vector> prepare_crawl_targets (bool sufficient_weight) const; - std::optional prepare_query_target (); + std::optional prepare_query_target () const; bool track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel); private: diff --git a/nano/secure/ledger.cpp b/nano/secure/ledger.cpp index d6447418..ca6fb91a 100644 --- a/nano/secure/ledger.cpp +++ b/nano/secure/ledger.cpp @@ -936,7 +936,7 @@ std::string nano::ledger::block_text (nano::block_hash const & hash_a) return result; } -std::pair nano::ledger::hash_root_random (secure::transaction const & transaction_a) const +std::pair nano::ledger::hash_root_random (secure::transaction const & transaction_a) const { nano::block_hash hash (0); nano::root root (0); @@ -962,7 +962,7 @@ std::pair nano::ledger::hash_root_random (se root = block->root (); } } - return std::make_pair (hash, root.as_block_hash ()); + return std::make_pair (hash, root); } // Vote weight of an account diff --git a/nano/secure/ledger.hpp b/nano/secure/ledger.hpp index 3171e459..9bb67878 100644 --- a/nano/secure/ledger.hpp +++ b/nano/secure/ledger.hpp @@ -58,7 +58,7 @@ public: nano::block_hash representative_calculated (secure::transaction const &, nano::block_hash const &); std::string block_text (char const *); std::string block_text (nano::block_hash const &); - std::pair hash_root_random (secure::transaction const &) const; + std::pair hash_root_random (secure::transaction const &) const; std::optional pending_info (secure::transaction const &, nano::pending_key const & key) const; std::deque> confirm (secure::write_transaction &, nano::block_hash const & hash, size_t max_blocks = 1024 * 128); nano::block_status process (secure::write_transaction const &, std::shared_ptr block); From dc3ff664dafaff3b512967c70f35b3f8d8dc5448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 16 Nov 2024 23:48:19 +0100 Subject: [PATCH 2/3] Rewrite ledger random block sampling --- nano/core_test/ledger.cpp | 53 ++++++++++++++++++++++++++++----------- nano/node/repcrawler.cpp | 23 +++++------------ nano/secure/ledger.cpp | 36 +++++++++++--------------- nano/secure/ledger.hpp | 2 +- 4 files changed, 59 insertions(+), 55 deletions(-) diff --git a/nano/core_test/ledger.cpp b/nano/core_test/ledger.cpp index 32658ba7..7693af46 100644 --- a/nano/core_test/ledger.cpp +++ b/nano/core_test/ledger.cpp @@ -5351,7 +5351,7 @@ TEST (ledger, pruning_safe_functions) ASSERT_EQ (nano::dev::genesis_key.pub, ledger.any.block_account (transaction, send2->hash ()).value ()); } -TEST (ledger, hash_root_random) +TEST (ledger, random_blocks) { nano::logger logger; auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants); @@ -5394,23 +5394,46 @@ TEST (ledger, hash_root_random) ASSERT_TRUE (store->pruned.exists (transaction, send1->hash ())); ASSERT_TRUE (ledger.any.block_exists (transaction, nano::dev::genesis->hash ())); ASSERT_TRUE (ledger.any.block_exists (transaction, send2->hash ())); - // Test random block including pruned - bool done (false); - auto iteration (0); - while (!done) + // Prunned block will not be included in the random selection because it's not in the blocks set { - ++iteration; - auto root_hash (ledger.hash_root_random (transaction)); - done = (root_hash.first == send1->hash ()) && (root_hash.second.is_zero ()); - ASSERT_LE (iteration, 1000); + bool done = false; + size_t iteration = 0; + while (!done && iteration < 42) + { + ++iteration; + auto blocks = ledger.random_blocks (transaction, 10); + ASSERT_EQ (blocks.size (), 10); // Random blocks should repeat if the ledger is smaller than the requested count + auto first = blocks.front (); + done = (first->hash () == send1->hash ()); + } + ASSERT_FALSE (done); } - done = false; - while (!done) + // Genesis and send2 should be included in the random selection { - ++iteration; - auto root_hash (ledger.hash_root_random (transaction)); - done = (root_hash.first == send2->hash ()) && (root_hash.second == send2->root ().as_block_hash ()); - ASSERT_LE (iteration, 1000); + bool done = false; + size_t iteration = 0; + while (!done) + { + ++iteration; + auto blocks = ledger.random_blocks (transaction, 1); + ASSERT_EQ (blocks.size (), 1); + auto first = blocks.front (); + done = (first->hash () == send2->hash ()); + ASSERT_LE (iteration, 1000); + } + } + { + bool done = false; + size_t iteration = 0; + while (!done) + { + ++iteration; + auto blocks = ledger.random_blocks (transaction, 1); + ASSERT_EQ (blocks.size (), 1); + auto first = blocks.front (); + done = (first->hash () == nano::dev::genesis->hash ()); + ASSERT_LE (iteration, 1000); + } } } diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index e8caeb29..5c0aa9de 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -271,31 +271,20 @@ std::vector> nano::rep_crawler::prepar auto nano::rep_crawler::prepare_query_target () const -> std::optional { - constexpr int max_attempts = 4; + constexpr int max_attempts = 10; auto transaction = node.ledger.tx_begin_read (); - std::optional> hash_root; - - // Randomly select a block from ledger to request votes for - for (auto i = 0; i < max_attempts && !hash_root; ++i) + auto random_blocks = node.ledger.random_blocks (transaction, max_attempts); + for (auto const & block : random_blocks) { - hash_root = node.ledger.hash_root_random (transaction); - - // Rebroadcasted votes for recently confirmed blocks might confuse the rep crawler - if (active.recently_confirmed.exists (hash_root->first)) + if (!active.recently_confirmed.exists (block->hash ())) { - hash_root = std::nullopt; + return std::make_pair (block->hash (), block->root ()); } } - // Special case for dev network where number of blocks might be very low: if we can't find a block to query, just pick genesis - if (node.network_params.network.is_dev_network () && !hash_root) - { - hash_root = std::make_pair (node.network_params.ledger.genesis->hash (), node.network_params.ledger.genesis->root ()); - } - - return hash_root; + return std::nullopt; } bool nano::rep_crawler::track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel) diff --git a/nano/secure/ledger.cpp b/nano/secure/ledger.cpp index ca6fb91a..46fe0f62 100644 --- a/nano/secure/ledger.cpp +++ b/nano/secure/ledger.cpp @@ -936,33 +936,25 @@ std::string nano::ledger::block_text (nano::block_hash const & hash_a) return result; } -std::pair nano::ledger::hash_root_random (secure::transaction const & transaction_a) const +std::deque> nano::ledger::random_blocks (secure::transaction const & transaction, size_t count) const { - nano::block_hash hash (0); - nano::root root (0); - if (!pruning) + std::deque> result; + + auto const starting_hash = nano::random_pool::generate (); + + // It is more efficient to choose a random starting point and pick a few sequential blocks from there + auto it = store.block.begin (transaction, starting_hash); + auto const end = store.block.end (transaction); + while (result.size () < count) { - auto block (store.block.random (transaction_a)); - hash = block->hash (); - root = block->root (); - } - else - { - uint64_t count (cache.block_count); - auto region = nano::random_pool::generate_word64 (0, count - 1); - // Pruned cache cannot guarantee that pruned blocks are already commited - if (region < cache.pruned_count) + if (it != end) { - hash = store.pruned.random (transaction_a); - } - if (hash.is_zero ()) - { - auto block (store.block.random (transaction_a)); - hash = block->hash (); - root = block->root (); + result.push_back (it->second.block); } + ++it; // Store iterators wrap around when reaching the end } - return std::make_pair (hash, root); + + return result; } // Vote weight of an account diff --git a/nano/secure/ledger.hpp b/nano/secure/ledger.hpp index 9bb67878..69c86378 100644 --- a/nano/secure/ledger.hpp +++ b/nano/secure/ledger.hpp @@ -58,7 +58,7 @@ public: nano::block_hash representative_calculated (secure::transaction const &, nano::block_hash const &); std::string block_text (char const *); std::string block_text (nano::block_hash const &); - std::pair hash_root_random (secure::transaction const &) const; + std::deque> random_blocks (secure::transaction const &, size_t count) const; std::optional pending_info (secure::transaction const &, nano::pending_key const & key) const; std::deque> confirm (secure::write_transaction &, nano::block_hash const & hash, size_t max_blocks = 1024 * 128); nano::block_status process (secure::write_transaction const &, std::shared_ptr block); From 4ad3629218a21327ba0d2fcde4366346e7d90ae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sun, 17 Nov 2024 00:06:58 +0100 Subject: [PATCH 3/3] Remove unused block store functions --- nano/core_test/block_store.cpp | 15 --------------- nano/secure/ledger.cpp | 34 ++++++++++++++++++++-------------- nano/store/block.hpp | 1 - nano/store/lmdb/block.cpp | 13 ------------- nano/store/lmdb/block.hpp | 1 - nano/store/rocksdb/block.cpp | 12 ------------ nano/store/rocksdb/block.hpp | 1 - 7 files changed, 20 insertions(+), 57 deletions(-) diff --git a/nano/core_test/block_store.cpp b/nano/core_test/block_store.cpp index e4294663..c8033103 100644 --- a/nano/core_test/block_store.cpp +++ b/nano/core_test/block_store.cpp @@ -904,21 +904,6 @@ TEST (block_store, cemented_count_cache) ASSERT_EQ (1, ledger.cemented_count ()); } -TEST (block_store, block_random) -{ - nano::logger logger; - auto store = nano::make_store (logger, nano::unique_path (), nano::dev::constants); - { - nano::ledger_cache ledger_cache{ store->rep_weight }; - auto transaction (store->tx_begin_write ()); - store->initialize (transaction, ledger_cache, nano::dev::constants); - } - auto transaction (store->tx_begin_read ()); - auto block (store->block.random (transaction)); - ASSERT_NE (nullptr, block); - ASSERT_EQ (*block, *nano::dev::genesis); -} - TEST (block_store, pruned_random) { nano::logger logger; diff --git a/nano/secure/ledger.cpp b/nano/secure/ledger.cpp index 46fe0f62..23550e74 100644 --- a/nano/secure/ledger.cpp +++ b/nano/secure/ledger.cpp @@ -1276,7 +1276,7 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p if (std::filesystem::exists (rockdb_data_path)) { - logger.error (nano::log::type::ledger, "Existing RocksDb folder found in '{}'. Please remove it and try again.", rockdb_data_path.string ()); + logger.error (nano::log::type::ledger, "Existing RocksDB folder found in '{}'. Please remove it and try again.", rockdb_data_path.string ()); return true; } @@ -1423,7 +1423,8 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p logger.info (nano::log::type::ledger, "{} entries converted ({}%)", count.load (), table_size > 0 ? count.load () * 100 / table_size : 100); logger.info (nano::log::type::ledger, "Finalizing migration..."); - auto lmdb_transaction (store.tx_begin_read ()); + + auto lmdb_transaction (tx_begin_read ()); auto version = store.version.get (lmdb_transaction); auto rocksdb_transaction (rocksdb_store->tx_begin_write ()); rocksdb_store->version.put (rocksdb_transaction, version); @@ -1447,21 +1448,26 @@ bool nano::ledger::migrate_lmdb_to_rocksdb (std::filesystem::path const & data_p error |= store.version.get (lmdb_transaction) != rocksdb_store->version.get (rocksdb_transaction); // For large tables a random key is used instead and makes sure it exists - auto random_block (store.block.random (lmdb_transaction)); - error |= rocksdb_store->block.get (rocksdb_transaction, random_block->hash ()) == nullptr; - - auto account = random_block->account (); - nano::account_info account_info; - error |= rocksdb_store->account.get (rocksdb_transaction, account, account_info); - - // If confirmation height exists in the lmdb ledger for this account it should exist in the rocksdb ledger - nano::confirmation_height_info confirmation_height_info{}; - if (!store.confirmation_height.get (lmdb_transaction, account, confirmation_height_info)) + auto blocks = random_blocks (lmdb_transaction, 42); + release_assert (!blocks.empty ()); + for (auto const & block : blocks) { - error |= rocksdb_store->confirmation_height.get (rocksdb_transaction, account, confirmation_height_info); + auto const account = block->account (); + + error |= rocksdb_store->block.get (rocksdb_transaction, block->hash ()) == nullptr; + + nano::account_info account_info; + error |= rocksdb_store->account.get (rocksdb_transaction, account, account_info); + + // If confirmation height exists in the lmdb ledger for this account it should exist in the rocksdb ledger + nano::confirmation_height_info confirmation_height_info{}; + if (!store.confirmation_height.get (lmdb_transaction, account, confirmation_height_info)) + { + error |= rocksdb_store->confirmation_height.get (rocksdb_transaction, account, confirmation_height_info); + } } - logger.info (nano::log::type::ledger, "Migration completed. Make sure to enable RocksDb in the config file under [node.rocksdb]"); + logger.info (nano::log::type::ledger, "Migration completed. Make sure to enable RocksDB in the config file under [node.rocksdb]"); logger.info (nano::log::type::ledger, "After confirming correct node operation, the data.ldb file can be deleted if no longer required"); } else diff --git a/nano/store/block.hpp b/nano/store/block.hpp index 10cda6d2..13a7b18b 100644 --- a/nano/store/block.hpp +++ b/nano/store/block.hpp @@ -30,7 +30,6 @@ public: virtual std::optional successor (transaction const & tx, nano::block_hash const &) const = 0; virtual void successor_clear (write_transaction const & tx, nano::block_hash const &) = 0; virtual std::shared_ptr get (transaction const & tx, nano::block_hash const &) const = 0; - virtual std::shared_ptr random (transaction const & tx) = 0; virtual void del (write_transaction const & tx, nano::block_hash const &) = 0; virtual bool exists (transaction const & tx, nano::block_hash const &) = 0; virtual uint64_t count (transaction const & tx) = 0; diff --git a/nano/store/lmdb/block.cpp b/nano/store/lmdb/block.cpp index 218245b3..7343372c 100644 --- a/nano/store/lmdb/block.cpp +++ b/nano/store/lmdb/block.cpp @@ -106,19 +106,6 @@ std::shared_ptr nano::store::lmdb::block::get (store::transaction c return result; } -std::shared_ptr nano::store::lmdb::block::random (store::transaction const & transaction) -{ - nano::block_hash hash; - nano::random_pool::generate_block (hash.bytes.data (), hash.bytes.size ()); - auto existing = begin (transaction, hash); - if (existing == end (transaction)) - { - existing = begin (transaction); - } - debug_assert (existing != end (transaction)); - return existing->second.block; -} - void nano::store::lmdb::block::del (store::write_transaction const & transaction_a, nano::block_hash const & hash_a) { auto status = store.del (transaction_a, tables::blocks, hash_a); diff --git a/nano/store/lmdb/block.hpp b/nano/store/lmdb/block.hpp index f1130fea..9da50b58 100644 --- a/nano/store/lmdb/block.hpp +++ b/nano/store/lmdb/block.hpp @@ -27,7 +27,6 @@ public: std::optional successor (store::transaction const & transaction_a, nano::block_hash const & hash_a) const override; void successor_clear (store::write_transaction const & transaction_a, nano::block_hash const & hash_a) override; std::shared_ptr get (store::transaction const & transaction_a, nano::block_hash const & hash_a) const override; - std::shared_ptr random (store::transaction const & transaction_a) override; void del (store::write_transaction const & transaction_a, nano::block_hash const & hash_a) override; bool exists (store::transaction const & transaction_a, nano::block_hash const & hash_a) override; uint64_t count (store::transaction const & transaction_a) override; diff --git a/nano/store/rocksdb/block.cpp b/nano/store/rocksdb/block.cpp index 6385704d..32ba7b8b 100644 --- a/nano/store/rocksdb/block.cpp +++ b/nano/store/rocksdb/block.cpp @@ -106,18 +106,6 @@ std::shared_ptr nano::store::rocksdb::block::get (store::transactio } return result; } -std::shared_ptr nano::store::rocksdb::block::random (store::transaction const & transaction) -{ - nano::block_hash hash; - nano::random_pool::generate_block (hash.bytes.data (), hash.bytes.size ()); - auto existing = begin (transaction, hash); - if (existing == end (transaction)) - { - existing = begin (transaction); - } - debug_assert (existing != end (transaction)); - return existing->second.block; -} void nano::store::rocksdb::block::del (store::write_transaction const & transaction_a, nano::block_hash const & hash_a) { diff --git a/nano/store/rocksdb/block.hpp b/nano/store/rocksdb/block.hpp index 741c862f..23918d83 100644 --- a/nano/store/rocksdb/block.hpp +++ b/nano/store/rocksdb/block.hpp @@ -25,7 +25,6 @@ public: std::optional successor (store::transaction const & transaction_a, nano::block_hash const & hash_a) const override; void successor_clear (store::write_transaction const & transaction_a, nano::block_hash const & hash_a) override; std::shared_ptr get (store::transaction const & transaction_a, nano::block_hash const & hash_a) const override; - std::shared_ptr random (store::transaction const & transaction_a) override; void del (store::write_transaction const & transaction_a, nano::block_hash const & hash_a) override; bool exists (store::transaction const & transaction_a, nano::block_hash const & hash_a) override; uint64_t count (store::transaction const & transaction_a) override;