Skip to content

BOLT4: Specify max HTLC nLocktime for expiry_too_far#1086

Merged
Roasbeef merged 1 commit intolightning:masterfrom
ariard:2023-06-specify-max-cltv-expiry-delta
Oct 23, 2023
Merged

BOLT4: Specify max HTLC nLocktime for expiry_too_far#1086
Roasbeef merged 1 commit intolightning:masterfrom
ariard:2023-06-specify-max-cltv-expiry-delta

Conversation

@ariard
Copy link

@ariard ariard commented Jun 5, 2023

Currently BOLT#4 recommends to forwarding node to reject HTLC with nLocktime field to far in the future from chain tip height (type 21: expiry_too_far). This recommendation is reasonable to avoid liquidity griefing where a HTLC would be send so far in the future (e.g 20 000 blocks from chain tip), than in case of on-chain force-closure of the link, the forwarding node cannot confirm the HTLC-timeout and therefore recover the satoshis locked.

I think this recommendation is implemented by all the main Lightning implementations. For e.g, for LDK will reject HTLC more than 2016 block in the future (CLTV_FAR_FAR_AWAY). This comes with few downsides, a) a payer cannot build payment path with more than 14 hops requesting at least a cltv_expiry_delta of 144 blocks, such path while onerous in term of off-chain fees could be done for privacy purpose and b) the forwarding nodes are limited in the selection of their own cltv_expiry_delta and as such the level of safety they wish for the HTLC forward (e.g in protection against time-dilation attacks).

This proposal suggests to adopt a common value between all implementations, dubbed max_htlc_cltv. A default value of 4032 blocks (4 weeks) is proposed (though I didn’t check what is selected by LND/CLN/Eclair currently and if it makes sense to adapt in consequence). If adopted, such max_htlc_cltv will improve the safety of routing nodes network-wide and allow more advanced rebalancing like #780.

Beyond, a new option could be introduced option_shrek and a corresponding channel_update message bit where routing hops are announcing they accept “unsafe” packets beyond 4032 blocks. This level of flexibility can be used by use-cases like multi-hop submarine swaps.

cc lightningdevkit/rust-lightning#2250

@ProofOfKeags
Copy link
Contributor

Implementations should specify what parameters they currently use for this: @rustyrussell @TheBlueMatt @t-bast @Roasbeef

@thomash-acinq
Copy link

Eclair has increased its default to 2016 recently (ACINQ/eclair#2677) to match LND and CLN. So now I believe all implementations use 2016 by default.
Given the current state of the network I think 2016 is enough and I only see downsides in doubling it to 4032. I disagree that having a limit of 14 hops is a downside, I challenge you to find a route this long that makes sense. Allowing longer routes makes jamming more potent and a higher max_htlc_cltv blocks liquidity longer in case of honest mistake (a node crashes while relaying a HTLC).

@ProofOfKeags
Copy link
Contributor

This appears to be 2016 in LND

@TheBlueMatt
Copy link
Collaborator

LDK is also 2016.

@niftynei
Copy link
Collaborator

niftynei commented Jul 31, 2023

Spec meeting notes (#1098): should update 4032 to 2016.

@rustyrussell
Copy link
Collaborator

OK, propose 2016 and everyone is happy.

A common value between implementations allow forwarding nodes to increase their `cltv_expiry_delta`
without reducing the payers ability to route along the network.

This new common value `max_htlc_cltv` is suggested to be 4032 blocks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we all do 2016 today, so let's stick with that as the recommendation?

This comment was marked as abuse.

@rustyrussell
Copy link
Collaborator

This should become a one liner.

Change:

  - if the `cltv_expiry` is unreasonably far in the future:
    - return an `expiry_too_far` error.

to:

  - if the `cltv_expiry` is more than 2016 blocks in the future:
    - return an `expiry_too_far` error.

@ariard

This comment was marked as abuse.

- MAY use the data specified in the various failure types for debugging
purposes.

<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove

This comment was marked as abuse.

@ariard

This comment was marked as abuse.

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK ecfcf07

@t-bast
Copy link
Collaborator

t-bast commented Aug 28, 2023

@ariard spec feedback is that we should make this change a one-liner instead of defining a new section.

@ariard

This comment was marked as abuse.

channel.
- return an `incorrect_cltv_expiry` error.
- if the `cltv_expiry` is unreasonably near the present:
- if the `cltv_expiry` is more than `max_htlc_cltv` near the present:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be modifying the line below?

This comment was marked as abuse.

@ariard

This comment was marked as abuse.

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK bccab9a

Copy link
Collaborator

@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 🦋

@Roasbeef Roasbeef merged commit 8a64c6a into lightning:master Oct 23, 2023
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.

8 participants