From aaad2e1d61bb49488a447724ff9fe5a1ae463094 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Thu, 25 May 2023 13:13:12 +0200 Subject: [PATCH] Accept closing fee above commit fee (#2662) When performing a mutual close, we initially rejected fees that were higher to the commit tx fees. This was removed from the specification for anchor output channels, and doesn't make a lot of sense for standard channels either: even at a higher fee, it makes sense to do a mutual close to avoid waiting for relative delays on our outputs. Fixes #2646 --- .../fr/acinq/eclair/channel/Helpers.scala | 35 +++++++------------ .../states/g/NegotiatingStateSpec.scala | 23 +++++++----- 2 files changed, 27 insertions(+), 31 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 7b22c886e..96c721a30 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 @@ -64,13 +64,13 @@ object Helpers { } } - def extractShutdownScript(channelId: ByteVector32, localFeatures: Features[InitFeature], remoteFeatures: Features[InitFeature], upfrontShutdownScript_opt: Option[ByteVector]): Either[ChannelException, Option[ByteVector]] = { + private def extractShutdownScript(channelId: ByteVector32, localFeatures: Features[InitFeature], remoteFeatures: Features[InitFeature], upfrontShutdownScript_opt: Option[ByteVector]): Either[ChannelException, Option[ByteVector]] = { val canUseUpfrontShutdownScript = Features.canUseFeature(localFeatures, remoteFeatures, Features.UpfrontShutdownScript) val canUseAnySegwit = Features.canUseFeature(localFeatures, remoteFeatures, Features.ShutdownAnySegwit) extractShutdownScript(channelId, canUseUpfrontShutdownScript, canUseAnySegwit, upfrontShutdownScript_opt) } - def extractShutdownScript(channelId: ByteVector32, hasOptionUpfrontShutdownScript: Boolean, allowAnySegwit: Boolean, upfrontShutdownScript_opt: Option[ByteVector]): Either[ChannelException, Option[ByteVector]] = { + private def extractShutdownScript(channelId: ByteVector32, hasOptionUpfrontShutdownScript: Boolean, allowAnySegwit: Boolean, upfrontShutdownScript_opt: Option[ByteVector]): Either[ChannelException, Option[ByteVector]] = { (hasOptionUpfrontShutdownScript, upfrontShutdownScript_opt) match { case (true, None) => Left(MissingUpfrontShutdownScript(channelId)) case (true, Some(script)) if script.isEmpty => Right(None) // but the provided script can be empty @@ -304,7 +304,7 @@ object Helpers { * @param remoteFeeratePerKw remote fee rate per kiloweight * @return true if the remote fee rate is too small */ - def isFeeTooSmall(remoteFeeratePerKw: FeeratePerKw): Boolean = { + private def isFeeTooSmall(remoteFeeratePerKw: FeeratePerKw): Boolean = { remoteFeeratePerKw < FeeratePerKw.MinimumFeeratePerKw } @@ -605,9 +605,6 @@ object Helpers { object MutualClose { - // used only to compute tx weights and estimate fees - lazy val dummyPublicKey: PublicKey = PrivateKey(ByteVector32(ByteVector.fill(32)(1))).publicKey - def isValidFinalScriptPubkey(scriptPubKey: ByteVector, allowAnySegwit: Boolean): Boolean = { Try(Script.parse(scriptPubKey)) match { case Success(OP_DUP :: OP_HASH160 :: OP_PUSHDATA(pubkeyHash, _) :: OP_EQUALVERIFY :: OP_CHECKSIG :: Nil) if pubkeyHash.size == 20 => true @@ -622,7 +619,7 @@ object Helpers { def firstClosingFee(commitment: FullCommitment, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector, feerates: ClosingFeerates)(implicit log: LoggingAdapter): ClosingFees = { // this is just to estimate the weight, it depends on size of the pubkey scripts val dummyClosingTx = Transactions.makeClosingTx(commitment.commitInput, localScriptPubkey, remoteScriptPubkey, commitment.localParams.isInitiator, Satoshi(0), Satoshi(0), commitment.localCommit.spec) - val closingWeight = Transaction.weight(Transactions.addSigs(dummyClosingTx, dummyPublicKey, commitment.remoteFundingPubKey, Transactions.PlaceHolderSig, Transactions.PlaceHolderSig).tx) + val closingWeight = Transaction.weight(Transactions.addSigs(dummyClosingTx, Transactions.PlaceHolderPubKey, commitment.remoteFundingPubKey, Transactions.PlaceHolderSig, Transactions.PlaceHolderSig).tx) log.info(s"using feerates=$feerates for initial closing tx") feerates.computeFees(closingWeight) } @@ -666,21 +663,15 @@ object Helpers { } def checkClosingSignature(keyManager: ChannelKeyManager, commitment: FullCommitment, localScriptPubkey: ByteVector, remoteScriptPubkey: ByteVector, remoteClosingFee: Satoshi, remoteClosingSig: ByteVector64)(implicit log: LoggingAdapter): Either[ChannelException, (ClosingTx, ClosingSigned)] = { - val lastCommitFeeSatoshi = commitment.commitInput.txOut.amount - commitment.localCommit.commitTxAndRemoteSig.commitTx.tx.txOut.map(_.amount).sum - if (remoteClosingFee > lastCommitFeeSatoshi && commitment.params.commitmentFormat == DefaultCommitmentFormat) { - 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(commitment.channelId, remoteClosingFee)) - } else { - val (closingTx, closingSigned) = makeClosingTx(keyManager, commitment, localScriptPubkey, remoteScriptPubkey, ClosingFees(remoteClosingFee, remoteClosingFee, remoteClosingFee)) - if (checkClosingDustAmounts(closingTx)) { - val signedClosingTx = Transactions.addSigs(closingTx, keyManager.fundingPublicKey(commitment.localParams.fundingKeyPath, commitment.fundingTxIndex).publicKey, commitment.remoteFundingPubKey, closingSigned.signature, remoteClosingSig) - Transactions.checkSpendable(signedClosingTx) match { - case Success(_) => Right(signedClosingTx, closingSigned) - case _ => Left(InvalidCloseSignature(commitment.channelId, signedClosingTx.tx.txid)) - } - } else { - Left(InvalidCloseAmountBelowDust(commitment.channelId, closingTx.tx.txid)) + val (closingTx, closingSigned) = makeClosingTx(keyManager, commitment, localScriptPubkey, remoteScriptPubkey, ClosingFees(remoteClosingFee, remoteClosingFee, remoteClosingFee)) + if (checkClosingDustAmounts(closingTx)) { + val signedClosingTx = Transactions.addSigs(closingTx, keyManager.fundingPublicKey(commitment.localParams.fundingKeyPath, commitment.fundingTxIndex).publicKey, commitment.remoteFundingPubKey, closingSigned.signature, remoteClosingSig) + Transactions.checkSpendable(signedClosingTx) match { + case Success(_) => Right(signedClosingTx, closingSigned) + case _ => Left(InvalidCloseSignature(commitment.channelId, signedClosingTx.tx.txid)) } + } else { + Left(InvalidCloseAmountBelowDust(commitment.channelId, closingTx.tx.txid)) } } @@ -995,7 +986,7 @@ object Helpers { * * This function returns the per-commitment secret in the first case, and None in the other cases. */ - def getRemotePerCommitmentSecret(keyManager: ChannelKeyManager, params: ChannelParams, remotePerCommitmentSecrets: ShaChain, commitTx: Transaction)(implicit log: LoggingAdapter): Option[(Long, PrivateKey)] = { + def getRemotePerCommitmentSecret(keyManager: ChannelKeyManager, params: ChannelParams, remotePerCommitmentSecrets: ShaChain, commitTx: Transaction): Option[(Long, PrivateKey)] = { import params._ // a valid tx will always have at least one input, but this ensures we don't throw in tests val sequence = commitTx.txIn.headOption.map(_.sequence).getOrElse(0L) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala index 0f2542594..1b467703a 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala @@ -26,6 +26,7 @@ import fr.acinq.eclair.channel.fsm.Channel import fr.acinq.eclair.channel.publish.TxPublisher.{PublishFinalTx, PublishTx} import fr.acinq.eclair.channel.states.{ChannelStateTestsBase, ChannelStateTestsTags} import fr.acinq.eclair.transactions.Transactions +import fr.acinq.eclair.transactions.Transactions.ZeroFeeHtlcTxAnchorOutputsCommitmentFormat import fr.acinq.eclair.wire.protocol.ClosingSignedTlv.FeeRange import fr.acinq.eclair.wire.protocol.{ClosingSigned, Error, Shutdown, TlvStream, Warning} import fr.acinq.eclair.{CltvExpiry, Features, MilliSatoshiLong, TestConstants, TestFeeEstimator, TestKitBaseClass, randomBytes32} @@ -415,17 +416,21 @@ class NegotiatingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike bob2alice.expectNoMessage(100 millis) } - test("recv ClosingSigned (fee too high)") { f => + test("recv ClosingSigned (fee higher than commit tx fee)", Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => import f._ - bobClose(f) + val commitment = alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest + val commitFee = Transactions.commitTxFeeMsat(commitment.localParams.dustLimit, commitment.localCommit.spec, ZeroFeeHtlcTxAnchorOutputsCommitmentFormat) + aliceClose(f) val aliceCloseSig = alice2bob.expectMsgType[ClosingSigned] - val tx = bob.stateData.asInstanceOf[DATA_NEGOTIATING].commitments.latest.localCommit.commitTxAndRemoteSig.commitTx.tx - alice2bob.forward(bob, aliceCloseSig.copy(feeSatoshis = 99000 sat)) // sig doesn't matter, it is checked later - val error = bob2alice.expectMsgType[Error] - assert(new String(error.data.toArray).startsWith("invalid close fee: fee_satoshis=99000 sat")) - assert(bob2blockchain.expectMsgType[PublishFinalTx].tx.txid == tx.txid) - bob2blockchain.expectMsgType[PublishTx] - bob2blockchain.expectMsgType[WatchTxConfirmed] + assert(aliceCloseSig.feeSatoshis > commitFee.truncateToSatoshi) + alice2bob.forward(bob, aliceCloseSig) + val bobCloseSig = bob2alice.expectMsgType[ClosingSigned] + assert(bobCloseSig.feeSatoshis == aliceCloseSig.feeSatoshis) + awaitCond(bob.stateName == CLOSING) + val closingTx = bob.stateData.asInstanceOf[DATA_CLOSING].mutualClosePublished.head.tx + assert(bob2blockchain.expectMsgType[PublishFinalTx].tx.txid == closingTx.txid) + bob2alice.forward(alice, bobCloseSig) + assert(alice2blockchain.expectMsgType[PublishFinalTx].tx.txid == closingTx.txid) } test("recv ClosingSigned (invalid sig)") { f =>