Clarify some comments and add tests (#1734)

This commit clarifies some parts of the code on which we regularly have
questions during pull requests.

We also add a test for an edge case in shutdown that was correctly handled,
but not properly tested, to ensure non-regression.

Author: t-bast

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

@@ -267,9 +267,10 @@ object Commitments {
    * @return either Left(failure, error message) where failure is a failure message (see BOLT #4 and the Failure Message class) or Right(new commitments, updateAddHtlc)
    */
   def sendAdd(commitments: Commitments, cmd: CMD_ADD_HTLC, blockHeight: Long, feeConf: OnChainFeeConf): Either[ChannelException, (Commitments, UpdateAddHtlc)] = {
-    // our counterparty needs a reasonable amount of time to pull the funds from downstream before we can get refunded (see BOLT 2 and BOLT 11 for a calculation and rationale)
-    val minExpiry = CltvExpiry(blockHeight)
-    if (cmd.cltvExpiry <= minExpiry) {
+    // we must ensure we're not relaying htlcs that are already expired, otherwise the downstream channel will instantly close
+    // NB: we add a 3 blocks safety to reduce the probability of running into this when our bitcoin node is slightly outdated
+    val minExpiry = CltvExpiry(blockHeight + 3)
+    if (cmd.cltvExpiry < minExpiry) {
       return Left(ExpiryTooSmall(commitments.channelId, minimum = minExpiry, actual = cmd.cltvExpiry, blockCount = blockHeight))
     }
     // we don't want to use too high a refund timeout, because our funds will be locked during that time if the payment is never fulfilled

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

@@ -147,7 +147,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     val expiryTooSmall = CltvExpiry(currentBlockHeight)
     val add = CMD_ADD_HTLC(sender.ref, 500000000 msat, randomBytes32, expiryTooSmall, TestConstants.emptyOnionPacket, localOrigin(sender.ref))
     alice ! add
-    val error = ExpiryTooSmall(channelId(alice), CltvExpiry(currentBlockHeight), expiryTooSmall, currentBlockHeight)
+    val error = ExpiryTooSmall(channelId(alice), CltvExpiry(currentBlockHeight + 3), expiryTooSmall, currentBlockHeight)
     sender.expectMsg(RES_ADD_FAILED(add, error, Some(initialState.channelUpdate)))
     alice2bob.expectNoMsg(200 millis)
   }

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/f/ShutdownStateSpec.scala

@@ -28,7 +28,7 @@ import fr.acinq.eclair.payment._
 import fr.acinq.eclair.payment.relay.Relayer._
 import fr.acinq.eclair.router.Router.ChannelHop
 import fr.acinq.eclair.wire.protocol.Onion.FinalLegacyPayload
-import fr.acinq.eclair.wire.protocol.{CommitSig, Error, FailureMessageCodecs, PermanentChannelFailure, RevokeAndAck, Shutdown, UpdateAddHtlc, UpdateFailHtlc, UpdateFailMalformedHtlc, UpdateFee, UpdateFulfillHtlc}
+import fr.acinq.eclair.wire.protocol.{ClosingSigned, CommitSig, Error, FailureMessageCodecs, PermanentChannelFailure, RevokeAndAck, Shutdown, UpdateAddHtlc, UpdateFailHtlc, UpdateFailMalformedHtlc, UpdateFee, UpdateFulfillHtlc}
 import fr.acinq.eclair.{CltvExpiry, CltvExpiryDelta, MilliSatoshiLong, TestConstants, TestKitBaseClass, randomBytes32}
 import org.scalatest.funsuite.FixtureAnyFunSuiteLike
 import org.scalatest.{Outcome, Tag}
@@ -792,6 +792,48 @@ class ShutdownStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike wit
     assert(alice.stateData.asInstanceOf[DATA_CLOSING].revokedCommitPublished.size == 1)
   }
 
+  test("recv BITCOIN_FUNDING_SPENT (revoked tx with updated commitment)") { f =>
+    import f._
+    val initialCommitTx = bob.stateData.asInstanceOf[DATA_SHUTDOWN].commitments.localCommit.publishableTxs.commitTx.tx
+    assert(initialCommitTx.txOut.size === 4) // two main outputs + 2 htlc
+
+    // bob fulfills one of the pending htlc (commitment update while in shutdown state)
+    fulfillHtlc(0, r1, bob, alice, bob2alice, alice2bob)
+    crossSign(bob, alice, bob2alice, alice2bob)
+    val revokedTx = bob.stateData.asInstanceOf[DATA_SHUTDOWN].commitments.localCommit.publishableTxs.commitTx.tx
+    assert(revokedTx.txOut.size === 3) // two main outputs + 1 htlc
+
+    // bob fulfills the second pending htlc (and revokes the previous commitment)
+    fulfillHtlc(1, r2, bob, alice, bob2alice, alice2bob)
+    crossSign(bob, alice, bob2alice, alice2bob)
+    alice2bob.expectMsgType[ClosingSigned] // no more htlcs in the commitment
+
+    // bob published the revoked tx
+    alice ! WatchEventSpent(BITCOIN_FUNDING_SPENT, revokedTx)
+    alice2bob.expectMsgType[Error]
+
+    val mainTx = alice2blockchain.expectMsgType[PublishAsap].tx
+    val mainPenaltyTx = alice2blockchain.expectMsgType[PublishAsap].tx
+    val htlcPenaltyTx = alice2blockchain.expectMsgType[PublishAsap].tx
+    assert(alice2blockchain.expectMsgType[WatchConfirmed].event == BITCOIN_TX_CONFIRMED(revokedTx))
+    assert(alice2blockchain.expectMsgType[WatchConfirmed].event == BITCOIN_TX_CONFIRMED(mainTx))
+    assert(alice2blockchain.expectMsgType[WatchSpent].event === BITCOIN_OUTPUT_SPENT) // main-penalty
+    assert(alice2blockchain.expectMsgType[WatchSpent].event === BITCOIN_OUTPUT_SPENT) // htlc-penalty
+    alice2blockchain.expectNoMsg(1 second)
+
+    Transaction.correctlySpends(mainTx, Seq(revokedTx), ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
+    Transaction.correctlySpends(mainPenaltyTx, Seq(revokedTx), ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
+    Transaction.correctlySpends(htlcPenaltyTx, Seq(revokedTx), ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS)
+
+    // two main outputs are 300 000 and 200 000, htlcs are 300 000 and 200 000
+    assert(mainTx.txOut(0).amount === 286660.sat)
+    assert(mainPenaltyTx.txOut(0).amount === 495160.sat)
+    assert(htlcPenaltyTx.txOut(0).amount === 194540.sat)
+
+    awaitCond(alice.stateName == CLOSING)
+    assert(alice.stateData.asInstanceOf[DATA_CLOSING].revokedCommitPublished.size == 1)
+  }
+
   test("recv CMD_CLOSE") { f =>
     import f._
     val sender = TestProbe()