Remove libevent as a dependency (HTTP / cli / torcontrol)

pinheadmz commented at 3:25 pm on October 31, 2024: member

Immediate advantages

UNIX domain sockets for JSON-RPC

Old feature request: #5029
UNIX sockets have been added to ZMQ and Tor proxy already
UNIX sockets with libevent doesn’t work

Remove a dependency

libevent hasn’t had a release since 2020
The xz utils backdoor raised some concern among contributors in the 2024/04/18 IRC meeting

Current libevent usage in the project

See this report by @fjahr. A few of these have already been addressed, like #29904

Strategies

I am starting by replacing the HTTP server, currently used for both REST and JSON-RPC and implemented with libevent. Nothing is quite ready for review yet but I have:

https://github.com/pinheadmz/bitcoin/tree/http-netbase-cmake

Still lots of TODOs and new tests to write but works and passes all current tests. In this branch I added a new http.cpp module with socket-handling code largely copied from ConnMan. This branch needs a lot of clean up before presenting as a PR with clear commits.

https://github.com/pinheadmz/bitcoin/tree/http-rewrite

This branch I am trying to write much more cleanly for PR presentation. It is based on #30988 by @vasild which cleanly separates the generic socket handling logic from the bitcoin protocol logic. It would be especially useful if there was any more future need for additional servers in bitcoin (such as @Sjors recently withdrawn proposal to add StratumV2: https://github.com/Sjors/bitcoin/pull/68)

Looking for feedback

Is this something worth doing at all?
- We could vendor libevent instead of requiring it as a dependency, and fix UNIX sockets there
- There might be another C++ HTTP library we could import instead. I did shop around a bit but everything I found was just a bit too much.
If(1) do we want to share generic sock manager code or have it mostly duplicated between p2p and http?

hebasto commented at 3:36 pm on October 31, 2024: member

libevent hasn’t had a release since 2020

Moreover, the new non-released code breaks our fuzzing tests. See: #30096.

fanquake commented at 3:47 pm on October 31, 2024: member

Moreover, the new non-released code breaks our fuzzing tests. See: #30096.

I’m not so sure that’s relevant. I think the conclusion in that issue was that vcpkg should probably stop shipping unstable/unreleased code, and I don’t think we care about fuzzing unreleased versions of dependencies. It’s less suprising that code which is still in development / might have API or other breaking changes we haven’t accounted for might not work correctly.

vasild commented at 1:39 pm on November 4, 2024: contributor

Concept ACK on removing libevent.

In addition to the motivation from the OP, I would add that libevent is non-intuitive and difficult to work with (at least for me, this could be subjective). It was related to a remote crash via RPC: https://github.com/bitcoin/bitcoin/pull/27468

fjahr commented at 11:17 pm on November 6, 2024: contributor

Concept ACK on removing libevent and replacing the HTTP server with our own code. FWIW, I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.

vasild commented at 7:42 am on November 7, 2024: contributor

I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.

:heart:

poke me for review

Reliestaff commented at 8:42 am on November 7, 2024: none

Agree

On Thu, Nov 7, 2024, 2:46 AM Vasil Dimov @.***> wrote:

I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.

❤️

poke me for review

— Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461527876, or unsubscribe https://github.com/notifications/unsubscribe-auth/BDSRHFZPVAHZK2F7J7W2T5LZ7MLGLAVCNFSM6AAAAABQ6RHYAKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRRGUZDOOBXGY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

pinheadmz commented at 2:30 pm on November 7, 2024: member

There is a Signal working group for this project as well if anyone wants in lemme know

JeremyRubin commented at 1:34 am on December 25, 2024: contributor

approach ack on removing libevent and having it operate over unix socket – http functionality / REST can be provided via a proxy server.

pinheadmz commented at 8:24 pm on January 10, 2025: member

I found another edge case in libevent regarding URI handling, it does not decode %XX characters before parsing.

This is in libevent’s evhttp_uri_parse() which is called by Bitcoin’s GetQueryParameterFromUri()

Example, the URI: /rest/endpoint/someresource.json%3Fp1%3Dv1%26p2%3D100%25 decodes to /rest/endpoint/someresource.json?p1=v1&p2=100%

It will be interpreted by libevent (and therefore bitcoin core, currently) as one long path with no query parameters.

Both Python’s urlparse() and JavaScript’s decodeURIComponent() will decode the %XX characters before parsing and identify the query, with two parameters, as separate from the path

pablomartin4btc commented at 7:20 pm on January 12, 2025: member

I’ve checked that and I see where the problem is…

If you run the following it works correctly (using/ changing src/test/httpserver_tests.cpp):

0    uri = "/rest/endpoint/someresource.json?p1=v1%20&p1=v2";
1    BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p1").value(), "v1 ");

0./build/src/test/test_bitcoin --log_level=all --run_test=httpserver_tests
1...
2./test/httpserver_tests.cpp(52): info: check GetQueryParameterFromUri(uri.c_str(), "p1").value() == "v1 " has passed
3...

So evhttp_uri_parse() works as expected(?) as soon the “encoded” char is in the variable’s value (e.g. the following won’t work: “/rest/endpoint/someresource.json%3Fp1=v1%20&p1=v2”).

For the case where the “encoded” chars are among the path, we’d need to run the evhttp_uridecode():

0    uri = "/rest/endpoint/someresource.json%3Fp1%3Dv1%26p2%3D100%2525";
1    char *decoded_path = evhttp_uridecode(uri.c_str(), 0, NULL); // Use c_str() here
2    BOOST_REQUIRE(decoded_path != nullptr); // Ensure decoding succeeded
3
4    std::cout << "Decoded path: " << decoded_path << std::endl;
5
6    // Test query parameter extraction
7    BOOST_CHECK_EQUAL(GetQueryParameterFromUri(decoded_path, "p2").value(), "100%");
8
9    free(decoded_path); // Don't forget to free the decoded string

But as you can see the “encoded” path should be “/rest/endpoint/someresource.json%3Fp1%3Dv1%26p2%3D100%2525” otherwise the libevent parse will fail because as in your example the “/rest/endpoint/someresource.json?p1=v1&p2=100%” is invalid (RFC 3986:A % character must always be followed by two valid hexadecimal digits (0–9, A–F, a–f) that represent a byte value in hexadecimal notation). evhttp_uridecode() doesn’t check if the encoded char is within the path or in the variable’s value so perhaps we need to handle this special use case somehow.

pablomartin4btc commented at 3:42 pm on January 13, 2025: member

btw ACK on dependencies removal.

pinheadmz commented at 6:50 pm on January 13, 2025: member

@pablomartin4btc thanks, I added this test to my branch:

0
1    // Multiple parameters, some characters encoded
2    uri = "/rest/endpoint/someresource.json?p1=v1%20&p2=100%25";
3    BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p1").value(), "v1 ");
4    BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p2").value(), "100%");

You’re totally right, libevent decodes %XX but only after it has parsed the URI. Meaning the special characters ? & and = (probably also #) can’t be encoded the way we’re currently using libevent.

laanwj added the label P2P on Jan 14, 2025

laanwj added the label RPC/REST/ZMQ on Jan 14, 2025

Remove libevent as a dependency (HTTP / cli / torcontrol) #31194