[draft] 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 consumes the Sockman class introduced in that PR. The p2p / Connman rebase isn’t needed for HTTP and therefore this branch could be refactored to only include sockman.{h,cpp}.

Commit strategy:

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.

Currently just opening as a draft for CI testing.

TODO:

Decide what to do about Sockman (merge #30988 or not)
Test against as many bitcoin HTTP client libraries as possible (i.e. avoid another #31039)

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

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:

#32073 (net: Block v2->v1 transport downgrade if !fNetworkActive by hodlinator)
#32065 (i2p: make a time gap between creating transient sessions and using them by vasild)
#32015 (net: replace manual reference counting of CNode with shared_ptr by vasild)
#31929 (http: Make server shutdown more robust by hodlinator)
#31672 (rpc: add cpu_load to getpeerinfo by vasild)
#30951 (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28584 (Fuzz: extend CConnman tests by vasild)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
#27731 (Prevent file descriptor exhaustion from too many RPC calls by fjahr)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading 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

net: separate the listening socket from the permissions

They were coupled in `struct ListenSocket`, but the socket belongs to
the lower level transport protocol, whereas the permissions are specific
to the higher Bitcoin P2P protocol.

9e37c00ddb

net: drop CConnman::ListenSocket

Now that `CConnman::ListenSocket` is a `struct` that contains only one
member variable of type `std::shared_ptr<Sock>`, drop `ListenSocket` and
use `shared_ptr` directly.

Replace the vector of `ListenSocket` with a vector of `shared_ptr`.

a044553414

net: split CConnman::BindListenPort() off CConnman

Introduce a new low-level socket managing class `SockMan`
and move the `CConnman::BindListenPort()` method to it.

fdbc6e8cd4

style: modernize the style of SockMan::BindListenPort()

It was copied verbatim from `CConnman::BindListenPort()` in the previous
commit. Modernize its variables and style and log the error messages
from the caller. Also categorize the informative messages to the "net"
category because they are quite specific to the networking layer.

8bba43bd3e

net: split CConnman::AcceptConnection() off CConnman

Move the `CConnman::AcceptConnection()` method to `SockMan` and split
parts of it:
* the flip-to-CJDNS part: to just after the `AcceptConnection()` call
* the permissions part: at the start of `CreateNodeFromAcceptedSocket()`

c80f908ce9

style: modernize the style of SockMan::AcceptConnection() 7be3a45c22

net: move the generation of ids for new nodes from CConnman to SockMan

Move `CConnman::GetNewNodeId()` to `SockMan::GetNewId()`. Avoid using
the word "node" because that is too specific for `CConnman`.

cb8e857d7c

net: move CConnman-specific parts away from ThreadI2PAcceptIncoming()

CConnman-specific or in other words, Bitcoin P2P specific. Now
the `ThreadI2PAcceptIncoming()` method is protocol agnostic and
can be moved to `SockMan`.

27e24113dc

net: move I2P-accept-incoming code from CConnman to SockMan 82258ce5f8

net: index nodes in CConnman by id

Change `CConnman::m_nodes` from `std::vector<CNode*>` to
`std::unordered_map<NodeId, CNode*>` because interaction
between `CConnman` and `SockMan` is going to be based on
`NodeId` and finding a node by its id would better be fast.

Change `PeerManagerImpl::EvictExtraOutboundPeers()` to account for nodes
no longer always being in order of id. The old code would have failed to
update `next_youngest_peer` correctly if `CConnman::m_nodes` hadn't
always had nodes in ascending order of id.

During fuzzing make sure that we don't generate duplicate `CNode` ids.
The easiest way to do that is to use sequential ids.

As a nice side effect the existent search-by-id operations in
`CConnman::AttemptToEvictConnection()`,
`CConnman::DisconnectNode()` and
`CConnman::ForNode()` now become `O(1)` (were `O(number of nodes)`),
as well as the erase in `CConnman::DisconnectNodes()`.

8f14eb6ab0

net: isolate P2P specifics from GenerateWaitSockets()

Move the parts of `CConnman::GenerateWaitSockets()` that are specific to
the Bitcoin-P2P protocol to dedicated methods:
`ShouldTryToSend()` and `ShouldTryToRecv()`.

This brings us one step closer to moving `GenerateWaitSockets()` to the
protocol agnostic `SockMan` (which would call `ShouldTry...()` from
`CConnman`).

756d8cf6fa

net: isolate P2P specifics from SocketHandlerConnected() and ThreadSocketHandler()

Move some parts of `CConnman::SocketHandlerConnected()` and
`CConnman::ThreadSocketHandler()` that are specific to the Bitcoin-P2P
protocol to dedicated methods:
`EventIOLoopCompletedForOne(id)` and
`EventIOLoopCompletedForAll()`.

This brings us one step closer to moving `SocketHandlerConnected()` and
`ThreadSocketHandler()` to the protocol agnostic `SockMan` (which would
call `EventIOLoopCompleted...()` from `CConnman`).

03a5596c96

net: isolate all remaining P2P specifics from SocketHandlerConnected()

Introduce 4 new methods for the interaction between `CConnman` and
`SockMan`:

* `EventReadyToSend()`:
  called when there is readiness to send and do the actual sending of data.

* `EventGotData()`, `EventGotEOF()`, `EventGotPermanentReadError()`:
  called when the corresponing recv events occur.

These methods contain logic that is specific to the Bitcoin-P2P protocol
and move it away from `CConnman::SocketHandlerConnected()` which will
become a protocol agnostic method of `SockMan`.

Also, move the counting of sent bytes to `CConnman::SocketSendData()` -
both callers of that method called `RecordBytesSent()` just after the
call, so move it from the callers to inside
`CConnman::SocketSendData()`.

97d4286916

net: split CConnman::ConnectNode()

Move the protocol agnostic parts of `CConnman::ConnectNode()` into
`SockMan::ConnectAndMakeId()` and leave the Bitcoin-P2P specific
stuff in `CConnman::ConnectNode()`.

Move the protocol agnostic `CConnman::m_unused_i2p_sessions`, its mutex
and `MAX_UNUSED_I2P_SESSIONS_SIZE` to `SockMan`.

Move `GetBindAddress()` from `net.cpp` to `sockman.cpp`.

a78fae491a

net: tweak EventNewConnectionAccepted()

Move `MaybeFlipIPv6toCJDNS()`, which is Bitcoin P2P specific from the
callers of `CConnman::EventNewConnectionAccepted()` to inside that
method.

Move the IsSelectable check, the `TCP_NODELAY` option set and the
generation of new connection id out of
`CConnman::EventNewConnectionAccepted()` because those are protocol
agnostic. Move those to a new method `SockMan::NewSockAccepted()` which
is called instead of `CConnman::EventNewConnectionAccepted()`.

f8af2a481b

net: move sockets from CNode to SockMan

Move `CNode::m_sock` and `CNode::m_i2p_sam_session` to `SockMan::m_connected`.
Also move all the code that handles sockets to `SockMan`.

`CNode::CloseSocketDisconnect()` becomes
`CConnman::MarkAsDisconnectAndCloseConnection()`.

`CConnman::SocketSendData()` is renamed to
`CConnman::SendMessagesAsBytes()` and its sockets-touching bits are moved to
`SockMan::SendBytes()`.

`CConnman::GenerateWaitSockets()` goes to
`SockMan::GenerateWaitSockets()`.

`CConnman::ThreadSocketHandler()` and
`CConnman::SocketHandler()` are combined into
`SockMan::ThreadSocketHandler()`.

`CConnman::SocketHandlerConnected()` goes to
`SockMan::SocketHandlerConnected()`.

`CConnman::SocketHandlerListening()` goes to
`SockMan::SocketHandlerListening()`.

45ee18defd

net: move-only: improve encapsulation of SockMan

`SockMan` members

`AcceptConnection()`
`NewSockAccepted()`
`GetNewId()`
`m_i2p_sam_session`
`m_listen`

are now used only by `SockMan`, thus make them private.

f2f9ff9823

pinheadmz force-pushed on Mar 20, 2025

DrahtBot removed the label Needs rebase on Mar 20, 2025

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.

8a946893f2

test: cover "chunked" Transfer-Encoding 712b59d658

string: implement ParseUInt64Hex da7b092d11

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_()

441c336592

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

6596c47382

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

c74797303c

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.

c4d6adfe29

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

3643702073

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

518f3c7fcf

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

60ffc6b82e

http: Begin implementation of HTTPClient and HTTPServer e40caf7dc8

http: read requests from connected clients bdc7f630f4

http: support "chunked" Transfer-Encoding 451b5889ca

http: compose and send replies to connected clients 52d0246ec4

http: disconnect clients 40ed7b2470

Allow http workers to send data optimistically as an optimization b698476045

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.

ee8f5cface

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.

841f4a84e8

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.

b84c89d487

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.

cf13c1a4ab

http: implement new server control methods to match legacy API 5122c74326

http: disconnect after idle timeout (-rpcservertimeout) c3d3798e7f

use CScheduler for HTTPRPCTimer

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

460db952bc

http: switch servers from libevent to bitcoin 5a9719d930

fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest 71915724b4

httpserver: delete libevent! 04fe4baf5e

pinheadmz force-pushed on Mar 21, 2025

DrahtBot added the label Needs rebase on Mar 24, 2025

DrahtBot commented at 10:30 pm on March 24, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Code Coverage & Benchmarks

Reviews

Conflicts