torcontrol: Remove libevent usage

fjahr commented at 2:20 pm on December 27, 2025: contributor

This is part of the effort to remove the libevent dependency from our code base: #31194

There is a dependency on #32061 but it only really needs one commit which is cherry-picked here in first position (add LineReader). I hope that a first chunk of that PR can be sliced off and reviewed independently so this PR here is not blocked by it.

The current approach tries to reuse existing code and follows roughly similar design decisions. It replaces the libevent-based async I/O with blocking I/O utilizing the existing Sock and CThreadInterrupt. The controller runs in a dedicated thread.

There are some optional code modernizations thrown in made along the way (namings, constexpr etc.). These are not strictly necessary but make the end result with the new code more consistent.

DrahtBot commented at 2:20 pm on December 27, 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/34158.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	sedited, pinheadmz, pablomartin4btc, janb84, waketraindev

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)

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.

sedited commented at 2:35 pm on December 27, 2025: contributor

Concept ACK

pinheadmz commented at 2:38 pm on December 27, 2025: member

concept ACK Quick review on github - where are you using LineReader?

fjahr force-pushed on Dec 27, 2025

DrahtBot added the label CI failed on Dec 27, 2025

DrahtBot commented at 3:32 pm on December 27, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross to arm64: https://github.com/bitcoin/bitcoin/actions/runs/20540187798/job/59003472023 LLM reason (✨ experimental): Compilation error in fuzz tests: undefined type ConnectionCB and invalid TorControlConnection initialization (nullptr) in torcontrol.cpp.

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.

fjahr commented at 3:35 pm on December 27, 2025: contributor

where are you using LineReader?

Heh, I did quite a bit of moving around after I got things working and seems like I picked from the branch where i didn’t use it instead of the one where I did at the end. Pushed the right code for it now, using it in ProcessBuffer.

Will work on fixing the fuzz test which I completely missed 🙃 That’s what’s failing the CI.

pablomartin4btc commented at 4:24 pm on December 27, 2025: member

Concept ACK

fjahr force-pushed on Dec 28, 2025

DrahtBot removed the label CI failed on Dec 28, 2025

waketraindev commented at 3:33 pm on December 28, 2025: contributor

Concept ACK

There’s some uncovered new code in the coverage report, https://corecheck.dev/bitcoin/bitcoin/pulls/34158 And some Sonarcloud warnings to check out.

in src/torcontrol.cpp:137 in 872feec3eb

146-    if (b_conn)
147-        bufferevent_free(b_conn);
148-    b_conn = nullptr;
149+    if (m_sock) {
150+        m_sock.reset();
151+    }

janb84 commented at 4:08 pm on December 28, 2025:

0        m_sock.reset();

NIT: I think you can remove the redundant check. calling reset() on a std::unique_ptr is safe even if the pointer is already null

fjahr commented at 9:57 pm on December 28, 2025:

done

janb84 commented at 4:19 pm on December 28, 2025: contributor

Concept ACK 8d6f1b02f5868d918bb72059eac3afb881ccafe3

PR removes LibEvent usages from TorControl and cleans/modernizes the code a bit where touched.

code review, build and test. I do not have sufficient exp. to say something about the fuzz improvement commit. Small NIT found while doing the code review.

fjahr force-pushed on Dec 28, 2025

DrahtBot added the label CI failed on Dec 28, 2025

DrahtBot commented at 10:02 pm on December 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20560088214/job/59049502835 LLM reason (✨ experimental): Python linting failed: an f-string had no placeholders (ruff warning F541) in test/functional/feature_torcontrol.py.

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.

fjahr commented at 10:16 pm on December 28, 2025: contributor

Addressed feedback.

There’s some uncovered new code in the coverage report, https://corecheck.dev/bitcoin/bitcoin/pulls/34158

The old code was largely uncovered as well but I have looked into it and added a simple functional test that provides some basic end-to-end coverage. I thought about unit tests too but I am not sure they provide much value aside from the functional and fuzz tests. But happy to be convinced otherwise if there are ideas for what they should cover.

And some Sonarcloud warnings to check out.

I don’t know what that is and the screenshot doesn’t allow me to see which lines are meant by the comment. Happy to look into them if you can transfer the comments matching to the correct lines here somehow.

waketraindev commented at 11:14 pm on December 28, 2025: contributor

I don’t know what that is and the screenshot doesn’t allow me to see which lines are meant by the comment. Happy to look into them if you can transfer the comments matching to the correct lines here somehow.

You can see the warnings on https://corecheck.dev/bitcoin/bitcoin/pulls/34158 scroll down the page, above benchmarks, below uncovered included code

DrahtBot removed the label CI failed on Dec 28, 2025

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

2ef96a51fe

refactor: Use constexpr in torcontrol where possible 188e63090d

refactor: Modernize member variable names in torcontrol 3a1f8c0947

refactor: Get rid of unnecessary newlines in logs c155617a2e

fjahr force-pushed on Dec 31, 2025

fjahr commented at 5:06 pm on December 31, 2025: contributor

You can see the warnings on https://corecheck.dev/bitcoin/bitcoin/pulls/34158 scroll down the page, above benchmarks, below uncovered included code

TIL, thanks. I addressed the ones that made sense to me.

DrahtBot added the label CI failed on Dec 31, 2025

DrahtBot commented at 6:18 pm on December 31, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross to arm64: https://github.com/bitcoin/bitcoin/actions/runs/20623563561/job/59230100443 LLM reason (✨ experimental): Compilation failed due to use of std::jthread not available in the target standard library (missing C++20 jthread support).

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.

torcontrol: Remove libevent usage

Replace libevent-based approach with using the Sock class and CThreadInterrupt.

c675b6f059

fuzz: Improve torcontrol fuzz test

Gets rid of the Dummy class and adds coverage of get_socks_cb.

197dbe910c

test: Add simple functional test for torcontrol 3de90a1159

fjahr force-pushed on Dec 31, 2025

DrahtBot removed the label CI failed on Dec 31, 2025

torcontrol: Remove libevent usage #34158

Code Coverage & Benchmarks

Reviews

Conflicts