Activate highest 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;
@@ -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/node/scheduler/optimistic.cpp

@@ -72,34 +72,42 @@ 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);
+	bool activated = false;
+
 	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
+
+		activated = inserted;
 	}
-	return false; // Not activated
+
+	if (activated)
+	{
+		condition.notify_all ();
+	}
+
+	return activated;
 }
 
 bool nano::scheduler::optimistic::predicate () const
@@ -115,9 +123,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, network_constants.optimistic_activation_delay);
 }
 
 void nano::scheduler::optimistic::run ()
@@ -127,27 +136,36 @@ void nano::scheduler::optimistic::run ()
 	{
 		stats.inc (nano::stat::type::optimistic_scheduler, nano::stat::detail::loop);
 
-		if (predicate ())
-		{
-			auto transaction = ledger.tx_begin_read ();
-
-			while (predicate ())
-			{
-				debug_assert (!candidates.empty ());
-				auto candidate = candidates.front ();
-				candidates.pop_front ();
-
-				lock.unlock ();
-
-				run_one (transaction, candidate);
-
-				lock.lock ();
-			}
-		}
-
 		condition.wait_for (lock, network_constants.optimistic_activation_delay / 2, [this] () {
 			return stopped || predicate ();
 		});
+
+		if (stopped)
+		{
+			return;
+		}
+
+		lock.unlock ();
+
+		// Acquire transaction outside of the lock
+		auto transaction = ledger.tx_begin_read ();
+
+		lock.lock ();
+
+		while (predicate () && !stopped)
+		{
+			debug_assert (!candidates.empty ());
+			auto & height_index = candidates.get<tag_unconfirmed_height> ();
+			auto candidate = *height_index.begin ();
+			height_index.erase (height_index.begin ());
+
+			lock.unlock ();
+
+			transaction.refresh_if_needed ();
+			run_one (transaction, candidate);
+
+			lock.lock ();
+		}
 	}
 }
 
@@ -160,7 +178,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);

nano/node/scheduler/optimistic.hpp

@@ -33,10 +33,10 @@ 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{ 32 };
 
 	/** Maximum number of candidates stored in memory */
-	std::size_t max_size{ 1024 * 64 };
+	std::size_t max_size{ 1024 * 4 };
 };
 
 class optimistic final
@@ -82,17 +82,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