Fix min_final_expiry_delta failures for on-the-fly funding (#714)

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 below `currentBlockHeight + 
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 if
`from_future_htlc` was used.

The BOLTs recommend that payers add an additional `cltv_expiry_delta` of
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 our
`fulfill_safety_before_timeout` parameter, 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).

Author: t-bast

src/commonMain/kotlin/fr/acinq/lightning/CltvExpiry.kt

@@ -40,7 +40,9 @@ data class CltvExpiryDelta(private val underlying: Int) : Comparable<CltvExpiryD
     operator fun plus(other: Int): CltvExpiryDelta = CltvExpiryDelta(underlying + other)
     operator fun plus(other: CltvExpiryDelta): CltvExpiryDelta = CltvExpiryDelta(underlying + other.underlying)
     operator fun minus(other: CltvExpiryDelta): CltvExpiryDelta = CltvExpiryDelta(underlying - other.underlying)
+    operator fun times(m: Int): CltvExpiryDelta = CltvExpiryDelta(underlying * m)
     override fun compareTo(other: CltvExpiryDelta): Int = underlying.compareTo(other.underlying)
+    fun min(other: CltvExpiryDelta): CltvExpiryDelta = if (this < other) this else other
     fun toInt(): Int = underlying
     fun toLong(): Long = underlying.toLong()
     override fun toString(): String = "CltvExpiryDelta($underlying)"

src/commonMain/kotlin/fr/acinq/lightning/NodeParams.kt

@@ -7,12 +7,10 @@ import fr.acinq.lightning.blockchain.fee.FeerateTolerance
 import fr.acinq.lightning.blockchain.fee.OnChainFeeConf
 import fr.acinq.lightning.crypto.KeyManager
 import fr.acinq.lightning.logging.LoggerFactory
-import fr.acinq.lightning.payment.Bolt11Invoice
 import fr.acinq.lightning.payment.LiquidityPolicy
 import fr.acinq.lightning.utils.msat
 import fr.acinq.lightning.utils.sat
 import fr.acinq.lightning.utils.toMilliSatoshi
-import fr.acinq.lightning.wire.LiquidityAds
 import fr.acinq.lightning.wire.OfferTypes
 import io.ktor.utils.io.charsets.*
 import io.ktor.utils.io.core.*
@@ -21,6 +19,7 @@ import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.SharedFlow
 import kotlinx.coroutines.flow.asSharedFlow
 import kotlin.time.Duration
+import kotlin.time.Duration.Companion.hours
 import kotlin.time.Duration.Companion.minutes
 import kotlin.time.Duration.Companion.seconds
 
@@ -81,8 +80,11 @@ data class WalletParams(
 /**
  * When sending a payment, if the expiry used for the last node is very close to the current block height,
  * it lets intermediate nodes figure out their position in the route. To protect against this, a random
- * delta between min and max will be added to the current block height, which makes it look like there
- * are more hops after the final node.
+ * number of blocks between min and max will be added to the current block height, before the `minFinalExpiryDelta`
+ * requested by the recipient, which makes it look like there are more hops after the final node.
+ *
+ * The overall cltv expiry delta for an outgoing trampoline payment will thus be:
+ * `cltvExpiryDelta` =  `random(min, max)` + `minFinalExpiryDelta` + `TrampolineFees.cltvExpiryDelta`
  */
 data class RecipientCltvExpiryParams(val min: CltvExpiryDelta, val max: CltvExpiryDelta) {
     fun computeFinalExpiry(currentBlockHeight: Int, minFinalExpiryDelta: CltvExpiryDelta): CltvExpiry {
@@ -105,9 +107,12 @@ data class RecipientCltvExpiryParams(val min: CltvExpiryDelta, val max: CltvExpi
  * @param maxHtlcValueInFlightMsat cap on the total value of pending HTLCs in a channel: this lets us limit our exposure to HTLCs risk.
  * @param maxAcceptedHtlcs cap on the number of pending HTLCs in a channel: this lets us limit our exposure to HTLCs risk.
  * @param expiryDeltaBlocks cltv-expiry-delta used in our channel_update: since our channels are private and we don't relay payments, this will be basically ignored.
+ * @param minFinalCltvExpiryDelta cltv-expiry-delta that we require when receiving a payment.
  * @param fulfillSafetyBeforeTimeoutBlocks number of blocks necessary to react to a malicious peer that doesn't acknowledge and sign our HTLC preimages.
  * @param checkHtlcTimeoutAfterStartupDelay delay before we check for timed out HTLCs in our channels after a wallet restart.
- * @param htlcMinimum minimum accepted htlc value.
+ * @param bolt11InvoiceExpiry duration for which bolt11 invoices that we create are valid.
+ * @param bolt12InvoiceExpiry duration for which bolt12 invoices that we create are valid.
+ * @param htlcMinimum minimum accepted HTLC value.
  * @param toRemoteDelayBlocks number of blocks our peer will have to wait before they get their main output back in case they force-close a channel.
  * @param maxToLocalDelayBlocks maximum number of blocks we will have to wait before we get our main output back in case we force-close a channel.
  * @param minDepthBlocks minimum depth of a transaction before we consider it safely confirmed.
@@ -116,14 +121,11 @@ data class RecipientCltvExpiryParams(val min: CltvExpiryDelta, val max: CltvExpi
  * @param pingInterval delay between ping messages.
  * @param initialRandomReconnectDelay delay before which we reconnect to our peers (will be randomized based on this value).
  * @param maxReconnectInterval maximum delay between reconnection attempts.
- * @param mppAggregationWindow amount of time we will wait to receive all parts of a multi-part payment.
+ * @param mppAggregationWindow amount of time we will wait to receive all parts of a multipart payment.
  * @param maxPaymentAttempts maximum number of retries when attempting an outgoing payment.
  * @param paymentRecipientExpiryParams configure the expiry delta used for the final node when sending payments.
  * @param zeroConfPeers list of peers with whom we use zero-conf (note that this is a strong trust assumption).
  * @param liquidityPolicy fee policy for liquidity events, can be modified at any time.
- * @param minFinalCltvExpiryDelta cltv-expiry-delta that we require when receiving a payment.
- * @param maxFinalCltvExpiryDelta maximum cltv-expiry-delta that we accept when receiving a payment.
- * @param bolt12invoiceExpiry duration for which bolt12 invoices that we create are valid.
  */
 data class NodeParams(
     val loggerFactory: LoggerFactory,
@@ -136,9 +138,12 @@ data class NodeParams(
     val maxHtlcValueInFlightMsat: Long,
     val maxAcceptedHtlcs: Int,
     val expiryDeltaBlocks: CltvExpiryDelta,
+    val minFinalCltvExpiryDelta: CltvExpiryDelta,
     val fulfillSafetyBeforeTimeoutBlocks: CltvExpiryDelta,
     val checkHtlcTimeoutAfterStartupDelay: Duration,
     val checkHtlcTimeoutInterval: Duration,
+    val bolt11InvoiceExpiry: Duration,
+    val bolt12InvoiceExpiry: Duration,
     val htlcMinimum: MilliSatoshi,
     val toRemoteDelayBlocks: CltvExpiryDelta,
     val maxToLocalDelayBlocks: CltvExpiryDelta,
@@ -153,9 +158,6 @@ data class NodeParams(
     val paymentRecipientExpiryParams: RecipientCltvExpiryParams,
     val zeroConfPeers: Set<PublicKey>,
     val liquidityPolicy: MutableStateFlow<LiquidityPolicy>,
-    val minFinalCltvExpiryDelta: CltvExpiryDelta,
-    val maxFinalCltvExpiryDelta: CltvExpiryDelta,
-    val bolt12invoiceExpiry: Duration,
 ) {
     val nodePrivateKey get() = keyManager.nodeKeys.nodeKey.privateKey
     val nodeId get() = keyManager.nodeKeys.nodeKey.publicKey
@@ -165,6 +167,7 @@ data class NodeParams(
     val nodeEvents: SharedFlow<NodeEvents> get() = _nodeEvents.asSharedFlow()
 
     init {
+        // Verify required features are set and obsolete features aren't set.
         require(features.hasFeature(Feature.VariableLengthOnion, FeatureSupport.Mandatory)) { "${Feature.VariableLengthOnion.rfcName} should be mandatory" }
         require(features.hasFeature(Feature.PaymentSecret, FeatureSupport.Mandatory)) { "${Feature.PaymentSecret.rfcName} should be mandatory" }
         require(features.hasFeature(Feature.ChannelType, FeatureSupport.Mandatory)) { "${Feature.ChannelType.rfcName} should be mandatory" }
@@ -176,6 +179,8 @@ data class NodeParams(
         require(!features.hasFeature(Feature.PayToOpenClient)) { "${Feature.PayToOpenClient.rfcName} has been deprecated" }
         require(!features.hasFeature(Feature.PayToOpenProvider)) { "${Feature.PayToOpenProvider.rfcName} has been deprecated" }
         Features.validateFeatureGraph(features)
+        // Verify expiry parameters are consistent with each other.
+        require((fulfillSafetyBeforeTimeoutBlocks * 2) < minFinalCltvExpiryDelta) { "min_final_expiry_delta must be at least twice as long as fulfill_safety_before_timeout_blocks" }
     }
 
     /**
@@ -217,9 +222,15 @@ data class NodeParams(
         maxHtlcValueInFlightMsat = 20_000_000_000L,
         maxAcceptedHtlcs = 6,
         expiryDeltaBlocks = CltvExpiryDelta(144),
-        fulfillSafetyBeforeTimeoutBlocks = CltvExpiryDelta(6),
+        // We use a long expiry delta here for a few reasons:
+        //  - we want to ensure we're able to get HTLC-success txs confirmed if our peer ignores our preimage
+        //  - we may be offline for a while, so we want our peer to be able to hold HTLCs and forward them when we come back online
+        minFinalCltvExpiryDelta = CltvExpiryDelta(144),
+        fulfillSafetyBeforeTimeoutBlocks = CltvExpiryDelta(12),
         checkHtlcTimeoutAfterStartupDelay = 30.seconds,
         checkHtlcTimeoutInterval = 10.seconds,
+        bolt11InvoiceExpiry = 24.hours,
+        bolt12InvoiceExpiry = 24.hours,
         htlcMinimum = 1000.msat,
         minDepthBlocks = 3,
         toRemoteDelayBlocks = CltvExpiryDelta(2016),
@@ -232,7 +243,7 @@ data class NodeParams(
         mppAggregationWindow = 60.seconds,
         maxPaymentAttempts = 5,
         zeroConfPeers = emptySet(),
-        paymentRecipientExpiryParams = RecipientCltvExpiryParams(CltvExpiryDelta(75), CltvExpiryDelta(200)),
+        paymentRecipientExpiryParams = RecipientCltvExpiryParams(CltvExpiryDelta(72), CltvExpiryDelta(144)),
         liquidityPolicy = MutableStateFlow<LiquidityPolicy>(
             LiquidityPolicy.Auto(
                 inboundLiquidityTarget = null,
@@ -243,9 +254,6 @@ data class NodeParams(
                 maxAllowedFeeCredit = 0.msat
             )
         ),
-        minFinalCltvExpiryDelta = Bolt11Invoice.DEFAULT_MIN_FINAL_EXPIRY_DELTA,
-        maxFinalCltvExpiryDelta = CltvExpiryDelta(360),
-        bolt12invoiceExpiry = 60.seconds,
     )
 
     /**

src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt

@@ -719,7 +719,7 @@ class Peer(
         return res.await()
     }
 
-    suspend fun createInvoice(paymentPreimage: ByteVector32, amount: MilliSatoshi?, description: Either<String, ByteVector32>, expirySeconds: Long? = null): Bolt11Invoice {
+    suspend fun createInvoice(paymentPreimage: ByteVector32, amount: MilliSatoshi?, description: Either<String, ByteVector32>, expiry: Duration? = null): Bolt11Invoice {
         // we add one extra hop which uses a virtual channel with a "peer id", using the highest remote fees and expiry across all
         // channels to maximize the likelihood of success on the first payment attempt
         val remoteChannelUpdates = _channels.values.mapNotNull { channelState ->
@@ -741,7 +741,7 @@ class Peer(
                 )
             )
         )
-        return incomingPaymentHandler.createInvoice(paymentPreimage, amount, description, extraHops, expirySeconds)
+        return incomingPaymentHandler.createInvoice(paymentPreimage, amount, description, extraHops, expiry ?: nodeParams.bolt11InvoiceExpiry)
     }
 
     // The (node_id, fcm_token) tuple only needs to be registered once.

src/commonMain/kotlin/fr/acinq/lightning/payment/Bolt11Invoice.kt

@@ -105,8 +105,8 @@ data class Bolt11Invoice(
     }
 
     companion object {
-        const val DEFAULT_EXPIRY_SECONDS = 3600
-        val DEFAULT_MIN_FINAL_EXPIRY_DELTA = CltvExpiryDelta(18)
+        const val DEFAULT_EXPIRY_SECONDS = 3600 // default value from Bolt 11
+        val DEFAULT_MIN_FINAL_EXPIRY_DELTA = CltvExpiryDelta(18) // default value from Bolt 11
 
         private val prefixes = mapOf(
             Chain.Regtest to "lnbcrt",

src/commonMain/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandler.kt

@@ -17,6 +17,7 @@ import fr.acinq.lightning.logging.MDCLogger
 import fr.acinq.lightning.logging.mdc
 import fr.acinq.lightning.utils.*
 import fr.acinq.lightning.wire.*
+import kotlin.time.Duration
 
 sealed class PaymentPart {
     abstract val amount: MilliSatoshi
@@ -79,7 +80,7 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
         amount: MilliSatoshi?,
         description: Either<String, ByteVector32>,
         extraHops: List<List<Bolt11Invoice.TaggedField.ExtraHop>>,
-        expirySeconds: Long? = null,
+        expiry: Duration? = null,
         timestampSeconds: Long = currentTimestampSeconds()
     ): Bolt11Invoice {
         val paymentHash = Crypto.sha256(paymentPreimage).toByteVector32()
@@ -95,7 +96,7 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
             randomBytes32(),
             // We always include a payment metadata in our invoices, which lets us test whether senders support it
             ByteVector("2a"),
-            expirySeconds,
+            expiry?.inWholeSeconds,
             extraHops,
             timestampSeconds
         )
@@ -159,7 +160,14 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
         return process(Either.Left(htlc), remoteFeatures, currentBlockHeight, currentFeerate, remoteFundingRates, currentFeeCredit)
     }
 
-    private suspend fun process(htlc: Either<WillAddHtlc, UpdateAddHtlc>, remoteFeatures: Features, currentBlockHeight: Int, currentFeerate: FeeratePerKw, remoteFundingRates: LiquidityAds.WillFundRates?, currentFeeCredit: MilliSatoshi): ProcessAddResult {
+    private suspend fun process(
+        htlc: Either<WillAddHtlc, UpdateAddHtlc>,
+        remoteFeatures: Features,
+        currentBlockHeight: Int,
+        currentFeerate: FeeratePerKw,
+        remoteFundingRates: LiquidityAds.WillFundRates?,
+        currentFeeCredit: MilliSatoshi
+    ): ProcessAddResult {
         // There are several checks we could perform *before* decrypting the onion.
         // But we need to carefully handle which error message is returned to prevent information leakage, so we always peel the onion first.
         return when (val res = toPaymentPart(privateKey, htlc)) {
@@ -169,7 +177,14 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
     }
 
     /** Main payment processing, that handles payment parts. */
-    private suspend fun processPaymentPart(paymentPart: PaymentPart, remoteFeatures: Features, currentBlockHeight: Int, currentFeerate: FeeratePerKw, remoteFundingRates: LiquidityAds.WillFundRates?, currentFeeCredit: MilliSatoshi): ProcessAddResult {
+    private suspend fun processPaymentPart(
+        paymentPart: PaymentPart,
+        remoteFeatures: Features,
+        currentBlockHeight: Int,
+        currentFeerate: FeeratePerKw,
+        remoteFundingRates: LiquidityAds.WillFundRates?,
+        currentFeeCredit: MilliSatoshi
+    ): ProcessAddResult {
         val logger = MDCLogger(logger.logger, staticMdc = paymentPart.mdc() + ("feeCredit" to currentFeeCredit))
         when (paymentPart) {
             is HtlcPart -> logger.info { "processing htlc part expiry=${paymentPart.htlc.cltvExpiry}" }
@@ -475,8 +490,8 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
                         logger.warning { "invalid amount (overpayment): ${paymentPart.totalAmount}, expected: ${incomingPayment.origin.paymentRequest.amount}" }
                         Either.Left(rejectPaymentPart(privateKey, paymentPart, incomingPayment, currentBlockHeight))
                     }
-                    paymentPart is HtlcPart && paymentPart.htlc.cltvExpiry < minFinalCltvExpiry(incomingPayment.origin.paymentRequest, currentBlockHeight) -> {
-                        logger.warning { "payment with expiry too small: ${paymentPart.htlc.cltvExpiry}, min is ${minFinalCltvExpiry(incomingPayment.origin.paymentRequest, currentBlockHeight)}" }
+                    paymentPart is HtlcPart && paymentPart.htlc.cltvExpiry < minFinalCltvExpiry(nodeParams, paymentPart, incomingPayment.origin, currentBlockHeight) -> {
+                        logger.warning { "payment with expiry too small: ${paymentPart.htlc.cltvExpiry}, min is ${minFinalCltvExpiry(nodeParams, paymentPart, incomingPayment.origin, currentBlockHeight)}" }
                         Either.Left(rejectPaymentPart(privateKey, paymentPart, incomingPayment, currentBlockHeight))
                     }
                     else -> Either.Right(incomingPayment)
@@ -504,11 +519,11 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
                                 logger.warning { "invalid amount (underpayment): ${paymentPart.totalAmount}, expected: ${metadata.amount}" }
                                 Either.Left(rejectPaymentPart(privateKey, paymentPart, incomingPayment, currentBlockHeight))
                             }
-                            paymentPart is HtlcPart && paymentPart.htlc.cltvExpiry < nodeParams.minFinalCltvExpiryDelta.toCltvExpiry(currentBlockHeight.toLong()) -> {
-                                logger.warning { "payment with expiry too small: ${paymentPart.htlc.cltvExpiry}, min is ${nodeParams.minFinalCltvExpiryDelta.toCltvExpiry(currentBlockHeight.toLong())}" }
+                            paymentPart is HtlcPart && paymentPart.htlc.cltvExpiry < minFinalCltvExpiry(nodeParams, paymentPart, incomingPayment.origin, currentBlockHeight) -> {
+                                logger.warning { "payment with expiry too small: ${paymentPart.htlc.cltvExpiry}, min is ${minFinalCltvExpiry(nodeParams, paymentPart, incomingPayment.origin, currentBlockHeight)}" }
                                 Either.Left(rejectPaymentPart(privateKey, paymentPart, incomingPayment, currentBlockHeight))
                             }
-                            metadata.createdAtMillis + nodeParams.bolt12invoiceExpiry.inWholeMilliseconds < currentTimestampMillis() && incomingPayment.received == null -> {
+                            metadata.createdAtMillis + nodeParams.bolt12InvoiceExpiry.inWholeMilliseconds < currentTimestampMillis() && incomingPayment.received == null -> {
                                 logger.warning { "the invoice is expired" }
                                 Either.Left(rejectPaymentPart(privateKey, paymentPart, incomingPayment, currentBlockHeight))
                             }
@@ -612,9 +627,23 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: PaymentsDb) {
             return SendOnTheFlyFundingMessage(msg)
         }
 
-        private fun minFinalCltvExpiry(paymentRequest: Bolt11Invoice, currentBlockHeight: Int): CltvExpiry {
-            val minFinalCltvExpiryDelta = paymentRequest.minFinalExpiryDelta ?: Bolt11Invoice.DEFAULT_MIN_FINAL_EXPIRY_DELTA
-            return minFinalCltvExpiryDelta.toCltvExpiry(currentBlockHeight.toLong())
+        private fun minFinalCltvExpiry(nodeParams: NodeParams, paymentPart: PaymentPart, origin: IncomingPayment.Origin, currentBlockHeight: Int): CltvExpiry {
+            val minFinalExpiryDelta = when (origin) {
+                is IncomingPayment.Origin.Invoice -> origin.paymentRequest.minFinalExpiryDelta ?: Bolt11Invoice.DEFAULT_MIN_FINAL_EXPIRY_DELTA
+                else -> nodeParams.minFinalCltvExpiryDelta
+            }
+            return when {
+                paymentPart is HtlcPart && paymentPart.htlc.usesOnTheFlyFunding -> {
+                    // This HTLC is using on-the-fly funding, so it may have been forwarded with a delay
+                    // because an on-chain transaction was required.
+                    // If the expiry is now below our min_final_expiry_delta, we sill want to accept this
+                    // HTLC if we can do so safely, because we need to pay funding fees by fulfilling it.
+                    // As long as we have time to publish a force-close, it is safe to accept the HTLC.
+                    val overrideFinalExpiryDelta = minFinalExpiryDelta.min(nodeParams.fulfillSafetyBeforeTimeoutBlocks * 2)
+                    overrideFinalExpiryDelta.toCltvExpiry(currentBlockHeight.toLong())
+                }
+                else -> minFinalExpiryDelta.toCltvExpiry(currentBlockHeight.toLong())
+            }
         }
     }