Merge bitcoin/bitcoin#35730: http: limit connected HTTPRemoteClients

bd4b1524ea init: do not count file descriptors for HTTPServer if -server=0 (Matthew Zipkin)
b08662060d init: account for maximum file descriptors needed by HTTP (Matthew Zipkin)
cc2acebefb http: configure simultaneous connection limit with -rpcmaxconnections (Matthew Zipkin)
b3d6d2d1a7 http: limit connected clients to 16 (Matthew Zipkin)
86651d8197 scripted-diff: Rename nUserBind, nBind, nMaxConnections to snake_case (Matthew Zipkin)

Pull request description:

  Introduces a new configuration option `-rpcmaxconnections` with default value `16`. This is used to limit the number of simultaneous `HTTPClient` connected to the `HTTPServer`. When the limit is reached, new pending connections remain queued in the kernel's socket buffer. Those connections have complete TCP handshakes with the kernel but do not occupy any application memory.

  The previous libevent-based HTTP server had no limit on connections but it did have a limit on the kernel socket queue:

  e7ff4ef2b4/http.c (L3510)
  ```c
  if (listen(fd, 128) == -1) {
  ```

  The current HTTP server, like the p2p server, uses a platform constant here:

  b6becf3534/src/httpserver.cpp (L743)

  (on my macOS `SOMAXCONN` is `128` but on my Debian machine it's `4096`)

  The default of 16 was chosen as a reasonable upper bound for single-user RPC use cases. Systems designed to handle more simultaneous HTTP connections than this (previously relying on the absence of a limit) can adjust the setting.

  ## File descriptors

  Because of the connection limit, we can now account for the maximum number of file descriptors needed by the HTTP server. This addresses several issues (#11368 #11322 maybe #27732) that could have been fixed by a PR waiting in vain for a libevent release (#27731).

  ## Bonus performance improvement

  The new limit is managed in a loop that drains the kernel's socket queue with `accept()`. All pending connections from the queue (up to the limit) are processed in one single call to `SocketHandlerListening()`. The previous code would only accept one connection from the queue on each I/O loop tick, with a `SELECT_TIMEOUT` (50ms) sleep between each.

ACKs for top commit:
  fjahr:
    tACK bd4b1524ea
  janb84:
    ACK bd4b1524ea
  winterrdog:
    tested ACK bd4b1524ea
  hodlinator:
    Concept ACK bd4b1524ea
  willcl-ark:
    ACK bd4b1524ea

Tree-SHA512: 2ef7a96da4d7037c7343ec0ea03fda5bb55d10c2a071fce4929141297515923b203d3d338dbcb6599849768f52aa3c9da509fb5d1d6f7c574a1d2034ea2a9e74

test/functional/feature_init.py

@@ -7,6 +7,7 @@ from concurrent.futures import ThreadPoolExecutor
 from pathlib import Path
 import os
 import platform
+import re
 import shutil
 import signal
 import subprocess
@@ -362,6 +363,61 @@ class InitTest(BitcoinTestFramework):
         self.log.info("Testing node startup with fd limit above INT_MAX")
         self.restart_node_with_fd_limit(1 << 31)
 
+    def init_fd_overflow_test(self):
+        node = self.nodes[1]
+        if node.running:
+            self.stop_node(1)
+
+        # A value larger than any possible int saturates to INT_MAX during arg parsing.
+        # Adding in other file descriptor requirements is guaranteed to overflow,
+        # so expect an InitError before RaiseFileDescriptorLimit() is called.
+        self.log.info("Checking -rpcmaxconnections setting that would overflow int is rejected")
+        node.assert_start_raises_init_error(
+            extra_args=[f"-rpcmaxconnections={2**64}"],
+            expected_msg="Error: Too many file descriptors requested.",
+            match=ErrorMatch.PARTIAL_REGEX
+        )
+
+        if self.RLIM_INFINITY is not None:
+            # Get the platform's file descriptor limit, if possible
+            import resource
+            soft, hard = resource.getrlimit(resource.RLIMIT_NOFILE)
+
+            # Lower the hard limit so RaiseFileDescriptorLimit() has a ceiling.
+            # The hard limit can not be raised again without root privilges,
+            # so this test should always be left for last in the process.
+            try:
+                resource.setrlimit(resource.RLIMIT_NOFILE, (soft, soft))
+            except (ValueError, OSError):
+                self.log.info(f"Skipping rlimit test: cannot reduce hard limit (soft={soft}, hard={hard})")
+                return
+
+            self.log.info("Checking that large -maxconnections setting gets adjusted for available file descriptors")
+            # Note this prints a message to the log and stderr but does not abort the process
+            with node.assert_debug_log(expected_msgs=[f"Reducing -maxconnections from {soft} "]):
+                self.restart_node(1, extra_args=[f"-maxconnections={soft}"])
+            self.stop_node(1, expected_stderr=re.compile(fr"Reducing -maxconnections from {soft} "))
+
+            # From httpserver.h
+            DEFAULT_MAX_HTTP_CONNECTIONS = 16
+
+            self.log.info("Checking -rpcmaxconnections gets blamed if available file descriptors are insufficient")
+            node.assert_start_raises_init_error(
+                extra_args=[f"-rpcmaxconnections={DEFAULT_MAX_HTTP_CONNECTIONS + 1}", f"-maxconnections={soft}"],
+                expected_msg="Not enough file descriptors available. Try reducing -rpcmaxconnections",
+                match=ErrorMatch.PARTIAL_REGEX
+            )
+
+        # Start without the HTTP server to ensure that -rpcmaxconnections is ignored
+        with node.assert_debug_log(
+            expected_msgs = ["net thread start"],
+            unexpected_msgs = ["Initialized HTTP server"],
+            timeout = 10
+        ):
+            node.start(extra_args=[f"-rpcmaxconnections={2**64}", "-server=0"])
+        # No HTTP server, no RPC `stop`
+        node.kill_process()
+
     def run_test(self):
         self.init_pid_test()
         self.init_stress_test_interrupt()
@@ -370,6 +426,7 @@ class InitTest(BitcoinTestFramework):
         self.init_empty_test()
         self.init_rlimit_test()
         self.init_rlimit_large_test()
+        self.init_fd_overflow_test()
 
 
 if __name__ == '__main__':

test/functional/interface_http.py

@@ -8,6 +8,7 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.netutil import NETWORK_ERRORS
 from test_framework.util import assert_equal, str_to_b64str
 
+import concurrent.futures
 import http.client
 import socket
 import threading
@@ -131,6 +132,7 @@ class HTTPBasicsTest (BitcoinTestFramework):
         self.check_null_byte_in_uri()
         self.check_invalid_http_version()
         self.check_whitespace_in_headers()
+        self.check_connection_limit()
 
 
     def check_default_connection(self):
@@ -609,5 +611,84 @@ class HTTPBasicsTest (BitcoinTestFramework):
         assert_equal(response.status, http.client.BAD_REQUEST)
 
 
+    def check_connection_limit(self):
+        self.log.info("Check connection limits")
+
+        # Disable timeout so the initial batch of clients stays connected
+        # until the end of the test.
+        for comment,                   extra_args,                                                 limit in [
+            ("default (16)",           ["-rpcservertimeout=0", "-rest"],                           16),
+            ("-rpcmaxconnections=128", ["-rpcservertimeout=0", "-rest", "-rpcmaxconnections=128"], 128)
+        ]:
+            self.log.info(f"Using connection limit: {comment}")
+            self.restart_node(0, extra_args=extra_args)
+
+            # Close the persistent HTTP connection to this node by replacing it with
+            # a new AuthServiceProxy, reducing HTTPServer::GetConnectionsCount() to 0.
+            # The new AuthServiceProxy won't actually open an HTTP connection until
+            # it needs to send an RPC (for example, to stop the node at the end of the test).
+            self.node._rpc = self.node.create_new_rpc_connection(mode="AUTHPROXY")
+
+            MAX_HTTP_CONNECTIONS = limit
+            connections = []
+
+            # Connections all succeed up to the limit
+            with self.node.assert_debug_log(
+                expected_msgs = [f"method=invalidrpc_{i} " for i in range(1, MAX_HTTP_CONNECTIONS + 1)]
+            ):
+                for i in range(1, MAX_HTTP_CONNECTIONS + 1):
+                    conn = BitcoinHTTPConnection(self.node)
+                    # Each client makes a unique request so it's easy to find in the log
+                    conn.post('/', f'{{"method": "invalidrpc_{i}"}}', connection_header='keep-alive').read()
+                    connections.append(conn)
+
+            # The next connection is over the limit, expect it to timeout
+            with self.node.assert_debug_log(
+                expected_msgs = [],
+                unexpected_msgs = ["method=never_accepted"]
+            ):
+                conn = BitcoinHTTPConnection(self.node)
+                conn.set_timeout(5)
+                try:
+                    conn.post('/', '{"method": "never_accepted"}', connection_header='keep-alive').read()
+                    assert False, "Connection succeeded unexpectedly"
+                except TimeoutError:
+                    pass
+
+            # All original clients are still connected
+            assert_equal(len(connections), MAX_HTTP_CONNECTIONS)
+            for client in connections:
+                assert not client.sock_closed()
+
+            # Try connecting again, but this time we'll wait for acceptance.
+            # Because the send is blocking, we'll execute in a background thread.
+
+            def wait_for_send(conn):
+                return conn.get('/rest/blockhashbyheight/0.json').read()
+
+            conn = BitcoinHTTPConnection(self.node)
+            conn.set_timeout(None)
+            executor = concurrent.futures.ThreadPoolExecutor(max_workers=1)
+            waiting_request = executor.submit(
+                wait_for_send,
+                conn
+            )
+
+            # We are waiting
+            assert not waiting_request.done()
+
+            # Close one of the original connections
+            popped_client = connections.pop()
+            popped_client.close_sock()
+
+            # The waiting connection gets processed
+            delayed_response = waiting_request.result(timeout=5)
+            assert "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206" in delayed_response.decode()
+
+            # Close all remaining connections for clean up
+            for client in connections:
+                client.close_sock()
+
+
 if __name__ == '__main__':
     HTTPBasicsTest(__file__).main()