torcontrol: Define tor reply code as const to improve our maintainability

eval-exec commented at 11:00 am on March 30, 2025: contributor

This PR want to:

replace tor repy code with const to improve out maintainability.
cherry-picked #31973 , add comment to explain Proxy credential randomization for Tor privacy

DrahtBot commented at 11:01 am on March 30, 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/32166.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hodlinator, laanwj

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:

#32176 (net: Prevent accidental circuit sharing when using Tor stream isolation by laanwj)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

eval-exec renamed this:
~~torcontrol: Define tor reply code as cosnt to improve readability) torcontrol: Define tor replay code as const to improve our maintainability~~
torcontrol: Define tor replay code as const to improve our maintainability
on Mar 30, 2025

eval-exec renamed this:
~~torcontrol: Define tor replay code as const to improve our maintainability~~
torcontrol: Define tor reply code as const to improve our maintainability
on Mar 30, 2025

eval-exec force-pushed on Mar 30, 2025

maflcko commented at 11:24 am on March 30, 2025: member

Could probably be cherry-picked into #31973, so that there is only a single pull with doc/refactor fixups of the same file?

eval-exec commented at 11:25 am on March 30, 2025: contributor

Could probably be cherry-picked into #31973, so that there is only a single pull with doc/refactor fixups of the same file?

Sure. Done

eval-exec closed this on Mar 30, 2025

eval-exec reopened this on Mar 30, 2025

fanquake requested review from laanwj on Mar 31, 2025

laanwj commented at 5:52 am on March 31, 2025: member

Could probably be cherry-picked into #31973, so that there is only a single pull with doc/refactor fixups of the same file?

Probably this was meant the other way around: cherry-pick this change into that PR, which already has review, to not have to open a new one. But this works too.

torcontrol: Define tor reply code as const to improve maintainability

Signed-off-by: Eval EXEC <execvy@gmail.com>

ec5c0b26ce

in src/torcontrol.cpp:56 in 9319f30211 outdated

