multi: make reassignment of alias channel edge atomic by ziggie1984 · Pull Request #8777 · lightningnetwork/lnd

ziggie1984 · 2024-05-24T10:59:13Z

When the option SCID is used we need to make sure we update the
channel data in an atomic way so that we don't miss any updates
by our peer because we rate limit ChanUpdate msg which can cause some side effects revealed in #8582

Prerequisite for: #8582

EDIT:

So the main rational behind this change is, that we need to make sure that in case a channel is an alias channel (zero-conf or non-zeroconf) that we "ReAdd" the edge info atomically. This guarantees that we delete all data which might still have the alias channel as a channel id and moreover we need to do it atomically so that we do not receive updates in the meantime and therefore rate-limit the gossip-messages.

coderabbitai · 2024-05-24T10:59:47Z

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>, please review it.
- 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @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 using 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 generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

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-05-24T11:08:00Z

@Crypt-iQ could you take a look and evaluate if this is the right direction I am going, it relates to the atomic deletion of the option-scid channels also discussed here(#7759 (comment))

Thank you in advance 🙏

Crypt-iQ · 2024-06-18T17:10:17Z

@Crypt-iQ could you take a look and evaluate if this is the right direction I am going, it relates to the atomic deletion of the option-scid channels also discussed here(#7759 (comment))

Thank you in advance 🙏

Yes this approach looks good

starius

LGTM 🎉
I left comments, mostly nits. One important question is if the announcement is still broadcasted to the channel peer as before (via addToGraph). See the question in the first commit.

starius · 2024-12-16T15:51:32Z

+
+	// Define the new channel id (under real conditions this is the new
+	// confirmed channel id).
+	newChanSCID := rand.Uint64()


Let's make it more deterministic and exclude (small but real) probability of newChanSCID == oldSCID:

newChanSCID := oldSCID

BTW what happens if ReAddChannelEdge is called with newChanSCID == oldSCID? IIUC, it succeeds. But this is not expected that newChanSCID == oldSCID, so maybe we should change this to fail with an error "new channel ID is equal to old channel ID" and add a test for such edge case here?

no it should succeed aslo with the same SCID, in case of an option-scid channel but not zeroconf.

But made the newChanID deterministic:

newChanSCID := oldSCID + 1

no it should succeed aslo with the same SCID, in case of an option-scid channel but not zeroconf.

I propose to unit test that when the same SCID is passed, ReAddChannelEdge succeeds in case of an option-scid channel, but fails for a zeroconf channel.

hmm I think I was not clear in my description what I meant is that for zeroconf-alias channels the channelid for the reassignment is always different,
For non-zeroconf alias channels the ChannelID for the reassignment is always the same, we do this only to clear the graph data storage to remove potential ChanUpdates which where based on the alias not the channelID. Which might happen in the period between channelconfirmation (usually 3 blocks) and the 6 blocks public gossip limit.

starius · 2024-12-17T21:18:18Z

-		err = f.addToGraph(
-			c, &confChan.shortChanID, nil, ourPolicy,
+		aliasScid := c.ShortChanID()
+		confirmedScid := confChan.shortChanID


The variables can be used above in the ReAssignSCID call and below in the sendChanUpdate call.

looks like this is not addressed

starius · 2024-12-17T21:22:47Z

+
+	// Define the new channel id (under real conditions this is the new
+	// confirmed channel id).
+	newChanSCID := rand.Uint64()


no it should succeed aslo with the same SCID, in case of an option-scid channel but not zeroconf.

I propose to unit test that when the same SCID is passed, ReAddChannelEdge succeeds in case of an option-scid channel, but fails for a zeroconf channel.

starius

LGTM! 🌴
The code looks good!
Few suggestions about tests coverage and removing copy-paste.

starius · 2024-12-17T21:49:04Z

+			err = f.sendChanUpdate(
+				completeChan, &baseScid, ourPolicy,


The unit tests pass even with this call commented out. I propose to cover this case of announcement sending in unit tests as well.

Another idea. DeleteSixConfs, ReAssignSCID, and sendChanUpdate is a common sequence in both cases where they are used. Does it make sense to create a method doing DeleteSixConfs, ReAssignSCID, and sendChanUpdate and reuse it in both cases? It case there will be a third case in the future, there will be less chances to use incorrectly (e.g. to forget to call sendChanUpdate).

yyforyongyu

Thanks for the fix! I think it may explain some of the flakes I've seen from the itest. Meanwhile, could you do a rebase on #9368 and change the base branch to that one? This way we can get a clean CI build and I wanna analyze the logs to understand the changes here.

yyforyongyu · 2024-12-19T02:20:24Z

+// sendChanUpdate sends a ChannelUpdate to the gossiper which is as a
+// consequence sent to the peer.
+//
+// TODO(ziggie): Refactor the gossip msges so that not always all msges have


msges -> msgs

unresolving as it's not addressed

yyforyongyu · 2024-12-19T02:27:27Z

+	errChan := f.cfg.SendAnnouncement(ann.chanUpdateAnn)
+	select {
+	case err := <-errChan:
+		if err != nil {


can be simplified to,

if graph.IsError(err, graph.ErrOutdated, graph.ErrIgnored) { log.Debugf("Graph rejected ChannelUpdate: %v", err) return nil } if err != nil { return fmt.Errorf("error sending channel update: %v", err) }

or just,

if graph.IsError(err, graph.ErrOutdated, graph.ErrIgnored) { log.Debugf("Graph rejected ChannelUpdate: %v", err) return nil } return err

since all the returned error from sendChanUpdate have already been wrapped with contexts and save us from a long chained error msgs.

not addressed

yyforyongyu · 2024-12-19T02:37:32Z

-	fwdMinHTLC, fwdMaxHTLC := f.extractAnnounceParams(completeChan)
-
-	ann, err := f.newChanAnnouncement(
+	chanAnn, err := f.newChanAnnouncement(


I think this belongs to another PR - since the PR is aimed at fixing the atomic behavior, and this commit is touching a different aspect (gossip), and I think there's already a de-dup logic in our gossiper?

true, I was more referring to the creation of these messages, we do not always use all of them so I decided to not create them in the first place.

Sure - in the case we should fix it in a different PR, I also wanna this change as this commit also fixes the confusing names such as newChanAnnouncement. Plus I don't see any dependencies between the atomic fix and this depup fix?

yyforyongyu · 2024-12-20T05:24:15Z

-	// DeleteAliasEdge allows the Manager to delete an alias channel edge
-	// from the graph. It also returns our local to-be-deleted policy.
-	DeleteAliasEdge func(scid lnwire.ShortChannelID) (
+	// ReassignSCID allows the Manager to assign a new SCID to an


ReassignSCID -> ReAssignSCID

yyforyongyu · 2024-12-20T06:17:13Z

 				"chan_id=%v", msg.ChannelID)
 		}

+		// Look up the funding pk script so that we can register the


In case we assume valid channels or have alias channels we make
sure we also add the channel point to the block filter so that
we are notified if the channel point is spent in a block.

Did you encounter this or can you demonstrate the behavior in a test? I think we have itests for the alias channels and the UTXO filter seems to work fine?

yes the zeroconf channel test is failing if we don't do this because we would never be notified if the channel closed because for alias channels we are not funneling through the channel_announcement again as before, we are just swapping it in the ReAssignmentSCID method, before we would send it again via addToGraph.

don't we already UpdateFilter below after validating the funding outpoint?

yyforyongyu · 2024-12-20T06:20:13Z

-	fwdMinHTLC, fwdMaxHTLC := f.extractAnnounceParams(completeChan)
-
-	ann, err := f.newChanAnnouncement(
+	chanAnn, err := f.newChanAnnouncement(


Sure - in the case we should fix it in a different PR, I also wanna this change as this commit also fixes the confusing names such as newChanAnnouncement. Plus I don't see any dependencies between the atomic fix and this depup fix?

yyforyongyu · 2024-12-20T07:07:52Z

+		// reassigned with the new confirmed scid. Moreover channel
+		// updates with the alias scid are removed so that we do not
+		// relay them to the broader network.
+		ourPolicy, err := f.cfg.ReAssignSCID(


I don't think this fixes the issue mentioned in the PR description re the possible race. We can still receive an channel_update from the peer while we are in the process of re-assigning the SCID. Meanwhile, in the gossiper, if we receive a channel_update that we don't understand, it's put in the prematureChannelUpdates and will be processed later.

I described the problem in detail here: ef6cd7a

Looking back I think this issue could have been solved by deleting the rejectCache before adding the edge via the addToGraph function. Tho I think having the deletion and addition atomic is the better desing wdyt ?

yyforyongyu · 2024-12-20T07:14:34Z

-			// addToGraph. This is because the peer may have
-			// sent us a ChannelUpdate with an alias and we don't
-			// want to relay this.
-			ourPolicy, err := f.cfg.DeleteAliasEdge(baseScid)


I think the actually issue is we call DeleteSixConfs too early - if we only call it once the edge is recreated, we can fix the case where the peer sends a channel_update using alias?

Hmm I don't think so, we don't want this ChanUpdate with the Alias so we need to delete the mapping earlier so we do not risk adding the Alias ChanUpdate after we readded the Edge ?

yyforyongyu · 2024-12-20T07:17:00Z

+// database and adds the new edge to guarantee atomicity.
+// This is important for option-scid-alias channels which may change its SCID
+// over the course of its lifetime (e.g., public zero-conf channels).
+func (c *ChannelGraph) ReAddChannelEdge(


A typical flow for adding an edge,

ChannelAnnouncement msg received by the gossiper

the gossiper validates it and sends it to the channel graph

the channel graph validates and adds it to the graph db

And a similar flow is used for ChannelUpdate, but now we just bypass all the validations here?

I think it is ok to skip the validation for the ChannelAnnouncement we created ourselves ? Why should we not trust our own ChannelAnnouncment and ChannelUpdate when its internally sourced ?

The point is to make sure there are no side effects - the full chain of processing ChannelUpdate consists of fundingManager -> gossiper -> router, each processing the msg and updating their states. One example is the validation barrier, where the router and gossiper each have one to ensure the ordering of msgs. Basically I need more time to access the flow changes here, so far it looks good as indicated by the itests, although the behavior does change as I'm seeing this log which is weird,

2025-01-20 20:09:57.115 [DBG] DISC gossiper.go:3546: Unable to fetch node announcement for [Dave]: node does not have node announcement

Going into detail in this case I think this was also a side effect before this PR, I guess you also noticed that we prioritize the AnnouncementSig before the NodeAnnouncement. IMO it does not make sense to wait for the nodeAnnouncment here but rather add it to the msges we propagate if it's available or just neglect it.

This will make it simple in the next commits to test soley single messages rather than always the combination of announcement and channel update msg.

When the option SCID is used we need to make sure we update the channel data in an atomic way so that we don't miss any updates by our peer and remove all edge info which still uses the alias as the channel id (e.g. ChannelUpdate msgs). Moreover add unit test for the new ReAddEdge method.

In case we assume valid channels or have alias channels we make sure we also add the channel point to the block filter so that we are notified if the channel point is spent in a block.

yyforyongyu

Can be rebased now

yyforyongyu · 2025-01-17T14:29:25Z

+					"ChannelUpdate: %v", err)
+			} else {
+				return fmt.Errorf("error sending channel "+
+					"update: %v", err)


%v -> %w, weird the linter didn't catch this case.

yyforyongyu · 2025-01-20T10:53:36Z

-		err = f.addToGraph(
-			c, &confChan.shortChanID, nil, ourPolicy,
+		aliasScid := c.ShortChanID()
+		confirmedScid := confChan.shortChanID


looks like this is not addressed

yyforyongyu · 2025-01-20T10:54:47Z

+	errChan := f.cfg.SendAnnouncement(ann.chanUpdateAnn)
+	select {
+	case err := <-errChan:
+		if err != nil {


not addressed

yyforyongyu · 2025-01-20T10:55:29Z

+
+	t.Helper()
+
+	// Validate custom parameter arrays have expected length


nit: missing a period

yyforyongyu · 2025-01-20T11:00:04Z

+	nodes := []*testNode{alice, bob}
+	for i, node := range nodes {
+		// Each node should send exactly 2 announcements
+		// ChannelAnnouncement and ChannelUpdate.


think we need to assert there are no ChannelAnnouncement otherwise it behaves the same as assertChannelAnnouncements?

yyforyongyu · 2025-01-20T11:02:10Z

+		graph.db, node1, node2,
+	)
+	if err := graph.AddChannelEdge(edgeInfo); err != nil {
+		t.Fatalf("unable to create channel edge: %v", err)


nit: use require.NoError

yyforyongyu · 2025-01-20T11:06:49Z

+	require.ErrorContains(t, err, "ChannelID doesn't match")
+
+	// Edge 2 should be nil, because we deleted the former peer data.
+	require.Nil(t, newEdge2)


Does it mean after this change the newEdge2 is temporarily missing until the remote sends us a ChannelUpdate?

yyforyongyu · 2025-01-20T12:24:45Z

+// database and adds the new edge to guarantee atomicity.
+// This is important for option-scid-alias channels which may change its SCID
+// over the course of its lifetime (e.g., public zero-conf channels).
+func (c *ChannelGraph) ReAddChannelEdge(


The point is to make sure there are no side effects - the full chain of processing ChannelUpdate consists of fundingManager -> gossiper -> router, each processing the msg and updating their states. One example is the validation barrier, where the router and gossiper each have one to ensure the ordering of msgs. Basically I need more time to access the flow changes here, so far it looks good as indicated by the itests, although the behavior does change as I'm seeing this log which is weird,

2025-01-20 20:09:57.115 [DBG] DISC gossiper.go:3546: Unable to fetch node announcement for [Dave]: node does not have node announcement

yyforyongyu · 2025-01-20T12:26:49Z

 				"chan_id=%v", msg.ChannelID)
 		}

+		// Look up the funding pk script so that we can register the


don't we already UpdateFilter below after validating the funding outpoint?

ziggie1984 · 2025-01-20T15:31:56Z

+		nodes := tx.ReadWriteBucket(nodeBucket)
+		if nodes == nil {
+			return ErrGraphNodeNotFound
+		}


do you think we need this consistency check ? I mean the nodebucket is a toplevel bucket so there shouldn't be ever the case that we do not have it ? I cpy pasted this part from the DeleteChannelEdges code (and I don't think it is needed there as well) @yyforyongyu

lightninglabs-deploy · 2025-05-05T23:55:02Z

@Crypt-iQ: review reminder
@ziggie1984, remember to re-request review from reviewers when ready

yyforyongyu · 2025-05-12T08:35:32Z

!lightninglabs-deploy mute

ziggie1984 added this to the v0.18.1 milestone May 24, 2024

ziggie1984 self-assigned this May 24, 2024

ziggie1984 added the gossip label May 24, 2024

Crypt-iQ self-requested a review May 29, 2024 18:50

saubyk modified the milestones: v0.18.1, 0.19.0 Jun 20, 2024

ziggie1984 force-pushed the removal-of-edge-atomic branch 2 times, most recently from 655f5b0 to ef2a738 Compare December 16, 2024 14:13

ziggie1984 marked this pull request as ready for review December 16, 2024 14:14

ziggie1984 requested a review from starius December 16, 2024 14:14

ziggie1984 added the size/kilo medium, proper context needed, less than 1000 lines label Dec 16, 2024

starius approved these changes Dec 16, 2024

View reviewed changes

ziggie1984 force-pushed the removal-of-edge-atomic branch 2 times, most recently from 0784e6b to 9ddb66e Compare December 17, 2024 14:30

yyforyongyu self-requested a review December 17, 2024 19:41

ziggie1984 changed the title ~~multi: make deletion of edge atomic.~~ multi: make reassignment of channel edge atomic Dec 17, 2024

ziggie1984 force-pushed the removal-of-edge-atomic branch 4 times, most recently from 71f2dc6 to fd595ba Compare December 17, 2024 20:34

ziggie1984 changed the title ~~multi: make reassignment of channel edge atomic~~ multi: make reassignment of alias channel edge atomic Dec 17, 2024

starius reviewed Dec 17, 2024

View reviewed changes

starius approved these changes Dec 17, 2024

View reviewed changes

ziggie1984 force-pushed the removal-of-edge-atomic branch from fd595ba to ac5ede1 Compare December 18, 2024 09:21

ziggie1984 mentioned this pull request Dec 18, 2024

set dont forward bit in chanupdate msg for private channels #8582

Closed

starius reviewed Dec 18, 2024

View reviewed changes

yyforyongyu reviewed Dec 19, 2024

View reviewed changes

ziggie1984 force-pushed the removal-of-edge-atomic branch from 76fb9af to 7877c7d Compare December 19, 2024 17:21

ziggie1984 requested a review from yyforyongyu December 19, 2024 18:07

yyforyongyu force-pushed the yy-waiting-on-merge branch 2 times, most recently from 9d62968 to d259917 Compare December 20, 2024 05:06

yyforyongyu reviewed Dec 20, 2024

View reviewed changes

yyforyongyu force-pushed the yy-waiting-on-merge branch from d259917 to 4430487 Compare December 20, 2024 06:43

yyforyongyu reviewed Dec 20, 2024

View reviewed changes

yyforyongyu force-pushed the yy-waiting-on-merge branch from 4430487 to e885bf5 Compare December 20, 2024 09:54

ziggie1984 mentioned this pull request Dec 20, 2024

bugfix createmissingedge #9383

Merged

ziggie1984 changed the base branch from yy-waiting-on-merge to master December 20, 2024 11:27

funding: refactor testcase

01c09ec

This will make it simple in the next commits to test soley single messages rather than always the combination of announcement and channel update msg.

ziggie1984 force-pushed the removal-of-edge-atomic branch 2 times, most recently from 7b4e121 to e8e8b94 Compare December 20, 2024 11:33

ziggie1984 added 6 commits December 20, 2024 12:35

lnwire: add string method to channel_ready type

ce27b34

graph: add channel point to utxo filter

48f7842

In case we assume valid channels or have alias channels we make sure we also add the channel point to the block filter so that we are notified if the channel point is spent in a block.

graph: fix log output

55a4ab6

docs: add release-notes

fc0a477

channeldb: fix typo.

115451c

ziggie1984 force-pushed the removal-of-edge-atomic branch from e8e8b94 to 115451c Compare December 20, 2024 11:36

ziggie1984 mentioned this pull request Dec 20, 2024

Refactor gossip msg code #9384

Closed

ziggie1984 requested a review from yyforyongyu December 20, 2024 11:41

yyforyongyu reviewed Jan 20, 2025

View reviewed changes

ziggie1984 commented Jan 20, 2025

View reviewed changes

saubyk modified the milestones: v0.19.0, v0.20.0 Feb 20, 2025

ziggie1984 closed this Jun 12, 2025

saubyk removed this from the v0.20.0 milestone Jun 12, 2025


		t.Helper()

		// Validate custom parameter arrays have expected length

Conversation

ziggie1984 commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

ziggie1984 commented May 24, 2024

Uh oh!

Crypt-iQ commented Jun 18, 2024

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

ziggie1984 commented May 24, 2024 •

edited

Loading

coderabbitai Bot commented May 24, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

ziggie1984 Jan 20, 2025 •

edited

Loading