Activated features should be a map instead of a set (#225)

It doesn't make sense to use a set, because we could end up having a feature
activated twice (once as optional and once as mandatory).

It should instead be a Map<Feature, FeatureSupport>.

Author: t-bast

eclair-kmp-test-fixtures/src/commonMain/kotlin/fr/acinq/eclair/tests/TestConstants.kt

@@ -41,19 +41,17 @@ object TestConstants {
             keyManager = keyManager,
             alias = "alice",
             features = Features(
-                setOf(
-                    ActivatedFeature(Feature.InitialRoutingSync, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.OptionDataLossProtect, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.ChannelRangeQueries, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.ChannelRangeQueriesExtended, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.VariableLengthOnion, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.PaymentSecret, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.BasicMultiPartPayment, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.Wumbo, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.StaticRemoteKey, FeatureSupport.Mandatory),
-                    ActivatedFeature(Feature.AnchorOutputs, FeatureSupport.Mandatory),
-                    ActivatedFeature(Feature.TrampolinePayment, FeatureSupport.Optional)
-                )
+                Feature.InitialRoutingSync to FeatureSupport.Optional,
+                Feature.OptionDataLossProtect to FeatureSupport.Optional,
+                Feature.ChannelRangeQueries to FeatureSupport.Optional,
+                Feature.ChannelRangeQueriesExtended to FeatureSupport.Optional,
+                Feature.VariableLengthOnion to FeatureSupport.Optional,
+                Feature.PaymentSecret to FeatureSupport.Optional,
+                Feature.BasicMultiPartPayment to FeatureSupport.Optional,
+                Feature.Wumbo to FeatureSupport.Optional,
+                Feature.StaticRemoteKey to FeatureSupport.Mandatory,
+                Feature.AnchorOutputs to FeatureSupport.Mandatory,
+                Feature.TrampolinePayment to FeatureSupport.Optional
             ),
             dustLimit = 1_100.sat,
             maxRemoteDustLimit = 1_500.sat,
@@ -110,19 +108,17 @@ object TestConstants {
             keyManager = keyManager,
             alias = "bob",
             features = Features(
-                setOf(
-                    ActivatedFeature(Feature.InitialRoutingSync, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.OptionDataLossProtect, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.ChannelRangeQueries, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.ChannelRangeQueriesExtended, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.VariableLengthOnion, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.PaymentSecret, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.BasicMultiPartPayment, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.Wumbo, FeatureSupport.Optional),
-                    ActivatedFeature(Feature.StaticRemoteKey, FeatureSupport.Mandatory),
-                    ActivatedFeature(Feature.AnchorOutputs, FeatureSupport.Mandatory),
-                    ActivatedFeature(Feature.TrampolinePayment, FeatureSupport.Optional)
-                )
+                Feature.InitialRoutingSync to FeatureSupport.Optional,
+                Feature.OptionDataLossProtect to FeatureSupport.Optional,
+                Feature.ChannelRangeQueries to FeatureSupport.Optional,
+                Feature.ChannelRangeQueriesExtended to FeatureSupport.Optional,
+                Feature.VariableLengthOnion to FeatureSupport.Optional,
+                Feature.PaymentSecret to FeatureSupport.Optional,
+                Feature.BasicMultiPartPayment to FeatureSupport.Optional,
+                Feature.Wumbo to FeatureSupport.Optional,
+                Feature.StaticRemoteKey to FeatureSupport.Mandatory,
+                Feature.AnchorOutputs to FeatureSupport.Mandatory,
+                Feature.TrampolinePayment to FeatureSupport.Optional
             ),
             dustLimit = 1_000.sat,
             maxRemoteDustLimit = 1_500.sat,

src/commonMain/kotlin/fr/acinq/eclair/Features.kt

@@ -101,53 +101,48 @@ sealed class Feature {
     }
 }
 
-@Serializable
-data class ActivatedFeature(val feature: Feature, val support: FeatureSupport)
-
 @Serializable
 data class UnknownFeature(val bitIndex: Int)
 
 @Serializable
-data class Features(val activated: Set<ActivatedFeature>, val unknown: Set<UnknownFeature> = emptySet()) {
+data class Features(val activated: Map<Feature, FeatureSupport>, val unknown: Set<UnknownFeature> = emptySet()) {
 
     fun hasFeature(feature: Feature, support: FeatureSupport? = null): Boolean =
-        if (support != null) activated.contains(ActivatedFeature(feature, support))
-        else hasFeature(feature, FeatureSupport.Optional) || hasFeature(feature, FeatureSupport.Mandatory)
+        if (support != null) activated[feature] == support
+        else activated.containsKey(feature)
 
     /** NB: this method is not reflexive, see [[Features.areCompatible]] if you want symmetric validation. */
     fun areSupported(remoteFeatures: Features): Boolean {
         // we allow unknown odd features (it's ok to be odd)
         val unknownFeaturesOk = remoteFeatures.unknown.all { it.bitIndex % 2 == 1 }
         // we verify that we activated every mandatory feature they require
-        val knownFeaturesOk = remoteFeatures.activated.all {
-            when (it.support) {
+        val knownFeaturesOk = remoteFeatures.activated.all { (feature, support) ->
+            when (support) {
                 FeatureSupport.Optional -> true
-                FeatureSupport.Mandatory -> hasFeature(it.feature)
+                FeatureSupport.Mandatory -> hasFeature(feature)
             }
         }
         return unknownFeaturesOk && knownFeaturesOk
     }
 
     fun toByteArray(): ByteArray {
-        val activatedFeatureBytes =
-            activated.mapTo(HashSet()) { it.feature.supportBit(it.support) }.indicesToByteArray()
-        val unknownFeatureBytes = unknown.mapTo(HashSet()) { it.bitIndex }.indicesToByteArray()
+        val activatedFeatureBytes = activated.map { (feature, support) -> feature.supportBit(support) }.toHashSet().indicesToByteArray()
+        val unknownFeatureBytes = unknown.map { it.bitIndex }.toHashSet().indicesToByteArray()
         val maxSize = activatedFeatureBytes.size.coerceAtLeast(unknownFeatureBytes.size)
         return activatedFeatureBytes.leftPaddedCopyOf(maxSize) or unknownFeatureBytes.leftPaddedCopyOf(maxSize)
     }
 
     private fun Set<Int>.indicesToByteArray(): ByteArray {
         if (isEmpty()) return ByteArray(0)
-        // When converting from BitVector to ByteVector, scodec pads right instead of left, so we make sure we pad to bytes *before* setting feature bits.
         val buf = BitField.forAtMost(maxOrNull()!! + 1)
         forEach { buf.setRight(it) }
         return buf.bytes
     }
 
     companion object {
-        val empty = Features(emptySet())
+        val empty = Features(mapOf())
 
-        val knownFeatures: Set<Feature> = setOf(
+        private val knownFeatures: Set<Feature> = setOf(
             Feature.OptionDataLossProtect,
             Feature.InitialRoutingSync,
             Feature.ChannelRangeQueries,
@@ -169,40 +164,19 @@ data class Features(val activated: Set<ActivatedFeature>, val unknown: Set<Unkno
             val all = bits.asRightSequence().withIndex()
                 .filter { it.value }
                 .map { (idx, _) ->
-                    knownFeatures.find { it.optional == idx }?.let { ActivatedFeature(it, FeatureSupport.Optional) }
-                        ?: knownFeatures.find { it.mandatory == idx }
-                            ?.let { ActivatedFeature(it, FeatureSupport.Mandatory) }
+                    knownFeatures.find { it.optional == idx }?.let { Pair(it, FeatureSupport.Optional) }
+                        ?: knownFeatures.find { it.mandatory == idx }?.let { Pair(it, FeatureSupport.Mandatory) }
                         ?: UnknownFeature(idx)
                 }
                 .toList()
 
             return Features(
-                activated = all.filterIsInstance<ActivatedFeature>().toSet(),
+                activated = all.filterIsInstance<Pair<Feature, FeatureSupport>>().toMap(),
                 unknown = all.filterIsInstance<UnknownFeature>().toSet()
             )
         }
 
-//        /** expects to have a top level config block named "features" */
-//        fun fromConfiguration(config: Config): Features = Features(
-//        knownFeatures.flatMap {
-//            feature =>
-//            getFeature(config, feature.rfcName) match {
-//                case Some(support) => Some(ActivatedFeature(feature, support))
-//                case _ => None
-//            }
-//        })
-//
-//        /** tries to extract the given feature name from the config, if successful returns its feature support */
-//        private def getFeature(config: Config, name: String): Option[FeatureSupport] = {
-//            if (!config.hasPath(s"features.$name")) None
-//            else {
-//                config.getString(s"features.$name") match {
-//                    case support if support == Mandatory.toString => Some(Mandatory)
-//                    case support if support == Optional.toString => Some(Optional)
-//                    case wrongSupport => throw new IllegalArgumentException(s"Wrong support specified ($wrongSupport)")
-//                }
-//            }
-//        }
+        operator fun invoke(vararg pairs: Pair<Feature, FeatureSupport>): Features = Features(mapOf(*pairs))
 
         // Features may depend on other features, as specified in Bolt 9.
         private val featuresDependency: Map<Feature, List<Feature>> = mapOf(
@@ -229,7 +203,6 @@ data class Features(val activated: Set<ActivatedFeature>, val unknown: Set<Unkno
 
         fun areCompatible(ours: Features, theirs: Features): Boolean = ours.areSupported(theirs) && theirs.areSupported(ours)
 
-
         /** returns true if both have at least optional support */
         fun canUseFeature(localFeatures: Features, remoteFeatures: Features, feature: Feature): Boolean =
             localFeatures.hasFeature(feature) && remoteFeatures.hasFeature(feature)

src/commonMain/kotlin/fr/acinq/eclair/payment/OutgoingPaymentHandler.kt

@@ -309,7 +309,7 @@ class OutgoingPaymentHandler(val nodeId: PublicKey, val walletParams: WalletPara
         val finalExpiry = finalExpiryDelta.toCltvExpiry(currentBlockHeight.toLong())
         val finalPayload = FinalPayload.createSinglePartPayload(request.amount, finalExpiry, request.details.paymentRequest.paymentSecret)
 
-        val invoiceFeatures = request.details.paymentRequest.features?.let { Features(it) } ?: Features(setOf())
+        val invoiceFeatures = request.details.paymentRequest.features?.let { Features(it) } ?: Features(mapOf())
         val (trampolineAmount, trampolineExpiry, trampolineOnion) = if (invoiceFeatures.hasFeature(Feature.TrampolinePayment)) {
             OutgoingPacket.buildPacket(request.paymentHash, trampolineRoute, finalPayload, OnionRoutingPacket.TrampolinePacketLength)
         } else {

src/commonMain/kotlin/fr/acinq/eclair/payment/PaymentRequest.kt

@@ -154,7 +154,7 @@ data class PaymentRequest(
          * This filters out all features unrelated to BOLT 11
          */
         fun invoiceFeatures(features: Features): Features {
-            return Features(activated = features.activated.filter { f -> bolt11Features.contains(f.feature) }.toSet())
+            return Features(activated = features.activated.filter { (f, _) -> bolt11Features.contains(f) })
         }
 
         fun create(