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

Bump default CLTV delta on in-flight HTLCs to 4 days of blocks #2250

Conversation

ariard
Copy link

@ariard ariard commented Apr 30, 2023

This is the other-side of #1884 bumping there the absolute timelocks on the in-flight HLTCs for the same reasons.

Those values are present in the received HTLC outputs witnessScript and in the nLocktime of the counter-signed {holder, counterparty} timeout transactions. LDK users stay free to downgrade those values if they wish faster on-chain recovery of their funds in case of lack of counterparty liveliness or cooperation.

@TheBlueMatt
Copy link
Collaborator

Generally seems reasonable to me, but what do most nodes enforce as the maximum when routing? I'd be afraid we'd just end up by-default never getting routed through with this.

@ariard
Copy link
Author

ariard commented May 2, 2023

Generally seems reasonable to me, but what do most nodes enforce as the maximum when routing? I'd be afraid we'd just end up by-default never getting routed through with this.

That’s correct with this pick of 4 blocks and assuming we would route a HTLC to a payment path of 0.0.115 LDK nodes we won’t be able to forward through more than 3 hops (our CLTV_FAR_FAR_AWAY=2016). Which raises a first question what the average payment path, if we have data on this ? IIRC the legacy onion format was designed to allow at least path of 20 hops ?

On the other hand, we have a default holder_max_htlc_value_in_flight_msat of 10% of the channel_value_satoshis this is only a limit on the inbound HTLC and assuming channels are configured symmetrically for a routing hop this put an implicit cap on the outbound HTLC value in-flight. So in case of security/safety wreckage or network mempools congestion, only 10% of the channel value are at risk ? So we can have shorter CLTV delta than the proposed 4 blocks, like 2 days ? 2 days give us 7-hop payment path length and there we might bump in coordination our own CLTV_FAR_FAR_AWAY ?

If we modify our CLTV_FAR_FAR_AWAY, we should check other implementation upper bound on nLocktime field value from chain tip check at HTLC forwarding and then maybe raise the issue at the spec-level to harmonize to a value both bumping routing hops security and increasing the payment path length (at the downside of timevalue cost in case of non-cooperative channel closue with in-flight HTLCs) ?

@TheBlueMatt
Copy link
Collaborator

I'm less worried about LDK's limit here, which is actually kinda broken we should limit per hop not in total, but what lnd/CLN/eclair do.

@ariard
Copy link
Author

ariard commented May 3, 2023

which is actually kinda broken we should limit per hop not in total

What do you mean more precisely here as the number of hops in the payment path should not be observable by the routing node for privacy reasons, and that’s the encryption of the hop payload ensures iirc ?

lnd/CLN/eclair

Should we include VLS in the list of software we’re aiming timelocks compatibility ? There is a policy-htlc-cltv-range policy rules iirc https://gitlab.com/lightning-signer/validating-lightning-signer/-/blob/main/docs/policy-controls.md

Checked BOLT4 again, there is only a "if the cltv_expiry is unreasonably far in the future:” without quantitative indication, though yep to look on what lnd/CLN/eclair are doing.

@TheBlueMatt
Copy link
Collaborator

We shouldn't have a "total path limit" we should have a per-hop limit, because otherwise dijkstras gets stuck.

@dunxen
Copy link
Contributor

dunxen commented May 8, 2023

Checked BOLT4 again, there is only a "if the cltv_expiry is unreasonably far in the future:” without quantitative indication, though yep to look on what lnd/CLN/eclair are doing.

// The value 2016 corresponds to on average two weeks worth of blocks
// and is based on the maximum number of hops (20), the default CLTV
// delta (40), and some extra margin to account for the other lightning
// implementations and past lnd versions which used to have a default
// CLTV delta of 144.

  • CLN: 2016 blocks, although looks like they want to convince LND to change that (to a probably lower value).
  • eclair: 1008 blocks

@TheBlueMatt
Copy link
Collaborator

Thanks! Sadly I don't think those are the relevant limits for this change, but rather what they limit in their routing code - the CLTV delta here is mostly about the difference between our inbound and outbound edges when routing, so we care about nodes refusing to route through us due to our CLTV delta.

@dunxen
Copy link
Contributor

dunxen commented May 9, 2023

but rather what they limit in their routing code

Ah, right. Might take a little while to look through routing lol

@dunxen dunxen self-requested a review May 23, 2023 09:57
@ariard
Copy link
Author

ariard commented May 25, 2023

Ah, right. Might take a little while to look through routing lol

Yes this value is not standardized in BOLT4, though I believe to avoid routing’s dijkstras do not solve.

Note we should consider this value in light of anchor output support as it’s a hedge against mempool spikes.

I still have to look on the interactions with VLS and embedded signers before to update here.

@ariard
Copy link
Author

ariard commented May 28, 2023

edited: Going to work on them, timelocks have interactions with a lot of things in the Lightning ecosystem.

@TheBlueMatt
Copy link
Collaborator

Labeling this blocked on spec - we either need a spec update or other implementations to unilaterally allow large CLTV deltas before we can do this.

@ariard
Copy link
Author

ariard commented Jun 5, 2023

Labeling this blocked on spec - we either need a spec update or other implementations to unilaterally allow large CLTV deltas before we can do this.

Opened a spec change see lightning/bolts#1086 - I think it makes more sense to have see this specific on the spec-level, then we can have routing policies announced to allow to route HTLC beyond CLTV_FAR_FAR_AWAY (e.g for submarine swaps).

@ariard
Copy link
Author

ariard commented Jul 20, 2023

I’ll pursue the work at the spec-level to unlock the state of this PR.

@TheBlueMatt
Copy link
Collaborator

You opened a spec issue only about the limit of maximum CLTV delta nodes will accept, but that's not the only issue here - the other issue is will we be selected for routing if we set a high CLTV delta. My understanding is generally no, so we need to clarify that with other implementations as well.

@ariard
Copy link
Author

ariard commented Aug 12, 2023

You opened a spec issue only about the limit of maximum CLTV delta nodes will accept, but that's not the only issue here - the other issue is will we be selected for routing if we set a high CLTV delta.

Yes see the old #1647, there is effectively a concern we could be downgraded by payment senders either because they cannot build a payment path fitting their average-length (under the assumed limit of 2016 blocks) or the timevalue locked up of 3 or 4 days of blocks by LDK-based routing hop would be come with a scoring penalty, I think.

Waiting on the spec-level PR to land first as I think the boundary of 2016 blocks / max_htlc_cltv needs to be define, to eventually propose routing scoring algorithms recommendations.

Effectively here you have a trade-off between channel funds safety in reason of "too short" timelocks and downgrade of the routing hops by payment senders seeking the cheapest path, would be good to have harmony at the network-level.

@ariard
Copy link
Author

ariard commented Oct 17, 2023

For now, no interest in the security maintenance of this codebase.

@ariard ariard closed this Oct 17, 2023
@ariard
Copy link
Author

ariard commented Oct 20, 2023

if @wpaulino or @tnull have time, this pr sounds good for 0.0.118 or latter release. iirc lnd 80 and eclair 144

@ariard
Copy link
Author

ariard commented Oct 20, 2023

can recommend 112 as a value half between lnd and eclair, unless yield issues with current test coverage.

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

Successfully merging this pull request may close these issues.

None yet

3 participants