From 1de21328589d4b265794a00856929e7cab80e7a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 3 Aug 2024 13:35:59 +0200 Subject: [PATCH 1/5] Fix c_str lifetimes --- nano/node/portmapping.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/nano/node/portmapping.cpp b/nano/node/portmapping.cpp index 135c4bbadf..59838f32fa 100644 --- a/nano/node/portmapping.cpp +++ b/nano/node/portmapping.cpp @@ -67,8 +67,11 @@ void nano::port_mapping::stop () { 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: {} ({})", @@ -167,8 +170,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 ())); From deecc96168e0ccfa8ed6187625b1331251d17e37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 3 Aug 2024 13:38:52 +0200 Subject: [PATCH 2/5] Logging --- nano/node/portmapping.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nano/node/portmapping.cpp b/nano/node/portmapping.cpp index 59838f32fa..fb6976b2f7 100644 --- a/nano/node/portmapping.cpp +++ b/nano/node/portmapping.cpp @@ -120,7 +120,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); @@ -277,14 +277,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; From b123e90f05eff33e6b6d48010ce612b588732b34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 3 Aug 2024 13:45:38 +0200 Subject: [PATCH 3/5] Use unique ptr --- nano/core_test/node.cpp | 1 + nano/node/network.cpp | 6 ++---- nano/node/node.cpp | 4 +++- nano/node/node.hpp | 5 +++-- 4 files changed, 9 insertions(+), 7 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/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 f296ae5cca..0561143edf 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; From 039903c82d4288383717fd5f74f3bd46efd75c98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 3 Aug 2024 13:51:17 +0200 Subject: [PATCH 4/5] Config to enable upnp port mapping --- nano/core_test/toml.cpp | 3 +++ nano/node/nodeconfig.cpp | 3 +++ nano/node/nodeconfig.hpp | 1 + nano/node/portmapping.cpp | 5 +++++ 4 files changed, 12 insertions(+) diff --git a/nano/core_test/toml.cpp b/nano/core_test/toml.cpp index 343ac1ec33..073c0ac32c 100644 --- a/nano/core_test/toml.cpp +++ b/nano/core_test/toml.cpp @@ -198,6 +198,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); @@ -449,6 +450,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 @@ -672,6 +674,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/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 fb6976b2f7..b70b8edd9c 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 ()) From 5f3337b53d61e8f4860af9b9d419ba3ceb5a22e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 3 Aug 2024 13:54:56 +0200 Subject: [PATCH 5/5] Execute shutdown on port mapping thread --- nano/node/portmapping.cpp | 9 +++++++++ nano/node/portmapping.hpp | 2 ++ 2 files changed, 11 insertions(+) diff --git a/nano/node/portmapping.cpp b/nano/node/portmapping.cpp index b70b8edd9c..0dd1605bda 100644 --- a/nano/node/portmapping.cpp +++ b/nano/node/portmapping.cpp @@ -66,6 +66,11 @@ 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; })) @@ -308,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