Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spec]: start to set the dont_forward bit in ChannelUpdate #7416

Open
Crypt-iQ opened this issue Feb 17, 2023 · 10 comments · May be fixed by #8582
Open

[spec]: start to set the dont_forward bit in ChannelUpdate #7416

Crypt-iQ opened this issue Feb 17, 2023 · 10 comments · May be fixed by #8582
Assignees
Labels
Milestone

Comments

@Crypt-iQ
Copy link
Collaborator

We need to start setting the bit at index 1 in the ChannelUpdate message flags per lightning/bolts@6fee63f. It is only set for updates that we don't want forwarded. See the spec change for more details.

@saubyk saubyk added this to the v0.16.1 milestone Feb 17, 2023
@saubyk
Copy link
Collaborator

saubyk commented Mar 13, 2023

prioed for 17

@saubyk saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023
@oldmonad
Copy link

oldmonad commented Mar 14, 2023

Hi, a new contributor here. Can I take a stab at this?

@saubyk
Copy link
Collaborator

saubyk commented Mar 15, 2023

Hi @oldmonad if you're interested, please go through the problem statement and the linked spec.
Once your analysis is complete, please provide a concise plan of action on this issue, before attempting a pr. Thanks.

@oldmonad
Copy link

Got it

@oldmonad
Copy link

Hi @saubyk, I provided a brief overview of the issue and action plan in this document. I would greatly appreciate any feedback.

@ellemouton
Copy link
Collaborator

thanks for the outline @oldmonad!

I think we just need to start setting the bit so that other impls dont start rejecting our channel_updates but I dont think we necessarily need to make this one required on our side. Keep to hear what @Crypt-iQ thinks though.

@Crypt-iQ
Copy link
Collaborator Author

We don't need to make the htlc_maximum_msat bit required because that may cause newer LND nodes to disconnect from older nodes. We'll need to set the dont_forward bit in funding/manager.go when sending out a channel update:

func (f *Manager) addToRouterGraph(completeChan *channeldb.OpenChannel,

and also in discovery/gossiper.go when sending the update to the remote peer:

if edgeInfo.Info.AuthProof == nil {

Also, this bit should be set even if aliases aren't used. It would be nice if all newer channels persisted the bit from the start and older channels performed an on-the-fly migration similar to what was done for the htlc-maximum bit here:

// This is an on-the-fly migration.
case !edge.MessageFlags.HasMaxHtlc():
edge.MaxHTLC = amtMax

I'll have to check if there are any other callsites that I'm forgetting

@Crypt-iQ Crypt-iQ assigned oldmonad and unassigned Crypt-iQ Mar 23, 2023
@saubyk saubyk added this to lnd v0.17 Mar 26, 2023
@saubyk saubyk moved this to 🏗 In progress in lnd v0.17 Mar 26, 2023
@ziggie1984
Copy link
Collaborator

@oldmonad are you still working on this ?

@ziggie1984
Copy link
Collaborator

Also, this bit should be set even if aliases aren't used. It would be nice if all newer channels persisted the bit from the start and older channels performed an on-the-fly migration similar to what was done for the htlc-maximum bit here:

Did I understand it correctly by "even channel which are not using aliases" but they have to be private tho, that's at least what I implemented. Basically you did all the work have not fund another place where we should set this bit ;)

@Crypt-iQ
Copy link
Collaborator Author

Yup they need to be private for the flag to be set

@saubyk saubyk moved this from 🏗 In progress to 👀 In review in lnd v0.17 Jun 15, 2023
@saubyk saubyk removed this from lnd v0.17 Jul 13, 2023
@saubyk saubyk modified the milestones: v0.17.0, v0.17.1 Jul 13, 2023
@saubyk saubyk modified the milestones: High Priority, v0.18.0 Aug 8, 2023
@saubyk saubyk added this to lnd v0.18 Aug 8, 2023
@saubyk saubyk moved this to 👀 In review in lnd v0.18 Aug 8, 2023
@saubyk saubyk moved this from 👀 In review to 📋 Backlog in lnd v0.18 Nov 1, 2023
@saubyk saubyk removed this from the v0.18.0 milestone Nov 5, 2023
@saubyk saubyk removed this from lnd v0.18 Nov 5, 2023
@saubyk saubyk added the P4 low prio label Nov 5, 2023
@saubyk saubyk added this to the v0.18.1 milestone May 9, 2024
@saubyk saubyk linked a pull request May 9, 2024 that will close this issue
@saubyk saubyk modified the milestones: v0.18.3, v0.19.0 Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants