From f47acd0fb4f430a5dd52f307e6008e6b1740715b Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Tue, 3 Dec 2024 15:49:37 +0100 Subject: [PATCH] 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. --- .../bitcoind/rpc/BitcoinCoreClient.scala | 2 +- .../fr/acinq/eclair/channel/Helpers.scala | 3 +- .../publish/ReplaceableTxPrePublisher.scala | 57 +++++++++++++++---- .../publish/ReplaceableTxPublisherSpec.scala | 31 ++++++++++ 4 files changed, 79 insertions(+), 14 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala index f6a8daa965..721e0179e3 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala +++ b/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 diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala index 033899eeb5..78db6d35d0 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala +++ b/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 diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPrePublisher.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPrePublisher.scala index 7847682c6d..a971cf7970 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPrePublisher.scala +++ b/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. diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala index e1d3d3f1e4..84e161dfd5 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala +++ b/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._