Skip to content

Commit

Permalink
Fix invalid_commit_sig issue when expecting tx_signatures (#536)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@t-bast
t-bast authored Sep 22, 2023
1 parent a1b0a31 commit 9f281a2
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 36 deletions.
4 changes: 2 additions & 2 deletions src/commonMain/kotlin/fr/acinq/lightning/channel/Commitments.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
Expand Down
16 changes: 12 additions & 4 deletions src/commonMain/kotlin/fr/acinq/lightning/channel/states/Channel.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,11 @@ sealed class ChannelState {
}

internal fun ChannelContext.unhandled(cmd: ChannelCommand): Pair<ChannelState, List<ChannelAction>> {
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())
}

Expand All @@ -162,7 +166,11 @@ sealed class ChannelState {
)

internal fun ChannelContext.handleLocalError(cmd: ChannelCommand, t: Throwable): Pair<ChannelState, List<ChannelAction>> {
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<ChannelState, List<ChannelAction>> {
val actions = buildList {
Expand All @@ -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)
Expand Down Expand Up @@ -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() }
Expand Down
20 changes: 15 additions & 5 deletions src/commonMain/kotlin/fr/acinq/lightning/channel/states/Normal.kt
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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.*
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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
Expand Down
41 changes: 22 additions & 19 deletions src/commonMain/kotlin/fr/acinq/lightning/channel/states/Syncing.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 0 additions & 2 deletions src/commonMain/kotlin/fr/acinq/lightning/io/Peer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -952,7 +951,6 @@ class Peer(
}
}
is ChannelUpdate -> {
logger.info { "received ${msg::class.simpleName} for channel ${msg.shortChannelId}" }
_channels.values.filterIsInstance<Normal>().find { it.shortChannelId == msg.shortChannelId }?.let { state ->
val event1 = ChannelCommand.MessageReceived(msg)
val (state1, actions) = state.process(event1)
Expand Down
11 changes: 10 additions & 1 deletion src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Deserialization.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
},
Expand Down
3 changes: 2 additions & 1 deletion src/commonMain/kotlin/fr/acinq/lightning/serialization/v4/Serialization.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
5 changes: 3 additions & 2 deletions src/commonTest/kotlin/fr/acinq/lightning/channel/CommitmentsTestsCommon.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()),
Expand Down Expand Up @@ -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()),
Expand Down
Loading

0 comments on commit 9f281a2

Please sign in to comment.