Introduce support for binding sockets to ports picked by the OS

nano/node/bootstrap/bootstrap_server.cpp

@@ -25,7 +25,42 @@ void nano::bootstrap_listener::start ()
 		node.logger.always_log (boost::str (boost::format ("Network: Error while binding for incoming TCP/bootstrap on port %1%: %2%") % listening_socket->listening_port () % ec.message ()));
 		throw std::runtime_error (ec.message ());
 	}
-	debug_assert (node.network.endpoint ().port () == listening_socket->listening_port ());
+
+	// the user can either specify a port value in the config or it can leave the choice up to the OS;
+	// independently of user's port choice, he may have also opted to disable UDP or not; this gives us 4 possibilities:
+	// (1): UDP enabled, port specified
+	// (2): UDP enabled, port not specified
+	// (3): UDP disabled, port specified
+	// (4): UDP disabled, port not specified
+	//
+	const auto listening_port = listening_socket->listening_port ();
+	if (!node.flags.disable_udp)
+	{
+		// (1) and (2) -- no matter if (1) or (2), since UDP socket binding happens before this TCP socket binding,
+		// we must have already been constructed with a valid port value, so check that it really is the same everywhere
+		//
+		debug_assert (port == listening_port);
+		debug_assert (port == node.network.port);
+		debug_assert (port == node.network.endpoint ().port ());
+	}
+	else
+	{
+		// (3) -- nothing to do, just check that port values match everywhere
+		//
+		if (port == listening_port)
+		{
+			debug_assert (port == node.network.port);
+			debug_assert (port == node.network.endpoint ().port ());
+		}
+		// (4) -- OS port choice happened at TCP socket bind time, so propagate this port value back
+		//
+		else
+		{
+			port = listening_port;
+			node.network.port = listening_port;
+		}
+	}
+
 	listening_socket->on_connection ([this] (std::shared_ptr<nano::socket> const & new_connection, boost::system::error_code const & ec_a) {
 		if (!ec_a)
 		{
@@ -81,7 +116,7 @@ boost::asio::ip::tcp::endpoint nano::bootstrap_listener::endpoint ()
 	nano::lock_guard<nano::mutex> lock (mutex);
 	if (on && listening_socket)
 	{
-		return boost::asio::ip::tcp::endpoint (boost::asio::ip::address_v6::loopback (), listening_socket->listening_port ());
+		return boost::asio::ip::tcp::endpoint (boost::asio::ip::address_v6::loopback (), port);
 	}
 	else
 	{
@@ -605,8 +640,8 @@ namespace
 class request_response_visitor : public nano::message_visitor
 {
 public:
-	explicit request_response_visitor (std::shared_ptr<nano::bootstrap_server> const & connection_a) :
-		connection (connection_a)
+	explicit request_response_visitor (std::shared_ptr<nano::bootstrap_server> connection_a) :
+		connection (std::move (connection_a))
 	{
 	}
 	void keepalive (nano::keepalive const & message_a) override

nano/node/distributed_work.cpp

@@ -8,16 +8,16 @@
 
 std::shared_ptr<request_type> nano::distributed_work::peer_request::get_prepared_json_request (std::string const & request_string_a) const
 {
-	auto request (std::make_shared<request_type> ());
-	request->method (boost::beast::http::verb::post);
-	request->set (boost::beast::http::field::content_type, "application/json");
+	auto http_request = std::make_shared<request_type> ();
+	http_request->method (boost::beast::http::verb::post);
+	http_request->set (boost::beast::http::field::content_type, "application/json");
 	auto address_string = boost::algorithm::erase_first_copy (endpoint.address ().to_string (), "::ffff:");
-	request->set (boost::beast::http::field::host, address_string);
-	request->target ("/");
-	request->version (11);
-	request->body () = request_string_a;
-	request->prepare_payload ();
-	return request;
+	http_request->set (boost::beast::http::field::host, address_string);
+	http_request->target ("/");
+	http_request->version (11);
+	http_request->body () = request_string_a;
+	http_request->prepare_payload ();
+	return http_request;
 }
 
 nano::distributed_work::distributed_work (nano::node & node_a, nano::work_request const & request_a, std::chrono::seconds const & backoff_a) :
@@ -139,16 +139,16 @@ void nano::distributed_work::do_request (nano::tcp_endpoint const & endpoint_a)
 		{
 			std::string request_string;
 			{
-				boost::property_tree::ptree request;
-				request.put ("action", "work_generate");
-				request.put ("hash", this_l->request.root.to_string ());
-				request.put ("difficulty", nano::to_string_hex (this_l->request.difficulty));
+				boost::property_tree::ptree rpc_request;
+				rpc_request.put ("action", "work_generate");
+				rpc_request.put ("hash", this_l->request.root.to_string ());
+				rpc_request.put ("difficulty", nano::to_string_hex (this_l->request.difficulty));
 				if (this_l->request.account.is_initialized ())
 				{
-					request.put ("account", this_l->request.account.get ().to_account ());
+					rpc_request.put ("account", this_l->request.account.get ().to_account ());
 				}
 				std::stringstream ostream;
-				boost::property_tree::write_json (ostream, request);
+				boost::property_tree::write_json (ostream, rpc_request);
 				request_string = ostream.str ();
 			}
 			auto peer_request (connection->get_prepared_json_request (request_string));
@@ -207,11 +207,11 @@ void nano::distributed_work::do_cancel (nano::tcp_endpoint const & endpoint_a)
 		{
 			std::string request_string;
 			{
-				boost::property_tree::ptree request;
-				request.put ("action", "work_cancel");
-				request.put ("hash", this_l->request.root.to_string ());
+				boost::property_tree::ptree rpc_request;
+				rpc_request.put ("action", "work_cancel");
+				rpc_request.put ("hash", this_l->request.root.to_string ());
 				std::stringstream ostream;
-				boost::property_tree::write_json (ostream, request);
+				boost::property_tree::write_json (ostream, rpc_request);
 				request_string = ostream.str ();
 			}
 			auto peer_cancel (cancelling_l->get_prepared_json_request (request_string));
@@ -366,11 +366,11 @@ void nano::distributed_work::handle_failure ()
 				node.logger.always_log ("Work peer(s) failed to generate work for root ", request.root.to_string (), ", retrying...");
 			}
 			auto now (std::chrono::steady_clock::now ());
-			std::weak_ptr<nano::node> node_w (node.shared ());
+			std::weak_ptr<nano::node> node_weak (node.shared ());
 			auto next_backoff (std::min (backoff * 2, std::chrono::seconds (5 * 60)));
-			node.workers.add_timed_task (now + std::chrono::seconds (backoff), [node_w, request_l = request, next_backoff] {
+			node.workers.add_timed_task (now + std::chrono::seconds (backoff), [node_weak, request_l = request, next_backoff] {
 				bool error_l{ true };
-				if (auto node_l = node_w.lock ())
+				if (auto node_l = node_weak.lock ())
 				{
 					error_l = node_l->distributed_work.make (next_backoff, request_l);
 				}

nano/node/network.cpp

@@ -35,6 +35,11 @@ nano::network::network (nano::node & node_a, uint16_t port_a) :
 	port (port_a),
 	disconnect_observer ([] () {})
 {
+	if (!node.flags.disable_udp)
+	{
+		port = udp_channels.get_local_endpoint ().port ();
+	}
+
 	boost::thread::attributes attrs;
 	nano::thread_attributes::set (attrs);
 	// UDP

nano/node/node.cpp

@@ -101,7 +101,14 @@ nano::node::node (boost::asio::io_context & io_ctx_a, boost::filesystem::path co
 	network (*this, config.peering_port),
 	telemetry (std::make_shared<nano::telemetry> (network, workers, observers.telemetry, stats, network_params, flags.disable_ongoing_telemetry_requests)),
 	bootstrap_initiator (*this),
-	bootstrap (config.peering_port, *this),
+	// BEWARE: `bootstrap` takes `network.port` instead of `config.peering_port` because when the user doesn't specify
+	//         a peering port and wants the OS to pick one, the picking happens when `network` gets initialized
+	//         (if UDP is active, otherwise it happens when `bootstrap` gets initialized), so then for TCP traffic
+	//         we want to tell `bootstrap` to use the already picked port instead of itself picking a different one.
+	//         Thus, be very careful if you change the order: if `bootstrap` gets constructed before `network`,
+	//         the latter would inherit the port from the former (if TCP is active, otherwise `network` picks first)
+	//
+	bootstrap (network.port, *this),
 	application_path (application_path_a),
 	port_mapping (*this),
 	rep_crawler (*this),

nano/node/nodeconfig.cpp

@@ -33,12 +33,17 @@ nano::node_config::node_config (uint16_t peering_port_a, nano::logging const & l
 	ipc_config{ network_params.network },
 	external_address{ boost::asio::ip::address_v6{}.to_string () }
 {
-	// The default constructor passes 0 to indicate we should use the default port,
-	// which is determined at node startup based on active network.
 	if (peering_port == 0)
 	{
-		peering_port = network_params.network.default_node_port;
+		// comment for posterity:
+		// - we used to consider ports being 0 a sentinel that meant to use a default port for that specific purpose
+		// - the actual default value was determined based on the active network (e.g. dev network peering port = 44000)
+		// - now, the 0 value means something different instead: user wants to let the OS pick a random port
+		// - for the specific case of the peering port, after it gets picked, it can be retrieved by client code via
+		//   node.network.endpoint ().port ()
+		// - the config value does not get back-propagated because it represents the choice of the user, and that was 0
 	}
+
 	switch (network_params.network.network ())
 	{
 		case nano::networks::nano_dev_network:
@@ -47,14 +52,14 @@ nano::node_config::node_config (uint16_t peering_port_a, nano::logging const & l
 			break;
 		case nano::networks::nano_beta_network:
 		{
-			preconfigured_peers.push_back (default_beta_peer_network);
+			preconfigured_peers.emplace_back (default_beta_peer_network);
 			nano::account offline_representative;
 			release_assert (!offline_representative.decode_account ("nano_1defau1t9off1ine9rep99999999999999999999999999999999wgmuzxxy"));
 			preconfigured_representatives.emplace_back (offline_representative);
 			break;
 		}
 		case nano::networks::nano_live_network:
-			preconfigured_peers.push_back (default_live_peer_network);
+			preconfigured_peers.emplace_back (default_live_peer_network);
 			preconfigured_representatives.emplace_back ("A30E0A32ED41C8607AA9212843392E853FCBCB4E7CB194E35C94F07F91DE59EF");
 			preconfigured_representatives.emplace_back ("67556D31DDFC2A440BF6147501449B4CB9572278D034EE686A6BEE29851681DF");
 			preconfigured_representatives.emplace_back ("5C2FBB148E006A8E8BA7A75DD86C9FE00C83F5FFDBFD76EAA09531071436B6AF");

nano/node/socket.hpp

@@ -134,7 +134,7 @@ class server_socket final : public socket
 	void on_connection (std::function<bool (std::shared_ptr<nano::socket> const & new_connection, boost::system::error_code const &)>);
 	uint16_t listening_port ()
 	{
-		return local.port ();
+		return acceptor.local_endpoint ().port ();
 	}
 
 private:

nano/node/transport/udp.cpp

@@ -64,7 +64,7 @@ std::string nano::transport::channel_udp::to_string () const
 nano::transport::udp_channels::udp_channels (nano::node & node_a, uint16_t port_a, std::function<void (nano::message const &, std::shared_ptr<nano::transport::channel> const &)> sink) :
 	node{ node_a },
 	strand{ node_a.io_ctx.get_executor () },
-	sink{ sink }
+	sink{ std::move (sink) }
 {
 	if (!node.flags.disable_udp)
 	{
@@ -87,10 +87,10 @@ nano::transport::udp_channels::udp_channels (nano::node & node_a, uint16_t port_
 void nano::transport::udp_channels::send (nano::shared_const_buffer const & buffer_a, nano::endpoint endpoint_a, std::function<void (boost::system::error_code const &, std::size_t)> const & callback_a)
 {
 	boost::asio::post (strand,
-	[this, buffer_a, endpoint_a, callback_a] () {
+	[this, buffer_a, endpoint = std::move (endpoint_a), callback_a] () {
 		if (!this->stopped)
 		{
-			this->socket->async_send_to (buffer_a, endpoint_a,
+			this->socket->async_send_to (buffer_a, endpoint,
 			boost::asio::bind_executor (strand, callback_a));
 		}
 	});
@@ -365,10 +365,10 @@ namespace
 class udp_message_visitor : public nano::message_visitor
 {
 public:
-	udp_message_visitor (nano::node & node_a, nano::endpoint const & endpoint_a, std::function<void (nano::message const &, std::shared_ptr<nano::transport::channel> const &)> sink) :
+	udp_message_visitor (nano::node & node_a, nano::endpoint endpoint_a, std::function<void (nano::message const &, std::shared_ptr<nano::transport::channel> const &)> sink) :
 		node{ node_a },
-		endpoint{ endpoint_a },
-		sink{ sink }
+		endpoint{ std::move (endpoint_a) },
+		sink{ std::move (sink) }
 	{
 	}
 	void keepalive (nano::keepalive const & message_a) override

nano/node/websocket.hpp

@@ -300,6 +300,11 @@ namespace websocket
 			return logger;
 		}
 
+		std::uint16_t listening_port ()
+		{
+			return acceptor.local_endpoint ().port ();
+		}
+
 		nano::wallets & get_wallets () const
 		{
 			return wallets;