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

zpay32: Change min_final_cltv_expiry_delta. #8308

Merged

Conversation

ziggie1984
Copy link
Collaborator

@ziggie1984 ziggie1984 commented Dec 22, 2023

We already do not allow a min_final_cltv_expiry_delta for invoices lower than 18 and default to 80 when not specified:

https://github.com/lightningnetwork/lnd/blob/master/lnrpc/invoicesrpc/addinvoice.go#L364-L380.

Lately during a chat with CDecker we found out that CLN will not always provide a min_cltv_expiry_delta in their invoice. In that case we would default to 9 when paying the invoice which hower CLN would reject because the delta is too low for them.

So this PR is mostly to smoothen out some interoperability issues between CLN<=>LND

For more infos see: https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#cltv_expiry_delta-selection

Hmm looking at https://github.com/lightningnetwork/lnd/blob/master/lncfg/config.go we might analyse all our constants because I think we are still at an old spec level.

For example:

// DefaultIncomingBroadcastDelta defines the number of blocks before the
// expiry of an incoming htlc at which we force close the channel. We
// only go to chain if we also have the preimage to actually pull in the
// htlc. BOLT #2 suggests 7 blocks.

But I think now the value is also at 18:

the deadline for received HTLCs this node has fulfilled: the deadline after which the channel has to be failed and the HTLC fulfilled on-chain before its cltv_expiry. See steps 4-7 above, which imply a deadline of 2R+G+S blocks before cltv_expiry: 18 blocks is reasonable.

what do you think ?

We adhere to BOLT 02 and use 18 instead of 9.
@ziggie1984 ziggie1984 marked this pull request as ready for review December 22, 2023 17:35
@Roasbeef
Copy link
Member

what do you think ?

Most implementations should accept a higher value, so I don't think the change should cause interop issues.

Looks like this change (from 9 to 18) was made 3 years ago: lightning/bolts#785

@Roasbeef
Copy link
Member

I think we should also make sure to note this in the release notes as well, just to potentially find some edge case during the rc1 process.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎒

Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending a release note, otherwise LGTM🙏

@ziggie1984
Copy link
Collaborator Author

ziggie1984 commented Dec 29, 2023

Connor kept the value at 9 because of backwards compatibility (lightning/bolts#785 (comment)). But since we have not allowed invoice creation with lower values than 18 blocks since 0.11 I think its a good time to change the values.

EDIT: Also changed the DefaultIncomingBroadcastDelta from 10 to 18 according to the spec. I don't think this causes compatibility issues but let's first see whether all the test pass.

@ziggie1984 ziggie1984 force-pushed the new_min_cltv_default branch 2 times, most recently from 7b2eadc to 0afc5a5 Compare December 29, 2023 19:49
Copy link
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM👍

@yyforyongyu yyforyongyu merged commit ab8fb2c into lightningnetwork:master Jan 2, 2024
22 of 25 checks passed
@saubyk saubyk added this to the v0.18.0 milestone Jan 2, 2024
@@ -170,6 +170,13 @@
be switched off using the new `protocol.no-timestamp-query-option` config
option.

* [Update min_final_cltv_expiry_delta](https://github.com/lightningnetwork/lnd/pull/8308).
This only effects external invoices which do not supply the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, just noticed a typo while writing the Optech mention for this PR:

Suggested change
This only effects external invoices which do not supply the
This only affects external invoices which do not supply the

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants