From 21c7542cf8841f4cf733578e27dda7e25e6bc81d Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Wed, 15 Jan 2025 15:44:16 -0500 Subject: [PATCH] http: switch servers from libevent to bitcoin --- doc/developer-notes.md | 4 ++-- src/httprpc.cpp | 5 +--- src/httpserver.cpp | 19 ++++++++------- src/httpserver.h | 11 ++++----- src/init.cpp | 10 ++++---- src/rest.cpp | 2 +- test/functional/interface_http.py | 40 +++++++++++-------------------- test/functional/interface_rest.py | 11 +++++---- 8 files changed, 44 insertions(+), 58 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 962e8851187..3edf5d34250 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -703,7 +703,7 @@ and its `cs_KeyStore` lock for example). : Parallel script validation threads for transactions in blocks. - [ThreadHTTP (`b-http`)](https://doxygen.bitcoincore.org/httpserver_8cpp.html#http) - : Libevent thread to listen for RPC and REST connections. + : Thread to listen for RPC and REST connections. - [HTTP worker threads (`b-http.xx`)](https://doxygen.bitcoincore.org/httpserver_8cpp.html#http_pool) : Threads to service RPC and REST requests. @@ -716,7 +716,7 @@ and its `cs_KeyStore` lock for example). addrman and running asynchronous validationinterface callbacks. - [TorControlThread (`b-torcontrol`)](https://doxygen.bitcoincore.org/class_tor_controller.html#torcontrol) - : Libevent thread for tor connections. + : Thread for tor connections. - Net threads: diff --git a/src/httprpc.cpp b/src/httprpc.cpp index 1a4e9b9dd5d..ed068a34b8e 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -26,8 +26,7 @@ #include #include -using http_libevent::EventBase; -using http_libevent::HTTPRequest; +using http_bitcoin::HTTPRequest; using util::SplitString; using util::TrimStringView; @@ -349,8 +348,6 @@ bool StartHTTPRPC(const std::any& context) if (g_wallet_init_interface.HasWalletSupport()) { RegisterHTTPHandler("/wallet/", false, handle_rpc); } - struct event_base* eventBase = EventBase(); - assert(eventBase); return true; } diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 3c35eede99a..d9e5ae59672 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -61,7 +61,7 @@ static constexpr auto SELECT_TIMEOUT{50ms}; static constexpr int SOCKET_OPTION_TRUE{1}; using common::InvalidPortErrMsg; -using http_libevent::HTTPRequest; +using http_bitcoin::HTTPRequest; /** Maximum size of http request (request line + headers) */ static const size_t MAX_HEADERS_SIZE = 8192; @@ -217,9 +217,6 @@ static void MaybeDispatchRequestToWorker(std::shared_ptr hreq) return; } - LogDebug(BCLog::HTTP, "Received a %s request for %s from %s\n", - RequestMethodString(hreq->GetRequestMethod()), SanitizeString(hreq->GetURI(), SAFE_CHARS_URI).substr(0, 100), hreq->GetPeer().ToStringAddrPort()); - // Find registered handler for prefix std::string strURI = hreq->GetURI(); std::string path; @@ -308,8 +305,12 @@ static void http_request_cb(struct evhttp_request* req, void* arg) } } } - auto hreq{std::make_shared(req, *static_cast(arg))}; - MaybeDispatchRequestToWorker(std::move(hreq)); + auto hreq{std::make_shared(req, *static_cast(arg))}; + + // Disabled now that http_libevent is deprecated, or code won't compile. + // This line is currently unreachable and will be cleaned up in a future commit. + // MaybeDispatchRequestToWorker(std::move(hreq)); + Assume(false); } /** Callback to reject HTTP requests after shutdown. */ @@ -319,7 +320,6 @@ static void http_reject_request_cb(struct evhttp_request* req, void*) evhttp_send_error(req, HTTP_SERVUNAVAIL, nullptr); } -/// \anchor http /** Event dispatcher thread */ static void ThreadHTTP(struct event_base* base) { @@ -1424,6 +1424,7 @@ HTTPServer::IOReadiness HTTPServer::GenerateWaitSockets() const return io_readiness; } +/// \anchor http void HTTPServer::ThreadSocketHandler() { while (!m_interrupt_net) { @@ -1685,8 +1686,8 @@ bool InitHTTPServer() return false; } - // Create HTTPServer, using a dummy request handler just for this commit - g_http_server = std::make_unique([&](std::unique_ptr req){}); + // Create HTTPServer + g_http_server = std::make_unique(MaybeDispatchRequestToWorker); g_http_server->SetServerTimeout(std::chrono::seconds(gArgs.GetIntArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT))); diff --git a/src/httpserver.h b/src/httpserver.h index 0045178e04b..9dee8f2c3ca 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -75,8 +75,12 @@ void StopHTTPServer(); void UpdateHTTPServerLogging(bool enable); } // namespace http_libevent +namespace http_bitcoin { + class HTTPRequest; +} /** Handler for requests to a certain HTTP path */ -typedef std::function HTTPRequestHandler; +using HTTPRequestHandler = std::function; + /** Register handler for prefix. * If multiple handlers match a prefix, the first-registered one will * be invoked. @@ -86,11 +90,6 @@ void RegisterHTTPHandler(const std::string &prefix, bool exactMatch, const HTTPR void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch); namespace http_libevent { -/** Return evhttp event base. This can be used by submodules to - * queue timers or custom events. - */ -struct event_base* EventBase(); - /** In-flight HTTP request. * Thin C++ wrapper around evhttp_request. */ diff --git a/src/init.cpp b/src/init.cpp index 320940e4e84..0e72443cc33 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -145,10 +145,10 @@ using common::InvalidPortErrMsg; using common::ResolveErrMsg; -using http_libevent::InitHTTPServer; -using http_libevent::InterruptHTTPServer; -using http_libevent::StartHTTPServer; -using http_libevent::StopHTTPServer; +using http_bitcoin::InitHTTPServer; +using http_bitcoin::InterruptHTTPServer; +using http_bitcoin::StartHTTPServer; +using http_bitcoin::StopHTTPServer; using node::ApplyArgsManOptions; using node::BlockManager; using node::CalculateCacheSizes; @@ -774,7 +774,7 @@ static void StartupNotify(const ArgsManager& args) static bool AppInitServers(NodeContext& node) { const ArgsManager& args = *Assert(node.args); - if (!InitHTTPServer(*Assert(node.shutdown_signal))) { + if (!InitHTTPServer()) { return false; } StartRPC(); diff --git a/src/rest.cpp b/src/rest.cpp index 5321cbee104..9be319a3866 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -37,7 +37,7 @@ #include -using http_libevent::HTTPRequest; +using http_bitcoin::HTTPRequest; using node::GetTransaction; using node::NodeContext; using util::SplitString; diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py index 902fe9ff086..a71fc474a8f 100755 --- a/test/functional/interface_http.py +++ b/test/functional/interface_http.py @@ -13,10 +13,9 @@ import urllib.parse # Configuration option for some tests RPCSERVERTIMEOUT = 2 -# Set in httpserver.cpp and passed to libevent evhttp_set_max_headers_size() +# Set in httpserver.h MAX_HEADERS_SIZE = 8192 -# Set in serialize.h and passed to libevent evhttp_set_max_body_size() -MAX_SIZE = 0x02000000 +MAX_BODY_SIZE = 32 * 1024 * 1024 # When a test expects a server disconnection, any of these errors are # acceptable. The specific event is determined by race condition and platform OS. @@ -205,11 +204,6 @@ class HTTPBasicsTest (BitcoinTestFramework): headers_below_limit = (MAX_HEADERS_SIZE - 1000) // header_line_length headers_above_limit = MAX_HEADERS_SIZE // header_line_length - # This is a libevent mystery: - # libevent does not reject the request until it is more than - # 1,000 bytes above the configured limit. - headers_above_limit += 1000 // header_line_length - # Many small header lines is ok conn = BitcoinHTTPConnection(self.node) for i in range(headers_below_limit): @@ -227,8 +221,8 @@ class HTTPBasicsTest (BitcoinTestFramework): # Compute how much data we can add to a request message body # to make / break the limit. base_request_body_size = len('{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": [""]}}') - bytes_below_limit = MAX_SIZE - base_request_body_size - bytes_above_limit = MAX_SIZE - base_request_body_size + 2 + bytes_below_limit = MAX_BODY_SIZE - base_request_body_size + bytes_above_limit = MAX_BODY_SIZE - base_request_body_size + 2 # Large request body size is ok conn = BitcoinHTTPConnection(self.node) @@ -440,11 +434,11 @@ class HTTPBasicsTest (BitcoinTestFramework): def check_disallowed_http_methods(self): self.log.info("Check that unsafe or unsupported HTTP methods are rejected") for method, err in [ - ['TRACE', http.client.NOT_IMPLEMENTED], - ['CONNECT', http.client.NOT_IMPLEMENTED], + ['TRACE', http.client.METHOD_NOT_ALLOWED], + ['CONNECT', http.client.METHOD_NOT_ALLOWED], ['DELETE', http.client.METHOD_NOT_ALLOWED], - ['PATCH', http.client.NOT_IMPLEMENTED], - ['OPTIONS', http.client.NOT_IMPLEMENTED], + ['PATCH', http.client.METHOD_NOT_ALLOWED], + ['OPTIONS', http.client.METHOD_NOT_ALLOWED], ['GET', http.client.METHOD_NOT_ALLOWED] # RPC endpoint '/' only handles POST ]: conn = BitcoinHTTPConnection(self.node) @@ -508,8 +502,7 @@ class HTTPBasicsTest (BitcoinTestFramework): self.log.info("Check that duplicate Content-Length headers are handled") # https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 # Multiple Content-Length headers with differing values "MUST" - # result in an error, but libevent is lenient about this and - # only reads the first. + # result in an error. conn = BitcoinHTTPConnection(self.node) body = '{"method":"getblockcount"}' raw = ( @@ -523,9 +516,7 @@ class HTTPBasicsTest (BitcoinTestFramework): ).encode("ascii") conn.send_raw(raw) response = conn.recv_raw().decode() - assert "HTTP/1.1 200 OK" in response - count = self.node.getblockcount() - assert f'"result":{count}' in response + assert response.startswith("HTTP/1.1 400") def check_null_byte_in_uri(self): @@ -562,23 +553,20 @@ class HTTPBasicsTest (BitcoinTestFramework): def check_whitespace_in_headers(self): self.log.info("Check that requests with whitespace in headers are rejected") # Extra whitespace before colon in header. - # This request should be rejected entirely but libevent handles it oddly: - # It allows the header and includes the trailing space in the header field-name. - # Authorization fails because "Authorization " != "Authorization" conn = BitcoinHTTPConnection(self.node) conn.headers = {"Authorization ": f"Basic {str_to_b64str(conn.authpair)}"} response = conn.post('/', '{"method": "getbestblockhash"}') - assert_equal(response.status, http.client.UNAUTHORIZED) + assert_equal(response.status, http.client.BAD_REQUEST) # Extra whitespace at start of new line. - # Libevent implements "line folding" as defined in + # "line folding" as defined in # https://www.rfc-editor.org/rfc/rfc2616#section-2.2 - # despite the practice being considered unsafe and explicitly deprecated in + # is considered unsafe and is explicitly deprecated in # https://www.rfc-editor.org/rfc/rfc7230#section-3.2.4 conn = BitcoinHTTPConnection(self.node) conn.headers = {"Authorization": f"Basic \n {str_to_b64str(conn.authpair)}"} response = conn.post('/', '{"method": "getbestblockhash"}') - assert_equal(response.status, http.client.OK) + assert_equal(response.status, http.client.BAD_REQUEST) if __name__ == '__main__': diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index 07b42e6b2e9..c3dc6bc406b 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -287,10 +287,12 @@ class RESTTest (BitcoinTestFramework): assert_equal(len(json_obj), 1) # ensure that there is one header in the json response assert_equal(json_obj[0]['hash'], bb_hash) # request/response hash should be the same - # Check invalid uri (% symbol at the end of the request) - for invalid_uri in [f"/headers/{bb_hash}%", f"/blockfilterheaders/basic/{bb_hash}%", "/mempool/contents.json?%"]: + # Check tolerance for invalid URI (% symbol at the end of the request) + for invalid_uri in [f"/headers/{bb_hash}%", f"/blockfilterheaders/basic/{bb_hash}%"]: resp = self.test_rest_request(invalid_uri, ret_type=RetType.OBJ, status=400) - assert_equal(resp.read().decode('utf-8').rstrip(), "URI parsing failed, it likely contained RFC 3986 invalid characters") + assert_equal(resp.read().decode('utf-8').rstrip(), f"Invalid hash: {bb_hash}%") + resp = self.test_rest_request("/mempool/contents.json?%", ret_type=RetType.OBJ, status=200) + assert_equal(resp.read().decode('utf-8').rstrip(), "{}") # Compare with normal RPC block response rpc_block_json = self.nodes[0].getblock(bb_hash) @@ -480,8 +482,7 @@ class RESTTest (BitcoinTestFramework): get_block_part(status=400, query_params={"offset": "x"}) get_block_part(status=400, query_params={"size": "y"}) get_block_part(status=400, query_params={"offset": "x", "size": "y"}) - assert get_block_part(status=400, query_params="%XY").decode("utf-8").startswith("URI parsing failed") - + get_block_part(status=400, query_params="%XY") get_block_part(status=400, query_params={"offset": 0, "size": 0}) get_block_part(status=400, query_params={"offset": len(block_bin), "size": 0}) get_block_part(status=400, query_params={"offset": len(block_bin), "size": 1})