Election insertion cleanup (#2890)

* Start election as passive by default, scrapping the idle state

* (Unrelated) small optimization to not run activate() twice when an account sends to itself

* Early-break with no fallthrough in election::valid_change

* Always generate votes if election inserted or ongoing

* Fix intermit failure in test due to activations

nano/core_test/election.cpp

@@ -11,11 +11,7 @@ TEST (election, construction)
 	auto & node = *system.nodes[0];
 	genesis.open->sideband_set (nano::block_sideband (nano::genesis_account, 0, nano::genesis_amount, 1, nano::seconds_since_epoch (), nano::epoch::epoch_0, false, false, false, nano::epoch::epoch_0));
 	auto election = node.active.insert (genesis.open).election;
-	ASSERT_TRUE (election->idle ());
 	election->transition_active ();
-	ASSERT_FALSE (election->idle ());
-	election->transition_passive ();
-	ASSERT_FALSE (election->idle ());
 }
 
 namespace nano

nano/core_test/request_aggregator.cpp

@@ -299,12 +299,16 @@ TEST (request_aggregator, unique)
 	ASSERT_TIMELY (3s, 1 == node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes));
 }
 
+namespace nano
+{
 TEST (request_aggregator, cannot_vote)
 {
 	nano::system system;
 	nano::node_flags flags;
 	flags.disable_request_loop = true;
 	auto & node (*system.add_node (flags));
+	// This prevents activation of blocks which are cemented
+	node.confirmation_height_processor.cemented_observers.clear ();
 	nano::genesis genesis;
 	nano::state_block_builder builder;
 	auto send1 = builder.make_block ()
@@ -378,3 +382,4 @@ TEST (request_aggregator, cannot_vote)
 	ASSERT_EQ (0, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_unknown));
 	ASSERT_TIMELY (3s, 1 == node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
 }
+}

nano/node/active_transactions.cpp

@@ -95,12 +95,17 @@ void nano::active_transactions::confirm_prioritized_frontiers (nano::transaction
 						{
 							auto block (this->node.store.block_get (transaction_a, info.head));
 							auto previous_balance (this->node.ledger.balance (transaction_a, block->previous ()));
-							auto insert_result = this->insert (block, previous_balance);
+							lk.lock ();
-							if (insert_result.inserted)
+							if (roots.get<tag_root> ().find (block->qualified_root ()) == roots.get<tag_root> ().end ())
 							{
-								insert_result.election->transition_active ();
+								auto insert_result = this->insert_impl (block, previous_balance);
-								++elections_count;
+								if (insert_result.inserted)
+								{
+									insert_result.election->transition_active ();
+									++elections_count;
+								}
 							}
+							lk.unlock ();
 						}
 					}
 				}
@@ -191,7 +196,7 @@ void nano::active_transactions::block_cemented_callback (std::shared_ptr<nano::b
 
 		// Start or vote for the next unconfirmed block in the destination account
 		auto const & destination (node.ledger.block_destination (transaction, *block_a));
-		if (!destination.is_zero ())
+		if (!destination.is_zero () && destination != account)
 		{
 			activate (destination);
 		}
@@ -648,6 +653,13 @@ nano::election_insertion_result nano::active_transactions::insert_impl (std::sha
 		{
 			result.election = existing->election;
 		}
+
+		// Votes are generated for inserted or ongoing elections if they're prioritized
+		// Non-priority elections generate votes when they gain priority in the future
+		if (result.election && result.election->prioritized ())
+		{
+			result.election->generate_votes ();
+		}
 	}
 	return result;
 }
@@ -791,17 +803,9 @@ nano::election_insertion_result nano::active_transactions::activate (nano::accou
 			if (node.ledger.can_vote (transaction, *block))
 			{
 				result = insert (block);
-				if (result.election)
+				if (result.inserted)
 				{
-					if (result.inserted)
+					result.election->transition_active ();
-					{
-						result.election->transition_active ();
-					}
-					else if (result.election->prioritized ())
-					{
-						// Generate vote for ongoing election
-						result.election->generate_votes (block->hash ());
-					}
 				}
 			}
 		}

nano/node/blockprocessor.cpp

