From 152f4449fa745106ae68acd015d7d1cc0b72893c Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Wed, 20 Mar 2024 10:09:59 +0000 Subject: [PATCH 1/2] Simplify active_transactions::block_cemented_callback Simplifies and merges logic that was spread across multiple functions and coupled with nano::election. Behaviour changed with respect to callbacks that were *not* called for confirmed blocks in certain circumstances. This was a purely synthetic case where an election was explicitly confirmed in code but didn't have an associated election. In other cases where there was no election yet the block was confirmed indirectly, the callbacks were still called. This behaviour change calls the callback for all blocks that are confirmed. Behaviour changed with respect to entries in the recently_cemented list. Previously blocks implicitly confirmed were not placed in this list yet were reported through callbacks. This behaviour was arbitrary and could be confusing. Now all blocks that are reported through callbacks are placed in the recently_cemented list. --- nano/core_test/confirmation_height.cpp | 8 +- nano/node/active_transactions.cpp | 136 +++++++------------------ nano/node/active_transactions.hpp | 8 +- nano/secure/common.hpp | 16 +-- 4 files changed, 50 insertions(+), 118 deletions(-) diff --git a/nano/core_test/confirmation_height.cpp b/nano/core_test/confirmation_height.cpp index a150d40e44..352d04d4b7 100644 --- a/nano/core_test/confirmation_height.cpp +++ b/nano/core_test/confirmation_height.cpp @@ -1444,8 +1444,9 @@ TEST (confirmation_height, pending_observer_callbacks) node->confirmation_height_processor.add (send1); - // Confirm the callback is not called under this circumstance because there is no election information - ASSERT_TIMELY (10s, node->stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out) == 1 && node->ledger.stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::all, nano::stat::dir::out) == 1); + // Callback is performed for all blocks that are confirmed + ASSERT_TIMELY_EQ (5s, 2, node->stats.count (nano::stat::type::http_callback, nano::stat::detail::http_callback, nano::stat::dir::out)) + ASSERT_TIMELY_EQ (5s, 2, node->ledger.stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::all, nano::stat::dir::out)); ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, nano::stat::detail::blocks_confirmed, nano::stat::dir::in)); ASSERT_EQ (2, node->stats.count (nano::stat::type::confirmation_height, get_stats_detail (mode_a), nano::stat::dir::in)); @@ -1528,7 +1529,8 @@ TEST (confirmation_height, callback_confirmed_history) ASSERT_TIMELY_EQ (10s, node->active.size (), 0); ASSERT_TIMELY_EQ (10s, node->stats.count (nano::stat::type::confirmation_observer, nano::stat::detail::active_quorum, nano::stat::dir::out), 1); - ASSERT_EQ (1, node->active.recently_cemented.list ().size ()); + // Each block that's confirmed is in the recently_cemented history + ASSERT_EQ (2, node->active.recently_cemented.list ().size ()); ASSERT_TRUE (node->active.empty ()); // Confirm the callback is not called under this circumstance diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index d9d8891848..350b4c28bb 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -80,77 +80,46 @@ void nano::active_transactions::stop () clear (); } -void nano::active_transactions::block_cemented_callback (std::shared_ptr const & block_a) +void nano::active_transactions::block_cemented_callback (std::shared_ptr const & block) { - auto status_type = election_status (block_a); - - if (!status_type) - return; - - auto transaction = node.store.tx_begin_read (); - switch (*status_type) + if (auto election_l = election (block->qualified_root ())) { - case nano::election_status_type::inactive_confirmation_height: - process_inactive_confirmation (transaction, block_a); - break; - - default: - process_active_confirmation (transaction, block_a, *status_type); - break; + election_l->try_confirm (block->hash ()); } - - handle_final_votes_confirmation (block_a, transaction, *status_type); -} - -boost::optional nano::active_transactions::election_status (std::shared_ptr const & block) -{ - boost::optional status_type; - - if (!confirmation_height_processor.is_processing_added_block (block->hash ())) + auto election = remove_election_winner_details (block->hash ()); + nano::election_status status; + std::vector votes; + status.winner = block; + if (election) { - status_type = confirm_block (block); + status = election->get_status (); + votes = election->votes_with_weight (); + } + if (confirmation_height_processor.is_processing_added_block (block->hash ())) + { + status.type = nano::election_status_type::active_confirmed_quorum; + } + else if (election) + { + status.type = nano::election_status_type::active_confirmation_height; } else { - status_type = nano::election_status_type::active_confirmed_quorum; + status.type = nano::election_status_type::inactive_confirmation_height; } + recently_cemented.put (status); + auto transaction = node.store.tx_begin_read (); + notify_observers (transaction, status, votes); + bool cemented_bootstrap_count_reached = node.ledger.cache.cemented_count >= node.ledger.bootstrap_weight_max_blocks; + bool was_active = status.type == nano::election_status_type::active_confirmed_quorum || status.type == nano::election_status_type::active_confirmation_height; - return status_type; -} - -void nano::active_transactions::process_inactive_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block) -{ - nano::election_status status{ block, 0, 0, std::chrono::duration_cast (std::chrono::system_clock::now ().time_since_epoch ()), std::chrono::duration_values::zero (), 0, 1, 0, nano::election_status_type::inactive_confirmation_height }; - notify_observers (transaction, status, {}); -} - -void nano::active_transactions::process_active_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block, nano::election_status_type status_type) -{ - auto hash (block->hash ()); - nano::unique_lock election_winners_lk{ election_winner_details_mutex }; - auto existing = election_winner_details.find (hash); - if (existing != election_winner_details.end ()) + // Next-block activations are only done for blocks with previously active elections + if (cemented_bootstrap_count_reached && was_active) { - auto election = existing->second; - election_winner_details.erase (hash); - election_winners_lk.unlock (); - if (election->confirmed () && election->winner ()->hash () == hash) - { - handle_confirmation (transaction, block, election, status_type); - } + activate_successors (transaction, block); } } -void nano::active_transactions::handle_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block, std::shared_ptr election, nano::election_status_type status_type) -{ - nano::block_hash hash = block->hash (); - recently_cemented.put (election->get_status ()); - - auto status = election->set_status_type (status_type); - auto votes = election->votes_with_weight (); - notify_observers (transaction, status, votes); -} - void nano::active_transactions::notify_observers (nano::store::read_transaction const & transaction, nano::election_status const & status, std::vector const & votes) { auto block = status.winner; @@ -170,19 +139,6 @@ void nano::active_transactions::notify_observers (nano::store::read_transaction } } -void nano::active_transactions::handle_final_votes_confirmation (std::shared_ptr const & block, nano::store::read_transaction const & transaction, nano::election_status_type status) -{ - auto account = block->account (); - bool cemented_bootstrap_count_reached = node.ledger.cache.cemented_count >= node.ledger.bootstrap_weight_max_blocks; - bool was_active = status == nano::election_status_type::active_confirmed_quorum || status == nano::election_status_type::active_confirmation_height; - - // Next-block activations are only done for blocks with previously active elections - if (cemented_bootstrap_count_reached && was_active) - { - activate_successors (transaction, block); - } -} - void nano::active_transactions::activate_successors (nano::store::read_transaction const & transaction, std::shared_ptr const & block) { node.scheduler.priority.activate (block->account (), transaction); @@ -200,10 +156,17 @@ void nano::active_transactions::add_election_winner_details (nano::block_hash co election_winner_details.emplace (hash_a, election_a); } -void nano::active_transactions::remove_election_winner_details (nano::block_hash const & hash_a) +std::shared_ptr nano::active_transactions::remove_election_winner_details (nano::block_hash const & hash_a) { nano::lock_guard guard{ election_winner_details_mutex }; - election_winner_details.erase (hash_a); + 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); + } + return result; } void nano::active_transactions::block_already_cemented_callback (nano::block_hash const & hash_a) @@ -653,33 +616,6 @@ bool nano::active_transactions::publish (std::shared_ptr const & bl return result; } -// Returns the type of election status requiring callbacks calling later -boost::optional nano::active_transactions::confirm_block (std::shared_ptr const & block_a) -{ - auto const hash = block_a->hash (); - std::shared_ptr election = nullptr; - { - nano::lock_guard guard{ mutex }; - auto existing = blocks.find (hash); - if (existing != blocks.end ()) - { - election = existing->second; - } - } - - boost::optional status_type; - if (election) - { - status_type = election->try_confirm (hash); - } - else - { - status_type = nano::election_status_type::inactive_confirmation_height; - } - - return status_type; -} - void nano::active_transactions::add_vote_cache (nano::block_hash const & hash, std::shared_ptr const vote) { if (node.ledger.weight (vote->account) > node.minimum_principal_weight ()) diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index 9f56d5cad6..7323bf51dc 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -160,7 +160,6 @@ class active_transactions final bool empty () const; std::size_t size () const; bool publish (std::shared_ptr const &); - boost::optional confirm_block (std::shared_ptr const &); void block_cemented_callback (std::shared_ptr const &); void block_already_cemented_callback (nano::block_hash const &); @@ -177,7 +176,7 @@ class active_transactions final std::size_t election_winner_details_size (); void add_election_winner_details (nano::block_hash const &, std::shared_ptr const &); - void remove_election_winner_details (nano::block_hash const &); + std::shared_ptr remove_election_winner_details (nano::block_hash const &); private: // Erase elections if we're over capacity @@ -195,11 +194,6 @@ class active_transactions final * TODO: Should be moved to `vote_cache` class */ void add_vote_cache (nano::block_hash const & hash, std::shared_ptr vote); - boost::optional election_status (std::shared_ptr const & block); - void process_inactive_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block); - void process_active_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block, nano::election_status_type status); - void handle_final_votes_confirmation (std::shared_ptr const & block, nano::store::read_transaction const & transaction, nano::election_status_type status); - void handle_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block, std::shared_ptr election, nano::election_status_type status); void activate_successors (nano::store::read_transaction const & transaction, std::shared_ptr const & block); void notify_observers (nano::store::read_transaction const & transaction, nano::election_status const & status, std::vector const & votes); diff --git a/nano/secure/common.hpp b/nano/secure/common.hpp index 0928289fd0..d585e1b222 100644 --- a/nano/secure/common.hpp +++ b/nano/secure/common.hpp @@ -360,14 +360,14 @@ class election_status final { public: std::shared_ptr winner; - nano::amount tally; - nano::amount final_tally; - std::chrono::milliseconds election_end; - std::chrono::milliseconds election_duration; - unsigned confirmation_request_count; - unsigned block_count; - unsigned voter_count; - election_status_type type; + nano::amount tally{ 0 }; + nano::amount final_tally{ 0 }; + std::chrono::milliseconds election_end{ std::chrono::duration_cast (std::chrono::system_clock::now ().time_since_epoch ()) }; + std::chrono::milliseconds election_duration{ std::chrono::duration_values::zero () }; + unsigned confirmation_request_count{ 0 }; + unsigned block_count{ 0 }; + unsigned voter_count{ 0 }; + election_status_type type{ nano::election_status_type::inactive_confirmation_height }; }; nano::wallet_id random_wallet_id (); From e4b491754ac098ddfc9295c982a41b7a163d8268 Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Wed, 20 Mar 2024 10:36:59 +0000 Subject: [PATCH 2/2] Remove unused code related to trying to externally set election status --- nano/node/election.cpp | 35 ++++++----------------------------- nano/node/election.hpp | 7 +++---- 2 files changed, 9 insertions(+), 33 deletions(-) diff --git a/nano/node/election.cpp b/nano/node/election.cpp index d179dabdb7..a41136e4c0 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -34,7 +34,7 @@ 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, nano::election_status_type type_a) +void nano::election::confirm_once (nano::unique_lock & lock_a) { debug_assert (lock_a.owns_lock ()); @@ -51,7 +51,6 @@ void nano::election::confirm_once (nano::unique_lock & lock_a, nano status.confirmation_request_count = confirmation_request_count; status.block_count = nano::narrow_cast (last_blocks.size ()); status.voter_count = nano::narrow_cast (last_votes.size ()); - status.type = type_a; auto const status_l = status; node.active.recently_confirmed.put (qualified_root, status_l.winner->hash ()); @@ -403,44 +402,22 @@ void nano::election::confirm_if_quorum (nano::unique_lock & lock_a) } if (final_weight >= node.online_reps.delta ()) { - confirm_once (lock_a, nano::election_status_type::active_confirmed_quorum); + confirm_once (lock_a); } } } -boost::optional nano::election::try_confirm (nano::block_hash const & hash) +void nano::election::try_confirm (nano::block_hash const & hash) { - boost::optional status_type; nano::unique_lock election_lock{ mutex }; auto winner = status.winner; if (winner && winner->hash () == hash) { - // Determine if the block was confirmed explicitly via election confirmation or implicitly via confirmation height if (!confirmed_locked ()) { - confirm_once (election_lock, nano::election_status_type::active_confirmation_height); - status_type = nano::election_status_type::active_confirmation_height; - } - else - { - status_type = nano::election_status_type::active_confirmed_quorum; + confirm_once (election_lock); } } - else - { - status_type = boost::optional{}; - } - return status_type; -} - -nano::election_status nano::election::set_status_type (nano::election_status_type status_type) -{ - nano::unique_lock election_lk{ mutex }; - status.type = status_type; - status.confirmation_request_count = confirmation_request_count; - nano::election_status status_l{ status }; - election_lk.unlock (); - return status_l; } std::shared_ptr nano::election::find (nano::block_hash const & hash_a) const @@ -709,11 +686,11 @@ bool nano::election::replace_by_weight (nano::unique_lock & lock_a, return replaced; } -void nano::election::force_confirm (nano::election_status_type type_a) +void nano::election::force_confirm () { release_assert (node.network_params.network.is_dev_network ()); nano::unique_lock lock{ mutex }; - confirm_once (lock, type_a); + confirm_once (lock); } std::unordered_map> nano::election::blocks () const diff --git a/nano/node/election.hpp b/nano/node/election.hpp index 63e7582785..59cf2ea9e0 100644 --- a/nano/node/election.hpp +++ b/nano/node/election.hpp @@ -146,8 +146,7 @@ class election final : public std::enable_shared_from_this bool publish (std::shared_ptr const & block_a); // Confirm this block if quorum is met void confirm_if_quorum (nano::unique_lock &); - boost::optional try_confirm (nano::block_hash const & hash); - nano::election_status set_status_type (nano::election_status_type status_type); + void try_confirm (nano::block_hash const & hash); /** * Broadcasts vote for the current winner of this election @@ -173,7 +172,7 @@ class election final : public std::enable_shared_from_this bool confirmed_locked () const; nano::election_extended_status current_status_locked () const; // lock_a does not own the mutex on return - void confirm_once (nano::unique_lock & lock_a, nano::election_status_type = nano::election_status_type::active_confirmed_quorum); + void confirm_once (nano::unique_lock & lock_a); bool broadcast_block_predicate () const; void broadcast_block (nano::confirmation_solicitor &); void send_confirm_req (nano::confirmation_solicitor &); @@ -217,7 +216,7 @@ class election final : public std::enable_shared_from_this friend class confirmation_solicitor; public: // Only used in tests - void force_confirm (nano::election_status_type = nano::election_status_type::active_confirmed_quorum); + void force_confirm (); std::unordered_map votes () const; std::unordered_map> blocks () const;