Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug]: Node announcements from peers are not re-propagated if they contain a DNS hostname #9473

Closed
MaxFangX opened this issue Feb 5, 2025 · 1 comment · Fixed by #9474
Closed
Assignees
Labels
bug Unintended code behaviour dns graph wire protocol Encoding of messages for the communication between nodes
Milestone

Comments

@MaxFangX
Copy link
Contributor

MaxFangX commented Feb 5, 2025

Background

When our LDK LSP ran on mainnet with LND peers, we discovered that although our node (with confirmed public channels and all other requirements satisfied per the BOLT 7 spec) was broadcasting our node announcements, our LND peers were not propagating them. Our node announcement included several addresses—a valid IPv4 address, an IPv6 address, and a TCP DNS (hostname) address. A connected LDK node saw our announcement when received directly from our LSP, yet our LSP’s LND peers never forwarded the announcement. After we removed the hostname address from our announcement, the node announcement propagated across the network within hours and appeared on LN explorers, strongly suggesting that the presence of a type‑5 (TCP DNS hostname) address is interfering with the normal gossip propagation.

According to #6337 (comment), this problem existed before but was fixed as of August 2022. It's possible that we are observing this problem because all of the LND nodes amongst our 9+ LN peers are running software at least that old, but given it is February 2025 this seems highly unlikely.

It's likely that #9455 fixes this issue, but I'm not sure, so I'm opening this issue and providing a bug report just in case. Would very much appreciate your input on this @mohamedawnallah !

Environment

  • version of lnd: Unknown, our peers run LND
  • which operating system (uname -a on *Nix): Unknown
  • version of btcd, bitcoind, or other backend: Unknown
  • mainnet

Steps to reproduce

Run a non-LND node Alice and broadcast a node announcement containing a DNS hostname as one of its addresses. (Must be a non-LND node since LND itself does not support configuring its own DNS hostnames). Connect a LND node Bob to Alice. Connect a non-LND node Charlie to Alice and Bob.

Expected behaviour

Charlie observes the node announcement coming from both Alice and Bob.

Actual behaviour

Charlie observes the node announcement coming from both Alice but not Bob.

Bug hypothesis

LND’s handling of node announcement addresses is performed in two stages: first during deserialization (reading the address from the wire) and later during serialization (writing the address when propagating a gossip message).

  1. Deserialization (ReadElement):

    In lnwire/lnwire.go, when decoding a []net.Addr, the function reads an initial descriptor byte that indicates the address type. For known types—IPv4 (tcp4Addr), IPv6 (tcp6Addr), v2 onion, and v3 onion—the function decodes the remaining bytes accordingly. However, if the address type is not one of these, the function falls into the default branch. (Note that BOLT 7 defines address type 5 for a TCP DNS hostname.) In that case the code does the following:

    default:
        // If we don't understand this address type,
        // we just store it along with the remaining
        // address bytes as type OpaqueAddrs. We need
        // to hold onto the bytes so that we can still
        // write them back to the wire when we
        // propagate this message.
        payloadLen := 1 + addrsLen - addrBytesRead
        payload := make([]byte, payloadLen)
        payload[0] = byte(aType)
        _, err := io.ReadFull(addrBuf, payload[1:])
        if err != nil {
            return err
        }
        address = &OpaqueAddrs{
            Payload: payload,
        }
        addrBytesRead = addrsLen
    

    Because type 5 is not explicitly handled, a DNS hostname is stored as an OpaqueAddrs. This in itself might be acceptable for re-transmission if the opaque payload were later handled properly.

  2. Serialization Issue (WriteElement):

    When propagating node announcements, LND calls WriteElement to serialize each element. In the type switch within WriteElement, addresses are re-encoded. However, there is no dedicated case to handle the OpaqueAddrs type produced by the default branch during deserialization. Instead, if an element is of a type that is not recognized by any of the explicit cases, WriteElement falls to its default branch, which is:

    default:
        return fmt.Errorf("unknown type in WriteElement: %T", e)

    In this case the function recurses (for example, when processing slices such as []net.Addr) and eventually encounters an element of type *OpaqueAddrs (created to hold our type‑5 DNS hostname). Since there is no case that handles *OpaqueAddrs, the default branch returns an error. This error in turn prevents the node announcement (which includes a DNS hostname) from being re-serialized and forwarded by the gossip layer.

@MaxFangX MaxFangX added bug Unintended code behaviour needs triage labels Feb 5, 2025
@ellemouton
Copy link
Collaborator

Hi @MaxFangX - thanks for the report & for testing both with and without DSN addrs 🙏

Regarding the WriteElement theory - we dont currently use that method for Serialising NodeAnnouncements. We use the (a *NodeAnnouncement) Encode method which calls WriteNetAddrs which then has a case for *OpaqueAddr.

So looks like we do write and read the actual lnwire message correctly. It looks like the issue may, however, be our internal model of the message (models.LightningNode) and the persistence of that message to the DB.

// SerializeAddr serializes an address into its raw bytes representation so that
// it can be deserialized without requiring address resolution.
func SerializeAddr(w io.Writer, address net.Addr) error {
	switch addr := address.(type) {
	case *net.TCPAddr:
		return encodeTCPAddr(w, addr)
	case *tor.OnionAddr:
		return encodeOnionAddr(w, addr)
	default:
		return ErrUnknownAddressType
	}
}

@ellemouton ellemouton self-assigned this Feb 5, 2025
@saubyk saubyk added this to the v0.19.0 milestone Feb 5, 2025
@saubyk saubyk added this to lnd v0.19 Feb 5, 2025
@saubyk saubyk moved this to In Progress in lnd v0.19 Feb 5, 2025
@saubyk saubyk added wire protocol Encoding of messages for the communication between nodes graph dns labels Feb 5, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in lnd v0.19 Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unintended code behaviour dns graph wire protocol Encoding of messages for the communication between nodes
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants