Enable host lookups for -proxy and -onion parameters #9774

jmcorgan commented at 1:20 AM on February 16, 2017: contributor

This allows using hostname lookups for the -proxy and -onion parameters in addition to raw IP addresses in the conf file or on the command line, in the same way and using the same -dns control switch that -addnode, -connect, and -seednode already use.

This was as simple as moving the the initialization of fNameLookup to earlier in the file, then using Lookup() instead of LookupNumeric() for -proxy and -onion.

Note: There are no documentation changes reflecting this in this pull request.

Also, -torcontrol is not affected, as it currently uses a utility function in libevent2 that only parses numeric IP addresses, and it wasn't clear what the best way was to reimplement this.

fanquake added the label P2P on Feb 16, 2017

laanwj commented at 7:25 PM on February 16, 2017: member

Concept ACK, implementation is much simpler as I expected

in src/init.cpp:None in 56f6bafde8 outdated

1262 | +        CService proxyAddr;
1263 | +        if (!Lookup(proxyArg.c_str(), proxyAddr, 9050, fNameLookup)) {
1264 | +            return InitError(strprintf(_("Invalid -proxy address: '%s'"), proxyArg));
1265 | +        }
1266 | +
1267 | +	proxyType addrProxy = proxyType(proxyAddr, proxyRandomize);

laanwj commented at 7:25 PM on February 16, 2017:

Indentation looks wonky here (also below)

jmcorgan commented at 9:25 PM on February 16, 2017:

Emacs C++ mode seems to like futzing with whitespace. I'll cleanup.

Since you wrote it, any thoughts on the -torcontrol parsing?

laanwj commented at 8:22 AM on February 17, 2017:

Yes, that'd have to use libevent DNS lookups, which are asynchronous by default, thus someone more complicated to handle. There may be a synchronous way of doing lookups as well, I don't remember if that's exposed outside the http sublibrary.

I'm not sure how important it is in practice, as running torcontrol over a network is extremely discouraged by the Tor developers [not even getting started about potentially leaking the DNS hostname of your Tor router to the internet!]. The choices are pretty much localhost TCP or localhost UNIX socket. The latter we don't currently handle, would be more pressing than hostname lookups.

Note that the plan is still to port over the P2P code to libevent, so these interfaces will likely grow together in the future. Might as well wait for that and to it in a similar way.

jmcorgan commented at 2:05 PM on February 17, 2017:

Agree on torcontrol. One of my use cases however is running the Tor proxy and bitcoind node inside a separate docker containers. In this setup they each are running on a virtual, internal network with ephemeral IP addresses but with fixed host names, with no actual ports open on the host machine.

It's not too hard to work around, just an annoyance.

laanwj commented at 5:16 PM on February 17, 2017:

Yeah, I always use fixed IP bindings in the DHCP configuration of VM subnets to avoid having to mess with DNS.

luke-jr commented at 8:32 PM on February 17, 2017: member

This seems like it just does a single lookup at startup, but it seems the purpose of providing a hostname would be to follow IP changes...?

jmcorgan force-pushed on Feb 18, 2017

jmcorgan commented at 11:58 AM on February 18, 2017: contributor

No, the purpose is not to track IP changes during runtime; merely to allow specifying the configuration parameter using hostnames instead of raw IP addresses. IP address assignments can change in a network and it is typical practice to use hostname/DNS resolution to keep configurations stable and make host->addr mappings in a single place.

This is already done in bitcoind for the -addnode, -connect, and -seenode parameters; this pull request merely extends the capability to include -proxy and -onion.

jmcorgan commented at 11:59 AM on February 18, 2017: contributor

@laanwj Whitespace issues have been fixed.

in src/init.cpp:None in 9f63533871 outdated

1259 |      if (proxyArg != "" && proxyArg != "0") {
1260 | -        CService resolved(LookupNumeric(proxyArg.c_str(), 9050));
1261 | -        proxyType addrProxy = proxyType(resolved, proxyRandomize);
1262 | +        CService proxyAddr;
1263 | +        if (!Lookup(proxyArg.c_str(), proxyAddr, 9050, fNameLookup)) {
1264 | +            return InitError(strprintf(_("Invalid -proxy address: '%s'"), proxyArg));

jonasschnelli commented at 7:35 PM on February 19, 2017:

Maybe "Invalid -proxy address or hostname: '%s'"?

jmcorgan commented at 5:50 AM on February 20, 2017:

Sure. I'll update the other cases of this as well.

jonasschnelli commented at 7:35 PM on February 19, 2017: contributor

Concept ACK

Enable host lookups for -proxy and -onion parameters

* Extends -dns parameter (via fNameLookup) to control these two new
  parameters in addition to -addnode, -connect, and -seednode

* Moves fNameLookup assignment earlier as needed

* Changes -proxy and -onion to use Lookup() instead of LookupNumeric()

f36bdf02ce

jmcorgan force-pushed on Feb 20, 2017

jmcorgan commented at 6:08 AM on February 20, 2017: contributor

@jonasschnelli Error messages updated.

laanwj merged this on Mar 3, 2017

laanwj closed this on Mar 3, 2017

laanwj referenced this in commit 90cb2a218e on Mar 3, 2017

jmcorgan deleted the branch on May 4, 2017

PastaPastaPasta referenced this in commit accb8c24ee on Dec 30, 2018

PastaPastaPasta referenced this in commit 84ffc1beeb on Dec 31, 2018

PastaPastaPasta referenced this in commit 93cb2d2246 on Dec 31, 2018

PastaPastaPasta referenced this in commit f6614b88f5 on Dec 31, 2018

PastaPastaPasta referenced this in commit 57de0e1b6b on Jan 2, 2019

PastaPastaPasta referenced this in commit 62a3ced850 on Jan 2, 2019

PastaPastaPasta referenced this in commit 19e970d1f6 on Jan 3, 2019

PastaPastaPasta referenced this in commit 4558304c43 on Jan 21, 2019

PastaPastaPasta referenced this in commit 4bc8fa15f3 on Jan 25, 2019

PastaPastaPasta referenced this in commit 7d4c8575ac on Jan 29, 2019

PastaPastaPasta referenced this in commit 4817eb40ad on Feb 1, 2019

PastaPastaPasta referenced this in commit 759d8bf631 on Feb 1, 2019

PastaPastaPasta referenced this in commit 395b537167 on Feb 1, 2019

MarcoFalke locked this on Sep 8, 2021