Process cemented elections with exclusive lock to avoid races

nano/node/active_elections.cpp

@@ -31,11 +31,24 @@ nano::active_elections::active_elections (nano::node & node_a, nano::confirming_
 
 	// Cementing blocks might implicitly confirm dependent elections
 	confirming_set.batch_cemented.add ([this] (auto const & cemented) {
-		auto transaction = node.ledger.tx_begin_read ();
-		for (auto const & [block, confirmation_root, source_election] : cemented)
+		std::deque<block_cemented_result> results;
 		{
-			transaction.refresh_if_needed ();
-			block_cemented (transaction, block, confirmation_root, source_election);
+			// Process all cemented blocks while holding the lock to avoid races where an election for a block that is already cemented is inserted
+			nano::lock_guard<nano::mutex> guard{ mutex };
+			for (auto const & [block, confirmation_root, source_election] : cemented)
+			{
+				auto result = block_cemented (block, confirmation_root, source_election);
+				results.push_back (result);
+			}
+		}
+		{
+			// TODO: This could be offloaded to a separate notification worker, profiling is needed
+			auto transaction = node.ledger.tx_begin_read ();
+			for (auto const & [status, votes] : results)
+			{
+				transaction.refresh_if_needed ();
+				notify_observers (transaction, status, votes);
+			}
 		}
 	});
 
@@ -83,16 +96,17 @@ void nano::active_elections::stop ()
 	clear ();
 }
 
-void nano::active_elections::block_cemented (nano::secure::transaction const & transaction, std::shared_ptr<nano::block> const & block, nano::block_hash const & confirmation_root, std::shared_ptr<nano::election> const & source_election)
+auto nano::active_elections::block_cemented (std::shared_ptr<nano::block> const & block, nano::block_hash const & confirmation_root, std::shared_ptr<nano::election> const & source_election) -> block_cemented_result
 {
+	debug_assert (!mutex.try_lock ());
 	debug_assert (node.block_confirmed (block->hash ()));
 
 	// Dependent elections are implicitly confirmed when their block is cemented
-	auto dependend_election = election (block->qualified_root ());
+	auto dependend_election = election_impl (block->qualified_root ());
 	if (dependend_election)
 	{
 		node.stats.inc (nano::stat::type::active_elections, nano::stat::detail::confirm_dependent);
-		dependend_election->try_confirm (block->hash ());
+		dependend_election->try_confirm (block->hash ()); // TODO: This should either confirm or cancel the election
 	}
 
 	nano::election_status status;
@@ -126,7 +140,7 @@ void nano::active_elections::block_cemented (nano::secure::transaction const & t
 	nano::log::arg{ "confirmation_root", confirmation_root },
 	nano::log::arg{ "source_election", source_election });
 
-	notify_observers (transaction, status, votes);
+	return { status, votes };
 }
 
 void nano::active_elections::notify_observers (nano::secure::transaction const & transaction, nano::election_status const & status, std::vector<nano::vote_with_weight_info> const & votes) const
@@ -443,11 +457,17 @@ bool nano::active_elections::active (nano::block const & block_a) const
 	return roots.get<tag_root> ().find (block_a.qualified_root ()) != roots.get<tag_root> ().end ();
 }
 
-std::shared_ptr<nano::election> nano::active_elections::election (nano::qualified_root const & root_a) const
+std::shared_ptr<nano::election> nano::active_elections::election (nano::qualified_root const & root) const
 {
-	std::shared_ptr<nano::election> result;
 	nano::lock_guard<nano::mutex> lock{ mutex };
-	auto existing = roots.get<tag_root> ().find (root_a);
+	return election_impl (root);
+}
+
+std::shared_ptr<nano::election> nano::active_elections::election_impl (nano::qualified_root const & root) const
+{
+	debug_assert (!mutex.try_lock ());
+	std::shared_ptr<nano::election> result;
+	auto existing = roots.get<tag_root> ().find (root);
 	if (existing != roots.get<tag_root> ().end ())
 	{
 		result = existing->election;

nano/node/active_elections.hpp

@@ -105,7 +105,7 @@ class active_elections final
 	bool active (nano::qualified_root const &) const;
 	std::shared_ptr<nano::election> election (nano::qualified_root const &) const;
 	// Returns a list of elections sorted by difficulty
-	std::vector<std::shared_ptr<nano::election>> list_active (std::size_t = std::numeric_limits<std::size_t>::max ());
+	std::vector<std::shared_ptr<nano::election>> list_active (std::size_t max_count = std::numeric_limits<std::size_t>::max ());
 	bool erase (nano::block const &);
 	bool erase (nano::qualified_root const &);
 	bool empty () const;
@@ -133,11 +133,13 @@ class active_elections final
 	void request_confirm (nano::unique_lock<nano::mutex> &);
 	// Erase all blocks from active and, if not confirmed, clear digests from network filters
 	void cleanup_election (nano::unique_lock<nano::mutex> & lock_a, std::shared_ptr<nano::election>);
-	nano::stat::type completion_type (nano::election const & election) const;
-	// Returns a list of elections sorted by difficulty, mutex must be locked
-	std::vector<std::shared_ptr<nano::election>> list_active_impl (std::size_t) const;
+
+	using block_cemented_result = std::pair<nano::election_status, std::vector<nano::vote_with_weight_info>>;
+	block_cemented_result block_cemented (std::shared_ptr<nano::block> const & block, nano::block_hash const & confirmation_root, std::shared_ptr<nano::election> const & source_election);
 	void notify_observers (nano::secure::transaction const &, nano::election_status const & status, std::vector<nano::vote_with_weight_info> const & votes) const;
-	void block_cemented (nano::secure::transaction const &, std::shared_ptr<nano::block> const & block, nano::block_hash const & confirmation_root, std::shared_ptr<nano::election> const & source_election);
+
+	std::shared_ptr<nano::election> election_impl (nano::qualified_root const &) const;
+	std::vector<std::shared_ptr<nano::election>> list_active_impl (std::size_t max_count) const;
 
 private: // Dependencies
 	active_elections_config const & config;