Fix vote_uniquer (#4339)

* Introduce generic `uniquer` class

* Convert `block_uniquer` to use generic implementation

* Convert `vote_uniquer` to use generic implementation

* Adjust `uniquer` class container info reporting

* Use nano specific lock primitives

nano/core_test/conflicts.cpp

@@ -179,15 +179,13 @@ TEST (conflicts, add_two)
 
 TEST (vote_uniquer, null)
 {
-	nano::block_uniquer block_uniquer;
-	nano::vote_uniquer uniquer (block_uniquer);
+	nano::vote_uniquer uniquer;
 	ASSERT_EQ (nullptr, uniquer.unique (nullptr));
 }
 
 TEST (vote_uniquer, vbh_one)
 {
-	nano::block_uniquer block_uniquer;
-	nano::vote_uniquer uniquer (block_uniquer);
+	nano::vote_uniquer uniquer;
 	nano::keypair key;
 	nano::block_builder builder;
 	auto block = builder
@@ -210,8 +208,7 @@ TEST (vote_uniquer, vbh_one)
 
 TEST (vote_uniquer, vbh_two)
 {
-	nano::block_uniquer block_uniquer;
-	nano::vote_uniquer uniquer (block_uniquer);
+	nano::vote_uniquer uniquer;
 	nano::keypair key;
 	nano::block_builder builder;
 	auto block1 = builder
@@ -246,8 +243,7 @@ TEST (vote_uniquer, vbh_two)
 
 TEST (vote_uniquer, cleanup)
 {
-	nano::block_uniquer block_uniquer;
-	nano::vote_uniquer uniquer (block_uniquer);
+	nano::vote_uniquer uniquer;
 	nano::keypair key;
 	auto vote1 = std::make_shared<nano::vote> (key.pub, key.prv, 0, 0, std::vector<nano::block_hash>{ nano::block_hash{ 0 } });
 	auto vote2 = std::make_shared<nano::vote> (key.pub, key.prv, nano::vote::timestamp_min * 1, 0, std::vector<nano::block_hash>{ nano::block_hash{ 0 } });
@@ -256,10 +252,7 @@ TEST (vote_uniquer, cleanup)
 	vote2.reset ();
 	vote4.reset ();
 	ASSERT_EQ (2, uniquer.size ());
-	auto iterations (0);
-	while (uniquer.size () == 2)
-	{
-		auto vote5 (uniquer.unique (vote1));
-		ASSERT_LT (iterations++, 200);
-	}
+	std::this_thread::sleep_for (nano::block_uniquer::cleanup_cutoff);
+	auto vote5 = uniquer.unique (vote1);
+	ASSERT_EQ (1, uniquer.size ());
 }

nano/core_test/message_deserializer.cpp

@@ -17,7 +17,7 @@ auto message_deserializer_success_checker (message_type & message_original) -> v
 	// Dependencies for the message deserializer.
 	nano::network_filter filter (1);
 	nano::block_uniquer block_uniquer;
-	nano::vote_uniquer vote_uniquer (block_uniquer);
+	nano::vote_uniquer vote_uniquer;
 
 	// Data used to simulate the incoming buffer to be deserialized, the offset tracks how much has been read from the input_source
 	// as the read function is called first to read the header, then called again to read the payload.

nano/lib/CMakeLists.txt

@@ -91,6 +91,7 @@ add_library(
   tlsconfig.cpp
   tomlconfig.hpp
   tomlconfig.cpp
+  uniquer.hpp
   utility.hpp
   utility.cpp
   walletconfig.hpp

nano/lib/blocks.cpp

@@ -1864,54 +1864,3 @@ bool nano::block_sideband::deserialize (nano::stream & stream_a, nano::block_typ
 
 	return result;
 }
-
-std::shared_ptr<nano::block> nano::block_uniquer::unique (std::shared_ptr<nano::block> const & block_a)
-{
-	auto result (block_a);
-	if (result != nullptr)
-	{
-		nano::uint256_union key (block_a->full_hash ());
-		nano::lock_guard<nano::mutex> lock{ mutex };
-		auto & existing (blocks[key]);
-		if (auto block_l = existing.lock ())
-		{
-			result = block_l;
-		}
-		else
-		{
-			existing = block_a;
-		}
-		auto now = std::chrono::steady_clock::now ();
-		if (cleanup_cutoff < now - cleanup_last)
-		{
-			cleanup_last = now;
-			for (auto i = blocks.begin (), n = blocks.end (); i != n;)
-			{
-				if (auto block_l = i->second.lock ())
-				{
-					++i;
-				}
-				else
-				{
-					i = blocks.erase (i);
-				}
-			}
-		}
-	}
-	return result;
-}
-
-size_t nano::block_uniquer::size ()
-{
-	nano::lock_guard<nano::mutex> lock{ mutex };
-	return blocks.size ();
-}
-
-std::unique_ptr<nano::container_info_component> nano::collect_container_info (block_uniquer & block_uniquer, std::string const & name)
-{
-	auto count = block_uniquer.size ();
-	auto sizeof_element = sizeof (block_uniquer::value_type);
-	auto composite = std::make_unique<container_info_composite> (name);
-	composite->add_component (std::make_unique<container_info_leaf> (container_info{ "blocks", count, sizeof_element }));
-	return composite;
-}

nano/lib/blocks.hpp

@@ -7,6 +7,7 @@
 #include <nano/lib/optional_ptr.hpp>
 #include <nano/lib/stream.hpp>
 #include <nano/lib/timer.hpp>
+#include <nano/lib/uniquer.hpp>
 #include <nano/lib/utility.hpp>
 #include <nano/lib/work.hpp>
 
@@ -399,27 +400,8 @@ class mutable_block_visitor
 	virtual void state_block (nano::state_block &) = 0;
 	virtual ~mutable_block_visitor () = default;
 };
-/**
- * This class serves to find and return unique variants of a block in order to minimize memory usage
- */
-class block_uniquer
-{
-public:
-	using value_type = std::pair<nano::uint256_union const, std::weak_ptr<nano::block>>;
-
-	std::shared_ptr<nano::block> unique (std::shared_ptr<nano::block> const &);
-	size_t size ();
-
-private:
-	nano::mutex mutex{ mutex_identifier (mutexes::block_uniquer) };
-	std::unordered_map<std::remove_const_t<value_type::first_type>, value_type::second_type> blocks;
-	std::chrono::steady_clock::time_point cleanup_last{ std::chrono::steady_clock::now () };
-
-public:
-	static std::chrono::milliseconds constexpr cleanup_cutoff{ 500 };
-};
 
-std::unique_ptr<container_info_component> collect_container_info (block_uniquer & block_uniquer, std::string const & name);
+using block_uniquer = nano::uniquer<nano::uint256_union, nano::block>;
 
 std::shared_ptr<nano::block> deserialize_block (nano::stream &);
 std::shared_ptr<nano::block> deserialize_block (nano::stream &, nano::block_type, nano::block_uniquer * = nullptr);

nano/lib/uniquer.hpp

@@ -0,0 +1,80 @@
+#pragma once
+
+#include <nano/lib/interval.hpp>
+#include <nano/lib/locks.hpp>
+#include <nano/lib/utility.hpp>
+
+#include <memory>
+
+namespace nano
+{
+template <typename Key, typename Value>
+class uniquer final
+{
+public:
+	using key_type = Key;
+	using value_type = Value;
+
+	std::shared_ptr<Value> unique (std::shared_ptr<Value> const & value)
+	{
+		if (value == nullptr)
+		{
+			return nullptr;
+		}
+
+		// Types used as value need to provide full_hash()
+		Key hash = value->full_hash ();
+
+		nano::lock_guard<nano::mutex> guard{ mutex };
+
+		if (cleanup_interval.elapsed ())
+		{
+			cleanup ();
+		}
+
+		auto & existing = values[hash];
+		if (auto result = existing.lock ())
+		{
+			return result;
+		}
+		else
+		{
+			existing = value;
+		}
+
+		return value;
+	}
+
+	std::size_t size () const
+	{
+		nano::lock_guard<nano::mutex> guard{ mutex };
+		return values.size ();
+	}
+
+	std::unique_ptr<container_info_component> collect_container_info (std::string const & name) const
+	{
+		nano::lock_guard<nano::mutex> guard{ mutex };
+
+		auto composite = std::make_unique<container_info_composite> (name);
+		composite->add_component (std::make_unique<container_info_leaf> (container_info{ "cache", values.size (), sizeof (Value) }));
+		return composite;
+	}
+
+	static std::chrono::milliseconds constexpr cleanup_cutoff{ 500 };
+
+private:
+	void cleanup ()
+	{
+		debug_assert (!mutex.try_lock ());
+
+		std::erase_if (values, [] (auto const & item) {
+			return item.second.expired ();
+		});
+	}
+
+private:
+	mutable nano::mutex mutex;
+	std::unordered_map<Key, std::weak_ptr<Value>> values;
+	nano::interval cleanup_interval{ cleanup_cutoff };
+};
+}

nano/node/node.cpp

@@ -180,7 +180,7 @@ nano::node::node (boost::asio::io_context & io_ctx_a, std::filesystem::path cons
 	block_processor (*this, write_database_queue),
 	online_reps (ledger, config),
 	history{ config.network_params.voting },
-	vote_uniquer (block_uniquer),
+	vote_uniquer{},
 	confirmation_height_processor (ledger, write_database_queue, config.conf_height_processor_batch_min_time, config.logging, logger, node_initialized_latch, flags.confirmation_height_processor_mode),
 	vote_cache{ config.vote_cache, stats },
 	generator{ config, ledger, wallets, vote_processor, history, network, stats, /* non-final */ false },
@@ -565,8 +565,8 @@ std::unique_ptr<nano::container_info_component> nano::collect_container_info (no
 	composite->add_component (collect_container_info (node.block_arrival, "block_arrival"));
 	composite->add_component (collect_container_info (node.online_reps, "online_reps"));
 	composite->add_component (collect_container_info (node.history, "history"));
-	composite->add_component (collect_container_info (node.block_uniquer, "block_uniquer"));
-	composite->add_component (collect_container_info (node.vote_uniquer, "vote_uniquer"));
+	composite->add_component (node.block_uniquer.collect_container_info ("block_uniquer"));
+	composite->add_component (node.vote_uniquer.collect_container_info ("vote_uniquer"));
 	composite->add_component (collect_container_info (node.confirmation_height_processor, "confirmation_height_processor"));
 	composite->add_component (collect_container_info (node.distributed_work, "distributed_work"));
 	composite->add_component (collect_container_info (node.aggregator, "request_aggregator"));

nano/rpc_test/rpc.cpp

@@ -5810,7 +5810,7 @@ TEST (rpc, memory_stats)
 	{
 		auto response (wait_response (system, rpc_ctx, request));
 
-		ASSERT_EQ (response.get_child ("node").get_child ("vote_uniquer").get_child ("votes").get<std::string> ("count"), "1");
+		ASSERT_EQ (response.get_child ("node").get_child ("vote_uniquer").get_child ("cache").get<std::string> ("count"), "1");
 	}
 
 	request.put ("type", "database");

nano/secure/common.cpp

@@ -430,69 +430,6 @@ nano::block_info::block_info (nano::account const & account_a, nano::amount cons
 {
 }
 
-nano::vote_uniquer::vote_uniquer (nano::block_uniquer & uniquer_a) :
-	uniquer (uniquer_a)
-{
-}
-
-std::shared_ptr<nano::vote> nano::vote_uniquer::unique (std::shared_ptr<nano::vote> const & vote_a)
-{
-	auto result = vote_a;
-	if (result != nullptr)
-	{
-		nano::block_hash key = vote_a->full_hash ();
-		nano::lock_guard<nano::mutex> lock{ mutex };
-		auto & existing = votes[key];
-		if (auto block_l = existing.lock ())
-		{
-			result = block_l;
-		}
-		else
-		{
-			existing = vote_a;
-		}
-
-		release_assert (std::numeric_limits<CryptoPP::word32>::max () > votes.size ());
-		for (auto i (0); i < cleanup_count && !votes.empty (); ++i)
-		{
-			auto random_offset = nano::random_pool::generate_word32 (0, static_cast<CryptoPP::word32> (votes.size () - 1));
-
-			auto existing (std::next (votes.begin (), random_offset));
-			if (existing == votes.end ())
-			{
-				existing = votes.begin ();
-			}
-			if (existing != votes.end ())
-			{
-				if (auto block_l = existing->second.lock ())
-				{
-					// Still live
-				}
-				else
-				{
-					votes.erase (existing);
-				}
-			}
-		}
-	}
-	return result;
-}
-
-size_t nano::vote_uniquer::size ()
-{
-	nano::lock_guard<nano::mutex> lock{ mutex };
-	return votes.size ();
-}
-
-std::unique_ptr<nano::container_info_component> nano::collect_container_info (vote_uniquer & vote_uniquer, std::string const & name)
-{
-	auto count = vote_uniquer.size ();
-	auto sizeof_element = sizeof (vote_uniquer::value_type);
-	auto composite = std::make_unique<container_info_composite> (name);
-	composite->add_component (std::make_unique<container_info_leaf> (container_info{ "votes", count, sizeof_element }));
-	return composite;
-}
-
 nano::wallet_id nano::random_wallet_id ()
 {
 	nano::wallet_id wallet_id;

nano/secure/common.hpp

@@ -244,27 +244,6 @@ namespace confirmation_height
 	uint64_t const unbounded_cutoff{ 16384 };
 }
 
-/**
- * This class serves to find and return unique variants of a vote in order to minimize memory usage
- */
-class vote_uniquer final
-{
-public:
-	using value_type = std::pair<nano::block_hash const, std::weak_ptr<nano::vote>>;
-
-	vote_uniquer (nano::block_uniquer &);
-	std::shared_ptr<nano::vote> unique (std::shared_ptr<nano::vote> const &);
-	size_t size ();
-
-private:
-	nano::block_uniquer & uniquer;
-	nano::mutex mutex{ mutex_identifier (mutexes::vote_uniquer) };
-	std::unordered_map<std::remove_const_t<value_type::first_type>, value_type::second_type> votes;
-	static unsigned constexpr cleanup_count = 2;
-};
-
-std::unique_ptr<container_info_component> collect_container_info (vote_uniquer & vote_uniquer, std::string const & name);
-
 enum class vote_code
 {
 	invalid, // Vote is not signed correctly

nano/secure/vote.hpp

@@ -3,6 +3,7 @@
 #include <nano/lib/numbers.hpp>
 #include <nano/lib/stream.hpp>
 #include <nano/lib/timer.hpp>
+#include <nano/lib/uniquer.hpp>
 
 #include <boost/iterator/transform_iterator.hpp>
 #include <boost/property_tree/ptree_fwd.hpp>
@@ -77,4 +78,6 @@ class vote final
 	// Vote timestamp
 	uint64_t timestamp_m{ 0 };
 };
+
+using vote_uniquer = nano::uniquer<nano::block_hash, nano::vote>;
 }