Validate and update vote reservation timestamp

This is done after vote generation to be correct, and the generated votes are dropped if validation failed (should not normally happen, but is in place to ensure correctness).

This fixes intermittent failures on `vote_generator.spacing` which came from the stamps on the generated votes being less than **round_time** apart.

nano/core_test/vote_generation.cpp

@@ -60,18 +60,24 @@ TEST (vote_reserver, basic)
 	ASSERT_TRUE (history.exists (1));
 	ASSERT_FALSE (reserver.add (2));
 	ASSERT_TRUE (reserver.add (1));
+	auto sleep_time = 100ms;
+	ASSERT_GT (reserver.round_time, 5 * sleep_time);
+	auto max_iterations (2 * reserver.round_time / 100ms);
 	auto iterations (0);
 	while (reserver.add (1))
 	{
 		ASSERT_TRUE (history.exists (1));
-		std::this_thread::sleep_for (100ms);
-		ASSERT_LT (++iterations, 20);
+		std::this_thread::sleep_for (sleep_time);
+		ASSERT_LT (++iterations, max_iterations);
 	}
 	ASSERT_FALSE (history.exists (1));
 	ASSERT_GT (iterations, 0);
 	ASSERT_TRUE (reserver.add (1));
 	ASSERT_FALSE (reserver.add (2));
 	ASSERT_TRUE (reserver.add (1));
+	ASSERT_FALSE (reserver.validate_and_update ({ 2 }));
+	std::this_thread::sleep_for (reserver.round_time);
+	ASSERT_TRUE (reserver.validate_and_update ({ 2 }));
 }
 
 TEST (vote_generator, cache)

nano/node/voting.cpp

@@ -25,6 +25,19 @@ bool nano::vote_reserver::add (nano::root const & root_a)
 	return !result;
 }
 
+bool nano::vote_reserver::validate_and_update (std::vector<nano::root> const & roots_a)
+{
+	clean ();
+	auto & reservations_by_root (reservations.get<tag_root> ());
+	auto now (std::chrono::steady_clock::now ());
+	auto update_time = [&now](auto & reservation_a) { reservation_a.time = now; };
+	auto result = std::any_of (roots_a.begin (), roots_a.end (), [&update_time, &reservations_by_root](auto const & root_a) {
+		auto existing (reservations_by_root.find (root_a));
+		return existing == reservations_by_root.end () || !reservations_by_root.modify (existing, update_time);
+	});
+	return result;
+}
+
 void nano::vote_reserver::clean ()
 {
 	auto & reservations_by_time (reservations.get<tag_time> ());
@@ -168,18 +181,15 @@ void nano::vote_generator::generate (std::vector<std::pair<nano::block_hash, nan
 				roots.push_back (root);
 				if (hashes.size () == nano::network::confirm_ack_hashes_max)
 				{
-					lock.unlock ();
-					vote (hashes, roots, action_a);
+					vote (lock, hashes, roots, action_a);
 					hashes.clear ();
 					roots.clear ();
-					lock.lock ();
 				}
 			}
 		}
 		if (!hashes.empty ())
 		{
-			lock.unlock ();
-			vote (hashes, roots, action_a);
+			vote (lock, hashes, roots, action_a);
 		}
 	}
 }
@@ -213,28 +223,38 @@ void nano::vote_generator::send (nano::unique_lock<std::mutex> & lock_a)
 		roots.push_back (front.first);
 		hashes_l.push_back (front.second);
 	}
-	lock_a.unlock ();
 	if (!hashes_l.empty ())
 	{
-		vote (hashes_l, roots, [this](auto vote_a) { this->broadcast_action (vote_a); });
+		vote (lock_a, hashes_l, roots, [this](auto vote_a) { this->broadcast_action (vote_a); });
 	}
-	lock_a.lock ();
 }
 
