Only persist trimmed htlcs (#724)

We persist htlc data in order to be able to claim htlc outputs in
case a revoked tx is published by our counterparty, so only htlcs
above remote's `dust_limit` matter.

Removed the TODO because we need data to be indexed by commit number so
it is ok to write the same htlc data for every commitment it is included
in.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

@@ -608,10 +608,12 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
                 case u: UpdateFailHtlc => relayer ! CommandBuffer.CommandAck(u.channelId, u.id)
                 case u: UpdateFailMalformedHtlc => relayer ! CommandBuffer.CommandAck(u.channelId, u.id)
               }
-              // TODO: be smarter and only consider commitments1.localChanges.signed and commitments1.remoteChanges.signed
               val nextRemoteCommit = commitments1.remoteNextCommitInfo.left.get.nextRemoteCommit
               val nextCommitNumber = nextRemoteCommit.index
-              nextRemoteCommit.spec.htlcs collect {
+              // we persist htlc data in order to be able to claim htlc outputs in case a revoked tx is published by our
+              // counterparty, so only htlcs above remote's dust_limit matter
+              val trimmedHtlcs = Transactions.trimOfferedHtlcs(Satoshi(d.commitments.remoteParams.dustLimitSatoshis), nextRemoteCommit.spec) ++ Transactions.trimReceivedHtlcs(Satoshi(d.commitments.remoteParams.dustLimitSatoshis), nextRemoteCommit.spec)
+              trimmedHtlcs collect {
                 case DirectedHtlc(_, u) =>
                   log.info(s"adding paymentHash=${u.paymentHash} cltvExpiry=${u.expiry} to htlcs db for commitNumber=$nextCommitNumber")
                   nodeParams.channelsDb.addOrUpdateHtlcInfo(d.channelId, nextCommitNumber, u.paymentHash, u.expiry)

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

@@ -558,7 +558,7 @@ object Helpers {
           })
 
           // we retrieve the informations needed to rebuild htlc scripts
-          val htlcInfos = db.listHtlcHtlcInfos(commitments.channelId, txnumber)
+          val htlcInfos = db.listHtlcInfos(commitments.channelId, txnumber)
           log.info(s"got htlcs=${htlcInfos.size} for txnumber=$txnumber")
           val htlcsRedeemScripts = (
             htlcInfos.map { case (paymentHash, cltvExpiry) => Scripts.htlcReceived(remoteHtlcPubkey, localHtlcPubkey, remoteRevocationPubkey, Crypto.ripemd160(paymentHash), cltvExpiry) } ++

eclair-core/src/main/scala/fr/acinq/eclair/db/ChannelsDb.scala

@@ -29,6 +29,6 @@ trait ChannelsDb {
 
   def addOrUpdateHtlcInfo(channelId: BinaryData, commitmentNumber: Long, paymentHash: BinaryData, cltvExpiry: Long)
 
-  def listHtlcHtlcInfos(channelId: BinaryData, commitmentNumber: Long): Seq[(BinaryData, Long)]
+  def listHtlcInfos(channelId: BinaryData, commitmentNumber: Long): Seq[(BinaryData, Long)]
 
 }

eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala

@@ -89,7 +89,7 @@ class SqliteChannelsDb(sqlite: Connection) extends ChannelsDb {
     }
   }
 
-  def listHtlcHtlcInfos(channelId: BinaryData, commitmentNumber: Long): Seq[(BinaryData, Long)] = {
+  def listHtlcInfos(channelId: BinaryData, commitmentNumber: Long): Seq[(BinaryData, Long)] = {
     using(sqlite.prepareStatement("SELECT payment_hash, cltv_expiry FROM htlc_infos WHERE channel_id=? AND commitment_number=?")) { statement =>
       statement.setBytes(1, channelId)
       statement.setLong(2, commitmentNumber)

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

@@ -32,6 +32,7 @@ import fr.acinq.eclair.router.Announcements
 import fr.acinq.eclair.transactions.{IN, OUT}
 import fr.acinq.eclair.wire.{AnnouncementSignatures, ChannelUpdate, ClosingSigned, CommitSig, Error, FailureMessageCodecs, PermanentChannelFailure, RevokeAndAck, Shutdown, UpdateAddHtlc, UpdateFailHtlc, UpdateFailMalformedHtlc, UpdateFee, UpdateFulfillHtlc}
 import fr.acinq.eclair.{Globals, TestConstants, TestkitBaseClass}
+import fr.acinq.eclair.transactions.Transactions.{htlcSuccessWeight, htlcTimeoutWeight, weight2fee}
 import org.scalatest.{Outcome, Tag}
 
 import scala.concurrent.duration._
@@ -461,6 +462,54 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods {
     assert(commitSig.htlcSignatures.toSet.size == 4)
   }
 
+  test("recv CMD_SIGN (check htlc info are persisted)") { f =>
+    import f._
+    val sender = TestProbe()
+    // for the test to be really useful we have constraint on parameters
+    assert(Alice.nodeParams.dustLimitSatoshis > Bob.nodeParams.dustLimitSatoshis)
+    // we're gonna exchange two htlcs in each direction, the goal is to have bob's commitment have 4 htlcs, and alice's
+    // commitment only have 3. We will then check that alice indeed persisted 4 htlcs, and bob only 3.
+    val aliceMinReceive = Alice.nodeParams.dustLimitSatoshis + weight2fee(TestConstants.feeratePerKw, htlcSuccessWeight).toLong
+    val aliceMinOffer = Alice.nodeParams.dustLimitSatoshis + weight2fee(TestConstants.feeratePerKw, htlcTimeoutWeight).toLong
+    val bobMinReceive = Bob.nodeParams.dustLimitSatoshis + weight2fee(TestConstants.feeratePerKw, htlcSuccessWeight).toLong
+    val bobMinOffer = Bob.nodeParams.dustLimitSatoshis + weight2fee(TestConstants.feeratePerKw, htlcTimeoutWeight).toLong
+    val a2b_1 = bobMinReceive + 10 // will be in alice and bob tx
+  val a2b_2 = bobMinReceive + 20 // will be in alice and bob tx
+  val b2a_1 = aliceMinReceive + 10 // will be in alice and bob tx
+  val b2a_2 = bobMinOffer + 10 // will be only be in bob tx
+    assert(a2b_1 > aliceMinOffer && a2b_1 > bobMinReceive)
+    assert(a2b_2 > aliceMinOffer && a2b_2 > bobMinReceive)
+    assert(b2a_1 > aliceMinReceive && b2a_1 > bobMinOffer)
+    assert(b2a_2 < aliceMinReceive && b2a_2 > bobMinOffer)
+    sender.send(alice, CMD_ADD_HTLC(a2b_1 * 1000, "11" * 32, 400144))
+    sender.expectMsg("ok")
+    alice2bob.expectMsgType[UpdateAddHtlc]
+    alice2bob.forward(bob)
+    sender.send(alice, CMD_ADD_HTLC(a2b_2 * 1000, "22" * 32, 400144))
+    sender.expectMsg("ok")
+    alice2bob.expectMsgType[UpdateAddHtlc]
+    alice2bob.forward(bob)
+    sender.send(bob, CMD_ADD_HTLC(b2a_1 * 1000, "33" * 32, 400144))
+    sender.expectMsg("ok")
+    bob2alice.expectMsgType[UpdateAddHtlc]
+    bob2alice.forward(alice)
+    sender.send(bob, CMD_ADD_HTLC(b2a_2 * 1000, "44" * 32, 400144))
+    sender.expectMsg("ok")
+    bob2alice.expectMsgType[UpdateAddHtlc]
+    bob2alice.forward(alice)
+
+    // actual test starts here
+    crossSign(alice, bob, alice2bob, bob2alice)
+    // depending on who starts signing first, there will be one or two commitments because both sides have changes
+    assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.index === 1)
+    assert(bob.stateData.asInstanceOf[DATA_NORMAL].commitments.localCommit.index === 2)
+    assert(alice.underlyingActor.nodeParams.channelsDb.listHtlcInfos(alice.stateData.asInstanceOf[DATA_NORMAL].channelId, 0).size == 0)
+    assert(alice.underlyingActor.nodeParams.channelsDb.listHtlcInfos(alice.stateData.asInstanceOf[DATA_NORMAL].channelId, 1).size == 2)
+    assert(alice.underlyingActor.nodeParams.channelsDb.listHtlcInfos(alice.stateData.asInstanceOf[DATA_NORMAL].channelId, 2).size == 4)
+    assert(bob.underlyingActor.nodeParams.channelsDb.listHtlcInfos(bob.stateData.asInstanceOf[DATA_NORMAL].channelId, 0).size == 0)
+    assert(bob.underlyingActor.nodeParams.channelsDb.listHtlcInfos(bob.stateData.asInstanceOf[DATA_NORMAL].channelId, 1).size == 3)
+  }
+
   test("recv CMD_SIGN (htlcs with same pubkeyScript but different amounts)") { f =>
     import f._
     val sender = TestProbe()

eclair-core/src/test/scala/fr/acinq/eclair/db/SqliteChannelsDbSpec.scala

@@ -54,15 +54,15 @@ class SqliteChannelsDbSpec extends FunSuite {
     db.addOrUpdateChannel(channel)
     assert(db.listChannels() === List(channel))
 
-    assert(db.listHtlcHtlcInfos(channel.channelId, commitNumber).toList == Nil)
+    assert(db.listHtlcInfos(channel.channelId, commitNumber).toList == Nil)
     db.addOrUpdateHtlcInfo(channel.channelId, commitNumber, paymentHash1, cltvExpiry1)
     db.addOrUpdateHtlcInfo(channel.channelId, commitNumber, paymentHash2, cltvExpiry2)
-    assert(db.listHtlcHtlcInfos(channel.channelId, commitNumber).toList == List((paymentHash1, cltvExpiry1), (paymentHash2, cltvExpiry2)))
-    assert(db.listHtlcHtlcInfos(channel.channelId, 43).toList == Nil)
+    assert(db.listHtlcInfos(channel.channelId, commitNumber).toList == List((paymentHash1, cltvExpiry1), (paymentHash2, cltvExpiry2)))
+    assert(db.listHtlcInfos(channel.channelId, 43).toList == Nil)
 
     db.removeChannel(channel.channelId)
     assert(db.listChannels() === Nil)
-    assert(db.listHtlcHtlcInfos(channel.channelId, commitNumber).toList == Nil)
+    assert(db.listHtlcInfos(channel.channelId, commitNumber).toList == Nil)
   }
 
 }