From 5a37059b5b4f130817dc21c46e07c81947e389f7 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Fri, 3 Nov 2023 14:06:54 +0100 Subject: [PATCH] Fix `tx_signatures` ordering for splices (#2768) We were incorrectly splitting the shared input amount based on each peer's balance, but for the signing order the peer that adds the shared input takes credit for the whole amount of that input. --- .../channel/fund/InteractiveTxBuilder.scala | 14 ++++++++++---- .../eclair/channel/InteractiveTxBuilderSpec.scala | 15 +++++++++++---- .../channel/states/e/NormalSplicesStateSpec.scala | 1 - 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala index 0991182d00..5ee766a652 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala @@ -873,13 +873,19 @@ object InteractiveTxSigningSession { /** A local commitment for which we haven't received our peer's signatures. */ case class UnsignedLocalCommit(index: Long, spec: CommitmentSpec, commitTx: CommitTx, htlcTxs: List[HtlcTx]) - private def shouldSignFirst(channelParams: ChannelParams, tx: SharedTransaction): Boolean = { - if (tx.localAmountIn == tx.remoteAmountIn) { + private def shouldSignFirst(isInitiator: Boolean, channelParams: ChannelParams, tx: SharedTransaction): Boolean = { + val sharedAmountIn = tx.sharedInput_opt.map(i => i.localAmount + i.remoteAmount + i.htlcAmount).getOrElse(0 msat).truncateToSatoshi + val (localAmountIn, remoteAmountIn) = if (isInitiator) { + (sharedAmountIn + tx.localInputs.map(i => i.previousTx.txOut(i.previousTxOutput.toInt).amount).sum, tx.remoteInputs.map(i => i.txOut.amount).sum) + } else { + (tx.localInputs.map(i => i.previousTx.txOut(i.previousTxOutput.toInt).amount).sum, sharedAmountIn + tx.remoteInputs.map(i => i.txOut.amount).sum) + } + if (localAmountIn == remoteAmountIn) { // When both peers contribute the same amount, the peer with the lowest pubkey must transmit its `tx_signatures` first. LexicographicalOrdering.isLessThan(channelParams.localNodeId.value, channelParams.remoteNodeId.value) } else { // Otherwise, the peer with the lowest total of input amount must transmit its `tx_signatures` first. - tx.localAmountIn < tx.remoteAmountIn + localAmountIn < remoteAmountIn } } @@ -944,7 +950,7 @@ object InteractiveTxSigningSession { val channelKeyPath = nodeParams.channelKeyManager.keyPath(channelParams.localParams, channelParams.channelConfig) val localPerCommitmentPoint = nodeParams.channelKeyManager.commitmentPoint(channelKeyPath, localCommitIndex) LocalCommit.fromCommitSig(nodeParams.channelKeyManager, channelParams, fundingTx.txId, fundingTxIndex, fundingParams.remoteFundingPubKey, commitInput, remoteCommitSig, localCommitIndex, unsignedLocalCommit.spec, localPerCommitmentPoint).map { signedLocalCommit => - if (shouldSignFirst(channelParams, fundingTx.tx)) { + if (shouldSignFirst(fundingParams.isInitiator, channelParams, fundingTx.tx)) { val fundingStatus = LocalFundingStatus.DualFundedUnconfirmedFundingTx(fundingTx, nodeParams.currentBlockHeight, fundingParams) val commitment = Commitment(fundingTxIndex, fundingParams.remoteFundingPubKey, fundingStatus, RemoteFundingStatus.NotLocked, signedLocalCommit, remoteCommit, None) SendingSigs(fundingStatus, commitment, fundingTx.localSigs) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/InteractiveTxBuilderSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/InteractiveTxBuilderSpec.scala index 28305fd43a..d6d1ad8c5c 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/InteractiveTxBuilderSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/InteractiveTxBuilderSpec.scala @@ -557,10 +557,14 @@ class InteractiveTxBuilderSpec extends TestKitBaseClass with AnyFunSuiteLike wit test("initiator and non-initiator splice-in") { val targetFeerate = FeeratePerKw(1000 sat) - val fundingA1 = 100_000 sat - val utxosA = Seq(350_000 sat, 150_000 sat) - val fundingB1 = 50_000 sat - val utxosB = Seq(175_000 sat, 90_000 sat) + // We chose those amounts to ensure that Bob always signs first: + // - funding tx: Alice has one 380 000 sat input and Bob has one 350 000 sat input + // - splice tx: Alice has the shared input (150 000 sat) and one 380 000 sat input, Bob has one 350 000 sat input + // It verifies that we don't split the shared input amount: if we did, + val fundingA1 = 50_000 sat + val utxosA = Seq(380_000 sat, 380_000 sat) + val fundingB1 = 100_000 sat + val utxosB = Seq(350_000 sat, 350_000 sat) withFixture(fundingA1, utxosA, fundingB1, utxosB, targetFeerate, 660 sat, 0, RequireConfirmedInputs(forLocal = true, forRemote = true)) { f => import f._ @@ -625,6 +629,9 @@ class InteractiveTxBuilderSpec extends TestKitBaseClass with AnyFunSuiteLike wit val successB2 = bob2alice.expectMsgType[Succeeded] assert(successB2.signingSession.fundingTx.localSigs.previousFundingTxSig_opt.nonEmpty) val (spliceTxA, commitmentA2, spliceTxB, commitmentB2) = fixtureParams.exchangeSigsBobFirst(spliceFixtureParams.fundingParamsB, successA2, successB2) + // Bob has more balance than Alice in the shared input, so its total contribution is greater than Alice. + // But Bob still signs first, because we don't split the shared input's balance when deciding who signs first. + assert(spliceTxA.tx.localAmountIn < spliceTxA.tx.remoteAmountIn) assert(spliceTxA.signedTx.txIn.exists(_.outPoint == commitmentA1.commitInput.outPoint)) assert(0.msat < spliceTxA.tx.localFees) assert(0.msat < spliceTxA.tx.remoteFees) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala index aa3fcf435e..6fbc53df7c 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala @@ -1232,7 +1232,6 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testDisconnectTxSignaturesReceivedByAliceZeroConf(f) } - test("disconnect (tx_signatures sent by alice, splice confirms while bob is offline)") { f => import f._