@@ -297,15 +297,7 @@ void nano::block_processor::process_live (nano::block_hash const & hash_a, std::
 	// Start collecting quorum on block
 	if (watch_work_a || node.ledger.can_vote (node.store.tx_begin_read (), *block_a))
 	{
-		auto election = node.active.insert (block_a, process_return_a.previous_balance.number ());
+		node.active.insert (block_a, process_return_a.previous_balance.number ());
-		if (election.inserted)
-		{
-			election.election->transition_passive ();
-		}
-		else if (election.election)
-		{
-			election.election->try_generate_votes (block_a->hash ());
-		}
 	}
 
 	// Announce block contents to the network

nano/node/confirmation_height_processor.hpp

@@ -93,6 +93,7 @@ private:
 	friend class confirmation_height_many_accounts_many_confirmations_Test;
 	friend class confirmation_height_long_chains_Test;
 	friend class confirmation_height_many_accounts_single_confirmation_Test;
+	friend class request_aggregator_cannot_vote_Test;
 };
 
 std::unique_ptr<container_info_component> collect_container_info (confirmation_height_processor &, const std::string &);

nano/node/election.cpp

@@ -34,10 +34,6 @@ root (block_a->root ())
 	last_votes.emplace (node.network_params.random.not_an_account, nano::vote_info{ std::chrono::steady_clock::now (), 0, block_a->hash () });
 	blocks.emplace (block_a->hash (), block_a);
 	update_dependent ();
-	if (prioritized_a)
-	{
-		generate_votes (block_a->hash ());
-	}
 }
 
 void nano::election::confirm_once (nano::election_status_type type_a)
