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, pablomartin4btc, janb84, waketraindev
Stale ACK	pinheadmz

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.

pinheadmz

ACK 3de90a1

Reviewed each commit, built and ran tests on arm64/macos.

Ran on mainnet with -onlynet=onion and observed tor connections in and out. Monitored bitcoind logs with -debug=tor and tor daemon logs for errors.

I also used wireshark to capture torcontrol packets on port 9051 to compare this branch against release 30.2 and observed identical behavior 👍

Checked the exponential retry backoff by killing the tor daemon process while bitcoind was running. Restarted tor after a few minutes and bitcoind continued as expected.

This is a very nice simplification of torcontrol and also demonstrates how we can replace libevent in bitcoin-cli as well ;-).

Show Signature

pinheadmz's public key is on openpgp.org

Bitcoin Core client v30.99.0-3de90a1159e6 - server 70016/Satoshi:30.99.0/ - services wl2

<->   type   net   serv  v  mping   ping send recv  txn  blk  hb addrp addrl  age id address                                                             version
 in        onion    wl2  2   1325   1325   46   46                   1          0  2 127.0.0.1:59473                                                     70016/Satoshi:30.0.0/
out   full onion  nwcl2  2    931    931    4    0         0      1000          1  0 wffaznxih3nah5tbjpiifbn5v3fzmr5tqzcinacr3cjtnags45owvaqd.onion:8333 70016/Satoshi:30.0.0/
out   full onion  nbwl2  2    934    934    4    0         0      1001          0  1 uathi7edrzgqsby7dikuqe7ehwpwjem2cy4mpsbahrsrtchjdymufxyd.onion:8333 70016/Satoshi:29.0.0/
out   full onion    nwl  1   2810   2810   11    0                1000          0  3 t423yolpdj5udh2yyemopmjtenmmwtfx3rxcdtneda5g2rw6isnjo7id.onion:8333 70016/Satoshi:25.0.0/
                               ms     ms  sec  sec  min  min                  min

        onion   total   block
in          1       1
out         3       3       0
total       4       4

Local addresses
zai7kzajktbkqom3edw45zyxnn6e5z5t45d2m7dhmhilgpkz2l3frmad.onion     port   8333    score      4

pinheadmz · 2026-01-14T16:16:26Z

src/torcontrol.cpp

 *
 */
-static std::vector<uint8_t> ComputeResponse(const std::string &key, const std::vector<uint8_t> &cookie,  const std::vector<uint8_t> &clientNonce, const std::vector<uint8_t> &serverNonce)
+static std::vector<uint8_t> ComputeResponse(const std::string &key, const std::vector<uint8_t> &cookie,  const std::vector<uint8_t> &client_nonce, const std::vector<uint8_t> &serverNonce)


3a1f8c0

why change client_nonce but not serverNonce? I'm guessing because there is a m_client_nonce at the call site, but the member property isn't being explicitly handled here.

Yeah, I was initially focussing on the member variables in order to not let this get out of hand but the param here was touched too by accident. Doesn't seem too bad to change the serverNonce too as well as the other local variables in this function, so taking care of all of them now.

pinheadmz · 2026-01-15T19:45:20Z

src/torcontrol.cpp

+    if (m_recv_buffer.size() > MAX_LINE_LENGTH) {
+        LogWarning("tor: Disconnecting because MAX_LINE_LENGTH exceeded");
+        return false;
+    }


c675b6f

We won't ever get multiple lines in the receive buffer?

Right, that's a good catch. If the processing is too slow this could fail unnecessarily and LineReader should catch this already fine, so I am removing this.

pinheadmz · 2026-01-15T19:51:43Z

src/torcontrol.cpp

+    // TODO: Maybe could give this as an option to LineReader instead
+    auto last_newline = std::find(m_recv_buffer.rbegin(), m_recv_buffer.rend(), std::byte{'\n'});
+    if (last_newline == m_recv_buffer.rend()) return true;
+    size_t complete = last_newline.base() - m_recv_buffer.begin();
+    util::LineReader reader({m_recv_buffer.data(), complete}, MAX_LINE_LENGTH);


c675b6f

I'm still learning how torcontrol protocol works all together, but without complete context it seems to me that all this could be simplified:

util::LineReader reader(m_recv_buffer, MAX_LINE_LENGTH);

You shouldn't have to find '\n', that's LineReader's job ...?

torcontrol unit and functional tests still pass like this.

