From 152f4449fa745106ae68acd015d7d1cc0b72893c Mon Sep 17 00:00:00 2001 From: Colin LeMahieu Date: Wed, 20 Mar 2024 10:09:59 +0000 Subject: [PATCH] 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 | 140 +++++++------------------ nano/node/active_transactions.hpp | 8 +- nano/secure/common.hpp | 16 +-- 4 files changed, 52 insertions(+), 120 deletions(-) diff --git a/nano/core_test/confirmation_height.cpp b/nano/core_test/confirmation_height.cpp index a150d40e..352d04d4 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 d9d88918..350b4c28 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -80,75 +80,44 @@ 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; } - - 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 ()) - { - 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); - } - } -} - -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 (); + 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; + + // 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::notify_observers (nano::store::read_transaction const & transaction, nano::election_status const & status, std::vector const & votes) @@ -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 9f56d5ca..7323bf51 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -160,7 +160,6 @@ public: 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 @@ public: 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 @@ private: * 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 0928289f..d585e1b2 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 ();