From f30ec1162212f6ec84cad8a6f6e0fb8a8d2ebc82 Mon Sep 17 00:00:00 2001 From: gr0vity-dev <85646666+gr0vity-dev@users.noreply.github.com> Date: Thu, 19 Oct 2023 16:48:20 +0200 Subject: [PATCH] Encapsulate election mutex (#4310) * Encapsulate mutex of election class * fix issue in notify_observers() where the lock was missing for `auto status = election->status;` * Update unit tests * remove the need to hold the lock during notify_observers * make use of `set_status_type ()` returning the updated election status --- nano/core_test/confirmation_solicitor.cpp | 3 +- nano/core_test/ledger.cpp | 19 ++++---- nano/node/active_transactions.cpp | 47 +++----------------- nano/node/active_transactions.hpp | 4 +- nano/node/election.cpp | 53 +++++++++++++++++++++++ nano/node/election.hpp | 5 +++ 6 files changed, 75 insertions(+), 56 deletions(-) diff --git a/nano/core_test/confirmation_solicitor.cpp b/nano/core_test/confirmation_solicitor.cpp index 0ad18cb906..578b575a78 100644 --- a/nano/core_test/confirmation_solicitor.cpp +++ b/nano/core_test/confirmation_solicitor.cpp @@ -137,8 +137,7 @@ TEST (confirmation_solicitor, bypass_max_requests_cap) // Add a vote for something else, not the winner for (auto const & rep : representatives) { - nano::lock_guard guard (election->mutex); - election->last_votes[rep.account] = { std::chrono::steady_clock::now (), 1, 1 }; + election->set_last_vote (rep.account, { std::chrono::steady_clock::now (), 1, 1 }); } ASSERT_FALSE (solicitor.add (*election)); ASSERT_FALSE (solicitor.broadcast (*election)); diff --git a/nano/core_test/ledger.cpp b/nano/core_test/ledger.cpp index b2d08ba9cf..db323513d5 100644 --- a/nano/core_test/ledger.cpp +++ b/nano/core_test/ledger.cpp @@ -1008,15 +1008,15 @@ TEST (votes, add_existing) ASSERT_TIMELY (5s, node1.active.active (*send2)); auto vote2 (std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, nano::vote::timestamp_min * 2, 0, std::vector{ send2->hash () })); // Pretend we've waited the timeout - nano::unique_lock lock{ election1->mutex }; - election1->last_votes[nano::dev::genesis_key.pub].time = std::chrono::steady_clock::now () - std::chrono::seconds (20); - lock.unlock (); + auto vote_info1 = election1->get_last_vote (nano::dev::genesis_key.pub); + vote_info1.time = std::chrono::steady_clock::now () - std::chrono::seconds (20); + election1->set_last_vote (nano::dev::genesis_key.pub, vote_info1); ASSERT_EQ (nano::vote_code::vote, node1.active.vote (vote2)); ASSERT_EQ (nano::vote::timestamp_min * 2, election1->last_votes[nano::dev::genesis_key.pub].timestamp); // Also resend the old vote, and see if we respect the timestamp - lock.lock (); - election1->last_votes[nano::dev::genesis_key.pub].time = std::chrono::steady_clock::now () - std::chrono::seconds (20); - lock.unlock (); + auto vote_info2 = election1->get_last_vote (nano::dev::genesis_key.pub); + vote_info2.time = std::chrono::steady_clock::now () - std::chrono::seconds (20); + election1->set_last_vote (nano::dev::genesis_key.pub, vote_info2); ASSERT_EQ (nano::vote_code::replay, node1.active.vote (vote1)); ASSERT_EQ (nano::vote::timestamp_min * 2, election1->votes ()[nano::dev::genesis_key.pub].timestamp); auto votes (election1->votes ()); @@ -1061,10 +1061,9 @@ TEST (votes, add_old) .build_shared (); node1.work_generate_blocking (*send2); auto vote2 = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, nano::vote::timestamp_min * 1, 0, std::vector{ send2->hash () }); - { - nano::lock_guard lock{ election1->mutex }; - election1->last_votes[nano::dev::genesis_key.pub].time = std::chrono::steady_clock::now () - std::chrono::seconds (20); - } + auto vote_info = election1->get_last_vote (nano::dev::genesis_key.pub); + vote_info.time = std::chrono::steady_clock::now () - std::chrono::seconds (20); + election1->set_last_vote (nano::dev::genesis_key.pub, vote_info); node1.vote_processor.vote_blocking (vote2, channel); ASSERT_EQ (2, election1->votes ().size ()); auto votes (election1->votes ()); diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 5079cb2cd8..2e15791551 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -134,7 +134,7 @@ void nano::active_transactions::process_active_confirmation (nano::store::read_t 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 (); - update_recently_cemented (election); + recently_cemented.put (election->get_status ()); nano::account account; nano::uint128_t amount (0); @@ -144,14 +144,9 @@ void nano::active_transactions::handle_confirmation (nano::store::read_transacti handle_block_confirmation (transaction, block, hash, account, amount, is_state_send, is_state_epoch, pending_account); - update_election_status (election, status_type); - notify_observers (election, account, amount, is_state_send, is_state_epoch, pending_account); -} - -void nano::active_transactions::update_recently_cemented (std::shared_ptr const & election) -{ - nano::unique_lock election_lk{ election->mutex }; - recently_cemented.put (election->status); + auto status = election->set_status_type (status_type); + auto votes = election->votes_with_weight (); + notify_observers (status, votes, account, amount, is_state_send, is_state_epoch, pending_account); } void nano::active_transactions::handle_block_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block, nano::block_hash const & hash, nano::account & account, nano::uint128_t & amount, bool & is_state_send, bool & is_state_epoch, nano::account & pending_account) @@ -161,18 +156,8 @@ void nano::active_transactions::handle_block_confirmation (nano::store::read_tra node.process_confirmed_data (transaction, block, hash, account, amount, is_state_send, is_state_epoch, pending_account); } -void nano::active_transactions::update_election_status (std::shared_ptr election, nano::election_status_type status_type) -{ - nano::unique_lock election_lk{ election->mutex }; - election->status.type = status_type; - election->status.confirmation_request_count = election->confirmation_request_count; -} - -void nano::active_transactions::notify_observers (std::shared_ptr const & election, nano::account const & account, nano::uint128_t amount, bool is_state_send, bool is_state_epoch, nano::account const & pending_account) +void nano::active_transactions::notify_observers (nano::election_status const & status, std::vector const & votes, nano::account const & account, nano::uint128_t amount, bool is_state_send, bool is_state_epoch, nano::account const & pending_account) { - auto status = election->status; - auto votes = election->votes_with_weight (); - node.observers.blocks.notify (status, votes, account, amount, is_state_send, is_state_epoch); if (amount > 0) @@ -696,27 +681,7 @@ boost::optional nano::active_transactions::confirm_b boost::optional status_type; if (election) { - nano::unique_lock election_lock{ election->mutex }; - if (election->status.winner && election->status.winner->hash () == hash) - { - if (!election->confirmed ()) - { - election->confirm_once (election_lock, nano::election_status_type::active_confirmation_height); - status_type = nano::election_status_type::active_confirmation_height; - } - else - { -#ifndef NDEBUG - nano::unique_lock election_winners_lk{ election_winner_details_mutex }; - debug_assert (election_winner_details.find (hash) != election_winner_details.cend ()); -#endif - status_type = nano::election_status_type::active_confirmed_quorum; - } - } - else - { - status_type = boost::optional{}; - } + status_type = election->try_confirm (hash); } else { diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index ff8988c498..36acd179e5 100644 --- a/nano/node/active_transactions.hpp +++ b/nano/node/active_transactions.hpp @@ -203,10 +203,8 @@ class active_transactions final 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 (const nano::account & account, std::shared_ptr const & block, nano::store::read_transaction const & transaction); - void update_recently_cemented (std::shared_ptr const & election); void handle_block_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr const & block, nano::block_hash const & hash, nano::account & account, nano::uint128_t & amount, bool & is_state_send, bool & is_state_epoch, nano::account & pending_account); - void update_election_status (std::shared_ptr election, nano::election_status_type status); - void notify_observers (std::shared_ptr const & election, nano::account const & account, nano::uint128_t amount, bool is_state_send, bool is_state_epoch, nano::account const & pending_account); + void notify_observers (nano::election_status const & status, std::vector const & votes, nano::account const & account, nano::uint128_t amount, bool is_state_send, bool is_state_epoch, nano::account const & pending_account); private: // Dependencies nano::confirmation_height_processor & confirmation_height_processor; diff --git a/nano/node/election.cpp b/nano/node/election.cpp index 03e7e68cc9..ae477559f0 100644 --- a/nano/node/election.cpp +++ b/nano/node/election.cpp @@ -188,6 +188,24 @@ void nano::election::broadcast_vote () } } +nano::vote_info nano::election::get_last_vote (nano::account const & account) +{ + nano::lock_guard guard{ mutex }; + return last_votes[account]; +} + +void nano::election::set_last_vote (nano::account const & account, nano::vote_info vote_info) +{ + nano::lock_guard guard{ mutex }; + last_votes[account] = vote_info; +} + +nano::election_status nano::election::get_status () const +{ + nano::lock_guard guard{ mutex }; + return status; +} + bool nano::election::transition_time (nano::confirmation_solicitor & solicitor_a) { bool result = false; @@ -356,6 +374,41 @@ void nano::election::confirm_if_quorum (nano::unique_lock & lock_a) } } +boost::optional 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 ()) + { + 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; + } + } + 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; +} + void nano::election::log_votes (nano::tally_t const & tally_a, std::string const & prefix_a) const { std::stringstream tally; diff --git a/nano/node/election.hpp b/nano/node/election.hpp index 0ee0cd9a24..b28337dacf 100644 --- a/nano/node/election.hpp +++ b/nano/node/election.hpp @@ -141,12 +141,17 @@ 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); /** * Broadcasts vote for the current winner of this election * Checks if sufficient amount of time (`vote_generation_interval`) passed since the last vote generation */ void broadcast_vote (); + nano::vote_info get_last_vote (nano::account const & account); + void set_last_vote (nano::account const & account, nano::vote_info vote_info); + nano::election_status get_status () const; private: // Dependencies nano::node & node;