Improve automatic frontiers confirmation (#2686)

* Improve automatic frontiers confirmation

* Refactor code into function and move up a level (Gui comment)

* Typo semi-colon

* Conditions need reversing

nano/core_test/confirmation_height.cpp

@@ -1000,7 +1000,7 @@ TEST (confirmation_height, prioritize_frontiers)
 		transaction.refresh ();
 		node->active.prioritize_frontiers_for_confirmation (transaction, std::chrono::seconds (1), std::chrono::seconds (1));
 		ASSERT_TRUE (priority_orders_match (node->active.priority_wallet_cementable_frontiers, std::array<nano::account, num_accounts>{ key3.pub, nano::genesis_account, key4.pub, key1.pub, key2.pub }));
-		node->active.search_frontiers (transaction);
+		node->active.confirm_prioritized_frontiers (transaction);
 
 		// Check that the active transactions roots contains the frontiers
 		system.deadline_set (std::chrono::seconds (10));

nano/node/active_transactions.cpp

@@ -32,7 +32,7 @@ thread ([this]() {
 		this->block_cemented_callback (callback_block_a);
 	});
 
-	// Register a callback which will get called after a batch of blocks is written and observer calls finished
+	// Register a callback which will get called if a block is already cemented
 	confirmation_height_processor.add_block_already_cemented_observer ([this](nano::block_hash const & hash_a) {
 		this->block_already_cemented_callback (hash_a);
 	});
@@ -46,7 +46,7 @@ nano::active_transactions::~active_transactions ()
 	stop ();
 }
 
