Fix min_final_expiry_delta failures for on-the-fly funding

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
 
@@ -165,6 +164,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 +176,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,7 +219,7 @@ data class NodeParams(
         maxHtlcValueInFlightMsat = 20_000_000_000L,
         maxAcceptedHtlcs = 6,
         expiryDeltaBlocks = CltvExpiryDelta(144),
-        fulfillSafetyBeforeTimeoutBlocks = CltvExpiryDelta(6),
+        fulfillSafetyBeforeTimeoutBlocks = CltvExpiryDelta(12),
         checkHtlcTimeoutAfterStartupDelay = 30.seconds,
         checkHtlcTimeoutInterval = 10.seconds,
         htlcMinimum = 1000.msat,
@@ -232,7 +234,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 +245,12 @@ data class NodeParams(
                 maxAllowedFeeCredit = 0.msat
             )
         ),
-        minFinalCltvExpiryDelta = Bolt11Invoice.DEFAULT_MIN_FINAL_EXPIRY_DELTA,
+        // 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),
         maxFinalCltvExpiryDelta = CltvExpiryDelta(360),
-        bolt12invoiceExpiry = 60.seconds,
+        bolt12invoiceExpiry = 24.hours,
     )
 
     /**

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 = 24 * 3600 // 1 day
+        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

@@ -159,7 +159,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 +176,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 +489,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,8 +518,8 @@ 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 -> {
@@ -612,9 +626,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())
+            }
         }
     }
 

src/commonMain/kotlin/fr/acinq/lightning/wire/LightningMessages.kt

@@ -1095,6 +1095,7 @@ data class UpdateAddHtlc(
 
     val blinding: PublicKey? = tlvStream.get<UpdateAddHtlcTlv.Blinding>()?.publicKey
     val fundingFee: LiquidityAds.FundingFee? = tlvStream.get<UpdateAddHtlcTlv.FundingFeeTlv>()?.fee
+    val usesOnTheFlyFunding: Boolean = fundingFee != null
 
     override fun write(out: Output) {
         LightningCodecs.writeBytes(channelId, out)

src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt

@@ -923,7 +923,10 @@ class IncomingPaymentHandlerTestsCommon : LightningTestSuite() {
             val fundingFee = purchase.fundingFee.copy(amount = 0.msat)
             val htlc = makeUpdateAddHtlc(7, channelId, paymentHandler, incomingPayment.paymentHash, makeMppPayload(amount, amount, paymentSecret), fundingFee = fundingFee)
             assertEquals(htlc.amountMsat, amount)
-            val result = paymentHandler.process(htlc, Features.empty, TestConstants.defaultBlockHeight, TestConstants.feeratePerKw, TestConstants.fundingRates)
+            // Before relaying the payment, we have created an on-chain transaction, which took some time.
+            // We are now closer to the min_final_expiry_delta, but we've committed to accepting this HTLC already.
+            val currentBlockHeight = TestConstants.defaultBlockHeight + 24
+            val result = paymentHandler.process(htlc, Features.empty, currentBlockHeight, TestConstants.feeratePerKw, TestConstants.fundingRates)
             assertIs<IncomingPaymentHandler.ProcessAddResult.Accepted>(result)
             val (expectedActions, expectedReceivedWith) = setOf(
                 // @formatter:off
@@ -1661,18 +1664,31 @@ class IncomingPaymentHandlerTestsCommon : LightningTestSuite() {
         val payment = InboundLiquidityOutgoingPayment(UUID.randomUUID(), channelId, TxId(randomBytes32()), 500.sat, purchase, 0, null, null)
         paymentHandler.db.addOutgoingPayment(payment)
 
-        val cltvExpiry = TestConstants.Bob.nodeParams.minFinalCltvExpiryDelta.toCltvExpiry(TestConstants.defaultBlockHeight.toLong())
-        val (finalPayload, route) = makeBlindedPayload(TestConstants.Bob.nodeParams.nodeId, defaultAmount, defaultAmount, cltvExpiry, preimage = preimage)
-        val add = makeUpdateAddHtlc(0, randomBytes32(), paymentHandler, paymentHash, finalPayload, route.blindingKey, payment.fundingFee)
-        val result = paymentHandler.process(add, Features.empty, TestConstants.defaultBlockHeight, TestConstants.feeratePerKw, TestConstants.fundingRates)
-        assertIs<IncomingPaymentHandler.ProcessAddResult.Accepted>(result)
-        val fulfill = ChannelCommand.Htlc.Settlement.Fulfill(add.id, preimage, commit = true)
-        assertEquals(setOf(WrappedChannelCommand(add.channelId, fulfill)), result.actions.toSet())
-        assertEquals(result.incomingPayment.received, result.received)
-        assertEquals(defaultAmount - payment.fundingFee.amount, result.received.amount)
-        val receivedWith = IncomingPayment.ReceivedWith.LightningPayment(defaultAmount - payment.fundingFee.amount, add.channelId, 0, payment.fundingFee)
-        assertEquals(listOf(receivedWith), result.received.receivedWith)
-        checkDbPayment(result.incomingPayment, paymentHandler.db)
+        // Before relaying the payment, we must create an on-chain transaction, which may take some time.
+        // We may thus be closer to the min_final_expiry_delta, but we've committed to accepting this HTLC already.
+        // As long as the expiry is greater than twice our fulfill_safety_before_timeout, we will accept it.
+        run {
+            val cltvExpiry = TestConstants.Bob.nodeParams.fulfillSafetyBeforeTimeoutBlocks.toCltvExpiry(TestConstants.defaultBlockHeight.toLong())
+            val (finalPayload, route) = makeBlindedPayload(TestConstants.Bob.nodeParams.nodeId, defaultAmount, defaultAmount, cltvExpiry, preimage = preimage)
+            val add = makeUpdateAddHtlc(0, randomBytes32(), paymentHandler, paymentHash, finalPayload, route.blindingKey, payment.fundingFee)
+            val result = paymentHandler.process(add, Features.empty, TestConstants.defaultBlockHeight, TestConstants.feeratePerKw, TestConstants.fundingRates)
+            assertIs<IncomingPaymentHandler.ProcessAddResult.Rejected>(result)
+        }
+        run {
+            assertTrue((TestConstants.Bob.nodeParams.fulfillSafetyBeforeTimeoutBlocks * 2) < TestConstants.Bob.nodeParams.minFinalCltvExpiryDelta)
+            val cltvExpiry = (TestConstants.Bob.nodeParams.fulfillSafetyBeforeTimeoutBlocks * 2).toCltvExpiry(TestConstants.defaultBlockHeight.toLong())
+            val (finalPayload, route) = makeBlindedPayload(TestConstants.Bob.nodeParams.nodeId, defaultAmount, defaultAmount, cltvExpiry, preimage = preimage)
+            val add = makeUpdateAddHtlc(0, randomBytes32(), paymentHandler, paymentHash, finalPayload, route.blindingKey, payment.fundingFee)
+            val result = paymentHandler.process(add, Features.empty, TestConstants.defaultBlockHeight, TestConstants.feeratePerKw, TestConstants.fundingRates)
+            assertIs<IncomingPaymentHandler.ProcessAddResult.Accepted>(result)
+            val fulfill = ChannelCommand.Htlc.Settlement.Fulfill(add.id, preimage, commit = true)
+            assertEquals(setOf(WrappedChannelCommand(add.channelId, fulfill)), result.actions.toSet())
+            assertEquals(result.incomingPayment.received, result.received)
+            assertEquals(defaultAmount - payment.fundingFee.amount, result.received.amount)
+            val receivedWith = IncomingPayment.ReceivedWith.LightningPayment(defaultAmount - payment.fundingFee.amount, add.channelId, 0, payment.fundingFee)
+            assertEquals(listOf(receivedWith), result.received.receivedWith)
+            checkDbPayment(result.incomingPayment, paymentHandler.db)
+        }
     }
 
     @Test

src/commonTest/kotlin/fr/acinq/lightning/payment/OutgoingPaymentHandlerTestsCommon.kt

@@ -470,7 +470,7 @@ class OutgoingPaymentHandlerTestsCommon : LightningTestSuite() {
             assertIs<PaymentOnion.FinalPayload.Standard>(payloadB)
             assertEquals(add.amount, payloadB.amount)
             assertEquals(300_000.msat, payloadB.totalAmount)
-            assertEquals(CltvExpiry(TestConstants.defaultBlockHeight.toLong()) + Bolt11Invoice.DEFAULT_MIN_FINAL_EXPIRY_DELTA, payloadB.expiry)
+            assertEquals(CltvExpiry(TestConstants.defaultBlockHeight.toLong()) + TestConstants.Alice.nodeParams.minFinalCltvExpiryDelta, payloadB.expiry)
             assertEquals(invoice.paymentSecret, payloadB.paymentSecret)
         }