diff --git a/nano/core_test/confirming_set.cpp b/nano/core_test/confirming_set.cpp index 46e7b914..a2b3d608 100644 --- a/nano/core_test/confirming_set.cpp +++ b/nano/core_test/confirming_set.cpp @@ -111,7 +111,6 @@ TEST (confirmation_callback, observer_callbacks) ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (3, node->ledger.cemented_count ()); - ASSERT_EQ (0, node->active.election_winner_details_size ()); } // The callback and confirmation history should only be updated after confirmation height is set (and not just after voting) @@ -186,7 +185,6 @@ TEST (confirmation_callback, confirmed_history) ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::inactive_conf_height, nano::stat::dir::out)); ASSERT_TIMELY_EQ (5s, 2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (3, node->ledger.cemented_count ()); - ASSERT_EQ (0, node->active.election_winner_details_size ()); } TEST (confirmation_callback, dependent_election) @@ -248,29 +246,4 @@ TEST (confirmation_callback, dependent_election) ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::active_conf_height, nano::stat::dir::out)); ASSERT_TIMELY_EQ (5s, 1, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::inactive_conf_height, nano::stat::dir::out)); ASSERT_EQ (4, node->ledger.cemented_count ()); - - ASSERT_EQ (0, node->active.election_winner_details_size ()); -} - -TEST (confirmation_callback, election_winner_details_clearing_node_process_confirmed) -{ - // Make sure election_winner_details is also cleared if the block never enters the confirmation height processor from node::process_confirmed - nano::test::system system (1); - auto node = system.nodes.front (); - - nano::block_builder builder; - auto send = builder - .send () - .previous (nano::dev::genesis->hash ()) - .destination (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - nano::Knano_ratio) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - // Add to election_winner_details. Use an unrealistic iteration so that it should fall into the else case and do a cleanup - node->active.add_election_winner_details (send->hash (), nullptr); - nano::election_status election; - election.winner = send; - node->process_confirmed (election, 1000000); - ASSERT_EQ (0, node->active.election_winner_details_size ()); } diff --git a/nano/core_test/ledger_confirm.cpp b/nano/core_test/ledger_confirm.cpp index 1301c9ff..f9bfeff9 100644 --- a/nano/core_test/ledger_confirm.cpp +++ b/nano/core_test/ledger_confirm.cpp @@ -752,29 +752,6 @@ TEST (ledger_confirm, observers) ASSERT_EQ (2, node1->ledger.cemented_count ()); } -TEST (ledger_confirm, election_winner_details_clearing_node_process_confirmed) -{ - // Make sure election_winner_details is also cleared if the block never enters the confirmation height processor from node::process_confirmed - nano::test::system system (1); - auto node = system.nodes.front (); - - nano::block_builder builder; - auto send = builder - .send () - .previous (nano::dev::genesis->hash ()) - .destination (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - nano::Knano_ratio) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - // Add to election_winner_details. Use an unrealistic iteration so that it should fall into the else case and do a cleanup - node->active.add_election_winner_details (send->hash (), nullptr); - nano::election_status election; - election.winner = send; - node->process_confirmed (election, 1000000); - ASSERT_EQ (0, node->active.election_winner_details_size ()); -} - TEST (ledger_confirm, pruned_source) { nano::test::system system; diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index ee5629ad..ea8c0778 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -412,6 +412,7 @@ enum class detail // active_elections started, stopped, + confirm_dependent, // unchecked put, diff --git a/nano/node/active_elections.cpp b/nano/node/active_elections.cpp index a8fb6469..feafab86 100644 --- a/nano/node/active_elections.cpp +++ b/nano/node/active_elections.cpp @@ -25,24 +25,16 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_ confirming_set{ confirming_set_a }, block_processor{ block_processor_a }, recently_confirmed{ config.confirmation_cache }, - recently_cemented{ config.confirmation_history_size }, - election_time_to_live{ node_a.network_params.network.is_dev_network () ? 0s : 2s } + recently_cemented{ config.confirmation_history_size } { count_by_behavior.fill (0); // Zero initialize array confirming_set.batch_cemented.add ([this] (auto const & cemented) { auto transaction = node.ledger.tx_begin_read (); - for (auto const & [block, confirmation_root, election] : cemented) + for (auto const & [block, confirmation_root, source_election] : cemented) { transaction.refresh_if_needed (); - block_cemented_callback (transaction, block, confirmation_root); - } - }); - - confirming_set.already_cemented.add ([this] (auto const & already_cemented) { - for (auto const & hash : already_cemented) - { - block_already_cemented_callback (hash); + block_cemented (transaction, block, confirmation_root, source_election); } }); @@ -91,28 +83,31 @@ void nano::active_elections::stop () clear (); } -void nano::active_elections::block_cemented_callback (nano::secure::transaction const & transaction, std::shared_ptr const & block, nano::block_hash const & confirmation_root) +void nano::active_elections::block_cemented (nano::secure::transaction const & transaction, std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election) { debug_assert (node.block_confirmed (block->hash ())); - if (auto election_l = election (block->qualified_root ())) + // Dependent elections are implicitly confirmed when their block is cemented + auto dependend_election = election (block->qualified_root ()); + if (dependend_election) { - election_l->try_confirm (block->hash ()); + node.stats.inc (nano::stat::type::active_elections, nano::stat::detail::confirm_dependent); + dependend_election->try_confirm (block->hash ()); } - auto election = remove_election_winner_details (block->hash ()); + nano::election_status status; std::vector votes; status.winner = block; - if (election) - { - status = election->get_status (); - votes = election->votes_with_weight (); - } - if (block->hash () == confirmation_root) + + // Check if the currently cemented block was part of an election that triggered the confirmation + if (source_election && source_election->qualified_root == block->qualified_root ()) { + status = source_election->get_status (); + debug_assert (status.winner->hash () == block->hash ()); + votes = source_election->votes_with_weight (); status.type = nano::election_status_type::active_confirmed_quorum; } - else if (election) + else if (dependend_election) { status.type = nano::election_status_type::active_confirmation_height; } @@ -120,12 +115,16 @@ void nano::active_elections::block_cemented_callback (nano::secure::transaction { status.type = nano::election_status_type::inactive_confirmation_height; } + recently_cemented.put (status); node.stats.inc (nano::stat::type::active_elections, nano::stat::detail::cemented); node.stats.inc (nano::stat::type::active_elections_cemented, to_stat_detail (status.type)); - node.logger.trace (nano::log::type::active_elections, nano::log::detail::active_cemented, nano::log::arg{ "election", election }); + node.logger.trace (nano::log::type::active_elections, nano::log::detail::active_cemented, + nano::log::arg{ "block", block }, + nano::log::arg{ "confirmation_root", confirmation_root }, + nano::log::arg{ "source_election", source_election }); notify_observers (transaction, status, votes); @@ -185,39 +184,6 @@ void nano::active_elections::activate_successors (nano::secure::transaction cons } } -void nano::active_elections::add_election_winner_details (nano::block_hash const & hash_a, std::shared_ptr const & election_a) -{ - nano::lock_guard guard{ election_winner_details_mutex }; - election_winner_details.emplace (hash_a, election_a); -} - -std::shared_ptr nano::active_elections::remove_election_winner_details (nano::block_hash const & hash_a) -{ - std::shared_ptr result; - { - 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; -} - -void nano::active_elections::block_already_cemented_callback (nano::block_hash const & hash_a) -{ - // Depending on timing there is a situation where the election_winner_details is not reset. - // This can happen when a block wins an election, and the block is confirmed + observer - // called before the block hash gets added to election_winner_details. If the block is confirmed - // callbacks have already been done, so we can safely just remove it. - remove_election_winner_details (hash_a); -} - int64_t nano::active_elections::limit (nano::election_behavior behavior) const { switch (behavior) @@ -265,7 +231,7 @@ int64_t nano::active_elections::vacancy (nano::election_behavior behavior) const }; auto election_winners_vacancy = [this] () -> int64_t { - return static_cast (config.max_election_winners) - static_cast (election_winner_details_size ()); + return static_cast (config.max_election_winners) - static_cast (confirming_set.size ()); }; return std::min (election_vacancy (behavior), election_winners_vacancy ()); @@ -572,12 +538,6 @@ bool nano::active_elections::publish (std::shared_ptr const & block return result; } -std::size_t nano::active_elections::election_winner_details_size () const -{ - nano::lock_guard guard{ election_winner_details_mutex }; - return election_winner_details.size (); -} - void nano::active_elections::clear () { // TODO: Call erased_callback for each election @@ -595,7 +555,6 @@ nano::container_info nano::active_elections::container_info () const nano::container_info info; info.put ("roots", roots.size ()); - info.put ("election_winner_details", election_winner_details_size ()); info.put ("normal", static_cast (count_by_behavior[nano::election_behavior::priority])); info.put ("hinted", static_cast (count_by_behavior[nano::election_behavior::hinted])); info.put ("optimistic", static_cast (count_by_behavior[nano::election_behavior::optimistic])); diff --git a/nano/node/active_elections.hpp b/nano/node/active_elections.hpp index 222d5d27..1a76cf04 100644 --- a/nano/node/active_elections.hpp +++ b/nano/node/active_elections.hpp @@ -123,10 +123,6 @@ public: int64_t vacancy (nano::election_behavior behavior) const; std::function vacancy_update{ [] () {} }; - 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 &); - nano::container_info container_info () const; private: @@ -139,8 +135,7 @@ private: std::vector> list_active_impl (std::size_t) const; void activate_successors (nano::secure::transaction const &, std::shared_ptr const & block); void notify_observers (nano::secure::transaction const &, nano::election_status const & status, std::vector const & votes) const; - void block_cemented_callback (nano::secure::transaction const &, std::shared_ptr const & block, nano::block_hash const & confirmation_root); - void block_already_cemented_callback (nano::block_hash const & hash); + void block_cemented (nano::secure::transaction const &, std::shared_ptr const & block, nano::block_hash const & confirmation_root, std::shared_ptr const & source_election); private: // Dependencies active_elections_config const & config; @@ -157,12 +152,6 @@ public: mutable nano::mutex mutex{ mutex_identifier (mutexes::active) }; private: - 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 - std::chrono::seconds const election_time_to_live; - /** Keeps track of number of elections by election behavior (normal, hinted, optimistic) */ nano::enum_array count_by_behavior{}; diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 48b856a6..a8a392db 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -35,18 +35,16 @@ nano::election::election (nano::node & node_a, std::shared_ptr cons last_blocks.emplace (block_a->hash (), block_a); } -void nano::election::confirm_once (nano::unique_lock & lock_a) +void nano::election::confirm_once (nano::unique_lock & lock) { - debug_assert (lock_a.owns_lock ()); + debug_assert (lock.owns_lock ()); + debug_assert (!mutex.try_lock ()); - // This must be kept above the setting of election state, as dependent confirmed elections require up to date changes to election_winner_details - nano::unique_lock election_winners_lk{ node.active.election_winner_details_mutex }; - auto just_confirmed = state_m != nano::election_state::confirmed; + bool just_confirmed = state_m != nano::election_state::confirmed; state_m = nano::election_state::confirmed; - if (just_confirmed && (node.active.election_winner_details.count (status.winner->hash ()) == 0)) + + if (just_confirmed) { - node.active.election_winner_details.emplace (status.winner->hash (), shared_from_this ()); - election_winners_lk.unlock (); status.election_end = std::chrono::duration_cast (std::chrono::system_clock::now ().time_since_epoch ()); status.election_duration = std::chrono::duration_cast (std::chrono::steady_clock::now () - election_start); status.confirmation_request_count = confirmation_request_count; @@ -62,11 +60,11 @@ void nano::election::confirm_once (nano::unique_lock & lock_a) nano::log::arg{ "qualified_root", qualified_root }, nano::log::arg{ "status", current_status_locked () }); - lock_a.unlock (); + node.confirming_set.add (status_l.winner->hash (), shared_from_this ()); - node.election_workers.push_task ([node_l = node.shared (), status_l, confirmation_action_l = confirmation_action] () { - node_l->process_confirmed (status_l); + lock.unlock (); + node.election_workers.push_task ([status_l, confirmation_action_l = confirmation_action] () { if (confirmation_action_l) { confirmation_action_l (status_l.winner); @@ -76,7 +74,7 @@ void nano::election::confirm_once (nano::unique_lock & lock_a) else { node.stats.inc (nano::stat::type::election, nano::stat::detail::confirm_once_failed); - lock_a.unlock (); + lock.unlock (); } } @@ -416,6 +414,7 @@ void nano::election::confirm_if_quorum (nano::unique_lock & lock_a) if (final_weight >= node.online_reps.delta ()) { confirm_once (lock_a); + debug_assert (!lock_a.owns_lock ()); } } } @@ -429,6 +428,7 @@ void nano::election::try_confirm (nano::block_hash const & hash) if (!confirmed_locked ()) { confirm_once (election_lock); + debug_assert (!election_lock.owns_lock ()); } } } diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 5871f2cf..b84f7539 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1172,7 +1172,6 @@ void nano::node::process_confirmed (nano::election_status const & status_a, uint stats.inc (nano::stat::type::process_confirmed, nano::stat::detail::timeout); // Do some cleanup due to this block never being processed by confirmation height processor - active.remove_election_winner_details (hash); } } diff --git a/nano/slow_test/node.cpp b/nano/slow_test/node.cpp index 4e4781d4..a51881b6 100644 --- a/nano/slow_test/node.cpp +++ b/nano/slow_test/node.cpp @@ -717,7 +717,6 @@ TEST (confirmation_height, many_accounts_single_confirmation) ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), 0); ASSERT_TIMELY_EQ (40s, (node->ledger.cemented_count () - 1), node->stats.count (nano::stat::type::confirmation_observer, nano::stat::dir::out)); - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, many_accounts_many_confirmations) @@ -792,8 +791,6 @@ TEST (confirmation_height, many_accounts_many_confirmations) ASSERT_EQ (cemented_count, node->ledger.cemented_count ()); ASSERT_TIMELY_EQ (20s, (node->ledger.cemented_count () - 1), node->stats.count (nano::stat::type::confirmation_observer, nano::stat::dir::out)); - - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, long_chains) @@ -939,7 +936,6 @@ TEST (confirmation_height, long_chains) ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), 0); ASSERT_TIMELY_EQ (40s, (node->ledger.cemented_count () - 1), node->stats.count (nano::stat::type::confirmation_observer, nano::stat::dir::out)); - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, dynamic_algorithm) @@ -987,7 +983,6 @@ TEST (confirmation_height, dynamic_algorithm) ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in), num_blocks); ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_bounded, nano::stat::dir::in), 1); ASSERT_EQ (node->ledger.stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed_unbounded, nano::stat::dir::in), num_blocks - 1); - ASSERT_TIMELY_EQ (10s, node->active.election_winner_details_size (), 0); } TEST (confirmation_height, many_accounts_send_receive_self) @@ -1118,10 +1113,6 @@ TEST (confirmation_height, many_accounts_send_receive_self) } system.deadline_set (60s); - while (node->active.election_winner_details_size () > 0) - { - ASSERT_NO_ERROR (system.poll ()); - } } // Same as the many_accounts_send_receive_self test, except works on the confirmation height processor directly