Add more force-close tests for HTLC settlement

This commit is importing test updates and refactoring to the closing
helpers from ACINQ/eclair#3040

We don't relay HTLCs, so we don't have an upstream channel to relay
preimages to, but it's important to relay preimages to the payment
handler to correctly mark payments as succeeded (or failed) and store
the proof of payment.

Author: t-bast

modules/core/src/commonMain/kotlin/fr/acinq/lightning/channel/ChannelData.kt

@@ -10,7 +10,6 @@ import fr.acinq.lightning.channel.Helpers.watchConfirmedIfNeeded
 import fr.acinq.lightning.channel.Helpers.watchSpentIfNeeded
 import fr.acinq.lightning.crypto.KeyManager
 import fr.acinq.lightning.logging.LoggingContext
-import fr.acinq.lightning.transactions.Scripts
 import fr.acinq.lightning.transactions.Transactions.TransactionWithInputInfo.*
 import fr.acinq.lightning.utils.toMilliSatoshi
 
@@ -70,7 +69,7 @@ data class LocalCommitPublished(
         // is the commitment tx buried? (we need to check this because we may not have any outputs)
         val isCommitTxConfirmed = confirmedTxs.contains(commitTx.txid)
         // is our main output confirmed (if we have one)?
-        val isMainOutputConfirmed = claimMainDelayedOutputTx?.let { irrevocablySpent.contains(it.input.outPoint) } ?: true
+        val isMainOutputConfirmed = claimMainDelayedOutputTx?.let { irrevocablySpent.contains(it.input.outPoint) } != false
         // are all htlc outputs from the commitment tx spent (we need to check them all because we may receive preimages later)?
         val allHtlcsSpent = (htlcTxs.keys - irrevocablySpent.keys).isEmpty()
         // are all outputs from htlc txs spent?
@@ -86,22 +85,6 @@ data class LocalCommitPublished(
         return irrevocablySpent.values.any { it.txid == commitTx.txid } || irrevocablySpent.keys.any { it.txid == commitTx.txid }
     }
 
-    fun isHtlcTimeout(tx: Transaction): Boolean {
-        return tx.txIn
-            .filter { htlcTxs[it.outPoint] is HtlcTx.HtlcTimeoutTx }
-            .map { it.witness }
-            .mapNotNull(Scripts.extractPaymentHashFromHtlcTimeout())
-            .isNotEmpty()
-    }
-
-    fun isHtlcSuccess(tx: Transaction): Boolean {
-        return tx.txIn
-            .filter { htlcTxs[it.outPoint] is HtlcTx.HtlcSuccessTx }
-            .map { it.witness }
-            .mapNotNull(Scripts.extractPreimageFromHtlcSuccess())
-            .isNotEmpty()
-    }
-
     internal fun LoggingContext.doPublish(nodeParams: NodeParams, channelId: ByteVector32): List<ChannelAction> {
         val publishQueue = buildList {
             add(ChannelAction.Blockchain.PublishTx(commitTx, ChannelAction.Blockchain.PublishTx.Type.CommitTx))
@@ -183,7 +166,7 @@ data class RemoteCommitPublished(
         // is the commitment tx buried? (we need to check this because we may not have any outputs)
         val isCommitTxConfirmed = confirmedTxs.contains(commitTx.txid)
         // is our main output confirmed (if we have one)?
-        val isMainOutputConfirmed = claimMainOutputTx?.let { irrevocablySpent.contains(it.input.outPoint) } ?: true
+        val isMainOutputConfirmed = claimMainOutputTx?.let { irrevocablySpent.contains(it.input.outPoint) } != false
         // are all htlc outputs from the commitment tx spent (we need to check them all because we may receive preimages later)?
         val allHtlcsSpent = (claimHtlcTxs.keys - irrevocablySpent.keys).isEmpty()
         return isCommitTxConfirmed && isMainOutputConfirmed && allHtlcsSpent
@@ -193,22 +176,6 @@ data class RemoteCommitPublished(
         return irrevocablySpent.values.any { it.txid == commitTx.txid } || irrevocablySpent.keys.any { it.txid == commitTx.txid }
     }
 
-    fun isClaimHtlcTimeout(tx: Transaction): Boolean {
-        return tx.txIn
-            .filter { claimHtlcTxs[it.outPoint] is ClaimHtlcTx.ClaimHtlcTimeoutTx }
-            .map { it.witness }
-            .mapNotNull(Scripts.extractPaymentHashFromClaimHtlcTimeout())
-            .isNotEmpty()
-    }
-
-    fun isClaimHtlcSuccess(tx: Transaction): Boolean {
-        return tx.txIn
-            .filter { claimHtlcTxs[it.outPoint] is ClaimHtlcTx.ClaimHtlcSuccessTx }
-            .map { it.witness }
-            .mapNotNull(Scripts.extractPreimageFromClaimHtlcSuccess())
-            .isNotEmpty()
-    }
-
     internal fun LoggingContext.doPublish(nodeParams: NodeParams, channelId: ByteVector32): List<ChannelAction> {
         val publishQueue = buildList {
             claimMainOutputTx?.let { add(ChannelAction.Blockchain.PublishTx(it)) }

modules/core/src/commonMain/kotlin/fr/acinq/lightning/channel/Helpers.kt

@@ -15,8 +15,10 @@ import fr.acinq.lightning.channel.Helpers.Closing.claimRevokedRemoteCommitTxHtlc
 import fr.acinq.lightning.channel.Helpers.Closing.extractPreimages
 import fr.acinq.lightning.channel.Helpers.Closing.onChainOutgoingHtlcs
 import fr.acinq.lightning.channel.Helpers.Closing.overriddenOutgoingHtlcs
-import fr.acinq.lightning.channel.Helpers.Closing.timedOutHtlcs
+import fr.acinq.lightning.channel.Helpers.Closing.trimmedOrTimedOutHtlcs
 import fr.acinq.lightning.transactions.Transactions.TransactionWithInputInfo.ClosingTx
+import fr.acinq.lightning.transactions.incomings
+import fr.acinq.lightning.transactions.outgoings
 import fr.acinq.lightning.utils.getValue
 import fr.acinq.lightning.wire.ChannelReestablish
 import fr.acinq.lightning.wire.Error
@@ -106,20 +108,38 @@ data class Closing(
                                 // This commitment may be revoked: we need to verify that its index matches our latest known index before overwriting our previous commitments.
                                 when {
                                     watch.tx.txid == commitments1.latest.localCommit.publishableTxs.commitTx.tx.txid -> {
-                                        // our local commit has been published from the outside, it's unexpected but let's deal with it anyway
+                                        // Our local commit has been published from the outside, it's unexpected but let's deal with it anyway.
                                         newState.run { spendLocalCurrent() }
                                     }
                                     watch.tx.txid == commitments1.latest.remoteCommit.txid && commitments1.remoteCommitIndex == commitments.remoteCommitIndex -> {
-                                        // counterparty may attempt to spend its last commit tx at any time
+                                        // Our counterparty may attempt to spend its last commit tx at any time.
                                         newState.run { handleRemoteSpentCurrent(watch.tx, commitments1.latest) }
                                     }
                                     watch.tx.txid == commitments1.latest.nextRemoteCommit?.commit?.txid && commitments1.remoteCommitIndex == commitments.remoteCommitIndex && commitments.remoteNextCommitInfo.isLeft -> {
-                                        // counterparty may attempt to spend its next commit tx at any time
+                                        // Our counterparty may attempt to spend its next commit tx at any time.
                                         newState.run { handleRemoteSpentNext(watch.tx, commitments1.latest) }
                                     }
                                     else -> {
-                                        // counterparty may attempt to spend a revoked commit tx at any time
-                                        newState.run { handleRemoteSpentOther(watch.tx) }
+                                        // Our counterparty is trying to broadcast a revoked commit tx (cheating attempt).
+                                        // We need to fail pending outgoing HTLCs, otherwise we will never properly settle them.
+                                        // We must do it here because since we're overwriting the commitments data, we will lose all information
+                                        // about HTLCs that are in the current commitments but were not in the revoked one.
+                                        // We fail *all* outgoing HTLCs:
+                                        //  - those that are not in the revoked commitment will never settle on-chain
+                                        //  - those that are in the revoked commitment will be claimed on-chain, so it's as if they were failed
+                                        // Note that if we already received the preimage for some of these HTLCs, we already relayed it to the
+                                        // outgoing payment handler so the fail command will be a no-op.
+                                        val outgoingHtlcs = commitments.latest.localCommit.spec.htlcs.outgoings().toSet() +
+                                                commitments.latest.remoteCommit.spec.htlcs.incomings().toSet() +
+                                                (commitments.latest.nextRemoteCommit?.commit?.spec?.htlcs ?: setOf()).incomings().toSet()
+                                        val htlcSettledActions = outgoingHtlcs.mapNotNull { add ->
+                                            commitments.payments[add.id]?.let { paymentId ->
+                                                logger.info { "failing htlc #${add.id} paymentHash=${add.paymentHash} paymentId=$paymentId: overridden by revoked remote commit" }
+                                                ChannelAction.ProcessCmdRes.AddSettledFail(paymentId, add, ChannelAction.HtlcResult.Fail.OnChainFail(HtlcOverriddenByLocalCommit(channelId, add)))
+                                            }
+                                        }
+                                        val (nextState, closingActions) = newState.run { handleRemoteSpentOther(watch.tx) }
+                                        Pair(nextState, closingActions + htlcSettledActions)
                                     }
                                 }
                             }
@@ -141,15 +161,18 @@ data class Closing(
                             // we may need to fail some htlcs in case a commitment tx was published and they have reached the timeout threshold
                             val htlcSettledActions = mutableListOf<ChannelAction>()
                             val timedOutHtlcs = when (val closingType = closing1.closingTypeAlreadyKnown()) {
-                                is LocalClose -> timedOutHtlcs(closingType.localCommit, closingType.localCommitPublished, commitments.params.localParams.dustLimit, watch.tx)
-                                is RemoteClose -> timedOutHtlcs(closingType.remoteCommit, closingType.remoteCommitPublished, commitments.params.remoteParams.dustLimit, watch.tx)
-                                else -> setOf() // we lose htlc outputs in option_data_loss_protect scenarios (future remote commit)
+                                is LocalClose -> trimmedOrTimedOutHtlcs(closingType.localCommit, closingType.localCommitPublished, commitments.params.localParams.dustLimit, watch.tx)
+                                is RemoteClose -> trimmedOrTimedOutHtlcs(closingType.remoteCommit, closingType.remoteCommitPublished, commitments.params.remoteParams.dustLimit, watch.tx)
+                                is RevokedClose -> setOf() // revoked commitments are handled using [overriddenOutgoingHtlcs] below
+                                is RecoveryClose -> setOf() // we lose htlc outputs in option_data_loss_protect scenarios (future remote commit)
+                                is MutualClose -> setOf()
+                                null -> setOf()
                             }
                             timedOutHtlcs.forEach { add ->
                                 when (val paymentId = commitments.payments[add.id]) {
                                     null -> {
                                         // same as for fulfilling the htlc (no big deal)
-                                        logger.info { "cannot fail timedout htlc #${add.id} paymentHash=${add.paymentHash} (payment not found)" }
+                                        logger.info { "cannot fail timed-out htlc #${add.id} paymentHash=${add.paymentHash} (payment not found)" }
                                     }
                                     else -> {
                                         logger.info { "failing htlc #${add.id} paymentHash=${add.paymentHash} paymentId=$paymentId: htlc timed out" }
@@ -243,7 +266,7 @@ data class Closing(
                             // we can then use these preimages to fulfill payments
                             logger.info { "processing spent closing output with txid=${watch.spendingTx.txid} tx=${watch.spendingTx}" }
                             val htlcSettledActions = mutableListOf<ChannelAction>()
-                            extractPreimages(commitments.latest.localCommit, watch.spendingTx).forEach { (htlc, preimage) ->
+                            extractPreimages(commitments.latest, watch.spendingTx).forEach { (htlc, preimage) ->
                                 when (val paymentId = commitments.payments[htlc.id]) {
                                     null -> {
                                         // if we don't have a reference to the payment, it means that we already have forwarded the fulfill so that's not a big deal.