-void nano::vote_generator::vote (std::vector<nano::block_hash> const & hashes_a, std::vector<nano::root> const & roots_a, std::function<void(std::shared_ptr<nano::vote> const &)> const & action_a) const
+void nano::vote_generator::vote (nano::unique_lock<std::mutex> & lock_a, std::vector<nano::block_hash> const & hashes_a, std::vector<nano::root> const & roots_a, std::function<void(std::shared_ptr<nano::vote> const &)> const & action_a)
 {
 	debug_assert (hashes_a.size () == roots_a.size ());
-	auto transaction (store.tx_begin_read ());
-	wallets.foreach_representative ([this, &hashes_a, &roots_a, &transaction, &action_a](nano::public_key const & pub_a, nano::raw_key const & prv_a) {
-		auto vote_l (this->store.vote_generate (transaction, pub_a, prv_a, hashes_a));
+	{
+		lock_a.unlock ();
+		auto transaction (store.tx_begin_read ());
+		std::vector<std::shared_ptr<nano::vote>> votes_l;
+		wallets.foreach_representative ([this, &hashes_a, &roots_a, &transaction, &votes_l](nano::public_key const & pub_a, nano::raw_key const & prv_a) {
+			votes_l.emplace_back (this->store.vote_generate (transaction, pub_a, prv_a, hashes_a));
+		});
+		lock_a.lock ();
+		// To be strictly correct, validation must go after vote generation. If validation fails, the votes are not used
+		if (!reserver.validate_and_update (roots_a))
 		{
-			for (size_t i (0), n (hashes_a.size ()); i != n; ++i)
+			lock_a.unlock ();
+			for (auto const & vote_l : votes_l)
 			{
-				this->history.add (roots_a[i], hashes_a[i], vote_l);
+				for (size_t i (0), n (hashes_a.size ()); i != n; ++i)
+				{
+					this->history.add (roots_a[i], hashes_a[i], vote_l);
+				}
+				action_a (vote_l);
 			}
+			lock_a.lock ();
 		}
-		action_a (vote_l);
-	});
+	}
 }
 
 void nano::vote_generator::broadcast_action (std::shared_ptr<nano::vote> const & vote_a) const

nano/node/voting.hpp

@@ -87,12 +87,15 @@ class vote_reserver final
 		{
 		}
 		nano::root root;
-		std::chrono::steady_clock::time_point const time;
+		std::chrono::steady_clock::time_point time;
 	};
 
 public:
 	vote_reserver (nano::local_vote_history &);
+	/** Returns false if the reservation was made */
 	bool add (nano::root const &);
+	/** Returns false if the reservations for these roots are still valid, and updates them with the current time. This should be called before vote generation */
+	bool validate_and_update (std::vector<nano::root> const &);
 	void clean ();
 
 	std::chrono::seconds const round_time{ nano::network_params ().network.is_test_network () ? 1 : 45 };
@@ -104,7 +107,7 @@ class vote_reserver final
 		mi::hashed_unique<mi::tag<class tag_root>,
 			mi::member<vote_reservation, nano::root, &vote_reservation::root>>,
 		mi::ordered_non_unique<mi::tag<class tag_time>,
-			mi::member<vote_reservation, std::chrono::steady_clock::time_point const, &vote_reservation::time>>>>
+			mi::member<vote_reservation, std::chrono::steady_clock::time_point, &vote_reservation::time>>>>
 	reservations;
 	// clang-format on
 
@@ -129,7 +132,7 @@ class vote_generator final
 private:
 	void run ();
 	void send (nano::unique_lock<std::mutex> &);
-	void vote (std::vector<nano::block_hash> const &, std::vector<nano::root> const &, std::function<void(std::shared_ptr<nano::vote> const &)> const &) const;
+	void vote (nano::unique_lock<std::mutex> &, std::vector<nano::block_hash> const &, std::vector<nano::root> const &, std::function<void(std::shared_ptr<nano::vote> const &)> const &);
 	void broadcast_action (std::shared_ptr<nano::vote> const &) const;
 	mutable std::mutex mutex;
 	nano::condition_variable condition;