torcontrol: Add comment explaining Proxy credential randomization for Tor privacy #31973

eval-exec commented at 3:28 PM on March 3, 2025: contributor

When I reviewing bitcoin's torcontrol.cpp's source code, the true in: https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4e58/src/torcontrol.cpp#L400 is not easy to understand, so I add a comment to help us read that. Friendly invite @luke-jr to review this.

DrahtBot commented at 3:28 PM on March 3, 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/31973.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj
Concept ACK	1440000bytes

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

laanwj added the label P2P on Mar 3, 2025

in src/torcontrol.cpp:404 in 7f7bdcc9a3 outdated

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

laanwj commented at 3:40 PM on March 3, 2025:

i'd use a named parameter:

Proxy addrOnion = Proxy(resolved, /*m_randomize_credentials=*/ true);

eval-exec commented at 3:41 PM on March 3, 2025:

Thank you.

eval-exec commented at 3:43 PM on March 3, 2025:

Hello @laanwj , what format tool do you use to format bitcoin's source code? I tried clang-format -i src/torcontrol.cpp, it format too much.

laanwj commented at 3:52 PM on March 3, 2025:

As you only add comments, i don't think there's a reason to run the formatter.

However, there's the clang-format-diff script in the repository that can be used to only re-format lines that have been changed, and not the whole file.

laanwj commented at 4:01 PM on March 3, 2025:

Sorry, it's failing lint now:

 /ci_container_base/src/torcontrol.cpp:404:39: error: argument name 'm_randomize_credentials' in comment does not match parameter name '_randomize_credentials' [bugprone-argument-comment,-warnings-as-errors]
[10:56:52.844]   404 |     Proxy addrOnion = Proxy(resolved, /*m_randomize_credentials=*/ true);
[10:56:52.844]       |                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
[10:56:52.844]       |                                       /*_randomize_credentials=*/
[10:56:52.844] /ci_container_base/src/netbase.h:62:49: note: '_randomize_credentials' declared here
[10:56:52.844]    62 |     explicit Proxy(const CService& _proxy, bool _randomize_credentials = false) : proxy(_proxy), m_is_unix_socket(false), m_randomize_credentials(_randomize_credentials) {}
[10:56:52.844]

The parameter is apparently called _randomize_credentials.

eval-exec commented at 4:11 PM on March 3, 2025:

Thank you, changed, and then I executed

docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter

it say:

Success: no issues found in 305 source files

eval-exec force-pushed on Mar 3, 2025

torcontrol: Add comment explaining Proxy credential randomization for Tor privacy

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

65e503e849

in src/torcontrol.cpp:402 in 88ee6c58ff outdated

 396 | @@ -397,7 +397,11 @@ void TorController::get_socks_cb(TorControlConnection& _conn, const TorControlRe
 397 |  
 398 |      Assume(resolved.IsValid());
 399 |      LogDebug(BCLog::TOR, "Configuring onion proxy for %s\n", resolved.ToStringAddrPort());
 400 | -    Proxy addrOnion = Proxy(resolved, true);
 401 | +
 402 | +    // With m_randomize_credentials = true, generates unique SOCKS credentials per proxy connection (e.g., Tor).
 403 | +    // Prevents connection correlation and enhances privacy by forcing different Tor circuits.

laanwj commented at 3:46 PM on March 3, 2025:

Yes-FWIW, the main way in which this increases privacy is that every new connection is likely to choose a different exit node, so will appear to come from a different host.

There's something to be said for adding this documentation for the Proxy constructor, but as this specific behavior is specific to Tor (and not SOCKS5 in general) i have no objection to adding it here.

eval-exec force-pushed on Mar 3, 2025

DrahtBot added the label CI failed on Mar 3, 2025

DrahtBot commented at 4:03 PM on March 3, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/38110419062</sub>

<details><summary>Hints</summary>

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.

</details>

1440000bytes commented at 4:09 PM on March 3, 2025: none

Concept ACK

eval-exec requested review from laanwj on Mar 3, 2025

DrahtBot removed the label CI failed on Mar 3, 2025

laanwj approved

laanwj commented at 10:17 PM on March 3, 2025: member

Code review ACK 65e503e8499f18e48d7801a298d9acdd6ed62d0b

eval-exec commented at 11:12 AM on March 30, 2025: contributor

Friendly ping @fanquake , I think this PR is ready to be merged.

eval-exec commented at 11:26 AM on March 30, 2025: contributor

Has cherry-pick this commit into https://github.com/bitcoin/bitcoin/pull/32166

eval-exec closed this on Mar 30, 2025

ryanofsky referenced this in commit 6af68bb84b on Apr 1, 2025