Replace libevent with our own HTTP and socket-handling implementation

pinheadmz commented at 7:32 pm on March 13, 2025: member

This is a major component of removing libevent as a dependency of the project

It is based on #30988 but only in the sense that it copies the Sockman class introduced in that PR. The p2p / Connman refactor isn’t needed for HTTP and therefore this branch can be reviewed and merged independently of the p2p changes.

Commit strategy:

Import sockman.{h,cpp} from #30988
Assert current behavior of HTTP with additional functional tests, including copying from libevent’s tests
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 (the namespace manages duplicate HTTPRequest definitions, etc)
Switch bitcoind from the libevent server to the new server
Clean up (delete http_libevent)

I am currently seeing about a 10% speed up in the functional tests on my own arm/macos machine.

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 commented at 7:32 pm on March 13, 2025: contributor

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, vasild, fjahr, w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
#31929 (http: Make server shutdown more robust by hodlinator)
#31672 (rpc: add cpu_load to getpeerinfo by vasild)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#27731 (Prevent file descriptor exhaustion from too many RPC calls by fjahr)
#26022 (Add util::ResultPtr class by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages 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.

pinheadmz marked this as a draft on Mar 13, 2025

DrahtBot added the label CI failed on Mar 13, 2025

DrahtBot commented at 8:29 pm on March 13, 2025: contributor

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

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 added the label RPC/REST/ZMQ on Mar 14, 2025

laanwj commented at 12:52 pm on March 14, 2025: member

Concept ACK, nice work

vasild commented at 1:57 pm on March 14, 2025: contributor

Concept ACK

fjahr commented at 8:29 pm on March 14, 2025: contributor

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 commented at 0:03 am on March 15, 2025: member

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.

pinheadmz force-pushed on Mar 18, 2025

vasild commented at 12:22 pm on March 19, 2025: contributor

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.

:100:

pinheadmz force-pushed on Mar 19, 2025

DrahtBot added the label Needs rebase on Mar 20, 2025

pinheadmz force-pushed on Mar 20, 2025

DrahtBot removed the label Needs rebase on Mar 20, 2025

pinheadmz force-pushed on Mar 21, 2025

DrahtBot added the label Needs rebase on Mar 24, 2025

pinheadmz force-pushed on Mar 29, 2025

DrahtBot removed the label CI failed on Mar 30, 2025

DrahtBot removed the label Needs rebase on Mar 30, 2025

pinheadmz force-pushed on Mar 31, 2025

pinheadmz commented at 10:18 am on March 31, 2025: member

I rebased this branch on a single squashed commit from #30988 essentially just cherry-picking sockman.{h,cpp} by @vasild and leaving out the p2p refactor. This will make rebase maintenance on master a lot easier by reducing conflicting scope, and hopefully also makes review easier. It also means to some extent this PR can be merged independently of #30988, and also gives @theuni some room to rewrite a specific HTTP sockman if a more efficient purpose-focused module can be written. (Will update PR description in a moment)

I’ve finally gotten all CI to pass so I’m going to mark this PR as ready for review as I move on to integration testing with all the bitcoin client libraries I can find!

pinheadmz marked this as ready for review on Mar 31, 2025

pinheadmz renamed this:
~~[draft] Replace libevent with our own HTTP and socket-handling implementation~~
Replace libevent with our own HTTP and socket-handling implementation
on Mar 31, 2025

pinheadmz force-pushed on Apr 3, 2025

pinheadmz commented at 7:14 pm on April 3, 2025: member

rebase to a225633e34 includes a new test for “pipelining” HTTP requests (thanks @theuni for pointing out this oversight) and also adds a queue of requests to each HTTPClient to ensure requests are processed in series, in the order they were received.

Introduce a new low-level socket managing class `SockMan` cfe5eba446

test: cover http pipelining 00d18bbe09

test: cover -rpcservertimeout

Testing this requires adding an option to TestNode to force
the test framework to establish a new HTTP connection for
every RPC. Otherwise, attempting to reuse a persistent connection
would cause framework RPCs during startup and shutdown to fail.

5ec06529bb

test: cover "chunked" Transfer-Encoding f259121f30

string: implement ParseUInt64Hex 4c83785d1d

string: add CaseInsensitiveComparator

https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
Field names in HTTP headers are case-insensitive. This
comparator will be used in the headers map to search by key.
In libevent these are compared in lowercase:
  evhttp_find_header()
  evutil_ascii_strcasecmp()
  EVUTIL_TOLOWER_()

144a777f86

time: implement and test RFC7231 timestamp string

HTTP 1.1 responses require a timestamp header with a
specific format, specified in:
https://www.rfc-editor.org/rfc/rfc7231#section-7.1.1.1

aeb8352a9d

string: add LineReader

This is a helper struct to parse HTTP messages from data in buffers
from sockets. HTTP messages begin with headers which are
CRLF-terminated lines (\n or \r\n) followed by an arbitrary amount of
body data. Whitespace is trimmed from the field lines but not the body.

https://httpwg.org/specs/rfc9110.html#rfc.section.5

a1e151c774

http: enclose libevent-dependent code in a namespace

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.

12bd25e2b5

http: Implement HTTPHeaders class

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

70d003ca10

http: Implement HTTPResponse class

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

1b14d00a57

http: Implement HTTPRequest class

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

8a933646ac

http: Begin implementation of HTTPClient and HTTPServer 8424daa101

http: read requests from connected clients 482382bd14

http: support "chunked" Transfer-Encoding ace3e198d7

http: compose and send replies to connected clients 73c3c2e3d3

http: disconnect clients e2b5a3fea5

Allow http workers to send data optimistically as an optimization d0224eecde

http: use a queue to pipeline requests from each connected client

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.

cd059b6e14

define HTTP request methods at module level outside of class

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.

90761d5026

Add helper methods to HTTPRequest to match original API

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

b828fa1e29

refactor: split http_request_cb into libevent callback and dispatch

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.

3adcd9617e

refactor: split HTTPBindAddresses into config parse and libevent setup

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.

7b205a21a1

http: implement new server control methods to match legacy API e3a94d93c4

http: disconnect after idle timeout (-rpcservertimeout) 196698c43e

use CScheduler for HTTPRPCTimer

This removes the dependency on libevent for scheduled events,
like re-locking a wallet some time after decryption.

a792549519

http: switch servers from libevent to bitcoin 8122a362bf

fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest 53ac965791

httpserver: delete libevent! 6a6285d268

pinheadmz force-pushed on Apr 3, 2025

DrahtBot added the label CI failed on Apr 3, 2025

DrahtBot removed the label CI failed on Apr 4, 2025

w0xlt commented at 7:06 pm on April 17, 2025: contributor

Concept ACK.

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

Integration testing:

Code Coverage & Benchmarks

Reviews

Conflicts