From 27973bc1be2706596f19bff2b21948e814b55ac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 22 Feb 2024 17:00:12 +0100 Subject: [PATCH 01/27] Improve channel logging --- nano/node/transport/channel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nano/node/transport/channel.cpp b/nano/node/transport/channel.cpp index 26680ddc72..15b6e4acf9 100644 --- a/nano/node/transport/channel.cpp +++ b/nano/node/transport/channel.cpp @@ -67,5 +67,5 @@ void nano::transport::channel::operator() (nano::object_stream & obs) const { obs.write ("endpoint", get_endpoint ()); obs.write ("peering_endpoint", get_peering_endpoint ()); - obs.write ("node_id", get_node_id ()); + obs.write ("node_id", get_node_id ().to_node_id ()); } From efe7606e55c38c1f859d0c6ee41fbf0894e55da6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 13 Feb 2024 19:43:21 +0100 Subject: [PATCH 02/27] Organize members --- nano/node/repcrawler.cpp | 58 ++++++++++++++++---------------- nano/node/repcrawler.hpp | 72 +++++++++++++++++++++------------------- 2 files changed, 66 insertions(+), 64 deletions(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 8d7bb5ceee..a807ac68bc 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -25,7 +25,7 @@ void nano::rep_crawler::start () ongoing_crawl (); } -void nano::rep_crawler::validate () +void nano::rep_crawler::validate_and_process () { decltype (responses) responses_l; { @@ -107,7 +107,7 @@ void nano::rep_crawler::ongoing_crawl () auto now (std::chrono::steady_clock::now ()); auto total_weight_l (total_weight ()); cleanup_reps (); - validate (); + validate_and_process (); query (get_crawl_targets (total_weight_l)); auto sufficient_weight (total_weight_l > node.online_reps.delta ()); // If online weight drops below minimum, reach out to preconfigured peers @@ -127,7 +127,7 @@ void nano::rep_crawler::ongoing_crawl () }); } -std::vector> nano::rep_crawler::get_crawl_targets (nano::uint128_t total_weight_a) +std::vector> nano::rep_crawler::get_crawl_targets (nano::uint128_t total_weight_a) const { constexpr std::size_t conservative_count = 10; constexpr std::size_t aggressive_count = 40; @@ -175,15 +175,16 @@ void nano::rep_crawler::query (std::vectorfirst); + active.insert (hash_root->first); // TODO: Is this really necessary? } - for (auto i (channels_a.begin ()), n (channels_a.end ()); i != n; ++i) + for (const auto & channel : channels_a) { - debug_assert (*i != nullptr); - on_rep_request (*i); - node.network.send_confirm_req (*i, *hash_root); + debug_assert (channel != nullptr); + on_rep_request (channel); + node.network.send_confirm_req (channel, *hash_root); } + // TODO: Use a thread+timeout instead of a timer // A representative must respond with a vote within the deadline std::weak_ptr node_w (node.shared ()); node.workers.add_timed_task (std::chrono::steady_clock::now () + std::chrono::seconds (5), [node_w, hash = hash_root->first] () { @@ -197,11 +198,10 @@ void nano::rep_crawler::query (std::vector const & channel_a) { - std::vector> peers; - peers.emplace_back (channel_a); - query (peers); + query (std::vector{ channel_a }); } +// TODO: Use a thread+timeout instead of a timer void nano::rep_crawler::throttled_remove (nano::block_hash const & hash_a, uint64_t const target_finished_processed) { if (node.vote_processor.total_processed >= target_finished_processed) @@ -224,23 +224,23 @@ bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const { nano::lock_guard lock{ probable_reps_mutex }; auto existing = probable_reps.get ().find (channel_a); - bool result = false; if (existing != probable_reps.get ().end ()) { - result = node.ledger.weight (existing->account) > node.minimum_principal_weight (); + return node.ledger.weight (existing->account) > node.minimum_principal_weight (); } - return result; + return false; } -bool nano::rep_crawler::response (std::shared_ptr const & channel_a, std::shared_ptr const & vote_a, bool force) +// TODO: Remove force parameter +bool nano::rep_crawler::response (std::shared_ptr const & channel, std::shared_ptr const & vote, bool force) { bool error = true; nano::lock_guard lock{ active_mutex }; - for (auto i = vote_a->hashes.begin (), n = vote_a->hashes.end (); i != n; ++i) + for (auto const & hash : vote->hashes) { - if (force || active.count (*i) != 0) + if (force || active.count (hash) != 0) { - responses.emplace_back (channel_a, vote_a); + responses.emplace_back (channel, vote); error = false; break; } @@ -252,11 +252,11 @@ nano::uint128_t nano::rep_crawler::total_weight () const { nano::lock_guard lock{ probable_reps_mutex }; nano::uint128_t result (0); - for (auto i (probable_reps.get ().begin ()), n (probable_reps.get ().end ()); i != n; ++i) + for (const auto & i : probable_reps.get ()) { - if (i->channel->alive ()) + if (i.channel->alive ()) { - result += node.ledger.weight (i->account); + result += node.ledger.weight (i.account); } } return result; @@ -267,7 +267,7 @@ void nano::rep_crawler::on_rep_request (std::shared_ptr lock{ probable_reps_mutex }; if (channel_a->get_tcp_endpoint ().address () != boost::asio::ip::address_v6::any ()) { - probably_rep_t::index::type & channel_ref_index = probable_reps.get (); + auto & channel_ref_index = probable_reps.get (); // Find and update the timestamp on all reps available on the endpoint (a single host may have multiple reps) auto itr_pair = channel_ref_index.equal_range (*channel_a); @@ -308,7 +308,7 @@ void nano::rep_crawler::cleanup_reps () if (i->get_type () == nano::transport::transport_type::tcp) { auto find_channel (node.network.tcp_channels.find_channel (i->get_tcp_endpoint ())); - if (find_channel != nullptr && *find_channel == *static_cast (i.get ())) + if (find_channel != nullptr && *find_channel == *static_cast (i.get ())) // TODO: WTF { equal = true; } @@ -327,15 +327,15 @@ void nano::rep_crawler::cleanup_reps () std::vector nano::rep_crawler::representatives (std::size_t count_a, nano::uint128_t const weight_a, boost::optional const & opt_version_min_a) { - auto version_min (opt_version_min_a.value_or (node.network_params.network.protocol_version_min)); + auto version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); std::multimap> ordered; nano::lock_guard lock{ probable_reps_mutex }; - for (auto i (probable_reps.get ().begin ()), n (probable_reps.get ().end ()); i != n; ++i) + for (const auto & i : probable_reps.get ()) { - auto weight = node.ledger.weight (i->account); - if (weight > weight_a && i->channel->get_network_version () >= version_min) + auto weight = node.ledger.weight (i.account); + if (weight > weight_a && i.channel->get_network_version () >= version_min) { - ordered.insert ({ nano::amount{ weight }, *i }); + ordered.insert ({ nano::amount{ weight }, i }); } } std::vector result; @@ -354,7 +354,7 @@ std::vector nano::rep_crawler::principal_representatives ( std::vector> nano::rep_crawler::representative_endpoints (std::size_t count_a) { std::vector> result; - auto reps (representatives (count_a)); + auto reps = representatives (count_a); for (auto const & rep : reps) { result.push_back (rep.channel); diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 70561df031..7926e9a5a7 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -54,24 +54,13 @@ class rep_crawler final { friend std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); - // clang-format off - class tag_account {}; - class tag_channel_ref {}; - class tag_last_request {}; - class tag_sequenced {}; - - using probably_rep_t = boost::multi_index_container, mi::member>, - mi::sequenced>, - mi::ordered_non_unique, - mi::member>, - mi::hashed_non_unique, - mi::const_mem_fun, &representative::channel_ref>>>>; - // clang-format on + friend class active_transactions_confirm_election_by_request_Test; + friend class active_transactions_confirm_frontier_Test; + friend class rep_crawler_local_Test; + friend class node_online_reps_rep_crawler_Test; public: - rep_crawler (nano::node & node_a); + explicit rep_crawler (nano::node & node_a); /** Start crawling */ void start (); @@ -80,7 +69,7 @@ class rep_crawler final void remove (nano::block_hash const &); /** Remove block hash from with delay depending on vote processor size */ - void throttled_remove (nano::block_hash const &, uint64_t const); + void throttled_remove (nano::block_hash const &, uint64_t target_finished_processed); /** Attempt to determine if the peer manages one or more representative accounts */ void query (std::vector> const & channels_a); @@ -103,7 +92,7 @@ class rep_crawler final nano::uint128_t total_weight () const; /** Request a list of the top \p count_a known representatives in descending order of weight, with at least \p weight_a voting weight, and optionally with a minimum version \p opt_version_min_a */ - std::vector representatives (std::size_t count_a = std::numeric_limits::max (), nano::uint128_t const weight_a = 0, boost::optional const & opt_version_min_a = boost::none); + std::vector representatives (std::size_t count_a = std::numeric_limits::max (), nano::uint128_t weight_a = 0, boost::optional const & opt_version_min_a = boost::none); /** Request a list of the top \p count_a known principal representatives in descending order of weight, optionally with a minimum version \p opt_version_min_a */ std::vector principal_representatives (std::size_t count_a = std::numeric_limits::max (), boost::optional const & opt_version_min_a = boost::none); @@ -114,23 +103,18 @@ class rep_crawler final /** Total number of representatives */ std::size_t representative_count (); -private: +private: // Dependencies nano::node & node; - /** Protects the active-hash container */ - nano::mutex active_mutex; - - /** We have solicted votes for these random blocks */ - std::unordered_set active; - +private: // Validate responses to see if they're reps - void validate (); + void validate_and_process (); /** Called continuously to crawl for representatives */ void ongoing_crawl (); /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ - std::vector> get_crawl_targets (nano::uint128_t total_weight_a); + std::vector> get_crawl_targets (nano::uint128_t total_weight_a) const; /** When a rep request is made, this is called to update the last-request timestamp. */ void on_rep_request (std::shared_ptr const & channel_a); @@ -138,17 +122,35 @@ class rep_crawler final /** Clean representatives with inactive channels */ void cleanup_reps (); - /** Protects the probable_reps container */ - mutable nano::mutex probable_reps_mutex; +private: + // clang-format off + class tag_account {}; + class tag_channel_ref {}; + class tag_last_request {}; + class tag_sequenced {}; - /** Probable representatives */ - probably_rep_t probable_reps; + using ordered_probable_reps = boost::multi_index_container, mi::member>, + mi::sequenced>, + mi::ordered_non_unique, + mi::member>, + mi::hashed_non_unique, + mi::const_mem_fun, &representative::channel_ref>>>>; + // clang-format on - friend class active_transactions_confirm_election_by_request_Test; - friend class active_transactions_confirm_frontier_Test; - friend class rep_crawler_local_Test; - friend class node_online_reps_rep_crawler_Test; + /** Probable representatives */ + ordered_probable_reps probable_reps; +private: std::deque, std::shared_ptr>> responses; + + /** We have solicted votes for these random blocks */ + std::unordered_set active; + + /** Protects the active-hash container */ + mutable nano::mutex active_mutex; + /** Protects the probable_reps container */ + mutable nano::mutex probable_reps_mutex; }; } From 167fefef9bcb3093f256f8417d7a16b0a4e82dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 15 Feb 2024 09:38:13 +0100 Subject: [PATCH 03/27] Single mutex --- nano/core_test/active_transactions.cpp | 16 +------- nano/core_test/node.cpp | 24 +++--------- nano/node/node.cpp | 2 +- nano/node/repcrawler.cpp | 53 ++++++++++++++++++++------ nano/node/repcrawler.hpp | 15 +++----- 5 files changed, 55 insertions(+), 55 deletions(-) diff --git a/nano/core_test/active_transactions.cpp b/nano/core_test/active_transactions.cpp index dc59f954f5..d9c350162f 100644 --- a/nano/core_test/active_transactions.cpp +++ b/nano/core_test/active_transactions.cpp @@ -14,8 +14,6 @@ using namespace std::chrono_literals; -namespace nano -{ /* * Tests that an election can be confirmed as the result of a confirmation request * @@ -80,10 +78,7 @@ TEST (active_transactions, confirm_election_by_request) ASSERT_FALSE (peers.empty ()); // Add representative (node1) to disabled rep crawler of node2 - { - nano::lock_guard guard (node2.rep_crawler.probable_reps_mutex); - node2.rep_crawler.probable_reps.emplace (nano::dev::genesis_key.pub, *peers.cbegin ()); - } + node2.rep_crawler.force_add_rep (nano::dev::genesis_key.pub, *peers.cbegin ()); // Expect a vote to come back ASSERT_TIMELY (5s, election->votes ().size () >= 1); @@ -97,10 +92,7 @@ TEST (active_transactions, confirm_election_by_request) ASSERT_TIMELY (5s, nano::test::confirmed (node1, { send1 })); ASSERT_TIMELY (5s, nano::test::confirmed (node2, { send1 })); } -} -namespace nano -{ TEST (active_transactions, confirm_frontier) { nano::test::system system; @@ -144,10 +136,7 @@ TEST (active_transactions, confirm_frontier) // Add representative to disabled rep crawler auto peers (node2.network.random_set (1)); ASSERT_FALSE (peers.empty ()); - { - nano::lock_guard guard (node2.rep_crawler.probable_reps_mutex); - node2.rep_crawler.probable_reps.emplace (nano::dev::genesis_key.pub, *peers.begin ()); - } + node2.rep_crawler.force_add_rep (nano::dev::genesis_key.pub, *peers.begin ()); ASSERT_EQ (nano::block_status::progress, node2.process (send)); ASSERT_TIMELY (5s, !node2.active.empty ()); @@ -160,7 +149,6 @@ TEST (active_transactions, confirm_frontier) ASSERT_TIMELY (5s, node2.active.empty ()); ASSERT_GT (election2->confirmation_request_count, 0u); } -} TEST (active_transactions, keep_local) { diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 35c06a9cfa..439c3dca9f 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2012,8 +2012,6 @@ TEST (node, online_reps) ASSERT_EQ (node1.config.online_weight_minimum, node1.online_reps.trended ()); } -namespace nano -{ TEST (node, online_reps_rep_crawler) { nano::test::system system; @@ -2026,14 +2024,10 @@ TEST (node, online_reps_rep_crawler) node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); ASSERT_EQ (0, node1.online_reps.online ()); // After inserting to rep crawler - { - nano::lock_guard guard{ node1.rep_crawler.probable_reps_mutex }; - node1.rep_crawler.active.insert (nano::dev::genesis->hash ()); - } + node1.rep_crawler.force_active (nano::dev::genesis->hash ()); node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); ASSERT_EQ (nano::dev::constants.genesis_amount, node1.online_reps.online ()); } -} TEST (node, online_reps_election) { @@ -3962,24 +3956,16 @@ TEST (rep_crawler, recently_confirmed) ASSERT_ALWAYS_EQ (0.5s, node1.rep_crawler.representative_count (), 0); } -namespace nano -{ -TEST (rep_crawler, local) +// Votes from local channels should be ignored +TEST (rep_crawler, ignore_local) { nano::test::system system; nano::node_flags flags; - flags.disable_rep_crawler = true; auto & node = *system.add_node (flags); auto loopback = std::make_shared (node, node); auto vote = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - { - nano::lock_guard guard{ node.rep_crawler.probable_reps_mutex }; - node.rep_crawler.active.insert (nano::dev::genesis->hash ()); - node.rep_crawler.responses.emplace_back (loopback, vote); - } - node.rep_crawler.validate (); - ASSERT_EQ (0, node.rep_crawler.representative_count ()); -} + node.rep_crawler.force_response (loopback, vote); + ASSERT_ALWAYS_EQ (0.5s, node.rep_crawler.representative_count (), 0); } // Test that a node configured with `enable_pruning` and `max_pruning_age = 1s` will automatically diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 834cabbd9f..5220777ae4 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -93,7 +93,7 @@ std::unique_ptr nano::collect_container_info (re { std::size_t count; { - nano::lock_guard guard{ rep_crawler.active_mutex }; + nano::lock_guard guard{ rep_crawler.mutex }; count = rep_crawler.active.size (); } diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index a807ac68bc..db944111b3 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -16,7 +16,7 @@ nano::rep_crawler::rep_crawler (nano::node & node_a) : void nano::rep_crawler::remove (nano::block_hash const & hash_a) { - nano::lock_guard lock{ active_mutex }; + nano::lock_guard lock{ mutex }; active.erase (hash_a); } @@ -29,7 +29,7 @@ void nano::rep_crawler::validate_and_process () { decltype (responses) responses_l; { - nano::lock_guard lock{ active_mutex }; + nano::lock_guard lock{ mutex }; responses_l.swap (responses); } @@ -65,7 +65,7 @@ void nano::rep_crawler::validate_and_process () auto updated = false; std::shared_ptr prev_channel; - nano::unique_lock lock{ probable_reps_mutex }; + nano::unique_lock lock{ mutex }; auto existing (probable_reps.find (vote->account)); if (existing != probable_reps.end ()) @@ -166,7 +166,7 @@ void nano::rep_crawler::query (std::vector lock{ active_mutex }; + nano::lock_guard lock{ mutex }; // Don't send same block multiple times in tests if (node.network_params.network.is_dev_network ()) { @@ -222,7 +222,7 @@ void nano::rep_crawler::throttled_remove (nano::block_hash const & hash_a, uint6 bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const { - nano::lock_guard lock{ probable_reps_mutex }; + nano::lock_guard lock{ mutex }; auto existing = probable_reps.get ().find (channel_a); if (existing != probable_reps.get ().end ()) { @@ -235,7 +235,7 @@ bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const bool nano::rep_crawler::response (std::shared_ptr const & channel, std::shared_ptr const & vote, bool force) { bool error = true; - nano::lock_guard lock{ active_mutex }; + nano::lock_guard lock{ mutex }; for (auto const & hash : vote->hashes) { if (force || active.count (hash) != 0) @@ -250,7 +250,7 @@ bool nano::rep_crawler::response (std::shared_ptr cons nano::uint128_t nano::rep_crawler::total_weight () const { - nano::lock_guard lock{ probable_reps_mutex }; + nano::lock_guard lock{ mutex }; nano::uint128_t result (0); for (const auto & i : probable_reps.get ()) { @@ -264,7 +264,7 @@ nano::uint128_t nano::rep_crawler::total_weight () const void nano::rep_crawler::on_rep_request (std::shared_ptr const & channel_a) { - nano::lock_guard lock{ probable_reps_mutex }; + nano::lock_guard lock{ mutex }; if (channel_a->get_tcp_endpoint ().address () != boost::asio::ip::address_v6::any ()) { auto & channel_ref_index = probable_reps.get (); @@ -285,7 +285,7 @@ void nano::rep_crawler::cleanup_reps () std::vector> channels; { // Check known rep channels - nano::lock_guard lock{ probable_reps_mutex }; + nano::lock_guard lock{ mutex }; auto iterator (probable_reps.get ().begin ()); while (iterator != probable_reps.get ().end ()) { @@ -319,7 +319,7 @@ void nano::rep_crawler::cleanup_reps () } if (!equal) { - nano::lock_guard lock{ probable_reps_mutex }; + nano::lock_guard lock{ mutex }; probable_reps.get ().erase (*i); } } @@ -329,7 +329,9 @@ std::vector nano::rep_crawler::representatives (std::size_ { auto version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); std::multimap> ordered; - nano::lock_guard lock{ probable_reps_mutex }; + + nano::lock_guard lock{ mutex }; + for (const auto & i : probable_reps.get ()) { auto weight = node.ledger.weight (i.account); @@ -365,6 +367,33 @@ std::vector> nano::rep_crawler::repres /** Total number of representatives */ std::size_t nano::rep_crawler::representative_count () { - nano::lock_guard lock{ probable_reps_mutex }; + nano::lock_guard lock{ mutex }; return probable_reps.size (); } + +// Only for tests +void nano::rep_crawler::force_add_rep (const nano::account & account, const std::shared_ptr & channel) +{ + release_assert (node.network_params.network.is_dev_network ()); + nano::lock_guard lock{ mutex }; + probable_reps.emplace (nano::representative (account, channel)); +} + +// Only for tests +void nano::rep_crawler::force_response (const std::shared_ptr & channel, const std::shared_ptr & vote) +{ + release_assert (node.network_params.network.is_dev_network ()); + nano::lock_guard lock{ mutex }; + for (auto const & hash : vote->hashes) + { + responses.emplace_back (channel, vote); + } +} + +// Only for tests +void nano::rep_crawler::force_active (const nano::block_hash & hash) +{ + release_assert (node.network_params.network.is_dev_network ()); + nano::lock_guard lock{ mutex }; + active.insert (hash); +} \ No newline at end of file diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 7926e9a5a7..53d3cf4645 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -54,11 +54,6 @@ class rep_crawler final { friend std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); - friend class active_transactions_confirm_election_by_request_Test; - friend class active_transactions_confirm_frontier_Test; - friend class rep_crawler_local_Test; - friend class node_online_reps_rep_crawler_Test; - public: explicit rep_crawler (nano::node & node_a); @@ -148,9 +143,11 @@ class rep_crawler final /** We have solicted votes for these random blocks */ std::unordered_set active; - /** Protects the active-hash container */ - mutable nano::mutex active_mutex; - /** Protects the probable_reps container */ - mutable nano::mutex probable_reps_mutex; + mutable nano::mutex mutex; + +public: // Testing + void force_add_rep (nano::account const & account, std::shared_ptr const & channel); + void force_response (std::shared_ptr const & channel, std::shared_ptr const & vote); + void force_active (nano::block_hash const & hash); }; } From e820ed071ab9272738dd9fcfe9e49e7279d69f40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 15 Feb 2024 10:51:48 +0100 Subject: [PATCH 04/27] Move collect_container_info into source file --- nano/node/node.cpp | 14 -------------- nano/node/node.hpp | 2 -- nano/node/repcrawler.cpp | 14 ++++++++++++++ nano/node/repcrawler.hpp | 2 ++ 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 5220777ae4..cea8a5a654 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -89,20 +89,6 @@ void nano::node::keepalive (std::string const & address_a, uint16_t port_a) }); } -std::unique_ptr nano::collect_container_info (rep_crawler & rep_crawler, std::string const & name) -{ - std::size_t count; - { - nano::lock_guard guard{ rep_crawler.mutex }; - count = rep_crawler.active.size (); - } - - auto const sizeof_element = sizeof (decltype (rep_crawler.active)::value_type); - auto composite = std::make_unique (name); - composite->add_component (std::make_unique (container_info{ "active", count, sizeof_element })); - return composite; -} - nano::keypair nano::load_or_create_node_id (std::filesystem::path const & application_path) { auto node_private_key_path = application_path / "node_id_private.key"; diff --git a/nano/node/node.hpp b/nano/node/node.hpp index abd7b3651b..e3daaae82a 100644 --- a/nano/node/node.hpp +++ b/nano/node/node.hpp @@ -57,8 +57,6 @@ namespace scheduler class component; } -std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); - // Configs backlog_population::config backlog_population_config (node_config const &); outbound_bandwidth_limiter::config outbound_bandwidth_limiter_config (node_config const &); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index db944111b3..a1a654d7bf 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -396,4 +396,18 @@ void nano::rep_crawler::force_active (const nano::block_hash & hash) release_assert (node.network_params.network.is_dev_network ()); nano::lock_guard lock{ mutex }; active.insert (hash); +} + +std::unique_ptr nano::collect_container_info (rep_crawler & rep_crawler, std::string const & name) +{ + std::size_t count; + { + nano::lock_guard guard{ rep_crawler.mutex }; + count = rep_crawler.active.size (); + } + + auto const sizeof_element = sizeof (decltype (rep_crawler.active)::value_type); + auto composite = std::make_unique (name); + composite->add_component (std::make_unique (container_info{ "active", count, sizeof_element })); + return composite; } \ No newline at end of file diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 53d3cf4645..32af6c2d52 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -150,4 +150,6 @@ class rep_crawler final void force_response (std::shared_ptr const & channel, std::shared_ptr const & vote); void force_active (nano::block_hash const & hash); }; + +std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); } From d867e5fe888622697c47f31d3f1189aee8f0fb1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Fri, 16 Feb 2024 12:28:03 +0100 Subject: [PATCH 05/27] Move rep_crawler tests to a dedicated file --- nano/core_test/CMakeLists.txt | 1 + nano/core_test/node.cpp | 250 ------------------------------- nano/core_test/rep_crawler.cpp | 264 +++++++++++++++++++++++++++++++++ nano/node/repcrawler.hpp | 1 + 4 files changed, 266 insertions(+), 250 deletions(-) create mode 100644 nano/core_test/rep_crawler.cpp diff --git a/nano/core_test/CMakeLists.txt b/nano/core_test/CMakeLists.txt index 8f16a81f9e..ba92b42681 100644 --- a/nano/core_test/CMakeLists.txt +++ b/nano/core_test/CMakeLists.txt @@ -36,6 +36,7 @@ add_executable( optimistic_scheduler.cpp processing_queue.cpp processor_service.cpp + rep_crawler.cpp peer_container.cpp scheduler_buckets.cpp request_aggregator.cpp diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 439c3dca9f..55b6c6b2c9 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -1597,256 +1597,6 @@ TEST (node, unconfirmed_send) ASSERT_TIMELY_EQ (5s, node1.balance (nano::dev::genesis->account ()), nano::dev::constants.genesis_amount); } -// Test that nodes can track nodes that have rep weight for priority broadcasting -TEST (node, rep_list) -{ - nano::test::system system (2); - auto & node1 (*system.nodes[1]); - auto wallet0 (system.wallet (0)); - auto wallet1 (system.wallet (1)); - // Node0 has a rep - wallet0->insert_adhoc (nano::dev::genesis_key.prv); - nano::keypair key1; - // Broadcast a confirm so others should know this is a rep node - wallet0->send_action (nano::dev::genesis_key.pub, key1.pub, nano::Mxrb_ratio); - ASSERT_EQ (0, node1.rep_crawler.representatives (1).size ()); - system.deadline_set (10s); - auto done (false); - while (!done) - { - auto reps = node1.rep_crawler.representatives (1); - if (!reps.empty ()) - { - if (!node1.ledger.weight (reps[0].account).is_zero ()) - { - done = true; - } - } - ASSERT_NO_ERROR (system.poll ()); - } -} - -TEST (node, rep_weight) -{ - nano::test::system system; - auto add_node = [&system] { - auto node = std::make_shared (system.io_ctx, system.get_available_port (), nano::unique_path (), system.work); - node->start (); - system.nodes.push_back (node); - return node; - }; - auto & node = *add_node (); - auto & node1 = *add_node (); - auto & node2 = *add_node (); - auto & node3 = *add_node (); - nano::keypair keypair1; - nano::keypair keypair2; - nano::block_builder builder; - auto amount_pr (node.minimum_principal_weight () + 100); - auto amount_not_pr (node.minimum_principal_weight () - 100); - std::shared_ptr block1 = builder - .state () - .account (nano::dev::genesis_key.pub) - .previous (nano::dev::genesis->hash ()) - .representative (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - amount_not_pr) - .link (keypair1.pub) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - std::shared_ptr block2 = builder - .state () - .account (keypair1.pub) - .previous (0) - .representative (keypair1.pub) - .balance (amount_not_pr) - .link (block1->hash ()) - .sign (keypair1.prv, keypair1.pub) - .work (*system.work.generate (keypair1.pub)) - .build (); - std::shared_ptr block3 = builder - .state () - .account (nano::dev::genesis_key.pub) - .previous (block1->hash ()) - .representative (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - amount_not_pr - amount_pr) - .link (keypair2.pub) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (block1->hash ())) - .build (); - std::shared_ptr block4 = builder - .state () - .account (keypair2.pub) - .previous (0) - .representative (keypair2.pub) - .balance (amount_pr) - .link (block3->hash ()) - .sign (keypair2.prv, keypair2.pub) - .work (*system.work.generate (keypair2.pub)) - .build (); - { - auto transaction = node.store.tx_begin_write (); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block1)); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block2)); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block3)); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block4)); - } - ASSERT_TRUE (node.rep_crawler.representatives (1).empty ()); - std::shared_ptr channel1 = nano::test::establish_tcp (system, node, node1.network.endpoint ()); - ASSERT_NE (nullptr, channel1); - std::shared_ptr channel2 = nano::test::establish_tcp (system, node, node2.network.endpoint ()); - ASSERT_NE (nullptr, channel2); - std::shared_ptr channel3 = nano::test::establish_tcp (system, node, node3.network.endpoint ()); - ASSERT_NE (nullptr, channel3); - auto vote0 = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - auto vote1 = std::make_shared (keypair1.pub, keypair1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - auto vote2 = std::make_shared (keypair2.pub, keypair2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - node.rep_crawler.response (channel1, vote0); - node.rep_crawler.response (channel2, vote1); - node.rep_crawler.response (channel3, vote2); - ASSERT_TIMELY_EQ (5s, node.rep_crawler.representative_count (), 2); - // Make sure we get the rep with the most weight first - auto reps = node.rep_crawler.representatives (1); - ASSERT_EQ (1, reps.size ()); - ASSERT_EQ (node.balance (nano::dev::genesis_key.pub), node.ledger.weight (reps[0].account)); - ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); - ASSERT_EQ (*channel1, reps[0].channel_ref ()); - ASSERT_TRUE (node.rep_crawler.is_pr (*channel1)); - ASSERT_FALSE (node.rep_crawler.is_pr (*channel2)); - ASSERT_TRUE (node.rep_crawler.is_pr (*channel3)); -} - -// Test that rep_crawler removes unreachable reps from its search results. -// This test creates three principal representatives (rep1, rep2, genesis_rep) and -// one node for searching them (searching_node). -TEST (node, rep_remove) -{ - nano::test::system system; - auto & searching_node = *system.add_node (); // will be used to find principal representatives - nano::keypair keys_rep1; // Principal representative 1 - nano::keypair keys_rep2; // Principal representative 2 - nano::block_builder builder; - - // Send enough nanos to Rep1 to make it a principal representative - std::shared_ptr send_to_rep1 = builder - .state () - .account (nano::dev::genesis_key.pub) - .previous (nano::dev::genesis->hash ()) - .representative (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - searching_node.minimum_principal_weight () * 2) - .link (keys_rep1.pub) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (nano::dev::genesis->hash ())) - .build (); - - // Receive by Rep1 - std::shared_ptr receive_rep1 = builder - .state () - .account (keys_rep1.pub) - .previous (0) - .representative (keys_rep1.pub) - .balance (searching_node.minimum_principal_weight () * 2) - .link (send_to_rep1->hash ()) - .sign (keys_rep1.prv, keys_rep1.pub) - .work (*system.work.generate (keys_rep1.pub)) - .build (); - - // Send enough nanos to Rep2 to make it a principal representative - std::shared_ptr send_to_rep2 = builder - .state () - .account (nano::dev::genesis_key.pub) - .previous (send_to_rep1->hash ()) - .representative (nano::dev::genesis_key.pub) - .balance (nano::dev::constants.genesis_amount - searching_node.minimum_principal_weight () * 4) - .link (keys_rep2.pub) - .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) - .work (*system.work.generate (send_to_rep1->hash ())) - .build (); - - // Receive by Rep2 - std::shared_ptr receive_rep2 = builder - .state () - .account (keys_rep2.pub) - .previous (0) - .representative (keys_rep2.pub) - .balance (searching_node.minimum_principal_weight () * 2) - .link (send_to_rep2->hash ()) - .sign (keys_rep2.prv, keys_rep2.pub) - .work (*system.work.generate (keys_rep2.pub)) - .build (); - { - auto transaction = searching_node.store.tx_begin_write (); - ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, send_to_rep1)); - ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, receive_rep1)); - ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, send_to_rep2)); - ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, receive_rep2)); - } - - // Create channel for Rep1 - auto channel_rep1 (std::make_shared (searching_node)); - - // Ensure Rep1 is found by the rep_crawler after receiving a vote from it - auto vote_rep1 = std::make_shared (keys_rep1.pub, keys_rep1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - ASSERT_FALSE (searching_node.rep_crawler.response (channel_rep1, vote_rep1, true)); - ASSERT_TIMELY_EQ (5s, searching_node.rep_crawler.representative_count (), 1); - auto reps (searching_node.rep_crawler.representatives (1)); - ASSERT_EQ (1, reps.size ()); - ASSERT_EQ (searching_node.minimum_principal_weight () * 2, searching_node.ledger.weight (reps[0].account)); - ASSERT_EQ (keys_rep1.pub, reps[0].account); - ASSERT_EQ (*channel_rep1, reps[0].channel_ref ()); - - // When rep1 disconnects then rep1 should not be found anymore - channel_rep1->close (); - ASSERT_TIMELY_EQ (5s, searching_node.rep_crawler.representative_count (), 0); - - // Add working node for genesis representative - auto node_genesis_rep = system.add_node (nano::node_config (system.get_available_port ())); - system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv); - auto channel_genesis_rep (searching_node.network.find_node_id (node_genesis_rep->get_node_id ())); - ASSERT_NE (nullptr, channel_genesis_rep); - - // genesis_rep should be found as principal representative after receiving a vote from it - auto vote_genesis_rep = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - searching_node.rep_crawler.response (channel_genesis_rep, vote_genesis_rep, true); - ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 1); - - // Start a node for Rep2 and wait until it is connected - auto node_rep2 (std::make_shared (system.io_ctx, nano::unique_path (), nano::node_config (system.get_available_port ()), system.work)); - node_rep2->start (); - searching_node.network.tcp_channels.start_tcp (node_rep2->network.endpoint ()); - std::shared_ptr channel_rep2; - ASSERT_TIMELY (10s, (channel_rep2 = searching_node.network.tcp_channels.find_node_id (node_rep2->get_node_id ())) != nullptr); - - // Rep2 should be found as a principal representative after receiving a vote from it - auto vote_rep2 = std::make_shared (keys_rep2.pub, keys_rep2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - ASSERT_FALSE (searching_node.rep_crawler.response (channel_rep2, vote_rep2, true)); - ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 2); - - // When Rep2 is stopped, it should not be found as principal representative anymore - node_rep2->stop (); - ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 1); - - // Now only genesisRep should be found: - reps = searching_node.rep_crawler.representatives (1); - ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); - ASSERT_TIMELY_EQ (5s, searching_node.network.size (), 1); - auto list (searching_node.network.list (1)); - ASSERT_EQ (node_genesis_rep->network.endpoint (), list[0]->get_endpoint ()); -} - -TEST (node, rep_connection_close) -{ - nano::test::system system (2); - auto & node1 (*system.nodes[0]); - auto & node2 (*system.nodes[1]); - // Add working representative (node 2) - system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv); - ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 1); - node2.stop (); - // Remove representative with closed channel - ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 0); -} - // Test that nodes can disable representative voting TEST (node, no_voting) { diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp new file mode 100644 index 0000000000..745bfdb553 --- /dev/null +++ b/nano/core_test/rep_crawler.cpp @@ -0,0 +1,264 @@ +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +using namespace std::chrono_literals; + +// Test that nodes can track nodes that have rep weight for priority broadcasting +TEST (rep_crawler, rep_list) +{ + nano::test::system system (2); + auto & node1 (*system.nodes[1]); + auto wallet0 (system.wallet (0)); + auto wallet1 (system.wallet (1)); + // Node0 has a rep + wallet0->insert_adhoc (nano::dev::genesis_key.prv); + nano::keypair key1; + // Broadcast a confirm so others should know this is a rep node + wallet0->send_action (nano::dev::genesis_key.pub, key1.pub, nano::Mxrb_ratio); + ASSERT_EQ (0, node1.rep_crawler.representatives (1).size ()); + system.deadline_set (10s); + auto done (false); + while (!done) + { + auto reps = node1.rep_crawler.representatives (1); + if (!reps.empty ()) + { + if (!node1.ledger.weight (reps[0].account).is_zero ()) + { + done = true; + } + } + ASSERT_NO_ERROR (system.poll ()); + } +} + +TEST (rep_crawler, rep_weight) +{ + nano::test::system system; + auto add_node = [&system] { + auto node = std::make_shared (system.io_ctx, system.get_available_port (), nano::unique_path (), system.work); + node->start (); + system.nodes.push_back (node); + return node; + }; + auto & node = *add_node (); + auto & node1 = *add_node (); + auto & node2 = *add_node (); + auto & node3 = *add_node (); + nano::keypair keypair1; + nano::keypair keypair2; + nano::block_builder builder; + auto amount_pr (node.minimum_principal_weight () + 100); + auto amount_not_pr (node.minimum_principal_weight () - 100); + std::shared_ptr block1 = builder + .state () + .account (nano::dev::genesis_key.pub) + .previous (nano::dev::genesis->hash ()) + .representative (nano::dev::genesis_key.pub) + .balance (nano::dev::constants.genesis_amount - amount_not_pr) + .link (keypair1.pub) + .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) + .work (*system.work.generate (nano::dev::genesis->hash ())) + .build (); + std::shared_ptr block2 = builder + .state () + .account (keypair1.pub) + .previous (0) + .representative (keypair1.pub) + .balance (amount_not_pr) + .link (block1->hash ()) + .sign (keypair1.prv, keypair1.pub) + .work (*system.work.generate (keypair1.pub)) + .build (); + std::shared_ptr block3 = builder + .state () + .account (nano::dev::genesis_key.pub) + .previous (block1->hash ()) + .representative (nano::dev::genesis_key.pub) + .balance (nano::dev::constants.genesis_amount - amount_not_pr - amount_pr) + .link (keypair2.pub) + .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) + .work (*system.work.generate (block1->hash ())) + .build (); + std::shared_ptr block4 = builder + .state () + .account (keypair2.pub) + .previous (0) + .representative (keypair2.pub) + .balance (amount_pr) + .link (block3->hash ()) + .sign (keypair2.prv, keypair2.pub) + .work (*system.work.generate (keypair2.pub)) + .build (); + { + auto transaction = node.store.tx_begin_write (); + ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block1)); + ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block2)); + ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block3)); + ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block4)); + } + ASSERT_TRUE (node.rep_crawler.representatives (1).empty ()); + std::shared_ptr channel1 = nano::test::establish_tcp (system, node, node1.network.endpoint ()); + ASSERT_NE (nullptr, channel1); + std::shared_ptr channel2 = nano::test::establish_tcp (system, node, node2.network.endpoint ()); + ASSERT_NE (nullptr, channel2); + std::shared_ptr channel3 = nano::test::establish_tcp (system, node, node3.network.endpoint ()); + ASSERT_NE (nullptr, channel3); + auto vote0 = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + auto vote1 = std::make_shared (keypair1.pub, keypair1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + auto vote2 = std::make_shared (keypair2.pub, keypair2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + node.rep_crawler.response (channel1, vote0); + node.rep_crawler.response (channel2, vote1); + node.rep_crawler.response (channel3, vote2); + ASSERT_TIMELY_EQ (5s, node.rep_crawler.representative_count (), 2); + // Make sure we get the rep with the most weight first + auto reps = node.rep_crawler.representatives (1); + ASSERT_EQ (1, reps.size ()); + ASSERT_EQ (node.balance (nano::dev::genesis_key.pub), node.ledger.weight (reps[0].account)); + ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); + ASSERT_EQ (*channel1, reps[0].channel_ref ()); + ASSERT_TRUE (node.rep_crawler.is_pr (*channel1)); + ASSERT_FALSE (node.rep_crawler.is_pr (*channel2)); + ASSERT_TRUE (node.rep_crawler.is_pr (*channel3)); +} + +// Test that rep_crawler removes unreachable reps from its search results. +// This test creates three principal representatives (rep1, rep2, genesis_rep) and +// one node for searching them (searching_node). +TEST (rep_crawler, rep_remove) +{ + nano::test::system system; + auto & searching_node = *system.add_node (); // will be used to find principal representatives + nano::keypair keys_rep1; // Principal representative 1 + nano::keypair keys_rep2; // Principal representative 2 + nano::block_builder builder; + + // Send enough nanos to Rep1 to make it a principal representative + std::shared_ptr send_to_rep1 = builder + .state () + .account (nano::dev::genesis_key.pub) + .previous (nano::dev::genesis->hash ()) + .representative (nano::dev::genesis_key.pub) + .balance (nano::dev::constants.genesis_amount - searching_node.minimum_principal_weight () * 2) + .link (keys_rep1.pub) + .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) + .work (*system.work.generate (nano::dev::genesis->hash ())) + .build (); + + // Receive by Rep1 + std::shared_ptr receive_rep1 = builder + .state () + .account (keys_rep1.pub) + .previous (0) + .representative (keys_rep1.pub) + .balance (searching_node.minimum_principal_weight () * 2) + .link (send_to_rep1->hash ()) + .sign (keys_rep1.prv, keys_rep1.pub) + .work (*system.work.generate (keys_rep1.pub)) + .build (); + + // Send enough nanos to Rep2 to make it a principal representative + std::shared_ptr send_to_rep2 = builder + .state () + .account (nano::dev::genesis_key.pub) + .previous (send_to_rep1->hash ()) + .representative (nano::dev::genesis_key.pub) + .balance (nano::dev::constants.genesis_amount - searching_node.minimum_principal_weight () * 4) + .link (keys_rep2.pub) + .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) + .work (*system.work.generate (send_to_rep1->hash ())) + .build (); + + // Receive by Rep2 + std::shared_ptr receive_rep2 = builder + .state () + .account (keys_rep2.pub) + .previous (0) + .representative (keys_rep2.pub) + .balance (searching_node.minimum_principal_weight () * 2) + .link (send_to_rep2->hash ()) + .sign (keys_rep2.prv, keys_rep2.pub) + .work (*system.work.generate (keys_rep2.pub)) + .build (); + { + auto transaction = searching_node.store.tx_begin_write (); + ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, send_to_rep1)); + ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, receive_rep1)); + ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, send_to_rep2)); + ASSERT_EQ (nano::block_status::progress, searching_node.ledger.process (transaction, receive_rep2)); + } + + // Create channel for Rep1 + auto channel_rep1 (std::make_shared (searching_node)); + + // Ensure Rep1 is found by the rep_crawler after receiving a vote from it + auto vote_rep1 = std::make_shared (keys_rep1.pub, keys_rep1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + ASSERT_FALSE (searching_node.rep_crawler.response (channel_rep1, vote_rep1, true)); + ASSERT_TIMELY_EQ (5s, searching_node.rep_crawler.representative_count (), 1); + auto reps (searching_node.rep_crawler.representatives (1)); + ASSERT_EQ (1, reps.size ()); + ASSERT_EQ (searching_node.minimum_principal_weight () * 2, searching_node.ledger.weight (reps[0].account)); + ASSERT_EQ (keys_rep1.pub, reps[0].account); + ASSERT_EQ (*channel_rep1, reps[0].channel_ref ()); + + // When rep1 disconnects then rep1 should not be found anymore + channel_rep1->close (); + ASSERT_TIMELY_EQ (5s, searching_node.rep_crawler.representative_count (), 0); + + // Add working node for genesis representative + auto node_genesis_rep = system.add_node (nano::node_config (system.get_available_port ())); + system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv); + auto channel_genesis_rep (searching_node.network.find_node_id (node_genesis_rep->get_node_id ())); + ASSERT_NE (nullptr, channel_genesis_rep); + + // genesis_rep should be found as principal representative after receiving a vote from it + auto vote_genesis_rep = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + searching_node.rep_crawler.response (channel_genesis_rep, vote_genesis_rep, true); + ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 1); + + // Start a node for Rep2 and wait until it is connected + auto node_rep2 (std::make_shared (system.io_ctx, nano::unique_path (), nano::node_config (system.get_available_port ()), system.work)); + node_rep2->start (); + searching_node.network.tcp_channels.start_tcp (node_rep2->network.endpoint ()); + std::shared_ptr channel_rep2; + ASSERT_TIMELY (10s, (channel_rep2 = searching_node.network.tcp_channels.find_node_id (node_rep2->get_node_id ())) != nullptr); + + // Rep2 should be found as a principal representative after receiving a vote from it + auto vote_rep2 = std::make_shared (keys_rep2.pub, keys_rep2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + ASSERT_FALSE (searching_node.rep_crawler.response (channel_rep2, vote_rep2, true)); + ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 2); + + // When Rep2 is stopped, it should not be found as principal representative anymore + node_rep2->stop (); + ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 1); + + // Now only genesisRep should be found: + reps = searching_node.rep_crawler.representatives (1); + ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); + ASSERT_TIMELY_EQ (5s, searching_node.network.size (), 1); + auto list (searching_node.network.list (1)); + ASSERT_EQ (node_genesis_rep->network.endpoint (), list[0]->get_endpoint ()); +} + +TEST (rep_crawler, rep_connection_close) +{ + nano::test::system system (2); + auto & node1 (*system.nodes[0]); + auto & node2 (*system.nodes[1]); + // Add working representative (node 2) + system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv); + ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 1); + node2.stop (); + // Remove representative with closed channel + ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 0); +} \ No newline at end of file diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 32af6c2d52..1fabd2fe18 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include From 3cdee96c132cdee477bfbc2c295b2110cf043b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 15 Feb 2024 11:55:17 +0100 Subject: [PATCH 06/27] Overhaul `rep_crawler` --- nano/core_test/node.cpp | 2 +- nano/lib/config.hpp | 5 + nano/lib/stats_enums.hpp | 4 + nano/lib/thread_roles.cpp | 3 + nano/lib/thread_roles.hpp | 1 + nano/node/node.cpp | 1 + nano/node/repcrawler.cpp | 290 +++++++++++++++++++++----------------- nano/node/repcrawler.hpp | 48 ++++--- 8 files changed, 205 insertions(+), 149 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 55b6c6b2c9..6a9b67eb8b 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -1774,7 +1774,7 @@ TEST (node, online_reps_rep_crawler) node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); ASSERT_EQ (0, node1.online_reps.online ()); // After inserting to rep crawler - node1.rep_crawler.force_active (nano::dev::genesis->hash ()); + node1.rep_crawler.force_active_query (nano::dev::genesis->hash ()); node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); ASSERT_EQ (nano::dev::constants.genesis_amount, node1.online_reps.online ()); } diff --git a/nano/lib/config.hpp b/nano/lib/config.hpp index a68b02c1b0..6b59f1bc6e 100644 --- a/nano/lib/config.hpp +++ b/nano/lib/config.hpp @@ -251,6 +251,8 @@ class network_constants telemetry_request_interval = 500ms; telemetry_broadcast_interval = 500ms; optimistic_activation_delay = 2s; + rep_crawler_normal_interval = 500ms; + rep_crawler_warmup_interval = 500ms; } } @@ -307,6 +309,9 @@ class network_constants /** How much to delay activation of optimistic elections to avoid interfering with election scheduler */ std::chrono::seconds optimistic_activation_delay{ 30 }; + std::chrono::milliseconds rep_crawler_normal_interval{ 1000 * 7 }; + std::chrono::milliseconds rep_crawler_warmup_interval{ 1000 * 3 }; + /** Returns the network this object contains values for */ nano::networks network () const { diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index 63c90bddc5..496d9f63cd 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -49,6 +49,7 @@ enum class type : uint8_t election_scheduler, optimistic_scheduler, handshake, + rep_crawler, bootstrap_ascending, bootstrap_ascending_accounts, @@ -328,6 +329,9 @@ enum class detail : uint8_t deprioritize, deprioritize_failed, + // rep_crawler + channel_dead, + _last // Must be the last enum }; diff --git a/nano/lib/thread_roles.cpp b/nano/lib/thread_roles.cpp index 0824d23733..e3f19d6ec4 100644 --- a/nano/lib/thread_roles.cpp +++ b/nano/lib/thread_roles.cpp @@ -100,6 +100,9 @@ std::string nano::thread_role::get_string (nano::thread_role::name role) case nano::thread_role::name::scheduler_priority: thread_role_name_string = "Sched Priority"; break; + case nano::thread_role::name::rep_crawler: + thread_role_name_string = "Rep Crawler"; + break; default: debug_assert (false && "nano::thread_role::get_string unhandled thread role"); } diff --git a/nano/lib/thread_roles.hpp b/nano/lib/thread_roles.hpp index 311ae58d1b..8bc95c8329 100644 --- a/nano/lib/thread_roles.hpp +++ b/nano/lib/thread_roles.hpp @@ -42,6 +42,7 @@ enum class name scheduler_manual, scheduler_optimistic, scheduler_priority, + rep_crawler, }; /* diff --git a/nano/node/node.cpp b/nano/node/node.cpp index cea8a5a654..4bb58d41a1 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -664,6 +664,7 @@ void nano::node::stop () { ascendboot.stop (); } + rep_crawler.stop (); unchecked.stop (); block_processor.stop (); aggregator.stop (); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index a1a654d7bf..413d081a5a 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -4,73 +4,99 @@ #include nano::rep_crawler::rep_crawler (nano::node & node_a) : - node (node_a) + node{ node_a }, + stats{ node_a.stats }, + network_constants{ node_a.network_params.network }, + active{ node_a.active } { if (!node.flags.disable_rep_crawler) { - node.observers.endpoint.add ([this] (std::shared_ptr const & channel_a) { - this->query (channel_a); + node.observers.endpoint.add ([this] (std::shared_ptr const & channel) { + query (channel); }); } } -void nano::rep_crawler::remove (nano::block_hash const & hash_a) +nano::rep_crawler::~rep_crawler () { - nano::lock_guard lock{ mutex }; - active.erase (hash_a); + // Thread must be stopped before destruction + debug_assert (!thread.joinable ()); } void nano::rep_crawler::start () { - ongoing_crawl (); + debug_assert (!thread.joinable ()); + + thread = std::thread{ [this] () { + nano::thread_role::set (nano::thread_role::name::rep_crawler); + run (); + } }; } -void nano::rep_crawler::validate_and_process () +void nano::rep_crawler::stop () { - decltype (responses) responses_l; { nano::lock_guard lock{ mutex }; - responses_l.swap (responses); + stopped = true; } + condition.notify_all (); + if (thread.joinable ()) + { + thread.join (); + } +} + +void nano::rep_crawler::remove (nano::block_hash const & hash) +{ + nano::lock_guard lock{ mutex }; + queries.erase (hash); +} + +void nano::rep_crawler::validate_and_process (nano::unique_lock & lock) +{ + debug_assert (!mutex.try_lock ()); + debug_assert (lock.owns_lock ()); + + decltype (responses) responses_l; + responses_l.swap (responses); + + lock.unlock (); // normally the rep_crawler only tracks principal reps but it can be made to track // reps with less weight by setting rep_crawler_weight_minimum to a low value - auto minimum = std::min (node.minimum_principal_weight (), node.config.rep_crawler_weight_minimum.number ()); + auto const minimum = std::min (node.minimum_principal_weight (), node.config.rep_crawler_weight_minimum.number ()); - for (auto const & i : responses_l) + for (auto const & response : responses_l) { - auto & vote = i.second; - auto & channel = i.first; + auto & vote = response.second; + auto & channel = response.first; debug_assert (channel != nullptr); if (channel->get_type () == nano::transport::transport_type::loopback) { node.logger.debug (nano::log::type::repcrawler, "Ignoring vote from loopback channel: {}", channel->to_string ()); - continue; } - nano::uint128_t rep_weight = node.ledger.weight (vote->account); + nano::uint128_t const rep_weight = node.ledger.weight (vote->account); if (rep_weight < minimum) { node.logger.debug (nano::log::type::repcrawler, "Ignoring vote from account {} with too little voting weight: {}", vote->account.to_account (), nano::util::to_str (rep_weight)); - continue; } // temporary data used for logging after dropping the lock - auto inserted = false; - auto updated = false; + bool inserted = false; + bool updated = false; std::shared_ptr prev_channel; - nano::unique_lock lock{ mutex }; + lock.lock (); - auto existing (probable_reps.find (vote->account)); - if (existing != probable_reps.end ()) + if (auto existing = reps.find (vote->account); existing != reps.end ()) { - probable_reps.modify (existing, [rep_weight, &updated, &vote, &channel, &prev_channel] (nano::representative & info) { + reps.modify (existing, [rep_weight, &updated, &vote, &channel, &prev_channel] (nano::representative & info) { info.last_response = std::chrono::steady_clock::now (); // Update if representative channel was changed @@ -85,7 +111,7 @@ void nano::rep_crawler::validate_and_process () } else { - probable_reps.emplace (nano::representative (vote->account, channel)); + reps.emplace (nano::representative (vote->account, channel)); inserted = true; } @@ -102,39 +128,68 @@ void nano::rep_crawler::validate_and_process () } } -void nano::rep_crawler::ongoing_crawl () +void nano::rep_crawler::run () { - auto now (std::chrono::steady_clock::now ()); - auto total_weight_l (total_weight ()); - cleanup_reps (); - validate_and_process (); - query (get_crawl_targets (total_weight_l)); - auto sufficient_weight (total_weight_l > node.online_reps.delta ()); - // If online weight drops below minimum, reach out to preconfigured peers - if (!sufficient_weight) + nano::unique_lock lock{ mutex }; + while (!stopped) { - node.keepalive_preconfigured (node.config.preconfigured_peers); + lock.unlock (); + + auto const current_total_weight = total_weight (); + bool const sufficient_weight = current_total_weight > node.online_reps.delta (); + + // If online weight drops below minimum, reach out to preconfigured peers + if (!sufficient_weight) + { + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::keepalive); + node.keepalive_preconfigured (node.config.preconfigured_peers); + } + + lock.lock (); + + condition.wait_for (lock, sufficient_weight ? network_constants.rep_crawler_normal_interval : network_constants.rep_crawler_warmup_interval); + if (stopped) + { + return; + } + + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::loop); + + cleanup (); + + validate_and_process (lock); + debug_assert (!lock.owns_lock ()); + + auto targets = get_crawl_targets (current_total_weight); + query (targets); + + lock.lock (); } - // Reduce crawl frequency when there's enough total peer weight - unsigned next_run_ms = node.network_params.network.is_dev_network () ? 100 : sufficient_weight ? 7000 - : 3000; - std::weak_ptr node_w (node.shared ()); - node.workers.add_timed_task (now + std::chrono::milliseconds (next_run_ms), [node_w, this] () { - if (auto node_l = node_w.lock ()) +} + +void nano::rep_crawler::cleanup () +{ + debug_assert (!mutex.try_lock ()); + + erase_if (reps, [this] (representative const & rep) { + if (!rep.channel->alive ()) { - this->ongoing_crawl (); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::channel_dead); + return true; // Erase } + return false; }); } -std::vector> nano::rep_crawler::get_crawl_targets (nano::uint128_t total_weight_a) const +std::vector> nano::rep_crawler::get_crawl_targets (nano::uint128_t current_total_weight) const { + // TODO: Make these values configurable constexpr std::size_t conservative_count = 10; constexpr std::size_t aggressive_count = 40; // Crawl more aggressively if we lack sufficient total peer weight. - bool sufficient_weight (total_weight_a > node.online_reps.delta ()); - uint16_t required_peer_count = sufficient_weight ? conservative_count : aggressive_count; + bool sufficient_weight = current_total_weight > node.online_reps.delta (); + auto required_peer_count = sufficient_weight ? conservative_count : aggressive_count; // Add random peers. We do this even if we have enough weight, in order to pick up reps // that didn't respond when first observed. If the current total weight isn't sufficient, this @@ -143,51 +198,79 @@ std::vector> nano::rep_crawler::get_cr required_peer_count += required_peer_count / 2; // The rest of the endpoints are picked randomly - auto random_peers (node.network.random_set (required_peer_count, 0, true)); // Include channels with ephemeral remote ports - std::vector> result; - result.insert (result.end (), random_peers.begin (), random_peers.end ()); - return result; + auto random_peers = node.network.random_set (required_peer_count, 0, true); // Include channels with ephemeral remote ports + return { random_peers.begin (), random_peers.end () }; } -void nano::rep_crawler::query (std::vector> const & channels_a) +std::optional> nano::rep_crawler::prepare_query_target () { - auto transaction (node.store.tx_begin_read ()); + constexpr int max_attempts = 4; + + auto transaction = node.store.tx_begin_read (); + std::optional> hash_root; - for (auto i = 0; i < 4 && !hash_root; ++i) + + // Randomly select a block from ledger to request votes for + for (auto i = 0; i < max_attempts && !hash_root; ++i) { hash_root = node.ledger.hash_root_random (transaction); - if (node.active.recently_confirmed.exists (hash_root->first)) + + // Rebroadcasted votes for recently confirmed blocks might confuse the rep crawler + if (active.recently_confirmed.exists (hash_root->first)) { hash_root = std::nullopt; } } + if (!hash_root) { - return; + return std::nullopt; } + + // Don't send same block multiple times in tests + if (node.network_params.network.is_dev_network ()) { nano::lock_guard lock{ mutex }; - // Don't send same block multiple times in tests - if (node.network_params.network.is_dev_network ()) + + for (auto i = 0; queries.count (hash_root->first) != 0 && i < 4; ++i) { - for (auto i (0); active.count (hash_root->first) != 0 && i < 4; ++i) - { - hash_root = node.ledger.hash_root_random (transaction); - } + hash_root = node.ledger.hash_root_random (transaction); } - active.insert (hash_root->first); // TODO: Is this really necessary? } - for (const auto & channel : channels_a) + + if (!hash_root) + { + return std::nullopt; + } + + { + nano::lock_guard lock{ mutex }; + queries.insert (hash_root->first); + } + + return hash_root; +} + +void nano::rep_crawler::query (std::vector> const & target_channels) +{ + auto maybe_hash_root = prepare_query_target (); + if (!maybe_hash_root) + { + return; + } + auto hash_root = *maybe_hash_root; + + for (const auto & channel : target_channels) { debug_assert (channel != nullptr); on_rep_request (channel); - node.network.send_confirm_req (channel, *hash_root); + node.network.send_confirm_req (channel, hash_root); } // TODO: Use a thread+timeout instead of a timer // A representative must respond with a vote within the deadline std::weak_ptr node_w (node.shared ()); - node.workers.add_timed_task (std::chrono::steady_clock::now () + std::chrono::seconds (5), [node_w, hash = hash_root->first] () { + node.workers.add_timed_task (std::chrono::steady_clock::now () + std::chrono::seconds (5), [node_w, hash = hash_root.first] () { if (auto node_l = node_w.lock ()) { auto target_finished_processed (node_l->vote_processor.total_processed + node_l->vote_processor.size ()); @@ -196,9 +279,9 @@ void nano::rep_crawler::query (std::vector const & channel_a) +void nano::rep_crawler::query (std::shared_ptr const & target_channel) { - query (std::vector{ channel_a }); + query (std::vector{ target_channel }); } // TODO: Use a thread+timeout instead of a timer @@ -223,8 +306,8 @@ void nano::rep_crawler::throttled_remove (nano::block_hash const & hash_a, uint6 bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const { nano::lock_guard lock{ mutex }; - auto existing = probable_reps.get ().find (channel_a); - if (existing != probable_reps.get ().end ()) + auto existing = reps.get ().find (channel_a); + if (existing != reps.get ().end ()) { return node.ledger.weight (existing->account) > node.minimum_principal_weight (); } @@ -238,7 +321,7 @@ bool nano::rep_crawler::response (std::shared_ptr cons nano::lock_guard lock{ mutex }; for (auto const & hash : vote->hashes) { - if (force || active.count (hash) != 0) + if (force || queries.count (hash) != 0) { responses.emplace_back (channel, vote); error = false; @@ -251,12 +334,12 @@ bool nano::rep_crawler::response (std::shared_ptr cons nano::uint128_t nano::rep_crawler::total_weight () const { nano::lock_guard lock{ mutex }; - nano::uint128_t result (0); - for (const auto & i : probable_reps.get ()) + nano::uint128_t result = 0; + for (const auto & rep : reps) { - if (i.channel->alive ()) + if (rep.channel->alive ()) { - result += node.ledger.weight (i.account); + result += node.ledger.weight (rep.account); } } return result; @@ -267,7 +350,7 @@ void nano::rep_crawler::on_rep_request (std::shared_ptr lock{ mutex }; if (channel_a->get_tcp_endpoint ().address () != boost::asio::ip::address_v6::any ()) { - auto & channel_ref_index = probable_reps.get (); + auto & channel_ref_index = reps.get (); // Find and update the timestamp on all reps available on the endpoint (a single host may have multiple reps) auto itr_pair = channel_ref_index.equal_range (*channel_a); @@ -280,51 +363,6 @@ void nano::rep_crawler::on_rep_request (std::shared_ptr> channels; - { - // Check known rep channels - nano::lock_guard lock{ mutex }; - auto iterator (probable_reps.get ().begin ()); - while (iterator != probable_reps.get ().end ()) - { - if (iterator->channel->alive ()) - { - channels.push_back (iterator->channel); - ++iterator; - } - else - { - // Remove reps with closed channels - iterator = probable_reps.get ().erase (iterator); - } - } - } - // Remove reps with inactive channels - for (auto const & i : channels) - { - bool equal (false); - if (i->get_type () == nano::transport::transport_type::tcp) - { - auto find_channel (node.network.tcp_channels.find_channel (i->get_tcp_endpoint ())); - if (find_channel != nullptr && *find_channel == *static_cast (i.get ())) // TODO: WTF - { - equal = true; - } - } - else if (i->get_type () == nano::transport::transport_type::fake) - { - equal = true; - } - if (!equal) - { - nano::lock_guard lock{ mutex }; - probable_reps.get ().erase (*i); - } - } -} - std::vector nano::rep_crawler::representatives (std::size_t count_a, nano::uint128_t const weight_a, boost::optional const & opt_version_min_a) { auto version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); @@ -332,7 +370,7 @@ std::vector nano::rep_crawler::representatives (std::size_ nano::lock_guard lock{ mutex }; - for (const auto & i : probable_reps.get ()) + for (const auto & i : reps.get ()) { auto weight = node.ledger.weight (i.account); if (weight > weight_a && i.channel->get_network_version () >= version_min) @@ -368,7 +406,7 @@ std::vector> nano::rep_crawler::repres std::size_t nano::rep_crawler::representative_count () { nano::lock_guard lock{ mutex }; - return probable_reps.size (); + return reps.size (); } // Only for tests @@ -376,7 +414,7 @@ void nano::rep_crawler::force_add_rep (const nano::account & account, const std: { release_assert (node.network_params.network.is_dev_network ()); nano::lock_guard lock{ mutex }; - probable_reps.emplace (nano::representative (account, channel)); + reps.emplace (nano::representative (account, channel)); } // Only for tests @@ -391,11 +429,11 @@ void nano::rep_crawler::force_response (const std::shared_ptr lock{ mutex }; - active.insert (hash); + queries.insert (hash); } std::unique_ptr nano::collect_container_info (rep_crawler & rep_crawler, std::string const & name) @@ -403,11 +441,11 @@ std::unique_ptr nano::collect_container_info (re std::size_t count; { nano::lock_guard guard{ rep_crawler.mutex }; - count = rep_crawler.active.size (); + count = rep_crawler.queries.size (); } - auto const sizeof_element = sizeof (decltype (rep_crawler.active)::value_type); + auto const sizeof_element = sizeof (decltype (rep_crawler.queries)::value_type); auto composite = std::make_unique (name); - composite->add_component (std::make_unique (container_info{ "active", count, sizeof_element })); + composite->add_component (std::make_unique (container_info{ "queries", count, sizeof_element })); return composite; } \ No newline at end of file diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 1fabd2fe18..3a1664e0ea 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -13,6 +14,7 @@ #include #include +#include #include namespace mi = boost::multi_index; @@ -20,6 +22,7 @@ namespace mi = boost::multi_index; namespace nano { class node; +class active_transactions; /** * A representative picked up during repcrawl. @@ -57,9 +60,10 @@ class rep_crawler final public: explicit rep_crawler (nano::node & node_a); + ~rep_crawler (); - /** Start crawling */ void start (); + void stop (); /** Remove block hash from list of active rep queries */ void remove (nano::block_hash const &); @@ -68,10 +72,10 @@ class rep_crawler final void throttled_remove (nano::block_hash const &, uint64_t target_finished_processed); /** Attempt to determine if the peer manages one or more representative accounts */ - void query (std::vector> const & channels_a); + void query (std::vector> const & target_channels); /** Attempt to determine if the peer manages one or more representative accounts */ - void query (std::shared_ptr const & channel_a); + void query (std::shared_ptr const & target_channel); /** Query if a peer manages a principle representative */ bool is_pr (nano::transport::channel const &) const; @@ -101,55 +105,55 @@ class rep_crawler final private: // Dependencies nano::node & node; + nano::stats & stats; + nano::network_constants & network_constants; + nano::active_transactions & active; private: - // Validate responses to see if they're reps - void validate_and_process (); - - /** Called continuously to crawl for representatives */ - void ongoing_crawl (); + void run (); + void cleanup (); + void validate_and_process (nano::unique_lock &); /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ - std::vector> get_crawl_targets (nano::uint128_t total_weight_a) const; + std::vector> get_crawl_targets (nano::uint128_t current_total_weight) const; /** When a rep request is made, this is called to update the last-request timestamp. */ void on_rep_request (std::shared_ptr const & channel_a); - /** Clean representatives with inactive channels */ - void cleanup_reps (); + std::optional> prepare_query_target (); private: // clang-format off class tag_account {}; class tag_channel_ref {}; - class tag_last_request {}; class tag_sequenced {}; - using ordered_probable_reps = boost::multi_index_container, mi::member>, + mi::hashed_unique, + mi::member>, mi::sequenced>, - mi::ordered_non_unique, - mi::member>, mi::hashed_non_unique, mi::const_mem_fun, &representative::channel_ref>>>>; // clang-format on - /** Probable representatives */ - ordered_probable_reps probable_reps; + ordered_reps reps; private: - std::deque, std::shared_ptr>> responses; + /** We have solicited votes for these random blocks */ + std::unordered_set queries; - /** We have solicted votes for these random blocks */ - std::unordered_set active; + std::deque, std::shared_ptr>> responses; + bool stopped{ false }; + nano::condition_variable condition; mutable nano::mutex mutex; + std::thread thread; public: // Testing void force_add_rep (nano::account const & account, std::shared_ptr const & channel); void force_response (std::shared_ptr const & channel, std::shared_ptr const & vote); - void force_active (nano::block_hash const & hash); + void force_active_query (nano::block_hash const & hash); }; std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); From 3d803181ee84c66f989efa9d304a44953c206024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Fri, 16 Feb 2024 12:40:47 +0100 Subject: [PATCH 07/27] Improve tests --- nano/core_test/rep_crawler.cpp | 69 ++++++++++++---------------------- nano/lib/logging_enums.hpp | 1 + nano/test_common/system.cpp | 2 + nano/test_common/testutil.cpp | 2 +- 4 files changed, 27 insertions(+), 47 deletions(-) diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index 745bfdb553..082e72bf45 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -16,50 +16,30 @@ using namespace std::chrono_literals; // Test that nodes can track nodes that have rep weight for priority broadcasting TEST (rep_crawler, rep_list) { - nano::test::system system (2); - auto & node1 (*system.nodes[1]); - auto wallet0 (system.wallet (0)); - auto wallet1 (system.wallet (1)); - // Node0 has a rep - wallet0->insert_adhoc (nano::dev::genesis_key.prv); - nano::keypair key1; - // Broadcast a confirm so others should know this is a rep node - wallet0->send_action (nano::dev::genesis_key.pub, key1.pub, nano::Mxrb_ratio); - ASSERT_EQ (0, node1.rep_crawler.representatives (1).size ()); - system.deadline_set (10s); - auto done (false); - while (!done) - { - auto reps = node1.rep_crawler.representatives (1); - if (!reps.empty ()) - { - if (!node1.ledger.weight (reps[0].account).is_zero ()) - { - done = true; - } - } - ASSERT_NO_ERROR (system.poll ()); - } + nano::test::system system; + auto & node1 = *system.add_node (); + auto & node2 = *system.add_node (); + ASSERT_EQ (0, node2.rep_crawler.representative_count ()); + // Node #1 has a rep + system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); + ASSERT_TIMELY_EQ (5s, node2.rep_crawler.representative_count (), 1); + auto reps = node2.rep_crawler.representatives (); + ASSERT_EQ (1, reps.size ()); + ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); } TEST (rep_crawler, rep_weight) { nano::test::system system; - auto add_node = [&system] { - auto node = std::make_shared (system.io_ctx, system.get_available_port (), nano::unique_path (), system.work); - node->start (); - system.nodes.push_back (node); - return node; - }; - auto & node = *add_node (); - auto & node1 = *add_node (); - auto & node2 = *add_node (); - auto & node3 = *add_node (); + auto & node = *system.add_node (); + auto & node1 = *system.add_node (); + auto & node2 = *system.add_node (); + auto & node3 = *system.add_node (); nano::keypair keypair1; nano::keypair keypair2; nano::block_builder builder; - auto amount_pr (node.minimum_principal_weight () + 100); - auto amount_not_pr (node.minimum_principal_weight () - 100); + auto const amount_pr = node.minimum_principal_weight () + 100; + auto const amount_not_pr = node.minimum_principal_weight () - 100; std::shared_ptr block1 = builder .state () .account (nano::dev::genesis_key.pub) @@ -100,13 +80,10 @@ TEST (rep_crawler, rep_weight) .sign (keypair2.prv, keypair2.pub) .work (*system.work.generate (keypair2.pub)) .build (); - { - auto transaction = node.store.tx_begin_write (); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block1)); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block2)); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block3)); - ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, block4)); - } + ASSERT_TRUE (nano::test::process (node, { block1, block2, block3, block4 })); + ASSERT_TRUE (nano::test::process (node1, { block1, block2, block3, block4 })); + ASSERT_TRUE (nano::test::process (node2, { block1, block2, block3, block4 })); + ASSERT_TRUE (nano::test::process (node3, { block1, block2, block3, block4 })); ASSERT_TRUE (node.rep_crawler.representatives (1).empty ()); std::shared_ptr channel1 = nano::test::establish_tcp (system, node, node1.network.endpoint ()); ASSERT_NE (nullptr, channel1); @@ -252,9 +229,9 @@ TEST (rep_crawler, rep_remove) TEST (rep_crawler, rep_connection_close) { - nano::test::system system (2); - auto & node1 (*system.nodes[0]); - auto & node2 (*system.nodes[1]); + nano::test::system system; + auto & node1 = *system.add_node (); + auto & node2 = *system.add_node (); // Add working representative (node 2) system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv); ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 1); diff --git a/nano/lib/logging_enums.hpp b/nano/lib/logging_enums.hpp index d3df7ed774..33d40a3ce1 100644 --- a/nano/lib/logging_enums.hpp +++ b/nano/lib/logging_enums.hpp @@ -25,6 +25,7 @@ enum class type generic, test, + system, init, config, logging, diff --git a/nano/test_common/system.cpp b/nano/test_common/system.cpp index 37e1376253..bf03c431d5 100644 --- a/nano/test_common/system.cpp +++ b/nano/test_common/system.cpp @@ -122,6 +122,8 @@ std::shared_ptr nano::test::system::add_node (nano::node_config cons debug_assert (!ec); } + logger.debug (nano::log::type::system, "Node started: {}", node->get_node_id ().to_node_id ()); + return node; } diff --git a/nano/test_common/testutil.cpp b/nano/test_common/testutil.cpp index 9d6d5bb76e..bfe87b08d3 100644 --- a/nano/test_common/testutil.cpp +++ b/nano/test_common/testutil.cpp @@ -65,7 +65,7 @@ bool nano::test::process (nano::node & node, std::vector Date: Sat, 17 Feb 2024 19:16:12 +0100 Subject: [PATCH 08/27] Simplify `nano::representative` struct --- nano/core_test/confirmation_solicitor.cpp | 4 +- nano/core_test/rep_crawler.cpp | 4 +- nano/node/repcrawler.cpp | 30 ++++------- nano/node/repcrawler.hpp | 65 +++++++++++++---------- 4 files changed, 50 insertions(+), 53 deletions(-) diff --git a/nano/core_test/confirmation_solicitor.cpp b/nano/core_test/confirmation_solicitor.cpp index c47ae54e88..0e0dfdd05f 100644 --- a/nano/core_test/confirmation_solicitor.cpp +++ b/nano/core_test/confirmation_solicitor.cpp @@ -20,7 +20,7 @@ TEST (confirmation_solicitor, batches) auto & node2 = *system.add_node (node_flags); auto channel1 = nano::test::establish_tcp (system, node2, node1.network.endpoint ()); // Solicitor will only solicit from this representative - nano::representative representative (nano::dev::genesis_key.pub, channel1); + nano::representative representative{ nano::dev::genesis_key.pub, channel1 }; std::vector representatives{ representative }; nano::confirmation_solicitor solicitor (node2.network, node2.config); solicitor.prepare (representatives); @@ -70,7 +70,7 @@ TEST (confirmation_solicitor, different_hash) auto & node2 = *system.add_node (node_flags); auto channel1 = nano::test::establish_tcp (system, node2, node1.network.endpoint ()); // Solicitor will only solicit from this representative - nano::representative representative (nano::dev::genesis_key.pub, channel1); + nano::representative representative{ nano::dev::genesis_key.pub, channel1 }; std::vector representatives{ representative }; nano::confirmation_solicitor solicitor (node2.network, node2.config); solicitor.prepare (representatives); diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index 082e72bf45..d9ae331645 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -103,7 +103,7 @@ TEST (rep_crawler, rep_weight) ASSERT_EQ (1, reps.size ()); ASSERT_EQ (node.balance (nano::dev::genesis_key.pub), node.ledger.weight (reps[0].account)); ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); - ASSERT_EQ (*channel1, reps[0].channel_ref ()); + ASSERT_EQ (*channel1, *reps[0].channel); ASSERT_TRUE (node.rep_crawler.is_pr (*channel1)); ASSERT_FALSE (node.rep_crawler.is_pr (*channel2)); ASSERT_TRUE (node.rep_crawler.is_pr (*channel3)); @@ -186,7 +186,7 @@ TEST (rep_crawler, rep_remove) ASSERT_EQ (1, reps.size ()); ASSERT_EQ (searching_node.minimum_principal_weight () * 2, searching_node.ledger.weight (reps[0].account)); ASSERT_EQ (keys_rep1.pub, reps[0].account); - ASSERT_EQ (*channel_rep1, reps[0].channel_ref ()); + ASSERT_EQ (*channel_rep1, *reps[0].channel); // When rep1 disconnects then rep1 should not be found anymore channel_rep1->close (); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 413d081a5a..a0968fa958 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -96,7 +96,7 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l if (auto existing = reps.find (vote->account); existing != reps.end ()) { - reps.modify (existing, [rep_weight, &updated, &vote, &channel, &prev_channel] (nano::representative & info) { + reps.modify (existing, [rep_weight, &updated, &vote, &channel, &prev_channel] (representative_entry & info) { info.last_response = std::chrono::steady_clock::now (); // Update if representative channel was changed @@ -111,7 +111,7 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l } else { - reps.emplace (nano::representative (vote->account, channel)); + reps.emplace (representative_entry{ vote->account, channel }); inserted = true; } @@ -171,7 +171,7 @@ void nano::rep_crawler::cleanup () { debug_assert (!mutex.try_lock ()); - erase_if (reps, [this] (representative const & rep) { + erase_if (reps, [this] (representative_entry const & rep) { if (!rep.channel->alive ()) { stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::channel_dead); @@ -356,8 +356,8 @@ void nano::rep_crawler::on_rep_request (std::shared_ptr nano::rep_crawler::representatives (std::size_t count_a, nano::uint128_t const weight_a, boost::optional const & opt_version_min_a) { - auto version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); - std::multimap> ordered; + auto const version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); nano::lock_guard lock{ mutex }; + std::multimap> ordered; for (const auto & i : reps.get ()) { auto weight = node.ledger.weight (i.account); @@ -378,10 +378,11 @@ std::vector nano::rep_crawler::representatives (std::size_ ordered.insert ({ nano::amount{ weight }, i }); } } + std::vector result; for (auto i = ordered.begin (), n = ordered.end (); i != n && result.size () < count_a; ++i) { - result.push_back (i->second); + result.push_back ({ i->second.account, i->second.channel }); } return result; } @@ -391,17 +392,6 @@ std::vector nano::rep_crawler::principal_representatives ( return representatives (count_a, node.minimum_principal_weight (), opt_version_min_a); } -std::vector> nano::rep_crawler::representative_endpoints (std::size_t count_a) -{ - std::vector> result; - auto reps = representatives (count_a); - for (auto const & rep : reps) - { - result.push_back (rep.channel); - } - return result; -} - /** Total number of representatives */ std::size_t nano::rep_crawler::representative_count () { @@ -414,7 +404,7 @@ void nano::rep_crawler::force_add_rep (const nano::account & account, const std: { release_assert (node.network_params.network.is_dev_network ()); nano::lock_guard lock{ mutex }; - reps.emplace (nano::representative (account, channel)); + reps.emplace (representative_entry{ account, channel }); } // Only for tests diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 3a1664e0ea..e824025179 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -24,30 +24,10 @@ namespace nano class node; class active_transactions; -/** - * A representative picked up during repcrawl. - */ -class representative +struct representative { -public: - representative () = default; - representative (nano::account account_a, std::shared_ptr const & channel_a) : - account (account_a), channel (channel_a) - { - debug_assert (channel != nullptr); - } - std::reference_wrapper channel_ref () const - { - return *channel; - }; - bool operator== (nano::representative const & other_a) const - { - return account == other_a.account; - } - nano::account account{}; + nano::account account; std::shared_ptr channel; - std::chrono::steady_clock::time_point last_request{ std::chrono::steady_clock::time_point () }; - std::chrono::steady_clock::time_point last_response{ std::chrono::steady_clock::time_point () }; }; /** @@ -59,7 +39,7 @@ class rep_crawler final friend std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); public: - explicit rep_crawler (nano::node & node_a); + explicit rep_crawler (nano::node &); ~rep_crawler (); void start (); @@ -97,9 +77,6 @@ class rep_crawler final /** Request a list of the top \p count_a known principal representatives in descending order of weight, optionally with a minimum version \p opt_version_min_a */ std::vector principal_representatives (std::size_t count_a = std::numeric_limits::max (), boost::optional const & opt_version_min_a = boost::none); - /** Request a list of the top \p count_a known representative endpoints. */ - std::vector> representative_endpoints (std::size_t count_a); - /** Total number of representatives */ std::size_t representative_count (); @@ -123,18 +100,48 @@ class rep_crawler final std::optional> prepare_query_target (); private: + /** + * A representative picked up during repcrawl. + */ + struct representative_entry + { + representative_entry (nano::account account_a, std::shared_ptr const & channel_a) : + account{ account_a }, + channel{ channel_a } + { + debug_assert (channel != nullptr); + } + + nano::account const account; + std::shared_ptr channel; + + std::chrono::steady_clock::time_point last_request{}; + std::chrono::steady_clock::time_point last_response{}; + + nano::account get_account () const + { + return account; + } + + std::reference_wrapper channel_ref () const + { + return *channel; + }; + }; + // clang-format off class tag_account {}; class tag_channel_ref {}; class tag_sequenced {}; - using ordered_reps = boost::multi_index_container, - mi::member>, + mi::const_mem_fun>, mi::sequenced>, mi::hashed_non_unique, - mi::const_mem_fun, &representative::channel_ref>>>>; + mi::const_mem_fun, &representative_entry::channel_ref>> + >>; // clang-format on ordered_reps reps; From fb516f33ab0a6995083e3d444f36f45a3b4597ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 19 Feb 2024 21:59:10 +0100 Subject: [PATCH 09/27] Use pointer as channel identity --- nano/core_test/rep_crawler.cpp | 6 +++--- nano/node/network.cpp | 2 +- nano/node/repcrawler.cpp | 19 +++++++++---------- nano/node/repcrawler.hpp | 13 ++++--------- 4 files changed, 17 insertions(+), 23 deletions(-) diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index d9ae331645..4b175cb358 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -104,9 +104,9 @@ TEST (rep_crawler, rep_weight) ASSERT_EQ (node.balance (nano::dev::genesis_key.pub), node.ledger.weight (reps[0].account)); ASSERT_EQ (nano::dev::genesis_key.pub, reps[0].account); ASSERT_EQ (*channel1, *reps[0].channel); - ASSERT_TRUE (node.rep_crawler.is_pr (*channel1)); - ASSERT_FALSE (node.rep_crawler.is_pr (*channel2)); - ASSERT_TRUE (node.rep_crawler.is_pr (*channel3)); + ASSERT_TRUE (node.rep_crawler.is_pr (channel1)); + ASSERT_FALSE (node.rep_crawler.is_pr (channel2)); + ASSERT_TRUE (node.rep_crawler.is_pr (channel3)); } // Test that rep_crawler removes unreachable reps from its search results. diff --git a/nano/node/network.cpp b/nano/node/network.cpp index 4d47533be6..ca56cccc95 100644 --- a/nano/node/network.cpp +++ b/nano/node/network.cpp @@ -419,7 +419,7 @@ std::deque> nano::network::list_non_pr tcp_channels.list (result); nano::random_pool_shuffle (result.begin (), result.end ()); result.erase (std::remove_if (result.begin (), result.end (), [this] (std::shared_ptr const & channel) { - return this->node.rep_crawler.is_pr (*channel); + return node.rep_crawler.is_pr (channel); }), result.end ()); if (result.size () > count_a) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index a0968fa958..c6014100e7 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -303,11 +303,11 @@ void nano::rep_crawler::throttled_remove (nano::block_hash const & hash_a, uint6 } } -bool nano::rep_crawler::is_pr (nano::transport::channel const & channel_a) const +bool nano::rep_crawler::is_pr (std::shared_ptr const & channel) const { nano::lock_guard lock{ mutex }; - auto existing = reps.get ().find (channel_a); - if (existing != reps.get ().end ()) + auto existing = reps.get ().find (channel); + if (existing != reps.get ().end ()) { return node.ledger.weight (existing->account) > node.minimum_principal_weight (); } @@ -345,18 +345,17 @@ nano::uint128_t nano::rep_crawler::total_weight () const return result; } -void nano::rep_crawler::on_rep_request (std::shared_ptr const & channel_a) +void nano::rep_crawler::on_rep_request (std::shared_ptr const & channel) { nano::lock_guard lock{ mutex }; - if (channel_a->get_tcp_endpoint ().address () != boost::asio::ip::address_v6::any ()) + if (channel->get_tcp_endpoint ().address () != boost::asio::ip::address_v6::any ()) { - auto & channel_ref_index = reps.get (); - // Find and update the timestamp on all reps available on the endpoint (a single host may have multiple reps) - auto itr_pair = channel_ref_index.equal_range (*channel_a); - for (; itr_pair.first != itr_pair.second; itr_pair.first++) + auto & index = reps.get (); + auto [begin, end] = index.equal_range (channel); + for (auto it = begin; it != end; ++it) { - channel_ref_index.modify (itr_pair.first, [] (representative_entry & info) { + index.modify (it, [] (representative_entry & info) { info.last_request = std::chrono::steady_clock::now (); }); } diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index e824025179..666c618e34 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -58,7 +58,7 @@ class rep_crawler final void query (std::shared_ptr const & target_channel); /** Query if a peer manages a principle representative */ - bool is_pr (nano::transport::channel const &) const; + bool is_pr (std::shared_ptr const &) const; /** * Called when a non-replay vote on a block previously sent by query() is received. This indicates @@ -122,16 +122,11 @@ class rep_crawler final { return account; } - - std::reference_wrapper channel_ref () const - { - return *channel; - }; }; // clang-format off class tag_account {}; - class tag_channel_ref {}; + class tag_channel {}; class tag_sequenced {}; using ordered_reps = boost::multi_index_container, mi::const_mem_fun>, mi::sequenced>, - mi::hashed_non_unique, - mi::const_mem_fun, &representative_entry::channel_ref>> + mi::hashed_non_unique, + mi::member, &representative_entry::channel>> >>; // clang-format on From 8c8026ddfe0cb44f70fc445b4c6d089b3d437027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 19 Feb 2024 21:44:20 +0100 Subject: [PATCH 10/27] Rework `rep_crawler::process ()` --- nano/core_test/election.cpp | 2 +- nano/core_test/node.cpp | 4 ++-- nano/core_test/rep_crawler.cpp | 12 ++++++------ nano/node/node.cpp | 9 +++++---- nano/node/repcrawler.cpp | 15 ++++++--------- nano/node/repcrawler.hpp | 18 +++++++++--------- 6 files changed, 29 insertions(+), 31 deletions(-) diff --git a/nano/core_test/election.cpp b/nano/core_test/election.cpp index fc193cc659..ec37d91371 100644 --- a/nano/core_test/election.cpp +++ b/nano/core_test/election.cpp @@ -254,7 +254,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks) ASSERT_NE (channel, nullptr); auto vote2 = nano::test::make_final_vote (key1, { send1->hash () }); - ASSERT_FALSE (node1.rep_crawler.response (channel, vote2, true)); + node1.rep_crawler.force_process (vote2, channel); ASSERT_FALSE (election->confirmed ()); { diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 6a9b67eb8b..defbadb8bd 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -1126,7 +1126,7 @@ TEST (node, DISABLED_fork_stale) auto channel = nano::test::establish_tcp (system1, node2, node1.network.endpoint ()); auto vote = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector ()); - node2.rep_crawler.response (channel, vote); + ASSERT_TRUE (node2.rep_crawler.process (vote, channel)); nano::keypair key1; nano::keypair key2; nano::state_block_builder builder; @@ -3714,7 +3714,7 @@ TEST (rep_crawler, ignore_local) auto & node = *system.add_node (flags); auto loopback = std::make_shared (node, node); auto vote = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - node.rep_crawler.force_response (loopback, vote); + node.rep_crawler.force_process (vote, loopback); ASSERT_ALWAYS_EQ (0.5s, node.rep_crawler.representative_count (), 0); } diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index 4b175cb358..9b83ebc64c 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -94,9 +94,9 @@ TEST (rep_crawler, rep_weight) auto vote0 = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); auto vote1 = std::make_shared (keypair1.pub, keypair1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); auto vote2 = std::make_shared (keypair2.pub, keypair2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - node.rep_crawler.response (channel1, vote0); - node.rep_crawler.response (channel2, vote1); - node.rep_crawler.response (channel3, vote2); + ASSERT_TRUE (node.rep_crawler.process (vote0, channel1)); + ASSERT_TRUE (node.rep_crawler.process (vote1, channel2)); + ASSERT_TRUE (node.rep_crawler.process (vote2, channel3)); ASSERT_TIMELY_EQ (5s, node.rep_crawler.representative_count (), 2); // Make sure we get the rep with the most weight first auto reps = node.rep_crawler.representatives (1); @@ -180,7 +180,7 @@ TEST (rep_crawler, rep_remove) // Ensure Rep1 is found by the rep_crawler after receiving a vote from it auto vote_rep1 = std::make_shared (keys_rep1.pub, keys_rep1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - ASSERT_FALSE (searching_node.rep_crawler.response (channel_rep1, vote_rep1, true)); + searching_node.rep_crawler.force_process (vote_rep1, channel_rep1); ASSERT_TIMELY_EQ (5s, searching_node.rep_crawler.representative_count (), 1); auto reps (searching_node.rep_crawler.representatives (1)); ASSERT_EQ (1, reps.size ()); @@ -200,7 +200,7 @@ TEST (rep_crawler, rep_remove) // genesis_rep should be found as principal representative after receiving a vote from it auto vote_genesis_rep = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - searching_node.rep_crawler.response (channel_genesis_rep, vote_genesis_rep, true); + searching_node.rep_crawler.force_process (vote_genesis_rep, channel_genesis_rep); ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 1); // Start a node for Rep2 and wait until it is connected @@ -212,7 +212,7 @@ TEST (rep_crawler, rep_remove) // Rep2 should be found as a principal representative after receiving a vote from it auto vote_rep2 = std::make_shared (keys_rep2.pub, keys_rep2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - ASSERT_FALSE (searching_node.rep_crawler.response (channel_rep2, vote_rep2, true)); + searching_node.rep_crawler.force_process (vote_rep2, channel_rep2); ASSERT_TIMELY_EQ (10s, searching_node.rep_crawler.representative_count (), 2); // When Rep2 is stopped, it should not be found as principal representative anymore diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 4bb58d41a1..ca99161876 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -305,13 +305,14 @@ nano::node::node (boost::asio::io_context & io_ctx_a, std::filesystem::path cons observers.endpoint.add ([this] (std::shared_ptr const & channel_a) { this->network.send_keepalive_self (channel_a); }); - observers.vote.add ([this] (std::shared_ptr vote_a, std::shared_ptr const & channel_a, nano::vote_code code_a) { - debug_assert (code_a != nano::vote_code::invalid); - auto active_in_rep_crawler (!this->rep_crawler.response (channel_a, vote_a)); + + observers.vote.add ([this] (std::shared_ptr vote, std::shared_ptr const & channel, nano::vote_code code) { + debug_assert (code != nano::vote_code::invalid); + bool active_in_rep_crawler = rep_crawler.process (vote, channel); if (active_in_rep_crawler) { // Representative is defined as online if replying to live votes or rep_crawler queries - this->online_reps.observe (vote_a->account); + online_reps.observe (vote->account); } }); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index c6014100e7..f1bb0cfc8c 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -314,21 +314,18 @@ bool nano::rep_crawler::is_pr (std::shared_ptr const & return false; } -// TODO: Remove force parameter -bool nano::rep_crawler::response (std::shared_ptr const & channel, std::shared_ptr const & vote, bool force) +bool nano::rep_crawler::process (std::shared_ptr const & vote, std::shared_ptr const & channel) { - bool error = true; nano::lock_guard lock{ mutex }; - for (auto const & hash : vote->hashes) + for (auto const & hash : vote->hashes) // TODO: This most likely should be a single hash vote { - if (force || queries.count (hash) != 0) + if (queries.count (hash) != 0) { responses.emplace_back (channel, vote); - error = false; - break; + return true; // Processed } } - return error; + return false; } nano::uint128_t nano::rep_crawler::total_weight () const @@ -407,7 +404,7 @@ void nano::rep_crawler::force_add_rep (const nano::account & account, const std: } // Only for tests -void nano::rep_crawler::force_response (const std::shared_ptr & channel, const std::shared_ptr & vote) +void nano::rep_crawler::force_process (const std::shared_ptr & vote, const std::shared_ptr & channel) { release_assert (node.network_params.network.is_dev_network ()); nano::lock_guard lock{ mutex }; diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 666c618e34..c58449a409 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -45,6 +45,14 @@ class rep_crawler final void start (); void stop (); + /** + * Called when a non-replay vote on a block previously sent by query() is received. This indicates + * with high probability that the endpoint is a representative node. + * The force flag can be set to skip the active check in unit testing when we want to force a vote in the rep crawler. + * @return false if any vote passed the checks and was added to the response queue of the rep crawler + */ + bool process (std::shared_ptr const &, std::shared_ptr const &); + /** Remove block hash from list of active rep queries */ void remove (nano::block_hash const &); @@ -60,14 +68,6 @@ class rep_crawler final /** Query if a peer manages a principle representative */ bool is_pr (std::shared_ptr const &) const; - /** - * Called when a non-replay vote on a block previously sent by query() is received. This indicates - * with high probability that the endpoint is a representative node. - * The force flag can be set to skip the active check in unit testing when we want to force a vote in the rep crawler. - * @return false if any vote passed the checks and was added to the response queue of the rep crawler - */ - bool response (std::shared_ptr const &, std::shared_ptr const &, bool force = false); - /** Get total available weight from representatives */ nano::uint128_t total_weight () const; @@ -154,7 +154,7 @@ class rep_crawler final public: // Testing void force_add_rep (nano::account const & account, std::shared_ptr const & channel); - void force_response (std::shared_ptr const & channel, std::shared_ptr const & vote); + void force_process (std::shared_ptr const & vote, std::shared_ptr const & channel); void force_active_query (nano::block_hash const & hash); }; From c45d78f6adb2b6692021c8b2d5b317af6ef507c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 20 Feb 2024 11:58:06 +0100 Subject: [PATCH 11/27] Move remaining rep_crawler tests --- nano/core_test/node.cpp | 31 ------------------------------- nano/core_test/rep_crawler.cpp | 31 +++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index defbadb8bd..15bcd4a87c 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -3687,37 +3687,6 @@ TEST (node, deferred_dependent_elections) } } -// This test checks that if a block is in the recently_confirmed list then the repcrawler will not send a request for it. -// The behaviour of this test previously was the opposite, that the repcrawler eventually send out such a block and deleted the block -// from the recently confirmed list to try to make ammends for sending it, which is bad behaviour. -// In the long term, we should have a better way to check for reps and this test should become redundant -TEST (rep_crawler, recently_confirmed) -{ - nano::test::system system (1); - auto & node1 (*system.nodes[0]); - ASSERT_EQ (1, node1.ledger.cache.block_count); - auto const block = nano::dev::genesis; - node1.active.recently_confirmed.put (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_node_id (node2.get_node_id ()); - ASSERT_NE (nullptr, channel); - node1.rep_crawler.query (channel); // this query should be dropped due to the recently_confirmed entry - ASSERT_ALWAYS_EQ (0.5s, node1.rep_crawler.representative_count (), 0); -} - -// Votes from local channels should be ignored -TEST (rep_crawler, ignore_local) -{ - nano::test::system system; - nano::node_flags flags; - auto & node = *system.add_node (flags); - auto loopback = std::make_shared (node, node); - auto vote = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - node.rep_crawler.force_process (vote, loopback); - ASSERT_ALWAYS_EQ (0.5s, node.rep_crawler.representative_count (), 0); -} - // Test that a node configured with `enable_pruning` and `max_pruning_age = 1s` will automatically // prune old confirmed blocks without explicitly saying `node.ledger_pruning` in the unit test TEST (node, pruning_automatic) diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index 9b83ebc64c..a6f88e5796 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -238,4 +238,35 @@ TEST (rep_crawler, rep_connection_close) node2.stop (); // Remove representative with closed channel ASSERT_TIMELY_EQ (10s, node1.rep_crawler.representative_count (), 0); +} + +// This test checks that if a block is in the recently_confirmed list then the repcrawler will not send a request for it. +// The behaviour of this test previously was the opposite, that the repcrawler eventually send out such a block and deleted the block +// from the recently confirmed list to try to make ammends for sending it, which is bad behaviour. +// In the long term, we should have a better way to check for reps and this test should become redundant +TEST (rep_crawler, recently_confirmed) +{ + nano::test::system system (1); + auto & node1 (*system.nodes[0]); + ASSERT_EQ (1, node1.ledger.cache.block_count); + auto const block = nano::dev::genesis; + node1.active.recently_confirmed.put (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_node_id (node2.get_node_id ()); + ASSERT_NE (nullptr, channel); + node1.rep_crawler.query (channel); // this query should be dropped due to the recently_confirmed entry + ASSERT_ALWAYS_EQ (0.5s, node1.rep_crawler.representative_count (), 0); +} + +// Votes from local channels should be ignored +TEST (rep_crawler, ignore_local) +{ + nano::test::system system; + nano::node_flags flags; + auto & node = *system.add_node (flags); + auto loopback = std::make_shared (node, node); + auto vote = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); + node.rep_crawler.force_process (vote, loopback); + ASSERT_ALWAYS_EQ (0.5s, node.rep_crawler.representative_count (), 0); } \ No newline at end of file From 80c0b8b6d6f9d912b3b93b6b5165a6bf99a223f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Wed, 21 Feb 2024 14:57:29 +0100 Subject: [PATCH 12/27] Overhaul `rep_crawler` part 2 --- nano/core_test/node.cpp | 3 +- nano/lib/stats_enums.hpp | 7 + nano/node/network.cpp | 5 +- nano/node/network.hpp | 2 +- nano/node/node.cpp | 4 +- nano/node/nodeconfig.cpp | 13 +- nano/node/nodeconfig.hpp | 2 + nano/node/repcrawler.cpp | 272 ++++++++++++++++++++++++--------------- nano/node/repcrawler.hpp | 77 +++++++---- 9 files changed, 248 insertions(+), 137 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 15bcd4a87c..c38cc8f4cc 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -1774,8 +1774,7 @@ TEST (node, online_reps_rep_crawler) node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); ASSERT_EQ (0, node1.online_reps.online ()); // After inserting to rep crawler - node1.rep_crawler.force_active_query (nano::dev::genesis->hash ()); - node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); + node1.rep_crawler.force_process (vote, std::make_shared (node1)); ASSERT_EQ (nano::dev::constants.genesis_amount, node1.online_reps.online ()); } diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index 496d9f63cd..fe15550b2f 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -331,6 +331,13 @@ enum class detail : uint8_t // rep_crawler channel_dead, + query_target_failed, + query_channel_busy, + query_sent, + rep_timeout, + query_timeout, + crawl_aggressive, + crawl_normal, _last // Must be the last enum }; diff --git a/nano/node/network.cpp b/nano/node/network.cpp index ca56cccc95..5d8613ecf5 100644 --- a/nano/node/network.cpp +++ b/nano/node/network.cpp @@ -219,10 +219,11 @@ void nano::network::flood_block_many (std::deque> b } } -void nano::network::send_confirm_req (std::shared_ptr const & channel_a, std::pair const & hash_root_a) +void nano::network::send_confirm_req (std::shared_ptr const & channel_a, std::pair const & hash_root_a) { + auto & [hash, root] = hash_root_a; // Confirmation request with hash + root - nano::confirm_req req (node.network_params.network, hash_root_a.first, hash_root_a.second); + nano::confirm_req req (node.network_params.network, hash, root); channel_a->send (req); } diff --git a/nano/node/network.hpp b/nano/node/network.hpp index 6ad08806c7..cd5b90e047 100644 --- a/nano/node/network.hpp +++ b/nano/node/network.hpp @@ -95,7 +95,7 @@ class network final void send_keepalive (std::shared_ptr const &); void send_keepalive_self (std::shared_ptr const &); void send_node_id_handshake (std::shared_ptr const &, std::optional const & cookie, std::optional const & respond_to); - void send_confirm_req (std::shared_ptr const & channel_a, std::pair const & hash_root_a); + void send_confirm_req (std::shared_ptr const & channel_a, std::pair const & hash_root_a); std::shared_ptr find_node_id (nano::account const &); std::shared_ptr find_channel (nano::endpoint const &); bool not_a_peer (nano::endpoint const &, bool); diff --git a/nano/node/node.cpp b/nano/node/node.cpp index ca99161876..009158ac64 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -161,7 +161,7 @@ nano::node::node (boost::asio::io_context & io_ctx_a, std::filesystem::path cons tcp_listener{ std::make_shared (network.port, *this, config.tcp_incoming_connections_max) }, application_path (application_path_a), port_mapping (*this), - rep_crawler (*this), + rep_crawler (config.rep_crawler, *this), vote_processor (active, observers, stats, config, flags, logger, online_reps, rep_crawler, ledger, network_params), warmed_up (0), block_processor (*this, write_database_queue), @@ -305,7 +305,7 @@ nano::node::node (boost::asio::io_context & io_ctx_a, std::filesystem::path cons observers.endpoint.add ([this] (std::shared_ptr const & channel_a) { this->network.send_keepalive_self (channel_a); }); - + observers.vote.add ([this] (std::shared_ptr vote, std::shared_ptr const & channel, nano::vote_code code) { debug_assert (code != nano::vote_code::invalid); bool active_in_rep_crawler = rep_crawler.process (vote, channel); diff --git a/nano/node/nodeconfig.cpp b/nano/node/nodeconfig.cpp index 175d78477b..f10f8c7c1e 100644 --- a/nano/node/nodeconfig.cpp +++ b/nano/node/nodeconfig.cpp @@ -31,7 +31,8 @@ nano::node_config::node_config (const std::optional & peering_port_a, hinted_scheduler{ network_params.network }, websocket_config{ network_params.network }, ipc_config{ network_params.network }, - external_address{ boost::asio::ip::address_v6{}.to_string () } + external_address{ boost::asio::ip::address_v6{}.to_string () }, + rep_crawler{ network_params.network } { if (peering_port == 0) { @@ -204,6 +205,10 @@ nano::error nano::node_config::serialize_toml (nano::tomlconfig & toml) const vote_cache.serialize (vote_cache_l); toml.put_child ("vote_cache", vote_cache_l); + nano::tomlconfig rep_crawler_l; + rep_crawler.serialize (rep_crawler_l); + toml.put_child ("rep_crawler", rep_crawler_l); + return toml.get_error (); } @@ -273,6 +278,12 @@ nano::error nano::node_config::deserialize_toml (nano::tomlconfig & toml) vote_cache.deserialize (config_l); } + if (toml.has_key ("rep_crawler")) + { + auto config_l = toml.get_required_child ("rep_crawler"); + rep_crawler.deserialize (config_l); + } + if (toml.has_key ("work_peers")) { work_peers.clear (); diff --git a/nano/node/nodeconfig.hpp b/nano/node/nodeconfig.hpp index 8f7580534a..a7a4c0b185 100644 --- a/nano/node/nodeconfig.hpp +++ b/nano/node/nodeconfig.hpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -127,6 +128,7 @@ class node_config /** Number of times per second to run backlog population batches. Number of accounts per single batch is `backlog_scan_batch_size / backlog_scan_frequency` */ unsigned backlog_scan_frequency{ 10 }; nano::vote_cache_config vote_cache; + nano::rep_crawler_config rep_crawler; public: std::string serialize_frontiers_confirmation (nano::frontiers_confirmation_mode) const; diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index f1bb0cfc8c..2b6fb041c3 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -3,9 +3,11 @@ #include -nano::rep_crawler::rep_crawler (nano::node & node_a) : +nano::rep_crawler::rep_crawler (nano::rep_crawler_config const & config_a, nano::node & node_a) : + config{ config_a }, node{ node_a }, stats{ node_a.stats }, + logger{ node_a.logger }, network_constants{ node_a.network_params.network }, active{ node_a.active } { @@ -46,18 +48,13 @@ void nano::rep_crawler::stop () } } -void nano::rep_crawler::remove (nano::block_hash const & hash) -{ - nano::lock_guard lock{ mutex }; - queries.erase (hash); -} - void nano::rep_crawler::validate_and_process (nano::unique_lock & lock) { debug_assert (!mutex.try_lock ()); debug_assert (lock.owns_lock ()); + debug_assert (!responses.empty ()); // Should be checked before calling this function - decltype (responses) responses_l; + decltype (responses) responses_l{ responses.capacity () }; responses_l.swap (responses); lock.unlock (); @@ -66,22 +63,24 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l // reps with less weight by setting rep_crawler_weight_minimum to a low value auto const minimum = std::min (node.minimum_principal_weight (), node.config.rep_crawler_weight_minimum.number ()); + // TODO: Is it really faster to repeatedly lock/unlock the mutex for each response? for (auto const & response : responses_l) { auto & vote = response.second; auto & channel = response.first; - debug_assert (channel != nullptr); + release_assert (vote != nullptr); + release_assert (channel != nullptr); if (channel->get_type () == nano::transport::transport_type::loopback) { - node.logger.debug (nano::log::type::repcrawler, "Ignoring vote from loopback channel: {}", channel->to_string ()); + logger.debug (nano::log::type::repcrawler, "Ignoring vote from loopback channel: {}", channel->to_string ()); continue; } nano::uint128_t const rep_weight = node.ledger.weight (vote->account); if (rep_weight < minimum) { - node.logger.debug (nano::log::type::repcrawler, "Ignoring vote from account {} with too little voting weight: {}", + logger.debug (nano::log::type::repcrawler, "Ignoring vote from account {} with too little voting weight: {}", vote->account.to_account (), nano::util::to_str (rep_weight)); continue; @@ -96,22 +95,22 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l if (auto existing = reps.find (vote->account); existing != reps.end ()) { - reps.modify (existing, [rep_weight, &updated, &vote, &channel, &prev_channel] (representative_entry & info) { - info.last_response = std::chrono::steady_clock::now (); + reps.modify (existing, [rep_weight, &updated, &vote, &channel, &prev_channel] (rep_entry & rep) { + rep.last_response = std::chrono::steady_clock::now (); // Update if representative channel was changed - if (info.channel->get_endpoint () != channel->get_endpoint ()) + if (rep.channel->get_endpoint () != channel->get_endpoint ()) { - debug_assert (info.account == vote->account); + debug_assert (rep.account == vote->account); updated = true; - prev_channel = info.channel; - info.channel = channel; + prev_channel = rep.channel; + rep.channel = channel; } }); } else { - reps.emplace (representative_entry{ vote->account, channel }); + reps.emplace (rep_entry{ vote->account, channel }); inserted = true; } @@ -119,15 +118,25 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l if (inserted) { - node.logger.info (nano::log::type::repcrawler, "Found representative {} at {}", vote->account.to_account (), channel->to_string ()); + logger.info (nano::log::type::repcrawler, "Found representative {} at {}", vote->account.to_account (), channel->to_string ()); } if (updated) { - node.logger.warn (nano::log::type::repcrawler, "Updated representative {} at {} (was at: {})", vote->account.to_account (), channel->to_string (), prev_channel->to_string ()); + logger.warn (nano::log::type::repcrawler, "Updated representative {} at {} (was at: {})", vote->account.to_account (), channel->to_string (), prev_channel->to_string ()); } } } +std::chrono::milliseconds nano::rep_crawler::query_interval (bool sufficient_weight) const +{ + return sufficient_weight ? network_constants.rep_crawler_normal_interval : network_constants.rep_crawler_warmup_interval; +} + +bool nano::rep_crawler::query_predicate (bool sufficient_weight) const +{ + return nano::elapsed (last_query, query_interval (sufficient_weight)); +} + void nano::rep_crawler::run () { nano::unique_lock lock{ mutex }; @@ -147,7 +156,10 @@ void nano::rep_crawler::run () lock.lock (); - condition.wait_for (lock, sufficient_weight ? network_constants.rep_crawler_normal_interval : network_constants.rep_crawler_warmup_interval); + condition.wait_for (lock, query_interval (sufficient_weight), [this, sufficient_weight] { + return stopped || query_predicate (sufficient_weight) || !responses.empty (); + }); + if (stopped) { return; @@ -155,15 +167,28 @@ void nano::rep_crawler::run () stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::loop); + if (!responses.empty ()) + { + validate_and_process (lock); + debug_assert (!lock.owns_lock ()); + lock.lock (); + } + cleanup (); - validate_and_process (lock); - debug_assert (!lock.owns_lock ()); + if (query_predicate (sufficient_weight)) + { + last_query = std::chrono::steady_clock::now (); - auto targets = get_crawl_targets (current_total_weight); - query (targets); + lock.unlock (); - lock.lock (); + auto targets = prepare_crawl_targets (sufficient_weight); + query (targets); + + lock.lock (); + } + + debug_assert (lock.owns_lock ()); } } @@ -171,26 +196,51 @@ void nano::rep_crawler::cleanup () { debug_assert (!mutex.try_lock ()); - erase_if (reps, [this] (representative_entry const & rep) { + // Evict reps with dead channels + erase_if (reps, [this] (rep_entry const & rep) { if (!rep.channel->alive ()) { + logger.debug (nano::log::type::repcrawler, "Evicting representative {} with dead channel at {}", rep.account.to_account (), rep.channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::channel_dead); return true; // Erase } return false; }); + + // Evict reps that haven't responded in a while + erase_if (reps, [this] (rep_entry const & rep) { + if (nano::elapsed (rep.last_response, config.rep_timeout)) + { + logger.debug (nano::log::type::repcrawler, "Evicting unresponsive representative {} at {}", rep.account.to_account (), rep.channel->to_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::rep_timeout); + return true; // Erase + } + return false; + }); + + // Evict queries that haven't been responded to in a while + erase_if (queries, [this] (query_entry const & query) { + if (nano::elapsed (query.time, config.query_timeout)) + { + logger.debug (nano::log::type::repcrawler, "Evicting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_timeout); + return true; // Erase + } + return false; + }); } -std::vector> nano::rep_crawler::get_crawl_targets (nano::uint128_t current_total_weight) const +std::vector> nano::rep_crawler::prepare_crawl_targets (bool sufficient_weight) const { // TODO: Make these values configurable constexpr std::size_t conservative_count = 10; constexpr std::size_t aggressive_count = 40; // Crawl more aggressively if we lack sufficient total peer weight. - bool sufficient_weight = current_total_weight > node.online_reps.delta (); auto required_peer_count = sufficient_weight ? conservative_count : aggressive_count; + stats.inc (nano::stat::type::rep_crawler, sufficient_weight ? nano::stat::detail::crawl_normal : nano::stat::detail::crawl_aggressive); + // Add random peers. We do this even if we have enough weight, in order to pick up reps // that didn't respond when first observed. If the current total weight isn't sufficient, this // will be more aggressive. When the node first starts, the rep container is empty and all @@ -202,7 +252,7 @@ std::vector> nano::rep_crawler::get_cr return { random_peers.begin (), random_peers.end () }; } -std::optional> nano::rep_crawler::prepare_query_target () +auto nano::rep_crawler::prepare_query_target () -> std::optional { constexpr int max_attempts = 4; @@ -232,23 +282,31 @@ std::optional> nano::rep_crawler:: { nano::lock_guard lock{ mutex }; - for (auto i = 0; queries.count (hash_root->first) != 0 && i < 4; ++i) + for (auto i = 0; queries.get ().count (hash_root->first) != 0 && i < max_attempts; ++i) { hash_root = node.ledger.hash_root_random (transaction); } } - if (!hash_root) - { - return std::nullopt; - } + return hash_root; +} + +void nano::rep_crawler::track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel) +{ + debug_assert (!mutex.try_lock ()); + + debug_assert (queries.count (channel) == 0); // Only a single query should be active per channel + queries.emplace (query_entry{ hash_root.first, channel }); + // Find and update the timestamp on all reps available on the endpoint (a single host may have multiple reps) + auto & index = reps.get (); + auto [begin, end] = index.equal_range (channel); + for (auto it = begin; it != end; ++it) { - nano::lock_guard lock{ mutex }; - queries.insert (hash_root->first); + index.modify (it, [] (rep_entry & info) { + info.last_request = std::chrono::steady_clock::now (); + }); } - - return hash_root; } void nano::rep_crawler::query (std::vector> const & target_channels) @@ -256,27 +314,31 @@ void nano::rep_crawler::query (std::vector lock{ mutex }; + for (const auto & channel : target_channels) { debug_assert (channel != nullptr); - on_rep_request (channel); - node.network.send_confirm_req (channel, hash_root); - } - // TODO: Use a thread+timeout instead of a timer - // A representative must respond with a vote within the deadline - std::weak_ptr node_w (node.shared ()); - node.workers.add_timed_task (std::chrono::steady_clock::now () + std::chrono::seconds (5), [node_w, hash = hash_root.first] () { - if (auto node_l = node_w.lock ()) + // Only a single query should be active per channel + if (queries.find (channel) == queries.end ()) { - auto target_finished_processed (node_l->vote_processor.total_processed + node_l->vote_processor.size ()); - node_l->rep_crawler.throttled_remove (hash, target_finished_processed); + track_rep_request (hash_root, channel); + node.network.send_confirm_req (channel, hash_root); + + logger.debug (nano::log::type::repcrawler, "Sending query for block {} to {}", hash_root.first.to_string (), channel->to_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_sent); } - }); + else + { + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_channel_busy); + } + } } void nano::rep_crawler::query (std::shared_ptr const & target_channel) @@ -284,32 +346,13 @@ void nano::rep_crawler::query (std::shared_ptr const & query (std::vector{ target_channel }); } -// TODO: Use a thread+timeout instead of a timer -void nano::rep_crawler::throttled_remove (nano::block_hash const & hash_a, uint64_t const target_finished_processed) -{ - if (node.vote_processor.total_processed >= target_finished_processed) - { - remove (hash_a); - } - else - { - std::weak_ptr node_w (node.shared ()); - node.workers.add_timed_task (std::chrono::steady_clock::now () + std::chrono::seconds (5), [node_w, hash_a, target_finished_processed] () { - if (auto node_l = node_w.lock ()) - { - node_l->rep_crawler.throttled_remove (hash_a, target_finished_processed); - } - }); - } -} - bool nano::rep_crawler::is_pr (std::shared_ptr const & channel) const { nano::lock_guard lock{ mutex }; auto existing = reps.get ().find (channel); if (existing != reps.get ().end ()) { - return node.ledger.weight (existing->account) > node.minimum_principal_weight (); + return node.ledger.weight (existing->account) >= node.minimum_principal_weight (); } return false; } @@ -317,12 +360,24 @@ bool nano::rep_crawler::is_pr (std::shared_ptr const & bool nano::rep_crawler::process (std::shared_ptr const & vote, std::shared_ptr const & channel) { nano::lock_guard lock{ mutex }; - for (auto const & hash : vote->hashes) // TODO: This most likely should be a single hash vote + if (auto info = queries.find (channel); info != queries.end ()) { - if (queries.count (hash) != 0) + // TODO: This linear search could be slow, especially with large votes. + auto const target_hash = info->hash; + bool found = std::any_of (vote->hashes.begin (), vote->hashes.end (), [&target_hash] (nano::block_hash const & hash) { + return hash == target_hash; + }); + + if (found) { - responses.emplace_back (channel, vote); - return true; // Processed + logger.debug (nano::log::type::repcrawler, "Processing response for block {} from {}", target_hash.to_string (), channel->to_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::response); + // TODO: Track query response time + + responses.push_back ({ channel, vote }); + + queries.erase (info); + return true; } } return false; @@ -342,30 +397,13 @@ nano::uint128_t nano::rep_crawler::total_weight () const return result; } -void nano::rep_crawler::on_rep_request (std::shared_ptr const & channel) -{ - nano::lock_guard lock{ mutex }; - if (channel->get_tcp_endpoint ().address () != boost::asio::ip::address_v6::any ()) - { - // Find and update the timestamp on all reps available on the endpoint (a single host may have multiple reps) - auto & index = reps.get (); - auto [begin, end] = index.equal_range (channel); - for (auto it = begin; it != end; ++it) - { - index.modify (it, [] (representative_entry & info) { - info.last_request = std::chrono::steady_clock::now (); - }); - } - } -} - std::vector nano::rep_crawler::representatives (std::size_t count_a, nano::uint128_t const weight_a, boost::optional const & opt_version_min_a) { auto const version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); nano::lock_guard lock{ mutex }; - std::multimap> ordered; + std::multimap> ordered; for (const auto & i : reps.get ()) { auto weight = node.ledger.weight (i.account); @@ -400,7 +438,7 @@ void nano::rep_crawler::force_add_rep (const nano::account & account, const std: { release_assert (node.network_params.network.is_dev_network ()); nano::lock_guard lock{ mutex }; - reps.emplace (representative_entry{ account, channel }); + reps.emplace (rep_entry{ account, channel }); } // Only for tests @@ -408,18 +446,7 @@ void nano::rep_crawler::force_process (const std::shared_ptr & vote, { release_assert (node.network_params.network.is_dev_network ()); nano::lock_guard lock{ mutex }; - for (auto const & hash : vote->hashes) - { - responses.emplace_back (channel, vote); - } -} - -// Only for tests -void nano::rep_crawler::force_active_query (const nano::block_hash & hash) -{ - release_assert (node.network_params.network.is_dev_network ()); - nano::lock_guard lock{ mutex }; - queries.insert (hash); + responses.push_back ({ channel, vote }); } std::unique_ptr nano::collect_container_info (rep_crawler & rep_crawler, std::string const & name) @@ -434,4 +461,39 @@ std::unique_ptr nano::collect_container_info (re auto composite = std::make_unique (name); composite->add_component (std::make_unique (container_info{ "queries", count, sizeof_element })); return composite; +} + +/* + * rep_crawler_config + */ + +nano::rep_crawler_config::rep_crawler_config (nano::network_constants const & network_constants) +{ + if (network_constants.is_dev_network ()) + { + rep_timeout = std::chrono::milliseconds{ 1000 * 3 }; + query_timeout = std::chrono::milliseconds{ 1000 }; + } +} + +nano::error nano::rep_crawler_config::serialize (nano::tomlconfig & toml) const +{ + // TODO: Descriptions + toml.put ("rep_timeout", rep_timeout.count ()); + toml.put ("query_timeout", query_timeout.count ()); + + return toml.get_error (); +} + +nano::error nano::rep_crawler_config::deserialize (nano::tomlconfig & toml) +{ + auto rep_timeout_l = rep_timeout.count (); + toml.get ("rep_timeout", rep_timeout_l); + rep_timeout = std::chrono::milliseconds{ rep_timeout_l }; + + auto query_timeout_l = query_timeout.count (); + toml.get ("query_timeout", query_timeout_l); + query_timeout = std::chrono::milliseconds{ query_timeout_l }; + + return toml.get_error (); } \ No newline at end of file diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index c58449a409..c3ab336264 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -30,6 +31,19 @@ struct representative std::shared_ptr channel; }; +class rep_crawler_config final +{ +public: + explicit rep_crawler_config (nano::network_constants const &); + + nano::error deserialize (nano::tomlconfig & toml); + nano::error serialize (nano::tomlconfig & toml) const; + +public: + std::chrono::milliseconds query_timeout{ 1000 * 30 }; + std::chrono::milliseconds rep_timeout{ 1000 * 60 * 5 }; +}; + /** * Crawls the network for representatives. Queries are performed by requesting confirmation of a * random block and observing the corresponding vote. @@ -39,7 +53,7 @@ class rep_crawler final friend std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); public: - explicit rep_crawler (nano::node &); + rep_crawler (rep_crawler_config const &, nano::node &); ~rep_crawler (); void start (); @@ -53,12 +67,6 @@ class rep_crawler final */ bool process (std::shared_ptr const &, std::shared_ptr const &); - /** Remove block hash from list of active rep queries */ - void remove (nano::block_hash const &); - - /** Remove block hash from with delay depending on vote processor size */ - void throttled_remove (nano::block_hash const &, uint64_t target_finished_processed); - /** Attempt to determine if the peer manages one or more representative accounts */ void query (std::vector> const & target_channels); @@ -81,8 +89,10 @@ class rep_crawler final std::size_t representative_count (); private: // Dependencies + rep_crawler_config const & config; nano::node & node; nano::stats & stats; + nano::logger & logger; nano::network_constants & network_constants; nano::active_transactions & active; @@ -90,22 +100,23 @@ class rep_crawler final void run (); void cleanup (); void validate_and_process (nano::unique_lock &); + bool query_predicate (bool sufficient_weight) const; + std::chrono::milliseconds query_interval (bool sufficient_weight) const; - /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ - std::vector> get_crawl_targets (nano::uint128_t current_total_weight) const; - - /** When a rep request is made, this is called to update the last-request timestamp. */ - void on_rep_request (std::shared_ptr const & channel_a); + using hash_root_t = std::pair; - std::optional> prepare_query_target (); + /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ + std::vector> prepare_crawl_targets (bool sufficient_weight) const; + std::optional prepare_query_target (); + void track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel_a); private: /** * A representative picked up during repcrawl. */ - struct representative_entry + struct rep_entry { - representative_entry (nano::account account_a, std::shared_ptr const & channel_a) : + rep_entry (nano::account account_a, std::shared_ptr const & channel_a) : account{ account_a }, channel{ channel_a } { @@ -116,7 +127,7 @@ class rep_crawler final std::shared_ptr channel; std::chrono::steady_clock::time_point last_request{}; - std::chrono::steady_clock::time_point last_response{}; + std::chrono::steady_clock::time_point last_response{ std::chrono::steady_clock::now () }; nano::account get_account () const { @@ -124,30 +135,49 @@ class rep_crawler final } }; + struct query_entry + { + nano::block_hash hash; + std::shared_ptr channel; + std::chrono::steady_clock::time_point time{ std::chrono::steady_clock::now () }; + }; + // clang-format off + class tag_hash {}; class tag_account {}; class tag_channel {}; class tag_sequenced {}; - using ordered_reps = boost::multi_index_container, - mi::const_mem_fun>, + mi::const_mem_fun>, mi::sequenced>, mi::hashed_non_unique, - mi::member, &representative_entry::channel>> + mi::member, &rep_entry::channel>> + >>; + + using ordered_queries = boost::multi_index_container, + mi::member, &query_entry::channel>>, + mi::sequenced>, + mi::hashed_non_unique, + mi::member> >>; // clang-format on ordered_reps reps; + ordered_queries queries; private: - /** We have solicited votes for these random blocks */ - std::unordered_set queries; + static size_t constexpr max_responses{ 1024 * 4 }; + using response_t = std::pair, std::shared_ptr>; + boost::circular_buffer responses{ max_responses }; - std::deque, std::shared_ptr>> responses; + std::chrono::steady_clock::time_point last_query{}; - bool stopped{ false }; + std::atomic stopped{ false }; nano::condition_variable condition; mutable nano::mutex mutex; std::thread thread; @@ -155,7 +185,6 @@ class rep_crawler final public: // Testing void force_add_rep (nano::account const & account, std::shared_ptr const & channel); void force_process (std::shared_ptr const & vote, std::shared_ptr const & channel); - void force_active_query (nano::block_hash const & hash); }; std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); From 60f19010c02de76ffdb96a0724715d3b26d83d4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Fri, 23 Feb 2024 12:57:01 +0100 Subject: [PATCH 13/27] Fix tests --- nano/core_test/node.cpp | 4 +++- nano/core_test/rep_crawler.cpp | 6 +++--- nano/node/repcrawler.cpp | 8 ++++++++ nano/node/repcrawler.hpp | 1 + 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index c38cc8f4cc..53b3872b3e 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -1774,7 +1774,9 @@ TEST (node, online_reps_rep_crawler) node1.vote_processor.vote_blocking (vote, std::make_shared (node1)); ASSERT_EQ (0, node1.online_reps.online ()); // After inserting to rep crawler - node1.rep_crawler.force_process (vote, std::make_shared (node1)); + auto channel = std::make_shared (node1); + node1.rep_crawler.force_query (nano::dev::genesis->hash (), channel); + node1.vote_processor.vote_blocking (vote, channel); ASSERT_EQ (nano::dev::constants.genesis_amount, node1.online_reps.online ()); } diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index a6f88e5796..85f4ba1811 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -94,9 +94,9 @@ TEST (rep_crawler, rep_weight) auto vote0 = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); auto vote1 = std::make_shared (keypair1.pub, keypair1.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); auto vote2 = std::make_shared (keypair2.pub, keypair2.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); - ASSERT_TRUE (node.rep_crawler.process (vote0, channel1)); - ASSERT_TRUE (node.rep_crawler.process (vote1, channel2)); - ASSERT_TRUE (node.rep_crawler.process (vote2, channel3)); + node.rep_crawler.force_process (vote0, channel1); + node.rep_crawler.force_process (vote1, channel2); + node.rep_crawler.force_process (vote2, channel3); ASSERT_TIMELY_EQ (5s, node.rep_crawler.representative_count (), 2); // Make sure we get the rep with the most weight first auto reps = node.rep_crawler.representatives (1); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 2b6fb041c3..f47cddb961 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -449,6 +449,14 @@ void nano::rep_crawler::force_process (const std::shared_ptr & vote, responses.push_back ({ channel, vote }); } +// Only for tests +void nano::rep_crawler::force_query (const nano::block_hash & hash, const std::shared_ptr & channel) +{ + release_assert (node.network_params.network.is_dev_network ()); + nano::lock_guard lock{ mutex }; + queries.emplace (query_entry{ hash, channel }); +} + std::unique_ptr nano::collect_container_info (rep_crawler & rep_crawler, std::string const & name) { std::size_t count; diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index c3ab336264..3a8ec3ab3e 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -185,6 +185,7 @@ class rep_crawler final public: // Testing void force_add_rep (nano::account const & account, std::shared_ptr const & channel); void force_process (std::shared_ptr const & vote, std::shared_ptr const & channel); + void force_query (nano::block_hash const & hash, std::shared_ptr const & channel); }; std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); From 331ea3cc2c516d02d8b70b1568ab9e3420fefd5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 24 Feb 2024 15:20:07 +0100 Subject: [PATCH 14/27] Cleanup --- nano/node/repcrawler.cpp | 23 ++++++++++++----------- nano/node/repcrawler.hpp | 11 +++++++---- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index f47cddb961..c5972bd72f 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -1,7 +1,7 @@ #include #include -#include +#include nano::rep_crawler::rep_crawler (nano::rep_crawler_config const & config_a, nano::node & node_a) : config{ config_a }, @@ -48,6 +48,7 @@ void nano::rep_crawler::stop () } } +// Exits with the lock unlocked void nano::rep_crawler::validate_and_process (nano::unique_lock & lock) { debug_assert (!mutex.try_lock ()); @@ -397,33 +398,33 @@ nano::uint128_t nano::rep_crawler::total_weight () const return result; } -std::vector nano::rep_crawler::representatives (std::size_t count_a, nano::uint128_t const weight_a, boost::optional const & opt_version_min_a) +std::vector nano::rep_crawler::representatives (std::size_t count, nano::uint128_t const minimum_weight, std::optional const & minimum_protocol_version) { - auto const version_min = opt_version_min_a.value_or (node.network_params.network.protocol_version_min); + auto const version_min = minimum_protocol_version.value_or (node.network_params.network.protocol_version_min); nano::lock_guard lock{ mutex }; std::multimap> ordered; - for (const auto & i : reps.get ()) + for (const auto & rep : reps.get ()) { - auto weight = node.ledger.weight (i.account); - if (weight > weight_a && i.channel->get_network_version () >= version_min) + auto weight = node.ledger.weight (rep.account); + if (weight >= minimum_weight && rep.channel->get_network_version () >= version_min) { - ordered.insert ({ nano::amount{ weight }, i }); + ordered.insert ({ nano::amount{ weight }, rep }); } } std::vector result; - for (auto i = ordered.begin (), n = ordered.end (); i != n && result.size () < count_a; ++i) + for (auto const & [weight, rep] : ordered | std::views::take (count)) { - result.push_back ({ i->second.account, i->second.channel }); + result.push_back ({ rep.account, rep.channel }); } return result; } -std::vector nano::rep_crawler::principal_representatives (std::size_t count_a, boost::optional const & opt_version_min_a) +std::vector nano::rep_crawler::principal_representatives (std::size_t count, std::optional const & minimum_protocol_version) { - return representatives (count_a, node.minimum_principal_weight (), opt_version_min_a); + return representatives (count, node.minimum_principal_weight (), minimum_protocol_version); } /** Total number of representatives */ diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 3a8ec3ab3e..48c8bda3a0 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -79,13 +79,16 @@ class rep_crawler final /** Get total available weight from representatives */ nano::uint128_t total_weight () const; - /** Request a list of the top \p count_a known representatives in descending order of weight, with at least \p weight_a voting weight, and optionally with a minimum version \p opt_version_min_a */ - std::vector representatives (std::size_t count_a = std::numeric_limits::max (), nano::uint128_t weight_a = 0, boost::optional const & opt_version_min_a = boost::none); + /** Request a list of the top \p count known representatives in descending order of weight, with at least \p weight_a voting weight, and optionally with a minimum version \p minimum_protocol_version + */ + std::vector representatives (std::size_t count = std::numeric_limits::max (), nano::uint128_t minimum_weight = 0, std::optional const & minimum_protocol_version = {}); - /** Request a list of the top \p count_a known principal representatives in descending order of weight, optionally with a minimum version \p opt_version_min_a */ - std::vector principal_representatives (std::size_t count_a = std::numeric_limits::max (), boost::optional const & opt_version_min_a = boost::none); + /** Request a list of the top \p count known principal representatives in descending order of weight, optionally with a minimum version \p minimum_protocol_version + */ + std::vector principal_representatives (std::size_t count = std::numeric_limits::max (), std::optional const & minimum_protocol_version = {}); /** Total number of representatives */ + std::size_t representative_count (); private: // Dependencies From 5c93dddac9d4bbd18edf10510a2da356f1e14166 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 24 Feb 2024 15:54:24 +0100 Subject: [PATCH 15/27] Uniform rep crawler log enum naming --- nano/lib/logging_enums.hpp | 2 +- nano/node/repcrawler.cpp | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/nano/lib/logging_enums.hpp b/nano/lib/logging_enums.hpp index 33d40a3ce1..41668e674e 100644 --- a/nano/lib/logging_enums.hpp +++ b/nano/lib/logging_enums.hpp @@ -63,7 +63,7 @@ enum class type epoch_upgrader, opencl_work, upnp, - repcrawler, + rep_crawler, lmdb, rocksdb, txn_tracker, diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index c5972bd72f..c5b6dbb700 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -74,14 +74,14 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l if (channel->get_type () == nano::transport::transport_type::loopback) { - logger.debug (nano::log::type::repcrawler, "Ignoring vote from loopback channel: {}", channel->to_string ()); + logger.debug (nano::log::type::rep_crawler, "Ignoring vote from loopback channel: {}", channel->to_string ()); continue; } nano::uint128_t const rep_weight = node.ledger.weight (vote->account); if (rep_weight < minimum) { - logger.debug (nano::log::type::repcrawler, "Ignoring vote from account {} with too little voting weight: {}", + logger.debug (nano::log::type::rep_crawler, "Ignoring vote from account {} with too little voting weight: {}", vote->account.to_account (), nano::util::to_str (rep_weight)); continue; @@ -119,11 +119,11 @@ void nano::rep_crawler::validate_and_process (nano::unique_lock & l if (inserted) { - logger.info (nano::log::type::repcrawler, "Found representative {} at {}", vote->account.to_account (), channel->to_string ()); + logger.info (nano::log::type::rep_crawler, "Found representative {} at {}", vote->account.to_account (), channel->to_string ()); } if (updated) { - logger.warn (nano::log::type::repcrawler, "Updated representative {} at {} (was at: {})", vote->account.to_account (), channel->to_string (), prev_channel->to_string ()); + logger.warn (nano::log::type::rep_crawler, "Updated representative {} at {} (was at: {})", vote->account.to_account (), channel->to_string (), prev_channel->to_string ()); } } } @@ -201,7 +201,7 @@ void nano::rep_crawler::cleanup () erase_if (reps, [this] (rep_entry const & rep) { if (!rep.channel->alive ()) { - logger.debug (nano::log::type::repcrawler, "Evicting representative {} with dead channel at {}", rep.account.to_account (), rep.channel->to_string ()); + logger.debug (nano::log::type::rep_crawler, "Evicting representative {} with dead channel at {}", rep.account.to_account (), rep.channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::channel_dead); return true; // Erase } @@ -212,7 +212,7 @@ void nano::rep_crawler::cleanup () erase_if (reps, [this] (rep_entry const & rep) { if (nano::elapsed (rep.last_response, config.rep_timeout)) { - logger.debug (nano::log::type::repcrawler, "Evicting unresponsive representative {} at {}", rep.account.to_account (), rep.channel->to_string ()); + logger.debug (nano::log::type::rep_crawler, "Evicting unresponsive representative {} at {}", rep.account.to_account (), rep.channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::rep_timeout); return true; // Erase } @@ -223,7 +223,7 @@ void nano::rep_crawler::cleanup () erase_if (queries, [this] (query_entry const & query) { if (nano::elapsed (query.time, config.query_timeout)) { - logger.debug (nano::log::type::repcrawler, "Evicting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); + logger.debug (nano::log::type::rep_crawler, "Evicting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_timeout); return true; // Erase } @@ -332,7 +332,7 @@ void nano::rep_crawler::query (std::vectorto_string ()); + logger.debug (nano::log::type::rep_crawler, "Sending query for block {} to {}", hash_root.first.to_string (), channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_sent); } else @@ -371,7 +371,7 @@ bool nano::rep_crawler::process (std::shared_ptr const & vote, std:: if (found) { - logger.debug (nano::log::type::repcrawler, "Processing response for block {} from {}", target_hash.to_string (), channel->to_string ()); + logger.debug (nano::log::type::rep_crawler, "Processing response for block {} from {}", target_hash.to_string (), channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::response); // TODO: Track query response time From 603379e4877b834482dc391e45f73d3c4e0718d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 24 Feb 2024 15:57:19 +0100 Subject: [PATCH 16/27] Notify condition after successful response --- nano/node/repcrawler.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index c5b6dbb700..5bb35e0dc6 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -376,8 +376,9 @@ bool nano::rep_crawler::process (std::shared_ptr const & vote, std:: // TODO: Track query response time responses.push_back ({ channel, vote }); - queries.erase (info); + + condition.notify_all (); return true; } } From 202d6ce7cb85c5f2a6d43ab8628103683f282260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sun, 25 Feb 2024 13:05:52 +0100 Subject: [PATCH 17/27] Simplify `keepalive_preconfigured ()` --- nano/node/node.cpp | 6 +++--- nano/node/node.hpp | 2 +- nano/node/repcrawler.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 009158ac64..53a1a1726c 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -689,14 +689,14 @@ void nano::node::stop () // work pool is not stopped on purpose due to testing setup } -void nano::node::keepalive_preconfigured (std::vector const & peers_a) +void nano::node::keepalive_preconfigured () { - for (auto i (peers_a.begin ()), n (peers_a.end ()); i != n; ++i) + for (auto const & peer : config.preconfigured_peers) { // can't use `network.port` here because preconfigured peers are referenced // just by their address, so we rely on them listening on the default port // - keepalive (*i, network_params.network.default_node_port); + keepalive (peer, network_params.network.default_node_port); } } diff --git a/nano/node/node.hpp b/nano/node/node.hpp index e3daaae82a..f87dc3096d 100644 --- a/nano/node/node.hpp +++ b/nano/node/node.hpp @@ -86,7 +86,7 @@ class node final : public std::enable_shared_from_this void process_active (std::shared_ptr const &); std::optional process_local (std::shared_ptr const &); void process_local_async (std::shared_ptr const &); - void keepalive_preconfigured (std::vector const &); + void keepalive_preconfigured (); std::shared_ptr block (nano::block_hash const &); std::pair balance_pending (nano::account const &, bool only_confirmed); nano::uint128_t weight (nano::account const &); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 5bb35e0dc6..2dded73005 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -152,7 +152,7 @@ void nano::rep_crawler::run () if (!sufficient_weight) { stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::keepalive); - node.keepalive_preconfigured (node.config.preconfigured_peers); + node.keepalive_preconfigured (); } lock.lock (); From beb4c5beb4851c26749f53b6a883335ab668a570 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sun, 25 Feb 2024 13:03:05 +0100 Subject: [PATCH 18/27] Allow multiple queries per channel during warmup --- nano/lib/stats_enums.hpp | 1 + nano/node/repcrawler.cpp | 58 ++++++++++++++++++++++++---------------- nano/node/repcrawler.hpp | 4 +-- 3 files changed, 38 insertions(+), 25 deletions(-) diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index fe15550b2f..edad231fd5 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -334,6 +334,7 @@ enum class detail : uint8_t query_target_failed, query_channel_busy, query_sent, + query_duplicate, rep_timeout, query_timeout, crawl_aggressive, diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 2dded73005..746de3040c 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -181,11 +181,10 @@ void nano::rep_crawler::run () { last_query = std::chrono::steady_clock::now (); - lock.unlock (); - auto targets = prepare_crawl_targets (sufficient_weight); - query (targets); + lock.unlock (); + query (targets); lock.lock (); } @@ -233,23 +232,27 @@ void nano::rep_crawler::cleanup () std::vector> nano::rep_crawler::prepare_crawl_targets (bool sufficient_weight) const { + debug_assert (!mutex.try_lock ()); + // TODO: Make these values configurable constexpr std::size_t conservative_count = 10; constexpr std::size_t aggressive_count = 40; + constexpr std::size_t conservative_max_attempts = 1; + constexpr std::size_t aggressive_max_attempts = 8; + + stats.inc (nano::stat::type::rep_crawler, sufficient_weight ? nano::stat::detail::crawl_normal : nano::stat::detail::crawl_aggressive); // Crawl more aggressively if we lack sufficient total peer weight. - auto required_peer_count = sufficient_weight ? conservative_count : aggressive_count; + auto const required_peer_count = sufficient_weight ? conservative_count : aggressive_count; - stats.inc (nano::stat::type::rep_crawler, sufficient_weight ? nano::stat::detail::crawl_normal : nano::stat::detail::crawl_aggressive); + auto random_peers = node.network.random_set (required_peer_count, 0, /* Include channels with ephemeral remote ports */ true); - // Add random peers. We do this even if we have enough weight, in order to pick up reps - // that didn't respond when first observed. If the current total weight isn't sufficient, this - // will be more aggressive. When the node first starts, the rep container is empty and all - // endpoints will originate from random peers. - required_peer_count += required_peer_count / 2; + // Avoid querying the same peer multiple times when rep crawler is warmed up + auto const max_attempts = sufficient_weight ? conservative_max_attempts : aggressive_max_attempts; + erase_if (random_peers, [this, max_attempts] (std::shared_ptr const & channel) { + return queries.get ().count (channel) >= max_attempts; + }); - // The rest of the endpoints are picked randomly - auto random_peers = node.network.random_set (required_peer_count, 0, true); // Include channels with ephemeral remote ports return { random_peers.begin (), random_peers.end () }; } @@ -292,12 +295,15 @@ auto nano::rep_crawler::prepare_query_target () -> std::optional return hash_root; } -void nano::rep_crawler::track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel) +bool nano::rep_crawler::track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel) { debug_assert (!mutex.try_lock ()); - debug_assert (queries.count (channel) == 0); // Only a single query should be active per channel - queries.emplace (query_entry{ hash_root.first, channel }); + auto [_, inserted] = queries.emplace (query_entry{ hash_root.first, channel }); + if (!inserted) + { + return false; // Duplicate, not tracked + } // Find and update the timestamp on all reps available on the endpoint (a single host may have multiple reps) auto & index = reps.get (); @@ -308,6 +314,8 @@ void nano::rep_crawler::track_rep_request (hash_root_t hash_root, std::shared_pt info.last_request = std::chrono::steady_clock::now (); }); } + + return true; } void nano::rep_crawler::query (std::vector> const & target_channels) @@ -315,6 +323,7 @@ void nano::rep_crawler::query (std::vectorto_string ()); @@ -337,7 +345,8 @@ void nano::rep_crawler::query (std::vectorto_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_duplicate); } } } @@ -361,10 +370,13 @@ bool nano::rep_crawler::is_pr (std::shared_ptr const & bool nano::rep_crawler::process (std::shared_ptr const & vote, std::shared_ptr const & channel) { nano::lock_guard lock{ mutex }; - if (auto info = queries.find (channel); info != queries.end ()) + + auto & index = queries.get (); + auto [begin, end] = index.equal_range (channel); + for (auto it = begin; it != end; ++it) { // TODO: This linear search could be slow, especially with large votes. - auto const target_hash = info->hash; + auto const target_hash = it->hash; bool found = std::any_of (vote->hashes.begin (), vote->hashes.end (), [&target_hash] (nano::block_hash const & hash) { return hash == target_hash; }); @@ -376,10 +388,10 @@ bool nano::rep_crawler::process (std::shared_ptr const & vote, std:: // TODO: Track query response time responses.push_back ({ channel, vote }); - queries.erase (info); + queries.erase (it); condition.notify_all (); - return true; + return true; // Found and processed } } return false; diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 48c8bda3a0..0b20530475 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -111,7 +111,7 @@ class rep_crawler final /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ std::vector> prepare_crawl_targets (bool sufficient_weight) const; std::optional prepare_query_target (); - void track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel_a); + bool track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel_a); private: /** @@ -162,7 +162,7 @@ class rep_crawler final using ordered_queries = boost::multi_index_container, + mi::hashed_non_unique, mi::member, &query_entry::channel>>, mi::sequenced>, mi::hashed_non_unique, From 638b18e47f1697e509eb8dd2d7e725edcfee487b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sun, 25 Feb 2024 15:07:38 +0100 Subject: [PATCH 19/27] Improve `collect_container_info ()` --- nano/node/node.cpp | 2 +- nano/node/repcrawler.cpp | 26 +++++++++++--------------- nano/node/repcrawler.hpp | 9 +++------ 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 53a1a1726c..9a34388c5b 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -523,7 +523,7 @@ std::unique_ptr nano::collect_container_info (no composite->add_component (collect_container_info (node.observers, "observers")); composite->add_component (collect_container_info (node.wallets, "wallets")); composite->add_component (collect_container_info (node.vote_processor, "vote_processor")); - composite->add_component (collect_container_info (node.rep_crawler, "rep_crawler")); + composite->add_component (node.rep_crawler.collect_container_info ("rep_crawler")); composite->add_component (node.block_processor.collect_container_info ("block_processor")); composite->add_component (collect_container_info (node.online_reps, "online_reps")); composite->add_component (collect_container_info (node.history, "history")); diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 746de3040c..fbb9eaf1e8 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -440,13 +440,23 @@ std::vector nano::rep_crawler::principal_representatives ( return representatives (count, node.minimum_principal_weight (), minimum_protocol_version); } -/** Total number of representatives */ std::size_t nano::rep_crawler::representative_count () { nano::lock_guard lock{ mutex }; return reps.size (); } +std::unique_ptr nano::rep_crawler::collect_container_info (const std::string & name) +{ + nano::lock_guard guard{ mutex }; + + auto composite = std::make_unique (name); + composite->add_component (std::make_unique (container_info{ "reps", reps.size (), sizeof (decltype (reps)::value_type) })); + composite->add_component (std::make_unique (container_info{ "queries", queries.size (), sizeof (decltype (queries)::value_type) })); + composite->add_component (std::make_unique (container_info{ "responses", responses.size (), sizeof (decltype (responses)::value_type) })); + return composite; +} + // Only for tests void nano::rep_crawler::force_add_rep (const nano::account & account, const std::shared_ptr & channel) { @@ -471,20 +481,6 @@ void nano::rep_crawler::force_query (const nano::block_hash & hash, const std::s queries.emplace (query_entry{ hash, channel }); } -std::unique_ptr nano::collect_container_info (rep_crawler & rep_crawler, std::string const & name) -{ - std::size_t count; - { - nano::lock_guard guard{ rep_crawler.mutex }; - count = rep_crawler.queries.size (); - } - - auto const sizeof_element = sizeof (decltype (rep_crawler.queries)::value_type); - auto composite = std::make_unique (name); - composite->add_component (std::make_unique (container_info{ "queries", count, sizeof_element })); - return composite; -} - /* * rep_crawler_config */ diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 0b20530475..c7dab61a24 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -50,8 +50,6 @@ class rep_crawler_config final */ class rep_crawler final { - friend std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); - public: rep_crawler (rep_crawler_config const &, nano::node &); ~rep_crawler (); @@ -88,9 +86,10 @@ class rep_crawler final std::vector principal_representatives (std::size_t count = std::numeric_limits::max (), std::optional const & minimum_protocol_version = {}); /** Total number of representatives */ - std::size_t representative_count (); + std::unique_ptr collect_container_info (std::string const & name); + private: // Dependencies rep_crawler_config const & config; nano::node & node; @@ -111,7 +110,7 @@ class rep_crawler final /** Returns a list of endpoints to crawl. The total weight is passed in to avoid computing it twice. */ std::vector> prepare_crawl_targets (bool sufficient_weight) const; std::optional prepare_query_target (); - bool track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel_a); + bool track_rep_request (hash_root_t hash_root, std::shared_ptr const & channel); private: /** @@ -190,6 +189,4 @@ class rep_crawler final void force_process (std::shared_ptr const & vote, std::shared_ptr const & channel); void force_query (nano::block_hash const & hash, std::shared_ptr const & channel); }; - -std::unique_ptr collect_container_info (rep_crawler & rep_crawler, std::string const & name); } From 701d8214b3d0c43e3fb17eb970affa7d2366a36a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:13:40 +0100 Subject: [PATCH 20/27] No limiter drop --- nano/node/repcrawler.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index fbb9eaf1e8..d41916fdae 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -338,10 +338,21 @@ void nano::rep_crawler::query (std::vectorto_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_sent); + + auto const & [hash, root] = hash_root; + nano::confirm_req req{ network_constants, hash, root }; + + channel->send ( + req, + [this] (auto & ec, auto size) { + if (ec) + { + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::write_error, nano::stat::dir::out); + } + }, + nano::transport::buffer_drop_policy::no_socket_drop); } else { From 5cc825627582e0671156ef967092c1b52d0be14f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 26 Feb 2024 17:57:23 +0100 Subject: [PATCH 21/27] Adjustments --- nano/node/repcrawler.cpp | 29 ++++++----------------------- nano/node/repcrawler.hpp | 3 +-- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index d41916fdae..c89f142185 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -200,29 +200,18 @@ void nano::rep_crawler::cleanup () erase_if (reps, [this] (rep_entry const & rep) { if (!rep.channel->alive ()) { - logger.debug (nano::log::type::rep_crawler, "Evicting representative {} with dead channel at {}", rep.account.to_account (), rep.channel->to_string ()); + logger.info (nano::log::type::rep_crawler, "Evicting representative {} with dead channel at {}", rep.account.to_account (), rep.channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::channel_dead); return true; // Erase } return false; }); - // Evict reps that haven't responded in a while - erase_if (reps, [this] (rep_entry const & rep) { - if (nano::elapsed (rep.last_response, config.rep_timeout)) - { - logger.debug (nano::log::type::rep_crawler, "Evicting unresponsive representative {} at {}", rep.account.to_account (), rep.channel->to_string ()); - stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::rep_timeout); - return true; // Erase - } - return false; - }); - // Evict queries that haven't been responded to in a while erase_if (queries, [this] (query_entry const & query) { if (nano::elapsed (query.time, config.query_timeout)) { - logger.debug (nano::log::type::rep_crawler, "Evicting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); + logger.debug (nano::log::type::rep_crawler, "Aborting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_timeout); return true; // Erase } @@ -235,9 +224,9 @@ std::vector> nano::rep_crawler::prepar debug_assert (!mutex.try_lock ()); // TODO: Make these values configurable - constexpr std::size_t conservative_count = 10; - constexpr std::size_t aggressive_count = 40; - constexpr std::size_t conservative_max_attempts = 1; + constexpr std::size_t conservative_count = 160; + constexpr std::size_t aggressive_count = 160; + constexpr std::size_t conservative_max_attempts = 4; constexpr std::size_t aggressive_max_attempts = 8; stats.inc (nano::stat::type::rep_crawler, sufficient_weight ? nano::stat::detail::crawl_normal : nano::stat::detail::crawl_aggressive); @@ -245,7 +234,7 @@ std::vector> nano::rep_crawler::prepar // Crawl more aggressively if we lack sufficient total peer weight. auto const required_peer_count = sufficient_weight ? conservative_count : aggressive_count; - auto random_peers = node.network.random_set (required_peer_count, 0, /* Include channels with ephemeral remote ports */ true); + auto random_peers = node.network.random_set (required_peer_count, 0, /* include channels with ephemeral remote ports */ true); // Avoid querying the same peer multiple times when rep crawler is warmed up auto const max_attempts = sufficient_weight ? conservative_max_attempts : aggressive_max_attempts; @@ -500,7 +489,6 @@ nano::rep_crawler_config::rep_crawler_config (nano::network_constants const & ne { if (network_constants.is_dev_network ()) { - rep_timeout = std::chrono::milliseconds{ 1000 * 3 }; query_timeout = std::chrono::milliseconds{ 1000 }; } } @@ -508,7 +496,6 @@ nano::rep_crawler_config::rep_crawler_config (nano::network_constants const & ne nano::error nano::rep_crawler_config::serialize (nano::tomlconfig & toml) const { // TODO: Descriptions - toml.put ("rep_timeout", rep_timeout.count ()); toml.put ("query_timeout", query_timeout.count ()); return toml.get_error (); @@ -516,10 +503,6 @@ nano::error nano::rep_crawler_config::serialize (nano::tomlconfig & toml) const nano::error nano::rep_crawler_config::deserialize (nano::tomlconfig & toml) { - auto rep_timeout_l = rep_timeout.count (); - toml.get ("rep_timeout", rep_timeout_l); - rep_timeout = std::chrono::milliseconds{ rep_timeout_l }; - auto query_timeout_l = query_timeout.count (); toml.get ("query_timeout", query_timeout_l); query_timeout = std::chrono::milliseconds{ query_timeout_l }; diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index c7dab61a24..6e74cf3c54 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -40,8 +40,7 @@ class rep_crawler_config final nano::error serialize (nano::tomlconfig & toml) const; public: - std::chrono::milliseconds query_timeout{ 1000 * 30 }; - std::chrono::milliseconds rep_timeout{ 1000 * 60 * 5 }; + std::chrono::milliseconds query_timeout{ 1000 * 60 }; }; /** From 3ddfd780e4a479a6b33f374b036bfb11eb8498c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Tue, 5 Mar 2024 13:11:17 +0100 Subject: [PATCH 22/27] Throttle queries to active reps --- nano/node/repcrawler.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index c89f142185..7934370b2a 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -228,6 +228,7 @@ std::vector> nano::rep_crawler::prepar constexpr std::size_t aggressive_count = 160; constexpr std::size_t conservative_max_attempts = 4; constexpr std::size_t aggressive_max_attempts = 8; + constexpr std::chrono::seconds rep_query_interval = std::chrono::seconds{ 60 }; stats.inc (nano::stat::type::rep_crawler, sufficient_weight ? nano::stat::detail::crawl_normal : nano::stat::detail::crawl_aggressive); @@ -236,10 +237,22 @@ std::vector> nano::rep_crawler::prepar auto random_peers = node.network.random_set (required_peer_count, 0, /* include channels with ephemeral remote ports */ true); - // Avoid querying the same peer multiple times when rep crawler is warmed up - auto const max_attempts = sufficient_weight ? conservative_max_attempts : aggressive_max_attempts; - erase_if (random_peers, [this, max_attempts] (std::shared_ptr const & channel) { - return queries.get ().count (channel) >= max_attempts; + auto should_query = [&, this] (std::shared_ptr const & channel) { + if (auto rep = reps.get ().find (channel); rep != reps.get ().end ()) + { + // Throttle queries to active reps + return elapsed (rep->last_request, rep_query_interval); + } + else + { + // Avoid querying the same peer multiple times when rep crawler is warmed up + auto const max_attempts = sufficient_weight ? conservative_max_attempts : aggressive_max_attempts; + return queries.get ().count (channel) < max_attempts; + } + }; + + erase_if (random_peers, [&, this] (std::shared_ptr const & channel) { + return !should_query (channel); }); return { random_peers.begin (), random_peers.end () }; From e7f28c6a92a5d6f4dc5321b995f38c2bce394608 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 7 Mar 2024 13:44:32 +0000 Subject: [PATCH 23/27] Rep crawler test case for having 2 reps inside one node --- nano/core_test/rep_crawler.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/nano/core_test/rep_crawler.cpp b/nano/core_test/rep_crawler.cpp index 85f4ba1811..3bebc0754c 100644 --- a/nano/core_test/rep_crawler.cpp +++ b/nano/core_test/rep_crawler.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -269,4 +270,28 @@ TEST (rep_crawler, ignore_local) auto vote = std::make_shared (nano::dev::genesis_key.pub, nano::dev::genesis_key.prv, 0, 0, std::vector{ nano::dev::genesis->hash () }); node.rep_crawler.force_process (vote, loopback); ASSERT_ALWAYS_EQ (0.5s, node.rep_crawler.representative_count (), 0); +} + +// Test that nodes can track PRs when multiple PRs are inside one node +TEST (rep_crawler, two_reps_one_node) +{ + nano::test::system system; + auto & node1 = *system.add_node (); + auto & node2 = *system.add_node (); + + // create a second PR account + nano::keypair second_rep = nano::test::setup_rep (system, node1, node1.balance (nano::dev::genesis_key.pub) / 10); + ASSERT_EQ (0, node2.rep_crawler.representative_count ()); + + // enable the two PRs in node1 + system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); + system.wallet (0)->insert_adhoc (second_rep.prv); + + ASSERT_TIMELY_EQ (5s, node2.rep_crawler.representative_count (), 2); + auto reps = node2.rep_crawler.representatives (); + ASSERT_EQ (2, reps.size ()); + + // check that the reps are correct + ASSERT_TRUE (nano::dev::genesis_key.pub == reps[0].account || nano::dev::genesis_key.pub == reps[1].account); + ASSERT_TRUE (second_rep.pub == reps[0].account || second_rep.pub == reps[1].account); } \ No newline at end of file From eb55b98ad74aa0b746d598f5f71866797da8d7c6 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 7 Mar 2024 13:58:35 +0000 Subject: [PATCH 24/27] rep_query_interval needs to be faster for dev network --- nano/node/repcrawler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 7934370b2a..a196749ccf 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -228,7 +228,7 @@ std::vector> nano::rep_crawler::prepar constexpr std::size_t aggressive_count = 160; constexpr std::size_t conservative_max_attempts = 4; constexpr std::size_t aggressive_max_attempts = 8; - constexpr std::chrono::seconds rep_query_interval = std::chrono::seconds{ 60 }; + std::chrono::milliseconds rep_query_interval = node.network_params.network.is_dev_network () ? std::chrono::milliseconds{ 500 } : std::chrono::milliseconds{ 60 * 1000 }; stats.inc (nano::stat::type::rep_crawler, sufficient_weight ? nano::stat::detail::crawl_normal : nano::stat::detail::crawl_aggressive); From 62fb8ffbd0ce4c334dd47bc9248d60e6f1a79782 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 7 Mar 2024 14:03:51 +0000 Subject: [PATCH 25/27] Fix rep tracking when there are multiple reps inside one node Do not delete the query when a confirm_ack is received with the right vote. We might get more replies and we have no way of knowing how many will come. Just let the query timeout. --- nano/node/repcrawler.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index a196749ccf..9898bfdc7e 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -401,7 +401,6 @@ bool nano::rep_crawler::process (std::shared_ptr const & vote, std:: // TODO: Track query response time responses.push_back ({ channel, vote }); - queries.erase (it); condition.notify_all (); return true; // Found and processed From 4f2f66bbe2356ffc84da779106b879f28186f94e Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 7 Mar 2024 15:14:52 +0000 Subject: [PATCH 26/27] Do not complain when a query timeout, if replies were received --- nano/lib/stats_enums.hpp | 1 + nano/node/repcrawler.cpp | 16 +++++++++++++--- nano/node/repcrawler.hpp | 1 + 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/nano/lib/stats_enums.hpp b/nano/lib/stats_enums.hpp index edad231fd5..a6bf2a5d4d 100644 --- a/nano/lib/stats_enums.hpp +++ b/nano/lib/stats_enums.hpp @@ -337,6 +337,7 @@ enum class detail : uint8_t query_duplicate, rep_timeout, query_timeout, + query_completion, crawl_aggressive, crawl_normal, diff --git a/nano/node/repcrawler.cpp b/nano/node/repcrawler.cpp index 9898bfdc7e..ebe3e88104 100644 --- a/nano/node/repcrawler.cpp +++ b/nano/node/repcrawler.cpp @@ -211,8 +211,16 @@ void nano::rep_crawler::cleanup () erase_if (queries, [this] (query_entry const & query) { if (nano::elapsed (query.time, config.query_timeout)) { - logger.debug (nano::log::type::rep_crawler, "Aborting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); - stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_timeout); + if (query.replies == 0) + { + logger.debug (nano::log::type::rep_crawler, "Aborting unresponsive query for block {} from {}", query.hash.to_string (), query.channel->to_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_timeout); + } + else + { + logger.debug (nano::log::type::rep_crawler, "Completion of query with {} replies for block {} from {}", query.replies, query.hash.to_string (), query.channel->to_string ()); + stats.inc (nano::stat::type::rep_crawler, nano::stat::detail::query_completion); + } return true; // Erase } return false; @@ -401,7 +409,9 @@ bool nano::rep_crawler::process (std::shared_ptr const & vote, std:: // TODO: Track query response time responses.push_back ({ channel, vote }); - + queries.modify (it, [] (query_entry & e) { + e.replies++; + }); condition.notify_all (); return true; // Found and processed } diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index 6e74cf3c54..e850b8f68a 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -141,6 +141,7 @@ class rep_crawler final nano::block_hash hash; std::shared_ptr channel; std::chrono::steady_clock::time_point time{ std::chrono::steady_clock::now () }; + unsigned int replies{ 0 }; // number of replies to the query }; // clang-format off From 49a6b4119be34b6f9fa4e2922522650173d35840 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Fri, 8 Mar 2024 15:52:09 +0000 Subject: [PATCH 27/27] Fix comment of rep_crawler::process function --- nano/node/repcrawler.hpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/nano/node/repcrawler.hpp b/nano/node/repcrawler.hpp index e850b8f68a..0855a7bdee 100644 --- a/nano/node/repcrawler.hpp +++ b/nano/node/repcrawler.hpp @@ -57,10 +57,8 @@ class rep_crawler final void stop (); /** - * Called when a non-replay vote on a block previously sent by query() is received. This indicates - * with high probability that the endpoint is a representative node. - * The force flag can be set to skip the active check in unit testing when we want to force a vote in the rep crawler. - * @return false if any vote passed the checks and was added to the response queue of the rep crawler + * Called when a non-replay vote arrives that might be of interest to rep crawler. + * @return true, if the vote was of interest and was processed, this indicates that the rep is likely online and voting */ bool process (std::shared_ptr const &, std::shared_ptr const &);