Fix inconsistent online representatives list from RPC (#2874)

* Erase rep crawler queried hashes from active recently_confirmed

This minor change increases rep crawler and online reps consistency in small ledgers with low activity - mostly for the beta network.

* Make a backup of online reps to avoid an incorrectly reported list via RPC

The list of online reps is only used externally by RPC, but it has been noted that it is often inconsistent. This is due to clearing the list upon sampling online weight.

With this change, a backup is made on each sample, and RPC will now report the list of both currently sampling reps, and the ones previously sampled.

nano/core_test/node.cpp

@@ -4537,6 +4537,51 @@ TEST (node, default_difficulty)
 	ASSERT_EQ (thresholds.epoch_2_receive, node.default_receive_difficulty (nano::work_version::work_1));
 }
 
+TEST (rep_crawler, recently_confirmed)
+{
+	nano::system system (1);
+	auto & node1 (*system.nodes[0]);
+	ASSERT_EQ (1, node1.ledger.cache.block_count);
+	auto const block = nano::genesis ().open;
+	{
+		nano::lock_guard<std::mutex> guard (node1.active.mutex);
+		node1.active.add_recently_confirmed (block->qualified_root (), block->hash ());
+	}
+	auto & node2 (*system.add_node ());
+	system.wallet (1)->insert_adhoc (nano::dev_genesis_key.prv);
+	auto channel = node1.network.find_channel (node2.network.endpoint ());
+	ASSERT_NE (nullptr, channel);
+	node1.rep_crawler.query (channel);
+	ASSERT_TIMELY (3s, node1.rep_crawler.representative_count () == 1);
+}
+
+TEST (online_reps, backup)
+{
+	nano::logger_mt logger;
+	auto store = nano::make_store (logger, nano::unique_path ());
+	ASSERT_TRUE (!store->init_error ());
+	nano::stat stats;
+	nano::ledger ledger (*store, stats);
+	{
+		nano::genesis genesis;
+		auto transaction (store->tx_begin_write ());
+		store->initialize (transaction, genesis, ledger.cache);
+	}
+	nano::network_params params;
+	nano::online_reps online_reps (ledger, params, 0);
+	ASSERT_EQ (0, online_reps.list ().size ());
+	online_reps.observe (nano::dev_genesis_key.pub);
+	// The reported list of online reps is the union of the current list and the backup list, which changes when sampling
+	ASSERT_EQ (1, online_reps.list ().size ());
+	ASSERT_TRUE (online_reps.online_stake ().is_zero ());
+	online_reps.sample ();
+	ASSERT_EQ (1, online_reps.list ().size ());
+	// The trend is also correctly updated
+	ASSERT_EQ (nano::genesis_amount, online_reps.online_stake ());
+	online_reps.sample ();
+	ASSERT_EQ (0, online_reps.list ().size ());
+}
+
 namespace
 {
 void add_required_children_node_config_tree (nano::jsonconfig & tree)

nano/node/active_transactions.cpp

@@ -1150,6 +1150,12 @@ void nano::active_transactions::add_recently_confirmed (nano::qualified_root con
 	}
 }
 
+void nano::active_transactions::erase_recently_confirmed (nano::block_hash const & hash_a)
+{
+	nano::lock_guard<std::mutex> guard (mutex);
+	recently_confirmed.get<tag_hash> ().erase (hash_a);
+}
+
 void nano::active_transactions::erase (nano::block const & block_a)
 {
 	nano::unique_lock<std::mutex> lock (mutex);

nano/node/active_transactions.hpp

@@ -212,6 +212,7 @@ public:
 
 	void add_recently_cemented (nano::election_status const &);
 	void add_recently_confirmed (nano::qualified_root const &, nano::block_hash const &);
+	void erase_recently_confirmed (nano::block_hash const &);
 	void add_inactive_votes_cache (nano::block_hash const &, nano::account const &);
 	// Inserts an election if conditions are met
 	void trigger_inactive_votes_cache_election (std::shared_ptr<nano::block> const &);

nano/node/online_reps.cpp

@@ -39,6 +39,7 @@ void nano::online_reps::sample ()
 	std::unordered_set<nano::account> reps_copy;
 	{
 		nano::lock_guard<std::mutex> lock (mutex);
+		last_reps = reps;
 		reps_copy.swap (reps);
 	}
 	for (auto & i : reps_copy)
@@ -76,8 +77,13 @@ nano::uint128_t nano::online_reps::online_stake () const
 std::vector<nano::account> nano::online_reps::list ()
 {
 	std::vector<nano::account> result;
-	nano::lock_guard<std::mutex> lock (mutex);
-	result.insert (result.end (), reps.begin (), reps.end ());
+	decltype (reps) all_reps;
+	{
+		nano::lock_guard<std::mutex> lock (mutex);
+		all_reps.insert (last_reps.begin (), last_reps.end ());
+		all_reps.insert (reps.begin (), reps.end ());
+	}
+	result.insert (result.end (), all_reps.begin (), all_reps.end ());
 	return result;
 }
 
@@ -86,10 +92,10 @@ std::unique_ptr<nano::container_info_component> nano::collect_container_info (on
 	size_t count;
 	{
 		nano::lock_guard<std::mutex> guard (online_reps.mutex);
-		count = online_reps.reps.size ();
+		count = online_reps.last_reps.size ();
 	}
 
-	auto sizeof_element = sizeof (decltype (online_reps.reps)::value_type);
+	auto sizeof_element = sizeof (decltype (online_reps.last_reps)::value_type);
 	auto composite = std::make_unique<container_info_composite> (name);
 	composite->add_component (std::make_unique<container_info_leaf> (container_info{ "reps", count, sizeof_element }));
 	return composite;

nano/node/online_reps.hpp

@@ -24,7 +24,7 @@ public:
 	void sample ();
 	/** Returns the trended online stake, but never less than configured minimum */
 	nano::uint128_t online_stake () const;
-	/** List of online representatives */
+	/** List of online representatives, both the currently sampling ones and the ones observed in the previous sampling period */
 	std::vector<nano::account> list ();
 
 private:
@@ -33,6 +33,7 @@ private:
 	nano::ledger & ledger;
 	nano::network_params & network_params;
 	std::unordered_set<nano::account> reps;
+	std::unordered_set<nano::account> last_reps;
 	nano::uint128_t online;
 	nano::uint128_t minimum;
 

nano/node/repcrawler.cpp

@@ -148,6 +148,10 @@ void nano::rep_crawler::query (std::vector<std::shared_ptr<nano::transport::chan
 		}
 		active.insert (hash);
 	}
+	if (!channels_a.empty ())
+	{
+		node.active.erase_recently_confirmed (hash);
+	}
 	for (auto i (channels_a.begin ()), n (channels_a.end ()); i != n; ++i)
 	{
 		debug_assert (*i != nullptr);