Skip to content

Commit 844829a

Browse files
t-bastt-bast
authored
Features should be a Map (#1715)
We previously used a Set, which means you could theoretically have a feature that is both activated as `optional` and `mandatory`. We change that to be a Map `feature -> support`.
1 parent 163700a commit 844829a

24 files changed

+183
-199
lines changed

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

+20-24
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package fr.acinq.eclair
1818

1919
import com.typesafe.config.Config
2020
import fr.acinq.eclair.FeatureSupport.{Mandatory, Optional}
21-
import fr.acinq.eclair.Features.{BasicMultiPartPayment, PaymentSecret}
2221
import scodec.bits.{BitVector, ByteVector}
2322

2423
/**
@@ -40,24 +39,22 @@ trait Feature {
4039
def optional: Int = mandatory + 1
4140

4241
def supportBit(support: FeatureSupport): Int = support match {
43-
case FeatureSupport.Mandatory => mandatory
44-
case FeatureSupport.Optional => optional
42+
case Mandatory => mandatory
43+
case Optional => optional
4544
}
4645

4746
override def toString = rfcName
4847

4948
}
5049
// @formatter:on
5150

52-
case class ActivatedFeature(feature: Feature, support: FeatureSupport)
53-
5451
case class UnknownFeature(bitIndex: Int)
5552

56-
case class Features(activated: Set[ActivatedFeature], unknown: Set[UnknownFeature] = Set.empty) {
53+
case class Features(activated: Map[Feature, FeatureSupport], unknown: Set[UnknownFeature] = Set.empty) {
5754

5855
def hasFeature(feature: Feature, support: Option[FeatureSupport] = None): Boolean = support match {
59-
case Some(s) => activated.contains(ActivatedFeature(feature, s))
60-
case None => hasFeature(feature, Some(Optional)) || hasFeature(feature, Some(Mandatory))
56+
case Some(s) => activated.get(feature).contains(s)
57+
case None => activated.contains(feature)
6158
}
6259

6360
def hasPluginFeature(feature: UnknownFeature): Boolean = unknown.contains(feature)
@@ -68,14 +65,14 @@ case class Features(activated: Set[ActivatedFeature], unknown: Set[UnknownFeatur
6865
val unknownFeaturesOk = remoteFeatures.unknown.forall(_.bitIndex % 2 == 1)
6966
// we verify that we activated every mandatory feature they require
7067
val knownFeaturesOk = remoteFeatures.activated.forall {
71-
case ActivatedFeature(_, Optional) => true
72-
case ActivatedFeature(feature, Mandatory) => hasFeature(feature)
68+
case (_, Optional) => true
69+
case (feature, Mandatory) => hasFeature(feature)
7370
}
7471
unknownFeaturesOk && knownFeaturesOk
7572
}
7673

7774
def toByteVector: ByteVector = {
78-
val activatedFeatureBytes = toByteVectorFromIndex(activated.map { case ActivatedFeature(f, s) => f.supportBit(s) })
75+
val activatedFeatureBytes = toByteVectorFromIndex(activated.map { case (feature, support) => feature.supportBit(support) }.toSet)
7976
val unknownFeatureBytes = toByteVectorFromIndex(unknown.map(_.bitIndex))
8077
val maxSize = activatedFeatureBytes.size.max(unknownFeatureBytes.size)
8178
activatedFeatureBytes.padLeft(maxSize) | unknownFeatureBytes.padLeft(maxSize)
@@ -85,35 +82,33 @@ case class Features(activated: Set[ActivatedFeature], unknown: Set[UnknownFeatur
8582
if (indexes.isEmpty) return ByteVector.empty
8683
// When converting from BitVector to ByteVector, scodec pads right instead of left, so we make sure we pad to bytes *before* setting feature bits.
8784
var buf = BitVector.fill(indexes.max + 1)(high = false).bytes.bits
88-
indexes.foreach { i =>
89-
buf = buf.set(i)
90-
}
85+
indexes.foreach { i => buf = buf.set(i) }
9186
buf.reverse.bytes
9287
}
9388

9489
override def toString: String = {
95-
val a = activated.map(f => f.feature.rfcName + ":" + f.support).mkString(",")
90+
val a = activated.map { case (feature, support) => feature.rfcName + ":" + support }.mkString(",")
9691
val u = unknown.map(_.bitIndex).mkString(",")
9792
s"$a" + (if (unknown.nonEmpty) s" (unknown=$u)" else "")
9893
}
9994
}
10095

10196
object Features {
10297

103-
def empty = Features(Set.empty[ActivatedFeature])
98+
def empty = Features(Map.empty[Feature, FeatureSupport])
10499

105-
def apply(features: Set[ActivatedFeature]): Features = Features(activated = features)
100+
def apply(features: (Feature, FeatureSupport)*): Features = Features(Map.from(features))
106101

107102
def apply(bytes: ByteVector): Features = apply(bytes.bits)
108103

109104
def apply(bits: BitVector): Features = {
110105
val all = bits.toIndexedSeq.reverse.zipWithIndex.collect {
111-
case (true, idx) if knownFeatures.exists(_.optional == idx) => Right(ActivatedFeature(knownFeatures.find(_.optional == idx).get, Optional))
112-
case (true, idx) if knownFeatures.exists(_.mandatory == idx) => Right(ActivatedFeature(knownFeatures.find(_.mandatory == idx).get, Mandatory))
106+
case (true, idx) if knownFeatures.exists(_.optional == idx) => Right((knownFeatures.find(_.optional == idx).get, Optional))
107+
case (true, idx) if knownFeatures.exists(_.mandatory == idx) => Right((knownFeatures.find(_.mandatory == idx).get, Mandatory))
113108
case (true, idx) => Left(UnknownFeature(idx))
114109
}
115110
Features(
116-
activated = all.collect { case Right(af) => af }.toSet,
111+
activated = all.collect { case Right((feature, support)) => feature -> support }.toMap,
117112
unknown = all.collect { case Left(inf) => inf }.toSet
118113
)
119114
}
@@ -123,15 +118,16 @@ object Features {
123118
knownFeatures.flatMap {
124119
feature =>
125120
getFeature(config, feature.rfcName) match {
126-
case Some(support) => Some(ActivatedFeature(feature, support))
121+
case Some(support) => Some(feature -> support)
127122
case _ => None
128123
}
129-
})
124+
}.toMap)
130125

131126
/** tries to extract the given feature name from the config, if successful returns its feature support */
132127
private def getFeature(config: Config, name: String): Option[FeatureSupport] = {
133-
if (!config.hasPath(s"features.$name")) None
134-
else {
128+
if (!config.hasPath(s"features.$name")) {
129+
None
130+
} else {
135131
config.getString(s"features.$name") match {
136132
case support if support == Mandatory.toString => Some(Mandatory)
137133
case support if support == Optional.toString => Some(Optional)

‎eclair-core/src/test/scala/fr/acinq/eclair/FeaturesSpec.scala

+27-27
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class FeaturesSpec extends AnyFunSuite {
3939
}
4040

4141
test("'initial_routing_sync', 'data_loss_protect' and 'variable_length_onion' features") {
42-
val features = Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(OptionDataLossProtect, Optional), ActivatedFeature(VariableLengthOnion, Mandatory)))
42+
val features = Features(InitialRoutingSync -> Optional, OptionDataLossProtect -> Optional, VariableLengthOnion -> Mandatory)
4343
assert(features.toByteVector == hex"010a")
4444
assert(features.hasFeature(OptionDataLossProtect))
4545
assert(features.hasFeature(InitialRoutingSync, None))
@@ -114,87 +114,87 @@ class FeaturesSpec extends AnyFunSuite {
114114
),
115115
TestCase(
116116
Features.empty,
117-
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional))),
117+
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Optional),
118118
oursSupportTheirs = true,
119119
theirsSupportOurs = true,
120120
compatible = true
121121
),
122122
TestCase(
123123
Features.empty,
124-
Features(Set.empty, Set(UnknownFeature(101), UnknownFeature(103))),
124+
Features(activated = Map.empty, Set(UnknownFeature(101), UnknownFeature(103))),
125125
oursSupportTheirs = true,
126126
theirsSupportOurs = true,
127127
compatible = true
128128
),
129129
// Same feature set
130130
TestCase(
131-
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))),
132-
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))),
131+
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Mandatory),
132+
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Mandatory),
133133
oursSupportTheirs = true,
134134
theirsSupportOurs = true,
135135
compatible = true
136136
),
137137
// Many optional features
138138
TestCase(
139-
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(ChannelRangeQueries, Optional), ActivatedFeature(PaymentSecret, Optional))),
140-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(ChannelRangeQueries, Optional), ActivatedFeature(ChannelRangeQueriesExtended, Optional))),
139+
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Optional, ChannelRangeQueries -> Optional, PaymentSecret -> Optional),
140+
Features(VariableLengthOnion -> Optional, ChannelRangeQueries -> Optional, ChannelRangeQueriesExtended -> Optional),
141141
oursSupportTheirs = true,
142142
theirsSupportOurs = true,
143143
compatible = true
144144
),
145145
// We support their mandatory features
146146
TestCase(
147-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
148-
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))),
147+
Features(VariableLengthOnion -> Optional),
148+
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Mandatory),
149149
oursSupportTheirs = true,
150150
theirsSupportOurs = true,
151151
compatible = true
152152
),
153153
// They support our mandatory features
154154
TestCase(
155-
Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory))),
156-
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional))),
155+
Features(VariableLengthOnion -> Mandatory),
156+
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Optional),
157157
oursSupportTheirs = true,
158158
theirsSupportOurs = true,
159159
compatible = true
160160
),
161161
// They have unknown optional features
162162
TestCase(
163-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
164-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional)), Set(UnknownFeature(141))),
163+
Features(VariableLengthOnion -> Optional),
164+
Features(Map[Feature, FeatureSupport](VariableLengthOnion -> Optional), Set(UnknownFeature(141))),
165165
oursSupportTheirs = true,
166166
theirsSupportOurs = true,
167167
compatible = true
168168
),
169169
// They have unknown mandatory features
170170
TestCase(
171-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
172-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional)), Set(UnknownFeature(142))),
171+
Features(VariableLengthOnion -> Optional),
172+
Features(Map[Feature, FeatureSupport](VariableLengthOnion -> Optional), Set(UnknownFeature(142))),
173173
oursSupportTheirs = false,
174174
theirsSupportOurs = true,
175175
compatible = false
176176
),
177177
// We don't support one of their mandatory features
178178
TestCase(
179-
Features(Set(ActivatedFeature(ChannelRangeQueries, Optional))),
180-
Features(Set(ActivatedFeature(ChannelRangeQueries, Mandatory), ActivatedFeature(VariableLengthOnion, Mandatory))),
179+
Features(ChannelRangeQueries -> Optional),
180+
Features(ChannelRangeQueries -> Mandatory, VariableLengthOnion -> Mandatory),
181181
oursSupportTheirs = false,
182182
theirsSupportOurs = true,
183183
compatible = false
184184
),
185185
// They don't support one of our mandatory features
186186
TestCase(
187-
Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory), ActivatedFeature(PaymentSecret, Mandatory))),
188-
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
187+
Features(VariableLengthOnion -> Mandatory, PaymentSecret -> Mandatory),
188+
Features(VariableLengthOnion -> Optional),
189189
oursSupportTheirs = true,
190190
theirsSupportOurs = false,
191191
compatible = false
192192
),
193193
// nonreg testing of future features (needs to be updated with every new supported mandatory bit)
194-
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(22))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
195-
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(23))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true),
196-
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(24))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
197-
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(25))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true)
194+
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(22))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
195+
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(23))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true),
196+
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(24))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
197+
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(25))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true)
198198
)
199199

