From 0d4fee76553e270c4823d54eed02105ca8a1b77a Mon Sep 17 00:00:00 2001 From: clemahieu Date: Sun, 11 Aug 2024 19:05:39 +0100 Subject: [PATCH] Merge pull request #4690 from pwojcikdev/fix-upnp Fixes for `port_mapping` --- nano/core_test/node.cpp | 1 + nano/core_test/toml.cpp | 3 +++ nano/node/network.cpp | 6 ++---- nano/node/node.cpp | 4 +++- nano/node/node.hpp | 5 +++-- nano/node/nodeconfig.cpp | 3 +++ nano/node/nodeconfig.hpp | 1 + nano/node/portmapping.cpp | 29 ++++++++++++++++++++++++----- nano/node/portmapping.hpp | 2 ++ 9 files changed, 42 insertions(+), 12 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index bc579fa10f..34af749f6b 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include diff --git a/nano/core_test/toml.cpp b/nano/core_test/toml.cpp index ddf4505a72..ce861da1d8 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -199,6 +199,7 @@ TEST (toml, daemon_config_deserialize_defaults) ASSERT_EQ (conf.node.max_unchecked_blocks, defaults.node.max_unchecked_blocks); ASSERT_EQ (conf.node.backlog_scan_batch_size, defaults.node.backlog_scan_batch_size); ASSERT_EQ (conf.node.backlog_scan_frequency, defaults.node.backlog_scan_frequency); + ASSERT_EQ (conf.node.enable_upnp, defaults.node.enable_upnp); ASSERT_EQ (conf.node.websocket_config.enabled, defaults.node.websocket_config.enabled); ASSERT_EQ (conf.node.websocket_config.address, defaults.node.websocket_config.address); @@ -463,6 +464,7 @@ TEST (toml, daemon_config_deserialize_no_defaults) frontiers_confirmation = "always" backlog_scan_batch_size = 999 backlog_scan_frequency = 999 + enable_upnp = false [node.block_processor] max_peer_queue = 999 @@ -700,6 +702,7 @@ TEST (toml, daemon_config_deserialize_no_defaults) ASSERT_NE (conf.node.request_aggregator_threads, defaults.node.request_aggregator_threads); ASSERT_NE (conf.node.backlog_scan_batch_size, defaults.node.backlog_scan_batch_size); ASSERT_NE (conf.node.backlog_scan_frequency, defaults.node.backlog_scan_frequency); + ASSERT_NE (conf.node.enable_upnp, defaults.node.enable_upnp); ASSERT_NE (conf.node.websocket_config.enabled, defaults.node.websocket_config.enabled); ASSERT_NE (conf.node.websocket_config.address, defaults.node.websocket_config.address); diff --git a/nano/node/network.cpp b/nano/node/network.cpp index 92443870df..3d15149c37 100644 --- a/nano/node/network.cpp +++ b/nano/node/network.cpp @@ -1,16 +1,14 @@ -#include "message_processor.hpp" - #include #include #include #include #include +#include #include #include +#include #include -#include - using namespace std::chrono_literals; // TODO: Return to static const and remove "disable_large_votes" when rolled out diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 3330b467b2..fba2567204 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -187,7 +188,8 @@ nano::node::node (std::shared_ptr io_ctx_a, std::filesy tcp_listener_impl{ std::make_unique (network.port, config.tcp, *this) }, tcp_listener{ *tcp_listener_impl }, application_path (application_path_a), - port_mapping (*this), + port_mapping_impl{ std::make_unique (*this) }, + port_mapping{ *port_mapping_impl }, block_processor (*this), confirming_set_impl{ std::make_unique (config.confirming_set, ledger, stats) }, confirming_set{ *confirming_set_impl }, diff --git a/nano/node/node.hpp b/nano/node/node.hpp index 020910bf1f..c24ca6a290 100644 --- a/nano/node/node.hpp +++ b/nano/node/node.hpp @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include @@ -51,6 +50,7 @@ class vote_cache_processor; class vote_router; class work_pool; class peer_history; +class port_mapping; class thread_runner; namespace scheduler @@ -181,7 +181,8 @@ class node final : public std::enable_shared_from_this nano::transport::tcp_listener & tcp_listener; std::filesystem::path application_path; nano::node_observers observers; - nano::port_mapping port_mapping; + std::unique_ptr port_mapping_impl; + nano::port_mapping & port_mapping; nano::block_processor block_processor; std::unique_ptr confirming_set_impl; nano::confirming_set & confirming_set; diff --git a/nano/node/nodeconfig.cpp b/nano/node/nodeconfig.cpp index 70e2e33409..9449df5ca9 100644 --- a/nano/node/nodeconfig.cpp +++ b/nano/node/nodeconfig.cpp @@ -145,6 +145,7 @@ nano::error nano::node_config::serialize_toml (nano::tomlconfig & toml) const 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"); toml.put ("backlog_scan_batch_size", backlog_scan_batch_size, "Number of accounts per second to process when doing backlog population scan. Increasing this value will help unconfirmed frontiers get into election prioritization queue faster, however it will also increase resource usage. \ntype:uint"); toml.put ("backlog_scan_frequency", backlog_scan_frequency, "Backlog scan divides the scan into smaller batches, number of which is controlled by this value. Higher frequency helps to utilize resources more uniformly, however it also introduces more overhead. The resulting number of accounts per single batch is `backlog_scan_batch_size / backlog_scan_frequency` \ntype:uint"); + toml.put ("enable_upnp", enable_upnp, "Enable or disable automatic UPnP port forwarding. This feature only works if the node is directly connected to a router (not inside a docker container, etc.).\ntype:bool"); 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) @@ -555,6 +556,8 @@ nano::error nano::node_config::deserialize_toml (nano::tomlconfig & toml) toml.get ("backlog_scan_batch_size", backlog_scan_batch_size); toml.get ("backlog_scan_frequency", backlog_scan_frequency); + toml.get ("enable_upnp", enable_upnp); + if (toml.has_key ("experimental")) { auto experimental_config_l (toml.get_required_child ("experimental")); diff --git a/nano/node/nodeconfig.hpp b/nano/node/nodeconfig.hpp index be7772dfe3..f811e9624f 100644 --- a/nano/node/nodeconfig.hpp +++ b/nano/node/nodeconfig.hpp @@ -147,6 +147,7 @@ class node_config unsigned backlog_scan_batch_size{ 10 * 1000 }; /** 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 }; + bool enable_upnp{ true }; nano::vote_cache_config vote_cache; nano::rep_crawler_config rep_crawler; nano::block_processor_config block_processor; diff --git a/nano/node/portmapping.cpp b/nano/node/portmapping.cpp index 135c4bbadf..0dd1605bda 100644 --- a/nano/node/portmapping.cpp +++ b/nano/node/portmapping.cpp @@ -36,6 +36,11 @@ void nano::port_mapping::start () { debug_assert (!thread.joinable ()); + if (!node.config.enable_upnp) + { + return; + } + // Long discovery time and fast setup/teardown make this impractical for testing // TODO: Find a way to test this if (node.network_params.network.is_dev_network ()) @@ -61,14 +66,22 @@ void nano::port_mapping::stop () { thread.join (); } +} + +void nano::port_mapping::shutdown () +{ + node.logger.debug (nano::log::type::upnp, "UPnP shutdown..."); nano::lock_guard guard_l (mutex); for (auto & protocol : protocols | boost::adaptors::filtered ([] (auto const & p) { return p.enabled; })) { if (protocol.external_port != 0) { + std::string external_port_str = std::to_string (protocol.external_port); + std::string address_str = address.to_string (); + // Be a good citizen for the router and shut down our mapping - auto delete_error_l (UPNP_DeletePortMapping (upnp.urls.controlURL, upnp.data.first.servicetype, std::to_string (protocol.external_port).c_str (), protocol.name, address.to_string ().c_str ())); + auto delete_error_l = UPNP_DeletePortMapping (upnp.urls.controlURL, upnp.data.first.servicetype, external_port_str.c_str (), protocol.name, address_str.c_str ()); if (delete_error_l) { node.logger.warn (nano::log::type::upnp, "UPnP shutdown {} port mapping failed: {} ({})", @@ -117,7 +130,7 @@ void nano::port_mapping::refresh_devices () // Bump logging level periodically node.logger.log ((check_count % 15 == 0) ? nano::log::level::info : nano::log::level::debug, - nano::log::type::upnp, "UPnP local address {}, discovery: {}, IGD search: {}", + nano::log::type::upnp, "UPnP local address: {}, discovery: {}, IGD search: {}", local_address_l.data (), discover_error_l, igd_error_l); @@ -167,8 +180,10 @@ void nano::port_mapping::refresh_mapping () for (auto & protocol : protocols | boost::adaptors::filtered ([] (auto const & p) { return p.enabled; })) { auto upnp_description = std::string ("Nano Node (") + node.network_params.network.get_current_network_as_string () + ")"; - auto add_port_mapping_error_l (UPNP_AddPortMapping (upnp.urls.controlURL, upnp.data.first.servicetype, config_port_l.c_str (), node_port_l.c_str (), address.to_string ().c_str (), upnp_description.c_str (), protocol.name, nullptr, std::to_string (node.network_params.portmapping.lease_duration.count ()).c_str ())); + std::string address_str = address.to_string (); + std::string lease_duration_str = std::to_string (node.network_params.portmapping.lease_duration.count ()); + auto add_port_mapping_error_l = UPNP_AddPortMapping (upnp.urls.controlURL, upnp.data.first.servicetype, config_port_l.c_str (), node_port_l.c_str (), address_str.c_str (), upnp_description.c_str (), protocol.name, nullptr, lease_duration_str.c_str ()); if (add_port_mapping_error_l == UPNPCOMMAND_SUCCESS) { protocol.external_port = static_cast (std::atoi (config_port_l.data ())); @@ -272,14 +287,14 @@ void nano::port_mapping::check_mapping () } else { - node.logger.info (nano::log::type::upnp, "UPnP No need to refresh the mapping"); + node.logger.info (nano::log::type::upnp, "UPnP no need to refresh the mapping"); } } else { // Bump logging level periodically node.logger.log ((check_count % 15 == 0) ? nano::log::level::info : nano::log::level::debug, - nano::log::type::upnp, "UPnP No IGD devices found"); + nano::log::type::upnp, "UPnP no IGD devices found"); } ++check_count; @@ -298,6 +313,10 @@ void nano::port_mapping::run () condition.wait_for (lock, node.network_params.portmapping.health_check_period, [this] { return stopped.load (); }); } + + lock.unlock (); + + shutdown (); } /* diff --git a/nano/node/portmapping.hpp b/nano/node/portmapping.hpp index 6e47dd1395..cc542896ad 100644 --- a/nano/node/portmapping.hpp +++ b/nano/node/portmapping.hpp @@ -56,12 +56,14 @@ class port_mapping private: void run (); + void shutdown (); /** Add port mappings for the node port (not RPC). Refresh when the lease ends. */ void refresh_mapping (); /** Check occasionally to refresh in case router loses mapping */ void check_mapping (); /** Returns false if mapping still exists */ bool check_lost_or_old_mapping (); + std::string get_config_port (std::string const &); private: // Dependencies