Simplify how we store HTLC transactions when the local/remote commit tx is published

eclair-core/src/main/scala/fr/acinq/eclair/balance/CheckBalance.scala

@@ -118,12 +118,12 @@ object CheckBalance {
     import l._
     val toLocal = localCommitPublished.claimMainDelayedOutputTx.toSeq.map(c => OutPoint(c.tx.txid, 0) -> c.tx.txOut.head.amount.toBtc).toMap
     // incoming htlcs for which we have a preimage and the to-local delay has expired: we have published a claim tx that pays directly to our wallet
-    val htlcsInOnChain = localCommitPublished.htlcTxs.values.flatten.collect { case htlcTx: HtlcSuccessTx => htlcTx }
+    val htlcsInOnChain = localCommit.htlcTxsAndRemoteSigs.collect { case HtlcTxAndRemoteSig(htlcTx: HtlcSuccessTx, _) => htlcTx }
       .filter(htlcTx => localCommitPublished.claimHtlcDelayedTxs.exists(_.input.outPoint.txid == htlcTx.tx.txid))
       .map(_.htlcId)
       .toSet
     // outgoing htlcs that have timed out and the to-local delay has expired: we have published a claim tx that pays directly to our wallet
-    val htlcsOutOnChain = localCommitPublished.htlcTxs.values.flatten.collect { case htlcTx: HtlcTimeoutTx => htlcTx }
+    val htlcsOutOnChain =  localCommit.htlcTxsAndRemoteSigs.collect { case HtlcTxAndRemoteSig(htlcTx: HtlcTimeoutTx, _) => htlcTx }
       .filter(htlcTx => localCommitPublished.claimHtlcDelayedTxs.exists(_.input.outPoint.txid == htlcTx.tx.txid))
       .map(_.htlcId)
       .toSet

eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelData.scala

@@ -19,7 +19,7 @@ package fr.acinq.eclair.channel
 import akka.actor.{ActorRef, PossiblyHarmful, typed}
 import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
 import fr.acinq.bitcoin.scalacompat.{ByteVector32, DeterministicWallet, OutPoint, Satoshi, SatoshiLong, Transaction, TxId, TxOut}
-import fr.acinq.eclair.blockchain.fee.{ConfirmationTarget, FeeratePerKw}
+import fr.acinq.eclair.blockchain.fee.{ConfirmationTarget, FeeratePerKw, OnChainFeeConf}
 import fr.acinq.eclair.channel.LocalFundingStatus.DualFundedUnconfirmedFundingTx
 import fr.acinq.eclair.channel.fund.InteractiveTxBuilder._
 import fr.acinq.eclair.channel.fund.{InteractiveTxBuilder, InteractiveTxSigningSession}
@@ -313,7 +313,7 @@ sealed trait CommitPublished {
   def commitTx: Transaction
   /** Map of relevant outpoints that have been spent and the confirmed transaction that spends them. */
   def irrevocablySpent: Map[OutPoint, Transaction]
-
+  /** Returns true if the commitment transaction is confirmed. */
   def isConfirmed: Boolean = {
     // NB: if multiple transactions end up in the same block, the first confirmation we receive may not be the commit tx.
     // However if the confirmed tx spends from the commit tx, we know that the commit tx is already confirmed and we know
@@ -329,11 +329,31 @@ sealed trait CommitPublished {
  * @param htlcTxs                  txs claiming HTLCs. There will be one entry for each pending HTLC. The value will be
  *                                 None only for incoming HTLCs for which we don't have the preimage (we can't claim them yet).
  * @param claimHtlcDelayedTxs      3rd-stage txs (spending the output of HTLC txs).
- * @param claimAnchorTxs           txs spending anchor outputs to bump the feerate of the commitment tx (if applicable).
- *                                 We currently only claim our local anchor, but it would be nice to claim both when it
- *                                 is economical to do so to avoid polluting the utxo set.
+ * @param claimAnchorTxs           txs spending our anchor output to bump the feerate of the commitment tx (if applicable).
  */
 case class LocalCommitPublished(commitTx: Transaction, claimMainDelayedOutputTx: Option[ClaimLocalDelayedOutputTx], htlcTxs: Map[OutPoint, Option[HtlcTx]], claimHtlcDelayedTxs: List[HtlcDelayedTx], claimAnchorTxs: List[ClaimAnchorOutputTx], irrevocablySpent: Map[OutPoint, Transaction]) extends CommitPublished {
+  val htlcTxOutpoints: Set[OutPoint] = htlcTxs.keySet
+
+  // We previously used a list of anchor transactions because we included the confirmation target, but that's obsolete and should be overridden on updates.
+  val claimAnchorTx_opt: Option[ClaimAnchorOutputTx] = claimAnchorTxs.headOption
+
+  /** Compute the confirmation target that should be used to get the [[commitTx]] confirmed. */
+  def confirmationTarget(onChainFeeConf: OnChainFeeConf, commitment: FullCommitment): Option[ConfirmationTarget] = {
+    if (isConfirmed) {
+      None
+    } else {
+      val expiries = commitment.localCommit.htlcTxsAndRemoteSigs.collect {
+        case htlcTxsAndRemoteSig if htlcTxOutpoints.contains(htlcTxsAndRemoteSig.htlcTx.input.outPoint) => htlcTxsAndRemoteSig.htlcTx.htlcExpiry.blockHeight
+      }
+      expiries.minOption match {
+        // If there are pending HTLCs, we must get the commit tx confirmed before they timeout.
+        case Some(htlcExpiry) => Some(ConfirmationTarget.Absolute(htlcExpiry))
+        // Otherwise, we don't have funds at risk, so we can aim for a slower confirmation.
+        case None => Some(ConfirmationTarget.Priority(onChainFeeConf.feeTargets.closing))
+      }
+    }
+  }
+
   /**
    * A local commit is considered done when:
    * - all commitment tx outputs that we can spend have been spent and confirmed (even if the spending tx was not ours)
@@ -346,7 +366,7 @@ case class LocalCommitPublished(commitTx: Transaction, claimMainDelayedOutputTx:
     // is our main output confirmed (if we have one)?
     val isMainOutputConfirmed = claimMainDelayedOutputTx.forall(tx => irrevocablySpent.contains(tx.input.outPoint))
     // are all htlc outputs from the commitment tx spent (we need to check them all because we may receive preimages later)?
-    val allHtlcsSpent = (htlcTxs.keySet -- irrevocablySpent.keys).isEmpty
+    val allHtlcsSpent = (htlcTxOutpoints -- irrevocablySpent.keys).isEmpty
     // are all outputs from htlc txs spent?
     val unconfirmedHtlcDelayedTxs = claimHtlcDelayedTxs.map(_.input.outPoint)
       // only the txs which parents are already confirmed may get confirmed (note that this eliminates outputs that have been double-spent by a competing tx)
@@ -363,11 +383,26 @@ case class LocalCommitPublished(commitTx: Transaction, claimMainDelayedOutputTx:
  * @param claimMainOutputTx tx claiming our main output (if we have one).
  * @param claimHtlcTxs      txs claiming HTLCs. There will be one entry for each pending HTLC. The value will be None
  *                          only for incoming HTLCs for which we don't have the preimage (we can't claim them yet).
- * @param claimAnchorTxs    txs spending anchor outputs to bump the feerate of the commitment tx (if applicable).
- *                          We currently only claim our local anchor, but it would be nice to claim both when it is
- *                          economical to do so to avoid polluting the utxo set.
+ * @param claimAnchorTxs    txs spending our anchor output to bump the feerate of the commitment tx (if applicable).
  */
 case class RemoteCommitPublished(commitTx: Transaction, claimMainOutputTx: Option[ClaimRemoteCommitMainOutputTx], claimHtlcTxs: Map[OutPoint, Option[ClaimHtlcTx]], claimAnchorTxs: List[ClaimAnchorOutputTx], irrevocablySpent: Map[OutPoint, Transaction]) extends CommitPublished {
+  // We previously used a list of anchor transactions because we included the confirmation target, but that's obsolete and should be overridden on updates.
+  val claimAnchorTx_opt: Option[ClaimAnchorOutputTx] = claimAnchorTxs.headOption
+
+  /** Compute the confirmation target that should be used to get the [[commitTx]] confirmed. */
+  def confirmationTarget(onChainFeeConf: OnChainFeeConf): Option[ConfirmationTarget] = {
+    if (isConfirmed) {
+      None
+    } else {
+      claimHtlcTxs.values.flatten.map(_.htlcExpiry.blockHeight).minOption match {
+        // If there are pending HTLCs, we must get the commit tx confirmed before they timeout.
+        case Some(htlcExpiry) => Some(ConfirmationTarget.Absolute(htlcExpiry))
+        // Otherwise, we don't have funds at risk, so we can aim for a slower confirmation.
+        case None => Some(ConfirmationTarget.Priority(onChainFeeConf.feeTargets.closing))
+      }
+    }
+  }
+
   /**
    * A remote commit is considered done when all commitment tx outputs that we can spend have been spent and confirmed
    * (even if the spending tx was not ours).

eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala

@@ -237,6 +237,19 @@ case class RemoteCommit(index: Long, spec: CommitmentSpec, txid: TxId, remotePer
 /** We have the next remote commit when we've sent our commit_sig but haven't yet received their revoke_and_ack. */
 case class NextRemoteCommit(sig: CommitSig, commit: RemoteCommit)
 
+/**
+ * If we ignore revoked commitments, there can be at most three concurrent commitment transactions during a force-close:
+ *  - the local commitment
+ *  - the remote commitment
+ *  - the next remote commitment, if we sent commit_sig but haven't yet received revoke_and_ack
+ */
+case class CommitTxIds(localCommitTxId: TxId, remoteCommitTxId: TxId, nextRemoteCommitTxId_opt: Option[TxId]) {
+  val txIds: Set[TxId] = nextRemoteCommitTxId_opt match {
+    case Some(nextRemoteCommitTxId) => Set(localCommitTxId, remoteCommitTxId, nextRemoteCommitTxId)
+    case None => Set(localCommitTxId, remoteCommitTxId)
+  }
+}
+
 /**
  * A minimal commitment for a given funding tx.
  *
@@ -255,6 +268,7 @@ case class Commitment(fundingTxIndex: Long,
                       localCommit: LocalCommit, remoteCommit: RemoteCommit, nextRemoteCommit_opt: Option[NextRemoteCommit]) {
   val commitInput: InputInfo = localCommit.commitTxAndRemoteSig.commitTx.input
   val fundingTxId: TxId = commitInput.outPoint.txid
+  val commitTxIds: CommitTxIds = CommitTxIds(localCommit.commitTxAndRemoteSig.commitTx.tx.txid, remoteCommit.txid, nextRemoteCommit_opt.map(_.commit.txid))
   val capacity: Satoshi = commitInput.txOut.amount
   /** Once the funding transaction is confirmed, short_channel_id matching this transaction. */
   val shortChannelId_opt: Option[RealShortChannelId] = localFundingStatus match {
@@ -735,6 +749,7 @@ case class FullCommitment(params: ChannelParams, changes: CommitmentChanges,
   val remoteParams: RemoteParams = params.remoteParams
   val commitInput: InputInfo = localCommit.commitTxAndRemoteSig.commitTx.input
   val fundingTxId: TxId = commitInput.outPoint.txid
+  val commitTxIds: CommitTxIds = CommitTxIds(localCommit.commitTxAndRemoteSig.commitTx.tx.txid, remoteCommit.txid, nextRemoteCommit_opt.map(_.commit.txid))
   val capacity: Satoshi = commitInput.txOut.amount
   val commitment: Commitment = Commitment(fundingTxIndex, firstRemoteCommitIndex, remoteFundingPubKey, localFundingStatus, remoteFundingStatus, localCommit, remoteCommit, nextRemoteCommit_opt)
 

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

@@ -850,25 +850,6 @@ object Helpers {
       if (localPaysCommitTxFees) commitInput.txOut.amount - commitTx.txOut.map(_.amount).sum else 0 sat
     }
 
-    /**
-     * This function checks if the proposed confirmation target is more aggressive than whatever confirmation target
-     * we previously had. Note that absolute targets are always considered more aggressive than relative targets.
-     */
-    private def shouldUpdateAnchorTxs(anchorTxs: List[ClaimAnchorOutputTx], confirmationTarget: ConfirmationTarget): Boolean = {
-      anchorTxs
-        .collect { case tx: ClaimAnchorOutputTx => tx.confirmationTarget }
-        .forall {
-          case ConfirmationTarget.Absolute(current) => confirmationTarget match {
-            case ConfirmationTarget.Absolute(proposed) => proposed < current
-            case _: ConfirmationTarget.Priority => false
-          }
-          case ConfirmationTarget.Priority(current) => confirmationTarget match {
-            case _: ConfirmationTarget.Absolute => true
-            case ConfirmationTarget.Priority(proposed) => current < proposed
-          }
-        }
-    }
-
     object LocalClose {
 
       /**
@@ -900,23 +881,17 @@ object Helpers {
         )
         val spendAnchors = htlcTxs.nonEmpty || onChainFeeConf.spendAnchorWithoutHtlcs
         if (spendAnchors) {
-          // If we don't have pending HTLCs, we don't have funds at risk, so we can aim for a slower confirmation.
-          val confirmCommitBefore = htlcTxs.values.flatten.map(htlcTx => ConfirmationTarget.Absolute(htlcTx.htlcExpiry.blockHeight)).minByOption(_.confirmBefore).getOrElse(ConfirmationTarget.Priority(onChainFeeConf.feeTargets.closing))
-          claimAnchors(fundingKey, commitmentKeys, lcp, confirmCommitBefore, commitment.params.commitmentFormat)
+          claimAnchors(fundingKey, commitmentKeys, lcp, commitment.params.commitmentFormat)
         } else {
           lcp
         }
       }
 
-      def claimAnchors(fundingKey: PrivateKey, commitKeys: LocalCommitmentKeys, lcp: LocalCommitPublished, confirmationTarget: ConfirmationTarget, commitmentFormat: CommitmentFormat)(implicit log: LoggingAdapter): LocalCommitPublished = {
-        if (shouldUpdateAnchorTxs(lcp.claimAnchorTxs, confirmationTarget)) {
-          val claimAnchorTx = withTxGenerationLog("local-anchor") {
-            ClaimAnchorOutputTx.createUnsignedTx(fundingKey, commitKeys.publicKeys, lcp.commitTx, confirmationTarget, commitmentFormat)
-          }
-          lcp.copy(claimAnchorTxs = claimAnchorTx.toList)
-        } else {
-          lcp
+      def claimAnchors(fundingKey: PrivateKey, commitKeys: LocalCommitmentKeys, lcp: LocalCommitPublished, commitmentFormat: CommitmentFormat)(implicit log: LoggingAdapter): LocalCommitPublished = {
+        val claimAnchorTx = withTxGenerationLog("local-anchor") {
+          ClaimAnchorOutputTx.createUnsignedTx(fundingKey, commitKeys.publicKeys, lcp.commitTx, commitmentFormat)
         }
+        lcp.copy(claimAnchorTxs = claimAnchorTx.toList)
       }
 
       /**
@@ -973,7 +948,8 @@ object Helpers {
        * doing that because it introduces a lot of subtle edge cases.
        */
       def claimHtlcDelayedOutput(localCommitPublished: LocalCommitPublished, channelKeys: ChannelKeys, commitment: FullCommitment, tx: Transaction, feerates: FeeratesPerKw, onChainFeeConf: OnChainFeeConf, finalScriptPubKey: ByteVector)(implicit log: LoggingAdapter): (LocalCommitPublished, Option[HtlcDelayedTx]) = {
-        if (tx.txIn.exists(txIn => localCommitPublished.htlcTxs.contains(txIn.outPoint))) {
+        val htlcTxsMap = claimHtlcOutputs(commitment.localKeys(channelKeys), commitment)
+        if (tx.txIn.exists(txIn => htlcTxsMap.contains(txIn.outPoint))) {
           val feeratePerKwDelayed = onChainFeeConf.getClosingFeerate(feerates)
           val commitKeys = commitment.localKeys(channelKeys)
           // Note that this will return None if the transaction wasn't one of our HTLC transactions, which may happen
@@ -1015,23 +991,17 @@ object Helpers {
         )
         val spendAnchors = htlcTxs.nonEmpty || onChainFeeConf.spendAnchorWithoutHtlcs
         if (spendAnchors) {
-          // If we don't have pending HTLCs, we don't have funds at risk, so we use the normal closing priority.
-          val confirmCommitBefore = htlcTxs.values.flatten.map(htlcTx => ConfirmationTarget.Absolute(htlcTx.htlcExpiry.blockHeight)).minByOption(_.confirmBefore).getOrElse(ConfirmationTarget.Priority(onChainFeeConf.feeTargets.closing))
-          claimAnchors(fundingKey, commitKeys, rcp, confirmCommitBefore, commitment.params.commitmentFormat)
+          claimAnchors(fundingKey, commitKeys, rcp, commitment.params.commitmentFormat)
         } else {
           rcp
         }
       }
 
-      def claimAnchors(fundingKey: PrivateKey, commitKeys: RemoteCommitmentKeys, rcp: RemoteCommitPublished, confirmationTarget: ConfirmationTarget, commitmentFormat: CommitmentFormat)(implicit log: LoggingAdapter): RemoteCommitPublished = {
-        if (shouldUpdateAnchorTxs(rcp.claimAnchorTxs, confirmationTarget)) {
-          val claimAnchorTx = withTxGenerationLog("remote-anchor") {
-            ClaimAnchorOutputTx.createUnsignedTx(fundingKey, commitKeys.publicKeys, rcp.commitTx, confirmationTarget, commitmentFormat)
-          }
-          rcp.copy(claimAnchorTxs = claimAnchorTx.toList)
-        } else {
-          rcp
+      def claimAnchors(fundingKey: PrivateKey, commitKeys: RemoteCommitmentKeys, rcp: RemoteCommitPublished, commitmentFormat: CommitmentFormat)(implicit log: LoggingAdapter): RemoteCommitPublished = {
+        val claimAnchorTx = withTxGenerationLog("remote-anchor") {
+          ClaimAnchorOutputTx.createUnsignedTx(fundingKey, commitKeys.publicKeys, rcp.commitTx, commitmentFormat)
         }
+        rcp.copy(claimAnchorTxs = claimAnchorTx.toList)
       }
 
       /**
@@ -1274,14 +1244,14 @@ object Helpers {
      * @param tx a tx that has reached mindepth
      * @return a set of htlcs that need to be failed upstream
      */
-    def trimmedOrTimedOutHtlcs(commitmentFormat: CommitmentFormat, localCommit: LocalCommit, localCommitPublished: LocalCommitPublished, localDustLimit: Satoshi, tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] = {
+    def trimmedOrTimedOutHtlcs(commitmentFormat: CommitmentFormat, localCommit: LocalCommit, htlcTxs: Map[OutPoint, Option[HtlcTx]], localDustLimit: Satoshi, tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] = {
       val untrimmedHtlcs = Transactions.trimOfferedHtlcs(localDustLimit, localCommit.spec, commitmentFormat).map(_.add)
       if (tx.txid == localCommit.commitTxAndRemoteSig.commitTx.tx.txid) {
         // The commitment tx is confirmed: we can immediately fail all dust htlcs (they don't have an output in the tx).
         localCommit.spec.htlcs.collect(outgoing) -- untrimmedHtlcs
       } else {
         // Maybe this is a timeout tx: in that case we can resolve and fail the corresponding htlc.
-        tx.txIn.flatMap(txIn => localCommitPublished.htlcTxs.get(txIn.outPoint) match {
+        tx.txIn.flatMap(txIn => htlcTxs.get(txIn.outPoint) match {
           // This may also be our peer claiming the HTLC by revealing the preimage: in that case we have already
           // extracted the preimage with [[extractPreimages]] and relayed it upstream.
           case Some(Some(htlcTimeoutTx: HtlcTimeoutTx)) if Scripts.extractPreimagesFromClaimHtlcSuccess(tx).isEmpty =>