200200
for (testCase <- testCases) {
@@ -207,10 +207,10 @@ class FeaturesSpec extends AnyFunSuite {
207207
test("features to bytes") {
208208
val testCases = Map(
209209
hex"" -> Features.empty,
210-
hex"0100" -> Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory))),
211-
hex"028a8a" -> Features(Set(ActivatedFeature(OptionDataLossProtect, Optional), ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(ChannelRangeQueries, Optional), ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(ChannelRangeQueriesExtended, Optional), ActivatedFeature(PaymentSecret, Optional), ActivatedFeature(BasicMultiPartPayment, Optional))),
212-
hex"09004200" -> Features(Set(ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(PaymentSecret, Mandatory)), Set(UnknownFeature(24), UnknownFeature(27))),
213-
hex"52000000" -> Features(Set.empty, Set(UnknownFeature(25), UnknownFeature(28), UnknownFeature(30)))
210+
hex"0100" -> Features(VariableLengthOnion -> Mandatory),
211+
hex"028a8a" -> Features(OptionDataLossProtect -> Optional, InitialRoutingSync -> Optional, ChannelRangeQueries -> Optional, VariableLengthOnion -> Optional, ChannelRangeQueriesExtended -> Optional, PaymentSecret -> Optional, BasicMultiPartPayment -> Optional),
212+
hex"09004200" -> Features(Map[Feature, FeatureSupport](VariableLengthOnion -> Optional, PaymentSecret -> Mandatory), Set(UnknownFeature(24), UnknownFeature(27))),
213+
hex"52000000" -> Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(25), UnknownFeature(28), UnknownFeature(30)))
214214
)
215215

