graph/db: extract DNS addresses during SQL migration

In this commit, we take advantage of the graph SQL migration and use it
to also extract DNS addresses from the opaque address type. We use
opaque addresses to store addresses that we dont understand yet. We
recently added logic for DNS addresses and so we may have persisted node
announcements that have DNS addresses but we would currently have them
stored under the opaque address type. So we use this migration to see if
we can extract such addresses.

A few decisions were made here:
1) If multiple DNS addressees are extracted, this is ok and we continue
   to migrate the node even though this is actually invalid at a
   protocol level. We will currently check (at a higher level) that a node
   announcement only has 1 DNS address in it before we broadcast it though.
2) If an invalid DNS address is encountered (so we hit the DNS type
   descriptor but then the rest of the DNS address payload is invalid
   and cannot be parsed into the expected hostname:port, then we skip
   migrating the node completely.

graph/db/sql_migration.go

@@ -260,18 +260,17 @@ func migrateNodes(ctx context.Context, cfg *sqldb.QueryConfig,
 				"opaque data for node %x: %w", pub, err)
 		}
 
+		if err = maybeOverrideNodeAddresses(node); err != nil {
+			skipped++
+			log.Warnf("Skipping migration of node %x with invalid "+
+				"address (%v): %v", pub, node.Addresses, err)
+
+			return nil
+		}
+
 		count++
 		chunk++
 
-		// TODO(elle): At this point, we should check the loaded node
-		// to see if we should extract any DNS addresses from its
-		// opaque type addresses. This is expected to be done in:
-		// https://github.com/lightningnetwork/lnd/pull/9455.
-		// This TODO is being tracked in
-		//  https://github.com/lightningnetwork/lnd/issues/9795 as this
-		// must be addressed before making this code path active in
-		// production.
-
 		// Write the node to the SQL database.
 		id, err := insertNodeSQLMig(ctx, sqlDB, node)
 		if err != nil {
@@ -323,12 +322,82 @@ func migrateNodes(ctx context.Context, cfg *sqldb.QueryConfig,
 	}
 
 	log.Infof("Migrated %d nodes from KV to SQL in %v (skipped %d nodes "+
-		"due to invalid TLV streams)", count, time.Since(totalTime),
+		"due to invalid TLV streams or invalid addresses)", count,
+		time.Since(totalTime),
+
 		skipped)
 
 	return nil
 }
 
