lnd
lnd copied to clipboard
lightningnetwork
[bug]: externalhosts does not advertize IPv4+IPv6 domain
Background
Describe your issue here.
Your environment
- version of
lnd0.17.3 - which operating system (
uname -aon *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 https://github.com/lightningnetwork/lnd/blob/613bfc07fb664086e78e8c58c9e1c8785e4656d9/server.go#L1644
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
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.
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.
- 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 inhost_ann.go? lncfg.ParseAddressStringis used in theLookupHostofHostAnnouncerConfig. Changing the signature of theHostAnnouncerConfig's LookupHost function means we would need to also change theTCPResolverthat is passed onlncfg.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 changingParseAddressStringwould 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.
Hi @aguxez
I think this is available still, so go for it.
- 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
lncfgto 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 innetannto test that if theLookupHostfunction returns multiple addresses that they are all announced properly. - I think I partially already answered this above. But I think we should have a copy of
lncfg.ParseAddressStringthat returns multiple addresses. And yes, for that we probably also need to add a new methodResolveTCPAddrs(plural) to thetor.Netinterface that can be used by the newlncfgfunction. But to implement that function for both clear net and Tor, it looks like you'll also need to go deep into thebtcdcode. 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.
@guggero It looks like resolving issue #6337 would also resolve this issue, as we no longer need to resolve external hosts?
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.
@PsychoPunkSage @Lokeshranjan8 are you working in this issue yet? otherwise I will do a PR!
Hey @guggero, could you give me a little help?
Looking at the code, the current behavior resolves and advertises only the first IP address returned from the DNS query for each host provided in externalhosts.
I'm wondering what the expected behavior is for the following scenario:
The DNS resolves multiple IPv4 and IPv6 addresses for the same externalhost.
Should LND:
Option 1: Advertise just the first IP address of each family (one IPv4 and one IPv6). This would maintain a simple dual-stack setup while still limiting the number of advertised addresses.
Option 2: Advertise all IP addresses returned by the DNS resolution with potentially multiple IPv4 and IPv6 addresses. This would reflect the full set of records configured in DNS.
Which option aligns better with the intended design of the node advertisement logic?
I think whatever address lnd chooses might still be the wrong one for the user. So IMO if the externalhosts option is chosen, all addresses that can be found and are valid for announcing should be announced.
This issue appears simple at first, but I believe it will require significant rework in how LND handles IP, DNS, and address resolution.
I created a draft where I modified the ParseAddressString signature to return an array of net.Addr. I'd appreciate yours thoughts on this approach https://github.com/lightningnetwork/lnd/pull/9877