Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@t-bast
t-bast committed Aug 31, 2021
1 parent cf0ed1b commit b6a6a73
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 22 deletions.
43 changes: 28 additions & 15 deletions eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1251,8 +1251,13 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
case Event(c: CMD_CLOSE, d: DATA_SHUTDOWN) =>
c.feerates match {
case Some(feerates) if c.feerates != d.closingFeerates =>
log.info("updating our closing feerates: {}", feerates)
handleCommandSuccess(c, d.copy(closingFeerates = c.feerates)) storing()
if (c.scriptPubKey.nonEmpty && !c.scriptPubKey.contains(d.localShutdown.scriptPubKey)) {
log.warning("cannot update closing script when closing is already in progress")
handleCommandError(ClosingAlreadyInProgress(d.channelId), c)
} else {
log.info("updating our closing feerates: {}", feerates)
handleCommandSuccess(c, d.copy(closingFeerates = c.feerates)) storing()
}
case _ =>
handleCommandError(ClosingAlreadyInProgress(d.channelId), c)
}
Expand All @@ -1270,8 +1275,9 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
}
stay()

case Event(c@ClosingSigned(_, remoteClosingFee, remoteSig, _), d: DATA_NEGOTIATING) =>
log.info("received closing fees={}", remoteClosingFee)
case Event(c: ClosingSigned, d: DATA_NEGOTIATING) =>
log.info("received closing fee={}", c.feeSatoshis)
val (remoteClosingFee, remoteSig) = (c.feeSatoshis, c.signature)
Closing.checkClosingSignature(keyManager, d.commitments, d.localShutdown.scriptPubKey, d.remoteShutdown.scriptPubKey, remoteClosingFee, remoteSig) match {
case Right((signedClosingTx, closingSignedRemoteFees)) =>
val lastLocalClosingSigned_opt = d.closingTxProposed.last.lastOption
Expand All @@ -1284,6 +1290,8 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
handleMutualClose(signedClosingTx, Left(d.copy(bestUnpublishedClosingTx_opt = Some(signedClosingTx)))) sending closingSignedRemoteFees
} else if (lastLocalClosingSigned_opt.flatMap(_.localClosingSigned.feeRange_opt).exists(r => r.min <= remoteClosingFee && remoteClosingFee <= r.max)) {
// they chose a fee inside our proposed fee range, so we close and send a closing_signed for that fee
val localFeeRange = lastLocalClosingSigned_opt.flatMap(_.localClosingSigned.feeRange_opt).get
log.info("they chose a closing fee={} within our fee range (min={} max={})", remoteClosingFee, localFeeRange.min, localFeeRange.max)
handleMutualClose(signedClosingTx, Left(d.copy(bestUnpublishedClosingTx_opt = Some(signedClosingTx)))) sending closingSignedRemoteFees
} else if (d.commitments.localCommit.spec.toLocal == 0.msat) {
// we have nothing at stake so there is no need to negotiate, we accept their fee right away
Expand All @@ -1294,7 +1302,7 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
// if we are fundee and they proposed a fee range, we pick a value in that range and they should accept it without further negotiation
// we don't care much about the closing fee since they're paying it (not us) and we can use CPFP if we want to speed up confirmation
// but we need to ensure it's high enough to at least propagate across the network
val txPropagationFeerate = nodeParams.onChainFeeConf.feeEstimator.getFeeratePerKw(1008) * 2
val txPropagationFeerate = nodeParams.onChainFeeConf.feeEstimator.getFeeratePerKw(1008)
val txPropagationMinFee = Transactions.weight2fee(txPropagationFeerate, signedClosingTx.tx.weight())
if (maxFee < txPropagationMinFee) {
log.warning("their highest closing fee is below our tx propagation threshold (feerate={}): {} < {}", txPropagationFeerate, maxFee, txPropagationMinFee)
Expand All @@ -1307,11 +1315,11 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
case ClosingFees(preferred, _, _) => preferred
}
if (closingFee == remoteClosingFee) {
log.info("accepting their closing fees={}", remoteClosingFee)
log.info("accepting their closing fee={}", remoteClosingFee)
handleMutualClose(signedClosingTx, Left(d.copy(bestUnpublishedClosingTx_opt = Some(signedClosingTx)))) sending closingSignedRemoteFees
} else {
val (closingTx, closingSigned) = Closing.makeClosingTx(keyManager, d.commitments, d.localShutdown.scriptPubKey, d.remoteShutdown.scriptPubKey, ClosingFees(closingFee, minFee, maxFee))
log.info("proposing closing fees={} in their fee range (min={} max={})", closingSigned.feeSatoshis, minFee, maxFee)
log.info("proposing closing fee={} in their fee range (min={} max={})", closingSigned.feeSatoshis, minFee, maxFee)
val closingTxProposed1 = (d.closingTxProposed: @unchecked) match {
case previousNegotiations :+ currentNegotiation => previousNegotiations :+ (currentNegotiation :+ ClosingTxProposed(closingTx, closingSigned))
}
Expand All @@ -1334,10 +1342,10 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
handleMutualClose(signedClosingTx, Left(d.copy(bestUnpublishedClosingTx_opt = Some(signedClosingTx))))
} else if (closingSigned.feeSatoshis == remoteClosingFee) {
// we have converged!
log.info("accepting their closing fees={}", remoteClosingFee)
log.info("accepting their closing fee={}", remoteClosingFee)
handleMutualClose(signedClosingTx, Left(d.copy(closingTxProposed = closingTxProposed1, bestUnpublishedClosingTx_opt = Some(signedClosingTx)))) sending closingSigned
} else {
log.info("proposing closing fees={}", closingSigned.feeSatoshis)
log.info("proposing closing fee={}", closingSigned.feeSatoshis)
stay() using d.copy(closingTxProposed = closingTxProposed1, bestUnpublishedClosingTx_opt = Some(signedClosingTx)) storing() sending closingSigned
}
}
Expand All @@ -1358,13 +1366,18 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId
case Event(c: CMD_CLOSE, d: DATA_NEGOTIATING) =>
c.feerates match {
case Some(feerates) =>
log.info("updating our closing feerates: {}", feerates)
val (closingTx, closingSigned) = Closing.makeFirstClosingTx(keyManager, d.commitments, d.localShutdown.scriptPubKey, d.remoteShutdown.scriptPubKey, nodeParams.onChainFeeConf.feeEstimator, nodeParams.onChainFeeConf.feeTargets, Some(feerates))
val closingTxProposed1 = d.closingTxProposed match {
case previousNegotiations :+ currentNegotiation => previousNegotiations :+ (currentNegotiation :+ ClosingTxProposed(closingTx, closingSigned))
case previousNegotiations => previousNegotiations :+ List(ClosingTxProposed(closingTx, closingSigned))
if (c.scriptPubKey.nonEmpty && !c.scriptPubKey.contains(d.localShutdown.scriptPubKey)) {
log.warning("cannot update closing script when closing is already in progress")
handleCommandError(ClosingAlreadyInProgress(d.channelId), c)
} else {
log.info("updating our closing feerates: {}", feerates)
val (closingTx, closingSigned) = Closing.makeFirstClosingTx(keyManager, d.commitments, d.localShutdown.scriptPubKey, d.remoteShutdown.scriptPubKey, nodeParams.onChainFeeConf.feeEstimator, nodeParams.onChainFeeConf.feeTargets, Some(feerates))
val closingTxProposed1 = d.closingTxProposed match {
case previousNegotiations :+ currentNegotiation => previousNegotiations :+ (currentNegotiation :+ ClosingTxProposed(closingTx, closingSigned))
case previousNegotiations => previousNegotiations :+ List(ClosingTxProposed(closingTx, closingSigned))
}
handleCommandSuccess(c, d.copy(closingTxProposed = closingTxProposed1)) storing() sending closingSigned
}
handleCommandSuccess(c, d.copy(closingTxProposed = closingTxProposed1)) storing() sending closingSigned
case _ =>
handleCommandError(ClosingAlreadyInProgress(d.channelId), c)
}
Expand Down
6 changes: 3 additions & 3 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 @@ -498,12 +498,12 @@ object Helpers {
val allowAnySegwit = Features.canUseFeature(commitments.localParams.initFeatures, commitments.remoteParams.initFeatures, Features.ShutdownAnySegwit)
require(isValidFinalScriptPubkey(actualLocalScript, allowAnySegwit), "invalid localScriptPubkey")
require(isValidFinalScriptPubkey(actualRemoteScript, allowAnySegwit), "invalid remoteScriptPubkey")
log.debug("making closing tx with closing fees={} and commitments:\n{}", closingFees.preferred, Commitments.specs2String(commitments))
log.debug("making closing tx with closing fee={} and commitments:\n{}", closingFees.preferred, Commitments.specs2String(commitments))
val dustLimit = localParams.dustLimit.max(remoteParams.dustLimit)
val closingTx = Transactions.makeClosingTx(commitInput, actualLocalScript, actualRemoteScript, localParams.isFunder, dustLimit, closingFees.preferred, localCommit.spec)
val localClosingSig = keyManager.sign(closingTx, keyManager.fundingPublicKey(commitments.localParams.fundingKeyPath), TxOwner.Local, commitmentFormat)
val closingSigned = ClosingSigned(channelId, closingFees.preferred, localClosingSig, TlvStream(ClosingSignedTlv.FeeRange(closingFees.min, closingFees.max)))
log.info(s"signed closing txid=${closingTx.tx.txid} with closing fees=${closingSigned.feeSatoshis}")
log.info(s"signed closing txid=${closingTx.tx.txid} with closing fee=${closingSigned.feeSatoshis}")
log.debug(s"closingTxid=${closingTx.tx.txid} closingTx=${closingTx.tx}}")
(closingTx, closingSigned)
}
Expand All @@ -512,7 +512,7 @@ object Helpers {
import commitments._
val lastCommitFeeSatoshi = commitments.commitInput.txOut.amount - commitments.localCommit.commitTxAndRemoteSig.commitTx.tx.txOut.map(_.amount).sum
if (remoteClosingFee > lastCommitFeeSatoshi && !commitments.channelFeatures.hasFeature(Features.AnchorOutputs)) {
log.error(s"remote proposed a commit fee higher than the last commitment fee: remote closing fees=${remoteClosingFee.toLong} last commit fees=$lastCommitFeeSatoshi")
log.error(s"remote proposed a commit fee higher than the last commitment fee: remote closing fee=${remoteClosingFee.toLong} last commit fees=$lastCommitFeeSatoshi")
Left(InvalidCloseFee(commitments.channelId, remoteClosingFee))
} else {
val (closingTx, closingSigned) = makeClosingTx(keyManager, commitments, localScriptPubkey, remoteScriptPubkey, ClosingFees(remoteClosingFee, remoteClosingFee, remoteClosingFee))
Expand Down
14 changes: 11 additions & 3 deletions eclair-core/src/test/scala/fr/acinq/eclair/channel/states/f/ShutdownStateSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -844,10 +844,18 @@ class ShutdownStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike wit
test("recv CMD_CLOSE with updated feerates") { f =>
import f._
val sender = TestProbe()
val closingFeerates = ClosingFeerates(FeeratePerKw(500 sat), FeeratePerKw(250 sat), FeeratePerKw(2500 sat))
alice ! CMD_CLOSE(sender.ref, None, Some(closingFeerates))
val closingFeerates1 = ClosingFeerates(FeeratePerKw(500 sat), FeeratePerKw(250 sat), FeeratePerKw(2500 sat))
alice ! CMD_CLOSE(sender.ref, None, Some(closingFeerates1))
sender.expectMsgType[RES_SUCCESS[CMD_CLOSE]]
assert(alice.stateData.asInstanceOf[DATA_SHUTDOWN].closingFeerates === Some(closingFeerates))
assert(alice.stateData.asInstanceOf[DATA_SHUTDOWN].closingFeerates === Some(closingFeerates1))

val closingScript = alice.stateData.asInstanceOf[DATA_SHUTDOWN].localShutdown.scriptPubKey
val closingFeerates2 = ClosingFeerates(FeeratePerKw(600 sat), FeeratePerKw(300 sat), FeeratePerKw(2500 sat))
alice ! CMD_CLOSE(sender.ref, Some(closingScript.reverse), Some(closingFeerates2))
sender.expectMsgType[RES_FAILURE[CMD_CLOSE, ClosingAlreadyInProgress]]
alice ! CMD_CLOSE(sender.ref, Some(closingScript), Some(closingFeerates2))
sender.expectMsgType[RES_SUCCESS[CMD_CLOSE]]
assert(alice.stateData.asInstanceOf[DATA_SHUTDOWN].closingFeerates === Some(closingFeerates2))
}

test("recv CMD_FORCECLOSE") { f =>
Expand Down
6 changes: 5 additions & 1 deletion eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,11 @@ class NegotiatingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
bob2alice.expectNoMessage(100 millis)
// Alice offered a fee range that was too low: she notices the warning sent by Bob and retries with higher fees.
val sender = TestProbe()
alice ! CMD_CLOSE(sender.ref, None, Some(ClosingFeerates(FeeratePerKw(1000 sat), FeeratePerKw(500 sat), FeeratePerKw(2000 sat))))
val closingScript = alice.stateData.asInstanceOf[DATA_NEGOTIATING].localShutdown.scriptPubKey
val closingFeerates = ClosingFeerates(FeeratePerKw(1000 sat), FeeratePerKw(500 sat), FeeratePerKw(2000 sat))
alice ! CMD_CLOSE(sender.ref, Some(closingScript.reverse), Some(closingFeerates))
sender.expectMsgType[RES_FAILURE[CMD_CLOSE, ClosingAlreadyInProgress]]
alice ! CMD_CLOSE(sender.ref, Some(closingScript), Some(closingFeerates))
sender.expectMsgType[RES_SUCCESS[CMD_CLOSE]]
val aliceClosing2 = alice2bob.expectMsgType[ClosingSigned]
assert(aliceClosing2.feeSatoshis > aliceClosing1.feeSatoshis)
Expand Down

0 comments on commit b6a6a73

Please sign in to comment.