Merge pull request #4690 from pwojcikdev/fix-upnp

Fixes for `port_mapping`

nano/core_test/node.cpp

@@ -7,6 +7,7 @@
 #include <nano/node/inactive_node.hpp>
 #include <nano/node/local_vote_history.hpp>
 #include <nano/node/make_store.hpp>
+#include <nano/node/portmapping.hpp>
 #include <nano/node/scheduler/component.hpp>
 #include <nano/node/scheduler/manual.hpp>
 #include <nano/node/scheduler/priority.hpp>

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);

nano/node/network.cpp

@@ -1,16 +1,14 @@
-#include "message_processor.hpp"
-
 #include <nano/crypto_lib/random_pool_shuffle.hpp>
 #include <nano/lib/blocks.hpp>
 #include <nano/lib/threading.hpp>
 #include <nano/lib/utility.hpp>
 #include <nano/node/bootstrap_ascending/service.hpp>
+#include <nano/node/message_processor.hpp>
 #include <nano/node/network.hpp>
 #include <nano/node/node.hpp>
+#include <nano/node/portmapping.hpp>
 #include <nano/node/telemetry.hpp>
 
-#include <boost/format.hpp>
-
 using namespace std::chrono_literals;
 
 // TODO: Return to static const and remove "disable_large_votes" when rolled out

nano/node/node.cpp

@@ -16,6 +16,7 @@
 #include <nano/node/monitor.hpp>
 #include <nano/node/node.hpp>
 #include <nano/node/peer_history.hpp>
+#include <nano/node/portmapping.hpp>
 #include <nano/node/request_aggregator.hpp>
 #include <nano/node/scheduler/component.hpp>
 #include <nano/node/scheduler/hinted.hpp>
@@ -187,7 +188,8 @@ nano::node::node (std::shared_ptr<boost::asio::io_context> io_ctx_a, std::filesy
 	tcp_listener_impl{ std::make_unique<nano::transport::tcp_listener> (network.port, config.tcp, *this) },
 	tcp_listener{ *tcp_listener_impl },
 	application_path (application_path_a),
-	port_mapping (*this),
+	port_mapping_impl{ std::make_unique<nano::port_mapping> (*this) },
+	port_mapping{ *port_mapping_impl },
 	block_processor (*this),
 	confirming_set_impl{ std::make_unique<nano::confirming_set> (config.confirming_set, ledger, stats) },
 	confirming_set{ *confirming_set_impl },

nano/node/node.hpp

@@ -19,7 +19,6 @@
 #include <nano/node/node_observers.hpp>
 #include <nano/node/nodeconfig.hpp>
 #include <nano/node/online_reps.hpp>
-#include <nano/node/portmapping.hpp>
 #include <nano/node/process_live_dispatcher.hpp>
 #include <nano/node/rep_tiers.hpp>
 #include <nano/node/repcrawler.hpp>
@@ -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<node>
 	nano::transport::tcp_listener & tcp_listener;
 	std::filesystem::path application_path;
 	nano::node_observers observers;
-	nano::port_mapping port_mapping;
+	std::unique_ptr<nano::port_mapping> port_mapping_impl;
+	nano::port_mapping & port_mapping;
 	nano::block_processor block_processor;
 	std::unique_ptr<nano::confirming_set> confirming_set_impl;
 	nano::confirming_set & confirming_set;

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<unsigned> ("backlog_scan_batch_size", backlog_scan_batch_size);
 		toml.get<unsigned> ("backlog_scan_frequency", backlog_scan_frequency);
 
+		toml.get<bool> ("enable_upnp", enable_upnp);
+
 		if (toml.has_key ("experimental"))
 		{
 			auto experimental_config_l (toml.get_required_child ("experimental"));

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;

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<nano::mutex> 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<uint16_t> (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 ();
 }
 
 /*

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