From 20e0b4bc4de840edca3339e4246cd807ccd8cb2f Mon Sep 17 00:00:00 2001 From: Pierre-Marie Padiou Date: Thu, 8 Oct 2020 17:59:01 +0200 Subject: [PATCH] Fix override-features implementation (#1549) We were calling `nodeParams.features` from inside the channel, which is problematic because we may have overridden those features for specific peers. This is now fixed. --- .../scala/fr/acinq/eclair/NodeParams.scala | 4 ++- .../fr/acinq/eclair/channel/Channel.scala | 2 +- .../fr/acinq/eclair/channel/Helpers.scala | 4 +-- .../main/scala/fr/acinq/eclair/io/Peer.scala | 28 ++++++++++--------- .../scala/fr/acinq/eclair/io/PeerEvents.scala | 2 +- .../fr/acinq/eclair/io/Switchboard.scala | 5 +--- .../scala/fr/acinq/eclair/StartupSpec.scala | 2 +- .../scala/fr/acinq/eclair/TestConstants.scala | 2 ++ .../a/WaitForAcceptChannelStateSpec.scala | 11 ++++---- .../a/WaitForOpenChannelStateSpec.scala | 6 ++-- 10 files changed, 36 insertions(+), 30 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala b/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala index b92859ec0..d6171a96f 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala @@ -51,7 +51,7 @@ case class NodeParams(keyManager: KeyManager, color: Color, publicAddresses: List[NodeAddress], features: Features, - overrideFeatures: Map[PublicKey, Features], + private val overrideFeatures: Map[PublicKey, Features], syncWhitelist: Set[PublicKey], pluginParams: Seq[PluginParams], dustLimit: Satoshi, @@ -92,6 +92,8 @@ case class NodeParams(keyManager: KeyManager, val keyPair = KeyPair(nodeId.value, privateKey.value) def currentBlockHeight: Long = blockCount.get + + def featuresFor(nodeId: PublicKey) = overrideFeatures.getOrElse(nodeId, features) } object NodeParams { diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala index b4c6ab871..158d80d54 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala @@ -289,7 +289,7 @@ class Channel(val nodeParams: NodeParams, val wallet: EclairWallet, remoteNodeId when(WAIT_FOR_OPEN_CHANNEL)(handleExceptions { case Event(open: OpenChannel, d@DATA_WAIT_FOR_OPEN_CHANNEL(INPUT_INIT_FUNDEE(_, localParams, _, remoteInit, channelVersion))) => log.info("received OpenChannel={}", open) - Try(Helpers.validateParamsFundee(nodeParams, open)) match { + Try(Helpers.validateParamsFundee(nodeParams, localParams.features, open)) match { case Failure(t) => handleLocalError(t, d, Some(open)) case Success(_) => context.system.eventStream.publish(ChannelCreated(self, peer, remoteNodeId, isFunder = false, open.temporaryChannelId, open.feeratePerKw, None)) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala index 56de79988..c7b97ed0b 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala @@ -95,7 +95,7 @@ object Helpers { /** * Called by the fundee */ - def validateParamsFundee(nodeParams: NodeParams, open: OpenChannel): Unit = { + def validateParamsFundee(nodeParams: NodeParams, features: Features, open: OpenChannel): Unit = { // BOLT #2: if the chain_hash value, within the open_channel, message is set to a hash of a chain that is unknown to the receiver: // MUST reject the channel. if (nodeParams.chainHash != open.chainHash) throw InvalidChainHash(open.temporaryChannelId, local = nodeParams.chainHash, remote = open.chainHash) @@ -103,7 +103,7 @@ object Helpers { if (open.fundingSatoshis < nodeParams.minFundingSatoshis || open.fundingSatoshis > nodeParams.maxFundingSatoshis) throw InvalidFundingAmount(open.temporaryChannelId, open.fundingSatoshis, nodeParams.minFundingSatoshis, nodeParams.maxFundingSatoshis) // BOLT #2: Channel funding limits - if (open.fundingSatoshis >= Channel.MAX_FUNDING && !nodeParams.features.hasFeature(Features.Wumbo)) throw InvalidFundingAmount(open.temporaryChannelId, open.fundingSatoshis, nodeParams.minFundingSatoshis, Channel.MAX_FUNDING) + if (open.fundingSatoshis >= Channel.MAX_FUNDING && !features.hasFeature(Features.Wumbo)) throw InvalidFundingAmount(open.temporaryChannelId, open.fundingSatoshis, nodeParams.minFundingSatoshis, Channel.MAX_FUNDING) // BOLT #2: The receiving node MUST fail the channel if: push_msat is greater than funding_satoshis * 1000. if (open.pushMsat > open.fundingSatoshis) throw InvalidPushAmount(open.temporaryChannelId, open.pushMsat, open.fundingSatoshis.toMilliSatoshi) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala b/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala index 67c07de90..9036d8f59 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala @@ -112,18 +112,18 @@ class Peer(val nodeParams: NodeParams, remoteNodeId: PublicKey, watcher: ActorRe stay case Event(c: Peer.OpenChannel, d: ConnectedData) => - if (c.fundingSatoshis >= Channel.MAX_FUNDING && !nodeParams.features.hasFeature(Wumbo)) { + if (c.fundingSatoshis >= Channel.MAX_FUNDING && !d.localFeatures.hasFeature(Wumbo)) { sender ! Status.Failure(new RuntimeException(s"fundingSatoshis=${c.fundingSatoshis} is too big, you must enable large channels support in 'eclair.features' to use funding above ${Channel.MAX_FUNDING} (see eclair.conf)")) stay - } else if (c.fundingSatoshis >= Channel.MAX_FUNDING && !d.remoteInit.features.hasFeature(Wumbo)) { + } else if (c.fundingSatoshis >= Channel.MAX_FUNDING && !d.remoteFeatures.hasFeature(Wumbo)) { sender ! Status.Failure(new RuntimeException(s"fundingSatoshis=${c.fundingSatoshis} is too big, the remote peer doesn't support wumbo")) stay } else if (c.fundingSatoshis > nodeParams.maxFundingSatoshis) { sender ! Status.Failure(new RuntimeException(s"fundingSatoshis=${c.fundingSatoshis} is too big for the current settings, increase 'eclair.max-funding-satoshis' (see eclair.conf)")) stay } else { - val channelVersion = ChannelVersion.pickChannelVersion(d.localInit.features, d.remoteInit.features) - val (channel, localParams) = createNewChannel(nodeParams, funder = true, c.fundingSatoshis, origin_opt = Some(sender), channelVersion) + val channelVersion = ChannelVersion.pickChannelVersion(d.localFeatures, d.remoteFeatures) + val (channel, localParams) = createNewChannel(nodeParams, d.localFeatures, funder = true, c.fundingSatoshis, origin_opt = Some(sender), channelVersion) c.timeout_opt.map(openTimeout => context.system.scheduler.scheduleOnce(openTimeout.duration, channel, Channel.TickChannelOpenTimeout)(context.dispatcher)) val temporaryChannelId = randomBytes32 val channelFeeratePerKw = nodeParams.onChainFeeConf.feeEstimator.getFeeratePerKw(target = nodeParams.onChainFeeConf.feeTargets.commitmentBlockTarget) @@ -136,8 +136,8 @@ class Peer(val nodeParams: NodeParams, remoteNodeId: PublicKey, watcher: ActorRe case Event(msg: wire.OpenChannel, d: ConnectedData) => d.channels.get(TemporaryChannelId(msg.temporaryChannelId)) match { case None => - val channelVersion = ChannelVersion.pickChannelVersion(d.localInit.features, d.remoteInit.features) - val (channel, localParams) = createNewChannel(nodeParams, funder = false, fundingAmount = msg.fundingSatoshis, origin_opt = None, channelVersion) + val channelVersion = ChannelVersion.pickChannelVersion(d.localFeatures, d.remoteFeatures) + val (channel, localParams) = createNewChannel(nodeParams, d.localFeatures, funder = false, fundingAmount = msg.fundingSatoshis, origin_opt = None, channelVersion) val temporaryChannelId = msg.temporaryChannelId log.info(s"accepting a new channel with temporaryChannelId=$temporaryChannelId localParams=$localParams") channel ! INPUT_INIT_FUNDEE(temporaryChannelId, localParams, d.peerConnection, d.remoteInit, channelVersion) @@ -282,7 +282,7 @@ class Peer(val nodeParams: NodeParams, remoteNodeId: PublicKey, watcher: ActorRe s(e) } - def createNewChannel(nodeParams: NodeParams, funder: Boolean, fundingAmount: Satoshi, origin_opt: Option[ActorRef], channelVersion: ChannelVersion): (ActorRef, LocalParams) = { + def createNewChannel(nodeParams: NodeParams, features: Features, funder: Boolean, fundingAmount: Satoshi, origin_opt: Option[ActorRef], channelVersion: ChannelVersion): (ActorRef, LocalParams) = { val (finalScript, walletStaticPaymentBasepoint) = channelVersion match { case v if v.paysDirectlyToWallet => val walletKey = Helpers.getWalletPaymentBasepoint(wallet) @@ -290,7 +290,7 @@ class Peer(val nodeParams: NodeParams, remoteNodeId: PublicKey, watcher: ActorRe case _ => (Helpers.getFinalScriptPubKey(wallet, nodeParams.chainHash), None) } - val localParams = makeChannelParams(nodeParams, finalScript, walletStaticPaymentBasepoint, funder, fundingAmount) + val localParams = makeChannelParams(nodeParams, features, finalScript, walletStaticPaymentBasepoint, funder, fundingAmount) val channel = spawnChannel(nodeParams, origin_opt) (channel, localParams) } @@ -367,7 +367,9 @@ object Peer { case object Nothing extends Data { override def channels = Map.empty } case class DisconnectedData(channels: Map[FinalChannelId, ActorRef]) extends Data case class ConnectedData(address: InetSocketAddress, peerConnection: ActorRef, localInit: wire.Init, remoteInit: wire.Init, channels: Map[ChannelId, ActorRef]) extends Data { - val connectionInfo: ConnectionInfo = ConnectionInfo(peerConnection, remoteInit) + val connectionInfo: ConnectionInfo = ConnectionInfo(peerConnection, localInit, remoteInit) + def localFeatures: Features = localInit.features + def remoteFeatures: Features = remoteInit.features } sealed trait State @@ -399,13 +401,13 @@ object Peer { // @formatter:on - def makeChannelParams(nodeParams: NodeParams, defaultFinalScriptPubkey: ByteVector, walletStaticPaymentBasepoint: Option[PublicKey], isFunder: Boolean, fundingAmount: Satoshi): LocalParams = { + def makeChannelParams(nodeParams: NodeParams, features: Features, defaultFinalScriptPubkey: ByteVector, walletStaticPaymentBasepoint: Option[PublicKey], isFunder: Boolean, fundingAmount: Satoshi): LocalParams = { // we make sure that funder and fundee key path end differently val fundingKeyPath = nodeParams.keyManager.newFundingKeyPath(isFunder) - makeChannelParams(nodeParams, defaultFinalScriptPubkey, walletStaticPaymentBasepoint, isFunder, fundingAmount, fundingKeyPath) + makeChannelParams(nodeParams, features, defaultFinalScriptPubkey, walletStaticPaymentBasepoint, isFunder, fundingAmount, fundingKeyPath) } - def makeChannelParams(nodeParams: NodeParams, defaultFinalScriptPubkey: ByteVector, walletStaticPaymentBasepoint: Option[PublicKey], isFunder: Boolean, fundingAmount: Satoshi, fundingKeyPath: DeterministicWallet.KeyPath): LocalParams = { + def makeChannelParams(nodeParams: NodeParams, features: Features, defaultFinalScriptPubkey: ByteVector, walletStaticPaymentBasepoint: Option[PublicKey], isFunder: Boolean, fundingAmount: Satoshi, fundingKeyPath: DeterministicWallet.KeyPath): LocalParams = { LocalParams( nodeParams.nodeId, fundingKeyPath, @@ -418,6 +420,6 @@ object Peer { isFunder = isFunder, defaultFinalScriptPubKey = defaultFinalScriptPubkey, walletStaticPaymentBasepoint = walletStaticPaymentBasepoint, - features = nodeParams.features) + features = features) } } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerEvents.scala b/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerEvents.scala index a946fde56..6f78dd161 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerEvents.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerEvents.scala @@ -23,7 +23,7 @@ import fr.acinq.eclair.wire sealed trait PeerEvent -case class ConnectionInfo(peerConnection: ActorRef, remoteInit: wire.Init) +case class ConnectionInfo(peerConnection: ActorRef, localInit: wire.Init, remoteInit: wire.Init) case class PeerConnected(peer: ActorRef, nodeId: PublicKey, connectionInfo: ConnectionInfo) extends PeerEvent diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/io/Switchboard.scala b/eclair-core/src/main/scala/fr/acinq/eclair/io/Switchboard.scala index b8a8eae71..2373cd097 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/io/Switchboard.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/io/Switchboard.scala @@ -72,10 +72,7 @@ class Switchboard(nodeParams: NodeParams, watcher: ActorRef, relayer: ActorRef, case authenticated: PeerConnection.Authenticated => // if this is an incoming connection, we might not yet have created the peer val peer = createOrGetPeer(authenticated.remoteNodeId, offlineChannels = Set.empty) - val features = nodeParams.overrideFeatures.get(authenticated.remoteNodeId) match { - case Some(f) => f - case None => nodeParams.features.maskFeaturesForEclairMobile() - } + val features = nodeParams.featuresFor(authenticated.remoteNodeId).maskFeaturesForEclairMobile() val doSync = nodeParams.syncWhitelist.isEmpty || nodeParams.syncWhitelist.contains(authenticated.remoteNodeId) authenticated.peerConnection ! PeerConnection.InitializeConnection(peer, nodeParams.chainHash, features, doSync) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala index 6c78bbfef..3aec13397 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala @@ -130,7 +130,7 @@ class StartupSpec extends AnyFunSuite { ) val nodeParams = makeNodeParamsWithDefaults(perNodeConf.withFallback(defaultConf)) - val perNodeFeatures = nodeParams.overrideFeatures(PublicKey(ByteVector.fromValidHex("02aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))) + val perNodeFeatures = nodeParams.featuresFor(PublicKey(ByteVector.fromValidHex("02aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))) assert(perNodeFeatures.hasFeature(BasicMultiPartPayment, Some(Mandatory))) } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala b/eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala index 8f537bed2..599ea8c37 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala @@ -224,6 +224,7 @@ object TestConstants { def channelParams = Peer.makeChannelParams( nodeParams, + nodeParams.features, Script.write(Script.pay2wpkh(PrivateKey(randomBytes32).publicKey)), None, isFunder = true, @@ -317,6 +318,7 @@ object TestConstants { def channelParams = Peer.makeChannelParams( nodeParams, + nodeParams.features, Script.write(Script.pay2wpkh(PrivateKey(randomBytes32).publicKey)), None, isFunder = false, diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala index b8a901be0..a2ded5954 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala @@ -49,21 +49,22 @@ class WaitForAcceptChannelStateSpec extends TestKitBaseClass with FixtureAnyFunS } import com.softwaremill.quicklens._ - val aliceNodeParams = TestConstants.Alice.nodeParams + val aliceNodeParams = Alice.nodeParams .modify(_.chainHash).setToIf(test.tags.contains("mainnet"))(Block.LivenetGenesisBlock.hash) - .modify(_.features).setToIf(test.tags.contains("wumbo"))(Features(Set(ActivatedFeature(Wumbo, Optional)))) .modify(_.maxFundingSatoshis).setToIf(test.tags.contains("high-max-funding-size"))(Btc(100)) + val aliceParams = Alice.channelParams + .modify(_.features).setToIf(test.tags.contains("wumbo"))(Features(Set(ActivatedFeature(Wumbo, Optional)))) - val bobNodeParams = TestConstants.Bob.nodeParams + val bobNodeParams = Bob.nodeParams .modify(_.chainHash).setToIf(test.tags.contains("mainnet"))(Block.LivenetGenesisBlock.hash) - .modify(_.features).setToIf(test.tags.contains("wumbo"))(Features(Set(ActivatedFeature(Wumbo, Optional)))) .modify(_.maxFundingSatoshis).setToIf(test.tags.contains("high-max-funding-size"))(Btc(100)) + val bobParams = Bob.channelParams + .modify(_.features).setToIf(test.tags.contains("wumbo"))(Features(Set(ActivatedFeature(Wumbo, Optional)))) val setup = init(aliceNodeParams, bobNodeParams, wallet = noopWallet) import setup._ val channelVersion = ChannelVersion.STANDARD - val (aliceParams, bobParams) = (Alice.channelParams, Bob.channelParams) val aliceInit = Init(aliceParams.features) val bobInit = Init(bobParams.features) within(30 seconds) { diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala index ba2b45463..1983a18cb 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala @@ -42,15 +42,17 @@ class WaitForOpenChannelStateSpec extends TestKitBaseClass with FixtureAnyFunSui override def withFixture(test: OneArgTest): Outcome = { import com.softwaremill.quicklens._ + val aliceParams = Alice.channelParams + val bobNodeParams = Bob.nodeParams - .modify(_.features).setToIf(test.tags.contains("wumbo"))(Features(Set(ActivatedFeature(Wumbo, Optional)))) .modify(_.maxFundingSatoshis).setToIf(test.tags.contains("max-funding-satoshis"))(Btc(1)) + val bobParams = Bob.channelParams + .modify(_.features).setToIf(test.tags.contains("wumbo"))(Features(Set(ActivatedFeature(Wumbo, Optional)))) val setup = init(nodeParamsB = bobNodeParams) import setup._ val channelVersion = ChannelVersion.STANDARD - val (aliceParams, bobParams) = (Alice.channelParams, Bob.channelParams) val aliceInit = Init(aliceParams.features) val bobInit = Init(bobParams.features) within(30 seconds) {