Skip to content

fixup! Use CLTV as tie-breaker for offered htlc output sorting (#790) #1360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pm47 merged 11 commits into from
Mar 31, 2020
Merged

fixup! Use CLTV as tie-breaker for offered htlc output sorting (#790) #1360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
617e2e6
fixup! Use CLTV as tie-breaker for offered htlc output sorting (#790)
pm47 Mar 30, 2020
68b3cbf
removed the `Direction` class
pm47 Mar 31, 2020
1c05a26
improved the non-reg test for htlcs
pm47 Mar 31, 2020
988d2ac
addressed review comments
pm47 Mar 31, 2020
7809e58
added comment on remainder
pm47 Mar 31, 2020
4b1a70b
refactored helper methods
pm47 Mar 31, 2020
fa2a134
nits in Helpers
pm47 Mar 31, 2020
9139e12
Update eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsS…
pm47 Mar 31, 2020
a4484e6
more nit in Helpers
pm47 Mar 31, 2020
33977e5
switched fulfillIncomingHtlc/fulfillOutgointHtlc
pm47 Mar 31, 2020
b5f3a1d
fixup! switched fulfillIncomingHtlc/fulfillOutgointHtlc
pm47 Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 38 additions & 26 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import fr.acinq.bitcoin.{ByteVector32, ByteVector64, Crypto}
import fr.acinq.eclair.blockchain.fee.{FeeEstimator, FeeTargets}
import fr.acinq.eclair.crypto.{Generators, KeyManager, ShaChain, Sphinx}
import fr.acinq.eclair.payment.relay.{Origin, Relayer}
import fr.acinq.eclair.transactions.DirectedHtlc._
import fr.acinq.eclair.transactions.Transactions._
import fr.acinq.eclair.transactions._
import fr.acinq.eclair.wire._
Expand Down Expand Up @@ -63,10 +64,13 @@ case class Commitments(channelVersion: ChannelVersion,

def hasNoPendingHtlcs: Boolean = localCommit.spec.htlcs.isEmpty && remoteCommit.spec.htlcs.isEmpty && remoteNextCommitInfo.isRight

def timedOutOutgoingHtlcs(blockheight: Long): Set[UpdateAddHtlc] =
(localCommit.spec.htlcs.filter(htlc => htlc.direction == OUT && blockheight >= htlc.add.cltvExpiry.toLong) ++
remoteCommit.spec.htlcs.filter(htlc => htlc.direction == IN && blockheight >= htlc.add.cltvExpiry.toLong) ++
remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit.spec.htlcs.filter(htlc => htlc.direction == IN && blockheight >= htlc.add.cltvExpiry.toLong)).getOrElse(Set.empty[DirectedHtlc])).map(_.add)
def timedOutOutgoingHtlcs(blockheight: Long): Set[UpdateAddHtlc] = {
def expired(add: UpdateAddHtlc) = blockheight >= add.cltvExpiry.toLong

localCommit.spec.htlcs.collect(outgoing).filter(expired) ++
remoteCommit.spec.htlcs.collect(incoming).filter(expired) ++
remoteNextCommitInfo.left.toSeq.flatMap(_.nextRemoteCommit.spec.htlcs.collect(incoming).filter(expired).toSet)
}

/**
* HTLCs that are close to timing out upstream are potentially dangerous. If we received the pre-image for those
Expand All @@ -75,9 +79,9 @@ case class Commitments(channelVersion: ChannelVersion,
* and our HTLC success in case of a force-close.
*/
def almostTimedOutIncomingHtlcs(blockheight: Long, fulfillSafety: CltvExpiryDelta): Set[UpdateAddHtlc] = {
localCommit.spec.htlcs.collect {
case htlc if htlc.direction == IN && blockheight >= (htlc.add.cltvExpiry - fulfillSafety).toLong => htlc.add
}
def nearlyExpired(add: UpdateAddHtlc) = blockheight >= (add.cltvExpiry - fulfillSafety).toLong

localCommit.spec.htlcs.collect(incoming).filter(nearlyExpired)
}

def addLocalProposal(proposal: UpdateMessage): Commitments = Commitments.addLocalProposal(this, proposal)
Expand Down Expand Up @@ -187,7 +191,7 @@ object Commitments {
val remoteCommit1 = commitments1.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit).getOrElse(commitments1.remoteCommit)
val reduced = CommitmentSpec.reduce(remoteCommit1.spec, commitments1.remoteChanges.acked, commitments1.localChanges.proposed)
// the HTLC we are about to create is outgoing, but from their point of view it is incoming
val outgoingHtlcs = reduced.htlcs.filter(_.direction == IN)
val outgoingHtlcs = reduced.htlcs.collect(incoming)

// note that the funder pays the fee, so if sender != funder, both sides will have to afford this payment
val fees = commitTxFee(commitments1.remoteParams.dustLimit, reduced)
Expand All @@ -207,7 +211,7 @@ object Commitments {
}

// NB: we need the `toSeq` because otherwise duplicate amountMsat would be removed (since outgoingHtlcs is a Set).
val htlcValueInFlight = outgoingHtlcs.toSeq.map(_.add.amountMsat).sum
val htlcValueInFlight = outgoingHtlcs.toSeq.map(_.amountMsat).sum
if (commitments1.remoteParams.maxHtlcValueInFlightMsat < htlcValueInFlight) {
// TODO: this should be a specific UPDATE error
return Failure(HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.remoteParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight))
Expand All @@ -232,7 +236,7 @@ object Commitments {
// let's compute the current commitment *as seen by us* including this change
val commitments1 = addRemoteProposal(commitments, add).copy(remoteNextHtlcId = commitments.remoteNextHtlcId + 1)
val reduced = CommitmentSpec.reduce(commitments1.localCommit.spec, commitments1.localChanges.acked, commitments1.remoteChanges.proposed)
val incomingHtlcs = reduced.htlcs.filter(_.direction == IN)
val incomingHtlcs = reduced.htlcs.collect(incoming)

// note that the funder pays the fee, so if sender != funder, both sides will have to afford this payment
val fees = commitTxFee(commitments1.remoteParams.dustLimit, reduced)
Expand All @@ -251,7 +255,7 @@ object Commitments {
}

// NB: we need the `toSeq` because otherwise duplicate amountMsat would be removed (since incomingHtlcs is a Set).
val htlcValueInFlight = incomingHtlcs.toSeq.map(_.add.amountMsat).sum
val htlcValueInFlight = incomingHtlcs.toSeq.map(_.amountMsat).sum
if (commitments1.localParams.maxHtlcValueInFlightMsat < htlcValueInFlight) {
throw HtlcValueTooHighInFlight(commitments.channelId, maximum = commitments1.localParams.maxHtlcValueInFlightMsat, actual = htlcValueInFlight)
}
Expand All @@ -263,16 +267,24 @@ object Commitments {
commitments1
}

def getHtlcCrossSigned(commitments: Commitments, directionRelativeToLocal: Direction, htlcId: Long): Option[UpdateAddHtlc] = for {
localSigned <- commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit).getOrElse(commitments.remoteCommit).spec.findHtlcById(htlcId, directionRelativeToLocal.opposite)
remoteSigned <- commitments.localCommit.spec.findHtlcById(htlcId, directionRelativeToLocal)
def getOutgoingHtlcCrossSigned(commitments: Commitments, htlcId: Long): Option[UpdateAddHtlc] = for {
localSigned <- commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit).getOrElse(commitments.remoteCommit).spec.findIncomingHtlcById(htlcId)
remoteSigned <- commitments.localCommit.spec.findOutgoingHtlcById(htlcId)
} yield {
require(localSigned.add == remoteSigned.add)
localSigned.add
}

def getIncomingHtlcCrossSigned(commitments: Commitments, htlcId: Long): Option[UpdateAddHtlc] = for {
localSigned <- commitments.remoteNextCommitInfo.left.toOption.map(_.nextRemoteCommit).getOrElse(commitments.remoteCommit).spec.findOutgoingHtlcById(htlcId)
remoteSigned <- commitments.localCommit.spec.findIncomingHtlcById(htlcId)
} yield {
require(localSigned.add == remoteSigned.add)
localSigned.add
}

def sendFulfill(commitments: Commitments, cmd: CMD_FULFILL_HTLC): Try[(Commitments, UpdateFulfillHtlc)] =
getHtlcCrossSigned(commitments, IN, cmd.id) match {
getIncomingHtlcCrossSigned(commitments, cmd.id) match {
case Some(htlc) if alreadyProposed(commitments.localChanges.proposed, htlc.id) =>
// we have already sent a fail/fulfill for this htlc
Failure(UnknownHtlcId(commitments.channelId, cmd.id))
Expand All @@ -285,14 +297,14 @@ object Commitments {
}

def receiveFulfill(commitments: Commitments, fulfill: UpdateFulfillHtlc): Try[(Commitments, Origin, UpdateAddHtlc)] =
getHtlcCrossSigned(commitments, OUT, fulfill.id) match {
getOutgoingHtlcCrossSigned(commitments, fulfill.id) match {
case Some(htlc) if htlc.paymentHash == sha256(fulfill.paymentPreimage) => Try((addRemoteProposal(commitments, fulfill), commitments.originChannels(fulfill.id), htlc))
case Some(_) => Failure(InvalidHtlcPreimage(commitments.channelId, fulfill.id))
case None => Failure(UnknownHtlcId(commitments.channelId, fulfill.id))
}

def sendFail(commitments: Commitments, cmd: CMD_FAIL_HTLC, nodeSecret: PrivateKey): Try[(Commitments, UpdateFailHtlc)] =
getHtlcCrossSigned(commitments, IN, cmd.id) match {
getIncomingHtlcCrossSigned(commitments, cmd.id) match {
case Some(htlc) if alreadyProposed(commitments.localChanges.proposed, htlc.id) =>
// we have already sent a fail/fulfill for this htlc
Failure(UnknownHtlcId(commitments.channelId, cmd.id))
Expand All @@ -317,7 +329,7 @@ object Commitments {
if ((cmd.failureCode & FailureMessageCodecs.BADONION) == 0) {
Failure(InvalidFailureCode(commitments.channelId))
} else {
getHtlcCrossSigned(commitments, IN, cmd.id) match {
getIncomingHtlcCrossSigned(commitments, cmd.id) match {
case Some(htlc) if alreadyProposed(commitments.localChanges.proposed, htlc.id) =>
// we have already sent a fail/fulfill for this htlc
Failure(UnknownHtlcId(commitments.channelId, cmd.id))
Expand All @@ -331,7 +343,7 @@ object Commitments {
}

def receiveFail(commitments: Commitments, fail: UpdateFailHtlc): Try[(Commitments, Origin, UpdateAddHtlc)] =
getHtlcCrossSigned(commitments, OUT, fail.id) match {
getOutgoingHtlcCrossSigned(commitments, fail.id) match {
case Some(htlc) => Try((addRemoteProposal(commitments, fail), commitments.originChannels(fail.id), htlc))
case None => Failure(UnknownHtlcId(commitments.channelId, fail.id))
}
Expand All @@ -341,7 +353,7 @@ object Commitments {
if ((fail.failureCode & FailureMessageCodecs.BADONION) == 0) {
Failure(InvalidFailureCode(commitments.channelId))
} else {
getHtlcCrossSigned(commitments, OUT, fail.id) match {
getOutgoingHtlcCrossSigned(commitments, fail.id) match {
case Some(htlc) => Try((addRemoteProposal(commitments, fail), commitments.originChannels(fail.id), htlc))
case None => Failure(UnknownHtlcId(commitments.channelId, fail.id))
}
Expand Down Expand Up @@ -396,7 +408,7 @@ object Commitments {
if (missing < 0.sat) {
Failure(CannotAffordFees(commitments.channelId, missing = -missing, reserve = commitments1.localParams.channelReserve, fees = fees))
} else {
Success(commitments1)
Success(commitments1)
}
}
}
Expand Down Expand Up @@ -430,7 +442,7 @@ object Commitments {
val htlcSigs = sortedHtlcTxs.map(keyManager.sign(_, keyManager.htlcPoint(channelKeyPath), remoteNextPerCommitmentPoint))

// NB: IN/OUT htlcs are inverted because this is the remote commit
log.info(s"built remote commit number=${remoteCommit.index + 1} toLocalMsat=${spec.toLocal.toLong} toRemoteMsat=${spec.toRemote.toLong} htlc_in={} htlc_out={} feeratePerKw=${spec.feeratePerKw} txid=${remoteCommitTx.tx.txid} tx={}", spec.htlcs.filter(_.direction == OUT).map(_.add.id).mkString(","), spec.htlcs.filter(_.direction == IN).map(_.add.id).mkString(","), remoteCommitTx.tx)
log.info(s"built remote commit number=${remoteCommit.index + 1} toLocalMsat=${spec.toLocal.toLong} toRemoteMsat=${spec.toRemote.toLong} htlc_in={} htlc_out={} feeratePerKw=${spec.feeratePerKw} txid=${remoteCommitTx.tx.txid} tx={}", spec.htlcs.collect(outgoing).map(_.id).mkString(","), spec.htlcs.collect(incoming).map(_.id).mkString(","), remoteCommitTx.tx)

// don't sign if they don't get paid
val commitSig = CommitSig(
Expand Down Expand Up @@ -477,7 +489,7 @@ object Commitments {
val (localCommitTx, htlcTimeoutTxs, htlcSuccessTxs) = makeLocalTxs(keyManager, channelVersion, localCommit.index + 1, localParams, remoteParams, commitInput, localPerCommitmentPoint, spec)
val sig = keyManager.sign(localCommitTx, keyManager.fundingPublicKey(commitments.localParams.fundingKeyPath))

log.info(s"built local commit number=${localCommit.index + 1} toLocalMsat=${spec.toLocal.toLong} toRemoteMsat=${spec.toRemote.toLong} htlc_in={} htlc_out={} feeratePerKw=${spec.feeratePerKw} txid=${localCommitTx.tx.txid} tx={}", spec.htlcs.filter(_.direction == IN).map(_.add.id).mkString(","), spec.htlcs.filter(_.direction == OUT).map(_.add.id).mkString(","), localCommitTx.tx)
log.info(s"built local commit number=${localCommit.index + 1} toLocalMsat=${spec.toLocal.toLong} toRemoteMsat=${spec.toRemote.toLong} htlc_in={} htlc_out={} feeratePerKw=${spec.feeratePerKw} txid=${localCommitTx.tx.txid} tx={}", spec.htlcs.collect(incoming).map(_.id).mkString(","), spec.htlcs.collect(outgoing).map(_.id).mkString(","), localCommitTx.tx)

// TODO: should we have optional sig? (original comment: this tx will NOT be signed if our output is empty)

Expand Down Expand Up @@ -543,18 +555,18 @@ object Commitments {
// same for fails: we need to make sure that they are in neither commitment before propagating the fail upstream
case fail: UpdateFailHtlc =>
val origin = commitments.originChannels(fail.id)
val add = commitments.remoteCommit.spec.findHtlcById(fail.id, IN).map(_.add).get
val add = commitments.remoteCommit.spec.findIncomingHtlcById(fail.id).map(_.add).get
Relayer.ForwardRemoteFail(fail, origin, add)
// same as above
case fail: UpdateFailMalformedHtlc =>
val origin = commitments.originChannels(fail.id)
val add = commitments.remoteCommit.spec.findHtlcById(fail.id, IN).map(_.add).get
val add = commitments.remoteCommit.spec.findIncomingHtlcById(fail.id).map(_.add).get
Relayer.ForwardRemoteFailMalformed(fail, origin, add)
}
// the outgoing following htlcs have been completed (fulfilled or failed) when we received this revocation
// they have been removed from both local and remote commitment
// (since fulfill/fail are sent by remote, they are (1) signed by them, (2) revoked by us, (3) signed by us, (4) revoked by them
val completedOutgoingHtlcs = commitments.remoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add.id) -- theirNextCommit.spec.htlcs.filter(_.direction == IN).map(_.add.id)
val completedOutgoingHtlcs = commitments.remoteCommit.spec.htlcs.collect(incoming).map(_.id) -- theirNextCommit.spec.htlcs.collect(incoming).map(_.id)
// we remove the newly completed htlcs from the origin map
val originChannels1 = commitments.originChannels -- completedOutgoingHtlcs
val commitments1 = commitments.copy(
Expand Down
26 changes: 13 additions & 13 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import fr.acinq.eclair.blockchain.fee.{FeeEstimator, FeeTargets}
import fr.acinq.eclair.channel.Channel.REFRESH_CHANNEL_UPDATE_INTERVAL
import fr.acinq.eclair.crypto.{Generators, KeyManager}
import fr.acinq.eclair.db.ChannelsDb
import fr.acinq.eclair.transactions.DirectedHtlc._
import fr.acinq.eclair.transactions.Scripts._
import fr.acinq.eclair.transactions.Transactions._
import fr.acinq.eclair.transactions._
Expand Down Expand Up @@ -835,10 +836,9 @@ object Helpers {
// if an outgoing htlc is in the remote commitment, then:
// - either it is in the local commitment (it was never fulfilled)
// - or we have already received the fulfill and forwarded it upstream
val outgoingHtlcs = localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
outgoingHtlcs.collect {
case add if add.paymentHash == sha256(paymentPreimage) => (add, paymentPreimage)
}
localCommit.spec.htlcs.collect {
case OutgoingHtlc(add) if add.paymentHash == sha256(paymentPreimage) => (add, paymentPreimage)
}
}
}

Expand All @@ -864,14 +864,14 @@ object Helpers {
def timedoutHtlcs(localCommit: LocalCommit, localDustLimit: Satoshi, tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] =
if (tx.txid == localCommit.publishableTxs.commitTx.tx.txid) {
// the tx is a commitment tx, we can immediately fail all dust htlcs (they don't have an output in the tx)
(localCommit.spec.htlcs.filter(_.direction == OUT) -- Transactions.trimOfferedHtlcs(localDustLimit, localCommit.spec)).map(_.add)
(localCommit.spec.htlcs.collect(outgoing) -- Transactions.trimOfferedHtlcs(localDustLimit, localCommit.spec).map(_.add))
} else {
// maybe this is a timeout tx, in that case we can resolve and fail the corresponding htlc
tx.txIn.map(_.witness match {
case ScriptWitness(Seq(ByteVector.empty, remoteSig, localSig, ByteVector.empty, htlcOfferedScript)) =>
val paymentHash160 = htlcOfferedScript.slice(109, 109 + 20)
log.info(s"extracted paymentHash160=$paymentHash160 from tx=$tx (htlc-timeout)")
localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add).filter(add => ripemd160(add.paymentHash) == paymentHash160)
localCommit.spec.htlcs.collect(outgoing).filter(add => ripemd160(add.paymentHash) == paymentHash160)
case _ => Set.empty
}).toSet.flatten
}
Expand All @@ -886,14 +886,14 @@ object Helpers {
def timedoutHtlcs(remoteCommit: RemoteCommit, remoteDustLimit: Satoshi, tx: Transaction)(implicit log: LoggingAdapter): Set[UpdateAddHtlc] =
if (tx.txid == remoteCommit.txid) {
// the tx is a commitment tx, we can immediately fail all dust htlcs (they don't have an output in the tx)
(remoteCommit.spec.htlcs.filter(_.direction == IN) -- Transactions.trimReceivedHtlcs(remoteDustLimit, remoteCommit.spec)).map(_.add)
(remoteCommit.spec.htlcs.collect(incoming) -- Transactions.trimReceivedHtlcs(remoteDustLimit, remoteCommit.spec).map(_.add))
} else {
// maybe this is a timeout tx, in that case we can resolve and fail the corresponding htlc
tx.txIn.map(_.witness match {
case ScriptWitness(Seq(remoteSig, ByteVector.empty, htlcReceivedScript)) =>
val paymentHash160 = htlcReceivedScript.slice(69, 69 + 20)
log.info(s"extracted paymentHash160=$paymentHash160 from tx=$tx (claim-htlc-timeout)")
remoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add).filter(add => ripemd160(add.paymentHash) == paymentHash160)
remoteCommit.spec.htlcs.collect(incoming).filter { add => ripemd160(add.paymentHash) == paymentHash160 }
case _ => Set.empty
}).toSet.flatten
}
Expand All @@ -906,11 +906,11 @@ object Helpers {
*/
def onchainOutgoingHtlcs(localCommit: LocalCommit, remoteCommit: RemoteCommit, nextRemoteCommit_opt: Option[RemoteCommit], tx: Transaction): Set[UpdateAddHtlc] = {
if (localCommit.publishableTxs.commitTx.tx.txid == tx.txid) {
localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
localCommit.spec.htlcs.collect(outgoing)
} else if (remoteCommit.txid == tx.txid) {
remoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add)
remoteCommit.spec.htlcs.collect(incoming)
} else if (nextRemoteCommit_opt.map(_.txid).contains(tx.txid)) {
nextRemoteCommit_opt.get.spec.htlcs.filter(_.direction == IN).map(_.add)
nextRemoteCommit_opt.get.spec.htlcs.collect(incoming)
} else {
Set.empty
}
Expand All @@ -930,14 +930,14 @@ object Helpers {
// our commit got confirmed, so any htlc that we signed but they didn't sign will never reach the chain
val mostRecentRemoteCommit = nextRemoteCommit_opt.getOrElse(remoteCommit)
// NB: from the p.o.v of remote, their incoming htlcs are our outgoing htlcs
mostRecentRemoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add) -- localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
mostRecentRemoteCommit.spec.htlcs.collect(incoming) -- localCommit.spec.htlcs.collect(outgoing)
} else if (remoteCommit.txid == tx.txid) {
// their commit got confirmed
nextRemoteCommit_opt match {
case Some(nextRemoteCommit) =>
// we had signed a new commitment but they committed the previous one
// any htlc that we signed in the new commitment that they didn't sign will never reach the chain
nextRemoteCommit.spec.htlcs.filter(_.direction == IN).map(_.add) -- localCommit.spec.htlcs.filter(_.direction == OUT).map(_.add)
nextRemoteCommit.spec.htlcs.collect(incoming) -- localCommit.spec.htlcs.collect(outgoing)
case None =>
// their last commitment got confirmed, so no htlcs will be overridden, they will timeout or be fulfilled on chain
Set.empty
Expand Down
Loading