Replace libevent with our own HTTP and socket-handling implementation by pinheadmz · Pull Request #32061 · bitcoin/bitcoin

pinheadmz · 2025-03-13T19:32:14Z

This is a major component of removing libevent as a dependency of the project, by replacing the HTTP server used for RPC and REST with one implemented entirely within the Bitcoin Core codebase. The new HTTPServer class runs its own I/O thread, handling socket connections with code based on #30988, but tailored specifically for HTTP.

Some functional tests were added in #32408 and #34772 to cover libevent behaviors that remain consistent with this branch. A few utilities were added in #34905.

Commit strategy:

Implement a few helper functions for strings, timestamps, etc needed by HTTP protocol
Isolate the existing libevent-based HTTP server in a namespace http_libevent
Implement HTTP in a new namespace http_bitcoin (classes like HTTPRequest, HTTPClient, etc...)
Switch bitcoind from the libevent server to the new server
Clean up (delete http_libevent)

Fuzz testing

libfuzzer

24 hours x8 cores at b75223b
generated corpus: https://github.com/pinheadmz/qa-assets/tree/httprequest-PR32061-b75223bfb3
coverage report: https://thebitcoinblockclock.com/demo/libfuzzer_PR32061_b75223bfb3/

fuzzamoto

24 hours x8 cores at b75223b using an HTTP dictionary
coverage report: https://thebitcoinblockclock.com/demo/fuzzamoto_PR32061_b75223b/

Integration testing:

I am testing the new HTTP server by forking projects that integrate with bitcoin via HTTP and running their integration tests with bitcoind built from this branch (on Github actions). I will continue adding integrations over time, and re-running these CI tests as this branch gets rebased:

DrahtBot · 2025-03-13T19:32:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32061.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj, fjahr, w0xlt, furszy, hodlinator, theStack
Stale ACK	romanz, vasild

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34411 ([POC] Full Libevent removal by fanquake)
#34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

overriden -> overridden [misspelled in the m_keep_alive comment]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

Lookup(address_string, port, false) in src/httpserver.cpp
Lookup("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaam2dqd.onion", 0, false) in src/test/httpserver_tests.cpp
Lookup("0.0.0.0", 0, false) in src/test/httpserver_tests.cpp

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_http.py] assert response.getheader('WWW-Authenticate') is not None -> assert_not_equal(response.getheader('WWW-Authenticate'), None)
[test/functional/interface_http.py] assert response.getheader('WWW-Authenticate') is not None -> assert_not_equal(response.getheader('WWW-Authenticate'), None)
[test/functional/interface_http.py] assert response.status == http.client.UNAUTHORIZED -> assert_equal(response.status, http.client.UNAUTHORIZED)
[test/functional/interface_http.py] assert response.status == http.client.UNAUTHORIZED -> assert_equal(response.status, http.client.UNAUTHORIZED)
[test/functional/interface_http.py] assert response.status == http.client.NOT_FOUND -> assert_equal(response.status, http.client.NOT_FOUND)

^{2026-04-30 14:17:50}

DrahtBot · 2025-03-13T20:29:34Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/38735177073}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

laanwj · 2025-03-14T12:52:30Z

Concept ACK, nice work

vasild · 2025-03-14T13:57:43Z

Concept ACK

fjahr · 2025-03-14T20:29:05Z

Concept ACK

My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!

pinheadmz · 2025-03-15T00:03:45Z

My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!

Sure, by coredev I had already written most of this implementation (based on sockman) but the performance was bad, and that was part of the motivation behind the deep-dive talk. However, by the end of the week I had reviewed that code in person with smart attendees and not only improved the performance of my code but started to improve performance vs master branch as well! Those updates came in the days just after the deep-dive discussion.

SOME kind of sockman is needed to replace libevent. The one @vasild wrote does actually seem to work well for this purpose as well as for p2p, and it would be "nice" to only have to maintain one I/O loop structure in bitcoind. @theuni is investigating how a sockman for http could be optimized if it had no other purpose, and I think that is the kind of feedback that will help us decide which path to take.

vasild · 2025-03-19T12:22:15Z

SOME kind of sockman is needed to replace libevent ... it would be "nice" to only have to maintain one I/O loop structure in bitcoind.

💯

This commit is a no-op to isolate HTTP methods and objects that depend on libevent. Following commits will add replacement objects and methods in a new namespace for testing and review before switching over the server.

