Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
eval-exec wants to merge 1 commit into from
Closed

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

eval-exec wants to merge 1 commit into from

Conversation

@eval-exec
Copy link
Contributor

@eval-exec eval-exec commented Mar 3, 2025 •
edited
Loading

When I reviewing bitcoin's torcontrol.cpp's source code, the true in:

bitcoin/src/torcontrol.cpp

Line 400 in 79bbb38

Proxy addrOnion = Proxy(resolved, true);

is not easy to understand, so I add a comment to help us read that. Friendly invite @luke-jr to review this.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 3, 2025 •
edited
Loading

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 laanwj added the P2P label Mar 3, 2025
laanwj
laanwj reviewed Mar 3, 2025
View reviewed changes
@eval-exec eval-exec force-pushed the exec/comment-onion-random-credential branch from 7f7bdcc to 88ee6c5 Compare March 3, 2025 15:41
laanwj
laanwj reviewed Mar 3, 2025
View reviewed changes
src/torcontrol.cpp Outdated
Copy link
Member

@laanwj laanwj Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
torcontrol: Add comment explaining Proxy credential randomization for…
65e503e 
… Tor privacy

Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec force-pushed the exec/comment-onion-random-credential branch from 88ee6c5 to 65e503e Compare March 3, 2025 16:03
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 3, 2025

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

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.

@1440000bytes

This comment was marked as abuse.

Sign in to view
@eval-exec eval-exec requested a review from laanwj March 3, 2025 16:10
@DrahtBot DrahtBot removed the CI failed label Mar 3, 2025
laanwj
laanwj approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@laanwj laanwj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 65e503e

@eval-exec
Copy link
Contributor Author

eval-exec commented Mar 30, 2025

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

@maflcko maflcko mentioned this pull request Mar 30, 2025
Merged
@eval-exec
Copy link
Contributor Author

eval-exec commented Mar 30, 2025

Has cherry-pick this commit into #32166

@eval-exec eval-exec closed this Mar 30, 2025
ryanofsky added a commit that referenced this pull request Apr 1, 2025
@ryanofsky
Merge #32166: torcontrol: Define tor reply code as const to improve o…
6af68bb 
…ur maintainability

8e4a0dd torcontrol: Add comment explaining Proxy credential randomization for Tor privacy (Eval EXEC)
ec5c0b2 torcontrol: Define tor reply code as const to improve maintainability (Eval EXEC)

Pull request description:

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

ACKs for top commit:
  hodlinator:
    re-ACK 8e4a0dd
  laanwj:
    re-ACK 8e4a0dd

Tree-SHA512: 038daa6508ca88fceed5c8e155430614cb56976f36d1f8baee5114bca1141122cf94f51814a869848b3442691ee765cbf609cf946b2b35d5135015a9b749d917
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laanwj laanwj laanwj approved these changes

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eval-exec @DrahtBot @1440000bytes @laanwj