Election refactor follow up (#2619)

* active_transactions include cleanup

* Allocate a new solicitor on every confirmation request loop

* Increment election confirmation_request_count

This was accidentally erased in the refactor and there was no test to ensure it. One test removed as it was not testing the intended functionality. Other tests were updated to ensure confirmation_request_count is incremented. It was necessary to disable the rep crawler to properly test the confirmation loop.

* Ensuring confirmed elections are not returned in RPC confirmation_info

* Adding information on confirmed blocks for RPC confirmation_active

* Moving election status definitions to secure/common

* Re-lock after activating dependencies (bug found by @cryptocode)

Otherwise, could call state_change to `expired_unconfirmed` without owning the mutex

* Enhance node.activate_dependencies test by ensuring full confirmation (Serg review)

nano/core_test/active_transactions.cpp

@@ -7,76 +7,93 @@
 
 using namespace std::chrono_literals;
 
+namespace nano
+{
 TEST (active_transactions, confirm_active)
 {
-	nano::system system (1);
-	auto & node1 = *system.nodes[0];
-	// Send and vote for a block before peering with node2
-	system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv);
-	auto send (system.wallet (0)->send_action (nano::test_genesis_key.pub, nano::public_key (), node1.config.receive_minimum.number ()));
-	system.deadline_set (5s);
-	while (!node1.active.empty () || !node1.block_confirmed_or_being_confirmed (node1.store.tx_begin_read (), send->hash ()))
-	{
-		ASSERT_NO_ERROR (system.poll ());
-	}
-	auto & node2 = *system.add_node (nano::node_config (nano::get_available_port (), system.logging));
+	nano::system system;
+	nano::node_flags node_flags;
+	node_flags.disable_request_loop = true;
+	auto & node1 = *system.add_node (node_flags);
+	nano::genesis genesis;
+	auto send (std::make_shared<nano::send_block> (genesis.hash (), nano::public_key (), nano::genesis_amount - 100, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (genesis.hash ())));
+	ASSERT_EQ (nano::process_result::progress, node1.process (*send).code);
+	nano::node_config node_config2 (nano::get_available_port (), system.logging);
+	node_config2.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;
+	nano::node_flags node_flags2;
+	// The rep crawler would otherwise request confirmations in order to find representatives
+	node_flags2.disable_rep_crawler = true;
+	auto & node2 = *system.add_node (node_config2, node_flags2);
 	system.deadline_set (5s);
 	// Let node2 know about the block
 	while (node2.active.empty ())
 	{
 		node1.network.flood_block (send, nano::buffer_drop_policy::no_limiter_drop);
 		ASSERT_NO_ERROR (system.poll ());
 	}
-	while (node2.ledger.cache.cemented_count < 2 || !node2.active.empty ())
-	{
-		ASSERT_NO_ERROR (system.poll ());
-	}
-}
-
-TEST (active_transactions, confirm_frontier)
-{
-	nano::system system (1);
-	auto & node1 = *system.nodes[0];
-	// Send and vote for a block before peering with node2
+	// Save election to check request count afterwards
+	auto election = node2.active.election (send->qualified_root ());
+	ASSERT_NE (nullptr, election);
+	// Add key to node1
 	system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv);
-	auto send (system.wallet (0)->send_action (nano::test_genesis_key.pub, nano::public_key (), node1.config.receive_minimum.number ()));
-	system.deadline_set (5s);
-	while (!node1.active.empty () || !node1.block_confirmed_or_being_confirmed (node1.store.tx_begin_read (), send->hash ()))
+	// Add representative to disabled rep crawler
+	auto peers (node2.network.random_set (1));
+	ASSERT_FALSE (peers.empty ());
 	{
-		ASSERT_NO_ERROR (system.poll ());
+		nano::lock_guard<std::mutex> guard (node2.rep_crawler.probable_reps_mutex);
+		node2.rep_crawler.probable_reps.emplace (nano::test_genesis_key.pub, nano::genesis_amount, *peers.begin ());
 	}
-	auto & node2 = *system.add_node (nano::node_config (nano::get_available_port (), system.logging));
-	ASSERT_EQ (nano::process_result::progress, node2.process (*send).code);
-	system.deadline_set (5s);
 	while (node2.ledger.cache.cemented_count < 2 || !node2.active.empty ())
 	{
 		ASSERT_NO_ERROR (system.poll ());
 	}
+	// At least one confirmation request
+	ASSERT_GT (election->confirmation_request_count, 0);
+	// Blocks were cleared (except for not_an_account)
+	ASSERT_EQ (1, election->blocks.size ());
+}
 }
 
