Resolve Tor control plane address #22288
Conversation
There was a problem hiding this comment.
ACK 5ed8657
Tested this branch on POP!_OS with:
$ cat /etc/hosts
127.0.0.1 localghost
$ cat .bitcoin/bitcoin.conf
torcontrol=localghost:9051
Confirmed that it fails on master branch with error:
tor: Error parsing socket address localghost:9051
tor: Initializing connection to Tor Control Port localghost:9051 failed
There was a problem hiding this comment.
tACK 5ed8657
Tested branch on MacOS 11.3.1.
Used the following configurations in the /etc/hosts/ and bitcoin.conf files respectively:
$ cat /etc/hosts
127.0.0.1 zerohost$ cat bitcoin.conf
torcontrol=zerohost:9051I got the following errors on master (as expected):
tor: Error parsing socket address zerohost:9051
tor: Initiating connection to Tor control port zerohost:9051 failed
After this patch, the errors are no longer reported, and the tor connections are established.
Nice first-time contribution and a warm welcome @adriansmares as a contributor! ;)
There was a problem hiding this comment.
tACK 809d020
Thanks for looking into this. This change fixes my original issue
|
re-ACK 809d020 |
|
re-ACK 809d020 |
|
ACK 809d020 tested various configurations on signet with this branch versus master |
There was a problem hiding this comment.
Concept ACK
Welcome as a new contributor adriansmares! 🎉
Note that on the PR branch connecting to a IPv6 Tor control port doesn't work anymore:
$ ./src/bitcoind -torcontrol=::1:9051
...
tor: Error parsing socket address ::1:9051
tor: Initiating connection to Tor control port ::1:9051 failed
...
Looking a bit deeper into the problem, it seems like the address length passed to GetSockAddr() is too small (16 bytes on my machine), leading to the failure. On all other places in the codebase, sizeof(sockaddr) refers to the size of an sockaddr_storage instance (always defined one line above), whereas in this case it takes the size of the sockaddr structure. See fix below.
Github-Pull: bitcoin#22288 Rebased-From: cdd51e8
|
One argument for not using DNS in the case of Tor is to prevent DNS leaks. I don't think this change, in itself, adds any extra risk of that though (as it only looks up the control host, which hopefully is local). LGTM ACK cdd51e8 |
cdd51e8 torcontrol: Resolve Tor control plane address (Adrian-Stefan Mares) Pull request description: Closes bitcoin#22236 This PR forces the Tor control plane address to be resolved before a connection attempt is made, similar to how the `-proxy` / `-onion` address is resolved. The use case for this change is that the control plane may not have a stable address - in a containerized environment perhaps. ACKs for top commit: jonatack: ACK cdd51e8 tested various configurations on signet with this branch versus master laanwj: LGTM ACK cdd51e8 theStack: ACK cdd51e8 🪐 prayank23: ACK bitcoin@cdd51e8 Tree-SHA512: 5335cfcb89089a2acd6d02b88c2022dec60bb74388a99187c901c1c35d32896814d5f81df55c053953276c51fcec263c6ddadd068316f8e428b841bd599fc21e
|
I'm currently running into this issue of the Tor control address not being resolved, as I have set up Tor within a separate Docker container. Unfortunately, this PR doesn't seem to have made it into I could go back to managing the hidden service within Tor (and not use a control port) as I have done in the past, but actually I'd prefer to hand over control to bitcoind. |
|
Should this be considered for backporting to 22.x? |
Closes #22236
This PR forces the Tor control plane address to be resolved before a connection attempt is made, similar to how the
-proxy/-onionaddress is resolved.The use case for this change is that the control plane may not have a stable address - in a containerized environment perhaps.