From d25d943ed531459a69184c11b4c7f5cdd386d7ca Mon Sep 17 00:00:00 2001 From: clemahieu Date: Tue, 19 Apr 2022 17:57:19 +0100 Subject: [PATCH 1/3] This change limits confirmation requests by the status of the tcp channel rather than by a fixed number. --- nano/core_test/confirmation_solicitor.cpp | 1 - nano/node/confirmation_solicitor.cpp | 3 +-- nano/node/transport/tcp.hpp | 10 ++++++++++ nano/node/transport/transport.hpp | 4 ++++ 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/nano/core_test/confirmation_solicitor.cpp b/nano/core_test/confirmation_solicitor.cpp index fe9db15e91..d9fa2c2e8a 100644 --- a/nano/core_test/confirmation_solicitor.cpp +++ b/nano/core_test/confirmation_solicitor.cpp @@ -39,7 +39,6 @@ TEST (confirmation_solicitor, batches) } // Reached the maximum amount of requests for the channel auto election (std::make_shared (node2, send, nullptr, nullptr, nano::election_behavior::normal)); - ASSERT_TRUE (solicitor.add (*election)); // Broadcasting should be immediate ASSERT_EQ (0, node2.stats.count (nano::stat::type::message, nano::stat::detail::publish, nano::stat::dir::out)); ASSERT_FALSE (solicitor.broadcast (*election)); diff --git a/nano/node/confirmation_solicitor.cpp b/nano/node/confirmation_solicitor.cpp index ee2ba8a7f1..919cc192dc 100644 --- a/nano/node/confirmation_solicitor.cpp +++ b/nano/node/confirmation_solicitor.cpp @@ -57,7 +57,6 @@ bool nano::confirmation_solicitor::add (nano::election const & election_a) debug_assert (prepared); bool error (true); unsigned count = 0; - auto const max_channel_requests (config.confirm_req_batches_max * nano::network::confirm_req_hashes_max); auto const & hash (election_a.status.winner->hash ()); for (auto i (representatives_requests.begin ()); i != representatives_requests.end () && count < max_election_requests;) { @@ -70,7 +69,7 @@ bool nano::confirmation_solicitor::add (nano::election const & election_a) if (!exists || !is_final || different) { auto & request_queue (requests[rep.channel]); - if (request_queue.size () < max_channel_requests) + if (!rep.channel->full ()) { request_queue.emplace_back (election_a.status.winner->hash (), election_a.status.winner->root ()); count += different ? 0 : 1; diff --git a/nano/node/transport/tcp.hpp b/nano/node/transport/tcp.hpp index 5f64d46f69..839ffb80dd 100644 --- a/nano/node/transport/tcp.hpp +++ b/nano/node/transport/tcp.hpp @@ -68,6 +68,16 @@ namespace transport return nano::transport::transport_type::tcp; } + virtual bool full () override + { + bool result = true; + if (auto socket_l = socket.lock ()) + { + result = socket_l->max (); + } + return result; + } + private: nano::tcp_endpoint endpoint{ boost::asio::ip::address_v6::any (), 0 }; }; diff --git a/nano/node/transport/transport.hpp b/nano/node/transport/transport.hpp index 8c413c65f6..631323c377 100644 --- a/nano/node/transport/transport.hpp +++ b/nano/node/transport/transport.hpp @@ -58,6 +58,10 @@ namespace transport virtual nano::endpoint get_endpoint () const = 0; virtual nano::tcp_endpoint get_tcp_endpoint () const = 0; virtual nano::transport::transport_type get_type () const = 0; + virtual bool full () + { + return false; + } std::chrono::steady_clock::time_point get_last_bootstrap_attempt () const { From 9e39e23f75c386047b0544dcde02e07513f0d912 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Wed, 20 Apr 2022 13:07:19 +0100 Subject: [PATCH 2/3] Removing confirm_req_batches_max as it's unused. --- nano/core_test/toml.cpp | 17 ----------------- nano/node/nodeconfig.cpp | 6 ------ nano/node/nodeconfig.hpp | 2 -- 3 files changed, 25 deletions(-) diff --git a/nano/core_test/toml.cpp b/nano/core_test/toml.cpp index 8025264adb..2a1216f59f 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -187,7 +187,6 @@ TEST (toml, daemon_config_deserialize_defaults) ASSERT_EQ (conf.node.work_peers, defaults.node.work_peers); ASSERT_EQ (conf.node.work_threads, defaults.node.work_threads); ASSERT_EQ (conf.node.max_queued_requests, defaults.node.max_queued_requests); - ASSERT_EQ (conf.node.confirm_req_batches_max, defaults.node.confirm_req_batches_max); ASSERT_EQ (conf.node.logging.bulk_pull_logging_value, defaults.node.logging.bulk_pull_logging_value); ASSERT_EQ (conf.node.logging.flush, defaults.node.logging.flush); @@ -594,7 +593,6 @@ TEST (toml, daemon_config_deserialize_no_defaults) ASSERT_NE (conf.node.work_peers, defaults.node.work_peers); ASSERT_NE (conf.node.work_threads, defaults.node.work_threads); ASSERT_NE (conf.node.max_queued_requests, defaults.node.max_queued_requests); - ASSERT_EQ (conf.node.confirm_req_batches_max, defaults.node.confirm_req_batches_max); ASSERT_NE (conf.node.logging.bulk_pull_logging_value, defaults.node.logging.bulk_pull_logging_value); ASSERT_NE (conf.node.logging.flush, defaults.node.logging.flush); @@ -830,21 +828,6 @@ TEST (toml, daemon_config_deserialize_errors) ASSERT_EQ (toml.get_error ().get_message (), "election_hint_weight_percent must be a number between 5 and 50"); } - { - std::stringstream ss; - ss << R"toml( - [node] - confirm_req_batches_max = 0 - )toml"; - - nano::tomlconfig toml; - toml.read (ss); - nano::daemon_config conf; - conf.deserialize_toml (toml); - - ASSERT_EQ (toml.get_error ().get_message (), "confirm_req_batches_max must be between 1 and 100"); - } - { std::stringstream ss; ss << R"toml( diff --git a/nano/node/nodeconfig.cpp b/nano/node/nodeconfig.cpp index 3a49b6879a..9488c0504c 100644 --- a/nano/node/nodeconfig.cpp +++ b/nano/node/nodeconfig.cpp @@ -121,7 +121,6 @@ nano::error nano::node_config::serialize_toml (nano::tomlconfig & toml) const toml.put ("max_work_generate_multiplier", max_work_generate_multiplier, "Maximum allowed difficulty multiplier for work generation.\ntype:double,[1..]"); toml.put ("frontiers_confirmation", serialize_frontiers_confirmation (frontiers_confirmation), "Mode controlling frontier confirmation rate.\ntype:string,{auto,always,disabled}"); toml.put ("max_queued_requests", max_queued_requests, "Limit for number of queued confirmation requests for one channel, after which new requests are dropped until the queue drops below this value.\ntype:uint32"); - toml.put ("confirm_req_batches_max", confirm_req_batches_max, "Limit for the number of confirmation requests for one channel per request attempt\ntype:uint32"); toml.put ("rep_crawler_weight_minimum", rep_crawler_weight_minimum.to_string_dec (), "Rep crawler minimum weight, if this is less than minimum principal weight then this is taken as the minimum weight a rep must have to be tracked. If you want to track all reps set this to 0. If you do not want this to influence anything then set it to max value. This is only useful for debugging or for people who really know what they are doing.\ntype:string,amount,raw"); auto work_peers_l (toml.create_array ("work_peers", "A list of \"address:port\" entries to identify work peers.")); @@ -370,7 +369,6 @@ nano::error nano::node_config::deserialize_toml (nano::tomlconfig & toml) toml.get ("max_work_generate_multiplier", max_work_generate_multiplier); toml.get ("max_queued_requests", max_queued_requests); - toml.get ("confirm_req_batches_max", confirm_req_batches_max); auto rep_crawler_weight_minimum_l (rep_crawler_weight_minimum.to_string_dec ()); if (toml.has_key ("rep_crawler_weight_minimum")) @@ -445,10 +443,6 @@ nano::error nano::node_config::deserialize_toml (nano::tomlconfig & toml) { toml.get_error ().set ("max_pruning_age must be greater than or equal to 5 minutes"); } - if (confirm_req_batches_max < 1 || confirm_req_batches_max > 100) - { - toml.get_error ().set ("confirm_req_batches_max must be between 1 and 100"); - } if (bootstrap_frontier_request_count < 1024) { toml.get_error ().set ("bootstrap_frontier_request_count must be greater than or equal to 1024"); diff --git a/nano/node/nodeconfig.hpp b/nano/node/nodeconfig.hpp index e83b1d7bdf..b3edf03cf2 100644 --- a/nano/node/nodeconfig.hpp +++ b/nano/node/nodeconfig.hpp @@ -97,8 +97,6 @@ class node_config bool backup_before_upgrade{ false }; double max_work_generate_multiplier{ 64. }; uint32_t max_queued_requests{ 512 }; - /** Maximum amount of confirmation requests (batches) to be sent to each channel */ - uint32_t confirm_req_batches_max{ network_params.network.is_dev_network () ? 1u : 2u }; std::chrono::seconds max_pruning_age{ !network_params.network.is_beta_network () ? std::chrono::seconds (24 * 60 * 60) : std::chrono::seconds (5 * 60) }; // 1 day; 5 minutes for beta network uint64_t max_pruning_depth{ 0 }; nano::rocksdb_config rocksdb_config; From 4fb2b1d7942366dde77ad138078f4839c5dfedb0 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Wed, 20 Apr 2022 18:26:16 +0100 Subject: [PATCH 3/3] Rename channel::full() to channel::max() to match socket::max() channel::full() was calling channel::max() instead of socket::full(), which also exists. So it makes sense for consistency to rename it. --- nano/node/confirmation_solicitor.cpp | 2 +- nano/node/transport/tcp.hpp | 2 +- nano/node/transport/transport.hpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nano/node/confirmation_solicitor.cpp b/nano/node/confirmation_solicitor.cpp index 919cc192dc..54e9f1dce3 100644 --- a/nano/node/confirmation_solicitor.cpp +++ b/nano/node/confirmation_solicitor.cpp @@ -69,7 +69,7 @@ bool nano::confirmation_solicitor::add (nano::election const & election_a) if (!exists || !is_final || different) { auto & request_queue (requests[rep.channel]); - if (!rep.channel->full ()) + if (!rep.channel->max ()) { request_queue.emplace_back (election_a.status.winner->hash (), election_a.status.winner->root ()); count += different ? 0 : 1; diff --git a/nano/node/transport/tcp.hpp b/nano/node/transport/tcp.hpp index 839ffb80dd..aff3104d1c 100644 --- a/nano/node/transport/tcp.hpp +++ b/nano/node/transport/tcp.hpp @@ -68,7 +68,7 @@ namespace transport return nano::transport::transport_type::tcp; } - virtual bool full () override + virtual bool max () override { bool result = true; if (auto socket_l = socket.lock ()) diff --git a/nano/node/transport/transport.hpp b/nano/node/transport/transport.hpp index 631323c377..95c56d2b6d 100644 --- a/nano/node/transport/transport.hpp +++ b/nano/node/transport/transport.hpp @@ -58,7 +58,7 @@ namespace transport virtual nano::endpoint get_endpoint () const = 0; virtual nano::tcp_endpoint get_tcp_endpoint () const = 0; virtual nano::transport::transport_type get_type () const = 0; - virtual bool full () + virtual bool max () { return false; }