torcontrol: Remove libevent usage #34158

fjahr · 2025-12-27T14:20:31Z

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 · 2025-12-27T14:20:37Z

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  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34242 (Prepare string and net utils for future HTTP operations by pinheadmz)
#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 · 2025-12-27T14:35:33Z

Concept ACK

pinheadmz · 2025-12-27T14:38:52Z

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

DrahtBot · 2025-12-27T15:32:27Z

🚧 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.}

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.

fjahr · 2025-12-27T15:35:22Z

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 · 2025-12-27T16:24:13Z

Concept ACK

waketraindev · 2025-12-28T15:33:23Z

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.

janb84

Concept ACK 8d6f1b0

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.

src/torcontrol.cpp

DrahtBot · 2025-12-28T22:02:16Z

🚧 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.}

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.

fjahr · 2025-12-28T22:16:13Z

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 · 2025-12-28T23:14:40Z

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

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

fjahr · 2025-12-31T17:06:21Z

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 · 2025-12-31T18:18:42Z

🚧 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).}

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.

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

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

fjahr mentioned this pull request Dec 27, 2025

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

Open

6 tasks

fjahr force-pushed the 2025-12-torcontrol-take-3 branch from 3ec2c0e to a830323 Compare December 27, 2025 15:31

DrahtBot added the CI failed label Dec 27, 2025

DrahtBot mentioned this pull request Dec 28, 2025

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

Open

fjahr force-pushed the 2025-12-torcontrol-take-3 branch from a830323 to 872feec Compare December 28, 2025 14:08

DrahtBot removed the CI failed label Dec 28, 2025

janb84 reviewed Dec 28, 2025

View reviewed changes

src/torcontrol.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the 2025-12-torcontrol-take-3 branch 2 times, most recently from 67f9f14 to 4966648 Compare December 28, 2025 22:01

DrahtBot added the CI failed label Dec 28, 2025

DrahtBot removed the CI failed label Dec 28, 2025

Matthew Zipkin and others added 4 commits December 31, 2025 18:05

refactor: Use constexpr in torcontrol where possible

188e630

refactor: Modernize member variable names in torcontrol

3a1f8c0

refactor: Get rid of unnecessary newlines in logs

c155617

fjahr force-pushed the 2025-12-torcontrol-take-3 branch from 4966648 to 122ff0a Compare December 31, 2025 17:05

DrahtBot added the CI failed label Dec 31, 2025

torcontrol: Remove libevent usage

c675b6f

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

fjahr added 2 commits December 31, 2025 22:04

fuzz: Improve torcontrol fuzz test

197dbe9

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

test: Add simple functional test for torcontrol

3de90a1

fjahr force-pushed the 2025-12-torcontrol-take-3 branch from 122ff0a to 3de90a1 Compare December 31, 2025 21:04

DrahtBot removed the CI failed label Dec 31, 2025

pinheadmz mentioned this pull request Jan 9, 2026

Prepare string and net utils for future HTTP operations #34242

Open

torcontrol: Remove libevent usage #34158

Are you sure you want to change the base?

torcontrol: Remove libevent usage #34158

Uh oh!

Conversation

fjahr commented Dec 27, 2025

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

sedited commented Dec 27, 2025

Uh oh!

pinheadmz commented Dec 27, 2025

Uh oh!

DrahtBot commented Dec 27, 2025

Uh oh!

fjahr commented Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablomartin4btc commented Dec 27, 2025

Uh oh!

waketraindev commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Dec 28, 2025

Uh oh!

fjahr commented Dec 28, 2025

Uh oh!

waketraindev commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Dec 31, 2025

Uh oh!

DrahtBot commented Dec 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Dec 27, 2025 •

edited

Loading

fjahr commented Dec 27, 2025 •

edited

Loading

waketraindev commented Dec 28, 2025 •

edited

Loading

waketraindev commented Dec 28, 2025 •

edited

Loading