+// maybeOverrideNodeAddresses checks if the node has any opaque addresses that
+// can be parsed. If so, it replaces the node's addresses with the parsed
+// addresses. If the address is unparseable, it returns an error.
+func maybeOverrideNodeAddresses(node *models.LightningNode) error {
+	// In the majority of cases, the number of node addresses will remain
+	// unchanged, so we pre-allocate a slice of the same length.
+	addrs := make([]net.Addr, 0, len(node.Addresses))
+
+	// Iterate over each address in search of any opaque addresses that we
+	// can inspect.
+	for _, addr := range node.Addresses {
+		opaque, ok := addr.(*lnwire.OpaqueAddrs)
+		if !ok {
+			// Any non-opaque address is left unchanged.
+			addrs = append(addrs, addr)
+			continue
+		}
+
+		// For each opaque address, we'll now attempt to parse out any
+		// known addresses. We'll do this in a loop, as it's possible
+		// that there are several addresses encoded in a single opaque
+		// address.
+		payload := opaque.Payload
+		for len(payload) > 0 {
+			var (
+				r            = bytes.NewReader(payload)
+				numAddrBytes = uint16(len(payload))
+			)
+			byteRead, readAddr, err := lnwire.ReadAddress(
+				r, numAddrBytes,
+			)
+			if err != nil {
+				return err
+			}
+
+			// If we were able to read an address, we'll add it to
+			// our list of addresses.
+			if readAddr != nil {
+				addrs = append(addrs, readAddr)
+			}
+
+			// If the address we read was an opaque address, it
+			// means we've hit an unknown address type, and it has
+			// consumed the rest of the payload. We can break out
+			// of the loop.
+			if _, ok := readAddr.(*lnwire.OpaqueAddrs); ok {
+				break
+			}
+
+			// If we've read all the bytes, we can also break.
+			if byteRead >= numAddrBytes {
+				break
+			}
+
+			// Otherwise, we'll advance our payload slice and
+			// continue.
+			payload = payload[byteRead:]
+		}
+	}
+
+	// Override the node addresses if we have any.
+	if len(addrs) != 0 {
+		node.Addresses = addrs
+	}
+
+	return nil
+}
+
 // migrateSourceNode migrates the source node from the KV backend to the
 // SQL database.
 func migrateSourceNode(ctx context.Context, kvdb kvdb.Backend,

graph/db/sql_migration_test.go

@@ -49,6 +49,78 @@ var (
 		BitcoinSig1Bytes: testSig.Serialize(),
 		BitcoinSig2Bytes: testSig.Serialize(),
 	}
+
+	// testOpaqueAddrWithEmbeddedDNSAddr is an opaque address that contains
+	// a single DNS address within it.
+	testOpaqueAddrWithEmbeddedDNSAddr = &lnwire.OpaqueAddrs{
+		Payload: []byte{
+			// The protocol level type for DNS addresses.
+			0x05,
+			// Hostname length: 11.
+			0x0b,
+			// The hostname itself.
+			'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
+			// Port 8080 in big-endian.
+			0x1f, 0x90,
+		},
+	}
+
+	// testOpaqueAddrWithEmbeddedDNSAddrAndMore is an opaque address that
+	// contains a DNS address within it, along with some extra bytes that
+	// represent some other unknown address type.
+	testOpaqueAddrWithEmbeddedDNSAddrAndMore = &lnwire.OpaqueAddrs{
+		Payload: []byte{
+			// The protocol level type for DNS addresses.
+			0x05,
+			// Hostname length: 11.
+			0x0B,
+			// The hostname itself.
+			'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
+			// port 8080 in big-endian.
+			0x1F, 0x90,
+			// Now we add more opaque bytes to represent more
+			// addresses that we don't know about yet.
+			// NOTE: the 0xff is an address type that we definitely
+			// don't know about yet
+			0xff, 0x02, 0x03, 0x04, 0x05, 0x06,
+		},
+	}
+
+	// testOpaqueAddrWithEmbeddedBadDNSAddr is an opaque address that
+	// contains an invalid DNS address within it.
+	testOpaqueAddrWithEmbeddedBadDNSAddr = &lnwire.OpaqueAddrs{
+		Payload: []byte{
+			// The protocol level type for DNS addresses.
+			0x05,
+			// Hostname length: We set this to a size that is
+			// incorrect in order to simulate the bad DNS address.
+			0xAA,
+			// The hostname itself.
+			'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
+			// port 9735 in big-endian.
+			0x26, 0x07,
+		},
+	}
+
+	// testOpaqueAddrWithTwoEmbeddedDNSAddrs is an opaque address that
+	// contains two valid DNS addresses within it.
+	testOpaqueAddrWithTwoEmbeddedDNSAddrs = &lnwire.OpaqueAddrs{
+		Payload: []byte{
+			// The protocol level type for DNS addresses.
+			0x05,
+			// Hostname length: 11.
+			0x0B,
+			// The hostname itself.
+			'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
+			// port 8080 in big-endian.
+			0x1F, 0x90,
+			// Another DNS address.
+			0x05,
+			0x0B,
+			'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
+			0x1F, 0x90,
+		},
+	}
 )
 
 // TestMigrateGraphToSQL tests various deterministic cases that we want to test
@@ -1174,6 +1246,104 @@ func TestSQLMigrationEdgeCases(t *testing.T) {
 			zombies: []uint64{2},
 		})
 	})
