Fix TimeStamp issue in the Gossip Syncer #9011

ziggie1984 · 2024-08-14T21:32:47Z

Fixes parts of #8889 especially nr. 1-3 in comment #8889 (comment)

The last commit introduces a check to not query channel which have outdated timestamp data. However I think we should also implement encoded_query_flags of the query_short_channel_ids msg. Meaning that we query for specific msg and therefore anticipate the correct msg from the peer. If the peer cannot send us a ChanUpdate although he claimed to have sone with a specific timestamp in the RangeReply which should probably punish him ?

coderabbitai · 2024-08-14T21:32:54Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

ziggie1984 · 2024-08-14T21:44:17Z

Still need to add unit tests probably, let's see whether the ci fails.

morehouse · 2024-08-14T22:37:33Z

channeldb/graph.go

+				// NOTE: In the past we used to resurrect zombie
+				// channels based on the timestamp value
+				// included in the `ReplyChannelRange` msg,
+				// however this is too risky because there is no
+				// proof that the peer has an `ChanUpdate` msg
+				// with this timestamp or could even just lie to
+				// us. Therefore to resurrect zombie channel we
+				// have to receive the actual `ChanUpdate` msg.
+				if isZombie {


See https://github.com/lightningnetwork/lnd/pull/8030/files#r1394730858.

I flagged this potential issue on the previous PR, and @ellemouton explained that we need zombies to be marked live before the channel announcement comes. Since we're reverting that change, we should make sure channel announcements are still handled correctly.

Hmm thinking about it twice and reading through the thread of the prior issue, I think we can somehow rely on the timestamp in resurrecting the zombie channels, however we should probably built in some banning protection if the peer promises a ChanUpdate with a specific timestamp but is not sending it.

The query msges, at least the range queries built on trust anyways so having a proper banning system in place is more important rather than not trusting the data in the first place ?

So building back ...

Just want to point out that resurrecting a zombie channel just means removing it from the index and not inserting it into the edgeIndexBucket. So I think we can keep it?

if we use %x here we would get the hex representation of the String() method of the vertex, which is wrong.

channeldb/graph.go

ziggie1984 · 2024-08-15T20:09:07Z

channeldb/graph.go

+	}
+
+	// isSkewed returns whether the timestamp is too far into the future.
+	isSkewed := func(timestamp time.Time) bool {


Maybe some services rely on setting the chanupdate far in the future to not have to update gossip very often ?

perhaps but I think they should not be allowed to do this (according to the spec too iirc) & so we should not need to accommodate for that behaviour

ellemouton

Great find - thanks for this! 🙏 ⌛

ellemouton · 2024-08-16T06:39:29Z

channeldb/graph.go

+				Node1UpdateTimestamp: time.Unix(0, 0),
+				Node2UpdateTimestamp: time.Unix(0, 0),


👍 great catch

perhaps we should add a NewChannelUpdateInfo constructor so that this is done by default & we dont need to remember to do it?

ellemouton · 2024-08-16T06:43:11Z

channeldb/graph.go

+				// However we should start punishing peers when
+				// they don't provide us honest data ?


yeah ideally but it is tricky because:

we query them by channel ID so it could be that by the time they reply, they have a new channel update and so will send that new one & so timestamp will not match (but at least should be greater than old one).

Where we currently send this query and where we receive the requested messages is completely disjoint at the moment. So will require some coupling between this and the gossiper.

We could do strict validation for every p2p message. In some cases it's useful, in some cases it's not. I'm not totally convinced that we should add logic for this unless not doing so presents a real problem.

ellemouton · 2024-08-16T06:45:58Z

channeldb/graph.go

+	}
+
+	// isSkewed returns whether the timestamp is too far into the future.
+	isSkewed := func(timestamp time.Time) bool {


perhaps but I think they should not be allowed to do this (according to the spec too iirc) & so we should not need to accommodate for that behaviour

channeldb/graph.go

ellemouton · 2024-08-16T06:51:02Z

channeldb/graph_test.go

@@ -1942,10 +1942,23 @@ func TestFilterKnownChanIDs(t *testing.T) {

 	// If we try to filter out a set of channel ID's before we even know of
 	// any channels, then we should get the entire set back.
+	currentTimeStamp := time.Now()


style nit: should squash this commit into the commit that changes the behaviour (ie, where the unit tests starts failing)

ok squashed the logic change into one commit because changes were all in the same file.

Crypt-iQ

looks good

channeldb/graph.go

ChanUpdate timestamps are now restircted so that they cannot be more than two weeks into the future. Moreover channels with both timestamps in the ReplyChannelRange msg either too far in the past or too far in the future are not queried. Moreover fix unitests.

Describe why it is ok to resurrect zombie channels based on the timestamp of the `ReplyChannelRange` msg although its not verifiable data.

Roasbeef

LGTM 🪢

ziggie1984 self-assigned this Aug 14, 2024

ziggie1984 added bug fix gossip labels Aug 14, 2024

ziggie1984 force-pushed the fix-gossip-syncer branch from 6905143 to f1005e7 Compare August 14, 2024 21:42

ziggie1984 marked this pull request as ready for review August 14, 2024 21:57

morehouse reviewed Aug 14, 2024

View reviewed changes

saubyk added this to the v0.18.3 milestone Aug 15, 2024

ziggie1984 force-pushed the fix-gossip-syncer branch from f1005e7 to 1aee909 Compare August 15, 2024 10:08

discovery: fix log line.

aecfe1c

if we use %x here we would get the hex representation of the String() method of the vertex, which is wrong.

ziggie1984 force-pushed the fix-gossip-syncer branch 2 times, most recently from 2ba4627 to 905cb17 Compare August 15, 2024 12:48

ziggie1984 commented Aug 15, 2024

View reviewed changes

channeldb/graph.go Outdated Show resolved Hide resolved

saubyk requested a review from ellemouton August 15, 2024 15:17

ziggie1984 commented Aug 15, 2024

View reviewed changes

ellemouton approved these changes Aug 16, 2024

View reviewed changes

ziggie1984 force-pushed the fix-gossip-syncer branch 5 times, most recently from 8b13935 to fce74f2 Compare August 16, 2024 10:37

Crypt-iQ reviewed Aug 16, 2024

View reviewed changes

channeldb/graph.go Outdated Show resolved Hide resolved

channeldb/graph.go Outdated Show resolved Hide resolved

ziggie1984 force-pushed the fix-gossip-syncer branch 5 times, most recently from cf122e0 to a46b8d2 Compare August 16, 2024 21:10

ziggie1984 added 2 commits August 17, 2024 04:29

discovery: add detailed comment.

e5d7a7d

Describe why it is ok to resurrect zombie channels based on the timestamp of the `ReplyChannelRange` msg although its not verifiable data.

docs: update release-notes.

fc5b763

ziggie1984 force-pushed the fix-gossip-syncer branch from a46b8d2 to fc5b763 Compare August 17, 2024 02:30

ziggie1984 requested a review from Crypt-iQ August 17, 2024 08:39

Roasbeef approved these changes Aug 19, 2024

View reviewed changes

Roasbeef merged commit b4693b2 into lightningnetwork:master Aug 19, 2024
29 of 34 checks passed

ziggie1984 deleted the fix-gossip-syncer branch August 20, 2024 08:05

Roasbeef mentioned this pull request Aug 27, 2024

release: bump version to v0.18.3 rc2 #9036

Merged

saubyk mentioned this pull request Aug 29, 2024

[bug]: GossipSyncer should remove unreliable Peers from syncing the ChannelGraph #8889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TimeStamp issue in the Gossip Syncer #9011

Fix TimeStamp issue in the Gossip Syncer #9011

ziggie1984 commented Aug 14, 2024 •

edited

Loading

coderabbitai bot commented Aug 14, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

ziggie1984 commented Aug 14, 2024

morehouse Aug 14, 2024

ziggie1984 Aug 15, 2024

ziggie1984 Aug 15, 2024 •

edited

Loading

Crypt-iQ Aug 15, 2024

ziggie1984 Aug 15, 2024

ellemouton Aug 16, 2024

ellemouton left a comment

ellemouton Aug 16, 2024

ellemouton Aug 16, 2024

ellemouton Aug 16, 2024

Crypt-iQ Aug 16, 2024

ellemouton Aug 16, 2024

ellemouton Aug 16, 2024

ziggie1984 Aug 16, 2024

Crypt-iQ left a comment

Roasbeef left a comment

		Node1UpdateTimestamp: time.Unix(0, 0),
		Node2UpdateTimestamp: time.Unix(0, 0),

		// However we should start punishing peers when
		// they don't provide us honest data ?

Fix TimeStamp issue in the Gossip Syncer #9011

Fix TimeStamp issue in the Gossip Syncer #9011

Conversation

ziggie1984 commented Aug 14, 2024 • edited Loading

coderabbitai bot commented Aug 14, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

ziggie1984 commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

ziggie1984 commented Aug 14, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

ziggie1984 Aug 15, 2024 •

edited

Loading