From 5f4f72031f1b6c8fca7415b1fd09398dd656662d Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Fri, 12 Aug 2022 11:56:58 +0200 Subject: [PATCH] Remove legacy force-close htlc matching (#2376) Before eclair v0.6.0, we didn't store a mapping between htlc_id and htlc txs, which made it tricky to correctly remove identical htlcs that used MPP during force-close, when an htlc tx was confirmed. We have added that mapping since then and released it more than one year ago, so we can now safely remove that code. --- .../fr/acinq/eclair/channel/Helpers.scala | 107 ++++-------------- .../fr/acinq/eclair/channel/HelpersSpec.scala | 66 ++++------- 2 files changed, 43 insertions(+), 130 deletions(-) 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 1da9e1906e..dd7861017e 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 @@ -1196,41 +1196,6 @@ object Helpers { }).map(_.witness).collect(Scripts.extractPreimageFromClaimHtlcSuccess).nonEmpty } - /** - * Before eclair v0.6.0, we didn't store the mapping between htlc txs and the htlc id. - * This function is only used for channels that were closing before upgrading to eclair v0.6.0 (released in may 2021). - * TODO: remove once we're confident all eclair nodes on the network have been upgraded. - * - * We may have multiple HTLCs with the same payment hash because of MPP. - * When a timeout transaction is confirmed, we need to find the best matching HTLC to fail upstream. - * We need to handle potentially duplicate HTLCs (same amount and expiry): this function will use a deterministic - * ordering of transactions and HTLCs to handle this. - */ - private def findTimedOutHtlc(tx: Transaction, paymentHash160: ByteVector, htlcs: Seq[UpdateAddHtlc], timeoutTxs: Seq[Transaction], extractPaymentHash: PartialFunction[ScriptWitness, ByteVector])(implicit log: LoggingAdapter): Option[UpdateAddHtlc] = { - // We use a deterministic ordering to match HTLCs to their corresponding timeout tx. - // We don't match on the expected amounts because this is error-prone: computing the correct weight of a timeout tx - // is hard because signatures can be either 71, 72 or 73 bytes long (ECDSA DER encoding). - // It's simpler to just use the amount as the first ordering key: since the feerate is the same for all timeout - // transactions we will find the right HTLC to fail upstream. - val matchingHtlcs = htlcs - .filter(add => add.cltvExpiry.toLong == tx.lockTime && Crypto.ripemd160(add.paymentHash) == paymentHash160) - .sortBy(add => (add.amountMsat.toLong, add.id)) - val matchingTxs = timeoutTxs - .filter(timeoutTx => timeoutTx.lockTime == tx.lockTime && timeoutTx.txIn.map(_.witness).collect(extractPaymentHash).contains(paymentHash160)) - .sortBy(timeoutTx => (timeoutTx.txOut.map(_.amount.toLong).sum, timeoutTx.txIn.head.outPoint.index)) - if (matchingTxs.size != matchingHtlcs.size) { - log.error(s"some htlcs don't have a corresponding timeout transaction: tx=$tx, htlcs=$matchingHtlcs, timeout-txs=$matchingTxs") - } - matchingHtlcs.zip(matchingTxs).collectFirst { - // HTLC transactions cannot change when anchor outputs is not used, so we could just check that the txids match. - // But claim-htlc transactions can be updated to pay more or less fees by changing the output amount, so we cannot - // rely on txid equality for them. - // We instead check that the mempool transaction spends exactly the same inputs and sends the funds to exactly - // the same addresses as our transaction. - case (add, timeoutTx) if timeoutTx.txIn.map(_.outPoint).toSet == tx.txIn.map(_.outPoint).toSet && timeoutTx.txOut.map(_.publicKeyScript).toSet == tx.txOut.map(_.publicKeyScript).toSet => add - } - } - /** * In CLOSING state, when we are notified that a transaction has been confirmed, we analyze it to find out if one or * more htlcs have timed out and need to be failed in an upstream channel. Trimmed htlcs can be failed as soon as @@ -1246,30 +1211,18 @@ object Helpers { localCommit.spec.htlcs.collect(outgoing) -- untrimmedHtlcs } else { // maybe this is a timeout tx, in that case we can resolve and fail the corresponding htlc - val isMissingHtlcIndex = localCommitPublished.htlcTxs.values.collect { case Some(HtlcTimeoutTx(_, _, htlcId, _)) => htlcId }.toSet == Set(0) - if (isMissingHtlcIndex && commitmentFormat == DefaultCommitmentFormat) { - tx.txIn - .map(_.witness) - .collect(Scripts.extractPaymentHashFromHtlcTimeout) - .flatMap { paymentHash160 => - log.info(s"htlc-timeout tx for paymentHash160=${paymentHash160.toHex} expiry=${tx.lockTime} has been confirmed (tx=$tx)") - val timeoutTxs = localCommitPublished.htlcTxs.values.collect { case Some(HtlcTimeoutTx(_, tx, _, _)) => tx }.toSeq - findTimedOutHtlc(tx, paymentHash160, untrimmedHtlcs, timeoutTxs, Scripts.extractPaymentHashFromHtlcTimeout) - }.toSet - } else { - tx.txIn.flatMap(txIn => localCommitPublished.htlcTxs.get(txIn.outPoint) match { - case Some(Some(HtlcTimeoutTx(_, _, htlcId, _))) if isHtlcTimeout(tx, localCommitPublished) => - untrimmedHtlcs.find(_.id == htlcId) match { - case Some(htlc) => - log.info(s"htlc-timeout tx for htlc #$htlcId paymentHash=${htlc.paymentHash} expiry=${tx.lockTime} has been confirmed (tx=$tx)") - Some(htlc) - case None => - log.error(s"could not find htlc #$htlcId for htlc-timeout tx=$tx") - None - } - case _ => None - }).toSet - } + tx.txIn.flatMap(txIn => localCommitPublished.htlcTxs.get(txIn.outPoint) match { + case Some(Some(HtlcTimeoutTx(_, _, htlcId, _))) if isHtlcTimeout(tx, localCommitPublished) => + untrimmedHtlcs.find(_.id == htlcId) match { + case Some(htlc) => + log.info(s"htlc-timeout tx for htlc #$htlcId paymentHash=${htlc.paymentHash} expiry=${tx.lockTime} has been confirmed (tx=$tx)") + Some(htlc) + case None => + log.error(s"could not find htlc #$htlcId for htlc-timeout tx=$tx") + None + } + case _ => None + }).toSet } } @@ -1288,30 +1241,18 @@ object Helpers { remoteCommit.spec.htlcs.collect(incoming) -- untrimmedHtlcs } else { // maybe this is a timeout tx, in that case we can resolve and fail the corresponding htlc - val isMissingHtlcIndex = remoteCommitPublished.claimHtlcTxs.values.collect { case Some(ClaimHtlcTimeoutTx(_, _, htlcId, _)) => htlcId }.toSet == Set(0) - if (isMissingHtlcIndex && commitmentFormat == DefaultCommitmentFormat) { - tx.txIn - .map(_.witness) - .collect(Scripts.extractPaymentHashFromClaimHtlcTimeout) - .flatMap { paymentHash160 => - log.info(s"claim-htlc-timeout tx for paymentHash160=${paymentHash160.toHex} expiry=${tx.lockTime} has been confirmed (tx=$tx)") - val timeoutTxs = remoteCommitPublished.claimHtlcTxs.values.collect { case Some(ClaimHtlcTimeoutTx(_, tx, _, _)) => tx }.toSeq - findTimedOutHtlc(tx, paymentHash160, untrimmedHtlcs, timeoutTxs, Scripts.extractPaymentHashFromClaimHtlcTimeout) - }.toSet - } else { - tx.txIn.flatMap(txIn => remoteCommitPublished.claimHtlcTxs.get(txIn.outPoint) match { - case Some(Some(ClaimHtlcTimeoutTx(_, _, htlcId, _))) if isClaimHtlcTimeout(tx, remoteCommitPublished) => - untrimmedHtlcs.find(_.id == htlcId) match { - case Some(htlc) => - log.info(s"claim-htlc-timeout tx for htlc #$htlcId paymentHash=${htlc.paymentHash} expiry=${tx.lockTime} has been confirmed (tx=$tx)") - Some(htlc) - case None => - log.error(s"could not find htlc #$htlcId for claim-htlc-timeout tx=$tx") - None - } - case _ => None - }).toSet - } + tx.txIn.flatMap(txIn => remoteCommitPublished.claimHtlcTxs.get(txIn.outPoint) match { + case Some(Some(ClaimHtlcTimeoutTx(_, _, htlcId, _))) if isClaimHtlcTimeout(tx, remoteCommitPublished) => + untrimmedHtlcs.find(_.id == htlcId) match { + case Some(htlc) => + log.info(s"claim-htlc-timeout tx for htlc #$htlcId paymentHash=${htlc.paymentHash} expiry=${tx.lockTime} has been confirmed (tx=$tx)") + Some(htlc) + case None => + log.error(s"could not find htlc #$htlcId for claim-htlc-timeout tx=$tx") + None + } + case _ => None + }).toSet } } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/HelpersSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/HelpersSpec.scala index 28444b5118..c5c51257c3 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/HelpersSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/HelpersSpec.scala @@ -156,26 +156,7 @@ class HelpersSpec extends TestKitBaseClass with AnyFunSuiteLike with ChannelStat identifyHtlcs(setupHtlcs(Set(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs))) } - private def removeHtlcId(htlcTx: HtlcTx): HtlcTx = htlcTx match { - case htlcTx: HtlcSuccessTx => htlcTx.copy(htlcId = 0) - case htlcTx: HtlcTimeoutTx => htlcTx.copy(htlcId = 0) - } - - private def removeHtlcIds(htlcTxs: Map[OutPoint, Option[HtlcTx]]): Map[OutPoint, Option[HtlcTx]] = { - htlcTxs.map { case (outpoint, htlcTx_opt) => (outpoint, htlcTx_opt.map(removeHtlcId)) } - } - - private def removeHtlcId(claimHtlcTx: ClaimHtlcTx): ClaimHtlcTx = claimHtlcTx match { - case claimHtlcTx: LegacyClaimHtlcSuccessTx => claimHtlcTx.copy(htlcId = 0) - case claimHtlcTx: ClaimHtlcSuccessTx => claimHtlcTx.copy(htlcId = 0) - case claimHtlcTx: ClaimHtlcTimeoutTx => claimHtlcTx.copy(htlcId = 0) - } - - private def removeClaimHtlcIds(claimHtlcTxs: Map[OutPoint, Option[ClaimHtlcTx]]): Map[OutPoint, Option[ClaimHtlcTx]] = { - claimHtlcTxs.map { case (outpoint, claimHtlcTx_opt) => (outpoint, claimHtlcTx_opt.map(removeHtlcId)) } - } - - def findTimedOutHtlcs(f: Fixture, withoutHtlcId: Boolean): Unit = { + def findTimedOutHtlcs(f: Fixture): Unit = { import f._ val dustLimit = alice.underlyingActor.nodeParams.channelConf.dustLimit @@ -183,64 +164,55 @@ class HelpersSpec extends TestKitBaseClass with AnyFunSuiteLike with ChannelStat val localCommit = alice.stateData.asInstanceOf[DATA_CLOSING].commitments.localCommit val remoteCommit = bob.stateData.asInstanceOf[DATA_CLOSING].commitments.remoteCommit - // Channels without anchor outputs that were closing before eclair v0.6.0 will not have their htlcId set after the - // update, but still need to be able to identify timed out htlcs. - val localCommitPublished = if (withoutHtlcId) aliceCommitPublished.copy(htlcTxs = removeHtlcIds(aliceCommitPublished.htlcTxs)) else aliceCommitPublished - val remoteCommitPublished = if (withoutHtlcId) bobCommitPublished.copy(claimHtlcTxs = removeClaimHtlcIds(bobCommitPublished.claimHtlcTxs)) else bobCommitPublished - - val htlcTimeoutTxs = getHtlcTimeoutTxs(localCommitPublished) - val htlcSuccessTxs = getHtlcSuccessTxs(localCommitPublished) + val htlcTimeoutTxs = getHtlcTimeoutTxs(aliceCommitPublished) + val htlcSuccessTxs = getHtlcSuccessTxs(aliceCommitPublished) // Claim-HTLC txs can be modified to pay more (or less) fees by changing the output amount. - val claimHtlcTimeoutTxs = getClaimHtlcTimeoutTxs(remoteCommitPublished) + val claimHtlcTimeoutTxs = getClaimHtlcTimeoutTxs(bobCommitPublished) val claimHtlcTimeoutTxsModifiedFees = claimHtlcTimeoutTxs.map(tx => tx.modify(_.tx.txOut).setTo(Seq(tx.tx.txOut.head.copy(amount = 5000 sat)))) - val claimHtlcSuccessTxs = getClaimHtlcSuccessTxs(remoteCommitPublished) + val claimHtlcSuccessTxs = getClaimHtlcSuccessTxs(bobCommitPublished) val claimHtlcSuccessTxsModifiedFees = claimHtlcSuccessTxs.map(tx => tx.modify(_.tx.txOut).setTo(Seq(tx.tx.txOut.head.copy(amount = 5000 sat)))) val aliceTimedOutHtlcs = htlcTimeoutTxs.map(htlcTimeout => { - val timedOutHtlcs = Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, localCommitPublished, dustLimit, htlcTimeout.tx) + val timedOutHtlcs = Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, aliceCommitPublished, dustLimit, htlcTimeout.tx) assert(timedOutHtlcs.size == 1) timedOutHtlcs.head }) assert(aliceTimedOutHtlcs.toSet == aliceHtlcs) val bobTimedOutHtlcs = claimHtlcTimeoutTxs.map(claimHtlcTimeout => { - val timedOutHtlcs = Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, remoteCommitPublished, dustLimit, claimHtlcTimeout.tx) + val timedOutHtlcs = Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, bobCommitPublished, dustLimit, claimHtlcTimeout.tx) assert(timedOutHtlcs.size == 1) timedOutHtlcs.head }) assert(bobTimedOutHtlcs.toSet == bobHtlcs) val bobTimedOutHtlcs2 = claimHtlcTimeoutTxsModifiedFees.map(claimHtlcTimeout => { - val timedOutHtlcs = Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, remoteCommitPublished, dustLimit, claimHtlcTimeout.tx) + val timedOutHtlcs = Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, bobCommitPublished, dustLimit, claimHtlcTimeout.tx) assert(timedOutHtlcs.size == 1) timedOutHtlcs.head }) assert(bobTimedOutHtlcs2.toSet == bobHtlcs) - htlcSuccessTxs.foreach(htlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, localCommitPublished, dustLimit, htlcSuccess.tx).isEmpty)) - htlcSuccessTxs.foreach(htlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, remoteCommitPublished, dustLimit, htlcSuccess.tx).isEmpty)) - claimHtlcSuccessTxs.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, localCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) - claimHtlcSuccessTxsModifiedFees.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, localCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) - claimHtlcSuccessTxs.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, remoteCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) - claimHtlcSuccessTxsModifiedFees.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, remoteCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) - htlcTimeoutTxs.foreach(htlcTimeout => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, remoteCommitPublished, dustLimit, htlcTimeout.tx).isEmpty)) - claimHtlcTimeoutTxs.foreach(claimHtlcTimeout => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, localCommitPublished, dustLimit, claimHtlcTimeout.tx).isEmpty)) + htlcSuccessTxs.foreach(htlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, aliceCommitPublished, dustLimit, htlcSuccess.tx).isEmpty)) + htlcSuccessTxs.foreach(htlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, bobCommitPublished, dustLimit, htlcSuccess.tx).isEmpty)) + claimHtlcSuccessTxs.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, aliceCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) + claimHtlcSuccessTxsModifiedFees.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, aliceCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) + claimHtlcSuccessTxs.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, bobCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) + claimHtlcSuccessTxsModifiedFees.foreach(claimHtlcSuccess => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, bobCommitPublished, dustLimit, claimHtlcSuccess.tx).isEmpty)) + htlcTimeoutTxs.foreach(htlcTimeout => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, remoteCommit, bobCommitPublished, dustLimit, htlcTimeout.tx).isEmpty)) + claimHtlcTimeoutTxs.foreach(claimHtlcTimeout => assert(Closing.trimmedOrTimedOutHtlcs(commitmentFormat, localCommit, aliceCommitPublished, dustLimit, claimHtlcTimeout.tx).isEmpty)) } test("find timed out htlcs") { - findTimedOutHtlcs(setupHtlcs(), withoutHtlcId = false) - } - - test("find timed out htlcs (without htlc id)") { - findTimedOutHtlcs(setupHtlcs(), withoutHtlcId = true) + findTimedOutHtlcs(setupHtlcs()) } test("find timed out htlcs (anchor outputs)", Tag(ChannelStateTestsTags.AnchorOutputs)) { - findTimedOutHtlcs(setupHtlcs(Set(ChannelStateTestsTags.AnchorOutputs)), withoutHtlcId = false) + findTimedOutHtlcs(setupHtlcs(Set(ChannelStateTestsTags.AnchorOutputs))) } test("find timed out htlcs (anchor outputs zero fee htlc txs)", Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { - findTimedOutHtlcs(setupHtlcs(Set(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)), withoutHtlcId = false) + findTimedOutHtlcs(setupHtlcs(Set(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs))) } test("check closing tx amounts above dust") {