Skip to content

Commit af380c9

Browse files
ellemoutonellemouton
committed
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.
1 parent d61a8af commit af380c9

File tree

2 files changed

+249
-10
lines changed

2 files changed

+249
-10
lines changed

‎graph/db/sql_migration.go‎

Lines changed: 79 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -260,18 +260,17 @@ func migrateNodes(ctx context.Context, cfg *sqldb.QueryConfig,
260260
"opaque data for node %x: %w", pub, err)
261261
}
262262

263+
if err = maybeOverrideNodeAddresses(node); err != nil {
264+
skipped++
265+
log.Warnf("Skipping migration of node %x with invalid "+
266+
"address (%v): %v", pub, node.Addresses, err)
267+
268+
return nil
269+
}
270+
263271
count++
264272
chunk++
265273

266-
// TODO(elle): At this point, we should check the loaded node
267-
// to see if we should extract any DNS addresses from its
268-
// opaque type addresses. This is expected to be done in:
269-
// https://github.com/lightningnetwork/lnd/pull/9455.
270-
// This TODO is being tracked in
271-
// https://github.com/lightningnetwork/lnd/issues/9795 as this
272-
// must be addressed before making this code path active in
273-
// production.
274-
275274
// Write the node to the SQL database.
276275
id, err := insertNodeSQLMig(ctx, sqlDB, node)
277276
if err != nil {
@@ -323,12 +322,82 @@ func migrateNodes(ctx context.Context, cfg *sqldb.QueryConfig,
323322
}
324323

325324
log.Infof("Migrated %d nodes from KV to SQL in %v (skipped %d nodes "+
326-
"due to invalid TLV streams)", count, time.Since(totalTime),
325+
"due to invalid TLV streams or invalid addresses)", count,
326+
time.Since(totalTime),
327+
327328
skipped)
328329

329330
return nil
330331
}
331332

