-
Notifications
You must be signed in to change notification settings - Fork 2.2k
multi: add upfront-shutdown-address to lnd.conf. #9432
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
multi: add upfront-shutdown-address to lnd.conf. #9432
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 (
|
|
PTAL @yyforyongyu |
0411618 to
e0e5d26
Compare
|
Hey @yyforyongyu, could you take a look at this PR and share any comments if you have any? Thanks! |
|
@NishantBansal2003 do you mind rebasing and fixing the conflicts?. I want to take a look. |
sure, working on it now... |
e0e5d26 to
69f1f79
Compare
|
I rebased this. I want to add that this was a relatively old PR at the time, and I was a beginner with LND and the Lightning Network as a whole (I still am ;)). Please let me know if there are any loose ends that need to be addressed, and if additional unit tests or itests could be added. |
69f1f79 to
a4a00e7
Compare
a4a00e7 to
1d80f3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 🙏
My main comment is around the fact that setting this global option means that we will never be able to open a channel with a peer that does not support upfront shutdown (given the current state of the logic). So we need to decide if this is desired behaviour and if so, we should warn the user of this in the config description.
itest/list_exclude_test.go
Outdated
| "wipe forwarding packages", | ||
|
|
||
| "coop close with htlcs", | ||
| "open channel with shutdown address", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to skip this one for windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was getting the cannot coop close channel with active HTLCs error. It looked similar to the exclude above so I thought it was some flaky behavior on windows but let's add NoWait: true to fix this
itest/lnd_open_channel_test.go
Outdated
| // Create an invoice for Bob and send payment from Alice. | ||
| preimage := ht.Random32Bytes() | ||
| invoice := &lnrpc.Invoice{ | ||
| RPreimage: preimage, | ||
| Value: paymentAmount, | ||
| } | ||
| invoiceResp := bob.RPC.AddInvoice(invoice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you can just use the ht.CreatePayReqs helper
funding/manager.go
Outdated
| if len(msg.ShutdownScript) == 0 { | ||
| msg.ShutdownScript = f.cfg.ShutdownScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, the problem with doing this is: not all our peers necessarily have advertised support for upfront shutdown. So now with this global script, if our peer doesnt support it, getUpfrontShutdownScript will always fail here:
// Check whether the remote peer supports upfront shutdown scripts.
remoteUpfrontShutdown := peer.RemoteFeatures().HasFeature(
lnwire.UpfrontShutdownScriptOptional,
)
// If the peer does not support upfront shutdown scripts, and one has been
// provided, return an error because the feature is not supported.
if !remoteUpfrontShutdown && len(script) != 0 {
return nil, errUpfrontShutdownScriptNotSupported
}
since the script will now never be empty.
So I think we need to decide if this is desired behaviour & if so, the config option should warn the user that setting the option will result in channel-open failures for any peer that doesnt support upfront shutdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering the cooperative nature of the close I think it is better to warn that users would not be able to open a channel with peers who does not support upfront shutdown. It is up to the user to decide whether they want this partial protection or prefer to disable this feature. wdyt? we can discuss any better ideas you may have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also similar behaviour is followed in the case of a channel acceptor -- if a peer has set up the channel acceptor with upfront_shutdown, the channel will not be opened by a peer that does not support this feature bit.
This can also be backed by https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-open_channel-message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going with warning the user about this behavior, then you should update the description of the field in config.go and sample-lnd.conf to explicitly state what this means for them.
Abdulkbk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, took a look and left some comments.
sample-lnd.conf
Outdated
| ; pong failure. | ||
| ; no-disconnect-on-pong-failure=false | ||
|
|
||
| ; An address to enforce payout of our funds to on cooperative close. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the commit msg you mentioned "...but
can be overridden by the value specified during...", if that is the case, I think "enforce" doesn't fit this IMO. Additionally, you could also mention here that this value can be overridden.
funding/manager.go
Outdated
| if len(msg.ShutdownScript) == 0 { | ||
| msg.ShutdownScript = f.cfg.ShutdownScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going with warning the user about this behavior, then you should update the description of the field in config.go and sample-lnd.conf to explicitly state what this means for them.
itest/lnd_open_channel_test.go
Outdated
| } | ||
|
|
||
| // testOpenChannelWithShutdownAddr verifies that if the funder or fundee | ||
| // specifies an upfront shutdown address in `lnd.conf`, the funds are correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: shutdown address in the config, the funds... since user can do lnd --upfront-shutdown-address=<addr>.
1d80f3d to
0daffc2
Compare
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, looks good!
Couple of comments but then i think we're g2g
config.go
Outdated
| // Note: If this field is set when opening a channel with a peer that | ||
| // does not advertise support for the upfront shutdown feature, the | ||
| // channel open will fail. | ||
| UpfrontShutdownAddr string `long:"upfront-shutdown-address" description:"Address that our funds will be paid out to on cooperative close applies to all channel opens unless overridden by openchannel options or a channel acceptor. Note: The channel open will fail if this value is set for a peer that does not support the upfront shutdown feature bit."` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The address that funds will be paid out to on a cooperative channel close. This applies to all channels that are opened from the time of this option being set unless overridden for a specific channel opening. NOTE: if this option is set, any channel open will fail if the peer does not explicitly advertise the upfront-shutdown feature bit.
funding/manager.go
Outdated
| return | ||
| } | ||
|
|
||
| shutdownScript := acceptorResp.UpfrontShutdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put this under the comment
funding/manager.go
Outdated
| if len(shutdownScript) == 0 && f.cfg.ShutdownScript.IsSome() { | ||
| // Extract shutdown script from fn.Option. It is safe to call | ||
| // UnsafeFromSome because we just checked that it is some. | ||
| shutdownScript = f.cfg.ShutdownScript.UnsafeFromSome() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just:
shutdownScript := acceptorResp.UpfrontShutdown
if len(shutdownScript) == 0 {
f.cfg.ShutdownScript.WhenSome(
func(script lnwire.DeliveryAddress) {
shutdownScript = script
},
)
}
funding/manager.go
Outdated
| // If the funder did not provide an upfront-shutdown address, fall back | ||
| // to the configured shutdown script (if any). | ||
| if len(shutdownScript) == 0 && f.cfg.ShutdownScript.IsSome() { | ||
| // Extract shutdown script from fn.Option. It is safe to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
sample-lnd.conf
Outdated
| ; pong failure. | ||
| ; no-disconnect-on-pong-failure=false | ||
|
|
||
| ; An address that our funds will be paid out to on cooperative channel close. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the suggestion i left in earlier comment
0daffc2 to
31722ee
Compare
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice!
31722ee to
cdbc0dc
Compare
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
Will merge once #10343 is merged.
|
@NishantBansal2003, remember to re-request review from reviewers when ready |
|
Missing a rebase, otherwise can be merged! |
Introduced a new config value `upfront-shutdown-address` in the `lnd.conf` file. This ensures that channel close funds are transferred to the specified shutdown address. The value applies to both the funder and the fundee but can be overridden by the value specified during `openchannel` or by the `channel acceptor`. NOTE: If this field is set when opening a channel with a peer that does not advertise support for upfront shutdown feature, the channel open will fail. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
cdbc0dc to
36fb79e
Compare
|
rebased! |
Change Description
This PR introduces a new configuration option,
upfront-shutdown-address, in thelnd.conffile.fixes: #7964
lnd.confto specify an upfront shutdown address.Steps to Test
Integration tests are provided to validate this new functionality.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.