multi: query chan update timestamps #8030

ellemouton · 2023-09-25T12:20:33Z

In this PR, we start supporting the timestamps query option in the query_channel_range message. If we receive such a message with that bit set then we now populate the timestamps field of the reply_channel_range message.

With this feature we can now ask for the timestamps for the latest channel updates for a channel to be sent along with the SCID. Thus, if we have marked one of the channels in the list as a zombie but we see that the timestamp sent with it make it not a zombie, then we request the channel_update messages for that channel so that we can perhaps awaken the channel.

How to test?

See this comment: #8030 (comment)

Fixes #7989

Roasbeef

Nice work! Main comment re if we should attempt to sanity check the timestamps we get from the remote party.

One way to test this out in the wild would be: create a new node on mainnet, no channels, set the zombie prune horizon to something short like 6 hrs, run this patch, and observe the zombie resurrection happen or not. We might want to add some extra logging statements wehn we realize we need to go fetch a zombie chan due to getting an update.

lnwire/query_channel_range.go

Roasbeef · 2023-10-20T00:45:31Z

lnwire/encoding.go

+// Encoding is an enum-like type that represents exactly how a set data is
+// encoded on the wire. The set of encodings allows us to take advantage of the
+// structure of the given list to achieving a high degree of compression.
+type Encoding uint8


ScidEncoding? As not a generic encoding that'll allow say proto buf instead of tlv or w/e.

The thing is that it isnt only used for Scid's though. It just defines how a wire message field type is encoded.

See in the commit named lnwire: add timestamps to ReplyChannelRange msg how it is used to maybe encode timestamps.

I could rename to ByteEncoding if that helps? just went for the terminology used in bolt 7

I also think Encoding seems too generic. Some other ideas:

SortedEncoding

QueryEncoding

went with QueryEncoding 👍

lnwire/timestamps.go

channeldb/graph.go

discovery/gossiper.go

routing/router.go

Roasbeef · 2023-10-20T01:08:46Z

channeldb/graph.go

