Fix vote_spacing logic

nano/core_test/node.cpp

@@ -2003,7 +2003,7 @@ TEST (node, vote_by_hash_bundle)
 
 	for (auto const & block : blocks)
 	{
-		system.nodes[0]->generator.add (block->root (), block->hash ());
+		system.nodes[0]->generator.add (*block);
 	}
 
 	// Verify that bundling occurs. While reaching 12 should be common on most hardware in release mode,

nano/core_test/voting.cpp

@@ -60,13 +60,56 @@ TEST (local_vote_history, basic)
 }
 }
 
+/*
+ * vote_spacing
+ */
+
+TEST (vote_spacing, basic)
+{
+	nano::vote_spacing spacing{ std::chrono::milliseconds{ 100 } };
+	nano::qualified_root root1{ 1 };
+	nano::qualified_root root2{ 2 };
+	nano::block_hash hash3{ 3 };
+	nano::block_hash hash4{ 4 };
+	nano::block_hash hash5{ 5 };
+	ASSERT_EQ (0, spacing.size ());
+	ASSERT_TRUE (spacing.votable (root1, hash3));
+	spacing.flag (root1, hash3);
+	ASSERT_EQ (1, spacing.size ());
+	ASSERT_FALSE (spacing.votable (root1, hash3));
+	ASSERT_TRUE (spacing.votable (root1, hash4));
+	spacing.flag (root2, hash5);
+	ASSERT_EQ (2, spacing.size ());
+}
+
+TEST (vote_spacing, prune)
+{
+	auto now = std::chrono::steady_clock::now ();
+
+	auto length = std::chrono::milliseconds{ 100 };
+	nano::vote_spacing spacing{ length };
+	nano::qualified_root root1{ 1 };
+	nano::qualified_root root2{ 2 };
+	nano::block_hash hash3{ 3 };
+	nano::block_hash hash4{ 4 };
+	spacing.flag (root1, hash3, now);
+	ASSERT_EQ (1, spacing.size ());
+	// Wait for the spacing to expire
+	spacing.flag (root2, hash4, now + length);
+	ASSERT_EQ (1, spacing.size ());
+}
+
+/*
+ * vote_generator
+ */
+
 TEST (vote_generator, cache)
 {
 	nano::test::system system (1);
 	auto & node (*system.nodes[0]);
 	auto epoch1 = system.upgrade_genesis_epoch (node, nano::epoch::epoch_1);
 	system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
-	node.generator.add (epoch1->root (), epoch1->hash ());
+	node.generator.add (*epoch1);
 	ASSERT_TIMELY (1s, !node.history.votes (epoch1->root (), epoch1->hash ()).empty ());
 	auto votes (node.history.votes (epoch1->root (), epoch1->hash ()));
 	ASSERT_FALSE (votes.empty ());
@@ -110,40 +153,7 @@ TEST (vote_generator, multiple_representatives)
 	}
 }
 
-TEST (vote_spacing, basic)
-{
-	nano::vote_spacing spacing{ std::chrono::milliseconds{ 100 } };
-	nano::root root1{ 1 };
-	nano::root root2{ 2 };
-	nano::block_hash hash3{ 3 };
-	nano::block_hash hash4{ 4 };
-	nano::block_hash hash5{ 5 };
-	ASSERT_EQ (0, spacing.size ());
-	ASSERT_TRUE (spacing.votable (root1, hash3));
-	spacing.flag (root1, hash3);
-	ASSERT_EQ (1, spacing.size ());
-	ASSERT_TRUE (spacing.votable (root1, hash3));
-	ASSERT_FALSE (spacing.votable (root1, hash4));
-	spacing.flag (root2, hash5);
-	ASSERT_EQ (2, spacing.size ());
-}
-
-TEST (vote_spacing, prune)
-{
-	auto length = std::chrono::milliseconds{ 100 };
-	nano::vote_spacing spacing{ length };
-	nano::root root1{ 1 };
-	nano::root root2{ 2 };
-	nano::block_hash hash3{ 3 };
-	nano::block_hash hash4{ 4 };
-	spacing.flag (root1, hash3);
-	ASSERT_EQ (1, spacing.size ());
-	std::this_thread::sleep_for (length);
-	spacing.flag (root2, hash4);
-	ASSERT_EQ (1, spacing.size ());
-}
-
-TEST (vote_spacing, vote_generator)
+TEST (vote_generator, vote_spacing)
 {
 	nano::node_config config;
 	config.backlog_scan.enable = false;
@@ -175,19 +185,20 @@ TEST (vote_spacing, vote_generator)
 				 .build ();
 	ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
 	ASSERT_EQ (0, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
-	node.generator.add (nano::dev::genesis->hash (), send1->hash ());
-	ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 1);
+	node.generator.add (*send1);
+	ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
+
 	ASSERT_FALSE (node.ledger.rollback (node.ledger.tx_begin_write (), send1->hash ()));
 	ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2));
