From 04aa5cf6618f8e73ab2098c516034357494c4c79 Mon Sep 17 00:00:00 2001 From: t-bast Date: Wed, 8 Jan 2025 10:54:17 +0100 Subject: [PATCH] Address @remyers comments - limit the number of stashed `announcement_signature`s - move `ANNOUNCEMENTS_MINCONF` to `Router.scala` - add `do_not_intercept_gossip` test tag --- .../fr/acinq/eclair/channel/fsm/Channel.scala | 10 +++++++--- .../scala/fr/acinq/eclair/router/Router.scala | 4 +++- .../states/ChannelStateTestsHelperMethods.scala | 11 ++++++++--- .../channel/states/e/NormalStateSpec.scala | 16 ++++++++-------- .../channel/states/f/ShutdownStateSpec.scala | 4 ++-- .../channel/states/g/NegotiatingStateSpec.scala | 4 ++-- .../channel/states/h/ClosingStateSpec.scala | 6 +++--- .../router/ChannelRouterIntegrationSpec.scala | 2 +- 8 files changed, 34 insertions(+), 23 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala index ef75b4555a..b083bb4046 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala +++ b/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() } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala b/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala index 561116fe3c..b6171d87c3 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala +++ b/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, diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala index 72fe4c5d56..bc7f5b0f1d 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/ChannelStateTestsHelperMethods.scala +++ b/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] diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala index 613ed858fe..bdbaead2cd 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala +++ b/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] diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/f/ShutdownStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/f/ShutdownStateSpec.scala index ba7d50ec0f..227b77dc98 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/f/ShutdownStateSpec.scala +++ b/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]) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala index 9c28378c17..675ac1fced 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala +++ b/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() diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala index ea0d5bba5b..36a819b674 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala +++ b/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() diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/router/ChannelRouterIntegrationSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/router/ChannelRouterIntegrationSpec.scala index cbd3d92af3..d3941792dd 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/router/ChannelRouterIntegrationSpec.scala +++ b/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)