Skip to content

Commit

Permalink
Fix PR comments
Browse files Browse the repository at this point in the history 
- use `forgetHtlcInfo` to avoid code duplication
- test exact number of DB entries cleaned up
- use commitment index mismatch in splice test
  • Loading branch information
@t-bast
t-bast committed Oct 4, 2023
1 parent 9632771 commit 125eafc
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 41 deletions.
6 changes: 1 addition & 5 deletions eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand Down
75 changes: 40 additions & 35 deletions eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalSplicesStateSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)

Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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._

Expand Down Expand Up @@ -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)))
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand All @@ -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)))
Expand All @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion eclair-core/src/test/scala/fr/acinq/eclair/db/ChannelsDbSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down

0 comments on commit 125eafc

Please sign in to comment.