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 1/4] 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 From d631c02ac0c6494ea043e0ce359356b005a5e9e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 5 Sep 2024 11:19:01 +0200 Subject: [PATCH 2/4] Notify about vacancy change --- nano/node/active_elections.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index 3d086423d3..7c4aadc631 100644 --- a/nano/node/active_elections.cpp +++ b/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::active_elections::remove_election_winner_details (nano::block_hash const & hash_a) { - nano::lock_guard guard{ election_winner_details_mutex }; std::shared_ptr 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 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; } @@ -344,6 +349,7 @@ void nano::active_elections::cleanup_election (nano::unique_lock & { entry.erased_callback (election); } + vacancy_update (); for (auto const & [hash, block] : blocks_l) From 167526a04e4f505effacd93898816d090d0f3580 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 5 Sep 2024 11:19:17 +0200 Subject: [PATCH 3/4] Make election buckets always respect AEC vacancy --- nano/node/scheduler/bucket.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/nano/node/scheduler/bucket.cpp b/nano/node/scheduler/bucket.cpp index f3e69383cd..f86fd4bf55 100644 --- a/nano/node/scheduler/bucket.cpp +++ b/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; } From 82d14238ae84cdec0b5b26884b016d7c283537e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 5 Sep 2024 09:38:03 +0200 Subject: [PATCH 4/4] Build script --- ci/build.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/build.sh b/ci/build.sh index bee8f057a7..41d13cf265 100755 --- a/ci/build.sh +++ b/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=" fi fi