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

BOLT4: Specify max HTLC nLocktime for expiry_too_far #1086

Merged

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?

Copy link
Author

Choose a reason for hiding this comment

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

Yep if we’re all doing 2016 today let’s stick to it. Good starting point then for a base value for routing hop gossips explicitly diverging from it.

@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 ariard force-pushed the 2023-06-specify-max-cltv-expiry-delta branch 2 times, most recently from 9036d7c to 029028a Compare August 12, 2023 02:19
@ariard
Copy link
Author

ariard commented Aug 12, 2023

Updated at 9036d7c with the value of 2016 as it sounds common between all implementations. Rebased with onion changes.

I disagree that having a limit of 14 hops is a downside, I challenge you to find a route this long that makes sense.

If you have sophisticated deanonymization adversaries targeting your flow of payments, and you’re looking to increase the number of hops to break common heuristics and you’re ready to pay the routing fees premium, potentially combined with things like #1008.

Note the max-htlc-cltv value to abstract from 2016 block and allow references by:

  • channel_update extensions e.g long-term held (< 2016 blocks) packets for swaps
  • mempool policy rules
  • smart lightning signers policy control rules

@@ -1412,6 +1413,7 @@ The _origin node_:
- 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

Copy link
Author

Choose a reason for hiding this comment

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

corrected thanks

@ariard ariard force-pushed the 2023-06-specify-max-cltv-expiry-delta branch from 029028a to ecfcf07 Compare August 15, 2023 00:44
@ariard
Copy link
Author

ariard commented Aug 15, 2023

Updated at ecfcf07.

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
Copy link
Author

ariard commented Sep 25, 2023

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

I bind my reasoning exposed above.

Note the max-htlc-cltv value to abstract from 2016 block and allow references by:

channel_update extensions e.g long-term held (< 2016 blocks) packets for swaps
mempool policy rules
smart lightning signers policy control rules

Especially for the first one channel_update extension, you wish to have a clear value you can reference from which to say “above max-htlc-cltv” you’re free to have custom routing feerates applied. Even for experimentation for now.

Apart keeping the spec succinct, what is the downside of more than one-line spec update ? Note if you look on TCP standards over the last decades they have been abstracting and naming magic constant over time.

@@ -1330,7 +1331,7 @@ A _forwarding node_ MAY, but a _final node_ MUST NOT:
- report the `cltv_expiry` of the outgoing HTLC and the current channel setting for the outgoing
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?

Copy link
Author

Choose a reason for hiding this comment

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

Done at bccab9a, thanks for the catch.

@ariard ariard force-pushed the 2023-06-specify-max-cltv-expiry-delta branch from ecfcf07 to bccab9a Compare September 25, 2023 20:26
@ariard
Copy link
Author

ariard commented Sep 25, 2023

Updated at bccab9a with introducing max-htlc-cltv on the right error line (the one above far in the future). The constify of the value is kept.

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