Signature checker blocking fix (#2659)

* Signature checker blocking fix

* Change variable name to remove shadowing

nano/core_test/signing.cpp

@@ -147,3 +147,55 @@ TEST (signature_checker, one)
 	block.signature.bytes[31] ^= 0x1;
 	verify_block (block, 1);
 }
+
+TEST (signature_checker, boundary_checks)
+{
+	// sizes container must be in incrementing order
+	std::vector<size_t> sizes{ 0, 1 };
+	auto add_boundary = [&sizes](size_t boundary) {
+		sizes.insert (sizes.end (), { boundary - 1, boundary, boundary + 1 });
+	};
+
+	for (auto i = 1; i <= 5; ++i)
+	{
+		add_boundary (nano::signature_checker::batch_size * i);
+	}
+
+	nano::signature_checker checker (1);
+	auto max_size = *(sizes.end () - 1);
+	std::vector<nano::uint256_union> hashes;
+	hashes.reserve (max_size);
+	std::vector<unsigned char const *> messages;
+	messages.reserve (max_size);
+	std::vector<size_t> lengths;
+	lengths.reserve (max_size);
+	std::vector<unsigned char const *> pub_keys;
+	pub_keys.reserve (max_size);
+	std::vector<unsigned char const *> signatures;
+	signatures.reserve (max_size);
+	nano::keypair key;
+	nano::state_block block (key.pub, 0, key.pub, 0, 0, key.prv, key.pub, 0);
+
+	auto last_size = 0;
+	for (auto size : sizes)
+	{
+		// The size needed to append to existing containers, saves re-initializing from scratch each iteration
+		auto extra_size = size - last_size;
+
+		std::vector<int> verifications;
+		verifications.resize (size);
+		for (auto i (0); i < extra_size; ++i)
+		{
+			hashes.push_back (block.hash ());
+			messages.push_back (hashes.back ().bytes.data ());
+			lengths.push_back (sizeof (decltype (hashes)::value_type));
+			pub_keys.push_back (block.hashables.account.bytes.data ());
+			signatures.push_back (block.signature.bytes.data ());
+		}
+		nano::signature_check_set check = { size, messages.data (), lengths.data (), pub_keys.data (), signatures.data (), verifications.data () };
+		checker.verify (check);
+		bool all_valid = std::all_of (verifications.cbegin (), verifications.cend (), [](auto verification) { return verification == 1; });
+		ASSERT_TRUE (all_valid);
+		last_size = size;
+	}
+}

nano/node/signatures.cpp

@@ -50,9 +50,16 @@ void nano::signature_checker::verify (nano::signature_check_set & check_a)
 	auto num_full_batches_thread = (num_base_batches_each * num_threads);
 	if (num_full_overflow_batches > 0)
 	{
-		size_calling_thread += batch_size;
-		auto remaining = num_full_overflow_batches - 1;
-		num_full_batches_thread += remaining;
+		if (overflow_size == 0)
+		{
+			// Give the calling thread priority over any batches when there is no excess remainder.
+			size_calling_thread += batch_size;
+			num_full_batches_thread += num_full_overflow_batches - 1;
+		}
+		else
+		{
+			num_full_batches_thread += num_full_overflow_batches;
+		}
 	}
 
 	release_assert (check_a.size == (num_full_batches_thread * batch_size + size_calling_thread));

nano/node/signatures.hpp

@@ -35,6 +35,8 @@ public:
 	void stop ();
 	void flush ();
 
+	static size_t constexpr batch_size = 256;
+
 private:
 	struct Task final
 	{
@@ -55,7 +57,6 @@ private:
 	void set_thread_names (unsigned num_threads);
 	boost::asio::thread_pool thread_pool;
 	std::atomic<int> tasks_remaining{ 0 };
-	static constexpr size_t batch_size = 256;
 	const bool single_threaded;
 	unsigned num_threads;
 	std::atomic<bool> stopped{ false };

nano/node/state_block_signature_verification.cpp

@@ -46,7 +46,7 @@ void nano::state_block_signature_verification::run (uint64_t state_block_signatu
 	{
 		if (!state_blocks.empty ())
 		{
-			size_t const max_verification_batch (state_block_signature_verification_size != 0 ? state_block_signature_verification_size : 256 * (node_config.signature_checker_threads + 1));
+			size_t const max_verification_batch (state_block_signature_verification_size != 0 ? state_block_signature_verification_size : nano::signature_checker::batch_size * (node_config.signature_checker_threads + 1));
 			active = true;
 			while (!state_blocks.empty () && !stopped)
 			{

nano/slow_test/node.cpp

@@ -986,3 +986,59 @@ namespace transport
 	}
 }
 }
+
+// Similar to signature_checker.boundary_checks but more exhaustive. Can take up to 1 minute
+TEST (signature_checker, mass_boundary_checks)
+{
+	// sizes container must be in incrementing order
+	std::vector<size_t> sizes{ 0, 1 };
+	auto add_boundary = [&sizes](size_t boundary) {
+		sizes.insert (sizes.end (), { boundary - 1, boundary, boundary + 1 });
+	};
+
+	for (auto i = 1; i <= 10; ++i)
+	{
+		add_boundary (nano::signature_checker::batch_size * i);
+	}
+
+	for (auto num_threads = 0; num_threads < 5; ++num_threads)
+	{
+		nano::signature_checker checker (num_threads);
+		auto max_size = *(sizes.end () - 1);
+		std::vector<nano::uint256_union> hashes;
+		hashes.reserve (max_size);
+		std::vector<unsigned char const *> messages;
+		messages.reserve (max_size);
+		std::vector<size_t> lengths;
+		lengths.reserve (max_size);
+		std::vector<unsigned char const *> pub_keys;
+		pub_keys.reserve (max_size);
+		std::vector<unsigned char const *> signatures;
+		signatures.reserve (max_size);
+		nano::keypair key;
+		nano::state_block block (key.pub, 0, key.pub, 0, 0, key.prv, key.pub, 0);
+
+		auto last_size = 0;
+		for (auto size : sizes)
+		{
+			// The size needed to append to existing containers, saves re-initializing from scratch each iteration
+			auto extra_size = size - last_size;
+
+			std::vector<int> verifications;
+			verifications.resize (size);
+			for (auto i (0); i < extra_size; ++i)
+			{
+				hashes.push_back (block.hash ());
+				messages.push_back (hashes.back ().bytes.data ());
+				lengths.push_back (sizeof (decltype (hashes)::value_type));
+				pub_keys.push_back (block.hashables.account.bytes.data ());
+				signatures.push_back (block.signature.bytes.data ());
+			}
+			nano::signature_check_set check = { size, messages.data (), lengths.data (), pub_keys.data (), signatures.data (), verifications.data () };
+			checker.verify (check);
+			bool all_valid = std::all_of (verifications.cbegin (), verifications.cend (), [](auto verification) { return verification == 1; });
+			ASSERT_TRUE (all_valid);
+			last_size = size;
+		}
+	}
+}