Use channel_reestablish tlv when sending channel_ready (#3025)

When the remote node supports the 'your_last_funding_locked' tlv we do not need to always resend `channel_ready` when there are no channel updates. The tlv removes the ambiguity about whether our last channel_ready was received.

We also changed how init features are computed so that in the testing framework `init` `reachNormal` and `reconnect` can all use the same set of init features defined by the testing tags.

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

@@ -2357,12 +2357,12 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
       }
 
     case Event(_: ChannelReestablish, d: DATA_WAIT_FOR_CHANNEL_READY) =>
-      log.debug("re-sending channelReady")
+      log.debug("re-sending channel_ready")
       val channelReady = createChannelReady(d.aliases, d.commitments.params)
       goto(WAIT_FOR_CHANNEL_READY) sending channelReady
 
     case Event(_: ChannelReestablish, d: DATA_WAIT_FOR_DUAL_FUNDING_READY) =>
-      log.debug("re-sending channelReady")
+      log.debug("re-sending channel_ready")
       val channelReady = createChannelReady(d.aliases, d.commitments.params)
       goto(WAIT_FOR_DUAL_FUNDING_READY) sending channelReady
 
@@ -2374,15 +2374,37 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
           var sendQueue = Queue.empty[LightningMessage]
           // normal case, our data is up-to-date
 
