Skip to content

Commit

Permalink
Remove legacy force-close htlc matching (#2376)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@t-bast
t-bast authored Aug 12, 2022
1 parent a13c3d5 commit 5f4f720
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 130 deletions.
107 changes: 24 additions & 83 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
}

Expand All @@ -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
}
}

Expand Down
66 changes: 19 additions & 47 deletions eclair-core/src/test/scala/fr/acinq/eclair/channel/HelpersSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,91 +156,63 @@ 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
val commitmentFormat = alice.stateData.asInstanceOf[DATA_CLOSING].commitments.commitmentFormat
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") {
Expand Down

0 comments on commit 5f4f720

Please sign in to comment.