Skip to content

Commit

Permalink
Features should be a Map (#1715)
Browse files Browse the repository at this point in the history 
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`.
  • Loading branch information
@t-bast
t-bast authored Mar 4, 2021
1 parent 163700a commit 844829a
Show file tree
Hide file tree
Showing 24 changed files with 183 additions and 199 deletions.
44 changes: 20 additions & 24 deletions eclair-core/src/main/scala/fr/acinq/eclair/Features.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package fr.acinq.eclair

import com.typesafe.config.Config
import fr.acinq.eclair.FeatureSupport.{Mandatory, Optional}
import fr.acinq.eclair.Features.{BasicMultiPartPayment, PaymentSecret}
import scodec.bits.{BitVector, ByteVector}

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

def supportBit(support: FeatureSupport): Int = support match {
case FeatureSupport.Mandatory => mandatory
case FeatureSupport.Optional => optional
case Mandatory => mandatory
case Optional => optional
}

override def toString = rfcName

}
// @formatter:on

case class ActivatedFeature(feature: Feature, support: FeatureSupport)

case class UnknownFeature(bitIndex: Int)

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

def hasFeature(feature: Feature, support: Option[FeatureSupport] = None): Boolean = support match {
case Some(s) => activated.contains(ActivatedFeature(feature, s))
case None => hasFeature(feature, Some(Optional)) || hasFeature(feature, Some(Mandatory))
case Some(s) => activated.get(feature).contains(s)
case None => activated.contains(feature)
}

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

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

override def toString: String = {
val a = activated.map(f => f.feature.rfcName + ":" + f.support).mkString(",")
val a = activated.map { case (feature, support) => feature.rfcName + ":" + support }.mkString(",")
val u = unknown.map(_.bitIndex).mkString(",")
s"$a" + (if (unknown.nonEmpty) s" (unknown=$u)" else "")
}
}

object Features {

def empty = Features(Set.empty[ActivatedFeature])
def empty = Features(Map.empty[Feature, FeatureSupport])

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

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

def apply(bits: BitVector): Features = {
val all = bits.toIndexedSeq.reverse.zipWithIndex.collect {
case (true, idx) if knownFeatures.exists(_.optional == idx) => Right(ActivatedFeature(knownFeatures.find(_.optional == idx).get, Optional))
case (true, idx) if knownFeatures.exists(_.mandatory == idx) => Right(ActivatedFeature(knownFeatures.find(_.mandatory == idx).get, Mandatory))
case (true, idx) if knownFeatures.exists(_.optional == idx) => Right((knownFeatures.find(_.optional == idx).get, Optional))
case (true, idx) if knownFeatures.exists(_.mandatory == idx) => Right((knownFeatures.find(_.mandatory == idx).get, Mandatory))
case (true, idx) => Left(UnknownFeature(idx))
}
Features(
activated = all.collect { case Right(af) => af }.toSet,
activated = all.collect { case Right((feature, support)) => feature -> support }.toMap,
unknown = all.collect { case Left(inf) => inf }.toSet
)
}
Expand All @@ -123,15 +118,16 @@ object Features {
knownFeatures.flatMap {
feature =>
getFeature(config, feature.rfcName) match {
case Some(support) => Some(ActivatedFeature(feature, support))
case Some(support) => Some(feature -> support)
case _ => None
}
})
}.toMap)

/** 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 {
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)
Expand Down
54 changes: 27 additions & 27 deletions eclair-core/src/test/scala/fr/acinq/eclair/FeaturesSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class FeaturesSpec extends AnyFunSuite {
}

test("'initial_routing_sync', 'data_loss_protect' and 'variable_length_onion' features") {
val features = Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(OptionDataLossProtect, Optional), ActivatedFeature(VariableLengthOnion, Mandatory)))
val features = Features(InitialRoutingSync -> Optional, OptionDataLossProtect -> Optional, VariableLengthOnion -> Mandatory)
assert(features.toByteVector == hex"010a")
assert(features.hasFeature(OptionDataLossProtect))
assert(features.hasFeature(InitialRoutingSync, None))
Expand Down Expand Up @@ -114,87 +114,87 @@ class FeaturesSpec extends AnyFunSuite {
),
TestCase(
Features.empty,
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional))),
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Optional),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
TestCase(
Features.empty,
Features(Set.empty, Set(UnknownFeature(101), UnknownFeature(103))),
Features(activated = Map.empty, Set(UnknownFeature(101), UnknownFeature(103))),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
// Same feature set
TestCase(
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))),
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))),
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Mandatory),
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Mandatory),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
// Many optional features
TestCase(
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(ChannelRangeQueries, Optional), ActivatedFeature(PaymentSecret, Optional))),
Features(Set(ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(ChannelRangeQueries, Optional), ActivatedFeature(ChannelRangeQueriesExtended, Optional))),
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Optional, ChannelRangeQueries -> Optional, PaymentSecret -> Optional),
Features(VariableLengthOnion -> Optional, ChannelRangeQueries -> Optional, ChannelRangeQueriesExtended -> Optional),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
// We support their mandatory features
TestCase(
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))),
Features(VariableLengthOnion -> Optional),
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Mandatory),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
// They support our mandatory features
TestCase(
Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory))),
Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional))),
Features(VariableLengthOnion -> Mandatory),
Features(InitialRoutingSync -> Optional, VariableLengthOnion -> Optional),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
// They have unknown optional features
TestCase(
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
Features(Set(ActivatedFeature(VariableLengthOnion, Optional)), Set(UnknownFeature(141))),
Features(VariableLengthOnion -> Optional),
Features(Map[Feature, FeatureSupport](VariableLengthOnion -> Optional), Set(UnknownFeature(141))),
oursSupportTheirs = true,
theirsSupportOurs = true,
compatible = true
),
// They have unknown mandatory features
TestCase(
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
Features(Set(ActivatedFeature(VariableLengthOnion, Optional)), Set(UnknownFeature(142))),
Features(VariableLengthOnion -> Optional),
Features(Map[Feature, FeatureSupport](VariableLengthOnion -> Optional), Set(UnknownFeature(142))),
oursSupportTheirs = false,
theirsSupportOurs = true,
compatible = false
),
// We don't support one of their mandatory features
TestCase(
Features(Set(ActivatedFeature(ChannelRangeQueries, Optional))),
Features(Set(ActivatedFeature(ChannelRangeQueries, Mandatory), ActivatedFeature(VariableLengthOnion, Mandatory))),
Features(ChannelRangeQueries -> Optional),
Features(ChannelRangeQueries -> Mandatory, VariableLengthOnion -> Mandatory),
oursSupportTheirs = false,
theirsSupportOurs = true,
compatible = false
),
// They don't support one of our mandatory features
TestCase(
Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory), ActivatedFeature(PaymentSecret, Mandatory))),
Features(Set(ActivatedFeature(VariableLengthOnion, Optional))),
Features(VariableLengthOnion -> Mandatory, PaymentSecret -> Mandatory),
Features(VariableLengthOnion -> Optional),
oursSupportTheirs = true,
theirsSupportOurs = false,
compatible = false
),
// nonreg testing of future features (needs to be updated with every new supported mandatory bit)
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(22))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(23))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(24))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(25))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true)
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(22))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(23))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true),
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(24))), oursSupportTheirs = false, theirsSupportOurs = true, compatible = false),
TestCase(Features.empty, Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(25))), oursSupportTheirs = true, theirsSupportOurs = true, compatible = true)
)

for (testCase <- testCases) {
Expand All @@ -207,10 +207,10 @@ class FeaturesSpec extends AnyFunSuite {
test("features to bytes") {
val testCases = Map(
hex"" -> Features.empty,
hex"0100" -> Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory))),
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))),
hex"09004200" -> Features(Set(ActivatedFeature(VariableLengthOnion, Optional), ActivatedFeature(PaymentSecret, Mandatory)), Set(UnknownFeature(24), UnknownFeature(27))),
hex"52000000" -> Features(Set.empty, Set(UnknownFeature(25), UnknownFeature(28), UnknownFeature(30)))
hex"0100" -> Features(VariableLengthOnion -> Mandatory),
hex"028a8a" -> Features(OptionDataLossProtect -> Optional, InitialRoutingSync -> Optional, ChannelRangeQueries -> Optional, VariableLengthOnion -> Optional, ChannelRangeQueriesExtended -> Optional, PaymentSecret -> Optional, BasicMultiPartPayment -> Optional),
hex"09004200" -> Features(Map[Feature, FeatureSupport](VariableLengthOnion -> Optional, PaymentSecret -> Mandatory), Set(UnknownFeature(24), UnknownFeature(27))),
hex"52000000" -> Features(Map.empty[Feature, FeatureSupport], Set(UnknownFeature(25), UnknownFeature(28), UnknownFeature(30)))
)

for ((bin, features) <- testCases) {
Expand Down
7 changes: 3 additions & 4 deletions eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@

package fr.acinq.eclair

import java.util.UUID
import java.util.concurrent.atomic.AtomicLong

import com.typesafe.config.{Config, ConfigFactory}
import fr.acinq.bitcoin.Block
import fr.acinq.bitcoin.Crypto.PublicKey
Expand All @@ -29,6 +26,8 @@ import fr.acinq.eclair.crypto.keymanager.{LocalChannelKeyManager, LocalNodeKeyMa
import org.scalatest.funsuite.AnyFunSuite
import scodec.bits.{ByteVector, HexStringSyntax}

import java.util.UUID
import java.util.concurrent.atomic.AtomicLong
import scala.jdk.CollectionConverters._
import scala.util.Try

Expand Down Expand Up @@ -145,7 +144,7 @@ class StartupSpec extends AnyFunSuite {

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

test("override feerate mismatch tolerance") {
Expand Down
30 changes: 15 additions & 15 deletions eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,13 @@ object TestConstants {
color = Color(1, 2, 3),
publicAddresses = NodeAddress.fromParts("localhost", 9731).get :: Nil,
features = Features(
Set(
ActivatedFeature(OptionDataLossProtect, Optional),
ActivatedFeature(ChannelRangeQueries, Optional),
ActivatedFeature(ChannelRangeQueriesExtended, Optional),
ActivatedFeature(VariableLengthOnion, Optional),
ActivatedFeature(PaymentSecret, Optional),
ActivatedFeature(BasicMultiPartPayment, Optional)
Map[Feature, FeatureSupport](
OptionDataLossProtect -> Optional,
ChannelRangeQueries -> Optional,
ChannelRangeQueriesExtended -> Optional,
VariableLengthOnion -> Optional,
PaymentSecret -> Optional,
BasicMultiPartPayment -> Optional
),
Set(UnknownFeature(TestFeature.optional))
),
Expand Down Expand Up @@ -260,14 +260,14 @@ object TestConstants {
alias = "bob",
color = Color(4, 5, 6),
publicAddresses = NodeAddress.fromParts("localhost", 9732).get :: Nil,
features = Features(Set(
ActivatedFeature(OptionDataLossProtect, Optional),
ActivatedFeature(ChannelRangeQueries, Optional),
ActivatedFeature(ChannelRangeQueriesExtended, Optional),
ActivatedFeature(VariableLengthOnion, Optional),
ActivatedFeature(PaymentSecret, Optional),
ActivatedFeature(BasicMultiPartPayment, Optional)
)),
features = Features(
OptionDataLossProtect -> Optional,
ChannelRangeQueries -> Optional,
ChannelRangeQueriesExtended -> Optional,
VariableLengthOnion -> Optional,
PaymentSecret -> Optional,
BasicMultiPartPayment -> Optional
),
pluginParams = Nil,
overrideFeatures = Map.empty,
syncWhitelist = Set.empty,
Expand Down
Loading

0 comments on commit 844829a

Please sign in to comment.