From 441c5db34619c694cf9d87de0aafd521452c066f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Wed, 4 Sep 2024 21:28:24 +0200 Subject: [PATCH] Bound the growth of `election_winner_details` set --- nano/core_test/active_elections.cpp | 50 +++++++++++++++++++++++++++++ nano/node/active_elections.cpp | 36 +++++++++++++-------- nano/node/active_elections.hpp | 6 ++-- 3 files changed, 76 insertions(+), 16 deletions(-) diff --git a/nano/core_test/active_elections.cpp b/nano/core_test/active_elections.cpp index fd94659a50..245f018579 100644 --- a/nano/core_test/active_elections.cpp +++ b/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); +} \ No newline at end of file diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index bef80ca378..3d086423d3 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -243,19 +243,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 guard{ mutex }; - switch (behavior) - { - case nano::election_behavior::manual: - return std::numeric_limits::max (); - case nano::election_behavior::priority: - return limit (nano::election_behavior::priority) - static_cast (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 guard{ mutex }; + switch (behavior) + { + case nano::election_behavior::manual: + return std::numeric_limits::max (); + case nano::election_behavior::priority: + return limit (nano::election_behavior::priority) - static_cast (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 (config.max_election_winners) - static_cast (election_winner_details_size ()); + }; + + return std::min (election_vacancy (behavior), election_winners_vacancy ()); } void nano::active_elections::request_confirm (nano::unique_lock & lock_a) @@ -558,7 +566,7 @@ bool nano::active_elections::publish (std::shared_ptr 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 guard{ election_winner_details_mutex }; return election_winner_details.size (); diff --git a/nano/node/active_elections.hpp b/nano/node/active_elections.hpp index d19a7c48fd..c74df00332 100644 --- a/nano/node/active_elections.hpp +++ b/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 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 const &); std::shared_ptr 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> election_winner_details; // Maximum time an election can be kept active if it is extending the container