From 9f281a262fca7964ba47130f59721c1595a7cc61 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Fri, 22 Sep 2023 17:11:39 +0200 Subject: [PATCH] Fix `invalid_commit_sig` issue when expecting `tx_signatures` (#536) * Always retransmit `tx_signatures` if requested If our peer asks us to retransmit our `tx_signatures`, we should do it even if the transaction is already confirmed. * Ignore previously received `commit_sig` When expecting a retransmission of `tx_signatures`, we should ignore the `commit_sig` they send just before if we've already received it. The right way to check that we've already received it is to compare its signature to our latest commitment transaction. --- .../fr/acinq/lightning/channel/Commitments.kt | 4 +- .../acinq/lightning/channel/states/Channel.kt | 16 ++++-- .../acinq/lightning/channel/states/Normal.kt | 20 ++++++-- .../acinq/lightning/channel/states/Syncing.kt | 41 ++++++++------- .../kotlin/fr/acinq/lightning/io/Peer.kt | 2 - .../serialization/v4/Deserialization.kt | 11 +++- .../serialization/v4/Serialization.kt | 3 +- .../channel/CommitmentsTestsCommon.kt | 5 +- .../channel/states/SpliceTestsCommon.kt | 51 +++++++++++++++++++ 9 files changed, 117 insertions(+), 36 deletions(-) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt b/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt index b0df8848e..1408bbb26 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt @@ -123,7 +123,7 @@ sealed class LocalFundingStatus { override val fee: Satoshi = sharedTx.tx.fees } - data class ConfirmedFundingTx(override val signedTx: Transaction, override val fee: Satoshi) : LocalFundingStatus() { + data class ConfirmedFundingTx(override val signedTx: Transaction, override val fee: Satoshi, val localSigs: TxSignatures) : LocalFundingStatus() { override val txId: ByteVector32 = signedTx.txid } } @@ -832,7 +832,7 @@ data class Commitments( when (c.localFundingStatus) { is LocalFundingStatus.UnconfirmedFundingTx -> { logger.debug { "setting localFundingStatus confirmed for fundingTxId=${fundingTx.txid}" } - c.copy(localFundingStatus = LocalFundingStatus.ConfirmedFundingTx(fundingTx, c.localFundingStatus.sharedTx.tx.fees)) + c.copy(localFundingStatus = LocalFundingStatus.ConfirmedFundingTx(fundingTx, c.localFundingStatus.sharedTx.tx.fees, c.localFundingStatus.sharedTx.localSigs)) } is LocalFundingStatus.ConfirmedFundingTx -> c } diff --git a/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Channel.kt b/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Channel.kt index 127168953..a58b20c48 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Channel.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Channel.kt @@ -144,7 +144,11 @@ sealed class ChannelState { } internal fun ChannelContext.unhandled(cmd: ChannelCommand): Pair> { - logger.warning { "unhandled command ${cmd::class.simpleName} in state ${this@ChannelState::class.simpleName}" } + when (cmd) { + is ChannelCommand.MessageReceived -> logger.warning { "unhandled message ${cmd.message::class.simpleName} in state ${this@ChannelState::class.simpleName}" } + is ChannelCommand.WatchReceived -> logger.warning { "unhandled watch event ${cmd.watch.event::class.simpleName} in state ${this@ChannelState::class.simpleName}" } + else -> logger.warning { "unhandled command ${cmd::class.simpleName} in state ${this@ChannelState::class.simpleName}" } + } return Pair(this@ChannelState, listOf()) } @@ -162,7 +166,11 @@ sealed class ChannelState { ) internal fun ChannelContext.handleLocalError(cmd: ChannelCommand, t: Throwable): Pair> { - logger.error(t) { "error on command ${cmd::class.simpleName}" } + when (cmd) { + is ChannelCommand.MessageReceived -> logger.error(t) { "error on message ${cmd.message::class.simpleName}" } + is ChannelCommand.WatchReceived -> logger.error { "error on watch event ${cmd.watch.event::class.simpleName}" } + else -> logger.error(t) { "error on command ${cmd::class.simpleName}" } + } fun abort(channelId: ByteVector32?, state: ChannelState): Pair> { val actions = buildList { @@ -180,7 +188,7 @@ sealed class ChannelState { return state.run { spendLocalCurrent().run { copy(second = second + ChannelAction.Message.Send(error)) } } } - return when(val state = this@ChannelState) { + return when (val state = this@ChannelState) { is WaitForInit -> abort(null, state) is WaitForOpenChannel -> abort(state.temporaryChannelId, state) is WaitForAcceptChannel -> abort(state.temporaryChannelId, state) @@ -213,7 +221,7 @@ sealed class ChannelState { // we already have published a mutual close tx, it's always better to use that Pair(state, emptyList()) } else { - state.localCommitPublished ?.let { + state.localCommitPublished?.let { // we're already trying to claim our commitment, there's nothing more we can do Pair(state, emptyList()) } ?: state.run { spendLocalCurrent() } diff --git a/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt b/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt index 7ec51e6fe..94bb0bae8 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt @@ -1,6 +1,7 @@ package fr.acinq.lightning.channel.states import fr.acinq.bitcoin.ByteVector32 +import fr.acinq.bitcoin.SigHash import fr.acinq.lightning.Feature import fr.acinq.lightning.Features import fr.acinq.lightning.ShortChannelId @@ -9,6 +10,7 @@ import fr.acinq.lightning.blockchain.WatchConfirmed import fr.acinq.lightning.blockchain.WatchEventConfirmed import fr.acinq.lightning.blockchain.WatchEventSpent import fr.acinq.lightning.channel.* +import fr.acinq.lightning.transactions.Scripts import fr.acinq.lightning.transactions.Transactions import fr.acinq.lightning.utils.* import fr.acinq.lightning.wire.* @@ -192,11 +194,10 @@ data class Normal( is InteractiveTxSigningSessionAction.SendTxSigs -> sendSpliceTxSigs(spliceStatus.origins, action, cmd.message.channelData) } } - commitments.params.channelFeatures.hasFeature(Feature.DualFunding) && commitments.latest.localFundingStatus.signedTx == null && cmd.message.batchSize == 1 -> { - // The latest funding transaction is unconfirmed and we're missing our peer's tx_signatures: any commit_sig that we receive before that should be ignored, - // it's either a retransmission of a commit_sig we've already received or a bug that will eventually lead to a force-close anyway. - // NB: we check the dual-funding feature, because legacy single-funding unconfirmed channels will have `localFundingStatus=UnconfirmedFundingTx`, - // so we cannot just check on the absence of `signedTx`. + ignoreRetransmittedCommitSig(cmd.message) -> { + // We haven't received our peer's tx_signatures for the latest funding transaction and asked them to resend it on reconnection. + // They also resend their corresponding commit_sig, but we have already received it so we should ignore it. + // Note that the funding transaction may have confirmed while we were offline. logger.info { "ignoring commit_sig, we're still waiting for tx_signatures" } Pair(this@Normal, listOf()) } @@ -733,6 +734,15 @@ data class Normal( return Pair(nextState, actions) } + /** This function should be used to ignore a commit_sig that we've already received. */ + private fun ignoreRetransmittedCommitSig(commit: CommitSig): Boolean { + // If we already have a signed commitment transaction containing their signature, we must have previously received that commit_sig. + val commitTx = commitments.latest.localCommit.publishableTxs.commitTx.tx + return commitments.params.channelFeatures.hasFeature(Feature.DualFunding) && + commit.batchSize == 1 && + commitTx.txIn.first().witness.stack.contains(Scripts.der(commit.signature, SigHash.SIGHASH_ALL)) + } + /** If we haven't completed the signing steps of an interactive-tx session, we will ask our peer to retransmit signatures for the corresponding transaction. */ fun getUnsignedFundingTxId(): ByteVector32? = when { spliceStatus is SpliceStatus.WaitingForSigs -> spliceStatus.session.fundingTx.txId diff --git a/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Syncing.kt b/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Syncing.kt index 09c9bf5d8..36b124858 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Syncing.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/channel/states/Syncing.kt @@ -172,26 +172,29 @@ data class Syncing(val state: PersistedChannelState, val channelReestablishSent: actions.add(ChannelAction.Message.Send(commitSig)) state.spliceStatus } else if (state.commitments.latest.fundingTxId == cmd.message.nextFundingTxId) { - if (state.commitments.latest.localFundingStatus is LocalFundingStatus.UnconfirmedFundingTx) { - if (state.commitments.latest.localFundingStatus.sharedTx is PartiallySignedSharedTransaction) { - // If we have not received their tx_signatures, we can't tell whether they had received our commit_sig, so we need to retransmit it - logger.info { "re-sending commit_sig for fundingTxIndex=${state.commitments.latest.fundingTxIndex} fundingTxId=${state.commitments.latest.fundingTxId}" } - val commitSig = state.commitments.latest.remoteCommit.sign( - channelKeys(), - state.commitments.params, - fundingTxIndex = state.commitments.latest.fundingTxIndex, - state.commitments.latest.remoteFundingPubkey, - state.commitments.latest.commitInput - ) - actions.add(ChannelAction.Message.Send(commitSig)) + when (val localFundingStatus = state.commitments.latest.localFundingStatus) { + is LocalFundingStatus.UnconfirmedFundingTx -> { + if (localFundingStatus.sharedTx is PartiallySignedSharedTransaction) { + // If we have not received their tx_signatures, we can't tell whether they had received our commit_sig, so we need to retransmit it + logger.info { "re-sending commit_sig for fundingTxIndex=${state.commitments.latest.fundingTxIndex} fundingTxId=${state.commitments.latest.fundingTxId}" } + val commitSig = state.commitments.latest.remoteCommit.sign( + channelKeys(), + state.commitments.params, + fundingTxIndex = state.commitments.latest.fundingTxIndex, + state.commitments.latest.remoteFundingPubkey, + state.commitments.latest.commitInput + ) + actions.add(ChannelAction.Message.Send(commitSig)) + } + logger.info { "re-sending tx_signatures for fundingTxId=${cmd.message.nextFundingTxId}" } + actions.add(ChannelAction.Message.Send(localFundingStatus.sharedTx.localSigs)) + } + is LocalFundingStatus.ConfirmedFundingTx -> { + // The funding tx is confirmed, and they have not received our tx_signatures, but they must have received our commit_sig, otherwise they + // would not have sent their tx_signatures and we would not have been able to publish the funding tx in the first place. + logger.info { "re-sending tx_signatures for fundingTxId=${cmd.message.nextFundingTxId}" } + actions.add(ChannelAction.Message.Send(localFundingStatus.localSigs)) } - logger.info { "re-sending tx_signatures for fundingTxId=${cmd.message.nextFundingTxId}" } - actions.add(ChannelAction.Message.Send(state.commitments.latest.localFundingStatus.sharedTx.localSigs)) - } else { - // The funding tx is confirmed, and they have not received our tx_signatures, but they must have received our commit_sig, otherwise they - // would not have sent their tx_signatures and we would not have been able to publish the funding tx in the first place. We could in theory - // recompute our tx_signatures, but instead we do nothing: they will shortly be notified that the funding tx has confirmed. - logger.warning { "cannot re-send tx_signatures for fundingTxId=${cmd.message.nextFundingTxId}, transaction is already confirmed" } } state.spliceStatus } else if (cmd.message.nextFundingTxId != null) { diff --git a/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt b/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt index be654f769..95434404c 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt @@ -936,7 +936,6 @@ class Peer( } } is HasChannelId -> { - logger.info { "received ${msg::class.simpleName} for channel ${msg.channelId}" } if (msg is Error && msg.channelId == ByteVector32.Zeroes) { logger.error { "connection error: ${msg.toAscii()}" } } else { @@ -952,7 +951,6 @@ class Peer( } } is ChannelUpdate -> { - logger.info { "received ${msg::class.simpleName} for channel ${msg.shortChannelId}" } _channels.values.filterIsInstance().find { it.shortChannelId == msg.shortChannelId }?.let { state -> val event1 = ChannelCommand.MessageReceived(msg) val (state1, actions) = state.process(event1) diff --git a/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Deserialization.kt b/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Deserialization.kt index f232e3b2d..0edf0c81d 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Deserialization.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Deserialization.kt @@ -419,7 +419,16 @@ object Deserialization { ) 0x01 -> LocalFundingStatus.ConfirmedFundingTx( signedTx = readTransaction(), - fee = readNumber().sat + fee = readNumber().sat, + // We previously didn't store the tx_signatures after the transaction was confirmed. + // It is only used to be retransmitted on reconnection if our peer had not received it. + // This happens very rarely in practice, so putting dummy values here shouldn't be an issue. + localSigs = TxSignatures(ByteVector32.Zeroes, ByteVector32.Zeroes, listOf()) + ) + 0x02 -> LocalFundingStatus.ConfirmedFundingTx( + signedTx = readTransaction(), + fee = readNumber().sat, + localSigs = readLightningMessage() as TxSignatures ) else -> error("unknown discriminator $discriminator for class ${LocalFundingStatus::class}") }, diff --git a/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Serialization.kt b/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Serialization.kt index b1d9f1e9e..f3948de37 100644 --- a/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Serialization.kt +++ b/src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Serialization.kt @@ -464,9 +464,10 @@ object Serialization { writeNumber(localFundingStatus.createdAt) } is LocalFundingStatus.ConfirmedFundingTx -> { - write(0x01) + write(0x02) writeBtcObject(localFundingStatus.signedTx) writeNumber(localFundingStatus.fee.toLong()) + writeLightningMessage(localFundingStatus.localSigs) } } when (remoteFundingStatus) { diff --git a/src/commonTest/kotlin/fr/acinq/lightning/channel/CommitmentsTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/channel/CommitmentsTestsCommon.kt index 883d285c5..df0afa7c6 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/channel/CommitmentsTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/channel/CommitmentsTestsCommon.kt @@ -26,6 +26,7 @@ import fr.acinq.lightning.transactions.Scripts import fr.acinq.lightning.transactions.Transactions import fr.acinq.lightning.utils.* import fr.acinq.lightning.wire.IncorrectOrUnknownPaymentDetails +import fr.acinq.lightning.wire.TxSignatures import fr.acinq.lightning.wire.UpdateAddHtlc import org.kodein.log.LoggerFactory import org.kodein.log.newLogger @@ -514,7 +515,7 @@ class CommitmentsTestsCommon : LightningTestSuite(), LoggingContext { Commitment( fundingTxIndex = 0, remoteFundingPubkey = randomKey().publicKey(), - LocalFundingStatus.ConfirmedFundingTx(dummyFundingTx, 500.sat), + LocalFundingStatus.ConfirmedFundingTx(dummyFundingTx, 500.sat, TxSignatures(randomBytes32(), randomBytes32(), listOf())), RemoteFundingStatus.Locked, LocalCommit(0, CommitmentSpec(setOf(), feeRatePerKw, toLocal, toRemote), PublishableTxs(localCommitTx, listOf())), RemoteCommit(0, CommitmentSpec(setOf(), feeRatePerKw, toRemote, toLocal), randomBytes32(), randomKey().publicKey()), @@ -559,7 +560,7 @@ class CommitmentsTestsCommon : LightningTestSuite(), LoggingContext { Commitment( fundingTxIndex = 0, remoteFundingPubkey = randomKey().publicKey(), - LocalFundingStatus.ConfirmedFundingTx(dummyFundingTx, 500.sat), + LocalFundingStatus.ConfirmedFundingTx(dummyFundingTx, 500.sat, TxSignatures(randomBytes32(), randomBytes32(), listOf())), RemoteFundingStatus.Locked, LocalCommit(0, CommitmentSpec(setOf(), FeeratePerKw(0.sat), toLocal, toRemote), PublishableTxs(localCommitTx, listOf())), RemoteCommit(0, CommitmentSpec(setOf(), FeeratePerKw(0.sat), toRemote, toLocal), randomBytes32(), randomKey().publicKey()), diff --git a/src/commonTest/kotlin/fr/acinq/lightning/channel/states/SpliceTestsCommon.kt b/src/commonTest/kotlin/fr/acinq/lightning/channel/states/SpliceTestsCommon.kt index df94114b7..a431f4aa6 100644 --- a/src/commonTest/kotlin/fr/acinq/lightning/channel/states/SpliceTestsCommon.kt +++ b/src/commonTest/kotlin/fr/acinq/lightning/channel/states/SpliceTestsCommon.kt @@ -469,6 +469,7 @@ class SpliceTestsCommon : LightningTestSuite() { val (alice3, actionsAlice3) = alice2.process(ChannelCommand.MessageReceived(channelReestablishBob)) assertEquals(actionsAlice3.size, 1) val commitSigAlice2 = actionsAlice3.findOutgoingMessage() + assertEquals(commitSigAlice1.signature, commitSigAlice2.signature) val (alice4, actionsAlice4) = alice3.process(ChannelCommand.MessageReceived(commitSigBob2)) assertTrue(actionsAlice4.isEmpty()) @@ -506,6 +507,56 @@ class SpliceTestsCommon : LightningTestSuite() { actionsBob7.has() } + @Test + fun `disconnect -- tx_signatures sent by alice -- confirms while bob is offline`() { + val (alice, bob) = reachNormalWithConfirmedFundingTx() + val (alice1, commitSigAlice1, bob1, commitSigBob1) = spliceOutWithoutSigs(alice, bob, 20_000.sat) + + // Bob completes the splice, but is missing Alice's tx_signatures. + val (bob2, actionsBob2) = bob1.process(ChannelCommand.MessageReceived(commitSigAlice1)) + assertIs>(bob2) + val txSigsBob = actionsBob2.hasOutgoingMessage() + assertEquals(bob2.state.spliceStatus, SpliceStatus.None) + + // Alice completes the splice, but Bob doesn't receive her tx_signatures. + val (alice2, actionsAlice2) = alice1.process(ChannelCommand.MessageReceived(commitSigBob1)) + assertTrue(actionsAlice2.isEmpty()) + val (alice3, actionsAlice3) = alice2.process(ChannelCommand.MessageReceived(txSigsBob)) + assertIs>(alice3) + actionsAlice3.hasOutgoingMessage() + assertEquals(alice3.state.spliceStatus, SpliceStatus.None) + val spliceTx = alice3.commitments.latest.localFundingStatus.signedTx!! + + // The transaction confirms. + val (alice4, actionsAlice4) = alice3.process(ChannelCommand.WatchReceived(WatchEventConfirmed(alice.channelId, BITCOIN_FUNDING_DEPTHOK, 100, 0, spliceTx))) + assertIs>(alice4) + assertEquals(3, actionsAlice4.size) + actionsAlice4.hasWatchFundingSpent(spliceTx.txid) + actionsAlice4.hasOutgoingMessage() + actionsAlice4.has() + + val (alice5, bob3, channelReestablishAlice) = disconnect(alice4, bob2) + assertNull(channelReestablishAlice.nextFundingTxId) + val (bob4, actionsBob4) = bob3.process(ChannelCommand.MessageReceived(channelReestablishAlice)) + assertEquals(1, actionsBob4.size) + val channelReestablishBob = actionsBob4.findOutgoingMessage() + assertEquals(channelReestablishBob.nextFundingTxId, spliceTx.txid) + val (alice6, actionsAlice6) = alice5.process(ChannelCommand.MessageReceived(channelReestablishBob)) + assertIs>(alice6) + assertEquals(alice6.state.spliceStatus, SpliceStatus.None) + assertEquals(2, actionsAlice6.size) + val txSigsAlice = actionsAlice6.hasOutgoingMessage() + actionsAlice6.hasOutgoingMessage() + + // Bob receives tx_signatures, which completes the splice. + val (bob5, actionsBob5) = bob4.process(ChannelCommand.MessageReceived(txSigsAlice)) + assertIs>(bob5) + assertEquals(bob5.state.spliceStatus, SpliceStatus.None) + assertEquals(2, actionsBob5.size) + actionsBob5.hasPublishTx(spliceTx) + actionsBob5.has() + } + @Test fun `disconnect -- tx_signatures received by alice`() { val (alice, bob) = reachNormalWithConfirmedFundingTx()