From 3201d61e102e96c090f72f59885c72d960941def Mon Sep 17 00:00:00 2001 From: t-bast Date: Mon, 2 Oct 2023 09:56:29 +0200 Subject: [PATCH] Avoid unusable channels after a large splice Splicing (and dual funding as well) introduce a new scenario that could not happen before, where the channel initiator (who pays the fees for the commit tx) can end up below the channel reserve, or right above it. In that case it means that most of the channels funds are on the non initiator side, so we should allow HTLCs from the non-initiator to the initiator to move funds towards the initiator. We allow slightly dipping into the channel reserve in that case, for at most 5 pending HTLCs. --- .../fr/acinq/eclair/channel/Commitments.scala | 13 ++ .../states/e/NormalSplicesStateSpec.scala | 150 +++++++++++------- 2 files changed, 106 insertions(+), 57 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala index 26665bd02b..fc7ee28546 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala @@ -458,6 +458,19 @@ case class Commitment(fundingTxIndex: Long, } else if (missingForReceiver < 0.msat) { if (params.localParams.isInitiator) { // receiver is not the channel initiator; it is ok if it can't maintain its channel_reserve for now, as long as its balance is increasing, which is the case if it is receiving a payment + } else if (reduced.toLocal > fees && reduced.htlcs.size < 5) { + // Receiver is the channel initiator; we usually don't want to let them dip into their channel reserve, because + // that may give them a commitment transaction where they have nothing at stake, which would create an incentive + // for them to force-close using that commitment after it has been revoked. + // But we let them dip slightly into their channel reserve to pay the fees, to ensure that the channel is not + // stuck and unusable, because we can end up in that state in the following scenario: + // - they were above their channel reserve + // - we spliced a lot of funds into the channel, which increased the reserve requirements + // - they are now below the new reserve, but if we don't allow htlcs to them, they have no way of increasing their balance + // Since we only allow a limited number of htlcs, that doesn't let them dip into their reserve much. + // We could also keep track of the previous channel reserve, but this is additional state that is awkward to + // store and not trivial to correctly keep up-to-date. This simpler solution has a similar result with less + // complexity. } else { return Left(RemoteCannotAffordFeesForNewHtlc(params.channelId, amount = amount, missing = -missingForReceiver.truncateToSatoshi, reserve = remoteChannelReserve(params), fees = fees)) } 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..365a174dd3 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 @@ -68,98 +68,103 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik private val defaultSpliceOutScriptPubKey = hex"0020aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - private def useQuiescence(f: FixtureParam): Boolean = f.alice.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.params.useQuiescence + private def useQuiescence(s: TestFSMRef[ChannelState, ChannelData, Channel]): Boolean = s.stateData.asInstanceOf[ChannelDataWithCommitments].commitments.params.useQuiescence - private def initiateSpliceWithoutSigs(f: FixtureParam, spliceIn_opt: Option[SpliceIn] = None, spliceOut_opt: Option[SpliceOut] = None): TestProbe = { - import f._ + private def useQuiescence(f: FixtureParam): Boolean = useQuiescence(f.alice) + private def initiateSpliceWithoutSigs(s: TestFSMRef[ChannelState, ChannelData, Channel], r: TestFSMRef[ChannelState, ChannelData, Channel], s2r: TestProbe, r2s: TestProbe, spliceIn_opt: Option[SpliceIn], spliceOut_opt: Option[SpliceOut]): TestProbe = { val sender = TestProbe() val cmd = CMD_SPLICE(sender.ref, spliceIn_opt, spliceOut_opt) - alice ! cmd - if (useQuiescence(f)) { - exchangeStfu(f) + s ! cmd + if (useQuiescence(s)) { + exchangeStfu(s, r, s2r, r2s) } - alice2bob.expectMsgType[SpliceInit] - alice2bob.forward(bob) - bob2alice.expectMsgType[SpliceAck] - bob2alice.forward(alice) - - alice2bob.expectMsgType[TxAddInput] - alice2bob.forward(bob) - bob2alice.expectMsgType[TxComplete] - bob2alice.forward(alice) + s2r.expectMsgType[SpliceInit] + s2r.forward(r) + r2s.expectMsgType[SpliceAck] + r2s.forward(s) + + s2r.expectMsgType[TxAddInput] + s2r.forward(r) + r2s.expectMsgType[TxComplete] + r2s.forward(s) if (spliceIn_opt.isDefined) { - alice2bob.expectMsgType[TxAddInput] - alice2bob.forward(bob) - bob2alice.expectMsgType[TxComplete] - bob2alice.forward(alice) - alice2bob.expectMsgType[TxAddOutput] - alice2bob.forward(bob) - bob2alice.expectMsgType[TxComplete] - bob2alice.forward(alice) + s2r.expectMsgType[TxAddInput] + s2r.forward(r) + r2s.expectMsgType[TxComplete] + r2s.forward(s) + s2r.expectMsgType[TxAddOutput] + s2r.forward(r) + r2s.expectMsgType[TxComplete] + r2s.forward(s) } if (spliceOut_opt.isDefined) { - alice2bob.expectMsgType[TxAddOutput] - alice2bob.forward(bob) - bob2alice.expectMsgType[TxComplete] - bob2alice.forward(alice) + s2r.expectMsgType[TxAddOutput] + s2r.forward(r) + r2s.expectMsgType[TxComplete] + r2s.forward(s) } - alice2bob.expectMsgType[TxAddOutput] - alice2bob.forward(bob) - bob2alice.expectMsgType[TxComplete] - bob2alice.forward(alice) - alice2bob.expectMsgType[TxComplete] - alice2bob.forward(bob) + s2r.expectMsgType[TxAddOutput] + s2r.forward(r) + r2s.expectMsgType[TxComplete] + r2s.forward(s) + s2r.expectMsgType[TxComplete] + s2r.forward(r) sender } - private def exchangeSpliceSigs(f: FixtureParam, sender: TestProbe): Transaction = { - import f._ + private def initiateSpliceWithoutSigs(f: FixtureParam, spliceIn_opt: Option[SpliceIn] = None, spliceOut_opt: Option[SpliceOut] = None): TestProbe = initiateSpliceWithoutSigs(f.alice, f.bob, f.alice2bob, f.bob2alice, spliceIn_opt, spliceOut_opt) - val commitSigBob = bob2alice.fishForMessage() { + private def exchangeSpliceSigs(s: TestFSMRef[ChannelState, ChannelData, Channel], r: TestFSMRef[ChannelState, ChannelData, Channel], s2r: TestProbe, r2s: TestProbe, sender: TestProbe): Transaction = { + val commitSigR = r2s.fishForMessage() { case _: CommitSig => true case _: ChannelReady => false } - bob2alice.forward(alice, commitSigBob) - val commitSigAlice = alice2bob.fishForMessage() { + r2s.forward(s, commitSigR) + val commitSigS = s2r.fishForMessage() { case _: CommitSig => true case _: ChannelReady => false } - alice2bob.forward(bob, commitSigAlice) + s2r.forward(r, commitSigS) - val txSigsBob = bob2alice.fishForMessage() { + val txSigsR = r2s.fishForMessage() { case _: TxSignatures => true case _: ChannelUpdate => false } - bob2alice.forward(alice, txSigsBob) - val txSigsAlice = alice2bob.fishForMessage() { + r2s.forward(s, txSigsR) + val txSigsS = s2r.fishForMessage() { case _: TxSignatures => true case _: ChannelUpdate => false } - alice2bob.forward(bob, txSigsAlice) + s2r.forward(r, txSigsS) sender.expectMsgType[RES_SPLICE] - awaitCond(alice.stateData.asInstanceOf[DATA_NORMAL].spliceStatus == SpliceStatus.NoSplice) - awaitCond(bob.stateData.asInstanceOf[DATA_NORMAL].spliceStatus == SpliceStatus.NoSplice) - awaitCond(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.asInstanceOf[DualFundedUnconfirmedFundingTx].sharedTx.isInstanceOf[FullySignedSharedTransaction]) - awaitCond(bob.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.asInstanceOf[DualFundedUnconfirmedFundingTx].sharedTx.isInstanceOf[FullySignedSharedTransaction]) - alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.signedTx_opt.get + awaitCond(s.stateData.asInstanceOf[DATA_NORMAL].spliceStatus == SpliceStatus.NoSplice) + awaitCond(r.stateData.asInstanceOf[DATA_NORMAL].spliceStatus == SpliceStatus.NoSplice) + awaitCond(s.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.asInstanceOf[DualFundedUnconfirmedFundingTx].sharedTx.isInstanceOf[FullySignedSharedTransaction]) + awaitCond(r.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.asInstanceOf[DualFundedUnconfirmedFundingTx].sharedTx.isInstanceOf[FullySignedSharedTransaction]) + s.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.signedTx_opt.get } - private def initiateSplice(f: FixtureParam, spliceIn_opt: Option[SpliceIn] = None, spliceOut_opt: Option[SpliceOut] = None): Transaction = { - val sender = initiateSpliceWithoutSigs(f, spliceIn_opt, spliceOut_opt) - exchangeSpliceSigs(f, sender) + private def exchangeSpliceSigs(f: FixtureParam, sender: TestProbe): Transaction = exchangeSpliceSigs(f.alice, f.bob, f.alice2bob, f.bob2alice, sender) + + private def initiateSplice(s: TestFSMRef[ChannelState, ChannelData, Channel], r: TestFSMRef[ChannelState, ChannelData, Channel], s2r: TestProbe, r2s: TestProbe, spliceIn_opt: Option[SpliceIn], spliceOut_opt: Option[SpliceOut]): Transaction = { + val sender = initiateSpliceWithoutSigs(s, r, s2r, r2s, spliceIn_opt, spliceOut_opt) + exchangeSpliceSigs(s, r, s2r, r2s, sender) } - private def exchangeStfu(f: FixtureParam): Unit = { - import f._ - alice2bob.expectMsgType[Stfu] - alice2bob.forward(bob) - bob2alice.expectMsgType[Stfu] - bob2alice.forward(alice) + private def initiateSplice(f: FixtureParam, spliceIn_opt: Option[SpliceIn] = None, spliceOut_opt: Option[SpliceOut] = None): Transaction = initiateSplice(f.alice, f.bob, f.alice2bob, f.bob2alice, spliceIn_opt, spliceOut_opt) + + private def exchangeStfu(s: TestFSMRef[ChannelState, ChannelData, Channel], r: TestFSMRef[ChannelState, ChannelData, Channel], s2r: TestProbe, r2s: TestProbe): Unit = { + s2r.expectMsgType[Stfu] + s2r.forward(r) + r2s.expectMsgType[Stfu] + r2s.forward(s) } + private def exchangeStfu(f: FixtureParam): Unit = exchangeStfu(f.alice, f.bob, f.alice2bob, f.bob2alice) + case class TestHtlcs(aliceToBob: Seq[(ByteVector32, UpdateAddHtlc)], bobToAlice: Seq[(ByteVector32, UpdateAddHtlc)]) private def setupHtlcs(f: FixtureParam): TestHtlcs = { @@ -416,6 +421,37 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik awaitCond(bob.stateData.asInstanceOf[DATA_NORMAL].spliceStatus == SpliceStatus.NoSplice) } + test("recv CMD_SPLICE (remote splices in which takes us below reserve)", Tag(ChannelStateTestsTags.NoMaxHtlcValueInFlight)) { f => + import f._ + + assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localCommit.spec.toLocal == 800_000_000.msat) + val (r1, htlc1) = addHtlc(750_000_000 msat, alice, bob, alice2bob, bob2alice) + crossSign(alice, bob, alice2bob, bob2alice) + fulfillHtlc(htlc1.id, r1, bob, alice, bob2alice, alice2bob) + crossSign(bob, alice, bob2alice, alice2bob) + + // Bob makes a large splice: Alice doesn't meet the new reserve requirements, but she met the previous one, so we allow this. + initiateSplice(bob, alice, bob2alice, alice2bob, spliceIn_opt = Some(SpliceIn(4_000_000 sat)), spliceOut_opt = None) + val postSpliceState = alice.stateData.asInstanceOf[DATA_NORMAL] + assert(postSpliceState.commitments.latest.localCommit.spec.toLocal < postSpliceState.commitments.latest.localChannelReserve) + + // Since Alice is below the reserve and most of the funds are on Bob's side, Alice cannot send HTLCs. + val probe = TestProbe() + val (_, cmd) = makeCmdAdd(5_000_000 msat, bob.nodeParams.nodeId, bob.nodeParams.currentBlockHeight) + alice ! cmd.copy(replyTo = probe.ref) + probe.expectMsgType[RES_ADD_FAILED[InsufficientFunds]] + + // But Bob can send HTLCs to take Alice above the reserve. + val (r2, htlc2) = addHtlc(50_000_000 msat, bob, alice, bob2alice, alice2bob) + crossSign(bob, alice, bob2alice, alice2bob) + fulfillHtlc(htlc2.id, r2, alice, bob, alice2bob, bob2alice) + crossSign(alice, bob, alice2bob, bob2alice) + + // Alice can now send HTLCs as well. + addHtlc(10_000_000 msat, alice, bob, alice2bob, bob2alice) + crossSign(alice, bob, alice2bob, bob2alice) + } + def testSpliceInAndOutCmd(f: FixtureParam): Unit = { val htlcs = setupHtlcs(f)