Closes #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.
ACK https://github.com/bitcoin/bitcoin/pull/22288/commits/5ed8657cf7c17fb1bc15bfa6ec0f1ab9fe28ec18
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
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:9051
I 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! ;)
141 | - (struct sockaddr*)&connect_to_addr, &connect_to_addrlen)<0) { 142 | - LogPrintf("tor: Error parsing socket address %s\n", tor_control_center); 143 | + 144 | + CService control_service; 145 | + if (!Lookup(tor_control_center, control_service, 9051, fNameLookup)) { 146 | + LogPrintf("tor: Failed to lookup control center %s\n", tor_control_center.c_str());
The verb form is "look up".
LogPrintf("tor: Failed to look up control center %s\n", tor_control_center.c_str());
147 | + return false; 148 | + } 149 | + 150 | + struct sockaddr_storage control_address; 151 | + socklen_t control_address_len = sizeof(sockaddr); 152 | + if (!control_service.GetSockAddr((struct sockaddr*)&control_address, &control_address_len)) {
while retouching this, could use a named cast (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast)
if (!control_service.GetSockAddr(reinterpret_cast<struct sockaddr*>(&control_address), &control_address_len)) {
157 | @@ -153,8 +158,8 @@ bool TorControlConnection::Connect(const std::string& tor_control_center, const 158 | this->disconnected = _disconnected; 159 | 160 | // Finally, connect to tor_control_center 161 | - if (bufferevent_socket_connect(b_conn, (struct sockaddr*)&connect_to_addr, connect_to_addrlen) < 0) { 162 | - LogPrintf("tor: Error connecting to address %s\n", tor_control_center); 163 | + if (bufferevent_socket_connect(b_conn, (struct sockaddr*)&control_address, control_address_len) < 0) {
idem
if (bufferevent_socket_connect(b_conn, reinterpret_cast<struct sockaddr*>(&control_address), control_address_len) < 0) {
133 | @@ -134,12 +134,17 @@ bool TorControlConnection::Connect(const std::string& tor_control_center, const
134 | {
135 | if (b_conn)
136 | Disconnect();
Don't hesitate to fix this if you retouch (per doc/developer-notes.md about code style for conditionals)
- if (b_conn)
+ if (b_conn) {
Disconnect();
+ }
Concept ACK, thanks for looking into this.
169 | 170 | // Finally, connect to tor_control_center 171 | - if (bufferevent_socket_connect(b_conn, (struct sockaddr*)&connect_to_addr, connect_to_addrlen) < 0) { 172 | - LogPrintf("tor: Error connecting to address %s\n", tor_control_center); 173 | + if (bufferevent_socket_connect(b_conn, reinterpret_cast<struct sockaddr*>(&control_address), control_address_len) < 0) { 174 | + LogPrintf("tor: Error connecting to address %s\n", tor_control_center.c_str());
Untested, but ISTM you can omit the 3 uses of c_str()
<details><summary>diff</summary><p>
diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index 4a56159228..dcb2175c94 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -138,14 +138,14 @@ bool TorControlConnection::Connect(const std::string& tor_control_center, const
CService control_service;
if (!Lookup(tor_control_center, control_service, 9051, fNameLookup)) {
- LogPrintf("tor: Failed to look up control center %s\n", tor_control_center.c_str());
+ LogPrintf("tor: Failed to look up control center %s\n", tor_control_center);
return false;
}
struct sockaddr_storage control_address;
socklen_t control_address_len = sizeof(sockaddr);
if (!control_service.GetSockAddr(reinterpret_cast<struct sockaddr*>(&control_address), &control_address_len)) {
- LogPrintf("tor: Error parsing socket address %s\n", tor_control_center.c_str());
+ LogPrintf("tor: Error parsing socket address %s\n", tor_control_center);
return false;
}
@@ -161,7 +161,7 @@ bool TorControlConnection::Connect(const std::string& tor_control_center, const
// Finally, connect to tor_control_center
if (bufferevent_socket_connect(b_conn, reinterpret_cast<struct sockaddr*>(&control_address), control_address_len) < 0) {
- LogPrintf("tor: Error connecting to address %s\n", tor_control_center.c_str());
+ LogPrintf("tor: Error connecting to address %s\n", tor_control_center);
return false;
}
</p></details>
(Optional, you'll save time by squashing the commits after each update, to the final version for review.)
Indeed, I see now LogPrintf is based on templates, not untyped varargs.
tACK 809d02044957b55a2869c0da71ff57656f53ffe4
Thanks for looking into this. This change fixes my original issue
re-ACK 809d02044957b55a2869c0da71ff57656f53ffe4
ACK 809d02044957b55a2869c0da71ff57656f53ffe4 tested various configurations on signet with this branch versus master
147 | + LogPrintf("tor: Failed to look up control center %s\n", tor_control_center); 148 | + return false; 149 | + } 150 | + 151 | + struct sockaddr_storage control_address; 152 | + socklen_t control_address_len = sizeof(sockaddr);
struct sockaddr_storage control_address;
socklen_t control_address_len = sizeof(control_address);
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.
ACK cdd51e8ee156f3bb3135be8aa51530a53734153e 🪐
Tested on Debian bullseye/sid by running ./src/bitcoind -torcontrol=localhost:9051 twice, once with localhost set to 127.0.0.1 in /etc/hosts, once set to ::1.
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
Good observation.
ACK https://github.com/bitcoin/bitcoin/pull/22288/commits/cdd51e8ee156f3bb3135be8aa51530a53734153e
Confirmed it works for ipv6 as well
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 cdd51e8ee156f3bb3135be8aa51530a53734153e
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 22.0. Does someone know a quick workaround to deal with the dynamically changing IP address, until a 22.1 is released?
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?