@@ -71,21 +67,9 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele
 	bool result = false;
 	switch (expected_a)
 	{
-		case nano::election::state_t::idle:
-			switch (desired_a)
-			{
-				case nano::election::state_t::passive:
-				case nano::election::state_t::active:
-					result = true;
-					break;
-				default:
-					break;
-			}
-			break;
 		case nano::election::state_t::passive:
 			switch (desired_a)
 			{
-				case nano::election::state_t::idle:
 				case nano::election::state_t::active:
 				case nano::election::state_t::confirmed:
 				case nano::election::state_t::expired_unconfirmed:
@@ -98,7 +82,6 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele
 		case nano::election::state_t::active:
 			switch (desired_a)
 			{
-				case nano::election::state_t::idle:
 				case nano::election::state_t::broadcasting:
 				case nano::election::state_t::confirmed:
 				case nano::election::state_t::expired_unconfirmed:
@@ -107,10 +90,10 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele
 				default:
 					break;
 			}
+			break;
 		case nano::election::state_t::broadcasting:
 			switch (desired_a)
 			{
-				case nano::election::state_t::idle:
 				case nano::election::state_t::backtracking:
 				case nano::election::state_t::confirmed:
 				case nano::election::state_t::expired_unconfirmed:
@@ -119,10 +102,10 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele
 				default:
 					break;
 			}
+			break;
 		case nano::election::state_t::backtracking:
 			switch (desired_a)
 			{
-				case nano::election::state_t::idle:
 				case nano::election::state_t::confirmed:
 				case nano::election::state_t::expired_unconfirmed:
 					result = true;
@@ -130,6 +113,7 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele
 				default:
 					break;
 			}
+			break;
 		case nano::election::state_t::confirmed:
 			switch (desired_a)
 			{
@@ -139,8 +123,8 @@ bool nano::election::valid_change (nano::election::state_t expected_a, nano::ele
 				default:
 					break;
 			}
-		case nano::election::state_t::expired_unconfirmed:
 			break;
+		case nano::election::state_t::expired_unconfirmed:
 		case nano::election::state_t::expired_confirmed:
 			break;
 	}
@@ -178,17 +162,6 @@ void nano::election::send_confirm_req (nano::confirmation_solicitor & solicitor_
 	}
 }
 
-void nano::election::transition_passive ()
-{
-	nano::lock_guard<std::mutex> guard (timepoints_mutex);
-	transition_passive_impl ();
-}
-
-void nano::election::transition_passive_impl ()
-{
-	state_change (nano::election::state_t::idle, nano::election::state_t::passive);
-}
-
 void nano::election::transition_active ()
 {
 	nano::lock_guard<std::mutex> guard (timepoints_mutex);
@@ -197,12 +170,7 @@ void nano::election::transition_active ()
 
 void nano::election::transition_active_impl ()
 {
-	state_change (nano::election::state_t::idle, nano::election::state_t::active);
+	state_change (nano::election::state_t::passive, nano::election::state_t::active);
-}
-
-bool nano::election::idle () const
-{
-	return state_m == nano::election::state_t::idle;
 }
 
 bool nano::election::confirmed () const
@@ -234,16 +202,12 @@ bool nano::election::transition_time (nano::confirmation_solicitor & solicitor_a
 	bool result = false;
 	switch (state_m)
 	{
-		case nano::election::state_t::idle:
-			break;
 		case nano::election::state_t::passive:
-		{
 			if (base_latency () * passive_duration_factor < std::chrono::steady_clock::now () - state_start)
 			{
 				state_change (nano::election::state_t::passive, nano::election::state_t::active);
 			}
 			break;
-		}
 		case nano::election::state_t::active:
 			send_confirm_req (solicitor_a);
 			if (confirmation_request_count > active_request_count_min)
@@ -579,21 +543,12 @@ void nano::election::prioritize_election (nano::vote_generator_session & generat
 	generator_session_a.add (root, status.winner->hash ());
 }
 
-void nano::election::try_generate_votes (nano::block_hash const & hash_a)
+void nano::election::generate_votes ()
-{
-	nano::unique_lock<std::mutex> lock (node.active.mutex);
-	if (status.winner->hash () == hash_a)
-	{
-		lock.unlock ();
-		generate_votes (hash_a);
-	}
-}
-
-void nano::election::generate_votes (nano::block_hash const & hash_a)
 {
+	debug_assert (!node.active.mutex.try_lock ());
 	if (node.config.enable_voting && node.wallets.reps ().voting > 0)
 	{
-		node.active.generator.add (root, hash_a);
+		node.active.generator.add (root, status.winner->hash ());
 	}
 }
 

nano/node/election.hpp

@@ -39,7 +39,6 @@ class election final : public std::enable_shared_from_this<nano::election>
 private: // State management
 	enum class state_t
 	{
-		idle,
 		passive, // only listening for incoming votes
 		active, // actively request confirmations
 		broadcasting, // request confirmations and broadcast the winner
@@ -52,7 +51,7 @@ private: // State management
 	static int constexpr active_request_count_min = 2;
 	static int constexpr active_broadcasting_duration_factor = 30;
 	static int constexpr confirmed_duration_factor = 5;
-	std::atomic<nano::election::state_t> state_m = { state_t::idle };
+	std::atomic<nano::election::state_t> state_m = { state_t::passive };
 
 	// These time points must be protected by this mutex
 	std::mutex timepoints_mutex;
@@ -66,7 +65,7 @@ private: // State management
 	void send_confirm_req (nano::confirmation_solicitor &);
 	void activate_dependencies ();
 	// Calculate votes for local representatives
-	void generate_votes (nano::block_hash const &);
+	void generate_votes ();
 	void remove_votes (nano::block_hash const &);
 	std::atomic<bool> prioritized_m = { false };
 
@@ -87,22 +86,17 @@ public:
 	size_t insert_inactive_votes_cache (nano::block_hash const &);
 	bool prioritized () const;
 	void prioritize_election (nano::vote_generator_session &);
-	// Calculate votes if the current winner matches \p hash_a
-	void try_generate_votes (nano::block_hash const & hash_a);
 	// Erase all blocks from active and, if not confirmed, clear digests from network filters
 	void cleanup ();
 
 public: // State transitions
 	bool transition_time (nano::confirmation_solicitor &);
-	void transition_passive ();
 	void transition_active ();
 
 private:
-	void transition_passive_impl ();
 	void transition_active_impl ();
 
 public:
-	bool idle () const;
 	bool confirmed () const;
 	nano::node & node;
 	std::unordered_map<nano::account, nano::vote_info> last_votes;