Increase confirmation solicitor single-round caps (#2897)

* Extend solicitor confirm_req and broadcast caps per election if the vote is for a different hash

* Increase confirmation solicitor max_election_requests from 30 to 50

* Avoid implicit cast from bool (Wes comment)

nano/core_test/confirmation_solicitor.cpp

@@ -87,3 +87,57 @@ TEST (confirmation_solicitor, different_hash)
 	solicitor.flush ();
 	ASSERT_EQ (1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out));
 }
+
+TEST (confirmation_solicitor, bypass_max_requests_cap)
+{
+	nano::system system;
+	nano::node_flags node_flags;
+	node_flags.disable_request_loop = true;
+	node_flags.disable_rep_crawler = true;
+	node_flags.disable_udp = false;
+	auto & node1 = *system.add_node (node_flags);
+	auto & node2 = *system.add_node (node_flags);
+	nano::confirmation_solicitor solicitor (node2.network, node2.network_params.network);
+	std::vector<nano::representative> representatives;
+	auto max_representatives = std::max<size_t> (solicitor.max_election_requests, solicitor.max_election_broadcasts);
+	representatives.reserve (max_representatives + 1);
+	for (auto i (0); i < max_representatives + 1; ++i)
+	{
+		auto channel (node2.network.udp_channels.create (node1.network.endpoint ()));
+		nano::representative representative (nano::account (i), i, channel);
+		representatives.push_back (representative);
+	}
+	ASSERT_EQ (max_representatives + 1, representatives.size ());
+	solicitor.prepare (representatives);
+	ASSERT_TIMELY (3s, node2.network.size () == 1);
+	auto send (std::make_shared<nano::send_block> (nano::genesis_hash, nano::keypair ().pub, nano::genesis_amount - 100, nano::dev_genesis_key.prv, nano::dev_genesis_key.pub, *system.work.generate (nano::genesis_hash)));
+	send->sideband_set ({});
+	{
+		nano::lock_guard<std::mutex> guard (node2.active.mutex);
+		auto election (std::make_shared<nano::election> (node2, send, nullptr, false));
+		// Add a vote for something else, not the winner
+		for (auto const & rep : representatives)
+		{
+			election->last_votes[rep.account] = { std::chrono::steady_clock::now (), 1, 1 };
+		}
+		ASSERT_FALSE (solicitor.add (*election));
+		ASSERT_FALSE (solicitor.broadcast (*election));
+	}
+	solicitor.flush ();
+	// All requests went through, the last one would normally not go through due to the cap but a vote for a different hash does not count towards the cap
+	ASSERT_EQ (max_representatives + 1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out));
+
+	solicitor.prepare (representatives);
+	{
+		nano::lock_guard<std::mutex> guard (node2.active.mutex);
+		auto election (std::make_shared<nano::election> (node2, send, nullptr, false));
+		// Erase all votes
+		election->last_votes.clear ();
+		ASSERT_FALSE (solicitor.add (*election));
+		ASSERT_FALSE (solicitor.broadcast (*election));
+	}
+
+	solicitor.flush ();
+	// All requests but one went through, due to the cap
+	ASSERT_EQ (2 * max_representatives + 1, node2.stats.count (nano::stat::type::message, nano::stat::detail::confirm_req, nano::stat::dir::out));
+}

nano/node/confirmation_solicitor.cpp

@@ -6,7 +6,7 @@ using namespace std::chrono_literals;
 nano::confirmation_solicitor::confirmation_solicitor (nano::network & network_a, nano::network_constants const & params_a) :
 max_confirm_req_batches (params_a.is_dev_network () ? 1 : 20),
 max_block_broadcasts (params_a.is_dev_network () ? 4 : 30),
-max_election_requests (30),
+max_election_requests (50),
 max_election_broadcasts (std::max<size_t> (network_a.fanout () / 2, 1)),
 network (network_a)
 {
@@ -36,10 +36,12 @@ bool nano::confirmation_solicitor::broadcast (nano::election const & election_a)
 		for (auto i (representatives_broadcasts.begin ()), n (representatives_broadcasts.end ()); i != n && count < max_election_broadcasts; ++i)
 		{
 			auto existing (election_a.last_votes.find (i->account));
-			if (existing == election_a.last_votes.end () || existing->second.hash != hash)
+			bool const exists (existing != election_a.last_votes.end ());
+			bool const different (exists && existing->second.hash != hash);
+			if (!exists || different)
 			{
 				i->channel->send (winner);
-				++count;
+				count += different ? 0 : 1;
 			}
 		}
 		// Random flood for block propagation
@@ -52,21 +54,25 @@ bool nano::confirmation_solicitor::broadcast (nano::election const & election_a)
 bool nano::confirmation_solicitor::add (nano::election const & election_a)
 {
 	debug_assert (prepared);
-	auto const max_channel_requests (max_confirm_req_batches * nano::network::confirm_req_hashes_max);
+	bool error (true);
 	unsigned count = 0;
+	auto const max_channel_requests (max_confirm_req_batches * nano::network::confirm_req_hashes_max);
 	auto const & hash (election_a.status.winner->hash ());
 	for (auto i (representatives_requests.begin ()); i != representatives_requests.end () && count < max_election_requests;)
 	{
 		bool full_queue (false);
 		auto rep (*i);
 		auto existing (election_a.last_votes.find (rep.account));
-		if (existing == election_a.last_votes.end () || existing->second.hash != hash)
+		bool const exists (existing != election_a.last_votes.end ());
+		bool const different (exists && existing->second.hash != hash);
+		if (!exists || different)
 		{
 			auto & request_queue (requests[rep.channel]);
 			if (request_queue.size () < max_channel_requests)
 			{
 				request_queue.emplace_back (election_a.status.winner->hash (), election_a.status.winner->root ());
-				++count;
+				count += different ? 0 : 1;
+				error = false;
 			}
 			else
 			{
@@ -75,7 +81,7 @@ bool nano::confirmation_solicitor::add (nano::election const & election_a)
 		}
 		i = !full_queue ? i + 1 : representatives_requests.erase (i);
 	}
-	return count == 0;
+	return error;
 }
 
 void nano::confirmation_solicitor::flush ()

nano/node/confirmation_solicitor.hpp

@@ -26,7 +26,7 @@ public:
 	size_t const max_confirm_req_batches;
 	/** Global maximum amount of block broadcasts */
 	size_t const max_block_broadcasts;
-	/** Maximum amount of requests to be sent per election */
+	/** Maximum amount of requests to be sent per election, bypassed if an existing vote is for a different hash*/
 	size_t const max_election_requests;
 	/** Maximum amount of directed broadcasts to be sent per election */
 	size_t const max_election_broadcasts;