From 0ffa688ecfcb49769a097560a4dfc1b32c558287 Mon Sep 17 00:00:00 2001 From: clemahieu Date: Tue, 16 Mar 2021 08:02:14 +0000 Subject: [PATCH] This decreases the confirm_req_batches_max from 20 to 2, equivalent to 280/sec -> 28/sec. This option is also made configurable. (#3148) Confirm_req batches are used in the bootstrap process and excessive requests to reps can degrade performance and often get dropped. --- nano/core_test/confirmation_solicitor.cpp | 7 +++---- nano/core_test/toml.cpp | 19 ++++++++++++++++++- nano/node/active_transactions.cpp | 2 +- nano/node/confirmation_solicitor.cpp | 11 ++++++----- nano/node/confirmation_solicitor.hpp | 6 +++--- nano/node/nodeconfig.cpp | 6 ++++++ nano/node/nodeconfig.hpp | 2 ++ 7 files changed, 39 insertions(+), 14 deletions(-) diff --git a/nano/core_test/confirmation_solicitor.cpp b/nano/core_test/confirmation_solicitor.cpp index 8a27ecf8f..1c384547d 100644 --- a/nano/core_test/confirmation_solicitor.cpp +++ b/nano/core_test/confirmation_solicitor.cpp @@ -21,7 +21,7 @@ TEST (confirmation_solicitor, batches) // Solicitor will only solicit from this representative nano::representative representative (nano::dev_genesis_key.pub, nano::genesis_amount, channel1); std::vector representatives{ representative }; - nano::confirmation_solicitor solicitor (node2.network, node2.network_params.network); + nano::confirmation_solicitor solicitor (node2.network, node2.config); solicitor.prepare (representatives); // Ensure the representatives are correct ASSERT_EQ (1, representatives.size ()); @@ -37,7 +37,6 @@ TEST (confirmation_solicitor, batches) auto election (std::make_shared (node2, send, nullptr, nullptr, false, nano::election_behavior::normal)); ASSERT_FALSE (solicitor.add (*election)); } - ASSERT_EQ (1, solicitor.max_confirm_req_batches); // Reached the maximum amount of requests for the channel auto election (std::make_shared (node2, send, nullptr, nullptr, false, nano::election_behavior::normal)); ASSERT_TRUE (solicitor.add (*election)); @@ -66,7 +65,7 @@ TEST (confirmation_solicitor, different_hash) // Solicitor will only solicit from this representative nano::representative representative (nano::dev_genesis_key.pub, nano::genesis_amount, channel1); std::vector representatives{ representative }; - nano::confirmation_solicitor solicitor (node2.network, node2.network_params.network); + nano::confirmation_solicitor solicitor (node2.network, node2.config); solicitor.prepare (representatives); // Ensure the representatives are correct ASSERT_EQ (1, representatives.size ()); @@ -96,7 +95,7 @@ TEST (confirmation_solicitor, bypass_max_requests_cap) node_flags.disable_udp = false; auto & node1 = *system.add_node (node_flags); auto & node2 = *system.add_node (node_flags); - nano::confirmation_solicitor solicitor (node2.network, node2.network_params.network); + nano::confirmation_solicitor solicitor (node2.network, node2.config); std::vector representatives; auto max_representatives = std::max (solicitor.max_election_requests, solicitor.max_election_broadcasts); representatives.reserve (max_representatives + 1); diff --git a/nano/core_test/toml.cpp b/nano/core_test/toml.cpp index 136b71cd1..eaac8e674 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -186,6 +186,7 @@ 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); @@ -371,7 +372,7 @@ TEST (toml, array) config_node.push ("items", "item 1"); config_node.push ("items", "item 2"); int i = 1; - config_node.array_entries_required ("items", [&i](std::string item) { + config_node.array_entries_required ("items", [&i] (std::string item) { ASSERT_TRUE (item == std::string ("item ") + std::to_string (i)); i++; }); @@ -589,6 +590,7 @@ 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); @@ -821,6 +823,21 @@ 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"); + } } TEST (toml, daemon_read_config) diff --git a/nano/node/active_transactions.cpp b/nano/node/active_transactions.cpp index d2b9ccf66..ad7b2d214 100644 --- a/nano/node/active_transactions.cpp +++ b/nano/node/active_transactions.cpp @@ -289,7 +289,7 @@ void nano::active_transactions::request_confirm (nano::unique_lock lock_a.unlock (); - nano::confirmation_solicitor solicitor (node.network, node.network_params.network); + nano::confirmation_solicitor solicitor (node.network, node.config); solicitor.prepare (node.rep_crawler.principal_representatives (std::numeric_limits::max ())); nano::vote_generator_session generator_session (generator); diff --git a/nano/node/confirmation_solicitor.cpp b/nano/node/confirmation_solicitor.cpp index 0dc0bd641..79e97a41f 100644 --- a/nano/node/confirmation_solicitor.cpp +++ b/nano/node/confirmation_solicitor.cpp @@ -1,14 +1,15 @@ #include #include +#include using namespace std::chrono_literals; -nano::confirmation_solicitor::confirmation_solicitor (nano::network & network_a, nano::network_constants const & params_a) : -max_confirm_req_batches (params_a.is_dev_network () ? 1 : 20), -max_block_broadcasts (params_a.is_dev_network () ? 4 : 30), +nano::confirmation_solicitor::confirmation_solicitor (nano::network & network_a, nano::node_config const & config_a) : +max_block_broadcasts (config_a.network_params.network.is_dev_network () ? 4 : 30), max_election_requests (50), max_election_broadcasts (std::max (network_a.fanout () / 2, 1)), -network (network_a) +network (network_a), +config (config_a) { } @@ -56,7 +57,7 @@ bool nano::confirmation_solicitor::add (nano::election const & election_a) debug_assert (prepared); bool error (true); unsigned count = 0; - auto const max_channel_requests (max_confirm_req_batches * nano::network::confirm_req_hashes_max); + 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;) { diff --git a/nano/node/confirmation_solicitor.hpp b/nano/node/confirmation_solicitor.hpp index ef04cca67..ed699302f 100644 --- a/nano/node/confirmation_solicitor.hpp +++ b/nano/node/confirmation_solicitor.hpp @@ -9,11 +9,12 @@ namespace nano { class election; class node; +class node_config; /** This class accepts elections that need further votes before they can be confirmed and bundles them in to single confirm_req packets */ class confirmation_solicitor final { public: - confirmation_solicitor (nano::network &, nano::network_constants const &); + confirmation_solicitor (nano::network &, nano::node_config const &); /** Prepare object for batching election confirmation requests*/ void prepare (std::vector const &); /** Broadcast the winner of an election if the broadcast limit has not been reached. Returns false if the broadcast was performed */ @@ -22,8 +23,6 @@ public: bool add (nano::election const &); /** Dispatch bundled requests to each channel*/ void flush (); - /** Maximum amount of confirmation requests (batches) to be sent to each channel */ - size_t const max_confirm_req_batches; /** Global maximum amount of block broadcasts */ size_t const max_block_broadcasts; /** Maximum amount of requests to be sent per election, bypassed if an existing vote is for a different hash*/ @@ -33,6 +32,7 @@ public: private: nano::network & network; + nano::node_config const & config; unsigned rebroadcasted{ 0 }; std::vector representatives_requests; diff --git a/nano/node/nodeconfig.cpp b/nano/node/nodeconfig.cpp index 1d40e840c..7d9509a1e 100644 --- a/nano/node/nodeconfig.cpp +++ b/nano/node/nodeconfig.cpp @@ -110,6 +110,7 @@ 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"); auto work_peers_l (toml.create_array ("work_peers", "A list of \"address:port\" entries to identify work peers.")); for (auto i (work_peers.begin ()), n (work_peers.end ()); i != n; ++i) @@ -372,6 +373,7 @@ 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); if (toml.has_key ("frontiers_confirmation")) { @@ -441,6 +443,10 @@ 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"); + } } catch (std::runtime_error const & ex) { diff --git a/nano/node/nodeconfig.hpp b/nano/node/nodeconfig.hpp index 5314a00cf..03237303f 100644 --- a/nano/node/nodeconfig.hpp +++ b/nano/node/nodeconfig.hpp @@ -99,6 +99,8 @@ public: std::chrono::seconds work_watcher_period{ std::chrono::seconds (5) }; 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;