We state not to do 'any local DNS' if --always-use-proxy flag is set,…

[Saibato]

… so we should do this

Even if it is on startup only once ...
Like @bitcoin-software indicated the expected UX should be in
line with what a user expects the software will do
so we should not dns if we say so with a flag that suggest that.

Signed-off-by: Saibato saibato.naga@pm.me

[darosior]

Shouldn't we go beyond and set use_dns to false so that connectd does not try to resolve unknown ids through bitcoinstats ? It would also cover the same feature as your present commit :

lightning/connectd/connectd.c

Lines 203 to 207 in 4bf2bbc

	/* If they told us to never do DNS queries, don't even do this one */
	if (!daemon->use_dns) {
	daemon->broken_resolver_response = NULL;
	return false;
	}

[Saibato]

Shouldn't we go beyond and set use_dns to false so that connectd does not try to resolve unknown ids through bitcoinstats ? It would also cover the same feature as your present commit :

lightning/connectd/connectd.c

Lines 203 to 207 in 4bf2bbc

	/* If they told us to never do DNS queries, don't even do this one */
	if (!daemon->use_dns) {
	daemon->broken_resolver_response = NULL;
	return false;
	}

--always_use_proxy=true even in active master prevents the bootstap resolve call even with out changing the value of use_dns.
The sad part here in this function, is that the comment and expected outcome is also false here. stated, i guess that was your point.
use_dns disables only the bootstrap call if default always_use_proxy false, but not the resolve for bogus dns servers.
Suppose the cmd line would have say --addr=nowhere:1234 even with use_dns false and use_proxy_always default , we would resolve nowhere, only use_proxy_always will disable all local dns and route the resolve to tor , even the seednames from id, that seams to me the right way to do since when we have semi fixed tor address you could guess tor addr by node id ( hmm.. of topic). (̶ ̶I̶ ̶a̶m̶ ̶n̶o̶t̶ ̶s̶u̶r̶e̶ ̶i̶f̶ ̶t̶h̶e̶ ̶t̶o̶r̶ ̶p̶r̶o̶x̶y̶ ̶a̶p̶i̶ ̶b̶y̶ ̶n̶o̶w̶ ̶o̶n̶ ̶r̶e̶l̶e̶a̶s̶e̶ ̶r̶e̶s̶o̶l̶v̶e̶ ̶a̶n̶d̶ ̶r̶e̶t̶u̶r̶n̶ ̶a̶l̶s̶o̶ ̶s̶r̶v̶ ̶e̶n̶t̶r̶y̶ ̶.̶.̶h̶m̶m̶.̶.̶.̶)̶ , all will be in line with documentation, if we also prevent the call to resolve bogus dns servers here,

Since we get the value for use_dns as bootstrap flag from towire, we should keep it and not change it here.
What we can do here, is do the if (!daemon.always_use_proxy) local in
static bool broken_resolver(struct daemon *daemon) since we have daemon in the function, side effect we could set the broken_resolver_response to NULL.since we accept the proxy we choose even tor clearly as not broken 😄
I push the change now . please review.

[rustyrussell]

Ack 1433711

Good catch!