+				isZombie, _, _ := isZombieEdge(
+					zombieIndex, scid,
+				)
+				if isZombie && isZombieChan(


Ah yeah, this makes sense, so scratch the idea about the cache I had. I had envisioned a look up here each time to double check our timestamp with theirs.

Thinking about it a bit more, perhaps we should do some sort of sanity check? Otherwise, they can send bogus timestamps and make us request everything all over again.

How would such a sanity check work? I imagine a malicious peer could trivially defeat any check by setting timestamps to a recent time.

Crypt-iQ

Still in the process of reviewing, but I think the chunkSize needs to be updated if timestamps are sent because instead of room for ~8000 scid's, there's only room for ~4000 because of the 2 4-byte timestamps

ellemouton · 2023-11-08T12:36:05Z

Still in the process of reviewing, but I think the chunkSize needs to be updated if timestamps are sent because instead of room for ~8000 scid's, there's only room for ~4000 because of the 2 4-byte timestamps

Great catch - thanks! Addressed 👍

Main comment re if we should attempt to sanity check the timestamps we get from the remote party.

👍 for now i've added a place holder that currently only checks that the times are not in the future. Will ponder more over if more can be done & if it is worth doing something more complicated like checking that what they end up sending matches the timestamps they claimed to have.

One way to test this out in the wild would be: create a new node on mainnet, no channels, set the zombie prune horizon to something short like 6 hrs, run this patch, and observe the zombie resurrection happen or not. We might want to add some extra logging statements wehn we realize we need to go fetch a zombie chan due to getting an update.

Good idea 👍 I'll do this next

ellemouton · 2023-11-13T13:15:07Z

Did the following manual testing:

Confirmed issue:

Set up a regtest env where there are a bunch of chans but main node, alice, has no chans but learns of all the other channels.
Set alice's ChannelPruneExpiry and GraphPruneInterval to low values so that the node quickly marks all the other channels as zombies. (a quick way to verify this is to do a describegraph and see that there are no edges.
Shutdown alice
now perform some channel updates on the existing channels so that there are some ChannelUpdates that alice misses while the node is offline.
Start alice , connect alice to a node in the network & see that Alice ignores all the SCIDs in the lnwire.ReplyChannelRange message due to them all being marked as zombies. (describegraph still empty).

PR fixes issue:

Restart alice with this PR and perform the above steps. At step 5, alice will mark the channels as alive and will re-add them to her graph.

ellemouton · 2023-11-13T13:32:46Z

This is ready for another round (see above comment for how to test) - the final thing I need to ponder on is how strict we really need to be with the timestamps they send us.

saubyk · 2023-11-13T15:42:33Z

...
5. Start alice , connect alice to a node in the network & see that Alice ignores all the SCIDs in the lnwire.ReplyChannelRange message due to them all being marked as zombies. (describegraph still empty).

PR fixes issue:

Restart alice with this PR and perform the above steps. At step 5, alice will mark the channels as alive and will re-add them to her graph.

Great explanation of the test 🤩

Concept ACK

discovery/syncer.go

morehouse · 2023-11-21T17:37:30Z

lnwire/encoding.go

+// Encoding is an enum-like type that represents exactly how a set data is
+// encoded on the wire. The set of encodings allows us to take advantage of the
+// structure of the given list to achieving a high degree of compression.
+type Encoding uint8


I also think Encoding seems too generic. Some other ideas:

SortedEncoding

QueryEncoding

lnwire/encoding.go

lnwire/timestamps.go

lnwire/reply_channel_range.go

lnwire/reply_channel_range_test.go

channeldb/graph.go

morehouse

LGTM

channeldb/graph.go

This commit adds a new QueryOptions type which will later be used in the QueryReplyRange message.

Since the the encoding can be used for multiple different fields, we rename it here to be more generic.

This is for later when the timestamps also need to be sorted according to the SCIDs.

The `edgeIndexBucket` key name is used to key a sub-bucket within the `edgeBucket` bucket. And so the `edgeIndexBucket` is not a top level bucket.

This commit also adds a new `protocol.no-timestamp-query-option` option to disable the new behaviour.

This commit is a set-up commit. It extracts the logic from `MarkEdgeLive` to a helper `markEdgeLive` method so that the logic can be called from within other kvdb.Update blocks. This will be used in the next commit.

lightninglabs-deploy · 2023-12-11T16:59:59Z

@Roasbeef: review reminder

Roasbeef

LGTM 🌲

ellemouton force-pushed the queryChanUpdateTimestamps branch from afc8971 to d922d67 Compare September 25, 2023 13:10

saubyk added this to the v0.18.0 milestone Sep 25, 2023

Roasbeef requested changes Oct 20, 2023

View reviewed changes

saubyk assigned ellemouton Oct 28, 2023

saubyk added gossip graph wire protocol Encoding of messages for the communication between nodes labels Oct 28, 2023

Crypt-iQ reviewed Nov 3, 2023

View reviewed changes

ellemouton force-pushed the queryChanUpdateTimestamps branch 2 times, most recently from 481d366 to 65062c4 Compare November 8, 2023 12:35

ellemouton force-pushed the queryChanUpdateTimestamps branch 2 times, most recently from 2034c28 to 24dcb3a Compare November 8, 2023 13:31

Crypt-iQ mentioned this pull request Nov 8, 2023

lnwire: add wire types for dynamic commitments #8026

Merged

7 tasks

ellemouton force-pushed the queryChanUpdateTimestamps branch 9 times, most recently from bd51d8c to e3aa151 Compare November 13, 2023 12:43

ellemouton requested review from Roasbeef and Crypt-iQ November 13, 2023 13:32

ellemouton force-pushed the queryChanUpdateTimestamps branch from e3aa151 to f33cd7e Compare November 14, 2023 09:07

Crypt-iQ reviewed Nov 15, 2023

View reviewed changes

discovery/syncer.go Outdated Show resolved Hide resolved

morehouse reviewed Nov 21, 2023

View reviewed changes

ellemouton force-pushed the queryChanUpdateTimestamps branch 7 times, most recently from bd50f11 to 2bebc49 Compare November 22, 2023 11:17

ellemouton requested a review from morehouse November 22, 2023 13:11

morehouse approved these changes Nov 27, 2023

View reviewed changes

channeldb/graph.go Show resolved Hide resolved

ellemouton added 14 commits December 11, 2023 09:12

lnwire: add QueryOptions type

8efd141

This commit adds a new QueryOptions type which will later be used in the QueryReplyRange message.

lnwire: add QueryOptions to QueryChannelRange

341bae0

lnwire+discovery: rename ShortChannelIDEncoding to QueryEncoding

c882223

Since the the encoding can be used for multiple different fields, we rename it here to be more generic.

lnwire: sort scids before encoding

4872010

This is for later when the timestamps also need to be sorted according to the SCIDs.

lnwire: add timestamps to ReplyChannelRange msg

49a0370

channeldb: don't initialise edge index bkt as top level

893147d

The `edgeIndexBucket` key name is used to key a sub-bucket within the `edgeBucket` bucket. And so the `edgeIndexBucket` is not a top level bucket.

multi: start asking for update timestamps in QueryChannelRange

a439cc9

This commit also adds a new `protocol.no-timestamp-query-option` option to disable the new behaviour.

channeldb: reformat TestFilterChannelRange

50e17da

channeldb+discovery: fetch timestamps from DB if required

0ad4ef3

discovery: start sending timestamps if requested

67df687

multi: extract and pass through IsZombieChannel method

e12321c

channeldb: make MarkEdgeLive logic reusable

ebe0755

This commit is a set-up commit. It extracts the logic from `MarkEdgeLive` to a helper `markEdgeLive` method so that the logic can be called from within other kvdb.Update blocks. This will be used in the next commit.

channeldb+discovery: use timestamps to maybe revive zombie

a242ad5

docs: update release notes

645eb44

ellemouton force-pushed the queryChanUpdateTimestamps branch from 2bebc49 to 645eb44 Compare December 11, 2023 07:13

Roasbeef approved these changes Dec 11, 2023

View reviewed changes

Roasbeef merged commit 2fee3f6 into lightningnetwork:master Dec 11, 2023
24 of 25 checks passed

ellemouton deleted the queryChanUpdateTimestamps branch December 11, 2023 19:28

ellemouton mentioned this pull request Apr 5, 2024

Problem with update_channel gossip propagation across LND nodes (wrongly marked as zombie channels?). #8624

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: query chan update timestamps #8030

multi: query chan update timestamps #8030

ellemouton commented Sep 25, 2023 •

edited

Loading

Roasbeef left a comment

Roasbeef Oct 20, 2023

ellemouton Nov 8, 2023

morehouse Nov 21, 2023

ellemouton Nov 22, 2023

Roasbeef Oct 20, 2023

morehouse Nov 6, 2023

Crypt-iQ left a comment

ellemouton commented Nov 8, 2023

ellemouton commented Nov 13, 2023

ellemouton commented Nov 13, 2023

saubyk commented Nov 13, 2023

PR fixes issue:

morehouse Nov 21, 2023

morehouse left a comment

lightninglabs-deploy commented Dec 11, 2023

Roasbeef left a comment

multi: query chan update timestamps #8030

multi: query chan update timestamps #8030

Conversation

ellemouton commented Sep 25, 2023 • edited Loading

How to test?

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Oct 20, 2023

Choose a reason for hiding this comment

ellemouton Nov 8, 2023

Choose a reason for hiding this comment

morehouse Nov 21, 2023

Choose a reason for hiding this comment

ellemouton Nov 22, 2023

Choose a reason for hiding this comment

Roasbeef Oct 20, 2023

Choose a reason for hiding this comment

morehouse Nov 6, 2023

Choose a reason for hiding this comment

Crypt-iQ left a comment

Choose a reason for hiding this comment

ellemouton commented Nov 8, 2023

ellemouton commented Nov 13, 2023

Confirmed issue:

PR fixes issue:

ellemouton commented Nov 13, 2023

saubyk commented Nov 13, 2023

PR fixes issue:

morehouse Nov 21, 2023

Choose a reason for hiding this comment

morehouse left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Dec 11, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

ellemouton commented Sep 25, 2023 •

edited

Loading