52@@ -53,6 +53,9 @@ const std::string DEFAULT_TOR_CONTROL = "127.0.0.1:" + ToString(DEFAULT_TOR_CONT
53 static const int TOR_COOKIE_SIZE = 32;
54 /** Size of client/server nonce for SAFECOOKIE */
55 static const int TOR_NONCE_SIZE = 32;
56+//* Tor control reply code. Ref: https://spec.torproject.org/control-spec/replies.html */

laanwj commented at 6:18 am on March 31, 2025:

nit: Please use the same comment syntax as the other comments

eval-exec commented at 6:20 am on March 31, 2025:

Thank you, fixed.

eval-exec force-pushed on Mar 31, 2025

in src/torcontrol.cpp:409 in f31ce35966 outdated

402@@ -400,7 +403,11 @@ void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlRe
403 
404     Assume(resolved.IsValid());
405     LogDebug(BCLog::TOR, "Configuring onion proxy for %s\n", resolved.ToStringAddrPort());
406-    Proxy addrOnion = Proxy(resolved, true);
407+
408+    // With m_randomize_credentials = true, generates unique SOCKS credentials per proxy connection (e.g., Tor).
409+    // Prevents connection correlation and enhances privacy by forcing different Tor circuits.
410+    // Requires Tor's IsolateSOCKSAuth (default enabled) for effective isolation (see IsolateSOCKSAuth section in https://2019.www.torproject.org/docs/tor-manual.html.en).

laanwj commented at 6:47 am on March 31, 2025:

It’s strange that the only torproject.org link for the C Tor manual is a 2019 backup of the site, but haven’t found anything better either. The only up-to-date canonical source is the manual page, which isn’t particularly readable on the web https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt (nothing to do here really, just noting)

eval-exec commented at 6:52 am on March 31, 2025:

Yes the web url link is old, but the IsolateSOCKSAuth explaination content is same with https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt

laanwj commented at 7:02 am on March 31, 2025:

Oh yes the site is correct, i’m just a bit worried about it going offline at some point, so was trying to find a more up-to-date official link. But it doesn’t exist.

laanwj approved

laanwj commented at 6:49 am on March 31, 2025: member

Code review ACK f31ce35966bb84608938b0ba2272b415bcd42618

laanwj added the label P2P on Mar 31, 2025

in src/torcontrol.cpp:58 in f31ce35966 outdated

52@@ -53,6 +53,9 @@ const std::string DEFAULT_TOR_CONTROL = "127.0.0.1:" + ToString(DEFAULT_TOR_CONT
53 static const int TOR_COOKIE_SIZE = 32;
54 /** Size of client/server nonce for SAFECOOKIE */
55 static const int TOR_NONCE_SIZE = 32;
56+/** Tor control reply code. Ref: https://spec.torproject.org/control-spec/replies.html */
57+static const int TOR_REPLY_OK = 250;
58+static const int TOR_REPLY_UNRECOGNIZED = 510;

hodlinator commented at 12:28 pm on March 31, 2025:

nit: Others might disagree but I prefer grouping with an enum:

0enum {
1    TOR_REPLY_OK = 250,
2    TOR_REPLY_UNRECOGNIZED = 510,
3};

(Developer-notes prefer enum class but I think implicit conversion to int is more important in this case, and there is no risk of forgetting the non-existent enum name).

These constants are hardcoded in src/test/fuzz/torcontrol.cpp as well, but I think it’s okay to let that be.

If you decide to not convert them to enum, it would be nice to use constexpr on these new lines.

eval-exec commented at 1:16 pm on March 31, 2025:

I’m fine with either option; let the Bitcoin maintainers decide.

laanwj commented at 4:12 pm on March 31, 2025:

i considered commenting this, but didn’t because it’s a bit much for defining two response values. Seems the benefit of enum class would be to make sure they’re grouped by name TorReply::OK TorReply::UNRECOGNIZED?

hodlinator commented at 7:07 pm on March 31, 2025:

Seems the benefit of enum class would be to make sure they’re grouped by name TorReply::OK TorReply::UNRECOGNIZED?

Yes, but then one would have to do if (foo->bar == static_cast<int>(TorReply::OK)) which becomes a bit verbose.

laanwj commented at 7:10 am on April 1, 2025:

Oh, right, and enums are not good for types that might have values that are not specifically defined, might as well keep this as-is.

in src/torcontrol.cpp:410 in f31ce35966 outdated

402@@ -400,7 +403,11 @@ void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlRe
403 
404     Assume(resolved.IsValid());
405     LogDebug(BCLog::TOR, "Configuring onion proxy for %s\n", resolved.ToStringAddrPort());
406-    Proxy addrOnion = Proxy(resolved, true);
407+
408+    // With m_randomize_credentials = true, generates unique SOCKS credentials per proxy connection (e.g., Tor).
409+    // Prevents connection correlation and enhances privacy by forcing different Tor circuits.
410+    // Requires Tor's IsolateSOCKSAuth (default enabled) for effective isolation (see IsolateSOCKSAuth section in https://2019.www.torproject.org/docs/tor-manual.html.en).
411+    Proxy addrOnion = Proxy(resolved, /*_randomize_credentials*/ true);

hodlinator commented at 12:41 pm on March 31, 2025:

informational, related:

Still feels scary to me how _randomize_credentials=true leads to a predictably incrementing integer to be used for user/pass after 5eaaa83ac1f5eb525f93e2808fafd73f5ed97013. What if guarantees of the Tor API were misunderstood, or change in the future?

Guess it’s very unlikely to be a problem if these are only used to authenticate with the local SOCKS5 proxy and don’t reach any further.

hodlinator commented at 1:01 pm on March 31, 2025:

nit: Think clang-tidy will verify the name matches if you append = to the comment.

0    Proxy addrOnion = Proxy(resolved, /*_randomize_credentials=*/ true);

eval-exec commented at 1:13 pm on March 31, 2025:

Thank you.

laanwj commented at 1:25 pm on March 31, 2025:

The requirements for stream isolation are documented here: https://spec.torproject.org/socks-extensions.html#stream-isolation . Our case is “They are both SOCKS5 with USERNAME/PASSWORD authentication, using legacy isolation parameters, and they have identical usernames and identical passwords”. The <torS0X> extension is very recent so we don’t use that (yet?).

So using different credentials is enough, there is no requirement, nor even benefit for them to be random.

Although thinking of it, it’s possible to think of edge cases:

if you restart the daemon in quick succession, the counter resets to 0 so a circuit may be reused
multiple bitcoinds running in parallel might use each other’s circuits

So maybe it’s worth doing something else here, for example generate a unique prefix per bitcoind session startup (this would be once, so no need for performant randomness). But it’s out of scope for this PR.

laanwj commented at 1:26 pm on March 31, 2025:

Pretty sure it is was already validated, this is how we discovered that the parameter is _randomize_credentials not randomize_credentials (https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977739770).

laanwj commented at 1:28 pm on March 31, 2025:

Oh, we used to have a = but it was lost? You’re right.

laanwj commented at 1:58 pm on March 31, 2025:

So maybe it’s worth doing something else here, for example generate a unique prefix per bitcoind session startup (this would be once, so no need for performant randomness). But it’s out of scope for this PR.

i’ll open a PR for this. i am also going to rename the parameter to tor_stream_isolation to make it more specific.

hodlinator commented at 3:27 pm on March 31, 2025:

Thanks! Happy I managed to prompt you to come up with those edge-cases.

hodlinator approved

hodlinator commented at 1:04 pm on March 31, 2025: contributor

crACK f31ce35966bb84608938b0ba2272b415bcd42618

Could move “Friendly invite …” out of PR description into its own comment.

torcontrol: Add comment explaining Proxy credential randomization for Tor privacy

Signed-off-by: Eval EXEC <execvy@gmail.com>

8e4a0ddd50

eval-exec force-pushed on Mar 31, 2025

hodlinator approved

hodlinator commented at 7:58 am on April 1, 2025: contributor

re-ACK 8e4a0ddd5084ba5bb4613f422b3ff044d0da3927

Only re-added accidentally dropped = in comment since previous review (f31ce35966bb84608938b0ba2272b415bcd42618).

DrahtBot requested review from laanwj on Apr 1, 2025

laanwj approved

laanwj commented at 8:27 am on April 1, 2025: member

re-ACK 8e4a0ddd5084ba5bb4613f422b3ff044d0da3927

ryanofsky assigned ryanofsky on Apr 1, 2025

ryanofsky merged this on Apr 1, 2025

ryanofsky closed this on Apr 1, 2025

jonatack commented at 6:10 pm on April 7, 2025: member

Post-merge review ACK

torcontrol: Define tor reply code as const to improve our maintainability #32166

Code Coverage & Benchmarks

Reviews

Conflicts