torcontrol: Query Tor for correct -onion configuration #15423
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
Agree with @practicalswift comments, apart from that tACK 962f168 (enabled/disabled 9050 and other ports in |
|
Concept ACK, thanks, this makes sense. |
|
This came up in a twitter discussion with @luke-jr yesterday. What's blocker here? Or just forgot in a long list of unmerged PRs? |
|
Concept ACK |
| The last travis run for this pull request was 387 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
vasild
left a comment
vasild
left a comment
There was a problem hiding this comment.
Light code review ok. This is based on master from Feb 2019 and does not compile configure: error: openssl not found.. Needs rebase. I would like to compile and test it.
|
Rebased, nits addressed |
|
@luke-jr, are you planning to update this PR? |
luke-jr
force-pushed
the
tor_socks_port
branch
from
d37d95a to
4314a21
Compare
|
Review comments addressed |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
utACK 4314a21
nit: #15423 (comment)
I never used 9050 port on Windows and it was 9150 with browser. Not sure how many Bitcoin Core users run this software on Windows but Windows is still used for most of the desktop PCs and easiest process is to use 9150 which would work as default.
vasild
left a comment
vasild
left a comment
There was a problem hiding this comment.
ACK 4314a216e319b70ab0b47969eae4d8ad86262f37
|
I am not sure about the default socks port on Windows, but from the point of view of this PR it was 9050 before and is 9050 after, so it is ok, it is not the purpose of this PR to change that. |
|
Rebased again |
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
ACK b2774fc review, rebased to master, debug build, ran unit tests, tested happy path only
2022-03-16T21:31:10Z [torcontrol] tor: Authentication successful
2022-03-16T21:31:10Z [torcontrol] tor: Get SOCKS port command yielded 127.0.0.1:9050
2022-03-16T21:31:10Z [torcontrol] tor: Configuring onion proxy for 127.0.0.1:9050
2022-03-16T21:31:10Z [torcontrol] tor: ADD_ONION successful
| * this is belt-and-suspenders sanity limit to prevent memory exhaustion. | ||
| */ | ||
| static const int MAX_LINE_LENGTH = 100000; | ||
| static const uint16_t DEFAULT_TOR_SOCKS_PORT = 9050; |
There was a problem hiding this comment.
-
s/const/constexpr/ if you retouch
-
as a follow-up, maybe use this constant in
src/init.cpp -
there is also
src/qt/optionsmodel.h:20:static constexpr uint16_t DEFAULT_GUI_PROXY_PORT = 9050;
This has nothing to do with Windows. Tor Browser's internal Tor uses port 9150 on every OS. This is an implementation detail and it is frowned upon to use it in third-party applications (it controls circuits tightly, for example, so anything interfering with it might accidentally affect privacy). The normal Tor version for windows uses the same default port, 9050. |
|
Tested ACK (FreeBSD) b2774fc I changed tor: bitcoind: |
Handle deleted github users gracefully when retrieving ACKs by replacing their name with `[deleted]` in the sanitization function. Tested with bitcoin/bitcoin#15423.
8b7b630 github-merge: Handle deleted users gracefully (laanwj) Pull request description: Handle deleted github users gracefully when retrieving ACKs by replacing their name with `[deleted]` in the sanitization function. Tested with bitcoin/bitcoin#15423. ACKs for top commit: MarcoFalke: cr ACK 8b7b630 Tree-SHA512: 21aa715e4d3196bcde6a8523892afd415c8aae4a440e9ea413a5459ce8c0c978747289a3300e4ff28647cb6af8aded59fd749a06d48b5c2b09896e3eee48d6c1
…ration b2774fc torcontrol: Query Tor for correct -onion configuration (Luke Dashjr) Pull request description: Currently, we just assume any running Tor instance provides localhost port 9050 for SOCKS, and configure `-onion` accordingly when we get a Tor control connection. This actually queries the Tor node for its SOCKS listeners, and uses the configured port instead. For backward compatibility, it falls back to localhost:9050 if it can't get any better port info. I'm not sure if that's the correct action to take when the Tor daemon explicitly says there are no ports listening... ACKs for top commit: laanwj: Tested ACK (FreeBSD) b2774fc vasild: ACK b2774fc jonatack: ACK b2774fc review, rebased to master, debug build, ran unit tests, tested happy path only Tree-SHA512: 2fa93a3cf0cb675801d1b51322ce953ea9b2317f78154a53b603244d74252f434cc1eaa5ae48cb3fe6bdc4ce984a6d976ff95bb046f7933b9740332942378c02
11 participants
Currently, we just assume any running Tor instance provides localhost port 9050 for SOCKS, and configure
-onionaccordingly when we get a Tor control connection.This actually queries the Tor node for its SOCKS listeners, and uses the configured port instead.
For backward compatibility, it falls back to localhost:9050 if it can't get any better port info. I'm not sure if that's the correct action to take when the Tor daemon explicitly says there are no ports listening...