diff --git a/graph/db/sql_migration.go b/graph/db/sql_migration.go index b88518fd8..1071974cb 100644 --- a/graph/db/sql_migration.go +++ b/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, diff --git a/graph/db/sql_migration_test.go b/graph/db/sql_migration_test.go index 90b2a8fd8..cb8443f81 100644 --- a/graph/db/sql_migration_test.go +++ b/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,