Merge pull request #4792 from pwojcikdev/random-block-sampling

Rework random block sampling for rep crawler

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;

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);
+	}
+	// Genesis and send2 should be included in the random selection
+	{
+		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);
+		}
 	}
-	done = false;
-	while (!done)
 	{
-		++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 () == nano::dev::genesis->hash ());
+			ASSERT_LE (iteration, 1000);
+		}
 	}
 }
 

nano/node/repcrawler.cpp

@@ -269,43 +269,22 @@ std::vector<std::shared_ptr<nano::transport::channel>> nano::rep_crawler::prepar
 	return { random_peers.begin (), random_peers.end () };
 }
 
-auto nano::rep_crawler::prepare_query_target () -> std::optional<hash_root_t>
+auto nano::rep_crawler::prepare_query_target () const -> std::optional<hash_root_t>
 {
-	constexpr int max_attempts = 4;
+	constexpr int max_attempts = 10;
 
 	auto transaction = node.ledger.tx_begin_read ();
 
-	std::optional<std::pair<nano::block_hash, nano::block_hash>> hash_root;
-
-	// Randomly select a block from ledger to request votes for
-	for (auto i = 0; i < max_attempts && !hash_root; ++i)
-	{
-		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))
-		{
-			hash_root = std::nullopt;
-		}
-	}
-
-	if (!hash_root)
-	{
-		return std::nullopt;
-	}
-
-	// Don't send same block multiple times in tests
-	if (node.network_params.network.is_dev_network ())
+	auto random_blocks = node.ledger.random_blocks (transaction, max_attempts);
+	for (auto const & block : random_blocks)
 	{
-		nano::lock_guard<nano::mutex> lock{ mutex };
-
-		for (auto i = 0; queries.get<tag_hash> ().count (hash_root->first) != 0 && i < max_attempts; ++i)
+		if (!active.recently_confirmed.exists (block->hash ()))
 		{
-			hash_root = node.ledger.hash_root_random (transaction);
+			return std::make_pair (block->hash (), block->root ());
 		}
 	}
 
-	return hash_root;
+	return std::nullopt;
 }
 
 bool nano::rep_crawler::track_rep_request (hash_root_t hash_root, std::shared_ptr<nano::transport::channel> const & channel)

nano/node/repcrawler.hpp

@@ -107,7 +107,7 @@ class rep_crawler final
 
 	/** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */
 	std::vector<std::shared_ptr<nano::transport::channel>> prepare_crawl_targets (bool sufficient_weight) const;
-	std::optional<hash_root_t> prepare_query_target ();
+	std::optional<hash_root_t> prepare_query_target () const;
 	bool track_rep_request (hash_root_t hash_root, std::shared_ptr<nano::transport::channel> const & channel);
 
 private:

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::block_hash, nano::block_hash> nano::ledger::hash_root_random (secure::transaction const & transaction_a) const
+std::deque<std::shared_ptr<nano::block>> nano::ledger::random_blocks (secure::transaction const & transaction, size_t count) const
 {
-	nano::block_hash hash (0);
-	nano::root root (0);
-	if (!pruning)
-	{
-		auto block (store.block.random (transaction_a));
-		hash = block->hash ();
-		root = block->root ();
-	}
-	else
+	std::deque<std::shared_ptr<nano::block>> result;
+
+	auto const starting_hash = nano::random_pool::generate<nano::block_hash> ();
+
+	// 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)
 	{
-		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)
-		{
-			hash = store.pruned.random (transaction_a);
-		}
-		if (hash.is_zero ())
+		if (it != end)
 		{
-			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.as_block_hash ());
+
+	return result;
 }
 
 // Vote weight of an account
@@ -1284,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;
 	}
 
@@ -1431,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);
@@ -1455,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 blocks = random_blocks (lmdb_transaction, 42);
+		release_assert (!blocks.empty ());
+		for (auto const & block : blocks)
+		{
+			auto const account = block->account ();
 
-		auto account = random_block->account ();
-		nano::account_info account_info;
-		error |= rocksdb_store->account.get (rocksdb_transaction, account, account_info);
+			error |= rocksdb_store->block.get (rocksdb_transaction, block->hash ()) == nullptr;
 
-		// 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);
+			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

nano/secure/ledger.hpp

@@ -58,7 +58,7 @@ class ledger final
 	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<nano::block_hash, nano::block_hash> hash_root_random (secure::transaction const &) const;
+	std::deque<std::shared_ptr<nano::block>> random_blocks (secure::transaction const &, size_t count) const;
 	std::optional<nano::pending_info> pending_info (secure::transaction const &, nano::pending_key const & key) const;
 	std::deque<std::shared_ptr<nano::block>> 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<nano::block> block);

nano/store/block.hpp

@@ -30,7 +30,6 @@ class block
 	virtual std::optional<nano::block_hash> 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<nano::block> get (transaction const & tx, nano::block_hash const &) const = 0;
-	virtual std::shared_ptr<nano::block> 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;

nano/store/lmdb/block.cpp

@@ -106,19 +106,6 @@ std::shared_ptr<nano::block> nano::store::lmdb::block::get (store::transaction c
 	return result;
 }
 
-std::shared_ptr<nano::block> 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);

nano/store/lmdb/block.hpp

@@ -27,7 +27,6 @@ class block : public nano::store::block
 	std::optional<nano::block_hash> 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<nano::block> get (store::transaction const & transaction_a, nano::block_hash const & hash_a) const override;
-	std::shared_ptr<nano::block> 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;

nano/store/rocksdb/block.cpp

@@ -106,18 +106,6 @@ std::shared_ptr<nano::block> nano::store::rocksdb::block::get (store::transactio
 	}
 	return result;
 }
-std::shared_ptr<nano::block> 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)
 {

nano/store/rocksdb/block.hpp

@@ -25,7 +25,6 @@ class block : public nano::store::block
 	std::optional<nano::block_hash> 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<nano::block> get (store::transaction const & transaction_a, nano::block_hash const & hash_a) const override;
-	std::shared_ptr<nano::block> 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;