[bug]: externalhosts does not advertize IPv4+IPv6 domain

[lukaszsamson]

Background

Describe your issue here.

Your environment

• version of lnd 0.17.3
• which operating system (uname -a on *Nix) ubuntu 20.04
• version of btcd, bitcoind, or other backend
• any other relevant environment details

Steps to reproduce

I run a node that I want accessible via clearnet IPv4, IPv6 and Tor.
I have a domain lightning.foo.com that has both A and AAAA DNS records. When I set this in lnd.conf

externalhosts=lightning.foo.com

The node advertises only IPv4 and Tor (as checked via getinfo)

Expected behaviour

If a domain resolves to both IPv4 and IPv6 lnd should advertise both protocols

Actual behaviour

The node favors one of the protocols. I guess this code does the resolve on the first interface

lnd/server.go

Line 1644 in 613bfc0

cfg.net.ResolveTCPAddr,

[lukaszsamson]

Setting it like this when lightning6.foo.com is IPv6 only appears to work

externalhosts=lightning.foo.com
externalhosts=lightning6.foo.com

But only when I explicitly listen on IPv6. This is also a bug as lnd should not make assumptions over which protocol it gets traffic. Nodes behind IPv6 to IPv4 NAT need to listen only on IPv4 interface to get both IPv6 traffic

[Roasbeef]

Our logic here mirrors the stdlib function ResolveTCPAddr, which also only returns a single TCP addr. We should use LookupHost here which returns a slice of addresses.

[aguxez]

Hey @Roasbeef , I'd like to work on this if it's still available. I've been checking the code and have questions to ensure I'm looking at the right things.

1. In server.go, we build the IPs to advertise, but according to Coveralls, this part is not covered in tests, so I don't know what tests to change to validate that this is properly working (https://coveralls.io/builds/68524462/source?filename=server.go#L1632). Would it be in host_ann.go?
2. lncfg.ParseAddressString is used in the LookupHost of HostAnnouncerConfig. Changing the signature of the HostAnnouncerConfig's LookupHost function means we would need to also change the TCPResolver that is passed on lncfg.ParseAddressString. Is that desirable here? This function is used in many places, and if I'm correct, that would mean we change how this function resolves addresses: https://github.com/lightningnetwork/lnd/blob/master/server.go#L1641-L1645. I'm just conscious that changing ParseAddressString would break other parts of the code like https://github.com/lightningnetwork/lnd/blob/master/lnrpc/wtclientrpc/wtclient.go#L245-L247 that use other functions to resolve the addresses.

[guggero]

Hi @aguxez

I think this is available still, so go for it.

1. You are correct. Some code that requires external components (such as a DNS server) are quite hard to test. So some areas currently aren't covered super well. You might want to extract the closure created here into a new function (in lncfg to avoid circular package dependencies) and allow it to return more than one address. Perhaps you can then unit test that with some sort of embedded DNS server? Not sure if one exists for that purpose written in Go. That would test the part for resolving multiple addresses. Then I think you can extend the test code in netann to test that if the LookupHost function returns multiple addresses that they are all announced properly.
2. I think I partially already answered this above. But I think we should have a copy of lncfg.ParseAddressString that returns multiple addresses. And yes, for that we probably also need to add a new method ResolveTCPAddrs (plural) to the tor.Net interface that can be used by the new lncfg function. But to implement that function for both clear net and Tor, it looks like you'll also need to go deep into the btcd code. So you'll have at least 3 dependent PRs to solve this one. And I'm not even sure that Tor can return multiple addresses in its resolution command...

Sooo, looking at it in more detail, maybe the "good first issue" and "beginner" labels aren't correct and this is quite a bit involved.

[akagami-harsh]

i'd like work this issue

[mohamedawnallah]

@guggero It looks like resolving issue #6337 would also resolve this issue, as we no longer need to resolve external hosts?

[guggero]

There could be some overlap, yes. But I could also see the use case where you have a dynamic DNS (e.g. dyndns.com) but don't want everyone to know the name of that, just the resolved IP address.