Refactor HTLC-penalty txs creation (#3071)

We previously split the creation of HTLC-penalty transactions between
`Helpers.scala` and `Transactions.scala`. It makes more sense to group
everything in `Transactions.scala` and is a pre-requisite for the next
wave of transaction refactoring.

Author: t-bast

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

@@ -26,8 +26,8 @@ import fr.acinq.eclair.blockchain.OnChainPubkeyCache
 import fr.acinq.eclair.blockchain.fee._
 import fr.acinq.eclair.channel.fsm.Channel
 import fr.acinq.eclair.channel.fsm.Channel.REFRESH_CHANNEL_UPDATE_INTERVAL
-import fr.acinq.eclair.crypto.keymanager.{ChannelKeys, LocalCommitmentKeys, RemoteCommitmentKeys}
 import fr.acinq.eclair.crypto.ShaChain
+import fr.acinq.eclair.crypto.keymanager.{ChannelKeys, LocalCommitmentKeys, RemoteCommitmentKeys}
 import fr.acinq.eclair.db.ChannelsDb
 import fr.acinq.eclair.payment.relay.Relayer.RelayFees
 import fr.acinq.eclair.router.Announcements
@@ -1170,10 +1170,10 @@ object Helpers {
         val revocationKey = channelKeys.revocationKey(remotePerCommitmentSecret)
 
         val feerateMain = onChainFeeConf.getClosingFeerate(feerates)
-        // we need to use a high fee here for punishment txs because after a delay they can be spent by the counterparty
+        // We need to use a high fee here for punishment txs because after a delay they can be spent by the counterparty.
         val feeratePenalty = feerates.fast
 
-        // first we will claim our main output right away
+        // First we will claim our main output right away.
         val mainTx = commitKeys.ourPaymentKey match {
           case Left(_) =>
             log.info("channel uses option_static_remotekey to pay directly to our wallet, there is nothing to do")
@@ -1194,34 +1194,24 @@ object Helpers {
           }
         }
 
-        // then we punish them by stealing their main output
+        // Then we punish them by stealing their main output.
         val mainPenaltyTx = withTxGenerationLog("main-penalty") {
           Transactions.makeMainPenaltyTx(commitKeys, commitTx, localParams.dustLimit, finalScriptPubKey, localParams.toSelfDelay, feeratePenalty).map(txinfo => {
             val sig = txinfo.sign(revocationKey, TxOwner.Local, commitmentFormat, Map.empty)
             txinfo.addSigs(sig)
           })
         }
 
-        // we retrieve the information needed to rebuild htlc scripts
+        // We retrieve the historical information needed to rebuild htlc scripts.
         val htlcInfos = db.listHtlcInfos(channelId, commitmentNumber)
         log.info("got {} htlcs for commitmentNumber={}", htlcInfos.size, commitmentNumber)
-        val htlcsRedeemScripts = (
-          htlcInfos.map { case (paymentHash, cltvExpiry) => Scripts.htlcReceived(commitKeys.publicKeys, paymentHash, cltvExpiry, commitmentFormat) } ++
-            htlcInfos.map { case (paymentHash, _) => Scripts.htlcOffered(commitKeys.publicKeys, paymentHash, commitmentFormat) }
-          )
-          .map(redeemScript => Script.write(pay2wsh(redeemScript)) -> Script.write(redeemScript))
-          .toMap
-
-        // and finally we steal the htlc outputs
-        val htlcPenaltyTxs = commitTx.txOut.zipWithIndex.collect { case (txOut, outputIndex) if htlcsRedeemScripts.contains(txOut.publicKeyScript) =>
-          val htlcRedeemScript = htlcsRedeemScripts(txOut.publicKeyScript)
+        // And finally we steal the htlc outputs.
+        val htlcPenaltyTxs = Transactions.makeHtlcPenaltyTxs(commitKeys, commitTx, htlcInfos, localParams.dustLimit, finalScriptPubKey, feeratePenalty, commitmentFormat).flatMap { htlcPenalty =>
           withTxGenerationLog("htlc-penalty") {
-            Transactions.makeHtlcPenaltyTx(commitKeys, commitTx, outputIndex, htlcRedeemScript, localParams.dustLimit, finalScriptPubKey, feeratePenalty).map(htlcPenalty => {
-              val sig = htlcPenalty.sign(revocationKey, TxOwner.Local, commitmentFormat, Map.empty)
-              htlcPenalty.addSigs(commitKeys, sig)
-            })
+            val sig = htlcPenalty.sign(revocationKey, TxOwner.Local, commitmentFormat, Map.empty)
+            Right(htlcPenalty.addSigs(commitKeys, sig))
           }
-        }.toList.flatten
+        }.toList
 
         RevokedCommitPublished(
           commitTx = commitTx,

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

@@ -868,6 +868,7 @@ object Transactions {
   def makeClaimHtlcDelayedOutputPenaltyTxs(keys: RemoteCommitmentKeys, htlcTx: Transaction, localDustLimit: Satoshi, toLocalDelay: CltvExpiryDelta, localFinalScriptPubKey: ByteVector, feeratePerKw: FeeratePerKw): Seq[Either[TxGenerationSkipped, ClaimHtlcDelayedOutputPenaltyTx]] = {
     val redeemScript = toLocalDelayed(keys.publicKeys, toLocalDelay)
     val pubkeyScript = write(pay2wsh(redeemScript))
+    // Note that we check *all* outputs of the tx, because it could spend a batch of HTLC outputs from the commit tx.
     findPubKeyScriptIndexes(htlcTx, pubkeyScript) match {
       case Left(skip) => Seq(Left(skip))
       case Right(outputIndexes) => outputIndexes.map(outputIndex => {
@@ -908,7 +909,35 @@ object Transactions {
     }
   }
 
-  def makeHtlcPenaltyTx(keys: RemoteCommitmentKeys, commitTx: Transaction, htlcOutputIndex: Int, redeemScript: ByteVector, localDustLimit: Satoshi, localFinalScriptPubKey: ByteVector, feeratePerKw: FeeratePerKw): Either[TxGenerationSkipped, HtlcPenaltyTx] = {
+  def makeHtlcPenaltyTxs(keys: RemoteCommitmentKeys,
+                         commitTx: Transaction,
+                         htlcs: Seq[(ByteVector32, CltvExpiry)],
+                         localDustLimit: Satoshi,
+                         localFinalScriptPubKey: ByteVector,
+                         feeratePerKw: FeeratePerKw,
+                         commitmentFormat: CommitmentFormat): Seq[HtlcPenaltyTx] = {
+    // We create the output scripts for the corresponding HTLCs.
+    val redeemInfos: Map[ByteVector, (ByteVector, ByteVector32, CltvExpiry)] = htlcs.flatMap {
+      case (paymentHash, expiry) =>
+        // We don't know if this was an incoming or outgoing HTLC, so we try both cases.
+        val offered = htlcOffered(keys.publicKeys, paymentHash, commitmentFormat)
+        val received = htlcReceived(keys.publicKeys, paymentHash, expiry, commitmentFormat)
+        Seq(
+          write(pay2wsh(offered)) -> (write(offered), paymentHash, expiry),
+          write(pay2wsh(received)) -> (write(received), paymentHash, expiry)
+        )
+    }.toMap
+    // We check every output of the commitment transaction, and create an HTLC-penalty transaction if it is an HTLC output.
+    commitTx.txOut.zipWithIndex.map {
+      case (txOut, outputIndex) =>
+        redeemInfos.get(txOut.publicKeyScript) match {
+          case Some((redeemScript, _, _)) => makeHtlcPenaltyTx(keys, commitTx, outputIndex, redeemScript, localDustLimit, localFinalScriptPubKey, feeratePerKw)
+          case None => Left(OutputNotFound)
+        }
+    }.collect { case Right(tx) => tx }
+  }
+
+  private def makeHtlcPenaltyTx(keys: RemoteCommitmentKeys, commitTx: Transaction, htlcOutputIndex: Int, redeemScript: ByteVector, localDustLimit: Satoshi, localFinalScriptPubKey: ByteVector, feeratePerKw: FeeratePerKw): Either[TxGenerationSkipped, HtlcPenaltyTx] = {
     val input = InputInfo(OutPoint(commitTx, htlcOutputIndex), commitTx.txOut(htlcOutputIndex), redeemScript)
     val unsignedTx = Transaction(
       version = 2,

eclair-core/src/test/scala/fr/acinq/eclair/transactions/TransactionsSpec.scala

@@ -26,7 +26,7 @@ import fr.acinq.eclair._
 import fr.acinq.eclair.blockchain.fee.{ConfirmationTarget, FeeratePerKw}
 import fr.acinq.eclair.channel.Helpers.Funding
 import fr.acinq.eclair.crypto.keymanager.{LocalCommitmentKeys, RemoteCommitmentKeys}
-import fr.acinq.eclair.transactions.CommitmentOutput.{InHtlc, OutHtlc}
+import fr.acinq.eclair.transactions.CommitmentOutput.OutHtlc
 import fr.acinq.eclair.transactions.Scripts._
 import fr.acinq.eclair.transactions.Transactions.AnchorOutputsCommitmentFormat.anchorAmount
 import fr.acinq.eclair.transactions.Transactions._
@@ -174,9 +174,10 @@ class TransactionsSpec extends AnyFunSuite with Logging {
       val redeemScript = htlcReceived(localKeys.publicKeys, htlc.paymentHash, htlc.cltvExpiry, DefaultCommitmentFormat)
       val pubKeyScript = write(pay2wsh(redeemScript))
       val commitTx = Transaction(version = 2, txIn = Nil, txOut = TxOut(htlc.amountMsat.truncateToSatoshi, pubKeyScript) :: Nil, lockTime = 0)
-      val Right(htlcPenaltyTx) = makeHtlcPenaltyTx(remoteKeys, commitTx, 0, Script.write(redeemScript), localDustLimit, finalPubKeyScript, feeratePerKw)
+      val htlcPenaltyTxs = makeHtlcPenaltyTxs(remoteKeys, commitTx, Seq((htlc.paymentHash, htlc.cltvExpiry)), localDustLimit, finalPubKeyScript, feeratePerKw, DefaultCommitmentFormat)
+      assert(htlcPenaltyTxs.size == 1)
       // we use dummy signatures to compute the weight
-      val weight = htlcPenaltyTx.addSigs(remoteKeys, PlaceHolderSig).tx.weight()
+      val weight = htlcPenaltyTxs.head.addSigs(remoteKeys, PlaceHolderSig).tx.weight()
       assert(htlcPenaltyWeight == weight)
     }
     {
@@ -414,16 +415,15 @@ class TransactionsSpec extends AnyFunSuite with Logging {
       assert(claimHtlcDelayedPenaltyTx1 == Seq(Left(AmountBelowDustLimit)))
     }
     {
-      // remote spends offered HTLC output with revocation key
-      val script = Script.write(Scripts.htlcOffered(remoteKeys.publicKeys, htlc1.paymentHash, DefaultCommitmentFormat))
-      val Some(htlcOutputIndex) = outputs.zipWithIndex.find {
-        case (CommitmentOutputLink(_, _, OutHtlc(OutgoingHtlc(someHtlc))), _) => someHtlc.id == htlc1.id
-        case _ => false
-      }.map(_._2)
-      val Right(htlcPenaltyTx) = makeHtlcPenaltyTx(remoteKeys, commitTx.tx, htlcOutputIndex, script, localDustLimit, finalPubKeyScript, feeratePerKw)
-      val sig = htlcPenaltyTx.sign(localRevocationPriv, TxOwner.Local, DefaultCommitmentFormat, Map.empty)
-      val signed = htlcPenaltyTx.addSigs(remoteKeys, sig)
-      assert(checkSpendable(signed).isSuccess)
+      // remote spends HTLC outputs with revocation key
+      val htlcs = spec.htlcs.map(_.add).map(add => (add.paymentHash, add.cltvExpiry)).toSeq
+      val htlcPenaltyTxs = makeHtlcPenaltyTxs(remoteKeys, commitTx.tx, htlcs, localDustLimit, finalPubKeyScript, feeratePerKw, DefaultCommitmentFormat)
+      assert(htlcPenaltyTxs.size == 4) // the first 4 htlcs are above the dust limit
+      htlcPenaltyTxs.foreach(htlcPenaltyTx => {
+        val sig = htlcPenaltyTx.sign(localRevocationPriv, TxOwner.Local, DefaultCommitmentFormat, Map.empty)
+        val signed = htlcPenaltyTx.addSigs(remoteKeys, sig)
+        assert(checkSpendable(signed).isSuccess)
+      })
     }
     {
       // remote spends htlc2's htlc-success tx with revocation key
@@ -435,18 +435,6 @@ class TransactionsSpec extends AnyFunSuite with Logging {
       val claimHtlcDelayedPenaltyTx1 = makeClaimHtlcDelayedOutputPenaltyTxs(remoteKeys, htlcSuccessTxs(0).tx, localDustLimit, toLocalDelay, finalPubKeyScript, feeratePerKw)
       assert(claimHtlcDelayedPenaltyTx1 == Seq(Left(AmountBelowDustLimit)))
     }
-    {
-      // remote spends received HTLC output with revocation key
-      val script = Script.write(Scripts.htlcReceived(remoteKeys.publicKeys, htlc2.paymentHash, htlc2.cltvExpiry, DefaultCommitmentFormat))
-      val Some(htlcOutputIndex) = outputs.zipWithIndex.find {
-        case (CommitmentOutputLink(_, _, InHtlc(IncomingHtlc(someHtlc))), _) => someHtlc.id == htlc2.id
-        case _ => false
-      }.map(_._2)
-      val Right(htlcPenaltyTx) = makeHtlcPenaltyTx(remoteKeys, commitTx.tx, htlcOutputIndex, script, localDustLimit, finalPubKeyScript, feeratePerKw)
-      val sig = htlcPenaltyTx.sign(localRevocationPriv, TxOwner.Local, DefaultCommitmentFormat, Map.empty)
-      val signed = htlcPenaltyTx.addSigs(remoteKeys, sig)
-      assert(checkSpendable(signed).isSuccess)
-    }
   }
 
   test("generate valid commitment with some outputs that don't materialize (anchor outputs)") {
@@ -748,30 +736,15 @@ class TransactionsSpec extends AnyFunSuite with Logging {
       assert(claimed.map(_.input.outPoint).toSet.size == 3)
     }
     {
-      // remote spends offered htlc output with revocation key
-      val script = Script.write(Scripts.htlcOffered(remoteKeys.publicKeys, htlc1.paymentHash, UnsafeLegacyAnchorOutputsCommitmentFormat))
-      val Some(htlcOutputIndex) = commitTxOutputs.zipWithIndex.find {
-        case (CommitmentOutputLink(_, _, OutHtlc(OutgoingHtlc(someHtlc))), _) => someHtlc.id == htlc1.id
-        case _ => false
-      }.map(_._2)
-      val Right(htlcPenaltyTx) = makeHtlcPenaltyTx(remoteKeys, commitTx.tx, htlcOutputIndex, script, localDustLimit, finalPubKeyScript, feeratePerKw)
-      val sig = htlcPenaltyTx.sign(localRevocationPriv, TxOwner.Local, UnsafeLegacyAnchorOutputsCommitmentFormat, Map.empty)
-      val signed = htlcPenaltyTx.addSigs(remoteKeys, sig)
-      assert(checkSpendable(signed).isSuccess)
-    }
-    {
-      // remote spends received htlc output with revocation key
-      for (htlc <- Seq(htlc2a, htlc2b)) {
-        val script = Script.write(Scripts.htlcReceived(remoteKeys.publicKeys, htlc.paymentHash, htlc.cltvExpiry, UnsafeLegacyAnchorOutputsCommitmentFormat))
-        val Some(htlcOutputIndex) = commitTxOutputs.zipWithIndex.find {
-          case (CommitmentOutputLink(_, _, InHtlc(IncomingHtlc(someHtlc))), _) => someHtlc.id == htlc.id
-          case _ => false
-        }.map(_._2)
-        val Right(htlcPenaltyTx) = makeHtlcPenaltyTx(remoteKeys, commitTx.tx, htlcOutputIndex, script, localDustLimit, finalPubKeyScript, feeratePerKw)
+      // remote spends htlc outputs with revocation key
+      val htlcs = spec.htlcs.map(_.add).map(add => (add.paymentHash, add.cltvExpiry)).toSeq
+      val htlcPenaltyTxs = makeHtlcPenaltyTxs(remoteKeys, commitTx.tx, htlcs, localDustLimit, finalPubKeyScript, feeratePerKw, UnsafeLegacyAnchorOutputsCommitmentFormat)
+      assert(htlcPenaltyTxs.size == 5) // the first 5 htlcs are above the dust limit
+      htlcPenaltyTxs.foreach(htlcPenaltyTx => {
         val sig = htlcPenaltyTx.sign(localRevocationPriv, TxOwner.Local, UnsafeLegacyAnchorOutputsCommitmentFormat, Map.empty)
         val signed = htlcPenaltyTx.addSigs(remoteKeys, sig)
         assert(checkSpendable(signed).isSuccess)
-      }
+      })
     }
   }