333+
// maybeOverrideNodeAddresses checks if the node has any opaque addresses that
334+
// can be parsed. If so, it replaces the node's addresses with the parsed
335+
// addresses. If the address is unparseable, it returns an error.
336+
func maybeOverrideNodeAddresses(node *models.LightningNode) error {
337+
// In the majority of cases, the number of node addresses will remain
338+
// unchanged, so we pre-allocate a slice of the same length.
339+
addrs := make([]net.Addr, 0, len(node.Addresses))
340+
341+
// Iterate over each address in search of any opaque addresses that we
342+
// can inspect.
343+
for _, addr := range node.Addresses {
344+
opaque, ok := addr.(*lnwire.OpaqueAddrs)
345+
if !ok {
346+
// Any non-opaque address is left unchanged.
347+
addrs = append(addrs, addr)
348+
continue
349+
}
350+
351+
// For each opaque address, we'll now attempt to parse out any
352+
// known addresses. We'll do this in a loop, as it's possible
353+
// that there are several addresses encoded in a single opaque
354+
// address.
355+
payload := opaque.Payload
356+
for len(payload) > 0 {
357+
var (
358+
r = bytes.NewReader(payload)
359+
numAddrBytes = uint16(len(payload))
360+
)
361+
byteRead, readAddr, err := lnwire.ReadAddress(
362+
r, numAddrBytes,
363+
)
364+
if err != nil {
365+
return err
366+
}
367+
368+
// If we were able to read an address, we'll add it to
369+
// our list of addresses.
370+
if readAddr != nil {
371+
addrs = append(addrs, readAddr)
372+
}
373+
374+
// If the address we read was an opaque address, it
375+
// means we've hit an unknown address type, and it has
376+
// consumed the rest of the payload. We can break out
377+
// of the loop.
378+
if _, ok := readAddr.(*lnwire.OpaqueAddrs); ok {
379+
break
380+
}
381+
382+
// If we've read all the bytes, we can also break.
383+
if byteRead >= numAddrBytes {
384+
break
385+
}
386+
387+
// Otherwise, we'll advance our payload slice and
388+
// continue.
389+
payload = payload[byteRead:]
390+
}
391+
}
392+
393+
// Override the node addresses if we have any.
394+
if len(addrs) != 0 {
395+
node.Addresses = addrs
396+
}
397+
398+
return nil
399+
}
400+
332401
// migrateSourceNode migrates the source node from the KV backend to the
333402
// SQL database.
334403
func migrateSourceNode(ctx context.Context, kvdb kvdb.Backend,

‎graph/db/sql_migration_test.go‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,78 @@ var (
4949
BitcoinSig1Bytes: testSig.Serialize(),
5050
BitcoinSig2Bytes: testSig.Serialize(),
5151
}
52+
53+
// testOpaqueAddrWithEmbeddedDNSAddr is an opaque address that contains
54+
// a single DNS address within it.
55+
testOpaqueAddrWithEmbeddedDNSAddr = &lnwire.OpaqueAddrs{
56+
Payload: []byte{
57+
// The protocol level type for DNS addresses.
58+
0x05,
59+
// Hostname length: 11.
60+
0x0b,
61+
// The hostname itself.
62+
'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
63+
// Port 8080 in big-endian.
64+
0x1f, 0x90,
65+
},
66+
}
67+
68+
// testOpaqueAddrWithEmbeddedDNSAddrAndMore is an opaque address that
69+
// contains a DNS address within it, along with some extra bytes that
70+
// represent some other unknown address type.
71+
testOpaqueAddrWithEmbeddedDNSAddrAndMore = &lnwire.OpaqueAddrs{
72+
Payload: []byte{
73+
// The protocol level type for DNS addresses.
74+
0x05,
75+
// Hostname length: 11.
76+
0x0B,
77+
// The hostname itself.
78+
'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
79+
// port 8080 in big-endian.
80+
0x1F, 0x90,
81+
// Now we add more opaque bytes to represent more
82+
// addresses that we don't know about yet.
83+
// NOTE: the 0xff is an address type that we definitely
84+
// don't know about yet
85+
0xff, 0x02, 0x03, 0x04, 0x05, 0x06,
86+
},
87+
}
88+
89+
// testOpaqueAddrWithEmbeddedBadDNSAddr is an opaque address that
90+
// contains an invalid DNS address within it.
91+
testOpaqueAddrWithEmbeddedBadDNSAddr = &lnwire.OpaqueAddrs{
92+
Payload: []byte{
93+
// The protocol level type for DNS addresses.
94+
0x05,
95+
// Hostname length: We set this to a size that is
96+
// incorrect in order to simulate the bad DNS address.
97+
0xAA,
98+
// The hostname itself.
99+
'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
100+
// port 9735 in big-endian.
101+
0x26, 0x07,
102+
},
103+
}
104+
105+
// testOpaqueAddrWithTwoEmbeddedDNSAddrs is an opaque address that
106+
// contains two valid DNS addresses within it.
107+
testOpaqueAddrWithTwoEmbeddedDNSAddrs = &lnwire.OpaqueAddrs{
108+
Payload: []byte{
109+
// The protocol level type for DNS addresses.
110+
0x05,
111+
// Hostname length: 11.
112+
0x0B,
113+
// The hostname itself.
114+
'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
115+
// port 8080 in big-endian.
116+
0x1F, 0x90,
117+
// Another DNS address.
118+
0x05,
119+
0x0B,
120+
'e', 'x', 'a', 'm', 'p', 'l', 'e', '.', 'c', 'o', 'm',
121+
0x1F, 0x90,
122+
},
123+
}
52124
)
53125

