From 1c7e3880ca1954ee3789fb25da6727aa0e15c1b8 Mon Sep 17 00:00:00 2001 From: Guilherme Lawless Date: Wed, 9 Sep 2020 23:08:03 +0100 Subject: [PATCH] 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 | 45 +++++++++++++++++++++++++++++++ nano/node/active_transactions.cpp | 6 +++++ nano/node/active_transactions.hpp | 1 + nano/node/online_reps.cpp | 14 +++++++--- nano/node/online_reps.hpp | 3 ++- nano/node/repcrawler.cpp | 4 +++ 6 files changed, 68 insertions(+), 5 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 9fc02bf7..8908d70c 100644 --- a/nano/core_test/node.cpp +++ b/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 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) diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index 88569744..0c106d27 100644 --- a/nano/node/active_transactions.cpp +++ b/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 guard (mutex); + recently_confirmed.get ().erase (hash_a); +} + void nano::active_transactions::erase (nano::block const & block_a) { nano::unique_lock lock (mutex); diff --git a/nano/node/active_transactions.hpp b/nano/node/active_transactions.hpp index f07d9451..9331ac8e 100644 --- a/nano/node/active_transactions.hpp +++ b/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 const &); diff --git a/nano/node/online_reps.cpp b/nano/node/online_reps.cpp index 92a03370..3f825ad3 100644 --- a/nano/node/online_reps.cpp +++ b/nano/node/online_reps.cpp @@ -39,6 +39,7 @@ void nano::online_reps::sample () std::unordered_set reps_copy; { nano::lock_guard 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::online_reps::list () { std::vector result; - nano::lock_guard lock (mutex); - result.insert (result.end (), reps.begin (), reps.end ()); + decltype (reps) all_reps; + { + nano::lock_guard 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::collect_container_info (on size_t count; { nano::lock_guard 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 (name); composite->add_component (std::make_unique (container_info{ "reps", count, sizeof_element })); return composite; diff --git a/nano/node/online_reps.hpp b/nano/node/online_reps.hpp index 216b8238..8d7bb104 100644 --- a/nano/node/online_reps.hpp +++ b/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 list (); private: @@ -33,6 +33,7 @@ private: nano::ledger & ledger; nano::network_params & network_params; std::unordered_set reps; + std::unordered_set last_reps; nano::uint128_t online; nano::uint128_t minimum; diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 2ded458e..9b49762a 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -148,6 +148,10 @@ void nano::rep_crawler::query (std::vector