From a3b1f9fb234efd18f49c751ca3e19783debab9c2 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Thu, 28 Sep 2023 17:50:54 +0200 Subject: [PATCH] Handle splice with local/remote index mismatch (#2757) We assumed that once quiescent, the local and remote commitment index would be the same, but that's not true. Those two indices may diverge because of concurrent updates. We need to keep using the right index for each commitment during and after a splice, otherwise it leads to "invalid commit sig" errors. --- .../fr/acinq/eclair/channel/Helpers.scala | 10 ++-- .../channel/fund/InteractiveTxBuilder.scala | 2 +- .../ChannelStateTestsHelperMethods.scala | 18 ++++--- .../states/e/NormalSplicesStateSpec.scala | 48 +++++++++++++++++++ 4 files changed, 65 insertions(+), 13 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 c8f40fe11d..fb86655cb9 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 @@ -378,7 +378,7 @@ object Helpers { fundingTxIndex = 0, fundingTxHash, fundingTxOutputIndex, remoteFundingPubKey = remoteFundingPubKey, remotePerCommitmentPoint = remoteFirstPerCommitmentPoint, - commitmentIndex = 0).map { + localCommitmentIndex = 0, remoteCommitmentIndex = 0).map { case (localSpec, localCommit, remoteSpec, remoteCommit, _) => (localSpec, localCommit, remoteSpec, remoteCommit) } } @@ -397,7 +397,7 @@ object Helpers { fundingTxHash: ByteVector32, fundingTxOutputIndex: Int, remoteFundingPubKey: PublicKey, remotePerCommitmentPoint: PublicKey, - commitmentIndex: Long): Either[ChannelException, (CommitmentSpec, CommitTx, CommitmentSpec, CommitTx, Seq[HtlcTx])] = { + localCommitmentIndex: Long, remoteCommitmentIndex: Long): Either[ChannelException, (CommitmentSpec, CommitTx, CommitmentSpec, CommitTx, Seq[HtlcTx])] = { import params._ val localSpec = CommitmentSpec(localHtlcs, commitTxFeerate, toLocal = toLocal, toRemote = toRemote) val remoteSpec = CommitmentSpec(localHtlcs.map(_.opposite), commitTxFeerate, toLocal = toRemote, toRemote = toLocal) @@ -417,9 +417,9 @@ object Helpers { val fundingPubKey = keyManager.fundingPublicKey(localParams.fundingKeyPath, fundingTxIndex) val channelKeyPath = keyManager.keyPath(localParams, channelConfig) val commitmentInput = makeFundingInputInfo(fundingTxHash, fundingTxOutputIndex, fundingAmount, fundingPubKey.publicKey, remoteFundingPubKey) - val localPerCommitmentPoint = keyManager.commitmentPoint(channelKeyPath, commitmentIndex) - val (localCommitTx, _) = Commitment.makeLocalTxs(keyManager, channelConfig, channelFeatures, commitmentIndex, localParams, remoteParams, fundingTxIndex, remoteFundingPubKey, commitmentInput, localPerCommitmentPoint, localSpec) - val (remoteCommitTx, htlcTxs) = Commitment.makeRemoteTxs(keyManager, channelConfig, channelFeatures, commitmentIndex, localParams, remoteParams, fundingTxIndex, remoteFundingPubKey, commitmentInput, remotePerCommitmentPoint, remoteSpec) + val localPerCommitmentPoint = keyManager.commitmentPoint(channelKeyPath, localCommitmentIndex) + val (localCommitTx, _) = Commitment.makeLocalTxs(keyManager, channelConfig, channelFeatures, localCommitmentIndex, localParams, remoteParams, fundingTxIndex, remoteFundingPubKey, commitmentInput, localPerCommitmentPoint, localSpec) + val (remoteCommitTx, htlcTxs) = Commitment.makeRemoteTxs(keyManager, channelConfig, channelFeatures, remoteCommitmentIndex, localParams, remoteParams, fundingTxIndex, remoteFundingPubKey, commitmentInput, remotePerCommitmentPoint, remoteSpec) val sortedHtlcTxs = htlcTxs.sortBy(_.input.outPoint.index) Right(localSpec, localCommitTx, remoteSpec, remoteCommitTx, sortedHtlcTxs) } 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 655291dfe6..3c16b24afb 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 @@ -737,7 +737,7 @@ private class InteractiveTxBuilder(replyTo: ActorRef[InteractiveTxBuilder.Respon fundingTxIndex = purpose.fundingTxIndex, fundingTx.hash, fundingOutputIndex, remotePerCommitmentPoint = purpose.remotePerCommitmentPoint, remoteFundingPubKey = fundingParams.remoteFundingPubKey, - commitmentIndex = purpose.localCommitIndex) match { + localCommitmentIndex = purpose.localCommitIndex, remoteCommitmentIndex = purpose.remoteCommitIndex) match { case Left(cause) => replyTo ! RemoteFailure(cause) unlockAndStop(completeTx) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala index d7c3172178..74bb158004 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala @@ -28,11 +28,11 @@ import fr.acinq.eclair._ import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher._ import fr.acinq.eclair.blockchain.fee.{FeeratePerKw, FeeratesPerKw} import fr.acinq.eclair.blockchain.{DummyOnChainWallet, OnChainWallet, OnchainPubkeyCache, SingleKeyOnChainWallet} -import fr.acinq.eclair.channel.{ChannelData, ChannelState, _} import fr.acinq.eclair.channel.fsm.Channel import fr.acinq.eclair.channel.publish.TxPublisher import fr.acinq.eclair.channel.publish.TxPublisher.PublishReplaceableTx import fr.acinq.eclair.channel.states.ChannelStateTestsBase.FakeTxPublisherFactory +import fr.acinq.eclair.channel._ import fr.acinq.eclair.payment.OutgoingPaymentPacket.Upstream import fr.acinq.eclair.payment.send.SpontaneousRecipient import fr.acinq.eclair.payment.{Invoice, OutgoingPaymentPacket} @@ -451,22 +451,26 @@ trait ChannelStateTestsBase extends Assertions with Eventually { s2r.expectMsgType[RevokeAndAck] s2r.forward(r) if (rHasChanges) { - s2r.expectMsgType[CommitSig] - s2r.forward(r) + sigs2r = 0 + do { + s2r.expectMsgType[CommitSig] + s2r.forward(r) + sigs2r += 1 + } while (sigs2r < batchSize) r2s.expectMsgType[RevokeAndAck] r2s.forward(s) eventually { assert(s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.localCommitIndex == sCommitIndex + 1) - assert(s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == sCommitIndex + 2) + assert(s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == rCommitIndex + 2) assert(r.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.localCommitIndex == rCommitIndex + 2) - assert(r.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == rCommitIndex + 1) + assert(r.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == sCommitIndex + 1) } } else { eventually { assert(s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.localCommitIndex == sCommitIndex + 1) - assert(s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == sCommitIndex + 1) + assert(s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == rCommitIndex + 1) assert(r.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.localCommitIndex == rCommitIndex + 1) - assert(r.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == rCommitIndex + 1) + assert(r.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.remoteCommitIndex == sCommitIndex + 1) } } } 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 b0cff34d78..aa3fcf435e 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 @@ -291,6 +291,54 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik assert(postSpliceState.commitments.latest.remoteChannelReserve == 15_000.sat) } + test("recv CMD_SPLICE (splice-in, local and remote commit index mismatch)", Tag(ChannelStateTestsTags.Quiescence)) { f => + import f._ + + // Alice and Bob asynchronously exchange HTLCs, which makes their commit indices diverge. + val (r1, add1) = addHtlc(15_000_000 msat, alice, bob, alice2bob, bob2alice) + alice ! CMD_SIGN() + alice2bob.expectMsgType[CommitSig] + val (r2, add2) = addHtlc(10_000_000 msat, bob, alice, bob2alice, alice2bob) + alice2bob.forward(bob) + bob2alice.expectMsgType[RevokeAndAck] + bob2alice.forward(alice) + bob2alice.expectMsgType[CommitSig] + bob2alice.forward(alice) + alice2bob.expectMsgType[RevokeAndAck] + alice2bob.forward(bob) + alice2bob.expectMsgType[CommitSig] + alice2bob.forward(bob) + alice2bob.expectNoMessage(100 millis) + bob2alice.expectMsgType[RevokeAndAck] + bob2alice.forward(alice) + bob2alice.expectNoMessage(100 millis) + + val initialState = alice.stateData.asInstanceOf[DATA_NORMAL] + assert(initialState.commitments.latest.capacity == 1_500_000.sat) + assert(initialState.commitments.latest.localCommit.spec.toLocal == 785_000_000.msat) + assert(initialState.commitments.latest.localCommit.spec.toRemote == 690_000_000.msat) + assert(initialState.commitments.latest.localCommit.index == 1) + assert(initialState.commitments.latest.remoteCommit.index == 2) + + initiateSplice(f, spliceIn_opt = Some(SpliceIn(500_000 sat))) + assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.capacity == 2_000_000.sat) + assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localCommit.spec.toLocal == 1_285_000_000.msat) + assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localCommit.spec.toRemote == 690_000_000.msat) + + // Resolve pending HTLCs (we have two active commitments). + fulfillHtlc(add1.id, r1, bob, alice, bob2alice, alice2bob) + fulfillHtlc(add2.id, r2, alice, bob, alice2bob, bob2alice) + crossSign(alice, bob, alice2bob, bob2alice) + alice2bob.expectNoMessage(100 millis) + bob2alice.expectNoMessage(100 millis) + + val finalState = alice.stateData.asInstanceOf[DATA_NORMAL] + assert(finalState.commitments.latest.localCommit.spec.toLocal == 1_295_000_000.msat) + assert(finalState.commitments.latest.localCommit.spec.toRemote == 705_000_000.msat) + assert(finalState.commitments.latest.localCommit.index == 2) + assert(finalState.commitments.latest.remoteCommit.index == 4) + } + test("recv CMD_SPLICE (splice-out)") { f => import f._