multi: make reassignment of alias channel edge atomic #8777
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@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 |
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
2 times, most recently
from
655f5b0 to
ef2a738
Compare
ziggie1984
added
the
size/kilo
graph/db/graph_test.go
Outdated
|
|
||
| // Define the new channel id (under real conditions this is the new | ||
| // confirmed channel id). | ||
| newChanSCID := rand.Uint64() |
There was a problem hiding this comment.
Let's make it more deterministic and exclude (small but real) probability of newChanSCID == oldSCID:
newChanSCID := oldSCIDBTW 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?
There was a problem hiding this comment.
no it should succeed aslo with the same SCID, in case of an option-scid channel but not zeroconf.
There was a problem hiding this comment.
But made the newChanID deterministic:
newChanSCID := oldSCID + 1
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
2 times, most recently
from
0784e6b to
9ddb66e
Compare
ziggie1984
changed the title
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
4 times, most recently
from
71f2dc6 to
fd595ba
Compare
ziggie1984
changed the title
| err = f.addToGraph( | ||
| c, &confChan.shortChanID, nil, ourPolicy, | ||
| aliasScid := c.ShortChanID() | ||
| confirmedScid := confChan.shortChanID |
There was a problem hiding this comment.
The variables can be used above in the ReAssignSCID call and below in the sendChanUpdate call.
graph/db/graph_test.go
Outdated
|
|
||
| // Define the new channel id (under real conditions this is the new | ||
| // confirmed channel id). | ||
| newChanSCID := rand.Uint64() |
There was a problem hiding this comment.
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.
| err = f.sendChanUpdate( | ||
| completeChan, &baseScid, ourPolicy, |
There was a problem hiding this comment.
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).
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
from
fd595ba to
ac5ede1
Compare
There was a problem hiding this comment.
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.
funding/manager.go
Outdated
| // 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 |
There was a problem hiding this comment.
unresolving as it's not addressed
| "update: %v", err) | ||
| } | ||
| } | ||
| case <-f.quit: |
funding/manager.go
Outdated
| fwdMinHTLC, fwdMaxHTLC := f.extractAnnounceParams(completeChan) | ||
|
|
||
| ann, err := f.newChanAnnouncement( | ||
| chanAnn, err := f.newChanAnnouncement( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
2 times, most recently
from
3a37dc6 to
a13596d
Compare
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
3 times, most recently
from
76fb9af to
7877c7d
Compare
yyforyongyu
force-pushed
the
yy-waiting-on-merge
branch
2 times, most recently
from
9d62968 to
d259917
Compare
funding/manager.go
Outdated
| // 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 |
There was a problem hiding this comment.
ReassignSCID -> ReAssignSCID
| @@ -1206,6 +1206,16 @@ func (b *Builder) processUpdate(msg interface{}, | |||
| "chan_id=%v", msg.ChannelID) | |||
| } | |||
|
|
|||
| // Look up the funding pk script so that we can register the | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
funding/manager.go
Outdated
| fwdMinHTLC, fwdMaxHTLC := f.extractAnnounceParams(completeChan) | ||
|
|
||
| ann, err := f.newChanAnnouncement( | ||
| chanAnn, err := f.newChanAnnouncement( |
There was a problem hiding this comment.
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
force-pushed
the
yy-waiting-on-merge
branch
from
d259917 to
4430487
Compare
| "update: %v", err) | ||
| } | ||
| } | ||
| case <-f.quit: |
| // 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
| // 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
| // 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( |
There was a problem hiding this comment.
A typical flow for adding an edge,
ChannelAnnouncementmsg 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?
There was a problem hiding this comment.
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 ?
yyforyongyu
force-pushed
the
yy-waiting-on-merge
branch
from
4430487 to
e885bf5
Compare
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
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.
ziggie1984
force-pushed
the
removal-of-edge-atomic
branch
from
e8e8b94 to
115451c
Compare
|
@Crypt-iQ: review reminder |
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.