From 125eafc176caac02462af335a636786e51359857 Mon Sep 17 00:00:00 2001 From: t-bast Date: Wed, 4 Oct 2023 14:39:05 +0200 Subject: [PATCH] Fix PR comments - use `forgetHtlcInfo` to avoid code duplication - test exact number of DB entries cleaned up - use commitment index mismatch in splice test --- .../fr/acinq/eclair/db/pg/PgChannelsDb.scala | 6 +- .../states/e/NormalSplicesStateSpec.scala | 75 ++++++++++--------- .../fr/acinq/eclair/db/ChannelsDbSpec.scala | 3 +- 3 files changed, 43 insertions(+), 41 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala b/eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala index 3dce363aab..e7e8851433 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala @@ -235,11 +235,7 @@ class PgChannelsDb(implicit ds: DataSource, lock: PgLock) extends ChannelsDb wit // The htlc_infos may contain millions of rows, which is very expensive to delete synchronously. // We instead run an asynchronous job to clean up that data in small batches. - using(pg.prepareStatement("INSERT INTO local.htlc_infos_to_remove (channel_id, before_commitment_number) VALUES(?, ?) ON CONFLICT (channel_id) DO UPDATE SET before_commitment_number = EXCLUDED.before_commitment_number")) { statement => - statement.setString(1, channelId.toHex) - statement.setLong(2, Long.MaxValue) - statement.executeUpdate() - } + forgetHtlcInfos(channelId, Long.MaxValue) using(pg.prepareStatement("UPDATE local.channels SET is_closed=TRUE, closed_timestamp=? WHERE channel_id=?")) { statement => statement.setTimestamp(1, Timestamp.from(Instant.now())) 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 efb9ab224b..cc5d4e67a6 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 @@ -31,9 +31,9 @@ import fr.acinq.eclair.channel._ import fr.acinq.eclair.channel.fsm.Channel import fr.acinq.eclair.channel.fund.InteractiveTxBuilder.FullySignedSharedTransaction import fr.acinq.eclair.channel.publish.TxPublisher.{PublishFinalTx, PublishReplaceableTx, PublishTx, SetChannelId} +import fr.acinq.eclair.channel.states.ChannelStateTestsBase import fr.acinq.eclair.channel.states.ChannelStateTestsBase.{FakeTxPublisherFactory, PimpTestFSM} -import fr.acinq.eclair.channel.states.ChannelStateTestsTags.{AnchorOutputsZeroFeeHtlcTxs, NoMaxHtlcValueInFlight, ZeroConf} -import fr.acinq.eclair.channel.states.{ChannelStateTestsBase, ChannelStateTestsTags} +import fr.acinq.eclair.channel.states.ChannelStateTestsTags._ import fr.acinq.eclair.db.RevokedHtlcInfoCleaner.ForgetHtlcInfos import fr.acinq.eclair.testutils.PimpTestProbe.convert import fr.acinq.eclair.transactions.DirectedHtlc.{incoming, outgoing} @@ -56,7 +56,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik implicit val log: akka.event.LoggingAdapter = akka.event.NoLogging override def withFixture(test: OneArgTest): Outcome = { - val tags = test.tags + ChannelStateTestsTags.DualFunding + ChannelStateTestsTags.Splicing + val tags = test.tags + DualFunding + Splicing val setup = init(tags = tags) import setup._ reachNormal(setup, tags) @@ -264,10 +264,10 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik } test("recv CMD_SPLICE (splice-in, non dual-funded channel)") { () => - val f = init(tags = Set(ChannelStateTestsTags.DualFunding, ChannelStateTestsTags.Splicing)) + val f = init(tags = Set(DualFunding, Splicing)) import f._ - reachNormal(f, tags = Set(ChannelStateTestsTags.Splicing)) // we open a non dual-funded channel + reachNormal(f, tags = Set(Splicing)) // we open a non dual-funded channel alice2bob.ignoreMsg { case _: ChannelUpdate => true } bob2alice.ignoreMsg { case _: ChannelUpdate => true } awaitCond(alice.stateName == NORMAL && bob.stateName == NORMAL) @@ -292,7 +292,7 @@ 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 => + test("recv CMD_SPLICE (splice-in, local and remote commit index mismatch)", Tag(Quiescence)) { f => import f._ // Alice and Bob asynchronously exchange HTLCs, which makes their commit indices diverge. @@ -367,7 +367,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik sender.expectMsgType[RES_FAILURE[_, _]] } - test("recv CMD_SPLICE (splice-out, would go below reserve, quiescent)", Tag(ChannelStateTestsTags.Quiescence), Tag(NoMaxHtlcValueInFlight)) { f => + test("recv CMD_SPLICE (splice-out, would go below reserve, quiescent)", Tag(Quiescence), Tag(NoMaxHtlcValueInFlight)) { f => import f._ setupHtlcs(f) @@ -429,7 +429,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testSpliceInAndOutCmd(f) } - test("recv CMD_SPLICE (splice-in + splice-out, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("recv CMD_SPLICE (splice-in + splice-out, quiescence)", Tag(Quiescence)) { f => testSpliceInAndOutCmd(f) } @@ -729,13 +729,19 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik assert(bob.stateData.asInstanceOf[DATA_NORMAL].commitments.inactive.map(_.fundingTxIndex) == Seq.empty) } - test("emit post-splice events", Tag(NoMaxHtlcValueInFlight)) { f => + test("emit post-splice events", Tag(NoMaxHtlcValueInFlight), Tag(Quiescence)) { f => import f._ + // Alice and Bob asynchronously exchange HTLCs, which makes their commit indices diverge. + addHtlc(25_000_000 msat, alice, bob, alice2bob, bob2alice) + addHtlc(50_000_000 msat, bob, alice, bob2alice, alice2bob) + crossSign(alice, bob, alice2bob, bob2alice) + val initialState = alice.stateData.asInstanceOf[DATA_NORMAL] assert(initialState.commitments.latest.capacity == 1_500_000.sat) - assert(initialState.commitments.latest.localCommit.spec.toLocal == 800_000_000.msat) - assert(initialState.commitments.latest.localCommit.spec.toRemote == 700_000_000.msat) + assert(initialState.commitments.latest.localCommit.spec.toLocal == 775_000_000.msat) + assert(initialState.commitments.latest.localCommit.spec.toRemote == 650_000_000.msat) + assert(initialState.commitments.localCommitIndex != initialState.commitments.remoteCommitIndex) val aliceEvents = TestProbe() val bobEvents = TestProbe() @@ -764,9 +770,9 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik bob ! WatchFundingConfirmedTriggered(BlockHeight(400000), 42, fundingTx1) bob2alice.expectMsgType[SpliceLocked] bob2alice.forward(alice) - aliceEvents.expectAvailableBalanceChanged(balance = 1_300_000_000.msat, capacity = 2_000_000.sat) + aliceEvents.expectAvailableBalanceChanged(balance = 1_275_000_000.msat, capacity = 2_000_000.sat) bobEvents.expectMsg(ForgetHtlcInfos(initialState.channelId, initialState.commitments.localCommitIndex)) - bobEvents.expectAvailableBalanceChanged(balance = 700_000_000.msat, capacity = 2_000_000.sat) + bobEvents.expectAvailableBalanceChanged(balance = 650_000_000.msat, capacity = 2_000_000.sat) aliceEvents.expectNoMessage(100 millis) bobEvents.expectNoMessage(100 millis) @@ -781,8 +787,8 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik alice2bob.expectMsgType[SpliceLocked] alice2bob.forward(bob) aliceEvents.expectMsg(ForgetHtlcInfos(initialState.channelId, initialState.commitments.remoteCommitIndex)) - aliceEvents.expectAvailableBalanceChanged(balance = 1_800_000_000.msat, capacity = 2_500_000.sat) - bobEvents.expectAvailableBalanceChanged(balance = 700_000_000.msat, capacity = 2_500_000.sat) + aliceEvents.expectAvailableBalanceChanged(balance = 1_775_000_000.msat, capacity = 2_500_000.sat) + bobEvents.expectAvailableBalanceChanged(balance = 650_000_000.msat, capacity = 2_500_000.sat) aliceEvents.expectNoMessage(100 millis) bobEvents.expectNoMessage(100 millis) } @@ -1054,7 +1060,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testDisconnectCommitSigNotReceived(f) } - test("disconnect (commit_sig not received, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("disconnect (commit_sig not received, quiescence)", Tag(Quiescence)) { f => testDisconnectCommitSigNotReceived(f) } @@ -1094,7 +1100,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testDisconnectCommitSigReceivedByAlice(f) } - test("disconnect (commit_sig received by alice, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("disconnect (commit_sig received by alice, quiescence)", Tag(Quiescence)) { f => testDisconnectCommitSigReceivedByAlice(f) } @@ -1136,7 +1142,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testDisconnectTxSignaturesSentByBob(f) } - test("disconnect (tx_signatures sent by bob, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("disconnect (tx_signatures sent by bob, quiescence)", Tag(Quiescence)) { f => testDisconnectTxSignaturesSentByBob(f) } @@ -1185,7 +1191,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testDisconnectTxSignaturesReceivedByAlice(f) } - test("disconnect (tx_signatures received by alice, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("disconnect (tx_signatures received by alice, quiescence)", Tag(Quiescence)) { f => testDisconnectTxSignaturesReceivedByAlice(f) } @@ -1231,15 +1237,14 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik resolveHtlcs(f, htlcs, spliceOutFee = 0.sat) } - test("disconnect (tx_signatures received by alice, zero-conf)", Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("disconnect (tx_signatures received by alice, zero-conf)", Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => testDisconnectTxSignaturesReceivedByAliceZeroConf(f) } - test("disconnect (tx_signatures received by alice, zero-conf, quiescence)", Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs), Tag(ChannelStateTestsTags.Quiescence)) { f => + test("disconnect (tx_signatures received by alice, zero-conf, quiescence)", Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs), Tag(Quiescence)) { f => testDisconnectTxSignaturesReceivedByAliceZeroConf(f) } - test("disconnect (tx_signatures sent by alice, splice confirms while bob is offline)") { f => import f._ @@ -1272,7 +1277,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik awaitCond(bob.stateData.asInstanceOf[DATA_NORMAL].spliceStatus == SpliceStatus.NoSplice) } - test("don't resend splice_locked when zero-conf channel confirms", Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("don't resend splice_locked when zero-conf channel confirms", Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => import f._ initiateSplice(f, spliceIn_opt = Some(SpliceIn(500_000 sat, pushAmount = 0 msat))) @@ -1469,7 +1474,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testForceCloseWithMultipleSplicesSimple(f) } - test("force-close with multiple splices (simple, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("force-close with multiple splices (simple, quiescence)", Tag(Quiescence)) { f => testForceCloseWithMultipleSplicesSimple(f) } @@ -1551,7 +1556,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testForceCloseWithMultipleSplicesPreviousActiveRemote(f) } - test("force-close with multiple splices (previous active remote, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("force-close with multiple splices (previous active remote, quiescence)", Tag(Quiescence)) { f => testForceCloseWithMultipleSplicesPreviousActiveRemote(f) } @@ -1626,7 +1631,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik testForceCloseWithMultipleSplicesPreviousActiveRevoked(f) } - test("force-close with multiple splices (previous active revoked, quiescent)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("force-close with multiple splices (previous active revoked, quiescent)", Tag(Quiescence)) { f => testForceCloseWithMultipleSplicesPreviousActiveRevoked(f) } @@ -1736,11 +1741,11 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik assert(Helpers.Closing.isClosed(alice.stateData.asInstanceOf[DATA_CLOSING], None).exists(_.isInstanceOf[RemoteClose])) } - test("force-close with multiple splices (inactive remote)", Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("force-close with multiple splices (inactive remote)", Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => testForceCloseWithMultipleSplicesInactiveRemote(f) } - test("force-close with multiple splices (inactive remote, quiescence)", Tag(ChannelStateTestsTags.Quiescence), Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("force-close with multiple splices (inactive remote, quiescence)", Tag(Quiescence), Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => testForceCloseWithMultipleSplicesInactiveRemote(f) } @@ -1845,11 +1850,11 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik assert(Helpers.Closing.isClosed(alice.stateData.asInstanceOf[DATA_CLOSING], None).exists(_.isInstanceOf[RevokedClose])) } - test("force-close with multiple splices (inactive revoked)", Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("force-close with multiple splices (inactive revoked)", Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => testForceCloseWithMultipleSplicesInactiveRevoked(f) } - test("force-close with multiple splices (inactive revoked, quiescence)", Tag(ChannelStateTestsTags.Quiescence), Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("force-close with multiple splices (inactive revoked, quiescence)", Tag(Quiescence), Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => testForceCloseWithMultipleSplicesInactiveRevoked(f) } @@ -1888,7 +1893,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik bob2blockchain.expectNoMessage(100 millis) } - test("put back watches after restart (inactive)", Tag(ChannelStateTestsTags.ZeroConf), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("put back watches after restart (inactive)", Tag(ZeroConf), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => import f._ val fundingTx0 = alice.stateData.asInstanceOf[DATA_NORMAL].commitments.latest.localFundingStatus.signedTx_opt.get @@ -1947,7 +1952,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik bob2blockchain.expectNoMessage(100 millis) } - test("recv CMD_SPLICE (splice-in + splice-out) with pre and post splice htlcs", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("recv CMD_SPLICE (splice-in + splice-out) with pre and post splice htlcs", Tag(Quiescence)) { f => import f._ val htlcs = setupHtlcs(f) @@ -1984,7 +1989,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik resolveHtlcs(f, htlcs, spliceOutFee = 0.sat) } - test("recv CMD_SPLICE (splice-in + splice-out) with pending htlcs, resolved after splice locked", Tag(ChannelStateTestsTags.Quiescence), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs)) { f => + test("recv CMD_SPLICE (splice-in + splice-out) with pending htlcs, resolved after splice locked", Tag(Quiescence), Tag(AnchorOutputsZeroFeeHtlcTxs)) { f => import f._ val htlcs = setupHtlcs(f) @@ -2003,7 +2008,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik resolveHtlcs(f, htlcs, spliceOutFee = 0.sat) } - test("recv multiple CMD_SPLICE (splice-in, splice-out, quiescence)", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("recv multiple CMD_SPLICE (splice-in, splice-out, quiescence)", Tag(Quiescence)) { f => val htlcs = setupHtlcs(f) initiateSplice(f, spliceIn_opt = Some(SpliceIn(500_000 sat))) @@ -2012,7 +2017,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik resolveHtlcs(f, htlcs, spliceOutFee = spliceOutFee(f, capacity = 1_900_000.sat)) } - test("recv invalid htlc signatures during splice-in", Tag(ChannelStateTestsTags.Quiescence)) { f => + test("recv invalid htlc signatures during splice-in", Tag(Quiescence)) { f => import f._ val htlcs = setupHtlcs(f) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/db/ChannelsDbSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/db/ChannelsDbSpec.scala index 5ee4f7b931..6517827e51 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/db/ChannelsDbSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/db/ChannelsDbSpec.scala @@ -147,8 +147,9 @@ class ChannelsDbSpec extends AnyFunSuite { (channel2.channelId, 74), ) db.removeHtlcInfos(10) // This should remove all the data for one of the two channels in one batch + assert(obsoleteHtlcInfo.flatMap { case (channelId, commitNumber) => db.listHtlcInfos(channelId, commitNumber) }.size == 5) db.removeHtlcInfos(3) // This should remove only part of the data for the remaining channel - assert(obsoleteHtlcInfo.exists { case (channelId, commitNumber) => db.listHtlcInfos(channelId, commitNumber).nonEmpty }) + assert(obsoleteHtlcInfo.flatMap { case (channelId, commitNumber) => db.listHtlcInfos(channelId, commitNumber) }.size == 2) db.removeHtlcInfos(3) // This should remove the rest of the data for the remaining channel obsoleteHtlcInfo.foreach { case (channelId, commitNumber) => db.listHtlcInfos(channelId, commitNumber).isEmpty }