216216
for ((bin, features) <- testCases) {

‎eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala

+3-4
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@
1616

1717
package fr.acinq.eclair
1818

19-
import java.util.UUID
20-
import java.util.concurrent.atomic.AtomicLong
21-
2219
import com.typesafe.config.{Config, ConfigFactory}
2320
import fr.acinq.bitcoin.Block
2421
import fr.acinq.bitcoin.Crypto.PublicKey
@@ -29,6 +26,8 @@ import fr.acinq.eclair.crypto.keymanager.{LocalChannelKeyManager, LocalNodeKeyMa
2926
import org.scalatest.funsuite.AnyFunSuite
3027
import scodec.bits.{ByteVector, HexStringSyntax}
3128

29+
import java.util.UUID
30+
import java.util.concurrent.atomic.AtomicLong
3231
import scala.jdk.CollectionConverters._
3332
import scala.util.Try
3433

@@ -145,7 +144,7 @@ class StartupSpec extends AnyFunSuite {
145144

146145
val nodeParams = makeNodeParamsWithDefaults(perNodeConf.withFallback(defaultConf))
147146
val perNodeFeatures = nodeParams.featuresFor(PublicKey(ByteVector.fromValidHex("02aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")))
148-
assert(perNodeFeatures === Features(Set(ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(PaymentSecret, Mandatory), ActivatedFeature(BasicMultiPartPayment, Mandatory))))
147+
assert(perNodeFeatures === Features(VariableLengthOnion -> Optional, PaymentSecret -> Mandatory, BasicMultiPartPayment -> Mandatory))
149148
}
150149

151150
test("override feerate mismatch tolerance") {

‎eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala

+15-15
Original file line numberDiff line numberDiff line change
@@ -153,13 +153,13 @@ object TestConstants {
153153
color = Color(1, 2, 3),
154154
publicAddresses = NodeAddress.fromParts("localhost", 9731).get :: Nil,
155155
features = Features(
156-
Set(
157-
ActivatedFeature(OptionDataLossProtect, Optional),
158-
ActivatedFeature(ChannelRangeQueries, Optional),
159-
ActivatedFeature(ChannelRangeQueriesExtended, Optional),
160-
ActivatedFeature(VariableLengthOnion, Optional),
161-
ActivatedFeature(PaymentSecret, Optional),
162-
ActivatedFeature(BasicMultiPartPayment, Optional)
156+
Map[Feature, FeatureSupport](
157+
OptionDataLossProtect -> Optional,
158+
ChannelRangeQueries -> Optional,
159+
ChannelRangeQueriesExtended -> Optional,
160+
VariableLengthOnion -> Optional,
161+
PaymentSecret -> Optional,
162+
BasicMultiPartPayment -> Optional
163163
),
164164
Set(UnknownFeature(TestFeature.optional))
165165
),
@@ -260,14 +260,14 @@ object TestConstants {
260260
alias = "bob",
261261
color = Color(4, 5, 6),
262262
publicAddresses = NodeAddress.fromParts("localhost", 9732).get :: Nil,
263-
features = Features(Set(
264-
ActivatedFeature(OptionDataLossProtect, Optional),
265-
ActivatedFeature(ChannelRangeQueries, Optional),
266-
ActivatedFeature(ChannelRangeQueriesExtended, Optional),
267-
ActivatedFeature(VariableLengthOnion, Optional),
268-
ActivatedFeature(PaymentSecret, Optional),
269-
ActivatedFeature(BasicMultiPartPayment, Optional)
270-
)),
263+
features = Features(
264+
OptionDataLossProtect -> Optional,
265+
ChannelRangeQueries -> Optional,
266+
ChannelRangeQueriesExtended -> Optional,
267+
VariableLengthOnion -> Optional,
268+
PaymentSecret -> Optional,
269+
BasicMultiPartPayment -> Optional
270+
),
271271
pluginParams = Nil,
272272
overrideFeatures = Map.empty,
273273
syncWhitelist = Set.empty,

0 commit comments

Comments
 (0)