Skip to content

Commit

Permalink
Fix tx_signatures ordering for splices (#2768)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
t-bast authored Nov 3, 2023
1 parent 5b11f76 commit 5a37059
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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._

Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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._

Expand Down

0 comments on commit 5a37059

Please sign in to comment.