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

@@ -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<nano::mutex> 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));

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::vote> (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, nano::vote::timestamp_min * 2, 0, std::vector<nano::block_hash>{ send2->hash () }));
 	// Pretend we've waited the timeout
-	nano::unique_lock<nano::mutex> 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::vote> (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, nano::vote::timestamp_min * 1, 0, std::vector<nano::block_hash>{ send2->hash () });
-	{
-		nano::lock_guard<nano::mutex> 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 ());

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<nano::block> const & block, std::shared_ptr<nano::election> 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<nano::election> const & election)
-{
-	nano::unique_lock<nano::mutex> 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<nano::block> 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<nano::election> election, nano::election_status_type status_type)
-{
-	nano::unique_lock<nano::mutex> 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<nano::election> 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<nano::vote_with_weight_info> 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::election_status_type> nano::active_transactions::confirm_b
 	boost::optional<nano::election_status_type> status_type;
 	if (election)
 	{
-		nano::unique_lock<nano::mutex> 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<nano::mutex> 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<nano::election_status_type>{};
-		}
+		status_type = election->try_confirm (hash);
 	}
 	else
 	{

nano/node/active_transactions.hpp

@@ -203,10 +203,8 @@ class active_transactions final
 	void handle_final_votes_confirmation (std::shared_ptr<nano::block> 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<nano::block> const & block, std::shared_ptr<nano::election> election, nano::election_status_type status);
 	void activate_successors (const nano::account & account, std::shared_ptr<nano::block> const & block, nano::store::read_transaction const & transaction);
-	void update_recently_cemented (std::shared_ptr<nano::election> const & election);
 	void handle_block_confirmation (nano::store::read_transaction const & transaction, std::shared_ptr<nano::block> 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<nano::election> election, nano::election_status_type status);
-	void notify_observers (std::shared_ptr<nano::election> 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<nano::vote_with_weight_info> 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;

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<nano::mutex> guard{ mutex };
+	return last_votes[account];
+}
+
+void nano::election::set_last_vote (nano::account const & account, nano::vote_info vote_info)
+{
+	nano::lock_guard<nano::mutex> guard{ mutex };
+	last_votes[account] = vote_info;
+}
+
+nano::election_status nano::election::get_status () const
+{
+	nano::lock_guard<nano::mutex> 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<nano::mutex> & lock_a)
 	}
 }
 
+boost::optional<nano::election_status_type> nano::election::try_confirm (nano::block_hash const & hash)
+{
+	boost::optional<nano::election_status_type> status_type;
+	nano::unique_lock<nano::mutex> 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<nano::election_status_type>{};
+	}
+	return status_type;
+}
+
+nano::election_status nano::election::set_status_type (nano::election_status_type status_type)
+{
+	nano::unique_lock<nano::mutex> 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;

nano/node/election.hpp

@@ -141,12 +141,17 @@ class election final : public std::enable_shared_from_this<nano::election>
 	bool publish (std::shared_ptr<nano::block> const & block_a);
 	// Confirm this block if quorum is met
 	void confirm_if_quorum (nano::unique_lock<nano::mutex> &);
+	boost::optional<nano::election_status_type> 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;