-TEST (active_transactions, confirm_dependent)
+namespace nano
+{
+TEST (active_transactions, confirm_frontier)
 {
 	nano::system system;
 	nano::node_flags node_flags;
 	node_flags.disable_request_loop = true;
 	auto & node1 = *system.add_node (node_flags);
+	nano::genesis genesis;
+	auto send (std::make_shared<nano::send_block> (genesis.hash (), nano::public_key (), nano::genesis_amount - 100, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (genesis.hash ())));
+	ASSERT_EQ (nano::process_result::progress, node1.process (*send).code);
+	nano::node_flags node_flags2;
+	// The rep crawler would otherwise request confirmations in order to find representatives
+	node_flags2.disable_rep_crawler = true;
+	auto & node2 = *system.add_node (node_flags2);
+	ASSERT_EQ (nano::process_result::progress, node2.process (*send).code);
+	system.deadline_set (5s);
+	while (node2.active.empty ())
+	{
+		ASSERT_NO_ERROR (system.poll ());
+	}
+	// Save election to check request count afterwards
+	auto election = node2.active.election (send->qualified_root ());
+	ASSERT_NE (nullptr, election);
+	// Add key to node1
 	system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv);
-	auto send1 (system.wallet (0)->send_action (nano::test_genesis_key.pub, nano::public_key (), node1.config.receive_minimum.number ()));
-	auto send2 (system.wallet (0)->send_action (nano::test_genesis_key.pub, nano::public_key (), node1.config.receive_minimum.number ()));
-	auto send3 (system.wallet (0)->send_action (nano::test_genesis_key.pub, nano::public_key (), node1.config.receive_minimum.number ()));
-	nano::node_config node_config;
-	node_config.peering_port = nano::get_available_port ();
-	node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;
-	auto & node2 = *system.add_node (node_config);
-	node2.process_local (send1);
-	node2.process_local (send2);
-	node2.process_active (send3);
+	// Add representative to disabled rep crawler
+	auto peers (node2.network.random_set (1));
+	ASSERT_FALSE (peers.empty ());
+	{
+		nano::lock_guard<std::mutex> guard (node2.rep_crawler.probable_reps_mutex);
+		node2.rep_crawler.probable_reps.emplace (nano::test_genesis_key.pub, nano::genesis_amount, *peers.begin ());
+	}
 	system.deadline_set (5s);
-	while (!node2.active.empty ())
+	while (node2.ledger.cache.cemented_count < 2 || !node2.active.empty ())
 	{
 		ASSERT_NO_ERROR (system.poll ());
 	}
-	ASSERT_EQ (4, node2.ledger.cache.cemented_count);
+	ASSERT_GT (election->confirmation_request_count, 0);
+}
 }
 
 TEST (active_transactions, adjusted_difficulty_priority)
@@ -694,7 +711,6 @@ TEST (active_transactions, activate_dependencies)
 	system.wallet (0)->insert_adhoc (nano::test_genesis_key.prv);
 	nano::genesis genesis;
 	nano::block_builder builder;
-	system.deadline_set (std::chrono::seconds (15));
 	std::shared_ptr<nano::block> block0 = builder.state ()
 	                                      .account (nano::test_genesis_key.pub)
 	                                      .previous (genesis.hash ())
@@ -707,6 +723,7 @@ TEST (active_transactions, activate_dependencies)
 	// Establish a representative
 	node2->process_active (block0);
 	node2->block_processor.flush ();
