Merge pull request #4806 from pwojcikdev/bootstrap-tuning-5

Bootstrap tuning

nano/core_test/bootstrap.cpp

@@ -122,7 +122,7 @@ TEST (account_sets, priority_up_down)
 	sets.priority_up (account);
 	ASSERT_EQ (sets.priority (account), nano::bootstrap::account_sets::priority_initial);
 	sets.priority_down (account);
-	ASSERT_EQ (sets.priority (account), nano::bootstrap::account_sets::priority_initial);
+	ASSERT_EQ (sets.priority (account), nano::bootstrap::account_sets::priority_initial / nano::bootstrap::account_sets::priority_divide);
 }
 
 TEST (account_sets, priority_down_empty)

nano/lib/stats_enums.hpp

@@ -127,6 +127,7 @@ enum class detail
 	erased,
 	request,
 	request_failed,
+	request_success,
 	broadcast,
 	cleanup,
 	top,

nano/node/bootstrap/account_sets.cpp

@@ -61,15 +61,18 @@ void nano::bootstrap::account_sets::priority_down (nano::account const & account
 	{
 		stats.inc (nano::stat::type::bootstrap_account_sets, nano::stat::detail::deprioritize);
 
-		if (it->fails >= account_sets::max_fails || it->fails >= it->priority)
+		auto priority = it->priority / account_sets::priority_divide;
+
+		if (it->fails >= account_sets::max_fails || it->fails >= it->priority || priority <= account_sets::priority_cutoff)
 		{
 			stats.inc (nano::stat::type::bootstrap_account_sets, nano::stat::detail::erase_by_threshold);
 			priorities.get<tag_account> ().erase (it);
 		}
 		else
 		{
-			priorities.get<tag_account> ().modify (it, [] (auto & val) {
+			priorities.get<tag_account> ().modify (it, [priority] (auto & val) {
 				val.fails += 1;
+				val.priority = priority;
 			});
 		}
 	}
@@ -217,7 +220,7 @@ void nano::bootstrap::account_sets::trim_overflow ()
 	}
 }
 
-nano::account nano::bootstrap::account_sets::next_priority (std::function<bool (nano::account const &)> const & filter)
+auto nano::bootstrap::account_sets::next_priority (std::function<bool (nano::account const &)> const & filter) -> priority_result
 {
 	if (priorities.empty ())
 	{
@@ -236,10 +239,14 @@ nano::account nano::bootstrap::account_sets::next_priority (std::function<bool (
 		{
 			continue;
 		}
-		return entry.account;
+		return {
+			.account = entry.account,
+			.priority = entry.priority,
+			.fails = entry.fails
+		};
 	}
 
-	return { 0 };
+	return {};
 }
 
 nano::block_hash nano::bootstrap::account_sets::next_blocking (std::function<bool (nano::block_hash const &)> const & filter)

nano/node/bootstrap/account_sets.hpp

@@ -63,10 +63,17 @@ namespace bootstrap
 		 */
 		void sync_dependencies ();
 
+		struct priority_result
+		{
+			nano::account account;
+			double priority;
+			unsigned fails;
+		};
+
 		/**
 		 * Sampling
 		 */
-		nano::account next_priority (std::function<bool (nano::account const &)> const & filter);
+		priority_result next_priority (std::function<bool (nano::account const &)> const & filter);
 		nano::block_hash next_blocking (std::function<bool (nano::block_hash const &)> const & filter);
 
 		bool blocked (nano::account const & account) const;

nano/node/bootstrap/bootstrap_service.cpp

@@ -153,6 +153,8 @@ bool nano::bootstrap_service::send (std::shared_ptr<nano::transport::channel> co
 	{
 		nano::lock_guard<nano::mutex> lock{ mutex };
 		debug_assert (tags.get<tag_id> ().count (tag.id) == 0);
+		// Give extra time for the request to be processed by the channel
+		tag.cutoff = std::chrono::steady_clock::now () + config.request_timeout * 4;
 		tags.get<tag_id> ().insert (tag);
 	}
 
@@ -202,10 +204,25 @@ bool nano::bootstrap_service::send (std::shared_ptr<nano::transport::channel> co
 	stats.inc (nano::stat::type::bootstrap, nano::stat::detail::request, nano::stat::dir::out);
 	stats.inc (nano::stat::type::bootstrap_request, to_stat_detail (tag.type));
 
-	// TODO: There is no feedback mechanism if bandwidth limiter starts dropping our requests
 	channel->send (
-	request, nullptr,
-	nano::transport::buffer_drop_policy::limiter, nano::transport::traffic_type::bootstrap);
+	request, [this, id = tag.id] (auto const & ec, auto size) {
+		nano::lock_guard<nano::mutex> lock{ mutex };
+		if (auto it = tags.get<tag_id> ().find (id); it != tags.get<tag_id> ().end ())
+		{
+			if (!ec)
+			{
+				stats.inc (nano::stat::type::bootstrap, nano::stat::detail::request_success, nano::stat::dir::out);
+				tags.get<tag_id> ().modify (it, [&] (auto & tag) {
+					// After the request has been sent, the peer has a limited time to respond
+					tag.cutoff = std::chrono::steady_clock::now () + config.request_timeout;
+				});
+			}
+			else
+			{
+				stats.inc (nano::stat::type::bootstrap, nano::stat::detail::request_failed, nano::stat::dir::out);
+				tags.get<tag_id> ().erase (it);
+			}
+		} }, nano::transport::buffer_drop_policy::limiter, nano::transport::traffic_type::bootstrap);
 
 	return true; // TODO: Return channel send result
 }
@@ -355,30 +372,28 @@ size_t nano::bootstrap_service::count_tags (nano::block_hash const & hash, query
 	return std::count_if (begin, end, [source] (auto const & tag) { return tag.source == source; });
 }
 
-std::pair<nano::account, double> nano::bootstrap_service::next_priority ()
+nano::bootstrap::account_sets::priority_result nano::bootstrap_service::next_priority ()
 {
 	debug_assert (!mutex.try_lock ());
 
-	auto account = accounts.next_priority ([this] (nano::account const & account) {
+	auto next = accounts.next_priority ([this] (nano::account const & account) {
 		return count_tags (account, query_source::priority) < 4;
 	});
-	if (account.is_zero ())
+	if (next.account.is_zero ())
 	{
 		return {};
 	}
 	stats.inc (nano::stat::type::bootstrap_next, nano::stat::detail::next_priority);
-	accounts.timestamp_set (account);
-	// TODO: Priority could be returned by the accounts.next_priority() call
-	return { account, accounts.priority (account) };
+	return next;
 }
 
-std::pair<nano::account, double> nano::bootstrap_service::wait_priority ()
+nano::bootstrap::account_sets::priority_result nano::bootstrap_service::wait_priority ()
 {
-	std::pair<nano::account, double> result{ 0, 0 };
+	nano::bootstrap::account_sets::priority_result result{};
 	wait ([this, &result] () {
 		debug_assert (!mutex.try_lock ());
 		result = next_priority ();
-		if (!result.first.is_zero ())
+		if (!result.account.is_zero ())
 		{
 			return true;
 		}
@@ -548,14 +563,26 @@ void nano::bootstrap_service::run_one_priority ()
 	{
 		return;
 	}
-	auto [account, priority] = wait_priority ();
+	auto [account, priority, fails] = wait_priority ();
 	if (account.is_zero ())
 	{
 		return;
 	}
+
+	// Decide how many blocks to request
 	size_t const min_pull_count = 2;
-	auto count = std::clamp (static_cast<size_t> (priority), min_pull_count, nano::bootstrap_server::max_blocks);
-	request (account, count, channel, query_source::priority);
+	auto pull_count = std::clamp (static_cast<size_t> (priority), min_pull_count, nano::bootstrap_server::max_blocks);
+
+	bool sent = request (account, pull_count, channel, query_source::priority);
+
+	// Only cooldown accounts that are likely to have more blocks
+	// This is to avoid requesting blocks from the same frontier multiple times, before the block processor had a chance to process them
+	// Not throttling accounts that are probably up-to-date allows us to evict them from the priority set faster
+	if (sent && fails == 0)
+	{
+		nano::lock_guard<nano::mutex> lock{ mutex };
+		accounts.timestamp_set (account);
+	}
 }
 
 void nano::bootstrap_service::run_priorities ()
@@ -674,9 +701,9 @@ void nano::bootstrap_service::cleanup_and_sync ()
 
 	throttle.resize (compute_throttle_size ());
 
-	auto const cutoff = std::chrono::steady_clock::now () - config.request_timeout;
-	auto should_timeout = [cutoff] (async_tag const & tag) {
-		return tag.timestamp < cutoff;
+	auto const now = std::chrono::steady_clock::now ();
+	auto should_timeout = [&] (async_tag const & tag) {
+		return tag.cutoff < now;
 	};
 
 	auto & tags_by_order = tags.get<tag_sequenced> ();
@@ -828,13 +855,18 @@ bool nano::bootstrap_service::process (const nano::asc_pull_ack::blocks_payload
 		case verify_result::nothing_new:
 		{
 			stats.inc (nano::stat::type::bootstrap_verify_blocks, nano::stat::detail::nothing_new);
-
-			nano::lock_guard<nano::mutex> lock{ mutex };
-			accounts.priority_down (tag.account);
-			if (tag.source == query_source::database)
 			{
-				throttle.add (false);
+				nano::lock_guard<nano::mutex> lock{ mutex };
+
+				accounts.priority_down (tag.account);
+				accounts.timestamp_reset (tag.account);
+
+				if (tag.source == query_source::database)
+				{
+					throttle.add (false);
+				}
 			}
+			condition.notify_all ();
 		}
 		break;
 		case verify_result::invalid:

nano/node/bootstrap/bootstrap_service.hpp

@@ -90,9 +90,9 @@ class bootstrap_service
 		nano::account account{ 0 };
 		nano::block_hash hash{ 0 };
 		size_t count{ 0 };
-
-		id_t id{ nano::bootstrap::generate_id () };
+		std::chrono::steady_clock::time_point cutoff{};
 		std::chrono::steady_clock::time_point timestamp{ std::chrono::steady_clock::now () };
+		id_t id{ nano::bootstrap::generate_id () };
 	};
 
 private:
@@ -117,9 +117,10 @@ class bootstrap_service
 	void wait_blockprocessor () const;
 	/* Waits for a channel that is not full */
 	std::shared_ptr<nano::transport::channel> wait_channel ();
-	/* Waits until a suitable account outside of cool down period is available */
-	std::pair<nano::account, double> next_priority ();
-	std::pair<nano::account, double> wait_priority ();
+	/* Waits until a suitable account outside of cooldown period is available */
+	using priority_result = nano::bootstrap::account_sets::priority_result;
+	priority_result next_priority ();
+	priority_result wait_priority ();
 	/* Gets the next account from the database */
 	nano::account next_database (bool should_throttle);
 	nano::account wait_database (bool should_throttle);