From ce84c3fcf09ba6c1601325a43db40a3a8cf52ee7 Mon Sep 17 00:00:00 2001 From: pm47 Date: Tue, 9 Apr 2024 15:37:56 +0200 Subject: [PATCH] replace maxAbsoluteFee by maxMiningFee It makes much more sense to consider only the mining fee for absolute fee check, as it is volatile and amount-independent. The service fee is in % and predictable. --- .../kotlin/fr/acinq/lightning/NodeEvents.kt | 6 ++++- .../kotlin/fr/acinq/lightning/NodeParams.kt | 4 +-- .../kotlin/fr/acinq/lightning/io/Peer.kt | 4 +-- .../payment/IncomingPaymentHandler.kt | 3 ++- .../lightning/payment/LiquidityPolicy.kt | 27 +++++++++++-------- .../acinq/lightning/wire/LightningMessages.kt | 2 +- .../IncomingPaymentHandlerTestsCommon.kt | 9 ++++--- .../payment/LiquidityPolicyTestsCommon.kt | 9 +++---- 8 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/NodeEvents.kt b/src/commonMain/kotlin/fr/acinq/lightning/NodeEvents.kt index 5187afaf0..1e41a50fc 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/NodeEvents.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/NodeEvents.kt @@ -41,14 +41,16 @@ sealed interface LiquidityEvents : NodeEvents { sealed class Reason { data object PolicySetToDisabled : Reason() sealed class TooExpensive : Reason() { - data class OverAbsoluteFee(val maxAbsoluteFee: Satoshi) : TooExpensive() + data class OverMaxMiningFee(val maxMiningFee: Satoshi) : TooExpensive() data class OverRelativeFee(val maxRelativeFeeBasisPoints: Int) : TooExpensive() } + data class OverMaxCredit(val maxAllowedCredit: Satoshi) : TooExpensive() data object ChannelInitializing : Reason() } } + data class AddedToFeeCredit(override val amount: MilliSatoshi, override val fee: MilliSatoshi, override val source: Source) : Decision data class Accepted(override val amount: MilliSatoshi, override val fee: MilliSatoshi, override val source: Source) : Decision } @@ -62,8 +64,10 @@ sealed interface SensitiveTaskEvents : NodeEvents { data class InteractiveTx(val channelId: ByteVector32, val fundingTxIndex: Long) : TaskIdentifier() { constructor(fundingParams: InteractiveTxParams) : this(fundingParams.channelId, (fundingParams.sharedInput as? SharedFundingInput.Multisig2of2)?.fundingTxIndex?.let { it + 1 } ?: 0) } + data class IncomingMultiPartPayment(val paymentHash: ByteVector32) : TaskIdentifier() } + data class TaskStarted(val id: TaskIdentifier) : SensitiveTaskEvents data class TaskEnded(val id: TaskIdentifier) : SensitiveTaskEvents diff --git a/src/commonMain/kotlin/fr/acinq/lightning/NodeParams.kt b/src/commonMain/kotlin/fr/acinq/lightning/NodeParams.kt index 6879d591f..23bacb466 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/NodeParams.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/NodeParams.kt @@ -231,9 +231,9 @@ data class NodeParams( paymentRecipientExpiryParams = RecipientCltvExpiryParams(CltvExpiryDelta(75), CltvExpiryDelta(200)), liquidityPolicy = MutableStateFlow( LiquidityPolicy.Auto( - maxAbsoluteFee = 2_000.sat, + maxMiningFee = 2_000.sat, maxRelativeFeeBasisPoints = 3_000 /* 3000 = 30 % */, - skipAbsoluteFeeCheck = false, + skipMiningFeeCheck = false, maxAllowedCredit = 0.sat ) ), diff --git a/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt b/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt index 76d2073c2..e924b56d5 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt @@ -1037,7 +1037,7 @@ class Peer( is Origin.PleaseOpenChannelOrigin -> when (val request = channelRequests[origin.requestId]) { is RequestChannelOpen -> { val totalFee = origin.serviceFee + origin.miningFee.toMilliSatoshi() - msg.pushAmount - val decision = nodeParams.liquidityPolicy.value.maybeReject(request.walletInputs.balance.toMilliSatoshi(), totalFee, LiquidityEvents.Source.OnChainWallet, logger, nodeParams.feeCredit.value) + val decision = nodeParams.liquidityPolicy.value.maybeReject(request.walletInputs.balance.toMilliSatoshi(), totalFee, LiquidityEvents.Source.OnChainWallet, logger, currentFeeCredit = nodeParams.feeCredit.value) when (decision) { is LiquidityEvents.Decision.Rejected -> { logger.info { "rejecting open_channel2: reason=${decision.reason}" } @@ -1249,7 +1249,7 @@ class Peer( val (feerate, fee) = client.computeSpliceCpfpFeerate(available.channel.commitments, targetFeerate, spliceWeight = weight, logger) logger.info { "requesting splice-in using balance=${cmd.walletInputs.balance} feerate=$feerate fee=$fee" } - val decision = nodeParams.liquidityPolicy.value.maybeReject(cmd.walletInputs.balance.toMilliSatoshi(), fee.toMilliSatoshi(), LiquidityEvents.Source.OnChainWallet, logger, nodeParams.feeCredit.value) + val decision = nodeParams.liquidityPolicy.value.maybeReject(cmd.walletInputs.balance.toMilliSatoshi(), fee.toMilliSatoshi(), LiquidityEvents.Source.OnChainWallet, logger, currentFeeCredit = nodeParams.feeCredit.value) nodeParams._nodeEvents.emit(decision) when (decision) { is LiquidityEvents.Decision.Rejected -> { diff --git a/src/commonMain/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandler.kt b/src/commonMain/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandler.kt index 22a31cf0e..5b628d18a 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandler.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandler.kt @@ -258,7 +258,8 @@ class IncomingPaymentHandler(val nodeParams: NodeParams, val db: IncomingPayment val liquidityDecision = if (payment.parts.filterIsInstance().isNotEmpty()) { // We consider the total amount received (not only the pay-to-open parts) to evaluate whether or not to accept the payment val payToOpenFee = payment.parts.filterIsInstance().map { it.payToOpenRequest.payToOpenFeeSatoshis }.sum() - val decision = nodeParams.liquidityPolicy.value.maybeReject(payment.amountReceived, payToOpenFee.toMilliSatoshi(), LiquidityEvents.Source.OffChainPayment, logger, nodeParams.feeCredit.value) + val liquidity = payment.parts.filterIsInstance().map { it.payToOpenRequest.liquidity }.sum() + val decision = nodeParams.liquidityPolicy.value.maybeReject(payment.amountReceived, payToOpenFee.toMilliSatoshi(), LiquidityEvents.Source.OffChainPayment, logger, currentFeeCredit = nodeParams.feeCredit.value, liquidity = liquidity) logger.info { "pay-to-open decision: $decision" } nodeParams._nodeEvents.emit(decision) when (decision) { diff --git a/src/commonMain/kotlin/fr/acinq/lightning/payment/LiquidityPolicy.kt b/src/commonMain/kotlin/fr/acinq/lightning/payment/LiquidityPolicy.kt index b044c6085..2d60946fa 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/payment/LiquidityPolicy.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/payment/LiquidityPolicy.kt @@ -15,32 +15,37 @@ sealed class LiquidityPolicy { /** * Allow automated liquidity managements, within relative and absolute fee limits. Both conditions must be met. - * @param maxAbsoluteFee max absolute fee + * @param maxMiningFee max mining fee * @param maxRelativeFeeBasisPoints max relative fee (all included: service fee and mining fee) (1_000 bips = 10 %) - * @param skipAbsoluteFeeCheck only applies for off-chain payments, being more lax may make sense when the sender doesn't retry payments + * @param skipMiningFeeCheck only applies for off-chain payments, being more lax may make sense when the sender doesn't retry payments * @param maxAllowedCredit if other checks fail, accept the payment and add the corresponding amount to fee credit up to this max value (only applies to offline payments, 0 sat to disable) */ - data class Auto(val maxAbsoluteFee: Satoshi, val maxRelativeFeeBasisPoints: Int, val skipAbsoluteFeeCheck: Boolean, val maxAllowedCredit: Satoshi) : LiquidityPolicy() + data class Auto(val maxMiningFee: Satoshi, val maxRelativeFeeBasisPoints: Int, val skipMiningFeeCheck: Boolean, val maxAllowedCredit: Satoshi) : LiquidityPolicy() /** Make decision for a particular liquidity event */ - fun maybeReject(amount: MilliSatoshi, fee: MilliSatoshi, source: LiquidityEvents.Source, logger: MDCLogger, currentFeeCredit: Satoshi): LiquidityEvents.Decision { + fun maybeReject(amount: MilliSatoshi, fee: MilliSatoshi, source: LiquidityEvents.Source, logger: MDCLogger, currentFeeCredit: Satoshi, liquidity: Satoshi = 0.sat): LiquidityEvents.Decision { + val serviceFee = when (source) { + LiquidityEvents.Source.OnChainWallet -> 0.msat // no service fee for swap-ins + LiquidityEvents.Source.OffChainPayment -> (amount + liquidity.toMilliSatoshi()) * 0.01 // 1% service fee + } + val miningFee = fee - serviceFee return when (this) { is Disable -> LiquidityEvents.Decision.Rejected(amount, fee, source, LiquidityEvents.Decision.Rejected.Reason.PolicySetToDisabled) is Auto -> { - val maxAbsoluteFee = if (skipAbsoluteFeeCheck && source == LiquidityEvents.Source.OffChainPayment) Long.MAX_VALUE.msat else this.maxAbsoluteFee.toMilliSatoshi() + val maxMiningFee = if (skipMiningFeeCheck && source == LiquidityEvents.Source.OffChainPayment) Long.MAX_VALUE.msat else this.maxMiningFee.toMilliSatoshi() if (maxAllowedCredit == 0.sat || source == LiquidityEvents.Source.OnChainWallet) { val maxRelativeFee = amount * maxRelativeFeeBasisPoints / 10_000 - logger.info { "auto liquidity policy check: amount=$amount fee=$fee maxAbsoluteFee=$maxAbsoluteFee maxRelativeFee=$maxRelativeFee policy=$this" } + logger.info { "auto liquidity policy check: amount=$amount fee=$fee (miningFee=$miningFee serviceFee=$serviceFee) maxMiningFee=$maxMiningFee maxRelativeFee=$maxRelativeFee policy=$this" } if (fee > maxRelativeFee) { LiquidityEvents.Decision.Rejected(amount, fee, source, LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverRelativeFee(maxRelativeFeeBasisPoints)) - } else if (fee > maxAbsoluteFee) { - LiquidityEvents.Decision.Rejected(amount, fee, source, LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverAbsoluteFee(this.maxAbsoluteFee)) + } else if (miningFee > maxMiningFee) { + LiquidityEvents.Decision.Rejected(amount, fee, source, LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverMaxMiningFee(this.maxMiningFee)) } else LiquidityEvents.Decision.Accepted(amount, fee, source) } else { - logger.info { "fee-credit liquidity policy check: amount=$amount fee=$fee maxAbsoluteFee=$maxAbsoluteFee currentFeeCredit=$currentFeeCredit maxAllowedCredit=$maxAllowedCredit policy=$this" } - // NB: we do check the max absolute fee, but will never raise an explicit error for it, because the payment will either be added to fee credit or rejected due to exceeding the + logger.info { "fee-credit liquidity policy check: amount=$amount fee=$fee (miningFee=$miningFee serviceFee=$serviceFee) maxMiningFee=$maxMiningFee currentFeeCredit=$currentFeeCredit maxAllowedCredit=$maxAllowedCredit policy=$this" } + // NB: we do check the max mining fee, but will never raise an explicit error for it, because the payment will either be added to fee credit or rejected due to exceeding the // max allowed credit - if (fee <= maxAbsoluteFee && fee < (amount + currentFeeCredit.toMilliSatoshi())) { + if (miningFee <= maxMiningFee && fee < (amount + currentFeeCredit.toMilliSatoshi())) { LiquidityEvents.Decision.Accepted(amount, fee, source) } else if ((amount + currentFeeCredit.toMilliSatoshi()) > maxAllowedCredit.toMilliSatoshi()) { LiquidityEvents.Decision.Rejected(amount, fee, source, LiquidityEvents.Decision.Rejected.Reason.OverMaxCredit(maxAllowedCredit)) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/wire/LightningMessages.kt b/src/commonMain/kotlin/fr/acinq/lightning/wire/LightningMessages.kt index dd3b98138..8a34cfac7 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/wire/LightningMessages.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/wire/LightningMessages.kt @@ -1622,7 +1622,7 @@ data class PayToOpenRequest( val paymentHash: ByteVector32, val expireAt: Long, val finalPacket: OnionRoutingPacket, - val liquidity: Satoshi = 0.sat, + val liquidity: Satoshi, val tlvStream: TlvStream = TlvStream.empty(), ) : LightningMessage, HasChainHash { override val type: Long get() = PayToOpenRequest.type diff --git a/src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt index e3d323917..428d59eec 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/payment/IncomingPaymentHandlerTestsCommon.kt @@ -248,7 +248,8 @@ class IncomingPaymentHandlerTestsCommon : LightningTestSuite() { hops = channelHops(paymentHandler.nodeParams.nodeId), finalPayload = makeMppPayload(defaultAmount, defaultAmount, randomBytes32()), payloadLength = OnionRoutingPacket.PaymentPacketLength - ).third.packet + ).third.packet, + liquidity = 0.sat ) val result = paymentHandler.process(payToOpenRequest, TestConstants.defaultBlockHeight) @@ -338,7 +339,8 @@ class IncomingPaymentHandlerTestsCommon : LightningTestSuite() { hops = trampolineHops, finalPayload = makeMppPayload(defaultAmount, defaultAmount, paymentSecret.reversed()), // <-- wrong secret payloadLength = 400 - ).third.packet + ).third.packet, + liquidity = 0.sat ) val result = paymentHandler.process(payToOpenRequest, TestConstants.defaultBlockHeight) @@ -1405,7 +1407,8 @@ class IncomingPaymentHandlerTestsCommon : LightningTestSuite() { hops = channelHops(TestConstants.Bob.nodeParams.nodeId), finalPayload = finalPayload, payloadLength = OnionRoutingPacket.PaymentPacketLength - ).third.packet + ).third.packet, + liquidity = 0.sat ) } diff --git a/src/commonTest/kotlin/fr/acinq/lightning/payment/LiquidityPolicyTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/payment/LiquidityPolicyTestsCommon.kt index c6361c3f3..16fb0f0eb 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/payment/LiquidityPolicyTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/payment/LiquidityPolicyTestsCommon.kt @@ -7,7 +7,6 @@ import fr.acinq.lightning.utils.msat import fr.acinq.lightning.utils.sat import kotlin.test.Test import kotlin.test.assertEquals -import kotlin.test.assertNull class LiquidityPolicyTestsCommon : LightningTestSuite() { @@ -16,7 +15,7 @@ class LiquidityPolicyTestsCommon : LightningTestSuite() { @Test fun `policy rejection`() { - val policy = LiquidityPolicy.Auto(maxAbsoluteFee = 2_000.sat, maxRelativeFeeBasisPoints = 3_000 /* 3000 = 30 % */, skipAbsoluteFeeCheck = false, maxAllowedCredit = 0.sat) + val policy = LiquidityPolicy.Auto(maxMiningFee = 2_000.sat, maxRelativeFeeBasisPoints = 3_000 /* 3000 = 30 % */, skipMiningFeeCheck = false, maxAllowedCredit = 0.sat) // fee over both absolute and relative assertEquals( @@ -26,7 +25,7 @@ class LiquidityPolicyTestsCommon : LightningTestSuite() { // fee over absolute assertEquals( - expected = LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverAbsoluteFee(policy.maxAbsoluteFee), + expected = LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverMaxMiningFee(policy.maxMiningFee), actual = (policy.maybeReject(amount = 15_000_000.msat, fee = 3_000_000.msat, source = LiquidityEvents.Source.OffChainPayment, logger, currentFeeCredit = 0.sat) as? LiquidityEvents.Decision.Rejected)?.reason ) @@ -45,7 +44,7 @@ class LiquidityPolicyTestsCommon : LightningTestSuite() { @Test fun `policy rejection skip absolute check`() { - val policy = LiquidityPolicy.Auto(maxAbsoluteFee = 1_000.sat, maxRelativeFeeBasisPoints = 5_000 /* 3000 = 30 % */, skipAbsoluteFeeCheck = true, maxAllowedCredit = 0.sat) + val policy = LiquidityPolicy.Auto(maxMiningFee = 1_000.sat, maxRelativeFeeBasisPoints = 5_000 /* 3000 = 30 % */, skipMiningFeeCheck = true, maxAllowedCredit = 0.sat) // fee is over absolute, and it's an offchain payment so the check passes assertEquals( @@ -54,7 +53,7 @@ class LiquidityPolicyTestsCommon : LightningTestSuite() { // fee is over absolute, but it's an on-chain payment so the check fails assertEquals( - expected = LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverAbsoluteFee(policy.maxAbsoluteFee), + expected = LiquidityEvents.Decision.Rejected.Reason.TooExpensive.OverMaxMiningFee(policy.maxMiningFee), actual = (policy.maybeReject(amount = 4_000_000.msat, fee = 2_000_000.msat, source = LiquidityEvents.Source.OnChainWallet, logger, currentFeeCredit = 0.sat) as? LiquidityEvents.Decision.Rejected)?.reason )