Address @remyers comments

- limit the number of stashed `announcement_signature`s
- move `ANNOUNCEMENTS_MINCONF` to `Router.scala`
- add `do_not_intercept_gossip` test tag

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

@@ -126,9 +126,6 @@ object Channel {
   def props(nodeParams: NodeParams, wallet: OnChainChannelFunder with OnchainPubkeyCache, remoteNodeId: PublicKey, blockchain: typed.ActorRef[ZmqWatcher.Command], relayer: ActorRef, txPublisherFactory: TxPublisherFactory): Props =
     Props(new Channel(nodeParams, wallet, remoteNodeId, blockchain, relayer, txPublisherFactory))
 
-  // see https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#requirements
-  val ANNOUNCEMENTS_MINCONF = 6
-
   // https://github.com/lightningnetwork/lightning-rfc/blob/master/02-peer-protocol.md#requirements
   val MAX_FUNDING_WITHOUT_WUMBO: Satoshi = 16777216 sat // = 2^24
   val MAX_ACCEPTED_HTLCS = 483
@@ -799,6 +796,13 @@ class Channel(val nodeParams: NodeParams, val wallet: OnChainChannelFunder with
           // it when we receive WatchFundingConfirmedTriggered. We don't need to persist this message, it will be
           // retransmitted on reconnection.
           log.debug("received remote announcement signatures for scid={}, delaying", remoteAnnSigs.shortChannelId)
+          if (announcementSigsStash.size >= 10) {
+            // We shouldn't store an unbounded number of announcement_signatures for scids that we don't have in our
+            // commitments, otherwise it can be used as a DoS vector.
+            val oldestScid = announcementSigsStash.keys.minBy(ShortChannelId.blockHeight(_))
+            log.warning("too many pending announcement_signatures: dropping scid={}", oldestScid)
+            announcementSigsStash -= oldestScid
+          }
           announcementSigsStash += (remoteAnnSigs.shortChannelId -> remoteAnnSigs)
           stay()
       }

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala

@@ -28,7 +28,6 @@ import fr.acinq.eclair._
 import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher
 import fr.acinq.eclair.blockchain.bitcoind.ZmqWatcher._
 import fr.acinq.eclair.channel._
-import fr.acinq.eclair.channel.fsm.Channel.ANNOUNCEMENTS_MINCONF
 import fr.acinq.eclair.crypto.TransportHandler
 import fr.acinq.eclair.db.NetworkDb
 import fr.acinq.eclair.io.Peer.PeerRoutingMessage
@@ -343,6 +342,9 @@ class Router(val nodeParams: NodeParams, watcher: typed.ActorRef[ZmqWatcher.Comm
 
 object Router {
 
+  // see https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#requirements
+  val ANNOUNCEMENTS_MINCONF = 6
+
   def props(nodeParams: NodeParams, watcher: typed.ActorRef[ZmqWatcher.Command], initialized: Option[Promise[Done]] = None) = Props(new Router(nodeParams, watcher, initialized))
 
   case class SearchBoundaries(maxFeeFlat: MilliSatoshi,

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

@@ -63,6 +63,8 @@ object ChannelStateTestsTags {
   val ShutdownAnySegwit = "shutdown_anysegwit"
   /** If set, channels will be public (otherwise we don't announce them by default). */
   val ChannelsPublic = "channels_public"
+  /** If set, initial announcement_signatures and channel_updates will not be intercepted and ignored. */
+  val DoNotInterceptGossip = "do_not_intercept_gossip"
   /** If set, no amount will be pushed when opening a channel (by default the initiator pushes a small amount). */
   val NoPushAmount = "no_push_amount"
   /** If set, the non-initiator will push a small amount when opening a dual-funded channel. */
@@ -230,8 +232,7 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
     (aliceParams, bobParams, channelType)
   }
 
-  def reachNormal(setup: SetupFixture, tags: Set[String] = Set.empty, interceptChannelUpdates: Boolean = true): Transaction = {
-
+  def reachNormal(setup: SetupFixture, tags: Set[String] = Set.empty): Transaction = {
     import setup._
 
     val channelConfig = ChannelConfig.standard
@@ -355,7 +356,11 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
       fundingTx
     }
 
-    if (interceptChannelUpdates && !tags.contains(ChannelStateTestsTags.ChannelsPublic)) {
+    if (!tags.contains(ChannelStateTestsTags.DoNotInterceptGossip)) {
+      if (tags.contains(ChannelStateTestsTags.ChannelsPublic) && !channelType.features.contains(Features.ZeroConf)) {
+        alice2bob.expectMsgType[AnnouncementSignatures]
+        bob2alice.expectMsgType[AnnouncementSignatures]
+      }
       // we don't forward the channel updates, in reality they would be processed by the router
       alice2bob.expectMsgType[ChannelUpdate]
       bob2alice.expectMsgType[ChannelUpdate]

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

@@ -911,7 +911,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.remoteNextCommitInfo == Left(waitForRevocation))
   }
 
-  test("recv CMD_SIGN (going above balance threshold)", Tag(ChannelStateTestsTags.NoPushAmount), Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.AdaptMaxHtlcAmount)) { f =>
+  test("recv CMD_SIGN (going above balance threshold)", Tag(ChannelStateTestsTags.NoPushAmount), Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip), Tag(ChannelStateTestsTags.AdaptMaxHtlcAmount)) { f =>
     import f._
 
     val aliceListener = TestProbe()
@@ -3594,7 +3594,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(addSettled.htlc == htlc1)
   }
 
-  test("recv WatchFundingConfirmedTriggered (public channel, zero-conf)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs), Tag(ChannelStateTestsTags.ZeroConf)) { f =>
+  test("recv WatchFundingConfirmedTriggered (public channel, zero-conf)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip), Tag(ChannelStateTestsTags.AnchorOutputsZeroFeeHtlcTxs), Tag(ChannelStateTestsTags.ZeroConf)) { f =>
     import f._
     // For zero-conf channels we don't have a real short_channel_id when going to the NORMAL state.
     val aliceState = alice.stateData.asInstanceOf[DATA_NORMAL]
@@ -3646,7 +3646,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     listener.expectMsgType[TransactionConfirmed]
   }
 
-  test("recv AnnouncementSignatures", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv AnnouncementSignatures", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
     val initialState = alice.stateData.asInstanceOf[DATA_NORMAL]
     val realShortChannelId = initialState.shortIds.real_opt.get
@@ -3680,7 +3680,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     channelUpdateListener.expectNoMessage(100 millis)
   }
 
-  test("recv AnnouncementSignatures (invalid)", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv AnnouncementSignatures (invalid)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
     val channelId = alice.stateData.asInstanceOf[DATA_NORMAL].channelId
     alice2bob.expectMsgType[AnnouncementSignatures]
@@ -3696,7 +3696,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     awaitCond(alice.stateName == CLOSING)
   }
 
-  test("recv BroadcastChannelUpdate", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv BroadcastChannelUpdate", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
     val realScid = bob2alice.expectMsgType[AnnouncementSignatures].shortChannelId
     bob2alice.forward(alice)
@@ -3711,7 +3711,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(update1.channelUpdate.timestamp < update2.channelUpdate.timestamp)
   }
 
-  test("recv BroadcastChannelUpdate (no changes)", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv BroadcastChannelUpdate (no changes)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
     bob2alice.expectMsgType[AnnouncementSignatures]
     bob2alice.forward(alice)
@@ -3758,7 +3758,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     awaitCond(alice.stateName == OFFLINE)
   }
 
-  test("recv INPUT_DISCONNECTED (public channel)", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv INPUT_DISCONNECTED (public channel)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
     bob2alice.expectMsgType[AnnouncementSignatures]
     bob2alice.forward(alice)
@@ -3771,7 +3771,7 @@ class NormalStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     channelUpdateListener.expectNoMessage(1 second)
   }
 
-  test("recv INPUT_DISCONNECTED (public channel, with pending unsigned htlcs)", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv INPUT_DISCONNECTED (public channel, with pending unsigned htlcs)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
     val sender = TestProbe()
     bob2alice.expectMsgType[AnnouncementSignatures]

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

@@ -102,11 +102,11 @@ class ShutdownStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike wit
     }
   }
 
-  test("emit disabled channel update", Tag(ChannelStateTestsTags.ChannelsPublic)) { () =>
+  test("emit disabled channel update", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { () =>
     val setup = init()
     import setup._
     within(30 seconds) {
-      reachNormal(setup, Set(ChannelStateTestsTags.ChannelsPublic))
+      reachNormal(setup, Set(ChannelStateTestsTags.ChannelsPublic, ChannelStateTestsTags.DoNotInterceptGossip))
 
       val aliceListener = TestProbe()
       systemA.eventStream.subscribe(aliceListener.ref, classOf[LocalChannelUpdate])

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

@@ -30,7 +30,7 @@ import fr.acinq.eclair.transactions.Transactions
 import fr.acinq.eclair.transactions.Transactions.ZeroFeeHtlcTxAnchorOutputsCommitmentFormat
 import fr.acinq.eclair.wire.protocol.ClosingSignedTlv.FeeRange
 import fr.acinq.eclair.wire.protocol.{AnnouncementSignatures, ChannelUpdate, ClosingSigned, Error, Shutdown, TlvStream, Warning}
-import fr.acinq.eclair.{BlockHeight, CltvExpiry, Features, MilliSatoshiLong, TestConstants, TestKitBaseClass, randomBytes32}
+import fr.acinq.eclair.{CltvExpiry, Features, MilliSatoshiLong, TestConstants, TestKitBaseClass, randomBytes32}
 import org.scalatest.funsuite.FixtureAnyFunSuiteLike
 import org.scalatest.{Outcome, Tag}
 
@@ -89,7 +89,7 @@ class NegotiatingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
   def buildFeerates(feerate: FeeratePerKw, minFeerate: FeeratePerKw = FeeratePerKw(250 sat)): FeeratesPerKw =
     FeeratesPerKw.single(feerate).copy(minimum = minFeerate, slow = minFeerate)
 
-  test("emit disabled channel update", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("emit disabled channel update", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
 
     val aliceListener = TestProbe()

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

@@ -104,7 +104,7 @@ class ClosingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     } else {
       within(30 seconds) {
         reachNormal(setup, test.tags)
-        if (test.tags.contains(ChannelStateTestsTags.ChannelsPublic)) {
+        if (test.tags.contains(ChannelStateTestsTags.ChannelsPublic) && test.tags.contains(ChannelStateTestsTags.DoNotInterceptGossip)) {
           alice2bob.expectMsgType[AnnouncementSignatures]
           alice2bob.forward(bob)
           alice2bob.expectMsgType[ChannelUpdate]
@@ -357,7 +357,7 @@ class ClosingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(alice.stateData == initialState) // this was a no-op
   }
 
-  test("recv WatchFundingSpentTriggered (local commit, public channel)", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv WatchFundingSpentTriggered (local commit, public channel)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
 
     val listener = TestProbe()
@@ -838,7 +838,7 @@ class ClosingStateSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike with
     assert(txPublished.miningFee > 0.sat) // alice is funder, she pays the fee for the remote commit
   }
 
-  test("recv WatchFundingSpentTriggered (remote commit, public channel)", Tag(ChannelStateTestsTags.ChannelsPublic)) { f =>
+  test("recv WatchFundingSpentTriggered (remote commit, public channel)", Tag(ChannelStateTestsTags.ChannelsPublic), Tag(ChannelStateTestsTags.DoNotInterceptGossip)) { f =>
     import f._
 
     val listener = TestProbe()

eclair-core/src/test/scala/fr/acinq/eclair/router/ChannelRouterIntegrationSpec.scala

@@ -46,7 +46,7 @@ class ChannelRouterIntegrationSpec extends TestKitBaseClass with FixtureAnyFunSu
     import f._
 
     val (aliceNodeId, bobNodeId) = (channels.alice.underlyingActor.nodeParams.nodeId, channels.bob.underlyingActor.nodeParams.nodeId)
-    val fundingTx = reachNormal(channels, testTags, interceptChannelUpdates = false)
+    val fundingTx = reachNormal(channels, testTags + ChannelStateTestsTags.DoNotInterceptGossip)
 
     // The router learns about the local, still unannounced, channel.
     awaitCond(router.stateData.privateChannels.size == 1)