Merge pull request #4939 from pwojcikdev/optimistic-scheduler-sorting

Activate largest-gap optimistic elections first

nano/core_test/optimistic_scheduler.cpp

@@ -17,8 +17,11 @@ using namespace std::chrono_literals;
  */
 TEST (optimistic_scheduler, activate_one)
 {
-	nano::test::system system{};
+	nano::test::system system;
-	auto & node = *system.add_node ();
+
+	nano::node_config config;
+	config.priority_scheduler.enable = false; // Disable priority scheduler to avoid interference
+	auto & node = *system.add_node (config);
 
 	// Needs to be greater than optimistic scheduler `gap_threshold`
 	const int howmany_blocks = 64;
@@ -41,8 +44,11 @@ TEST (optimistic_scheduler, activate_one)
  */
 TEST (optimistic_scheduler, activate_one_zero_conf)
 {
-	nano::test::system system{};
+	nano::test::system system;
-	auto & node = *system.add_node ();
+
+	nano::node_config config;
+	config.priority_scheduler.enable = false; // Disable priority scheduler to avoid interference
+	auto & node = *system.add_node (config);
 
 	// Can be smaller than optimistic scheduler `gap_threshold`
 	// This is meant to activate short account chains (eg. binary tree spam leaf accounts)
@@ -63,8 +69,11 @@ TEST (optimistic_scheduler, activate_one_zero_conf)
  */
 TEST (optimistic_scheduler, activate_many)
 {
-	nano::test::system system{};
+	nano::test::system system;
-	auto & node = *system.add_node ();
+
+	nano::node_config config;
+	config.priority_scheduler.enable = false; // Disable priority scheduler to avoid interference
+	auto & node = *system.add_node (config);
 
 	// Needs to be greater than optimistic scheduler `gap_threshold`
 	const int howmany_blocks = 64;
@@ -72,8 +81,8 @@ TEST (optimistic_scheduler, activate_many)
 
 	auto chains = nano::test::setup_chains (system, node, howmany_chains, howmany_blocks, nano::dev::genesis_key, /* do not confirm */ false);
 
-	// Ensure all unconfirmed accounts head block gets activated
+	// Ensure all unconfirmed account head blocks get activated
-	ASSERT_TIMELY (5s, std::all_of (chains.begin (), chains.end (), [&] (auto const & entry) {
+	ASSERT_TIMELY (15s, std::all_of (chains.begin (), chains.end (), [&] (auto const & entry) {
 		auto const & [account, blocks] = entry;
 		auto const & block = blocks.back ();
 		auto election = node.active.election (block->qualified_root ());
@@ -86,7 +95,8 @@ TEST (optimistic_scheduler, activate_many)
  */
 TEST (optimistic_scheduler, under_gap_threshold)
 {
-	nano::test::system system{};
+	nano::test::system system;
+
 	nano::node_config config = system.default_config ();
 	config.backlog_scan.enable = false;
 	auto & node = *system.add_node (config);

nano/lib/constants.hpp

@@ -132,7 +132,6 @@ public:
 			telemetry_cache_cutoff = 2000ms;
 			telemetry_request_interval = 500ms;
 			telemetry_broadcast_interval = 500ms;
-			optimistic_activation_delay = 2s;
 			rep_crawler_normal_interval = 500ms;
 			rep_crawler_warmup_interval = 500ms;
 		}
@@ -183,9 +182,6 @@ public:
 	/** Telemetry data older than this value is considered stale */
 	std::chrono::milliseconds telemetry_cache_cutoff{ 1000 * 130 }; // 2 * `telemetry_broadcast_interval` + some margin
 
-	/** How much to delay activation of optimistic elections to avoid interfering with election scheduler */
-	std::chrono::seconds optimistic_activation_delay{ 30 };
-
 	std::chrono::milliseconds rep_crawler_normal_interval{ 1000 * 7 };
 	std::chrono::milliseconds rep_crawler_warmup_interval{ 1000 * 3 };
 

nano/node/scheduler/optimistic.cpp

@@ -52,7 +52,11 @@ void nano::scheduler::optimistic::stop ()
 
 void nano::scheduler::optimistic::notify ()
 {
-	condition.notify_all ();
+	// Only wake up the thread if there is space inside AEC for optimistic elections
+	if (active.vacancy (nano::election_behavior::optimistic) > 0)
+	{
+		condition.notify_all ();
+	}
 }
 
 bool nano::scheduler::optimistic::activate_predicate (const nano::account_info & account_info, const nano::confirmation_height_info & conf_info) const
@@ -72,33 +76,36 @@ bool nano::scheduler::optimistic::activate_predicate (const nano::account_info &
 
 bool nano::scheduler::optimistic::activate (const nano::account & account, const nano::account_info & account_info, const nano::confirmation_height_info & conf_info)
 {
+	debug_assert (account_info.block_count >= conf_info.height);
+
 	if (!config.enable)
 	{
 		return false;
 	}
 
-	debug_assert (account_info.block_count >= conf_info.height);
 	if (activate_predicate (account_info, conf_info))
 	{
+		nano::lock_guard<nano::mutex> lock{ mutex };
+
+		// Assume gap_threshold for accounts with nothing confirmed
+		auto const unconfirmed_height = std::max (account_info.block_count - conf_info.height, config.gap_threshold);
+
+		auto [it, inserted] = candidates.push_back ({ account, nano::clock::now (), unconfirmed_height });
+
+		stats.inc (nano::stat::type::optimistic_scheduler, inserted ? nano::stat::detail::activated : nano::stat::detail::duplicate);
+
+		// Limit candidates container size
+		if (candidates.size () > config.max_size)
 		{
-			nano::lock_guard<nano::mutex> lock{ mutex };
+			// Remove oldest candidate
-
+			candidates.pop_front ();
-			// Prevent duplicate candidate accounts
-			if (candidates.get<tag_account> ().contains (account))
-			{
-				return false; // Not activated
-			}
-			// Limit candidates container size
-			if (candidates.size () >= config.max_size)
-			{
-				return false; // Not activated
-			}
-
-			stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::activated);
-			candidates.push_back ({ account, nano::clock::now () });
 		}
-		return true; // Activated
+
+		// Not notifying the thread immediately here, since we need to wait for activation_delay to elapse
+
+		return inserted;
 	}
+
 	return false; // Not activated
 }
 
@@ -115,9 +122,10 @@ bool nano::scheduler::optimistic::predicate () const
 		return false;
 	}
 
-	auto candidate = candidates.front ();
+	auto candidate = candidates.get<tag_unconfirmed_height> ().begin ();
-	bool result = nano::elapsed (candidate.timestamp, network_constants.optimistic_activation_delay);
+	debug_assert (candidate != candidates.get<tag_unconfirmed_height> ().end ());
-	return result;
+
+	return elapsed (candidate->timestamp, config.activation_delay);
 }
 
 void nano::scheduler::optimistic::run ()
@@ -125,29 +133,41 @@ void nano::scheduler::optimistic::run ()
 	nano::unique_lock<nano::mutex> lock{ mutex };
 	while (!stopped)
 	{
-		stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop);
+		condition.wait_for (lock, config.activation_delay / 2, [this] () {
+			return stopped || predicate ();
+		});
+
+		if (stopped)
+		{
+			return;
+		}
 
 		if (predicate ())
 		{
+			stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop);
+
+			lock.unlock ();
+
+			// Acquire transaction outside of the lock
 			auto transaction = ledger.tx_begin_read ();
 
-			while (predicate ())
+			lock.lock ();
+
+			while (predicate () && !stopped)
 			{
 				debug_assert (!candidates.empty ());
-				auto candidate = candidates.front ();
+				auto & height_index = candidates.get<tag_unconfirmed_height> ();
-				candidates.pop_front ();
+				auto candidate = *height_index.begin ();
+				height_index.erase (height_index.begin ());
 
 				lock.unlock ();
 
+				transaction.refresh_if_needed ();
 				run_one (transaction, candidate);
 
 				lock.lock ();
 			}
 		}
-
-		condition.wait_for (lock, network_constants.optimistic_activation_delay / 2, [this] () {
-			return stopped || predicate ();
-		});
 	}
 }
 
@@ -160,7 +180,7 @@ void nano::scheduler::optimistic::run_one (secure::transaction const & transacti
 		if (!node.block_confirmed_or_being_confirmed (transaction, block->hash ()))
 		{
 			// Try to insert it into AEC
-			// We check for AEC vacancy inside our predicate
+			// We check for AEC vacancy inside the predicate
 			auto result = node.active.insert (block, nano::election_behavior::optimistic);
 
 			stats.inc (nano::stat::type::optimistic_scheduler, result.inserted ? nano::stat::detail::insert : nano::stat::detail::insert_failed);
@@ -186,6 +206,7 @@ nano::error nano::scheduler::optimistic_config::deserialize (nano::tomlconfig &
 	toml.get ("enable", enable);
 	toml.get ("gap_threshold", gap_threshold);
 	toml.get ("max_size", max_size);
+	toml.get_duration ("activation_delay", activation_delay);
 
 	return toml.get_error ();
 }
@@ -195,6 +216,7 @@ nano::error nano::scheduler::optimistic_config::serialize (nano::tomlconfig & to
 	toml.put ("enable", enable, "Enable or disable optimistic elections\ntype:bool");
 	toml.put ("gap_threshold", gap_threshold, "Minimum difference between confirmation frontier and account frontier to become a candidate for optimistic confirmation\ntype:uint64");
 	toml.put ("max_size", max_size, "Maximum number of candidates stored in memory\ntype:uint64");
+	toml.put ("activation_delay", activation_delay.count (), "How much to delay activation of optimistic elections to avoid interfering with election scheduler\ntype:milliseconds");
 
 	return toml.get_error ();
 }

nano/node/scheduler/optimistic.hpp

@@ -33,10 +33,13 @@ public:
 	bool enable{ true };
 
 	/** Minimum difference between confirmation frontier and account frontier to become a candidate for optimistic confirmation */
-	std::size_t gap_threshold{ 32 };
+	uint64_t gap_threshold{ 16 };
 
 	/** Maximum number of candidates stored in memory */
-	std::size_t max_size{ 1024 * 64 };
+	std::size_t max_size{ 1024 * 4 };
+
+	/** How much to delay activation of optimistic elections to avoid interfering with election scheduler */
+	std::chrono::milliseconds activation_delay{ 1s };
 };
 
 class optimistic final
@@ -82,17 +85,21 @@ private:
 	{
 		nano::account account;
 		nano::clock::time_point timestamp;
+		uint64_t unconfirmed_height;
 	};
 
 	// clang-format off
 	class tag_sequenced {};
 	class tag_account {};
+	class tag_unconfirmed_height {};
 
 	using ordered_candidates = boost::multi_index_container<entry,
 	mi::indexed_by<
 		mi::sequenced<mi::tag<tag_sequenced>>,
 		mi::hashed_unique<mi::tag<tag_account>,
-			mi::member<entry, nano::account, &entry::account>>
+			mi::member<entry, nano::account, &entry::account>>,
+		mi::ordered_non_unique<mi::tag<tag_unconfirmed_height>,
+			mi::member<entry, uint64_t, &entry::unconfirmed_height>, std::greater<uint64_t>> // Descending
 	>>;
 	// clang-format on