Add hints when features check fail (#2777)

This makes it much easier to diagnose incompatibilities.

Author: pm47

eclair-core/src/main/scala/fr/acinq/eclair/Features.scala

@@ -77,6 +77,20 @@ trait ChannelTypeFeature extends PermanentChannelFeature
 
 case class UnknownFeature(bitIndex: Int)
 
+// @formatter:off
+sealed trait FeatureCompatibilityResult {
+  def areCompatible: Boolean = this == FeatureCompatibilityResult.Compatible
+  def errorHints: Set[String] = this match {
+    case FeatureCompatibilityResult.Compatible => Set.empty
+    case r: FeatureCompatibilityResult.NotCompatible => r.hints
+  }
+}
+object FeatureCompatibilityResult {
+  case object Compatible extends FeatureCompatibilityResult
+  case class NotCompatible(hints: Set[String]) extends FeatureCompatibilityResult
+}
+// @formatter:on
+
 case class Features[T <: Feature](activated: Map[T, FeatureSupport], unknown: Set[UnknownFeature] = Set.empty) {
 
   def isEmpty: Boolean = activated.isEmpty && unknown.isEmpty
@@ -87,17 +101,20 @@ case class Features[T <: Feature](activated: Map[T, FeatureSupport], unknown: Se
   }
 
   /** NB: this method is not reflexive, see [[Features.areCompatible]] if you want symmetric validation. */
-  def areSupported(remoteFeatures: Features[T]): Boolean = {
+  def testSupported(remoteFeatures: Features[T]): FeatureCompatibilityResult = {
     // we allow unknown odd features (it's ok to be odd)
-    val unknownFeaturesOk = remoteFeatures.unknown.forall(_.bitIndex % 2 == 1)
+    val incompatibleUnknownFeatures = remoteFeatures.unknown.filter(_.bitIndex % 2 == 0)
     // we verify that we activated every mandatory feature they require
-    val knownFeaturesOk = remoteFeatures.activated.forall {
-      case (_, Optional) => true
-      case (feature, Mandatory) => hasFeature(feature)
-    }
-    unknownFeaturesOk && knownFeaturesOk
+    val incompatibleKnownFeatures = remoteFeatures.activated.filter {
+      case (_, Optional) => false
+      case (feature, Mandatory) => !hasFeature(feature)
+    }.keySet
+    val incompatibleFeatures = incompatibleUnknownFeatures.map(u => s"unknown_${u.bitIndex}") ++ incompatibleKnownFeatures.map(_.rfcName)
+    if (incompatibleFeatures.isEmpty) FeatureCompatibilityResult.Compatible else FeatureCompatibilityResult.NotCompatible(incompatibleFeatures)
   }
 
+  def areSupported(remoteFeatures: Features[T]): Boolean = testSupported(remoteFeatures).areCompatible
+
   def initFeatures(): Features[InitFeature] = Features(activated.collect { case (f: InitFeature, s) => (f, s) }, unknown)
 
   def nodeAnnouncementFeatures(): Features[NodeFeature] = Features(activated.collect { case (f: NodeFeature, s) => (f, s) }, unknown)
@@ -354,8 +371,13 @@ object Features {
       FeatureException(s"$feature is set but is missing a dependency (${dependencies.filter(d => !features.unscoped().hasFeature(d)).mkString(" and ")})")
   }
 
+  def testCompatible[T <: Feature](ours: Features[T], theirs: Features[T]): FeatureCompatibilityResult = (ours.testSupported(theirs), theirs.testSupported(ours)) match {
+    case (FeatureCompatibilityResult.Compatible, FeatureCompatibilityResult.Compatible) => FeatureCompatibilityResult.Compatible
+    case (r1, r2) => FeatureCompatibilityResult.NotCompatible(r1.errorHints ++ r2.errorHints)
+  }
+
   /** Returns true if both feature sets are compatible. */
-  def areCompatible[T <: Feature](ours: Features[T], theirs: Features[T]): Boolean = ours.areSupported(theirs) && theirs.areSupported(ours)
+  def areCompatible[T <: Feature](ours: Features[T], theirs: Features[T]): Boolean = testCompatible(ours, theirs).areCompatible
 
   /** returns true if both have at least optional support */
   def canUseFeature[T <: Feature](localFeatures: Features[T], remoteFeatures: Features[T], feature: T): Boolean = {

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

@@ -28,7 +28,7 @@ import fr.acinq.eclair.remote.EclairInternalsSerializer.RemoteTypes
 import fr.acinq.eclair.router.Router._
 import fr.acinq.eclair.wire.protocol
 import fr.acinq.eclair.wire.protocol._
-import fr.acinq.eclair.{FSMDiagnosticActorLogging, Features, InitFeature, Logs, TimestampMilli, TimestampSecond}
+import fr.acinq.eclair.{FSMDiagnosticActorLogging, FeatureCompatibilityResult, Features, InitFeature, Logs, TimestampMilli, TimestampSecond}
 import scodec.Attempt
 import scodec.bits.ByteVector
 
@@ -132,6 +132,7 @@ class PeerConnection(keyPair: KeyPair, conf: PeerConnection.Conf, switchboard: A
         remoteInit.remoteAddress_opt.foreach(address => log.info("peer reports that our IP address is {} (public={})", address.toString, NodeAddress.isPublicIPAddress(address)))
 
         val featureGraphErr_opt = Features.validateFeatureGraph(remoteInit.features)
+        val featuresCompatibilityResult = Features.testCompatible(d.localInit.features, remoteInit.features)
         if (remoteInit.networks.nonEmpty && remoteInit.networks.intersect(d.localInit.networks).isEmpty) {
           log.warning(s"incompatible networks (${remoteInit.networks}), disconnecting")
           d.pendingAuth.origin_opt.foreach(_ ! ConnectionResult.InitializationFailed("incompatible networks"))
@@ -143,9 +144,9 @@ class PeerConnection(keyPair: KeyPair, conf: PeerConnection.Conf, switchboard: A
           d.pendingAuth.origin_opt.foreach(_ ! ConnectionResult.InitializationFailed(featureGraphErr.message))
           d.transport ! PoisonPill
           stay()
-        } else if (!Features.areCompatible(d.localInit.features, remoteInit.features)) {
-          log.warning("incompatible features, disconnecting")
-          d.pendingAuth.origin_opt.foreach(_ ! ConnectionResult.InitializationFailed("incompatible features"))
+        } else if (!featuresCompatibilityResult.areCompatible) {
+          log.warning(s"incompatible features (${featuresCompatibilityResult.errorHints.mkString(",")}), disconnecting")
+          d.pendingAuth.origin_opt.foreach(_ ! ConnectionResult.InitializationFailed(s"incompatible features (${featuresCompatibilityResult.errorHints.mkString(",")})"))
           d.transport ! PoisonPill
           stay()
         } else {

eclair-core/src/test/scala/fr/acinq/eclair/io/PeerConnectionSpec.scala

@@ -153,7 +153,7 @@ class PeerConnectionSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike wi
     transport.send(peerConnection, LightningMessageCodecs.initCodec.decode(hex"0000 00050100000000".bits).require.value)
     transport.expectMsgType[TransportHandler.ReadAck]
     probe.expectTerminated(transport.ref)
-    origin.expectMsg(PeerConnection.ConnectionResult.InitializationFailed("incompatible features"))
+    origin.expectMsg(PeerConnection.ConnectionResult.InitializationFailed("incompatible features (unknown_32,payment_secret,var_onion_optin)"))
     peer.expectMsg(ConnectionDown(peerConnection))
   }
 
@@ -170,7 +170,7 @@ class PeerConnectionSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike wi
     transport.send(peerConnection, LightningMessageCodecs.initCodec.decode(hex"00050100000000 0000".bits).require.value)
     transport.expectMsgType[TransportHandler.ReadAck]
     probe.expectTerminated(transport.ref)
-    origin.expectMsg(PeerConnection.ConnectionResult.InitializationFailed("incompatible features"))
+    origin.expectMsg(PeerConnection.ConnectionResult.InitializationFailed("incompatible features (unknown_32,payment_secret,var_onion_optin)"))
     peer.expectMsg(ConnectionDown(peerConnection))
   }