Refactor channel_type validation

As suggested by @sstone.

Author: t-bast

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

@@ -16,6 +16,7 @@
 
 package fr.acinq.eclair.channel
 
+import fr.acinq.bitcoin.scalacompat.ByteVector32
 import fr.acinq.eclair.transactions.Transactions._
 import fr.acinq.eclair.{ChannelTypeFeature, FeatureSupport, Features, InitFeature, PermanentChannelFeature}
 
@@ -140,15 +141,12 @@ object ChannelTypes {
   def fromFeatures(features: Features[InitFeature]): ChannelType = features2ChannelType.getOrElse(features, UnsupportedChannelType(features))
 
   /** Check if a given channel type is compatible with our features. */
-  def areCompatible(localFeatures: Features[InitFeature], remoteChannelType: ChannelType): Option[SupportedChannelType] = remoteChannelType match {
-    case _: UnsupportedChannelType => None
+  def areCompatible(channelId: ByteVector32, localFeatures: Features[InitFeature], remoteChannelType_opt: Option[ChannelType]): Either[ChannelException, SupportedChannelType] = remoteChannelType_opt match {
+    case None => Left(MissingChannelType(channelId))
+    case Some(channelType: UnsupportedChannelType) => Left(InvalidChannelType(channelId, channelType))
     // We ensure that we support the features necessary for this channel type.
-    case proposedChannelType: SupportedChannelType =>
-      if (proposedChannelType.features.forall(f => localFeatures.hasFeature(f))) {
-        Some(proposedChannelType)
-      } else {
-        None
-      }
+    case Some(proposedChannelType: SupportedChannelType) if proposedChannelType.features.forall(f => localFeatures.hasFeature(f)) => Right(proposedChannelType)
+    case Some(proposedChannelType: SupportedChannelType) => Left(InvalidChannelType(channelId, proposedChannelType))
   }
 
 }

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

@@ -99,7 +99,7 @@ object Helpers {
   }
 
   /** Called by the fundee of a single-funded channel. */
-  def validateParamsSingleFundedFundee(nodeParams: NodeParams, channelType: SupportedChannelType, localFeatures: Features[InitFeature], open: OpenChannel, remoteNodeId: PublicKey, remoteFeatures: Features[InitFeature]): Either[ChannelException, (ChannelFeatures, Option[ByteVector])] = {
+  def validateParamsSingleFundedFundee(nodeParams: NodeParams, localFeatures: Features[InitFeature], open: OpenChannel, remoteNodeId: PublicKey, remoteFeatures: Features[InitFeature]): Either[ChannelException, (ChannelFeatures, Option[ByteVector])] = {
     // 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) return Left(InvalidChainHash(open.temporaryChannelId, local = nodeParams.chainHash, remote = open.chainHash))
@@ -133,6 +133,10 @@ object Helpers {
       return Left(ChannelReserveNotMet(open.temporaryChannelId, toLocalMsat, toRemoteMsat, open.channelReserveSatoshis))
     }
 
+    val channelType = ChannelTypes.areCompatible(open.temporaryChannelId, localFeatures, open.channelType_opt) match {
+      case Left(f) => return Left(f)
+      case Right(proposedChannelType) => proposedChannelType
+    }
     val channelFeatures = ChannelFeatures(channelType, localFeatures, remoteFeatures, open.channelFlags.announceChannel)
     channelType.commitmentFormat match {
       case _: SimpleTaprootChannelCommitmentFormat => if (open.commitNonce_opt.isEmpty) return Left(MissingCommitNonce(open.temporaryChannelId, TxId(ByteVector32.Zeroes), commitmentNumber = 0))
@@ -154,7 +158,6 @@ object Helpers {
 
   /** Called by the non-initiator of a dual-funded channel. */
   def validateParamsDualFundedNonInitiator(nodeParams: NodeParams,
-                                           channelType: SupportedChannelType,
                                            open: OpenDualFundedChannel,
                                            fundingScript: ByteVector,
                                            remoteNodeId: PublicKey,
@@ -184,6 +187,10 @@ object Helpers {
     if (open.dustLimit < Channel.MIN_DUST_LIMIT) return Left(DustLimitTooSmall(open.temporaryChannelId, open.dustLimit, Channel.MIN_DUST_LIMIT))
     if (open.dustLimit > nodeParams.channelConf.maxRemoteDustLimit) return Left(DustLimitTooLarge(open.temporaryChannelId, open.dustLimit, nodeParams.channelConf.maxRemoteDustLimit))
 
+    val channelType = ChannelTypes.areCompatible(open.temporaryChannelId, localFeatures, open.channelType_opt) match {
+      case Left(f) => return Left(f)
+      case Right(proposedChannelType) => proposedChannelType
+    }
     val channelFeatures = ChannelFeatures(channelType, localFeatures, remoteFeatures, open.channelFlags.announceChannel)
 
     // BOLT #2: The receiving node MUST fail the channel if: it considers feerate_per_kw too small for timely processing or unreasonably large.
@@ -196,22 +203,19 @@ object Helpers {
     } yield (channelFeatures, script_opt, willFund_opt)
   }
 
-  private def validateChannelType(channelId: ByteVector32, channelType: SupportedChannelType, openChannelType_opt: Option[ChannelType], acceptChannelType_opt: Option[ChannelType]): Option[ChannelException] = {
-    if (openChannelType_opt.isEmpty || acceptChannelType_opt.isEmpty) {
-      Some(MissingChannelType(channelId))
-    } else if (!openChannelType_opt.contains(channelType) || !acceptChannelType_opt.contains(channelType)) {
-      Some(InvalidChannelType(channelId, acceptChannelType_opt.get))
-    } else {
-      // we agree on channel type
-      None
+  private def validateChannelTypeInitiator(channelId: ByteVector32, openChannelType_opt: Option[ChannelType], acceptChannelType_opt: Option[ChannelType]): Either[ChannelException, SupportedChannelType] = {
+    (openChannelType_opt, acceptChannelType_opt) match {
+      case (Some(proposed: SupportedChannelType), Some(received)) if proposed == received => Right(proposed)
+      case (Some(_), Some(received)) => Left(InvalidChannelType(channelId, received))
+      case _ => Left(MissingChannelType(channelId))
     }
   }
 
   /** Called by the funder of a single-funded channel. */
-  def validateParamsSingleFundedFunder(nodeParams: NodeParams, channelType: SupportedChannelType, localFeatures: Features[InitFeature], remoteFeatures: Features[InitFeature], open: OpenChannel, accept: AcceptChannel): Either[ChannelException, (ChannelFeatures, Option[ByteVector])] = {
-    validateChannelType(open.temporaryChannelId, channelType, open.channelType_opt, accept.channelType_opt) match {
-      case Some(t) => return Left(t)
-      case None => // we agree on channel type
+  def validateParamsSingleFundedFunder(nodeParams: NodeParams, localFeatures: Features[InitFeature], remoteFeatures: Features[InitFeature], open: OpenChannel, accept: AcceptChannel): Either[ChannelException, (ChannelFeatures, Option[ByteVector])] = {
+    val channelType = validateChannelTypeInitiator(open.temporaryChannelId, open.channelType_opt, accept.channelType_opt) match {
+      case Left(t) => return Left(t)
+      case Right(channelType) => channelType
     }
 
     if (accept.maxAcceptedHtlcs > Channel.MAX_ACCEPTED_HTLCS) return Left(InvalidMaxAcceptedHtlcs(accept.temporaryChannelId, accept.maxAcceptedHtlcs, Channel.MAX_ACCEPTED_HTLCS))
@@ -248,14 +252,13 @@ object Helpers {
   /** Called by the initiator of a dual-funded channel. */
   def validateParamsDualFundedInitiator(nodeParams: NodeParams,
                                         remoteNodeId: PublicKey,
-                                        channelType: SupportedChannelType,
                                         localFeatures: Features[InitFeature],
                                         remoteFeatures: Features[InitFeature],
                                         open: OpenDualFundedChannel,
                                         accept: AcceptDualFundedChannel): Either[ChannelException, (ChannelFeatures, Option[ByteVector], Option[LiquidityAds.Purchase])] = {
-    validateChannelType(open.temporaryChannelId, channelType, open.channelType_opt, accept.channelType_opt) match {
-      case Some(t) => return Left(t)
-      case None => // we agree on channel type
+    val channelType = validateChannelTypeInitiator(open.temporaryChannelId, open.channelType_opt, accept.channelType_opt) match {
+      case Left(t) => return Left(t)
+      case Right(channelType) => channelType
     }
 
     // BOLT #2: Channel funding limits

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

@@ -141,7 +141,7 @@ trait ChannelOpenDualFunded extends DualFundingHandlers with ErrorHandlers {
     case Event(open: OpenDualFundedChannel, d: DATA_WAIT_FOR_OPEN_DUAL_FUNDED_CHANNEL) =>
       val localFundingPubkey = channelKeys.fundingKey(fundingTxIndex = 0).publicKey
       val fundingScript = Transactions.makeFundingScript(localFundingPubkey, open.fundingPubkey, d.init.channelType.commitmentFormat).pubkeyScript
-      Helpers.validateParamsDualFundedNonInitiator(nodeParams, d.init.channelType, open, fundingScript, remoteNodeId, d.init.localChannelParams.initFeatures, d.init.remoteInit.features, d.init.fundingContribution_opt) match {
+      Helpers.validateParamsDualFundedNonInitiator(nodeParams, open, fundingScript, remoteNodeId, d.init.localChannelParams.initFeatures, d.init.remoteInit.features, d.init.fundingContribution_opt) match {
         case Left(t) => handleLocalError(t, d, Some(open))
         case Right((channelFeatures, remoteShutdownScript, willFund_opt)) =>
           context.system.eventStream.publish(ChannelCreated(self, peer, remoteNodeId, isOpener = false, open.temporaryChannelId, open.commitmentFeerate, Some(open.fundingFeerate)))
@@ -224,7 +224,7 @@ trait ChannelOpenDualFunded extends DualFundingHandlers with ErrorHandlers {
 
   when(WAIT_FOR_ACCEPT_DUAL_FUNDED_CHANNEL)(handleExceptions {
     case Event(accept: AcceptDualFundedChannel, d: DATA_WAIT_FOR_ACCEPT_DUAL_FUNDED_CHANNEL) =>
-      Helpers.validateParamsDualFundedInitiator(nodeParams, remoteNodeId, d.init.channelType, d.init.localChannelParams.initFeatures, d.init.remoteInit.features, d.lastSent, accept) match {
+      Helpers.validateParamsDualFundedInitiator(nodeParams, remoteNodeId, d.init.localChannelParams.initFeatures, d.init.remoteInit.features, d.lastSent, accept) match {
         case Left(t) =>
           d.init.replyTo ! OpenChannelResponse.Rejected(t.getMessage)
           handleLocalError(t, d, Some(accept))

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

@@ -112,7 +112,7 @@ trait ChannelOpenSingleFunded extends SingleFundingHandlers with ErrorHandlers {
 
   when(WAIT_FOR_OPEN_CHANNEL)(handleExceptions {
     case Event(open: OpenChannel, d: DATA_WAIT_FOR_OPEN_CHANNEL) =>
-      Helpers.validateParamsSingleFundedFundee(nodeParams, d.initFundee.channelType, d.initFundee.localChannelParams.initFeatures, open, remoteNodeId, d.initFundee.remoteInit.features) match {
+      Helpers.validateParamsSingleFundedFundee(nodeParams, d.initFundee.localChannelParams.initFeatures, open, remoteNodeId, d.initFundee.remoteInit.features) match {
         case Left(t) => handleLocalError(t, d, Some(open))
         case Right((channelFeatures, remoteShutdownScript)) =>
           context.system.eventStream.publish(ChannelCreated(self, peer, remoteNodeId, isOpener = false, open.temporaryChannelId, open.feeratePerKw, None))
@@ -168,7 +168,7 @@ trait ChannelOpenSingleFunded extends SingleFundingHandlers with ErrorHandlers {
 
   when(WAIT_FOR_ACCEPT_CHANNEL)(handleExceptions {
     case Event(accept: AcceptChannel, d: DATA_WAIT_FOR_ACCEPT_CHANNEL) =>
-      Helpers.validateParamsSingleFundedFunder(nodeParams, d.initFunder.channelType, d.initFunder.localChannelParams.initFeatures, d.initFunder.remoteInit.features, d.lastSent, accept) match {
+      Helpers.validateParamsSingleFundedFunder(nodeParams, d.initFunder.localChannelParams.initFeatures, d.initFunder.remoteInit.features, d.lastSent, accept) match {
         case Left(t) =>
           d.initFunder.replyTo ! OpenChannelResponse.Rejected(t.getMessage)
           handleLocalError(t, d, Some(accept))

eclair-core/src/main/scala/fr/acinq/eclair/io/OpenChannelInterceptor.scala

@@ -132,7 +132,7 @@ private class OpenChannelInterceptor(peer: ActorRef[Any],
   }
 
   private def sanityCheckNonInitiator(request: OpenChannelNonInitiator): Behavior[Command] = {
-    validateRemoteChannelType(request.temporaryChannelId, request.channelType_opt, request.localFeatures) match {
+    ChannelTypes.areCompatible(request.temporaryChannelId, request.localFeatures, request.channelType_opt) match {
       case Right(channelType) =>
         val dualFunded = Features.canUseFeature(request.localFeatures, request.remoteFeatures, Features.DualFunding)
         val upfrontShutdownScript = Features.canUseFeature(request.localFeatures, request.remoteFeatures, Features.UpfrontShutdownScript)
@@ -274,17 +274,6 @@ private class OpenChannelInterceptor(peer: ActorRef[Any],
     }
   }
 
-  private def validateRemoteChannelType(temporaryChannelId: ByteVector32, remoteChannelType_opt: Option[ChannelType], localFeatures: Features[InitFeature]): Either[ChannelException, SupportedChannelType] = {
-    remoteChannelType_opt match {
-      // remote explicitly specifies a channel type: we check whether we want to allow it
-      case Some(remoteChannelType) => ChannelTypes.areCompatible(localFeatures, remoteChannelType) match {
-        case Some(acceptedChannelType) => Right(acceptedChannelType)
-        case None => Left(InvalidChannelType(temporaryChannelId, remoteChannelType))
-      }
-      case None => Left(MissingChannelType(temporaryChannelId))
-    }
-  }
-
   private def createLocalParams(nodeParams: NodeParams, initFeatures: Features[InitFeature], upfrontShutdownScript: Boolean, channelType: SupportedChannelType, isChannelOpener: Boolean, paysCommitTxFees: Boolean, dualFunded: Boolean, fundingAmount: Satoshi): LocalChannelParams = {
     makeChannelParams(
       nodeParams, initFeatures,

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

@@ -260,7 +260,7 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DualFunding))(_.updated(Features.DualFunding, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.SimpleClose))(_.updated(Features.SimpleClose, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.AnchorOutputsPhoenix))(_.removed(Features.AnchorOutputsZeroFeeHtlcTx).updated(Features.AnchorOutputs, FeatureSupport.Optional))
-      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.OptionSimpleTaprootPhoenix))(_.removed(Features.SimpleTaprootChannelsStaging).updated(Features.SimpleTaprootChannelsPhoenix, FeatureSupport.Optional))
+      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.OptionSimpleTaprootPhoenix))(_.removed(Features.SimpleTaprootChannelsStaging).updated(Features.SimpleTaprootChannelsPhoenix, FeatureSupport.Optional).updated(Features.PhoenixZeroReserve, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.OptionSimpleTaproot))(_.updated(Features.SimpleTaprootChannelsStaging, FeatureSupport.Optional))
     )
     val nodeParamsB1 = nodeParamsB.copy(features = nodeParamsB.features
@@ -272,7 +272,7 @@ trait ChannelStateTestsBase extends Assertions with Eventually {
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.SimpleClose))(_.updated(Features.SimpleClose, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.DisableSplice))(_.removed(Features.SplicePrototype))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.AnchorOutputsPhoenix))(_.removed(Features.AnchorOutputsZeroFeeHtlcTx).updated(Features.AnchorOutputs, FeatureSupport.Optional))
-      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.OptionSimpleTaprootPhoenix))(_.removed(Features.SimpleTaprootChannelsStaging).updated(Features.SimpleTaprootChannelsPhoenix, FeatureSupport.Optional))
+      .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.OptionSimpleTaprootPhoenix))(_.removed(Features.SimpleTaprootChannelsStaging).updated(Features.SimpleTaprootChannelsPhoenix, FeatureSupport.Optional).updated(Features.PhoenixZeroReserve, FeatureSupport.Optional))
       .modify(_.activated).usingIf(tags.contains(ChannelStateTestsTags.OptionSimpleTaproot))(_.updated(Features.SimpleTaprootChannelsStaging, FeatureSupport.Optional))
     )
     (nodeParamsA1, nodeParamsB1)

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

@@ -259,7 +259,7 @@ class WaitForOpenChannelStateSpec extends TestKitBaseClass with FixtureAnyFunSui
   test("recv OpenChannel (empty upfront shutdown script)", Tag(ChannelStateTestsTags.UpfrontShutdownScript)) { f =>
     import f._
     val open = alice2bob.expectMsgType[OpenChannel]
-    val open1 = open.copy(tlvStream = TlvStream(ChannelTlv.UpfrontShutdownScriptTlv(ByteVector.empty)))
+    val open1 = open.copy(tlvStream = TlvStream(open.tlvStream.records.filterNot(_.isInstanceOf[ChannelTlv.UpfrontShutdownScriptTlv]) + ChannelTlv.UpfrontShutdownScriptTlv(ByteVector.empty)))
     alice2bob.forward(bob, open1)
     awaitCond(bob.stateName == WAIT_FOR_FUNDING_CREATED)
     assert(bob.stateData.asInstanceOf[DATA_WAIT_FOR_FUNDING_CREATED].channelParams.remoteParams.upfrontShutdownScript_opt.isEmpty)