From e1b162a5b06156182d18e218e96822a59709ac17 Mon Sep 17 00:00:00 2001 From: t-bast Date: Wed, 21 Aug 2024 17:35:07 +0200 Subject: [PATCH 1/3] Include `min_final_cltv_expiry_delta` in blinded paths If a payer uses the current block height as the expiry for the payments they send to us, we will reject it because we need to have a few blocks to potentially force-close and get our funds back safely. With blinded paths, we don't need to rely on payers to add a safety margin: we can directly encode it in the blinded path's total expiry delta. --- .../kotlin/fr/acinq/lightning/payment/OfferManager.kt | 9 +++++++-- .../acinq/lightning/payment/OfferManagerTestsCommon.kt | 8 ++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt b/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt index 1d0aa5b2d..030f113f3 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt @@ -159,10 +159,15 @@ class OfferManager(val nodeParams: NodeParams, val walletParams: WalletParams, v } val pathId = OfferPaymentMetadata.V1(ByteVector32(decrypted.pathId), amount, preimage, request.payerId, truncatedPayerNote, request.quantity, currentTimestampMillis()).toPathId(nodeParams.nodePrivateKey) val recipientPayload = RouteBlindingEncryptedData(TlvStream(RouteBlindingEncryptedDataTlv.PathId(pathId))).write().toByteVector() + val cltvExpiryDelta = remoteChannelUpdates.maxOfOrNull { it.cltvExpiryDelta } ?: walletParams.invoiceDefaultRoutingFees.cltvExpiryDelta val paymentInfo = OfferTypes.PaymentInfo( feeBase = remoteChannelUpdates.maxOfOrNull { it.feeBaseMsat } ?: walletParams.invoiceDefaultRoutingFees.feeBase, feeProportionalMillionths = remoteChannelUpdates.maxOfOrNull { it.feeProportionalMillionths } ?: walletParams.invoiceDefaultRoutingFees.feeProportional, - cltvExpiryDelta = remoteChannelUpdates.maxOfOrNull { it.cltvExpiryDelta } ?: walletParams.invoiceDefaultRoutingFees.cltvExpiryDelta, + // We include our min_final_cltv_expiry_delta in the path, but we *don't* include it in the payment_relay field + // for our trampoline node (below). This ensures that we will receive payments with at least this final expiry delta. + // This ensures that even when payers haven't received the latest block(s) or don't include a safety margin in the + // expiry they use, we can still safely receive their payment. + cltvExpiryDelta = cltvExpiryDelta + nodeParams.minFinalCltvExpiryDelta, minHtlc = remoteChannelUpdates.minOfOrNull { it.htlcMinimumMsat } ?: 1.msat, maxHtlc = amount, allowedFeatures = Features.empty @@ -170,7 +175,7 @@ class OfferManager(val nodeParams: NodeParams, val walletParams: WalletParams, v val remoteNodePayload = RouteBlindingEncryptedData( TlvStream( RouteBlindingEncryptedDataTlv.OutgoingNodeId(EncodedNodeId.WithPublicKey.Wallet(nodeParams.nodeId)), - RouteBlindingEncryptedDataTlv.PaymentRelay(paymentInfo.cltvExpiryDelta, paymentInfo.feeProportionalMillionths, paymentInfo.feeBase), + RouteBlindingEncryptedDataTlv.PaymentRelay(cltvExpiryDelta, paymentInfo.feeProportionalMillionths, paymentInfo.feeBase), RouteBlindingEncryptedDataTlv.PaymentConstraints((paymentInfo.cltvExpiryDelta + nodeParams.maxFinalCltvExpiryDelta).toCltvExpiry(currentBlockHeight.toLong()), paymentInfo.minHtlc) ) ).write().toByteVector() diff --git a/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt index ffdbeebd7..974664394 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt @@ -85,6 +85,10 @@ class OfferManagerTestsCommon : LightningTestSuite() { assertIs(payInvoice) assertEquals(OfferInvoiceReceived(payOffer, payInvoice.invoice), bobOfferManager.eventsFlow.first()) assertEquals(payOffer, payInvoice.payOffer) + assertEquals(1, payInvoice.invoice.blindedPaths.size) + val path = payInvoice.invoice.blindedPaths.first() + assertEquals(EncodedNodeId(aliceTrampolineKey.publicKey()), path.route.route.introductionNodeId) + assertEquals(aliceOfferManager.nodeParams.expiryDeltaBlocks + aliceOfferManager.nodeParams.minFinalCltvExpiryDelta, path.paymentInfo.cltvExpiryDelta) } @Test @@ -113,6 +117,10 @@ class OfferManagerTestsCommon : LightningTestSuite() { assertIs(payInvoice) assertEquals(OfferInvoiceReceived(payOffer, payInvoice.invoice), bobOfferManager.eventsFlow.first()) assertEquals(payOffer, payInvoice.payOffer) + assertEquals(1, payInvoice.invoice.blindedPaths.size) + val path = payInvoice.invoice.blindedPaths.first() + assertEquals(EncodedNodeId(aliceTrampolineKey.publicKey()), path.route.route.introductionNodeId) + assertEquals(aliceOfferManager.nodeParams.expiryDeltaBlocks + aliceOfferManager.nodeParams.minFinalCltvExpiryDelta, path.paymentInfo.cltvExpiryDelta) } @Test From 073efa0e531bcc7375dddd9132d20d83fe9e011c Mon Sep 17 00:00:00 2001 From: t-bast Date: Thu, 22 Aug 2024 14:01:56 +0200 Subject: [PATCH 2/3] Correctly set `htlc_minimum` in blinded paths We must use the most restrictive value for `htlc_minimum`, not the least restrictive one. When we don't have the remote channel update we should use the value from our node params. The `htlc_maximum` is allowed to be twice the payment amount according to the BOLTs: we reflect that in the blinded path data. --- .../fr/acinq/lightning/payment/OfferManager.kt | 12 ++++++++---- .../lightning/payment/OfferManagerTestsCommon.kt | 4 ++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt b/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt index 030f113f3..2b04369ff 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt @@ -4,9 +4,12 @@ import fr.acinq.bitcoin.ByteVector32 import fr.acinq.bitcoin.PublicKey import fr.acinq.bitcoin.utils.Either.Left import fr.acinq.bitcoin.utils.Either.Right -import fr.acinq.lightning.* +import fr.acinq.lightning.EncodedNodeId +import fr.acinq.lightning.Features import fr.acinq.lightning.Lightning.randomBytes32 import fr.acinq.lightning.Lightning.randomKey +import fr.acinq.lightning.NodeParams +import fr.acinq.lightning.WalletParams import fr.acinq.lightning.crypto.RouteBlinding import fr.acinq.lightning.io.OfferInvoiceReceived import fr.acinq.lightning.io.OfferNotPaid @@ -18,7 +21,6 @@ import fr.acinq.lightning.message.OnionMessages.Destination import fr.acinq.lightning.message.OnionMessages.IntermediateNode import fr.acinq.lightning.message.OnionMessages.buildMessage import fr.acinq.lightning.utils.currentTimestampMillis -import fr.acinq.lightning.utils.msat import fr.acinq.lightning.utils.toByteVector import fr.acinq.lightning.wire.* import kotlinx.coroutines.flow.MutableSharedFlow @@ -168,8 +170,10 @@ class OfferManager(val nodeParams: NodeParams, val walletParams: WalletParams, v // This ensures that even when payers haven't received the latest block(s) or don't include a safety margin in the // expiry they use, we can still safely receive their payment. cltvExpiryDelta = cltvExpiryDelta + nodeParams.minFinalCltvExpiryDelta, - minHtlc = remoteChannelUpdates.minOfOrNull { it.htlcMinimumMsat } ?: 1.msat, - maxHtlc = amount, + // We must use the most restrictive minimum HTLC value between local and remote. + minHtlc = (listOf(nodeParams.htlcMinimum) + remoteChannelUpdates.map { it.htlcMinimumMsat }).max(), + // Payments are allowed to overpay at most two times the invoice amount. + maxHtlc = amount * 2, allowedFeatures = Features.empty ) val remoteNodePayload = RouteBlindingEncryptedData( diff --git a/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt index 974664394..851eedddf 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt @@ -89,6 +89,8 @@ class OfferManagerTestsCommon : LightningTestSuite() { val path = payInvoice.invoice.blindedPaths.first() assertEquals(EncodedNodeId(aliceTrampolineKey.publicKey()), path.route.route.introductionNodeId) assertEquals(aliceOfferManager.nodeParams.expiryDeltaBlocks + aliceOfferManager.nodeParams.minFinalCltvExpiryDelta, path.paymentInfo.cltvExpiryDelta) + assertEquals(TestConstants.Alice.nodeParams.htlcMinimum, path.paymentInfo.minHtlc) + assertEquals(payOffer.amount * 2, path.paymentInfo.maxHtlc) } @Test @@ -121,6 +123,8 @@ class OfferManagerTestsCommon : LightningTestSuite() { val path = payInvoice.invoice.blindedPaths.first() assertEquals(EncodedNodeId(aliceTrampolineKey.publicKey()), path.route.route.introductionNodeId) assertEquals(aliceOfferManager.nodeParams.expiryDeltaBlocks + aliceOfferManager.nodeParams.minFinalCltvExpiryDelta, path.paymentInfo.cltvExpiryDelta) + assertEquals(TestConstants.Alice.nodeParams.htlcMinimum, path.paymentInfo.minHtlc) + assertEquals(payOffer.amount * 2, path.paymentInfo.maxHtlc) } @Test From 39f2413b700506f2bff2f5e62d62b20457739093 Mon Sep 17 00:00:00 2001 From: t-bast Date: Fri, 23 Aug 2024 11:43:43 +0200 Subject: [PATCH 3/3] Reject `invoice_request`s below `htlc_minimum` We reject invoice requests that specify an amount below our `htlc_minimum`. It doesn't make sense to attempt that payment, it won't be relayed by our peer. --- .../acinq/lightning/payment/OfferManager.kt | 11 ++++++--- .../fr/acinq/lightning/wire/OfferTypes.kt | 2 ++ .../channel/states/NormalTestsCommon.kt | 4 +--- .../payment/OfferManagerTestsCommon.kt | 23 ++++++++++++++++++- .../fr/acinq/lightning/tests/TestConstants.kt | 2 +- 5 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt b/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt index 2b04369ff..45af0851c 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/payment/OfferManager.kt @@ -132,6 +132,8 @@ class OfferManager(val nodeParams: NodeParams, val walletParams: WalletParams, v private fun receiveInvoiceRequest(decrypted: OnionMessages.DecryptedMessage, remoteChannelUpdates: List, currentBlockHeight: Int): OnionMessageAction.SendMessage? { val offer = localOffers[decrypted.pathId]!! val request = decrypted.content.records.get()?.let { OfferTypes.InvoiceRequest.validate(it.tlvs).right } + // We must use the most restrictive minimum HTLC value between local and remote. + val minHtlc = (listOf(nodeParams.htlcMinimum) + remoteChannelUpdates.map { it.htlcMinimumMsat }).max() return when { request == null -> { logger.warning { "offerId:${offer.offerId} pathId:${decrypted.pathId} ignoring onion message: missing or invalid invoice request" } @@ -149,8 +151,12 @@ class OfferManager(val nodeParams: NodeParams, val walletParams: WalletParams, v logger.warning { "offerId:${offer.offerId} pathId:${decrypted.pathId} ignoring invalid invoice request ($request)" } sendInvoiceError("ignoring invalid invoice request", decrypted.content.replyPath) } + request.requestedAmount()?.let { it < minHtlc } ?: false -> { + logger.warning { "offerId:${offer.offerId} pathId:${decrypted.pathId} amount too low (amount=${request.requestedAmount()} minHtlc=$minHtlc)" } + sendInvoiceError("amount too low, minimum amount = $minHtlc", decrypted.content.replyPath) + } else -> { - val amount = request.amount ?: (request.offer.amount!! * request.quantity) + val amount = request.requestedAmount()!! val preimage = randomBytes32() val truncatedPayerNote = request.payerNote?.let { if (it.length <= 64) { @@ -170,8 +176,7 @@ class OfferManager(val nodeParams: NodeParams, val walletParams: WalletParams, v // This ensures that even when payers haven't received the latest block(s) or don't include a safety margin in the // expiry they use, we can still safely receive their payment. cltvExpiryDelta = cltvExpiryDelta + nodeParams.minFinalCltvExpiryDelta, - // We must use the most restrictive minimum HTLC value between local and remote. - minHtlc = (listOf(nodeParams.htlcMinimum) + remoteChannelUpdates.map { it.htlcMinimumMsat }).max(), + minHtlc = minHtlc, // Payments are allowed to overpay at most two times the invoice amount. maxHtlc = amount * 2, allowedFeatures = Features.empty diff --git a/src/commonMain/kotlin/fr/acinq/lightning/wire/OfferTypes.kt b/src/commonMain/kotlin/fr/acinq/lightning/wire/OfferTypes.kt index ad5fdfdc1..e361b8e17 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/wire/OfferTypes.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/wire/OfferTypes.kt @@ -860,6 +860,8 @@ object OfferTypes { Features.areCompatible(offer.features, features) && checkSignature() + fun requestedAmount(): MilliSatoshi? = amount ?: offer.amount?.let { it * quantity } + fun checkSignature(): Boolean = verifySchnorr( signatureTag, diff --git a/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt index f80517101..4b5335731 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/channel/states/NormalTestsCommon.kt @@ -151,12 +151,10 @@ class NormalTestsCommon : LightningTestSuite() { @Test fun `recv ChannelCommand_Htlc_Add -- 0 msat`() { val (alice0, bob0) = reachNormal() - // Alice has a minimum set to 0 msat (which should be invalid, but may mislead Bob into relaying 0-value HTLCs which is forbidden by the spec). - assertEquals(0.msat, alice0.commitments.params.localParams.htlcMinimum) val add = defaultAdd.copy(amount = 0.msat) val (bob1, actions) = bob0.process(add) val actualError = actions.findCommandError() - val expectedError = HtlcValueTooSmall(bob0.channelId, 1.msat, 0.msat) + val expectedError = HtlcValueTooSmall(bob0.channelId, alice0.commitments.params.localParams.htlcMinimum, 0.msat) assertEquals(expectedError, actualError) assertEquals(bob0, bob1) } diff --git a/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt index 851eedddf..5e2db8d8b 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/payment/OfferManagerTestsCommon.kt @@ -206,7 +206,7 @@ class OfferManagerTestsCommon : LightningTestSuite() { } @Test - fun `receive invoice error`() = runSuspendTest { + fun `receive invoice error -- amount below offer amount`() = runSuspendTest { // Alice and Bob use the same trampoline node. val aliceOfferManager = OfferManager(TestConstants.Alice.nodeParams, aliceWalletParams, MutableSharedFlow(replay = 10), logger) val bobOfferManager = OfferManager(TestConstants.Bob.nodeParams, aliceWalletParams, MutableSharedFlow(replay = 10), logger) @@ -226,6 +226,27 @@ class OfferManagerTestsCommon : LightningTestSuite() { assertIs(event.reason) } + @Test + fun `receive invoice error -- amount too low`() = runSuspendTest { + // Alice and Bob use the same trampoline node. + val aliceOfferManager = OfferManager(TestConstants.Alice.nodeParams, aliceWalletParams, MutableSharedFlow(replay = 10), logger) + val bobOfferManager = OfferManager(TestConstants.Bob.nodeParams, aliceWalletParams, MutableSharedFlow(replay = 10), logger) + val offer = createOffer(aliceOfferManager, null) + + // Bob sends an invoice request to Alice that pays less than the minimum htlc amount. + val payOffer = PayOffer(UUID.randomUUID(), randomKey(), null, 10.msat, offer, 20.seconds) + val (_, invoiceRequests) = bobOfferManager.requestInvoice(payOffer) + val (messageForAlice, _) = trampolineRelay(invoiceRequests.first(), aliceTrampolineKey) + // Alice sends an invoice error back to Bob. + val invoiceResponse = aliceOfferManager.receiveMessage(messageForAlice, listOf(), 0) + assertIs(invoiceResponse) + val (messageForBob, _) = trampolineRelay(invoiceResponse.message, aliceTrampolineKey) + assertNull(bobOfferManager.receiveMessage(messageForBob, listOf(), 0)) + val event = bobOfferManager.eventsFlow.first() + assertIs(event) + assertIs(event.reason) + } + @Test fun `pay offer with payer note`() = runSuspendTest { // Alice and Bob use the same trampoline node. diff --git a/src/commonTest/kotlin/fr/acinq/lightning/tests/TestConstants.kt b/src/commonTest/kotlin/fr/acinq/lightning/tests/TestConstants.kt index 5263c8ba4..f15e9d22f 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/tests/TestConstants.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/tests/TestConstants.kt @@ -78,7 +78,7 @@ object TestConstants { ), maxHtlcValueInFlightMsat = 1_500_000_000L, maxAcceptedHtlcs = 100, - htlcMinimum = 0.msat, + htlcMinimum = 100.msat, toRemoteDelayBlocks = CltvExpiryDelta(144), maxToLocalDelayBlocks = CltvExpiryDelta(2048), feeBase = 100.msat,