+
+	// We have used this migration as a chance to also extract any DNS
+	// addresses that we previously may have wrapped in an opaque address.
+	// If we do encounter such a case, then the migrated node set will look
+	// slightly different from the original node set in the KV store, and so
+	// we test for that here.
+	t.Run("node with wrapped DNS address inside opaque addr",
+		func(t *testing.T) {
+			t.Parallel()
+
+			var expectedNodes []*models.LightningNode
+
+			// Let the first node have an opaque address that we
+			// still don't understand. This node will remain the
+			// same in the SQL store.
+			n1 := makeTestNode(t, func(n *models.LightningNode) {
+				n.Addresses = []net.Addr{
+					testOpaqueAddr,
+				}
+			})
+			expectedNodes = append(expectedNodes, n1)
+
+			// The second node will have a wrapped DNS address
+			// inside an opaque address. The opaque address will
+			// only contain a DNS address and so the migrated node
+			// will only contain a DNS address and no opaque
+			// address.
+			n2 := makeTestNode(t, func(n *models.LightningNode) {
+				n.Addresses = []net.Addr{
+					testOpaqueAddrWithEmbeddedDNSAddr,
+				}
+			})
+			n2Expected := *n2
+			n2Expected.Addresses = []net.Addr{
+				testDNSAddr,
+			}
+			expectedNodes = append(expectedNodes, &n2Expected)
+
+			// The third node will have an opaque address that
+			// wraps a DNS address along with some other data.
+			// So the resulting migrated node should have both
+			// the DNS address and remaining opaque address data.
+			n3 := makeTestNode(t, func(n *models.LightningNode) {
+				n.Addresses = []net.Addr{
+					//nolint:ll
+					testOpaqueAddrWithEmbeddedDNSAddrAndMore,
+				}
+			})
+			n3Expected := *n3
+			n3Expected.Addresses = []net.Addr{
+				testDNSAddr,
+				testOpaqueAddr,
+			}
+			expectedNodes = append(expectedNodes, &n3Expected)
+
+			// The fourth node will have an opaque address that
+			// wraps an invalid DNS address. Such a node will not be
+			// migrated since propagating an invalid DNS address
+			// is not allowed.
+			n4 := makeTestNode(t, func(n *models.LightningNode) {
+				n.Addresses = []net.Addr{
+					testOpaqueAddrWithEmbeddedBadDNSAddr,
+				}
+			})
+			// NOTE: we don't add this node to the expected nodes
+			// slice.
+
+			// The fifth node will have 2 DNS addresses embedded
+			// in the opaque address. The migration will result
+			// in _both_ dns addresses being extracted. This is
+			// invalid at a protocol level, and so we should not
+			// propagate such addresses, but this is left to higher
+			// level gossip logic.
+			n5 := makeTestNode(t, func(n *models.LightningNode) {
+				n.Addresses = []net.Addr{
+					testOpaqueAddrWithTwoEmbeddedDNSAddrs,
+				}
+			})
+			n5Expected := *n5
+			n5Expected.Addresses = []net.Addr{
+				testDNSAddr,
+				testDNSAddr,
+			}
+			expectedNodes = append(expectedNodes, &n5Expected)
+
+			populateKV := func(t *testing.T, db *KVStore) {
+				require.NoError(t, db.AddLightningNode(ctx, n1))
+				require.NoError(t, db.AddLightningNode(ctx, n2))
+				require.NoError(t, db.AddLightningNode(ctx, n3))
+				require.NoError(t, db.AddLightningNode(ctx, n4))
+				require.NoError(t, db.AddLightningNode(ctx, n5))
+			}
+
+			runTestMigration(t, populateKV, dbState{
+				nodes: expectedNodes,
+			})
+		},
+	)
 }
 
 // runTestMigration is a helper function that sets up the KVStore and SQLStore,