-	node.generator.add (nano::dev::genesis->hash (), send2->hash ());
-	ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing), 1);
-	ASSERT_EQ (1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
-	std::this_thread::sleep_for (config.network_params.voting.delay);
-	node.generator.add (nano::dev::genesis->hash (), send2->hash ());
-	ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 2);
+	node.generator.add (*send2);
+	ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
+	// vote on send2 second time (rapid succession) - expect spacing
+	node.generator.add (*send2);
+	ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
+	ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
 }
 
-TEST (vote_spacing, rapid)
+TEST (vote_generator, vote_spacing_rapid)
 {
 	nano::node_config config;
 	config.backlog_scan.enable = false;
@@ -218,14 +229,22 @@ TEST (vote_spacing, rapid)
 				 .work (*system.work.generate (nano::dev::genesis->hash ()))
 				 .build ();
 	ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send1));
-	node.generator.add (nano::dev::genesis->hash (), send1->hash ());
-	ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 1);
+	node.generator.add (*send1);
+	ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
 	ASSERT_FALSE (node.ledger.rollback (node.ledger.tx_begin_write (), send1->hash ()));
 	ASSERT_EQ (nano::block_status::progress, node.ledger.process (node.ledger.tx_begin_write (), send2));
-	node.generator.add (nano::dev::genesis->hash (), send2->hash ());
-	ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing), 1);
-	ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
+	// vote on send2 first time - expect : new broadcast & no spacing
+	node.generator.add (*send2);
+	ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
+	ASSERT_TIMELY_EQ (3s, 0, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
+
+	// vote on send2 again rapidly - expect : no broadcast & new spacing
+	node.generator.add (*send2);
+	ASSERT_TIMELY_EQ (3s, 2, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
+	ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
 	std::this_thread::sleep_for (config.network_params.voting.delay);
-	node.generator.add (nano::dev::genesis->hash (), send2->hash ());
-	ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts), 2);
+	// vote on send2 again after the spacing delay - expect : new broadcast & no spacing
+	node.generator.add (*send2);
+	ASSERT_TIMELY_EQ (3s, 3, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_broadcasts));
+	ASSERT_TIMELY_EQ (3s, 1, node.stats.count (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing));
 }

nano/node/election.cpp

@@ -463,7 +463,7 @@ void nano::election::confirm_if_quorum (nano::unique_lock<nano::mutex> & lock_a)
 		if (!is_quorum.exchange (true) && node.config.enable_voting && node.wallets.reps ().voting > 0)
 		{
 			++vote_broadcast_count;
-			node.final_generator.add (root, status.winner->hash ());
+			node.final_generator.add (*status.winner);
 		}
 		if (final_weight >= node.online_reps.delta ())
 		{
@@ -668,7 +668,7 @@ void nano::election::broadcast_vote_locked (nano::unique_lock<nano::mutex> & loc
 			nano::log::arg{ "winner", status.winner },
 			nano::log::arg{ "type", "final" });
 
-			node.final_generator.add (root, status.winner->hash ()); // Broadcasts vote to the network
+			node.final_generator.add (*status.winner); // Broadcasts vote to the network
 		}
 		else
 		{
@@ -679,7 +679,7 @@ void nano::election::broadcast_vote_locked (nano::unique_lock<nano::mutex> & loc
 			nano::log::arg{ "winner", status.winner },
 			nano::log::arg{ "type", "normal" });
 
-			node.generator.add (root, status.winner->hash ()); // Broadcasts vote to the network
+			node.generator.add (*status.winner); // Broadcasts vote to the network
 		}
 	}
 }

nano/node/vote_generator.cpp

@@ -43,7 +43,7 @@ nano::vote_generator::~vote_generator ()
 	debug_assert (!thread.joinable ());
 }
 
-bool nano::vote_generator::should_vote (transaction_variant_t const & transaction_variant, nano::root const & root_a, nano::block_hash const & hash_a) const
+bool nano::vote_generator::should_vote (transaction_variant_t const & transaction_variant, nano::qualified_root const & root_a, nano::block_hash const & hash_a) const
 {
 	bool should_vote = false;
 	std::shared_ptr<nano::block> block;
@@ -54,7 +54,7 @@ bool nano::vote_generator::should_vote (transaction_variant_t const & transactio
 
 		block = ledger.any.block_get (transaction, hash_a);
 		should_vote = block != nullptr && ledger.dependents_confirmed (transaction, *block) && ledger.store.final_vote.put (transaction, block->qualified_root (), hash_a);
-		debug_assert (block == nullptr || root_a == block->root ());
+		debug_assert (block == nullptr || root_a == block->qualified_root ());
 	}
 	else
 	{
@@ -98,9 +98,9 @@ void nano::vote_generator::stop ()
 	}
 }
 
-void nano::vote_generator::add (const root & root, const block_hash & hash)
+void nano::vote_generator::add (nano::block const & block)
 {
-	vote_generation_queue.add (std::make_pair (root, hash));
+	vote_generation_queue.add (std::make_pair (block.qualified_root (), block.hash ()));
 }
 
 void nano::vote_generator::process_batch (std::deque<queue_entry_t> & batch)
@@ -157,7 +157,7 @@ std::size_t nano::vote_generator::generate (std::vector<std::shared_ptr<nano::bl
 			return this->ledger.dependents_confirmed (transaction, *block_a);
 		};
 		auto as_candidate = [] (auto const & block_a) {
-			return candidate_t{ block_a->root (), block_a->hash () };
+			return candidate_t{ block_a->qualified_root (), block_a->hash () };
 		};
 		nano::transform_if (blocks_a.begin (), blocks_a.end (), std::back_inserter (req_candidates), dependents_confirmed, as_candidate);
 	}
@@ -178,7 +178,7 @@ void nano::vote_generator::broadcast (nano::unique_lock<nano::mutex> & lock_a)
 	debug_assert (lock_a.owns_lock ());
 
 	std::vector<nano::block_hash> hashes;
-	std::vector<nano::root> roots;
+	std::vector<nano::qualified_root> roots;
 	hashes.reserve (nano::network::confirm_ack_hashes_max);
 	roots.reserve (nano::network::confirm_ack_hashes_max);
 	while (!candidates.empty () && hashes.size () < nano::network::confirm_ack_hashes_max)
@@ -222,23 +222,17 @@ void nano::vote_generator::reply (nano::unique_lock<nano::mutex> & lock_a, reque
 	while (i != n && !stopped)
 	{
 		std::vector<nano::block_hash> hashes;
-		std::vector<nano::root> roots;
+		std::vector<nano::qualified_root> roots;
 		hashes.reserve (nano::network::confirm_ack_hashes_max);
 		roots.reserve (nano::network::confirm_ack_hashes_max);
 		for (; i != n && hashes.size () < nano::network::confirm_ack_hashes_max; ++i)
 		{
 			auto const & [root, hash] = *i;
 			if (std::find (roots.begin (), roots.end (), root) == roots.end ())
 			{
-				if (spacing.votable (root, hash))
-				{
-					roots.push_back (root);
-					hashes.push_back (hash);
-				}
-				else
-				{
-					stats.inc (nano::stat::type::vote_generator, nano::stat::detail::generator_spacing);
-				}
+				// Vote spacing skipped for replies - allows all peers to request same vote
+				roots.push_back (root);
+				hashes.push_back (hash);
 			}
 		}
 		if (!hashes.empty ())
@@ -256,7 +250,7 @@ void nano::vote_generator::reply (nano::unique_lock<nano::mutex> & lock_a, reque
 	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)
+void nano::vote_generator::vote (std::vector<nano::block_hash> const & hashes_a, std::vector<nano::qualified_root> const & roots_a, std::function<void (std::shared_ptr<nano::vote> const &)> const & action_a)
 {
 	debug_assert (hashes_a.size () == roots_a.size ());
 	std::vector<std::shared_ptr<nano::vote>> votes_l;
@@ -269,7 +263,7 @@ void nano::vote_generator::vote (std::vector<nano::block_hash> const & hashes_a,
 	{
 		for (std::size_t i (0), n (hashes_a.size ()); i != n; ++i)
 		{
-			history.add (roots_a[i], hashes_a[i], vote_l);
+			history.add (roots_a[i].root (), hashes_a[i], vote_l);
 			spacing.flag (roots_a[i], hashes_a[i]);
 		}
 		action_a (vote_l);

nano/node/vote_generator.hpp

@@ -27,19 +27,19 @@ namespace nano
 class vote_generator final
 {
 private:
-	using candidate_t = std::pair<nano::root, nano::block_hash>;
+	using candidate_t = std::pair<nano::qualified_root, nano::block_hash>;
 	using request_t = std::pair<std::vector<candidate_t>, std::shared_ptr<nano::transport::channel>>;
-	using queue_entry_t = std::pair<nano::root, nano::block_hash>;
+	using queue_entry_t = std::pair<nano::qualified_root, nano::block_hash>;
 	std::chrono::steady_clock::time_point next_broadcast = { std::chrono::steady_clock::now () };
 
 public:
 	vote_generator (nano::node_config const &, nano::node &, nano::ledger &, nano::wallets &, nano::vote_processor &, nano::local_vote_history &, nano::network &, nano::stats &, nano::logger &, bool is_final);
 	~vote_generator ();
 
 	/** Queue items for vote generation, or broadcast votes already in cache */
-	void add (nano::root const &, nano::block_hash const &);
+	void add (nano::block const & block);
 	/** Queue blocks for vote generation, returning the number of successful candidates.*/
-	std::size_t generate (std::vector<std::shared_ptr<nano::block>> const & blocks_a, std::shared_ptr<nano::transport::channel> const & channel_a);
+	std::size_t generate (std::vector<std::shared_ptr<nano::block>> const & blocks, std::shared_ptr<nano::transport::channel> const & channel);
 
 	void start ();
 	void stop ();
@@ -52,10 +52,10 @@ class vote_generator final
 	void run ();
 	void broadcast (nano::unique_lock<nano::mutex> &);
 	void reply (nano::unique_lock<nano::mutex> &, request_t &&);
-	void vote (std::vector<nano::block_hash> const &, std::vector<nano::root> const &, std::function<void (std::shared_ptr<nano::vote> const &)> const &);
+	void vote (std::vector<nano::block_hash> const &, std::vector<nano::qualified_root> const &, std::function<void (std::shared_ptr<nano::vote> const &)> const &);
 	void broadcast_action (std::shared_ptr<nano::vote> const &) const;
 	void process_batch (std::deque<queue_entry_t> & batch);
-	bool should_vote (transaction_variant_t const &, nano::root const &, nano::block_hash const &) const;
+	bool should_vote (transaction_variant_t const &, nano::qualified_root const &, nano::block_hash const &) const;
 	bool broadcast_predicate () const;
 
 private: // Dependencies

nano/node/vote_spacing.cpp

@@ -1,35 +1,39 @@
 #include <nano/node/vote_spacing.hpp>
 
-void nano::vote_spacing::trim ()
+void nano::vote_spacing::trim (std::chrono::steady_clock::time_point now)
 {
-	recent.get<tag_time> ().erase (recent.get<tag_time> ().begin (), recent.get<tag_time> ().upper_bound (std::chrono::steady_clock::now () - delay));
+	recent.get<tag_time> ().erase (recent.get<tag_time> ().begin (), recent.get<tag_time> ().upper_bound (now - delay));
 }
 
-bool nano::vote_spacing::votable (nano::root const & root_a, nano::block_hash const & hash_a) const
+bool nano::vote_spacing::votable (nano::qualified_root const & root, nano::block_hash const & hash, std::chrono::steady_clock::time_point now) const
 {
-	bool result = true;
-	for (auto range = recent.get<tag_root> ().equal_range (root_a); result && range.first != range.second; ++range.first)
+	if (auto it = recent.get<tag_root> ().find (root); it != recent.end ())
 	{
-		auto & item = *range.first;
-		result = hash_a == item.hash || item.time < std::chrono::steady_clock::now () - delay;
+		if (hash == it->hash && it->time >= now - delay)
+		{
+			return false; // Same hash and not expired -> not votable
+		}
 	}
-	return result;
+	return true; // Either different hash or expired -> votable
 }
 
-void nano::vote_spacing::flag (nano::root const & root_a, nano::block_hash const & hash_a)
+void nano::vote_spacing::flag (nano::qualified_root const & root, nano::block_hash const & hash, std::chrono::steady_clock::time_point now)
 {
-	trim ();
-	auto now = std::chrono::steady_clock::now ();
-	auto existing = recent.get<tag_root> ().find (root_a);
-	if (existing != recent.end ())
+	trim (now);
+
+	if (auto it = recent.get<tag_root> ().find (root); it != recent.end ())
 	{
-		recent.get<tag_root> ().modify (existing, [now] (entry & entry) {
+		// We update both timestamp and hash because we want to track which fork
+		// we most recently voted for. This ensures proper spacing between votes
+		// for the same block while allowing immediate votes for competing forks.
+		recent.get<tag_root> ().modify (it, [now, hash] (entry & entry) {
+			entry.hash = hash;
 			entry.time = now;
 		});
 	}
 	else
 	{
-		recent.insert ({ root_a, now, hash_a });
+		recent.insert ({ root, hash, now });
 	}
 }