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
  1. 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.

  2. 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.

  3. laanwj added the label P2P on Mar 3, 2025
  4. 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.

    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:

    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
    
  5. eval-exec force-pushed on Mar 3, 2025
  6. torcontrol: Add comment explaining Proxy credential randomization for Tor privacy
    Signed-off-by: Eval EXEC <execvy@gmail.com>
    65e503e849
  7. 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.

  8. eval-exec force-pushed on Mar 3, 2025
  9. DrahtBot added the label CI failed on Mar 3, 2025
  10. DrahtBot 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.

  11. 1440000bytes commented at 4:09 pm on March 3, 2025: none
    Concept ACK
  12. eval-exec requested review from laanwj on Mar 3, 2025
  13. DrahtBot removed the label CI failed on Mar 3, 2025
  14. laanwj approved
  15. laanwj commented at 10:17 pm on March 3, 2025: member
    Code review ACK 65e503e8499f18e48d7801a298d9acdd6ed62d0b

github-metadata-mirror

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

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me