Bound the growth of election_winner_details set

ci/build.sh

@@ -11,10 +11,10 @@ SRC=${SRC:-${PWD}}
 OS=$(uname)
 
 CMAKE_BACKTRACE=""
-if [[ "$OS" == 'Linux' ]]; then
+if [[ ${OS} == 'Linux' ]]; then
     CMAKE_BACKTRACE="-DNANO_STACKTRACE_BACKTRACE=ON"
 
-    if [[ "$COMPILER" == 'clang' ]]; then
+    if [[ ${COMPILER:-} == 'clang' ]]; then
         CMAKE_BACKTRACE="${CMAKE_BACKTRACE} -DNANO_BACKTRACE_INCLUDE=</tmp/backtrace.h>"
     fi
 fi

nano/core_test/active_elections.cpp

@@ -1388,3 +1388,53 @@ TEST (active_elections, limit_vote_hinted_elections)
 	// Ensure there was no overflow of elections
 	ASSERT_EQ (0, node.stats.count (nano::stat::type::active_elections_dropped, nano::stat::detail::priority));
 }
+
+/*
+ * Ensures that election winners set won't grow without bounds when cementing is slower that the rate of confirming new elections
+ */
+TEST (active_elections, bound_election_winners)
+{
+	nano::test::system system;
+	nano::node_config config = system.default_config ();
+	// Set election winner limit to a low value
+	config.active_elections.max_election_winners = 5;
+	auto & node = *system.add_node (config);
+
+	// Start elections for a couple of blocks, number of elections is larger than the election winner set limit
+	auto blocks = nano::test::setup_independent_blocks (system, node, 10);
+	ASSERT_TIMELY (5s, std::all_of (blocks.begin (), blocks.end (), [&node] (auto & block) {
+		return node.active.active (*block);
+	}));
+
+	{
+		// Prevent cementing of confirmed blocks
+		auto guard = node.ledger.tx_begin_write ({}, nano::store::writer::testing);
+
+		// Ensure that when the number of election winners reaches the limit, AEC vacancy reflects that
+		ASSERT_TRUE (node.active.vacancy (nano::election_behavior::priority) > 0);
+
+		int index = 0;
+		for (; index < config.active_elections.max_election_winners; ++index)
+		{
+			auto election = node.vote_router.election (blocks[index]->hash ());
+			ASSERT_TRUE (election);
+			election->force_confirm ();
+		}
+
+		ASSERT_TIMELY_EQ (5s, node.active.vacancy (nano::election_behavior::priority), 0);
+
+		// Confirming more elections should make the vacancy negative
+		for (; index < blocks.size (); ++index)
+		{
+			auto election = node.vote_router.election (blocks[index]->hash ());
+			ASSERT_TRUE (election);
+			election->force_confirm ();
+		}
+
+		ASSERT_TIMELY (5s, node.active.vacancy (nano::election_behavior::priority) < 0);
+
+		// Release the guard to allow cementing, there should be some vacancy now
+	}
+
+	ASSERT_TIMELY (5s, node.active.vacancy (nano::election_behavior::priority) > 0);
+}

nano/node/active_elections.cpp