+	system.deadline_set (10s);
 	while (node1->block (block0->hash ()) == nullptr)
 	{
 		ASSERT_NO_ERROR (system.poll ());
@@ -735,9 +752,17 @@ TEST (active_transactions, activate_dependencies)
 	                                      .build ();
 	node2->process_active (block2);
 	node2->block_processor.flush ();
+	system.deadline_set (10s);
 	while (node1->block (block2->hash ()) == nullptr)
 	{
 		ASSERT_NO_ERROR (system.poll ());
 	}
 	ASSERT_NE (nullptr, node1->block (block2->hash ()));
+	system.deadline_set (10s);
+	while (!node1->active.empty () || !node2->active.empty ())
+	{
+		ASSERT_NO_ERROR (system.poll ());
+	}
+	ASSERT_TRUE (node1->ledger.block_confirmed (node1->store.tx_begin_read (), block2->hash ()));
+	ASSERT_TRUE (node2->ledger.block_confirmed (node2->store.tx_begin_read (), block2->hash ()));
 }

nano/core_test/confirmation_solicitor.cpp

@@ -1,5 +1,6 @@
 #include <nano/core_test/testutil.hpp>
 #include <nano/lib/jsonconfig.hpp>
+#include <nano/node/confirmation_solicitor.hpp>
 #include <nano/node/testing.hpp>
 
 #include <gtest/gtest.h>
@@ -9,51 +10,49 @@ using namespace std::chrono_literals;
 TEST (confirmation_solicitor, batches)
 {
 	nano::system system;
-	nano::node_config node_config (nano::get_available_port (), system.logging);
-	node_config.enable_voting = false;
-	node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;
 	nano::node_flags node_flags;
+	node_flags.disable_request_loop = true;
 	node_flags.disable_udp = false;
-	auto & node1 = *system.add_node (node_config, node_flags);
-	node_config.peering_port = nano::get_available_port ();
-	// To prevent races on the solicitor
+	auto & node1 = *system.add_node (node_flags);
+	// This tests instantiates a solicitor
 	node_flags.disable_request_loop = true;
-	auto & node2 = *system.add_node (node_config, node_flags);
+	auto & node2 = *system.add_node (node_flags);
 	// Solicitor will only solicit from this representative
 	auto channel1 (node2.network.udp_channels.create (node1.network.endpoint ()));
 	nano::representative representative (nano::test_genesis_key.pub, nano::genesis_amount, channel1);
-	// Lock active_transactions which uses the solicitor
+
+	std::vector<nano::representative> representatives{ representative };
+	nano::confirmation_solicitor solicitor (node2.network, node2.network_params.network);
+	solicitor.prepare (representatives);
+	// Ensure the representatives are correct
+	ASSERT_EQ (1, representatives.size ());
+	ASSERT_EQ (channel1, representatives.front ().channel);
+	ASSERT_EQ (nano::test_genesis_key.pub, representatives.front ().account);
+	auto send (std::make_shared<nano::send_block> (nano::genesis_hash, nano::keypair ().pub, nano::genesis_amount - 100, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (nano::genesis_hash)));
 	{
-		nano::lock_guard<std::mutex> active_guard (node2.active.mutex);
-		std::vector<nano::representative> representatives{ representative };
-		node2.active.solicitor.prepare (representatives);
-		// Ensure the representatives are correct
-		ASSERT_EQ (1, representatives.size ());
-		ASSERT_EQ (channel1, representatives.front ().channel);
-		ASSERT_EQ (nano::test_genesis_key.pub, representatives.front ().account);
-		auto send (std::make_shared<nano::send_block> (nano::genesis_hash, nano::keypair ().pub, nano::genesis_amount - 100, nano::test_genesis_key.prv, nano::test_genesis_key.pub, *system.work.generate (nano::genesis_hash)));
+		nano::lock_guard<std::mutex> guard (node2.active.mutex);
 		for (size_t i (0); i < nano::network::confirm_req_hashes_max; ++i)
 		{
 			auto election (std::make_shared<nano::election> (node2, send, nullptr));
-			ASSERT_FALSE (node2.active.solicitor.add (*election));
+			ASSERT_FALSE (solicitor.add (*election));
 		}
-		ASSERT_EQ (1, node2.active.solicitor.max_confirm_req_batches);
+		ASSERT_EQ (1, solicitor.max_confirm_req_batches);
 		// Reached the maximum amount of requests for the channel
 		auto election (std::make_shared<nano::election> (node2, send, nullptr));
-		ASSERT_TRUE (node2.active.solicitor.add (*election));
+		ASSERT_TRUE (solicitor.add (*election));
 		// Broadcasting should be immediate
 		ASSERT_EQ (0, node2.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::out));
-		ASSERT_FALSE (node2.active.solicitor.broadcast (*election));
-		system.deadline_set (5s);
-		while (node2.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::out) < 1)
-		{
-			ASSERT_NO_ERROR (system.poll ());
-		}
+		ASSERT_FALSE (solicitor.broadcast (*election));
+	}
+	system.deadline_set (5s);
+	while (node2.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::out) < 1)
+	{
+		ASSERT_NO_ERROR (system.poll ());
 	}
 	// From rep crawler
 	ASSERT_EQ (1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out));
 	system.deadline_set (5s);