54126
// TestMigrateGraphToSQL tests various deterministic cases that we want to test
@@ -1174,6 +1246,104 @@ func TestSQLMigrationEdgeCases(t *testing.T) {
11741246
zombies: []uint64{2},
11751247
})
11761248
})
1249+
1250+
// We have used this migration as a chance to also extract any DNS
1251+
// addresses that we previously may have wrapped in an opaque address.
1252+
// If we do encounter such a case, then the migrated node set will look
1253+
// slightly different from the original node set in the KV store, and so
1254+
// we test for that here.
1255+
t.Run("node with wrapped DNS address inside opaque addr",
1256+
func(t *testing.T) {
1257+
t.Parallel()
1258+
1259+
var expectedNodes []*models.LightningNode
1260+
1261+
// Let the first node have an opaque address that we
1262+
// still don't understand. This node will remain the
1263+
// same in the SQL store.
1264+
n1 := makeTestNode(t, func(n *models.LightningNode) {
1265+
n.Addresses = []net.Addr{
1266+
testOpaqueAddr,
1267+
}
1268+
})
1269+
expectedNodes = append(expectedNodes, n1)
1270+
1271+
// The second node will have a wrapped DNS address
1272+
// inside an opaque address. The opaque address will
1273+
// only contain a DNS address and so the migrated node
1274+
// will only contain a DNS address and no opaque
1275+
// address.
1276+
n2 := makeTestNode(t, func(n *models.LightningNode) {
1277+
n.Addresses = []net.Addr{
1278+
testOpaqueAddrWithEmbeddedDNSAddr,
1279+
}
1280+
})
1281+
n2Expected := *n2
1282+
n2Expected.Addresses = []net.Addr{
1283+
testDNSAddr,
1284+
}
1285+
expectedNodes = append(expectedNodes, &n2Expected)
1286+
1287+
// The third node will have an opaque address that
1288+
// wraps a DNS address along with some other data.
1289+
// So the resulting migrated node should have both
1290+
// the DNS address and remaining opaque address data.
1291+
n3 := makeTestNode(t, func(n *models.LightningNode) {
1292+
n.Addresses = []net.Addr{
1293+
//nolint:ll
1294+
testOpaqueAddrWithEmbeddedDNSAddrAndMore,
1295+
}
1296+
})
1297+
n3Expected := *n3
1298+
n3Expected.Addresses = []net.Addr{
1299+
testDNSAddr,
1300+
testOpaqueAddr,
1301+
}
1302+
expectedNodes = append(expectedNodes, &n3Expected)
1303+
1304+
// The fourth node will have an opaque address that
1305+
// wraps an invalid DNS address. Such a node will not be
1306+
// migrated since propagating an invalid DNS address
1307+
// is not allowed.
1308+
n4 := makeTestNode(t, func(n *models.LightningNode) {
1309+
n.Addresses = []net.Addr{
1310+
testOpaqueAddrWithEmbeddedBadDNSAddr,
1311+
}
1312+
})
1313+
// NOTE: we don't add this node to the expected nodes
1314+
// slice.
1315+
1316+
// The fifth node will have 2 DNS addresses embedded
1317+
// in the opaque address. The migration will result
1318+
// in _both_ dns addresses being extracted. This is
1319+
// invalid at a protocol level, and so we should not
1320+
// propagate such addresses, but this is left to higher
1321+
// level gossip logic.
1322+
n5 := makeTestNode(t, func(n *models.LightningNode) {
1323+
n.Addresses = []net.Addr{
1324+
testOpaqueAddrWithTwoEmbeddedDNSAddrs,
1325+
}
1326+
})
1327+
n5Expected := *n5
1328+
n5Expected.Addresses = []net.Addr{
1329+
testDNSAddr,
1330+
testDNSAddr,
1331+
}
1332+
expectedNodes = append(expectedNodes, &n5Expected)
1333+
1334+
populateKV := func(t *testing.T, db *KVStore) {
1335+
require.NoError(t, db.AddLightningNode(ctx, n1))
1336+
require.NoError(t, db.AddLightningNode(ctx, n2))
1337+
require.NoError(t, db.AddLightningNode(ctx, n3))
1338+
require.NoError(t, db.AddLightningNode(ctx, n4))
1339+
require.NoError(t, db.AddLightningNode(ctx, n5))
1340+
}
1341+
1342+
runTestMigration(t, populateKV, dbState{
1343+
nodes: expectedNodes,
1344+
})
1345+
},
1346+
)
11771347
}
11781348

11791349
// runTestMigration is a helper function that sets up the KVStore and SQLStore,

0 commit comments

Comments
 (0)