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.
torcontrol: Add comment explaining Proxy credential randomization for Tor privacy #31973
pull eval-exec wants to merge 1 commits into bitcoin:master from eval-exec:exec/comment-onion-random-credential changing 1 files +5 −1-
eval-exec commented at 3:28 pm on March 3, 2025: contributor
-
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:
0Proxy addrOnion = Proxy(resolved, /*m_randomize_credentials=*/ true);
eval-exec commented at 3:41 pm on March 3, 2025:Thank you.
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:
0 /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] 1[10:56:52.844] 404 | Proxy addrOnion = Proxy(resolved, /*m_randomize_credentials=*/ true); 2[10:56:52.844] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3[10:56:52.844] | /*_randomize_credentials=*/ 4[10:56:52.844] /ci_container_base/src/netbase.h:62:49: note: '_randomize_credentials' declared here 5[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) {} 6[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
0docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter
it say:
0Success: no issues found in 305 source files
eval-exec force-pushed on Mar 3, 2025torcontrol: Add comment explaining Proxy credential randomization for Tor privacy
Signed-off-by: Eval EXEC <execvy@gmail.com>
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, 2025DrahtBot added the label CI failed on Mar 3, 2025DrahtBot commented at 4:03 pm on March 3, 2025: contributor🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38110419062
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.
1440000bytes commented at 4:09 pm on March 3, 2025: noneConcept ACKeval-exec requested review from laanwj on Mar 3, 2025DrahtBot removed the label CI failed on Mar 3, 2025laanwj approvedlaanwj commented at 10:17 pm on March 3, 2025: memberCode review ACK 65e503e8499f18e48d7801a298d9acdd6ed62d0b
eval-exec
DrahtBot
laanwj
1440000bytes
Labels
P2P
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-29 18:12 UTC
More mirrored repositories can be found on mirror.b10c.me