-	node2.active.solicitor.flush ();
+	solicitor.flush ();
 	while (node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out) < 2)
 	{
 		ASSERT_NO_ERROR (system.poll ());

nano/node/active_transactions.cpp

@@ -1,8 +1,11 @@
 #include <nano/lib/threading.hpp>
 #include <nano/node/active_transactions.hpp>
 #include <nano/node/confirmation_height_processor.hpp>
+#include <nano/node/confirmation_solicitor.hpp>
 #include <nano/node/election.hpp>
 #include <nano/node/node.hpp>
+#include <nano/node/repcrawler.hpp>
+#include <nano/secure/blockstore.hpp>
 
 #include <boost/format.hpp>
 #include <boost/variant/get.hpp>
@@ -16,7 +19,6 @@ confirmation_height_processor (confirmation_height_processor_a),
 node (node_a),
 multipliers_cb (20, 1.),
 trended_active_difficulty (node_a.network_params.network.publish_threshold),
-solicitor (node_a.network, node_a.network_params.network),
 election_time_to_live (node_a.network_params.network.is_test_network () ? 0s : 2s),
 thread ([this]() {
 	nano::thread_role::set (nano::thread_role::name::request_loop);
@@ -225,6 +227,7 @@ void nano::active_transactions::request_confirm (nano::unique_lock<std::mutex> &
 
 	// Only representatives ready to receive batched confirm_req
 	lock_a.unlock ();
+	nano::confirmation_solicitor solicitor (node.network, node.network_params.network);
 	solicitor.prepare (node.rep_crawler.representatives (node.network_params.protocol.tcp_realtime_protocol_version_min));
 	lock_a.lock ();
 
@@ -244,7 +247,8 @@ void nano::active_transactions::request_confirm (nano::unique_lock<std::mutex> &
 	for (auto i = sorted_roots_l.begin (), n = sorted_roots_l.end (); i != n; ++count_l)
 	{
 		auto & election_l (i->election);
-		if ((count_l >= node.config.active_elections_size && election_l->election_start < election_ttl_cutoff_l && !node.wallets.watcher->is_watched (i->root)) || election_l->transition_time (saturated_l))
+		bool const overflow_l (count_l >= node.config.active_elections_size && election_l->election_start < election_ttl_cutoff_l && !node.wallets.watcher->is_watched (i->root));
+		if (overflow_l || election_l->transition_time (solicitor, saturated_l))
 		{
 			election_l->clear_blocks ();
 			i = sorted_roots_l.erase (i);

nano/node/active_transactions.hpp

@@ -1,12 +1,6 @@
 #pragma once
 
 #include <nano/lib/numbers.hpp>
-#include <nano/node/confirmation_solicitor.hpp>
-#include <nano/node/election.hpp>
-#include <nano/node/gap_cache.hpp>
-#include <nano/node/repcrawler.hpp>
-#include <nano/node/transport/transport.hpp>
-#include <nano/secure/blockstore.hpp>
 #include <nano/secure/common.hpp>
 
 #include <boost/circular_buffer.hpp>
@@ -142,7 +136,6 @@ class active_transactions final
 	size_t inactive_votes_cache_size ();
 	size_t election_winner_details_size ();
 	void add_election_winner_details (nano::block_hash const &, std::shared_ptr<nano::election> const &);
-	nano::confirmation_solicitor solicitor;
 
 private:
 	std::mutex election_winner_details_mutex;

nano/node/blockprocessor.cpp

@@ -1,5 +1,6 @@
 #include <nano/lib/timer.hpp>
 #include <nano/node/blockprocessor.hpp>
+#include <nano/node/election.hpp>
 #include <nano/node/node.hpp>
 #include <nano/secure/blockstore.hpp>
 

nano/node/election.cpp

@@ -1,3 +1,4 @@
+#include <nano/node/confirmation_solicitor.hpp>
 #include <nano/node/election.hpp>
 #include <nano/node/node.hpp>
 
@@ -148,13 +149,14 @@ bool nano::election::state_change (nano::election::state_t expected_a, nano::ele
 	return result;
 }
 
-void nano::election::send_confirm_req ()
+void nano::election::send_confirm_req (nano::confirmation_solicitor & solicitor_a)
 {
 	if (last_req + std::chrono::seconds (15) < std::chrono::steady_clock::now ())
 	{
-		if (!node.active.solicitor.add (*this))
+		if (!solicitor_a.add (*this))
 		{
 			last_req = std::chrono::steady_clock::now ();
+			++confirmation_request_count;
 		}
 	}
 }
@@ -242,18 +244,18 @@ void nano::election::activate_dependencies ()
 	}
 }
 
-void nano::election::broadcast_block ()
+void nano::election::broadcast_block (nano::confirmation_solicitor & solicitor_a)
 {
 	if (base_latency () * 5 < std::chrono::steady_clock::now () - last_block)
 	{
-		if (!node.active.solicitor.broadcast (*this))
+		if (!solicitor_a.broadcast (*this))
 		{
 			last_block = std::chrono::steady_clock::now ();
 		}
 	}
 }
 
-bool nano::election::transition_time (bool const saturated_a)
+bool nano::election::transition_time (nano::confirmation_solicitor & solicitor_a, bool const saturated_a)
 {
 	debug_assert (!node.active.mutex.try_lock ());
 	nano::unique_lock<std::mutex> lock (timepoints_mutex);
@@ -271,18 +273,19 @@ bool nano::election::transition_time (bool const saturated_a)
 			break;
 		}
 		case nano::election::state_t::active:
-			broadcast_block ();
-			send_confirm_req ();
+			broadcast_block (solicitor_a);
+			send_confirm_req (solicitor_a);
 			if (base_latency () * active_duration_factor < std::chrono::steady_clock::now () - state_start)
 			{
 				state_change (nano::election::state_t::active, nano::election::state_t::backtracking);
 				lock.unlock ();
 				activate_dependencies ();
+				lock.lock ();
 			}
 			break;
 		case nano::election::state_t::backtracking:
-			broadcast_block ();
-			send_confirm_req ();
+			broadcast_block (solicitor_a);
+			send_confirm_req (solicitor_a);
 			break;
 		case nano::election::state_t::confirmed:
 			if (base_latency () * (saturated_a ? confirmed_duration_factor_saturated : confirmed_duration_factor) < std::chrono::steady_clock::now () - state_start)
@@ -296,7 +299,6 @@ bool nano::election::transition_time (bool const saturated_a)
 			debug_assert (false);
 			break;
 	}
-	// Note: lock (timepoints_mutex) is at an unknown state here - possibly unlocked before activate_dependencies
 	if (!confirmed () && std::chrono::minutes (5) < std::chrono::steady_clock::now () - election_start)
 	{
 		result = true;