-          // re-send channel_ready if necessary
-          if (d.commitments.latest.fundingTxIndex == 0 && channelReestablish.nextLocalCommitmentNumber == 1 && d.commitments.localCommitIndex == 0) {
-            // If next_local_commitment_number is 1 in both the channel_reestablish it sent and received, then the node MUST retransmit channel_ready, otherwise it MUST NOT
-            // TODO: when the remote node enables option_splice we can use your_last_funding_locked to detect they did not receive our channel_ready.
-            log.debug("re-sending channelReady")
-            val channelKeyPath = keyManager.keyPath(d.commitments.params.localParams, d.commitments.params.channelConfig)
-            val nextPerCommitmentPoint = keyManager.commitmentPoint(channelKeyPath, 1)
-            val channelReady = ChannelReady(d.commitments.channelId, nextPerCommitmentPoint)
-            sendQueue = sendQueue :+ channelReady
+          // re-send channel_ready and announcement_signatures if necessary
+          d.commitments.lastLocalLocked_opt match {
+            case None => ()
+            // We only send channel_ready for initial funding transactions.
+            case Some(c) if c.fundingTxIndex != 0 => ()
+            case Some(c) =>
+              val remoteSpliceSupport = d.commitments.params.remoteParams.initFeatures.hasFeature(Features.SplicePrototype)
+              // If our peer has not received our channel_ready, we retransmit it.
+              val notReceivedByRemote = remoteSpliceSupport && channelReestablish.yourLastFundingLocked_opt.isEmpty
+              // If next_local_commitment_number is 1 in both the channel_reestablish it sent and received, then the node
+              // MUST retransmit channel_ready, otherwise it MUST NOT
+              val notReceivedByRemoteLegacy = !remoteSpliceSupport && channelReestablish.nextLocalCommitmentNumber == 1 && c.localCommit.index == 0
+              // If this is a public channel and we haven't announced the channel, we retransmit our channel_ready and
+              // will also send announcement_signatures.
+              val notAnnouncedYet = d.commitments.announceChannel && c.shortChannelId_opt.nonEmpty && d.lastAnnouncement_opt.isEmpty
+              if (notAnnouncedYet || notReceivedByRemote || notReceivedByRemoteLegacy) {
+                log.debug("re-sending channel_ready")
+                val channelKeyPath = keyManager.keyPath(d.commitments.params.localParams, d.commitments.params.channelConfig)
+                val nextPerCommitmentPoint = keyManager.commitmentPoint(channelKeyPath, 1)
+                sendQueue = sendQueue :+ ChannelReady(d.commitments.channelId, nextPerCommitmentPoint)
+              }
+              if (notAnnouncedYet) {
+                // The funding transaction is confirmed, so we've already sent our announcement_signatures.
+                // We haven't announced the channel yet, which means we haven't received our peer's announcement_signatures.
+                // We retransmit our announcement_signatures to let our peer know that we're ready to announce the channel.
+                val localAnnSigs = c.signAnnouncement(nodeParams, d.commitments.params)
+                localAnnSigs.foreach(annSigs => {
+                  announcementSigsSent += annSigs.shortChannelId
+                  sendQueue = sendQueue :+ annSigs
+                })
+              }
           }
 
           // resume splice signing session if any
@@ -2433,26 +2455,25 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
             // We then clean up unsigned updates that haven't been received before the disconnection.
             .discardUnsignedUpdates()
 
-          val spliceLocked_opt = commitments1.lastLocalLocked_opt match {
-            case None => None
+          commitments1.lastLocalLocked_opt match {
+            case None => ()
             // We only send splice_locked for splice transactions.
-            case Some(c) if c.fundingTxIndex == 0 => None
+            case Some(c) if c.fundingTxIndex == 0 => ()
             case Some(c) =>
               // If our peer has not received our splice_locked, we retransmit it.
               val notReceivedByRemote = !channelReestablish.yourLastFundingLocked_opt.contains(c.fundingTxId)
               // If this is a public channel and we haven't announced the splice, we retransmit our splice_locked and
               // will exchange announcement_signatures afterwards.
               val notAnnouncedYet = commitments1.announceChannel && d.lastAnnouncement_opt.forall(ann => !c.shortChannelId_opt.contains(ann.shortChannelId))
               if (notReceivedByRemote || notAnnouncedYet) {
+                // Retransmission of local announcement_signatures for splices are done when receiving splice_locked, no need
+                // to retransmit here.
                 log.debug("re-sending splice_locked for fundingTxId={}", c.fundingTxId)
                 spliceLockedSent += (c.fundingTxId -> c.fundingTxIndex)
                 trimSpliceLockedSentIfNeeded()
-                Some(SpliceLocked(d.channelId, c.fundingTxId))
-              } else {
-                None
+                sendQueue = sendQueue :+ SpliceLocked(d.channelId, c.fundingTxId)
               }
           }
-          sendQueue = sendQueue ++ spliceLocked_opt.toSeq
 
           // we may need to retransmit updates and/or commit_sig and/or revocation
           sendQueue = sendQueue ++ syncSuccess.retransmit
@@ -2472,23 +2493,10 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
           // BOLT 2: A node if it has sent a previous shutdown MUST retransmit shutdown.
           d.localShutdown.foreach {
             localShutdown =>
-              log.debug("re-sending localShutdown")
+              log.debug("re-sending local_shutdown")
               sendQueue = sendQueue :+ localShutdown
           }
 
-          // Retransmission of local announcement_signatures for splices are done when receiving splice_locked, no need
-          // to retransmit here.
-          d.commitments.all.find(_.fundingTxIndex == 0) match {
-            case Some(c) if d.commitments.announceChannel && c.shortChannelId_opt.nonEmpty && d.lastAnnouncement_opt.isEmpty =>
-              // The funding transaction is confirmed, so we've already sent our announcement_signatures.
-              // We haven't announced the channel yet, which means we haven't received our peer's announcement_signatures.
-              // We retransmit our announcement_signatures to let our peer know that we're ready to announce the channel.
-              val localAnnSigs_opt = c.signAnnouncement(nodeParams, d.commitments.params)
-              localAnnSigs_opt.foreach(annSigs => announcementSigsSent += annSigs.shortChannelId)
-              sendQueue = sendQueue ++ localAnnSigs_opt.toSeq
-            case _ => ()
-          }
-
           if (d.commitments.announceChannel) {
             // we will re-enable the channel after some delay to prevent flappy updates in case the connection is unstable
             startSingleTimer(Reconnected.toString, BroadcastChannelUpdate(Reconnected), 10 seconds)

eclair-core/src/test/scala/fr/acinq/eclair/channel/RestoreSpec.scala

@@ -4,21 +4,14 @@ import akka.actor.ActorRef
 import akka.actor.typed.scaladsl.adapter.actorRefAdapter
 import akka.testkit.{TestActor, TestFSMRef, TestProbe}
 import com.softwaremill.quicklens.ModifyPimp
-import fr.acinq.bitcoin
-import fr.acinq.bitcoin.ScriptFlags
-import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
 import fr.acinq.bitcoin.scalacompat._
 import fr.acinq.eclair.TestConstants.{Alice, Bob}
 import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher.WatchFundingSpentTriggered
 import fr.acinq.eclair.channel.fsm.Channel
 import fr.acinq.eclair.channel.states.{ChannelStateTestsBase, ChannelStateTestsTags}
 import fr.acinq.eclair.channel.states.ChannelStateTestsBase.FakeTxPublisherFactory
-import fr.acinq.eclair.crypto.Generators
-import fr.acinq.eclair.crypto.keymanager.ChannelKeyManager
 import fr.acinq.eclair.router.Announcements
-import fr.acinq.eclair.transactions.Scripts
-import fr.acinq.eclair.transactions.Transactions.{ClaimP2WPKHOutputTx, DefaultCommitmentFormat, InputInfo, TxOwner}
-import fr.acinq.eclair.wire.protocol.{ChannelReady, ChannelReestablish, ChannelUpdate, CommitSig, Error, Init, RevokeAndAck}
+import fr.acinq.eclair.wire.protocol.{ChannelReestablish, ChannelUpdate, CommitSig, Error, Init, RevokeAndAck}
 import fr.acinq.eclair.{TestKitBaseClass, _}
 import org.scalatest.{Outcome, Tag}
 import org.scalatest.funsuite.FixtureAnyFunSuiteLike
@@ -177,7 +170,7 @@ class RestoreSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with Chan
     // and we terminate Alice
     alice.stop()
 
-    // there should ne no pending messages
+    // there should be no pending messages
     alice2bob.expectNoMessage()
     bob2alice.expectNoMessage()
 
@@ -209,10 +202,6 @@ class RestoreSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with Chan
       bob2alice.expectMsgType[ChannelReestablish]
       alice2bob.forward(bob)
       bob2alice.forward(newAlice)
-      alice2bob.expectMsgType[ChannelReady]
-      bob2alice.expectMsgType[ChannelReady]
-      alice2bob.forward(bob)
-      bob2alice.forward(newAlice)
       alice2bob.expectMsgType[ChannelUpdate]
       bob2alice.expectMsgType[ChannelUpdate]
       alice2bob.expectNoMessage()

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

@@ -98,6 +98,8 @@ object ChannelStateTestsTags {
   val AdaptMaxHtlcAmount = "adapt_max_htlc_amount"
   /** If set, closing will use option_simple_close. */
   val SimpleClose = "option_simple_close"
+  /** If set, disable option_splice for one node. */
+  val DisableSplice = "disable_splice"
 }
 
 trait ChannelStateTestsBase extends Assertions with Eventually {
@@ -144,7 +146,8 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
     systemB.eventStream.subscribe(channelUpdateListener.ref, classOf[LocalChannelUpdate])
     systemB.eventStream.subscribe(channelUpdateListener.ref, classOf[LocalChannelDown])
     val router = TestProbe()
-    val finalNodeParamsA = nodeParamsA
+    val (nodeParamsA1, nodeParamsB1) = updateInitFeatures(nodeParamsA, nodeParamsB, tags)
+    val finalNodeParamsA = nodeParamsA1
       .modify(_.channelConf.dustLimit).setToIf(tags.contains(ChannelStateTestsTags.HighDustLimitDifferenceAliceBob))(5000 sat)
       .modify(_.channelConf.dustLimit).setToIf(tags.contains(ChannelStateTestsTags.HighDustLimitDifferenceBobAlice))(1000 sat)
       .modify(_.channelConf.maxRemoteDustLimit).setToIf(tags.contains(ChannelStateTestsTags.HighDustLimitDifferenceAliceBob))(10000 sat)
@@ -153,7 +156,7 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
       .modify(_.onChainFeeConf.defaultFeerateTolerance.ratioHigh).setToIf(tags.contains(ChannelStateTestsTags.HighFeerateMismatchTolerance))(1000000)
       .modify(_.onChainFeeConf.spendAnchorWithoutHtlcs).setToIf(tags.contains(ChannelStateTestsTags.DontSpendAnchorWithoutHtlcs))(false)
       .modify(_.channelConf.balanceThresholds).setToIf(tags.contains(ChannelStateTestsTags.AdaptMaxHtlcAmount))(Seq(Channel.BalanceThreshold(1_000 sat, 0 sat), Channel.BalanceThreshold(5_000 sat, 1_000 sat), Channel.BalanceThreshold(10_000 sat, 5_000 sat)))
-    val finalNodeParamsB = nodeParamsB
+    val finalNodeParamsB = nodeParamsB1
       .modify(_.channelConf.dustLimit).setToIf(tags.contains(ChannelStateTestsTags.HighDustLimitDifferenceAliceBob))(1000 sat)
       .modify(_.channelConf.dustLimit).setToIf(tags.contains(ChannelStateTestsTags.HighDustLimitDifferenceBobAlice))(5000 sat)
       .modify(_.channelConf.maxRemoteDustLimit).setToIf(tags.contains(ChannelStateTestsTags.HighDustLimitDifferenceAliceBob))(10000 sat)
@@ -179,10 +182,8 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
     SetupFixture(alice, bob, aliceOpenReplyTo, alice2bob, bob2alice, alice2blockchain, bob2blockchain, router, alice2relayer, bob2relayer, channelUpdateListener, wallet, alicePeer, bobPeer)
   }
 
-  def computeFeatures(setup: SetupFixture, tags: Set[String], channelFlags: ChannelFlags): (LocalParams, LocalParams, SupportedChannelType) = {
-    import setup._
-
-    val aliceInitFeatures = Alice.nodeParams.features
+  def updateInitFeatures(nodeParamsA: NodeParams, nodeParamsB: NodeParams, tags: Set[String]): (NodeParams, NodeParams) = {
+    (nodeParamsA.copy(features = nodeParamsA.features
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DisableWumbo))(_.removed(Features.Wumbo))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.StaticRemoteKey))(_.updated(Features.StaticRemoteKey, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.AnchorOutputs))(_.updated(Features.StaticRemoteKey, FeatureSupport.Optional).updated(Features.AnchorOutputs, FeatureSupport.Optional))
@@ -193,9 +194,8 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.ZeroConf))(_.updated(Features.ZeroConf, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.ScidAlias))(_.updated(Features.ScidAlias, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DualFunding))(_.updated(Features.DualFunding, FeatureSupport.Optional))
-      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.SimpleClose))(_.updated(Features.SimpleClose, FeatureSupport.Optional))
-      .initFeatures()
-    val bobInitFeatures = Bob.nodeParams.features
+      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.SimpleClose))(_.updated(Features.SimpleClose, FeatureSupport.Optional))),
+    nodeParamsB.copy(features = nodeParamsB.features
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DisableWumbo))(_.removed(Features.Wumbo))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.StaticRemoteKey))(_.updated(Features.StaticRemoteKey, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.AnchorOutputs))(_.updated(Features.StaticRemoteKey, FeatureSupport.Optional).updated(Features.AnchorOutputs, FeatureSupport.Optional))
@@ -207,7 +207,16 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.ScidAlias))(_.updated(Features.ScidAlias, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DualFunding))(_.updated(Features.DualFunding, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.SimpleClose))(_.updated(Features.SimpleClose, FeatureSupport.Optional))
-      .initFeatures()
+      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DisableSplice))(_.removed(Features.SplicePrototype))
+      ))
+  }
+
+  def computeFeatures(setup: SetupFixture, tags: Set[String], channelFlags: ChannelFlags): (LocalParams, LocalParams, SupportedChannelType) = {
+    import setup._
+
+    val (nodeParamsA, nodeParamsB) = updateInitFeatures(alice.underlyingActor.nodeParams, bob.underlyingActor.nodeParams, tags)
+    val aliceInitFeatures = nodeParamsA.features.initFeatures()
+    val bobInitFeatures = nodeParamsB.features.initFeatures()
 
     val channelType = ChannelTypes.defaultFromFeatures(aliceInitFeatures, bobInitFeatures, announceChannel = channelFlags.announceChannel)
 

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/c/WaitForDualFundingReadyStateSpec.scala

@@ -244,10 +244,10 @@ class WaitForDualFundingReadyStateSpec extends TestKitBaseClass with FixtureAnyF
     alice2bob.forward(bob)
     bob2alice.expectMsgType[ChannelReestablish]
     bob2alice.forward(alice)
-    bob2alice.expectMsgType[ChannelReady]
-    bob2alice.forward(alice)
+    // Bob does not retransmit channel_ready and announcement_signatures because he has already received both of them from Alice.
     bob2alice.expectNoMessage(100 millis)
-    // When receiving channel_ready, Bob retransmits announcement_signatures.
+    // Alice has already received Bob's channel_ready, but not its announcement_signatures.
+    // She retransmits channel_ready and Bob will retransmit its announcement_signatures in response.
     alice2bob.expectMsgType[ChannelReady]
     alice2bob.forward(bob)
     alice2bob.expectMsgType[AnnouncementSignatures]

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

@@ -87,14 +87,14 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
     r2s.expectMsgType[TxComplete]
     r2s.forward(s)
     if (spliceIn_opt.isDefined) {
-      s2r.expectMsgType[TxAddInput]
-      s2r.forward(r)
-      r2s.expectMsgType[TxComplete]
-      r2s.forward(s)
-      s2r.expectMsgType[TxAddOutput]
-      s2r.forward(r)
-      r2s.expectMsgType[TxComplete]
-      r2s.forward(s)
+        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) {
       s2r.expectMsgType[TxAddOutput]
@@ -366,7 +366,7 @@ class NormalSplicesStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLik
   }
 
   test("recv CMD_SPLICE (splice-in, non dual-funded channel)") { () =>
-    val f = init(tags = Set(ChannelStateTestsTags.DualFunding))
+    val f = init(tags = Set.empty, wallet_opt = Some(new SingleKeyOnChainWallet()))
     import f._
 
     reachNormal(f, tags = Set.empty) // we open a non dual-funded channel