-void nano::active_transactions::search_frontiers (nano::transaction const & transaction_a)
+void nano::active_transactions::confirm_prioritized_frontiers (nano::transaction const & transaction_a)
 {
 	// Limit maximum count of elections to start
 	auto rep_counts (node.wallets.rep_counts ());
@@ -54,19 +54,13 @@ void nano::active_transactions::search_frontiers (nano::transaction const & tran
 	bool half_princpal_representative (representative && rep_counts.half_principal > 0);
 	/* Check less frequently for regular nodes in auto mode */
 	bool agressive_mode (half_princpal_representative || node.config.frontiers_confirmation == nano::frontiers_confirmation_mode::always);
-	auto request_interval (std::chrono::milliseconds (node.network_params.network.request_interval_ms));
-	auto agressive_factor = request_interval * (agressive_mode ? 20 : 100);
-	// Decrease check time for test network
 	auto is_test_network = node.network_params.network.is_test_network ();
-	int test_network_factor = is_test_network ? 1000 : 1;
 	auto roots_size = size ();
-	nano::unique_lock<std::mutex> lk (mutex);
 	auto check_time_exceeded = std::chrono::steady_clock::now () >= next_frontier_check;
-	lk.unlock ();
 	auto max_elections = 1000;
 	auto low_active_elections = roots_size < max_elections;
 	bool wallets_check_required = (!skip_wallets || !priority_wallet_cementable_frontiers.empty ()) && !agressive_mode;
-	// Minimise dropping real-time transactions, set the number of frontiers added to a factor of the total number of active elections
+	// Minimise dropping real-time transactions, set the number of frontiers added to a factor of the maximum number of possible active elections
 	auto max_active = node.config.active_elections_size / 20;
 	if (roots_size <= max_active && (check_time_exceeded || wallets_check_required || (!is_test_network && low_active_elections && agressive_mode)))
 	{
@@ -76,13 +70,8 @@ void nano::active_transactions::search_frontiers (nano::transaction const & tran
 			max_elections = max_active - roots_size;
 		}
 
-		// Spend time prioritizing accounts to reduce voting traffic
-		auto time_spent_prioritizing_ledger_accounts = request_interval / 10;
-		auto time_spent_prioritizing_wallet_accounts = request_interval / 25;
-		prioritize_frontiers_for_confirmation (transaction_a, is_test_network ? std::chrono::milliseconds (50) : time_spent_prioritizing_ledger_accounts, time_spent_prioritizing_wallet_accounts);
-
 		size_t elections_count (0);
-		lk.lock ();
+		nano::unique_lock<std::mutex> lk (mutex);
 		auto start_elections_for_prioritized_frontiers = [&transaction_a, &elections_count, max_elections, &lk, &representative, this](prioritize_num_uncemented & cementable_frontiers) {
 			while (!cementable_frontiers.empty () && !this->stopped && elections_count < max_elections)
 			{
@@ -91,25 +80,28 @@ void nano::active_transactions::search_frontiers (nano::transaction const & tran
 				cementable_frontiers.get<tag_uncemented> ().erase (cementable_account_front_it);
 				lk.unlock ();
 				nano::account_info info;
-				auto error = node.store.account_get (transaction_a, cementable_account.account, info);
+				auto error = this->node.store.account_get (transaction_a, cementable_account.account, info);
 				if (!error)
 				{
-					nano::confirmation_height_info confirmation_height_info;
+					if (!this->confirmation_height_processor.is_processing_block (info.head))
-					error = node.store.confirmation_height_get (transaction_a, cementable_account.account, confirmation_height_info);
-					release_assert (!error);
-
-					if (info.block_count > confirmation_height_info.height && !this->confirmation_height_processor.is_processing_block (info.head))
 					{
-						auto block (this->node.store.block_get (transaction_a, info.head));
+						nano::confirmation_height_info confirmation_height_info;
-						auto election = this->insert (block);
+						error = this->node.store.confirmation_height_get (transaction_a, cementable_account.account, confirmation_height_info);
-						if (election.inserted)
+						release_assert (!error);
+
+						if (info.block_count > confirmation_height_info.height)
 						{
-							election.election->transition_active ();
+							auto block (this->node.store.block_get (transaction_a, info.head));
-							++elections_count;
+							auto election_insert_result = this->insert (block);
-							// Calculate votes for local representatives
+							if (election_insert_result.inserted)
-							if (election.prioritized && representative)
 							{
-								this->node.block_processor.generator.add (info.head);
+								election_insert_result.election->transition_active ();
+								++elections_count;
+								// Calculate votes for local representatives
+								if (election_insert_result.prioritized && representative)
+								{
+									this->node.block_processor.generator.add (info.head);
+								}
 							}
 						}
 					}
@@ -119,7 +111,13 @@ void nano::active_transactions::search_frontiers (nano::transaction const & tran
 		};
 		start_elections_for_prioritized_frontiers (priority_cementable_frontiers);
 		start_elections_for_prioritized_frontiers (priority_wallet_cementable_frontiers);
-		next_frontier_check = steady_clock::now () + (agressive_factor / test_network_factor);
+
+		auto request_interval (std::chrono::milliseconds (node.network_params.network.request_interval_ms));
+		auto rel_time_next_frontier_check = request_interval * (agressive_mode ? 20 : 60);
+		// Decrease check time for test network
+		int test_network_factor = is_test_network ? 1000 : 1;
+
+		next_frontier_check = steady_clock::now () + (rel_time_next_frontier_check / test_network_factor);
 	}
 }
 
@@ -208,24 +206,6 @@ void nano::active_transactions::block_already_cemented_callback (nano::block_has
 void nano::active_transactions::request_confirm (nano::unique_lock<std::mutex> & lock_a)
 {
 	debug_assert (!mutex.try_lock ());
-	/*
-	 * Confirm frontiers when there aren't many confirmations already pending and node finished initial bootstrap
-	 * In auto mode start confirm only if node contains almost principal representative (half of required for principal weight)
-	 */
-
-	// Due to the confirmation height processor working asynchronously and compressing several roots into one frontier, probably_unconfirmed_frontiers can be wrong
-	{
-		auto pending_confirmation_height_size (confirmation_height_processor.awaiting_processing_size ());
-		bool probably_unconfirmed_frontiers (node.ledger.cache.block_count > node.ledger.cache.cemented_count + roots.size () + pending_confirmation_height_size);
-		bool bootstrap_weight_reached (node.ledger.cache.block_count >= node.ledger.bootstrap_weight_max_blocks);
-		if (node.config.frontiers_confirmation != nano::frontiers_confirmation_mode::disabled && bootstrap_weight_reached && probably_unconfirmed_frontiers && pending_confirmation_height_size < confirmed_frontiers_max_pending_cut_off)
-		{
-			lock_a.unlock ();
-			search_frontiers (node.store.tx_begin_read ());
-			lock_a.lock ();
-			update_adjusted_difficulty (); // New roots sorting
-		}
-	}
 
 	// Only representatives ready to receive batched confirm_req
 	nano::confirmation_solicitor solicitor (node.network, node.network_params.network);
@@ -293,6 +273,30 @@ void nano::active_transactions::request_confirm (nano::unique_lock<std::mutex> &
 	}
 }
 
+void nano::active_transactions::frontiers_confirmation (nano::unique_lock<std::mutex> & lock_a)
+{
+	/*
+ 	 * Confirm frontiers when there aren't many confirmations already pending and node finished initial bootstrap
+  	 */
+	auto pending_confirmation_height_size (confirmation_height_processor.awaiting_processing_size ());
+	auto bootstrap_weight_reached (node.ledger.cache.block_count >= node.ledger.bootstrap_weight_max_blocks);
+	auto disabled_confirmation_mode = (node.config.frontiers_confirmation == nano::frontiers_confirmation_mode::disabled);
+	auto conf_height_capacity_reached = pending_confirmation_height_size > confirmed_frontiers_max_pending_size;
+	auto all_cemented = node.ledger.cache.block_count == node.ledger.cache.cemented_count;
+	if (!disabled_confirmation_mode && bootstrap_weight_reached && !conf_height_capacity_reached && !all_cemented)
+	{
+		// Spend some time prioritizing accounts with the most uncemented blocks to reduce voting traffic
+		auto request_interval = std::chrono::milliseconds (node.network_params.network.request_interval_ms);
+		auto time_spent_prioritizing_ledger_accounts = request_interval / 100;
+		auto time_spent_prioritizing_wallet_accounts = request_interval / 250;
+		lock_a.unlock ();
+		auto transaction = node.store.tx_begin_read ();
+		prioritize_frontiers_for_confirmation (transaction, node.network_params.network.is_test_network () ? std::chrono::milliseconds (50) : time_spent_prioritizing_ledger_accounts, time_spent_prioritizing_wallet_accounts);
+		confirm_prioritized_frontiers (transaction);
+		lock_a.lock ();
+	}
+}
+
 void nano::active_transactions::request_loop ()
 {
 	nano::unique_lock<std::mutex> lock (mutex);
@@ -312,6 +316,8 @@ void nano::active_transactions::request_loop ()
 		const auto wakeup_l (std::chrono::steady_clock::now () + std::chrono::milliseconds (node.network_params.network.request_interval_ms));
 
 		update_adjusted_difficulty ();
+		// frontiers_confirmation should be above update_active_difficulty to ensure new sorted roots are updated
+		frontiers_confirmation (lock);
 		update_active_difficulty (lock);
 		request_confirm (lock);
 
@@ -364,10 +370,10 @@ void nano::active_transactions::prioritize_account_for_confirmation (nano::activ
 	}
 }
 
-void nano::active_transactions::prioritize_frontiers_for_confirmation (nano::transaction const & transaction_a, std::chrono::milliseconds ledger_accounts_time_a, std::chrono::milliseconds wallet_account_time_a)
+void nano::active_transactions::prioritize_frontiers_for_confirmation (nano::transaction const & transaction_a, std::chrono::milliseconds ledger_account_traversal_max_time_a, std::chrono::milliseconds wallet_account_traversal_max_time_a)
 {
 	// Don't try to prioritize when there are a large number of pending confirmation heights as blocks can be cemented in the meantime, making the prioritization less reliable
-	if (confirmation_height_processor.awaiting_processing_size () < confirmed_frontiers_max_pending_cut_off)
+	if (confirmation_height_processor.awaiting_processing_size () < confirmed_frontiers_max_pending_size)
 	{
 		size_t priority_cementable_frontiers_size;
 		size_t priority_wallet_cementable_frontiers_size;
@@ -423,7 +429,7 @@ void nano::active_transactions::prioritize_frontiers_for_confirmation (nano::tra
 
 							prioritize_account_for_confirmation (priority_wallet_cementable_frontiers, priority_wallet_cementable_frontiers_size, account, info, confirmation_height_info.height);
 
-							if (wallet_account_timer.since_start () >= wallet_account_time_a)
+							if (wallet_account_timer.since_start () >= wallet_account_traversal_max_time_a)
 							{
 								break;
 							}
@@ -465,7 +471,7 @@ void nano::active_transactions::prioritize_frontiers_for_confirmation (nano::tra
 				}
 			}
 			next_frontier_account = account.number () + 1;
-			if (timer.since_start () >= ledger_accounts_time_a)
+			if (timer.since_start () >= ledger_account_traversal_max_time_a)
 			{
 				break;
 			}

nano/node/active_transactions.hpp

@@ -159,8 +159,9 @@ private:
 	nano::election_insertion_result insert_impl (std::shared_ptr<nano::block>, std::function<void(std::shared_ptr<nano::block>)> const & = [](std::shared_ptr<nano::block>) {});
 	// clang-format on
 	void request_loop ();
-	void search_frontiers (nano::transaction const &);
+	void confirm_prioritized_frontiers (nano::transaction const & transaction_a);
 	void request_confirm (nano::unique_lock<std::mutex> &);
+	void frontiers_confirmation (nano::unique_lock<std::mutex> &);
 	nano::account next_frontier_account{ 0 };
 	std::chrono::steady_clock::time_point next_frontier_check{ std::chrono::steady_clock::now () };
 	nano::condition_variable condition;
@@ -204,7 +205,7 @@ private:
 	bool skip_wallets{ false };
 	void prioritize_account_for_confirmation (prioritize_num_uncemented &, size_t &, nano::account const &, nano::account_info const &, uint64_t);
 	static size_t constexpr max_priority_cementable_frontiers{ 100000 };
-	static size_t constexpr confirmed_frontiers_max_pending_cut_off{ 1000 };
+	static size_t constexpr confirmed_frontiers_max_pending_size{ 10000 };
 	std::deque<nano::block_hash> adjust_difficulty_list;
 	// clang-format off
 	using ordered_cache = boost::multi_index_container<nano::inactive_cache_information,