Allow setting of max incoming connection count on command line and in config file #3163

erikd · 2018-01-20T21:35:46Z

With this patch, the maximum number of incoming peers can be set either on the command line (with --in-peers <n> or in the config file. I've tested this and even after 12 hours the number of incoming connections does not go over the specified number whereas previously I would see between 10 and 50 incoming connections after 12 hours.

However it is also supposed to be possible to modify the number of incoming connections on the fly via an RPC call and that is not currently working. However setting the number of outgoing peer connections via an RPC call isn't working either.

~~Therefore I'll leave this PR as is and fix the RPC call business in an upcoming PR.~~

RPC is now working as well.

moneromooo-monero · 2018-01-21T13:12:54Z

src/p2p/net_node.inl

+  bool node_server<t_payload_net_handler>::set_max_in_peers(const boost::program_options::variables_map& vm, int64_t max)
+  {
+    if(max == -1) {
+      m_config.m_net_config.max_in_connection_count = P2P_DEFAULT_CONNECTIONS_COUNT;


That change causes a large change in the network, no ?
I think that if a new node comes online, it might not be able to connect to the network (or will take a while to find a node which doesn't have all its slots free). I would either consider the -1 default to mean no limit, or set a substantially larger limit than the out one.

Set to -1.

moneromooo-monero · 2018-01-21T13:13:50Z

src/rpc/core_rpc_server_commands_defs.h

+	struct request
+    {
+	  uint64_t in_peers;
+      BEGIN_KV_SERIALIZE_MAP()


Would be nice to get proper indent on new code.

moneromooo-monero · 2018-01-21T13:14:42Z

contrib/epee/include/net/levin_protocol_handler_async.h

+		}
+		--count;
+	}
+


Please factor with the existing function, should be pretty much all the same execpt the selection condition near the top.

I factored out the deletion of the connections, but my C++ is too rusty to factor it further.

moneromooo-monero · 2018-01-22T09:39:46Z

"opertations" in the last commit message.

erikd · 2018-01-22T09:51:51Z

"opertations" fixed!

erikd · 2018-01-23T07:40:06Z

This is working as intended on my system.

moneromooo-monero · 2018-01-23T13:43:45Z

contrib/epee/include/net/levin_protocol_handler_async.h

+
+	if (out_connections.size() == 0)
+		return;
+


That whole part can be factored too, since the only difference is a boolean which can be passed.

Wouldn't t be better to change that to:

if (out_connections.size() > 0) delete_connections(count, out_connections);

The early return without executing CRITICAL_REGION_END is a bit problematic.

Nevermind. Now I get it. Will fix tomorrow.

moneromooo-monero · 2018-01-24T15:24:53Z

Something bludgeoned indentation :/

moneromooo-monero · 2018-01-28T18:49:41Z

contrib/epee/include/net/levin_protocol_handler_async.h

+      del_connection(conn);
+      close(*i);
+      connections.erase(i);
+    }


A recent PR modified this to delete from the end, please apply to this version too when you rebase.

This is needed so that a max_in_connection_count can be added.

This rename is needed so that delete_in_connections can be added.

leonklingele

Please also revert trimming of trailing spaces.

leonklingele · 2018-01-28T22:23:31Z

src/common/rpc_client.h

      if (!ok)
      {
        fail_msg_writer() << "Couldn't connect to daemon: " << m_http_client.get_host() << ":" << m_http_client.get_port();
        return false;
      }
-      else if (res.status != CORE_RPC_STATUS_OK) // TODO - handle CORE_RPC_STATUS_BUSY ?
+      ok = epee::net_utils::invoke_http_json_rpc("/json_rpc", method_name, req, res, m_http_client, t_http_connection::TIMEOUT());
+      if (! ok || res.status != CORE_RPC_STATUS_OK) // TODO - handle CORE_RPC_STATUS_BUSY ?


Remove whitespace after !

leonklingele · 2018-01-28T22:24:14Z

src/common/rpc_client.h

      if (!ok)
      {
        fail_msg_writer() << "Couldn't connect to daemon: " << m_http_client.get_host() << ":" << m_http_client.get_port();
        return false;
      }
-      else if (res.status != CORE_RPC_STATUS_OK) // TODO - handle CORE_RPC_STATUS_BUSY ?
+      ok = epee::net_utils::invoke_http_json(relative_url, req, res, m_http_client, t_http_connection::TIMEOUT());
+      if (! ok || res.status != CORE_RPC_STATUS_OK) // TODO - handle CORE_RPC_STATUS_BUSY ?


Remove whitespace after !

leonklingele · 2018-01-28T22:24:36Z

src/daemon/command_parser_executor.cpp

+	try {
+		limit = std::stoi(args[0]);
+	}
+


Remove this newline

This matches the rest of the code in this file.

leonklingele · 2018-01-28T22:25:54Z

src/daemon/command_parser_executor.cpp

+		limit = std::stoi(args[0]);
+	}
+
+	catch(const std::exception& ex) {


Add whitespace after catch

Move curly bracket to new line

This matches the rest of the code in this file.

leonklingele · 2018-01-28T22:26:09Z

src/daemon/command_parser_executor.cpp

+	if (args.empty()) return false;
+
+	unsigned int limit;
+	try {


Move curly bracket to new line

This matches the rest of the code in this file.

leonklingele · 2018-01-28T22:26:56Z

src/daemon/rpc_command_executor.cpp

@@ -1262,7 +1262,7 @@ bool t_rpc_command_executor::out_peers(uint64_t limit)

 	if (m_is_rpc)
 	{
-		if (!m_rpc_client->json_rpc_request(req, res, "out_peers", fail_message.c_str()))
+		if (!m_rpc_client->rpc_request(req, res, "/out_peers", fail_message.c_str()))


Why did this change?

Its not a JSON request. Calling it via json_rpc_request doesn't work (in fact never worked). Calling it via rpc_request does work.

So this is an (unrelated) bugfix?

That is a separate commit that is part of this PR because it is related.

leonklingele · 2018-01-28T22:29:02Z

src/p2p/net_node.inl

+    size_t count = 0;
+    m_net_server.get_config_object().foreach_connection([&](const p2p_connection_context& cntxt)
+    {
+      if(cntxt.m_is_income)


Add a space after if

This matches the rest of the code in this file.

It was already possible to limit outgoing connections. One might want to do this on home network connections with high bandwidth but low usage caps.

Previously, the method name was printed as an exmpty string because the input string had already been moved with `std::move`.

Previous code was unable to distingush between a connection error and a communication error.

Original implementations could never have worked.

erikd · 2018-02-03T07:26:44Z

I've noticed that if I set in_peers and start the daemon, the number of input peers climbs to the specified number, holds that value for a while and then the number slowly drops back to zero over a couple of days. I suspect that when an incoming connection is lost, the incoming connection count isn't being decremented so that a replacement connection isn't being allowed.

More testing showed that this was indeed working correctly. No changes needed.

fluffypony

Reviewed

628b78a Fix in_peers/out_peers RPC operations (Erik de Castro Lopo) ece9bcf rpc_client: Fix error handling (Erik de Castro Lopo) 8f30350 Fix method name in invoke_http_json_rpc (Erik de Castro Lopo) 32c0f90 Allow the number of incoming connections to be limited (Erik de Castro Lopo) d609a2c Rename delete_connections to delete_out_connections (Erik de Castro Lopo) b927c0f Rename connections_count to max_out_connection_count (Erik de Castro Lopo)

erikd changed the title ~~Allow setting of incoming connections on command line and config file~~ Allow setting of max incoming connection count on command line and in config file Jan 21, 2018

erikd changed the title ~~Allow setting of max incoming connection count on command line and in config file~~ [Do not merge, still testing]: Allow setting of max incoming connection count on command line and in config file Jan 21, 2018

moneromooo-monero reviewed Jan 21, 2018

View reviewed changes

erikd changed the title ~~[Do not merge, still testing]: Allow setting of max incoming connection count on command line and in config file~~ Allow setting of max incoming connection count on command line and in config file Jan 23, 2018

moneromooo-monero reviewed Jan 23, 2018

View reviewed changes

moneromooo-monero approved these changes Jan 28, 2018

View reviewed changes

erikd added 2 commits January 29, 2018 07:06

Rename connections_count to max_out_connection_count

b927c0f

This is needed so that a max_in_connection_count can be added.

Rename delete_connections to delete_out_connections

d609a2c

This rename is needed so that delete_in_connections can be added.

leonklingele reviewed Jan 28, 2018

View reviewed changes

erikd added 4 commits January 29, 2018 11:14

Allow the number of incoming connections to be limited

32c0f90

It was already possible to limit outgoing connections. One might want to do this on home network connections with high bandwidth but low usage caps.

Fix method name in invoke_http_json_rpc

8f30350

Previously, the method name was printed as an exmpty string because the input string had already been moved with `std::move`.

rpc_client: Fix error handling

ece9bcf

Previous code was unable to distingush between a connection error and a communication error.

Fix in_peers/out_peers RPC operations

628b78a

Original implementations could never have worked.

erikd changed the title ~~Allow setting of max incoming connection count on command line and in config file~~ [Do not merge, may be a problem with this] : Allow setting of max incoming connection count on command line and in config file Feb 3, 2018

erikd changed the title ~~[Do not merge, may be a problem with this] : Allow setting of max incoming connection count on command line and in config file~~ Allow setting of max incoming connection count on command line and in config file Feb 3, 2018

fluffypony approved these changes Feb 16, 2018

View reviewed changes

fluffypony merged commit 628b78a into monero-project:master Feb 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting of max incoming connection count on command line and in config file #3163

Allow setting of max incoming connection count on command line and in config file #3163

erikd commented Jan 20, 2018 •

edited

Loading

moneromooo-monero Jan 21, 2018

erikd Jan 22, 2018

moneromooo-monero Jan 21, 2018

erikd Jan 22, 2018

moneromooo-monero Jan 21, 2018

erikd Jan 22, 2018

moneromooo-monero commented Jan 22, 2018

erikd commented Jan 22, 2018

erikd commented Jan 23, 2018

moneromooo-monero Jan 23, 2018

erikd Jan 24, 2018

erikd Jan 24, 2018

moneromooo-monero commented Jan 24, 2018

moneromooo-monero Jan 28, 2018

leonklingele left a comment

leonklingele Jan 28, 2018

erikd Jan 29, 2018

leonklingele Jan 28, 2018

leonklingele Jan 28, 2018

erikd Jan 29, 2018

leonklingele Jan 28, 2018

erikd Jan 29, 2018

leonklingele Jan 28, 2018

erikd Jan 29, 2018

leonklingele Jan 28, 2018

erikd Jan 28, 2018 •

edited

Loading

leonklingele Jan 28, 2018

erikd Jan 29, 2018

leonklingele Jan 28, 2018

erikd Jan 29, 2018

erikd commented Feb 3, 2018 •

edited

Loading

fluffypony left a comment

Allow setting of max incoming connection count on command line and in config file #3163

Allow setting of max incoming connection count on command line and in config file #3163

Conversation

erikd commented Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Jan 22, 2018

erikd commented Jan 22, 2018

erikd commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Jan 24, 2018

Choose a reason for hiding this comment

leonklingele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd commented Feb 3, 2018 • edited Loading

fluffypony left a comment

Choose a reason for hiding this comment

erikd commented Jan 20, 2018 •

edited

Loading

erikd Jan 28, 2018 •

edited

Loading

erikd commented Feb 3, 2018 •

edited

Loading