From a8d8d2899e298024ed30fc0126a793ce267dbc2c Mon Sep 17 00:00:00 2001 From: clemahieu Date: Mon, 24 Apr 2023 21:02:01 +0100 Subject: [PATCH] Remove live updating bandwidth limits (#4219) * Rewrite and separate bandwidth limiter tests to avoid using set_bandwidth_params which was identified as problematic by tsan. * Remove live updating of bandwidth parameters as it has race conditions according to TSAN. Live config updating was never fully implemented so removing this until a full solution is implemented. --- nano/core_test/network.cpp | 36 +++++++++++++++++++++++++++++------- nano/nano_node/daemon.cpp | 24 ------------------------ nano/node/node.cpp | 8 -------- nano/node/node.hpp | 1 - 4 files changed, 29 insertions(+), 40 deletions(-) diff --git a/nano/core_test/network.cpp b/nano/core_test/network.cpp index e1c77708..ab653bbc 100644 --- a/nano/core_test/network.cpp +++ b/nano/core_test/network.cpp @@ -897,7 +897,7 @@ TEST (network, duplicate_revert_publish) } // The test must be completed in less than 1 second -TEST (network, bandwidth_limiter) +TEST (network, bandwidth_limiter_4_messages) { nano::test::system system; nano::publish message{ nano::dev::network_params.network, nano::dev::genesis }; @@ -925,24 +925,46 @@ TEST (network, bandwidth_limiter) // Send non-droppable message, i.e. drop stats should not increase channel2.send (message, nullptr, nano::transport::buffer_drop_policy::no_limiter_drop); ASSERT_TIMELY (1s, 1 == node.stats.count (nano::stat::type::drop, nano::stat::detail::publish, nano::stat::dir::out)); +} +TEST (network, bandwidth_limiter_2_messages) +{ + nano::test::system system; + nano::publish message{ nano::dev::network_params.network, nano::dev::genesis }; + auto message_size = message.to_bytes ()->size (); + auto message_limit = 2; // must be multiple of the number of channels + nano::node_config node_config (nano::test::get_available_port (), system.logging); + node_config.bandwidth_limit = message_limit * message_size; + node_config.bandwidth_limit_burst_ratio = 1.0; + auto & node = *system.add_node (node_config); + nano::transport::inproc::channel channel1{ node, node }; + nano::transport::inproc::channel channel2{ node, node }; // change the bandwidth settings, 2 packets will be dropped - node.set_bandwidth_params (message_size * 2, 1.1); channel1.send (message); channel2.send (message); channel1.send (message); channel2.send (message); - ASSERT_TIMELY (1s, 3 == node.stats.count (nano::stat::type::drop, nano::stat::detail::publish, nano::stat::dir::out)); + ASSERT_TIMELY (1s, 2 == node.stats.count (nano::stat::type::drop, nano::stat::detail::publish, nano::stat::dir::out)); +} +TEST (network, bandwidth_limiter_with_burst) +{ + nano::test::system system; + nano::publish message{ nano::dev::network_params.network, nano::dev::genesis }; + auto message_size = message.to_bytes ()->size (); + auto message_limit = 2; // must be multiple of the number of channels + nano::node_config node_config (nano::test::get_available_port (), system.logging); + node_config.bandwidth_limit = message_limit * message_size; + node_config.bandwidth_limit_burst_ratio = 4.0; // High burst + auto & node = *system.add_node (node_config); + nano::transport::inproc::channel channel1{ node, node }; + nano::transport::inproc::channel channel2{ node, node }; // change the bandwidth settings, no packet will be dropped - node.set_bandwidth_params (message_size, 4); channel1.send (message); channel2.send (message); channel1.send (message); channel2.send (message); - ASSERT_TIMELY (1s, 3 == node.stats.count (nano::stat::type::drop, nano::stat::detail::publish, nano::stat::dir::out)); - - node.stop (); + ASSERT_TIMELY (1s, 0 == node.stats.count (nano::stat::type::drop, nano::stat::detail::publish, nano::stat::dir::out)); } namespace nano diff --git a/nano/nano_node/daemon.cpp b/nano/nano_node/daemon.cpp index 5264a9c0..a8431fc6 100644 --- a/nano/nano_node/daemon.cpp +++ b/nano/nano_node/daemon.cpp @@ -57,21 +57,6 @@ volatile sig_atomic_t sig_int_or_term = 0; constexpr std::size_t OPEN_FILE_DESCRIPTORS_LIMIT = 16384; } -static void load_and_set_bandwidth_params (std::shared_ptr const & node, boost::filesystem::path const & data_path, nano::node_flags const & flags) -{ - nano::daemon_config config{ data_path, node->network_params }; - - auto error = nano::read_node_config_toml (data_path, config, flags.config_overrides); - if (!error) - { - error = nano::flags_config_conflicts (flags, config.node); - if (!error) - { - node->set_bandwidth_params (config.node.bandwidth_limit, config.node.bandwidth_limit_burst_ratio); - } - } -} - void nano_daemon::daemon::run (boost::filesystem::path const & data_path, nano::node_flags const & flags) { install_abort_signal_handler (); @@ -212,15 +197,6 @@ void nano_daemon::daemon::run (boost::filesystem::path const & data_path, nano:: // sigterm is less likely to come in bunches so only trap it once sigman.register_signal_handler (SIGTERM, &nano::signal_handler, false); -#ifndef _WIN32 - // on sighup we should reload the bandwidth parameters - std::function sighup_signal_handler ([&node, &data_path, &flags] (int signum) { - debug_assert (signum == SIGHUP); - load_and_set_bandwidth_params (node, data_path, flags); - }); - sigman.register_signal_handler (SIGHUP, sighup_signal_handler, true); -#endif - runner = std::make_unique (io_ctx, node->config.io_threads); runner->join (); diff --git a/nano/node/node.cpp b/nano/node/node.cpp index cb519f63..fab425ac 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -1438,14 +1438,6 @@ bool nano::node::init_error () const return store.init_error () || wallets_store.init_error (); } -void nano::node::set_bandwidth_params (std::size_t limit, double ratio) -{ - config.bandwidth_limit_burst_ratio = ratio; - config.bandwidth_limit = limit; - outbound_limiter.reset (limit, ratio); - logger.always_log (boost::str (boost::format ("set_bandwidth_params(%1%, %2%)") % limit % ratio)); -} - std::pair nano::node::get_bootstrap_weights () const { std::unordered_map weights; diff --git a/nano/node/node.hpp b/nano/node/node.hpp index 53f6aef1..c8da025c 100644 --- a/nano/node/node.hpp +++ b/nano/node/node.hpp @@ -132,7 +132,6 @@ public: void ongoing_online_weight_calculation_queue (); bool online () const; bool init_error () const; - void set_bandwidth_params (std::size_t limit, double ratio); std::pair get_bootstrap_weights () const; uint64_t get_confirmation_height (nano::transaction const &, nano::account &); /*