Fix min_final_expiry_delta failures for on-the-fly funding
#714
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We increase the default expiry of Bolt 11 and Bolt 12 invoices: they are now valid for 1 day. This ensures that if a payment takes a while to reach us (most likely because we've been offline for a few hours), the invoice is still valid when we come back online to receive the payment. We also increase our `min_final_expiry_delta` to 144 blocks and our `fulfill_safety_before_timeout` to 12 blocks. This gives us more time to publish our commitment transaction and HTLC-success transaction in case our peer doesn't acknowledge a preimage we reveal, so that we can claim the payment on-chain.
This commit is purely a refactoring (without any behavior change) of the minimum cltv expiry we require on incoming payments.
When using on-the-fly funding, we have to create and publish an on-chain transaction after accepting `will_add_htlc`, before we can relay the matching HTLCs. This can create issues when there isn't enough margin in the `min_final_expiry_delta` parameter where the HTLC is rejected. We add failing tests that demonstrate this behavior.
We fix the failing tests added in the previous commit, by allowing on-the-fly HTLCs that are below `min_final_expiry_delta` as long as their expiry is greater than twice our `fulfill_safety_before_timeout` parameter. This is what we want to guarantee: that we have enough time to potentially force-close and claim HTLC-success transactions.
pm47
reviewed
Oct 15, 2024
src/commonMain/kotlin/fr/acinq/lightning/payment/Bolt11Invoice.kt
Outdated
Show resolved
Hide resolved
This field was not at all doing what was documented: it doesn't make sense to reject a payment because its `min_final_expiry_delta` is too large, we only care about the total path's expiry delta which cannot exceed 2016 blocks. We were using this for the blinded path expiry, but the spec says we must rather use the invoice expiry, roughly converted to a block height. We also reorder `minFinalExpiryDelta` to put it closer to other related parameters.
Also, revert the change to `Bolt11Invoice.DEFAULT_EXPIRY_SECONDS`. This value is from the spec and should be kept.
pm47
approved these changes
Oct 15, 2024
pm47
added a commit
to ACINQ/phoenixd
that referenced
this pull request
Oct 16, 2024
It mostly includes cltv fixes for on-the-fly funding ACINQ/lightning-kmp#714. Also, the default invoice expiry for both bolt11/bolt12 invoices is now 24 hours.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using on-the-fly funding, we have to create and publish an on-chain transaction after accepting
will_add_htlc, before we can relay the matching HTLCs. It may take a few blocks before the transaction is published (or even confirmed), which means the HTLCs will be held and relayed after a delay.This can create an issue when there isn't enough margin in the HTLC's expiry: when we receive
will_add_htlc, its expiry is belowcurrentBlockHeight + minFinalExpiryDelta, so we accept the on-the-fly funding proposal, but when the HTLC is actually relayed a few blocks later, the previous condition isn't true anymore so we reject the HTLC and thus aren't paying the LSP iffrom_future_htlcwas used.The BOLTs recommend that payers add an additional
cltv_expiry_deltaof their own to the current block height when sending payments: Phoenix correctly does that and adds at least 72 blocks, but there are other implementations that only add a couple of blocks. We thus relax this condition when receiving on-the-fly HTLCs: as long as the remaining expiry delta is greater than twice ourfulfill_safety_before_timeoutparameter, we have enough time to publish a force-close and claim funds on-chain. We use this condition when evaluating whether we accept HTLCs to fix this issue.We also increase all of our default expiry parameters (which is a sane thing to do even outside of the context of this issue).
I recommend reviewing this PR commit-by-commit.