Check HTLC output status before funding HTLC tx (#2944)

When a channel force-closes, we publish our commit tx and HTLC txs.
HTLC transactions conflict with our peer's transactions that also spend
the HTLC outputs. If our peer is able to get their transaction confirmed
before ours, we should stop retrying to publish our HTLC transaction as
that will never succeed.

Since we didn't check the output status, we kept retrying until the
channel was closed (which requires waiting for the `to_self_delay`).
The retries always fail at funding time: `bitcoind` returns an error
saying that the UTXO cannot be found (which is expected because it has
already been spent by our peer). This creates a lot of unnecessary
retries and a lot of noise in the logs.

This scenario usually happens when our peer didn't send the preimage
before force-closing the channel, but reveals it on-chain before the
HTLC timeout: when that happens we kept retrying to publish our HTLC
timeout transaction, which cannot succeed.

We now check the output status in our publishing preconditions, and
immediately abort if the output has already been spent by a confirmed
transaction.

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala

@@ -125,7 +125,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onChainKeyManag
    * Return true if this output has already been spent by a confirmed transaction.
    * Note that a reorg may invalidate the result of this function and make a spent output spendable again.
    */
-  private def isTransactionOutputSpent(txid: TxId, outputIndex: Int)(implicit ec: ExecutionContext): Future[Boolean] = {
+  def isTransactionOutputSpent(txid: TxId, outputIndex: Int)(implicit ec: ExecutionContext): Future[Boolean] = {
     getTxConfirmations(txid).flatMap {
       case Some(confirmations) if confirmations > 0 =>
         // There is an important limitation when using isTransactionOutputSpendable: if it returns false, it can mean a

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

@@ -997,7 +997,8 @@ object Helpers {
         val remoteHtlcPubkey = Generators.derivePubKey(commitment.remoteParams.htlcBasepoint, remoteCommit.remotePerCommitmentPoint)
         val remoteRevocationPubkey = Generators.revocationPubKey(keyManager.revocationPoint(channelKeyPath).publicKey, remoteCommit.remotePerCommitmentPoint)
         val remoteDelayedPaymentPubkey = Generators.derivePubKey(commitment.remoteParams.delayedPaymentBasepoint, remoteCommit.remotePerCommitmentPoint)
-        val localPaymentPubkey = Generators.derivePubKey(keyManager.paymentPoint(channelKeyPath).publicKey, remoteCommit.remotePerCommitmentPoint)
+        val localPaymentBasepoint = commitment.localParams.walletStaticPaymentBasepoint.getOrElse(keyManager.paymentPoint(channelKeyPath).publicKey)
+        val localPaymentPubkey = if (commitment.params.channelFeatures.hasFeature(Features.StaticRemoteKey)) localPaymentBasepoint else Generators.derivePubKey(localPaymentBasepoint, remoteCommit.remotePerCommitmentPoint)
         val outputs = makeCommitTxOutputs(!commitment.localParams.paysCommitTxFees, commitment.remoteParams.dustLimit, remoteRevocationPubkey, commitment.localParams.toSelfDelay, remoteDelayedPaymentPubkey, localPaymentPubkey, remoteHtlcPubkey, localHtlcPubkey, commitment.remoteFundingPubKey, localFundingPubkey, remoteCommit.spec, commitment.params.commitmentFormat)
         // we need to use a rather high fee for htlc-claim because we compete with the counterparty
         val feeratePerKwHtlc = feerates.fast

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPrePublisher.scala

@@ -52,6 +52,7 @@ object ReplaceableTxPrePublisher {
   private case object LocalCommitTxConfirmed extends Command
   private case object RemoteCommitTxConfirmed extends Command
   private case object RemoteCommitTxPublished extends Command
+  private case object HtlcOutputAlreadySpent extends Command
   private case class UnknownFailure(reason: Throwable) extends Command
   // @formatter:on
 
@@ -203,17 +204,29 @@ private class ReplaceableTxPrePublisher(nodeParams: NodeParams,
     }
   }
 
+  /**
+   * We verify that:
+   *  - their commit is not confirmed: if it is, there is no need to publish our htlc transactions
+   *  - the HTLC output isn't already spent by a confirmed transaction (race between HTLC-timeout and HTLC-success)
+   */
+  private def checkHtlcOutput(commitment: FullCommitment, htlcTx: HtlcTx): Future[Command] = {
+    getRemoteCommitConfirmations(commitment).flatMap {
+      case Some(depth) if depth >= nodeParams.channelConf.minDepthBlocks => Future.successful(RemoteCommitTxConfirmed)
+      case _ => bitcoinClient.isTransactionOutputSpent(htlcTx.input.outPoint.txid, htlcTx.input.outPoint.index.toInt).map {
+        case true => HtlcOutputAlreadySpent
+        case false => ParentTxOk
+      }
+    }
+  }
+
   private def checkHtlcPreconditions(htlcTx: HtlcTx): Behavior[Command] = {
-    // We verify that:
-    //  - their commit is not confirmed: if it is, there is no need to publish our htlc transactions
-    //  - if this is an htlc-success transaction, we have the preimage
-    context.pipeToSelf(getRemoteCommitConfirmations(cmd.commitment)) {
-      case Success(Some(depth)) if depth >= nodeParams.channelConf.minDepthBlocks => RemoteCommitTxConfirmed
-      case Success(_) => ParentTxOk
+    context.pipeToSelf(checkHtlcOutput(cmd.commitment, htlcTx)) {
+      case Success(result) => result
       case Failure(reason) => UnknownFailure(reason)
     }
     Behaviors.receiveMessagePartial {
       case ParentTxOk =>
+        // We make sure that if this is an htlc-success transaction, we have the preimage.
         extractHtlcWitnessData(htlcTx, cmd.commitment) match {
           case Some(txWithWitnessData) => replyTo ! PreconditionsOk(txWithWitnessData)
           case None => replyTo ! PreconditionsFailed(TxPublisher.TxRejectedReason.TxSkipped(retryNextBlock = false))
@@ -223,6 +236,10 @@ private class ReplaceableTxPrePublisher(nodeParams: NodeParams,
         log.warn("cannot publish {}: remote commit has been confirmed", cmd.desc)
         replyTo ! PreconditionsFailed(TxPublisher.TxRejectedReason.ConflictingTxConfirmed)
         Behaviors.stopped
+      case HtlcOutputAlreadySpent =>
+        log.warn("cannot publish {}: htlc output has already been spent", cmd.desc)
+        replyTo ! PreconditionsFailed(TxPublisher.TxRejectedReason.ConflictingTxConfirmed)
+        Behaviors.stopped
       case UnknownFailure(reason) =>
         log.error(s"could not check ${cmd.desc} preconditions, proceeding anyway: ", reason)
         // If our checks fail, we don't want it to prevent us from trying to publish our htlc transactions.
@@ -265,17 +282,29 @@ private class ReplaceableTxPrePublisher(nodeParams: NodeParams,
     }
   }
 
+  /**
+   * We verify that:
+   *  - our commit is not confirmed: if it is, there is no need to publish our claim-htlc transactions
+   *  - the HTLC output isn't already spent by a confirmed transaction (race between HTLC-timeout and HTLC-success)
+   */
+  private def checkClaimHtlcOutput(commitment: FullCommitment, claimHtlcTx: ClaimHtlcTx): Future[Command] = {
+    bitcoinClient.getTxConfirmations(commitment.localCommit.commitTxAndRemoteSig.commitTx.tx.txid).flatMap {
+      case Some(depth) if depth >= nodeParams.channelConf.minDepthBlocks => Future.successful(LocalCommitTxConfirmed)
+      case _ => bitcoinClient.isTransactionOutputSpent(claimHtlcTx.input.outPoint.txid, claimHtlcTx.input.outPoint.index.toInt).map {
+        case true => HtlcOutputAlreadySpent
+        case false => ParentTxOk
+      }
+    }
+  }
+
   private def checkClaimHtlcPreconditions(claimHtlcTx: ClaimHtlcTx): Behavior[Command] = {
-    // We verify that:
-    //  - our commit is not confirmed: if it is, there is no need to publish our claim-htlc transactions
-    //  - if this is a claim-htlc-success transaction, we have the preimage
-    context.pipeToSelf(bitcoinClient.getTxConfirmations(cmd.commitment.localCommit.commitTxAndRemoteSig.commitTx.tx.txid)) {
-      case Success(Some(depth)) if depth >= nodeParams.channelConf.minDepthBlocks => LocalCommitTxConfirmed
-      case Success(_) => ParentTxOk
+    context.pipeToSelf(checkClaimHtlcOutput(cmd.commitment, claimHtlcTx)) {
+      case Success(result) => result
       case Failure(reason) => UnknownFailure(reason)
     }
     Behaviors.receiveMessagePartial {
       case ParentTxOk =>
+        // We verify that if this is a claim-htlc-success transaction, we have the preimage.
         extractClaimHtlcWitnessData(claimHtlcTx, cmd.commitment) match {
           case Some(txWithWitnessData) => replyTo ! PreconditionsOk(txWithWitnessData)
           case None => replyTo ! PreconditionsFailed(TxPublisher.TxRejectedReason.TxSkipped(retryNextBlock = false))
@@ -285,6 +314,10 @@ private class ReplaceableTxPrePublisher(nodeParams: NodeParams,
         log.warn("cannot publish {}: local commit has been confirmed", cmd.desc)
         replyTo ! PreconditionsFailed(TxPublisher.TxRejectedReason.ConflictingTxConfirmed)
         Behaviors.stopped
+      case HtlcOutputAlreadySpent =>
+        log.warn("cannot publish {}: htlc output has already been spent", cmd.desc)
+        replyTo ! PreconditionsFailed(TxPublisher.TxRejectedReason.ConflictingTxConfirmed)
+        Behaviors.stopped
       case UnknownFailure(reason) =>
         log.error(s"could not check ${cmd.desc} preconditions, proceeding anyway: ", reason)
         // If our checks fail, we don't want it to prevent us from trying to publish our htlc transactions.

eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala

@@ -1278,6 +1278,37 @@ class ReplaceableTxPublisherSpec extends TestKitBaseClass with AnyFunSuiteLike w
     }
   }
 
+  test("htlc success tx double-spent by claim-htlc-timeout") {
+    withFixture(Seq(500 millibtc), ChannelTypes.AnchorOutputsZeroFeeHtlcTx()) { f =>
+      import f._
+
+      val feerate = FeeratePerKw(15_000 sat)
+      setFeerate(feerate, fastest = feerate)
+      val (commitTx, htlcSuccess, _) = closeChannelWithHtlcs(f, aliceBlockHeight() + 144)
+      // We reach the HTLC timeout before publishing our HTLC-success transaction.
+      generateBlocks(144)
+      system.eventStream.publish(CurrentBlockHeight(currentBlockHeight(probe)))
+
+      // Bob detects Alice's commit tx and publishes his claim-htlc-timeout, which gets confirmed.
+      probe.send(bob, WatchFundingSpentTriggered(commitTx))
+      bob2blockchain.expectMsgType[PublishReplaceableTx] // claim anchor
+      bob2blockchain.expectMsgType[PublishFinalTx] // claim main output
+      val claimHtlcTimeout = bob2blockchain.expectMsgType[PublishReplaceableTx] // claim-htlc-timeout
+      assert(claimHtlcTimeout.txInfo.isInstanceOf[ClaimHtlcTimeoutTx])
+      wallet.publishTransaction(claimHtlcTimeout.txInfo.tx).pipeTo(probe.ref)
+      probe.expectMsg(claimHtlcTimeout.txInfo.tx.txid)
+      generateBlocks(1)
+
+      // When Alice tries to publish her HTLC-success, it is immediately aborted.
+      val htlcSuccessPublisher = createPublisher()
+      htlcSuccessPublisher ! Publish(probe.ref, htlcSuccess)
+      val result = probe.expectMsgType[TxRejected]
+      assert(result.cmd == htlcSuccess)
+      assert(result.reason == ConflictingTxConfirmed)
+      htlcSuccessPublisher ! Stop
+    }
+  }
+
   test("htlc success tx confirmation target reached, increasing fees") {
     withFixture(Seq(50 millibtc), ChannelTypes.AnchorOutputsZeroFeeHtlcTx()) { f =>
       import f._