@@ -193,14 +193,19 @@ void nano::active_elections::add_election_winner_details (nano::block_hash const
 
 std::shared_ptr<nano::election> nano::active_elections::remove_election_winner_details (nano::block_hash const & hash_a)
 {
-	nano::lock_guard<nano::mutex> guard{ election_winner_details_mutex };
 	std::shared_ptr<nano::election> result;
-	auto existing = election_winner_details.find (hash_a);
-	if (existing != election_winner_details.end ())
 	{
-		result = existing->second;
-		election_winner_details.erase (existing);
+		nano::lock_guard<nano::mutex> guard{ election_winner_details_mutex };
+		auto existing = election_winner_details.find (hash_a);
+		if (existing != election_winner_details.end ())
+		{
+			result = existing->second;
+			election_winner_details.erase (existing);
+		}
 	}
+
+	vacancy_update ();
+
 	return result;
 }
 
@@ -243,19 +248,27 @@ int64_t nano::active_elections::limit (nano::election_behavior behavior) const
 
 int64_t nano::active_elections::vacancy (nano::election_behavior behavior) const
 {
-	nano::lock_guard<nano::mutex> guard{ mutex };
-	switch (behavior)
-	{
-		case nano::election_behavior::manual:
-			return std::numeric_limits<int64_t>::max ();
-		case nano::election_behavior::priority:
-			return limit (nano::election_behavior::priority) - static_cast<int64_t> (roots.size ());
-		case nano::election_behavior::hinted:
-		case nano::election_behavior::optimistic:
-			return limit (behavior) - count_by_behavior[behavior];
-	}
-	debug_assert (false); // Unknown enum
-	return 0;
+	auto election_vacancy = [this] (nano::election_behavior behavior) -> int64_t {
+		nano::lock_guard<nano::mutex> guard{ mutex };
+		switch (behavior)
+		{
+			case nano::election_behavior::manual:
+				return std::numeric_limits<int64_t>::max ();
+			case nano::election_behavior::priority:
+				return limit (nano::election_behavior::priority) - static_cast<int64_t> (roots.size ());
+			case nano::election_behavior::hinted:
+			case nano::election_behavior::optimistic:
+				return limit (behavior) - count_by_behavior[behavior];
+		}
+		debug_assert (false); // Unknown enum
+		return 0;
+	};
+
+	auto election_winners_vacancy = [this] () -> int64_t {
+		return static_cast<int64_t> (config.max_election_winners) - static_cast<int64_t> (election_winner_details_size ());
+	};
+
+	return std::min (election_vacancy (behavior), election_winners_vacancy ());
 }
 
 void nano::active_elections::request_confirm (nano::unique_lock<nano::mutex> & lock_a)
@@ -336,6 +349,7 @@ void nano::active_elections::cleanup_election (nano::unique_lock<nano::mutex> &
 	{
 		entry.erased_callback (election);
 	}
+
 	vacancy_update ();
 
 	for (auto const & [hash, block] : blocks_l)
@@ -558,7 +572,7 @@ bool nano::active_elections::publish (std::shared_ptr<nano::block> const & block
 	return result;
 }
 
-std::size_t nano::active_elections::election_winner_details_size ()
+std::size_t nano::active_elections::election_winner_details_size () const
 {
 	nano::lock_guard<nano::mutex> guard{ election_winner_details_mutex };
 	return election_winner_details.size ();

nano/node/active_elections.hpp

@@ -64,6 +64,8 @@ class active_elections_config final
 	std::size_t confirmation_history_size{ 2048 };
 	// Maximum cache size for recently_confirmed
 	std::size_t confirmation_cache{ 65536 };
+	// Maximum size of election winner details set
+	std::size_t max_election_winners{ 1024 * 16 };
 };
 
 /**
@@ -138,7 +140,7 @@ class active_elections final
 	int64_t vacancy (nano::election_behavior behavior) const;
 	std::function<void ()> vacancy_update{ [] () {} };
 
-	std::size_t election_winner_details_size ();
+	std::size_t election_winner_details_size () const;
 	void add_election_winner_details (nano::block_hash const &, std::shared_ptr<nano::election> const &);
 	std::shared_ptr<nano::election> remove_election_winner_details (nano::block_hash const &);
 
@@ -170,7 +172,7 @@ class active_elections final
 	mutable nano::mutex mutex{ mutex_identifier (mutexes::active) };
 
 private:
-	nano::mutex election_winner_details_mutex{ mutex_identifier (mutexes::election_winner_details) };
+	mutable nano::mutex election_winner_details_mutex{ mutex_identifier (mutexes::election_winner_details) };
 	std::unordered_map<nano::block_hash, std::shared_ptr<nano::election>> election_winner_details;
 
 	// Maximum time an election can be kept active if it is extending the container

nano/node/scheduler/bucket.cpp

@@ -38,11 +38,7 @@ bool nano::scheduler::bucket::election_vacancy (priority_t candidate) const
 {
 	debug_assert (!mutex.try_lock ());
 
-	if (elections.size () < config.reserved_elections)
-	{
-		return true;
-	}
-	if (elections.size () < config.max_elections)
+	if (elections.size () < config.reserved_elections || elections.size () < config.max_elections)
 	{
 		return active.vacancy (nano::election_behavior::priority) > 0;
 	}