see: https://www.rfc-editor.org/rfc/rfc2616#section-4.2 https://www.rfc-editor.org/rfc/rfc7231#section-5 https://www.rfc-editor.org/rfc/rfc7231#section-7 https://httpwg.org/specs/rfc9111.html#header.field.definitions

HTTP Response message: https://datatracker.ietf.org/doc/html/rfc1945#section-6 Status line (first line of response): https://datatracker.ietf.org/doc/html/rfc1945#section-6.1 Status code definitions: https://datatracker.ietf.org/doc/html/rfc1945#section-9

HTTP Request message: https://datatracker.ietf.org/doc/html/rfc1945#section-5 Request Line aka Control Line aka first line: https://datatracker.ietf.org/doc/html/rfc1945#section-5.1 See message_read_status() in libevent http.c for how `MORE_DATA_EXPECTED` is handled there

…ocket Introduce a new low-level socket managing class `HTTPServer`. BindAndStartListening() was copied from CConnMan's BindListenPort() in net.cpp and modernized. Unit-test it with a new class `SocketTestingSetup` which mocks `CreateSock()` and will enable mock client I/O in future commits. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

AcceptConnection() is mostly copied from CConmann in net.cpp and then modernized. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Socket handling methods are copied from CConnMan: `CConnman::GenerateWaitSockets()` `CConnman::SocketHandlerListening()` `CConnman::ThreadSocketHandler()` and `CConnman::SocketHandler()` are combined into ThreadSocketHandler()`. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

`SocketHandlerConnected()` adapted from CConnman Testing this requires adding a new feature to the SocketTestingSetup, inserting a "request" payload into the mock client that connects to us. This commit also moves IOErrorIsPermanent() from sock.cpp to sock.h so it can be called from the socket handler in httpserver.cpp Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Sockets-touching bits copied and adapted from `CConnman::SocketSendData()` Testing this requires adding a new feature to the SocketTestingSetup, returning the DynSock I/O pipes from the mock socket so the received data can be checked. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2 > A server MAY process a sequence of pipelined requests in parallel if they all have safe methods (Section 4.2.1 of [RFC7231]), but it MUST send the corresponding responses in the same order that the requests were received. We choose NOT to process requests in parallel. They are executed in the order recevied as well as responded to in the order received. This prevents race conditions where old state may get sent in response to requests that are very quick to process but were requested later on in the queue.

This is a refactor to prepare for matching the API of HTTPRequest definitions in both namespaces http_bitcoin and http_libevent. In particular, to provide a consistent return type for GetRequestMethod() in both classes.

These methods are called by http_request_cb() and are present in the original http_libevent::HTTPRequest.

The original function is passed to libevent as a callback when HTTP requests are received and processed. It wrapped the libevent request object in a http_libevent::HTTPRequest and then handed that off to bitcoin for basic checks and finally dispatch to worker threads. In this commit we split the function after the http_libevent::HTTPRequest is created, and pass that object to a new function that maintains the logic of checking and dispatching. This will be the merge point for http_libevent and http_bitcoin, where HTTPRequest objects from either namespace have the same downstream lifecycle.

The original function was already naturally split into two chunks: First, we parse and validate the users' RPC configuration for IPs and ports. Next we bind libevent's http server to the appropriate endpoints. This commit splits these chunks into two separate functions, leaving the argument parsing in the common space of the module and moving the libevent-specific binding into the http_libevent namespace. A future commit will implement http_bitcoin::HTTPBindAddresses to bind the validate list of endpoints by the new HTTP server.

pinheadmz

Push to 91b2225:

Fix conflicts from #32297 and collapse merged commits from #34905 #34772 and #35086.

One notable change is defining a specific error type ContentTooLargeError
because functional test interface_http now expects (and libevent provides)
a different HTTP error code for excessive request size.

Address review from @vasild and @hodlinator

A few of these comments I couldn't respond to inline, so below

Re: comment #32061 (comment)

would also remove leading whitespace.

Good catch, I checked how libevent handles this and the behavior is the same (silent lenience)
so I think it's ok to keep the same.

In libevent, evhttp_handle_chunked_read() calls evutil_strtoll() to parse the chunk size,
which calls strtoll() from the standard C library, which skips leading whitespace!

They even have a test:

s = " 99999stuff";
tt_want(evutil_strtoll(s, &endptr, 10) == (ev_int64_t)99999);

Re: comment #32061 (comment)

Can use std::atomic m_idle_since;

done

Re: comment #32061 (comment)

remove all mention of libevent from our logging

I added a new commit for this. This makes #34411 a cleaner removal from just the build system.

Address review from @b-l-u-e and @janb84

Some great bugs caught here including a potential crash vulnerability. Taking a cue
from them I asked claude to write a functional test to cover the most common and
well-known HTTP bugs/vulnerabilities and added that to interface_http.py. It's
the new first commit in the PR to cover libevent behavior before switching over.
It may be picked off in to its own PR.

pinheadmz · 2026-04-24T16:52:11Z

+    switch (code) {
+        case HTTP_OK: return "OK";
+        case HTTP_NO_CONTENT: return "No Content";
+        case HTTP_BAD_REQUEST: return "Bad Request";
+        case HTTP_UNAUTHORIZED: return "Unauthorized";
+        case HTTP_FORBIDDEN: return "Forbidden";
+        case HTTP_NOT_FOUND: return "Not Found";
+        case HTTP_BAD_METHOD: return "Method Not Allowed";
+        case HTTP_INTERNAL_SERVER_ERROR: return "Internal Server Error";
+        case HTTP_SERVICE_UNAVAILABLE: return "Service Unavailable";
+    }


cool fixed the indentation here.

pinheadmz · 2026-04-24T17:04:21Z

+        // Not enough data in buffer for expected body
+        if (reader.Remaining() < *content_length) return false;
+
+        if (*content_length > MAX_SIZE) throw std::runtime_error("Max body size exceeded");


okay I'll create a unique constant here instead of using the serialize.h constant.

pinheadmz · 2026-04-24T18:16:48Z

+//! Maximum size of each headers line in an HTTP request.
+//! See https://github.com/bitcoin/bitcoin/pull/6859
+//! And libevent http.c evhttp_parse_headers_()
+constexpr size_t MAX_HEADERS_SIZE{8192};


It's both (its the LineReader max line argument and also checked against the total consumed), extended the comment.

pinheadmz · 2026-04-27T14:51:15Z

+    {
+        // Prevent "chunked" transfer from exceeding size limit
+        HTTPRequest req;
+        std::span<const std::byte> ok_chunked{StringToBytes("GET / HTTP/1.0\n"


using the better name here

pinheadmz · 2026-04-27T14:54:02Z

-    self->handler();
-    if (self->deleteWhenTriggered)
-        delete self;
+    return GetQueryParameterFromUri(GetURI(), key);


pinheadmz · 2026-04-29T13:30:39Z

+            const auto chunk_size{ToIntegral<uint64_t>(util::TrimStringView(chunk_size_noext), /*base=*/16)};
+            if (!chunk_size) throw std::runtime_error("Cannot parse chunk length value");
+
+            if (*chunk_size + m_body.size() > MAX_SIZE) throw std::runtime_error("Chunk will exceed max body size");


OOoh that is a fun one. Reproduced attack and confirmed the suggested patch is a fix.

pinheadmz · 2026-04-29T13:44:46Z

+    {
+        // Malformed: version is out of range
+        HTTPRequest req;
+        std::span<const std::byte> malformed_request_line{StringToBytes("GET / HTTP/-1.0\r\nHost: 127.0.0.1\r\n\r\n")};


Thanks and as pointed out by @b-l-u-e I need to be more strict on the number of characters even allowed on either side of the decimal point, so that has been addressed.

pinheadmz · 2026-04-29T13:45:13Z

+    if (version_parts.size() != 2) throw std::runtime_error("HTTP request line malformed");
+    auto major = ToIntegral<uint8_t>(version_parts[0]);
+    auto minor = ToIntegral<uint8_t>(version_parts[1]);
+    if (!major || !minor || major > 1 || minor > 9) throw std::runtime_error("HTTP bad version");


Thanks, took your suggestion here and I agree we should be rejecting HTTP/0.9 anyway

pinheadmz · 2026-04-29T13:56:23Z

+    // https://httpwg.org/specs/rfc9110.html#rfc.section.2.5
+    const std::vector<std::string_view> version_parts{Split<std::string_view>(parts[2].substr(5), ".")};
+    if (version_parts.size() != 2) throw std::runtime_error("HTTP request line malformed");
+    auto major = ToIntegral<uint8_t>(version_parts[0]);


good catch! suggestion taken and added unit test.

pinheadmz · 2026-04-29T15:29:01Z

+        // that can not be empty.
+        if (key.empty()) throw std::runtime_error("Empty HTTP header name");
+
+        Write(std::move(key), std::move(value));


Great catch, adding NUL checks in HTTPHeaders::Read() and HTTPRequest::LoadControlData() and covered in unit tests.

pinheadmz · 2026-04-30T15:41:51Z

Moving to #35182 see you there! <3

pinheadmz mentioned this pull request Mar 13, 2025

Split CConnman #30988

Draft

pinheadmz marked this pull request as draft March 13, 2025 19:37

DrahtBot added the CI failed label Mar 13, 2025

laanwj added the RPC/REST/ZMQ label Mar 14, 2025

DrahtBot mentioned this pull request Mar 14, 2025

i2p: make a time gap between creating transient sessions and using them #32065

Open

This was referenced Mar 14, 2025

net: replace manual reference counting of CNode with shared_ptr #32015

Open

http: Make server shutdown more robust #31929

Closed

This was referenced Mar 14, 2025

[IBD] specialize block serialization #31868

Draft

rpc: add cpu_load to getpeerinfo #31672

Open

refactor: Use std::span over Span #31519

Merged

net: option to disallow v1 connection on ipv4 and ipv6 peers #30951

Open

This was referenced Mar 14, 2025

p2p: Fill reconciliation sets (Erlay) attempt 2 #30116

Closed

kernel, refactor: return error status on all fatal errors #29700

Draft

rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' #29418

Open

pinheadmz force-pushed the http-rewrite-13march2025 branch 2 times, most recently from 640f38f to 49784b3 Compare March 18, 2025 19:44

Bitcoin Dev and others added 26 commits April 30, 2026 10:03

test: cover common HTTP attacks and common malformed requests

4ee40f5

http: enclose libevent-dependent code in a namespace

7dbf554

This commit is a no-op to isolate HTTP methods and objects that depend on libevent. Following commits will add replacement objects and methods in a new namespace for testing and review before switching over the server.

http: Implement HTTPHeaders class

a6cbbcb

see: https://www.rfc-editor.org/rfc/rfc2616#section-4.2 https://www.rfc-editor.org/rfc/rfc7231#section-5 https://www.rfc-editor.org/rfc/rfc7231#section-7 https://httpwg.org/specs/rfc9111.html#header.field.definitions

HTTPServer: implement and test AcceptConnection()

55845da

AcceptConnection() is mostly copied from CConmann in net.cpp and then modernized. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

HTTPServer: generate sequential Ids for each newly accepted connection

ecdab4d

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

http: Introduce HTTPClient class

f86f584

HTTPserver: support "chunked" Transfer-Encoding

ded8161

HTTPServer: disconnect clients

ed7c833

Allow http workers to send data optimistically as an optimization

19314da

define HTTP request methods at module level outside of class

21102c5

This is a refactor to prepare for matching the API of HTTPRequest definitions in both namespaces http_bitcoin and http_libevent. In particular, to provide a consistent return type for GetRequestMethod() in both classes.

Add helper methods to HTTPRequest to match original API

9e1a83d

These methods are called by http_request_cb() and are present in the original http_libevent::HTTPRequest.

HTTPServer: implement control methods to match legacy API

f4e8317

HTTPServer: disconnect after idle timeout (-rpcservertimeout)

c5acd11

http: switch servers from libevent to bitcoin

dcce3b4

fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest

e327bac

http: remove libevent usage from this subsystem

a69a2e5

logging: remove libevent category

91b2225

pinheadmz commented Apr 30, 2026

View reviewed changes

pinheadmz mentioned this pull request Apr 30, 2026

Replace libevent with our own HTTP and socket-handling implementation #35182

Merged

DrahtBot mentioned this pull request May 1, 2026

logging: replace -loglevel with -trace, expose trace logging via RPC #34038

Open

Uh oh!

Conversation

pinheadmz commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fuzz testing

libfuzzer

fuzzamoto

Integration testing:

Uh oh!

DrahtBot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

DrahtBot commented Mar 13, 2025

Uh oh!

laanwj commented Mar 14, 2025

Uh oh!

vasild commented Mar 14, 2025

Uh oh!

fjahr commented Mar 14, 2025

Uh oh!

pinheadmz commented Mar 15, 2025

Uh oh!

vasild commented Mar 19, 2025

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Fix conflicts from #32297 and collapse merged commits from #34905 #34772 and #35086.

Address review from @vasild and @hodlinator

Re: comment #32061 (comment)

Re: comment #32061 (comment)

Re: comment #32061 (comment)

Address review from @b-l-u-e and @janb84

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Apr 30, 2026

Moving to #35182 see you there! <3

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

pinheadmz commented Mar 13, 2025 •

edited

Loading

DrahtBot commented Mar 13, 2025 •

edited

Loading