The idea here is that we may receive an incomplete line in the stream just because of packets arriving that way and if we would ask the LineReader for it we would receive it but couldn't do anything with it. That's why I am looking for the last \n and then only process the buffer up until that point. But this is something that could be done by LineReader, too. So maybe there could be a bool param and then LineReader would only return the lines that end with \n but I didn't really spend time making up my mind if that is so much better. It looks cleaner here but this functionality would only have one user, so I don't know... Either way I will wait until LineReader is merged and then I might try to add the functionality there if you think it's better that way.

There should be a test for this though, currently the tests all send the data nice and complete.

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

Addressed feedback from @pinheadmz , thanks for the review!

EDIT: And also, I had actually lost sight of bitcoin-cli, I will look at that next to see what, if any, could be reusable from here to apply there.

fjahr · 2026-01-16T23:17:24Z

src/torcontrol.cpp

+    // TODO: Maybe could give this as an option to LineReader instead
+    auto last_newline = std::find(m_recv_buffer.rbegin(), m_recv_buffer.rend(), std::byte{'\n'});
+    if (last_newline == m_recv_buffer.rend()) return true;
+    size_t complete = last_newline.base() - m_recv_buffer.begin();
+    util::LineReader reader({m_recv_buffer.data(), complete}, MAX_LINE_LENGTH);


The idea here is that we may receive an incomplete line in the stream just because of packets arriving that way and if we would ask the LineReader for it we would receive it but couldn't do anything with it. That's why I am looking for the last \n and then only process the buffer up until that point. But this is something that could be done by LineReader, too. So maybe there could be a bool param and then LineReader would only return the lines that end with \n but I didn't really spend time making up my mind if that is so much better. It looks cleaner here but this functionality would only have one user, so I don't know... Either way I will wait until LineReader is merged and then I might try to add the functionality there if you think it's better that way.

There should be a test for this though, currently the tests all send the data nice and complete.

fjahr · 2026-01-17T14:37:03Z

src/torcontrol.cpp

+    if (m_recv_buffer.size() > MAX_LINE_LENGTH) {
+        LogWarning("tor: Disconnecting because MAX_LINE_LENGTH exceeded");
+        return false;
+    }


Right, that's a good catch. If the processing is too slow this could fail unnecessarily and LineReader should catch this already fine, so I am removing this.

fjahr · 2026-01-17T14:39:32Z

src/torcontrol.cpp

 *
 */
-static std::vector<uint8_t> ComputeResponse(const std::string &key, const std::vector<uint8_t> &cookie,  const std::vector<uint8_t> &clientNonce, const std::vector<uint8_t> &serverNonce)
+static std::vector<uint8_t> ComputeResponse(const std::string &key, const std::vector<uint8_t> &cookie,  const std::vector<uint8_t> &client_nonce, const std::vector<uint8_t> &serverNonce)


Yeah, I was initially focussing on the member variables in order to not let this get out of hand but the param here was touched too by accident. Doesn't seem too bad to change the serverNonce too as well as the other local variables in this function, so taking care of all of them now.

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 2 commits December 31, 2025 18:05

refactor: Use constexpr in torcontrol where possible

188e630

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

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

pinheadmz approved these changes Jan 15, 2026

View reviewed changes

DrahtBot requested review from janb84, pablomartin4btc and sedited January 15, 2026 20:24

fjahr force-pushed the 2025-12-torcontrol-take-3 branch from 3de90a1 to d3ce644 Compare January 17, 2026 14:25

fjahr added 6 commits January 17, 2026 15:33

refactor: Modernize member variable names in torcontrol

68c1e1d

refactor: Get rid of unnecessary newlines in logs

fc6b8b5

torcontrol: Remove libevent usage

732cdb1

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

fuzz: Improve torcontrol fuzz test

92ed172

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

test: Add simple functional test for torcontrol

9991aa8

test: Add test for partial message handling in torcontrol

2808981

fjahr force-pushed the 2025-12-torcontrol-take-3 branch from d3ce644 to 2808981 Compare January 17, 2026 14:36

DrahtBot added the CI failed label Jan 17, 2026

fjahr commented Jan 17, 2026

View reviewed changes

DrahtBot removed the CI failed label Jan 17, 2026

torcontrol: Remove libevent usage #34158

Are you sure you want to change the base?

torcontrol: Remove libevent usage #34158

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!

pinheadmz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

pinheadmz left a comment •

edited

Loading

fjahr left a comment •

edited

Loading