Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework features compatibility #1576

Merged
merged 2 commits into from
Oct 23, 2020
Merged

Rework features compatibility #1576

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b19c137
Rework features compatibility
t-bast Oct 20, 2020
c026fa0
Add tests for areSupported
t-bast Oct 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 13 additions & 21 deletions eclair-core/src/main/scala/fr/acinq/eclair/Features.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,17 @@ case class Features(activated: Set[ActivatedFeature], unknown: Set[UnknownFeatur

def hasPluginFeature(feature: UnknownFeature): Boolean = unknown.contains(feature)

def areSupported(remoteFeatures: Features): Boolean = {
pm47 marked this conversation as resolved.
Show resolved Hide resolved
// we allow unknown odd features (it's ok to be odd)
val unknownFeaturesOk = !remoteFeatures.unknown.exists(_.bitIndex % 2 == 0)
pm47 marked this conversation as resolved.
Show resolved Hide resolved
// 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)
}
unknownFeaturesOk && knownFeaturesOk
}

def toByteVector: ByteVector = {
val activatedFeatureBytes = toByteVectorFromIndex(activated.map { case ActivatedFeature(f, s) => f.supportBit(s) })
val unknownFeatureBytes = toByteVectorFromIndex(unknown.map(_.bitIndex))
Expand Down Expand Up @@ -225,17 +236,6 @@ object Features {
KeySend
)

private val supportedMandatoryFeatures: Set[Feature] = Set(
OptionDataLossProtect,
ChannelRangeQueries,
VariableLengthOnion,
ChannelRangeQueriesExtended,
StaticRemoteKey,
PaymentSecret,
BasicMultiPartPayment,
Wumbo
)

// Features may depend on other features, as specified in Bolt 9.
private val featuresDependency = Map(
ChannelRangeQueriesExtended -> (ChannelRangeQueries :: Nil),
Expand All @@ -255,16 +255,8 @@ object Features {
FeatureException(s"$feature is set but is missing a dependency (${dependencies.filter(d => !features.hasFeature(d)).mkString(" and ")})")
}

/**
* A feature set is supported if all even bits are supported.
* We just ignore unknown odd bits.
*/
def areSupported(features: Features): Boolean = {
!features.unknown.exists(_.bitIndex % 2 == 0) && features.activated.forall {
case ActivatedFeature(_, Optional) => true
case ActivatedFeature(feature, Mandatory) => supportedMandatoryFeatures.contains(feature)
}
}
/** Returns true if both feature sets are compatible. */
def areCompatible(ours: Features, theirs: Features): Boolean = ours.areSupported(theirs) && theirs.areSupported(ours)

/** returns true if both have at least optional support */
def canUseFeature(localFeatures: Features, remoteFeatures: Features, feature: Feature): Boolean = {
Expand Down
2 changes: 1 addition & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/io/PeerConnection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ 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.areSupported(remoteInit.features)) {
} else if (!Features.areCompatible(d.localInit.features, remoteInit.features)) {
log.warning("incompatible features, disconnecting")
d.pendingAuth.origin_opt.foreach(_ ! ConnectionResult.InitializationFailed("incompatible features"))
d.transport ! PoisonPill
Expand Down
5 changes: 3 additions & 2 deletions eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package fr.acinq.eclair.payment
import fr.acinq.bitcoin.Crypto.{PrivateKey, PublicKey}
import fr.acinq.bitcoin.{Base58, Base58Check, Bech32, Block, ByteVector32, ByteVector64, Crypto}
import fr.acinq.eclair.payment.PaymentRequest._
import fr.acinq.eclair.{CltvExpiryDelta, FeatureSupport, Features, LongToBtcAmount, MilliSatoshi, ShortChannelId, randomBytes32}
import fr.acinq.eclair.{CltvExpiryDelta, FeatureSupport, Features, LongToBtcAmount, MilliSatoshi, NodeParams, ShortChannelId, randomBytes32}
import scodec.bits.{BitVector, ByteOrdering, ByteVector}
import scodec.codecs.{list, ubyte}
import scodec.{Codec, Err}
Expand Down Expand Up @@ -339,14 +339,15 @@ object PaymentRequest {
*/
case class PaymentRequestFeatures(bitmask: BitVector) extends TaggedField {
lazy val features: Features = Features(bitmask)
lazy val supported: Boolean = Features.areSupported(features)
lazy val allowMultiPart: Boolean = features.hasFeature(Features.BasicMultiPartPayment)
lazy val allowPaymentSecret: Boolean = features.hasFeature(Features.PaymentSecret)
lazy val requirePaymentSecret: Boolean = features.hasFeature(Features.PaymentSecret, Some(FeatureSupport.Mandatory))
lazy val allowTrampoline: Boolean = features.hasFeature(Features.TrampolinePayment)

def toByteVector: ByteVector = features.toByteVector

def areSupported(nodeParams: NodeParams): Boolean = nodeParams.features.areSupported(features)

override def toString: String = s"Features(${bitmask.toBin})"
}

Expand Down
2 changes: 1 addition & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentInitiator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class PaymentInitiator(nodeParams: NodeParams, router: ActorRef, register: Actor
val paymentCfg = SendPaymentConfig(paymentId, paymentId, r.externalId, r.paymentHash, r.recipientAmount, r.recipientNodeId, Upstream.Local(paymentId), r.paymentRequest, storeInDb = true, publishEvent = true, Nil)
val finalExpiry = r.finalExpiry(nodeParams.currentBlockHeight)
r.paymentRequest match {
case Some(invoice) if !invoice.features.supported =>
case Some(invoice) if !invoice.features.areSupported(nodeParams) =>
sender ! PaymentFailed(paymentId, r.paymentHash, LocalFailure(Nil, UnsupportedFeatures(invoice.features.features)) :: Nil)
case Some(invoice) if invoice.features.allowMultiPart && nodeParams.features.hasFeature(BasicMultiPartPayment) =>
invoice.paymentSecret match {
Expand Down
78 changes: 31 additions & 47 deletions eclair-core/src/test/scala/fr/acinq/eclair/FeaturesSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ 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)))
assert(features.toByteVector == hex"010a")
assert(areSupported(features))
assert(features.hasFeature(OptionDataLossProtect))
assert(features.hasFeature(InitialRoutingSync, None))
assert(features.hasFeature(VariableLengthOnion))
Expand All @@ -57,10 +56,8 @@ class FeaturesSpec extends AnyFunSuite {
test("'option_static_remotekey' feature") {
assert(Features(hex"1000").hasFeature(StaticRemoteKey))
assert(Features(hex"1000").hasFeature(StaticRemoteKey, Some(Mandatory)))
assert(areSupported(Features(hex"1000")))
assert(Features(hex"2000").hasFeature(StaticRemoteKey))
assert(Features(hex"2000").hasFeature(StaticRemoteKey, Some(Optional)))
assert(areSupported(Features(hex"2000")))
}

test("features dependencies") {
Expand Down Expand Up @@ -105,47 +102,37 @@ class FeaturesSpec extends AnyFunSuite {
}

test("features compatibility") {
assert(areSupported(Features(Set(ActivatedFeature(InitialRoutingSync, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(OptionDataLossProtect, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(OptionDataLossProtect, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(ChannelRangeQueries, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(ChannelRangeQueries, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(ChannelRangeQueriesExtended, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(ChannelRangeQueriesExtended, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(VariableLengthOnion, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(PaymentSecret, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(PaymentSecret, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(BasicMultiPartPayment, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(BasicMultiPartPayment, Optional)))))
assert(areSupported(Features(Set(ActivatedFeature(Wumbo, Mandatory)))))
assert(areSupported(Features(Set(ActivatedFeature(Wumbo, Optional)))))
assert(!areSupported(Features(Set(ActivatedFeature(AnchorOutputs, Mandatory))))) // NB: we're not ready to fully support anchor outputs
assert(areSupported(Features(Set(ActivatedFeature(AnchorOutputs, Optional)))))

val testCases = Map(
bin" 00000000000000001011" -> true,
bin" 00010000100001000000" -> true,
bin" 00100000100000100000" -> true,
bin" 00010100000000001000" -> true,
bin" 00011000001000000000" -> true,
bin" 00101000000000000000" -> true,
bin" 00000000010001000000" -> true,
bin" 01000000000000000000" -> true,
bin" 10000000000000000000" -> true,
// unknown optional feature bits
bin" 001000000000000000000000" -> true,
bin" 100000000000000000000000" -> true,
// those are useful for nonreg testing of the areSupported method (which needs to be updated with every new supported mandatory bit)
bin" 000100000000000000000000" -> false,
bin" 010000000000000000000000" -> false,
bin" 0001000000000000000000000000" -> false,
bin" 0100000000000000000000000000" -> false,
bin"00010000000000000000000000000000" -> false,
bin"01000000000000000000000000000000" -> false
case class TestCase(ours: Features, theirs: Features, compatible: Boolean)
pm47 marked this conversation as resolved.
Show resolved Hide resolved
val testCases = Seq(
// Empty features
TestCase(Features.empty, Features.empty, compatible = true),
TestCase(Features.empty, Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional))), compatible = true),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(101), UnknownFeature(103))), compatible = true),
// Same feature set
TestCase(Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))), Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))), 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))), compatible = true),
// We support their mandatory features
TestCase(Features(Set(ActivatedFeature(VariableLengthOnion, Optional))), Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Mandatory))), compatible = true),
// They support our mandatory features
TestCase(Features(Set(ActivatedFeature(VariableLengthOnion, Mandatory))), Features(Set(ActivatedFeature(InitialRoutingSync, Optional), ActivatedFeature(VariableLengthOnion, Optional))), compatible = true),
// They have unknown optional features
TestCase(Features(Set(ActivatedFeature(VariableLengthOnion, Optional))), Features(Set(ActivatedFeature(VariableLengthOnion, Optional)), Set(UnknownFeature(141))), compatible = true),
// They have unknown mandatory features
TestCase(Features(Set(ActivatedFeature(VariableLengthOnion, Optional))), Features(Set(ActivatedFeature(VariableLengthOnion, Optional)), Set(UnknownFeature(142))), 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))), 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))), 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))), compatible = false),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(23))), compatible = true),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(24))), compatible = false),
TestCase(Features.empty, Features(Set.empty, Set(UnknownFeature(25))), compatible = true)
)
for ((testCase, expected) <- testCases) {
assert(areSupported(Features(testCase)) === expected, testCase.toBin)

for (testCase <- testCases) {
assert(areCompatible(testCase.ours, testCase.theirs) === testCase.compatible)
}
}

Expand Down Expand Up @@ -185,7 +172,6 @@ class FeaturesSpec extends AnyFunSuite {
val features = fromConfiguration(conf)
assert(features.toByteVector === hex"028a8a")
assert(Features(hex"028a8a") === features)
assert(areSupported(features))
assert(validateFeatureGraph(features) === None)
assert(features.hasFeature(OptionDataLossProtect, Some(Optional)))
assert(features.hasFeature(InitialRoutingSync, Some(Optional)))
Expand Down Expand Up @@ -213,7 +199,6 @@ class FeaturesSpec extends AnyFunSuite {
val features = fromConfiguration(conf)
assert(features.toByteVector === hex"068a")
assert(Features(hex"068a") === features)
assert(areSupported(features))
assert(validateFeatureGraph(features) === None)
assert(features.hasFeature(OptionDataLossProtect, Some(Optional)))
assert(features.hasFeature(InitialRoutingSync, Some(Optional)))
Expand All @@ -237,7 +222,6 @@ class FeaturesSpec extends AnyFunSuite {
val features = fromConfiguration(confWithUnknownFeatures)
assert(features.toByteVector === hex"4080")
assert(Features(hex"4080") === features)
assert(areSupported(features))
assert(features.hasFeature(ChannelRangeQueries, Some(Optional)))
assert(features.hasFeature(PaymentSecret, Some(Mandatory)))
}
Expand Down Expand Up @@ -278,7 +262,7 @@ class FeaturesSpec extends AnyFunSuite {

test("'knownFeatures' contains all our known features (reflection test)") {
import scala.reflect.runtime.universe._
import scala.reflect.runtime.{ universe => runtime }
import scala.reflect.runtime.{universe => runtime}
val mirror = runtime.runtimeMirror(ClassLoader.getSystemClassLoader)
val subclasses = typeOf[Feature].typeSymbol.asClass.knownDirectSubclasses
val knownFeatures = subclasses.map({ desc =>
Expand Down
15 changes: 8 additions & 7 deletions eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentRequestSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import fr.acinq.bitcoin.Crypto.{PrivateKey, PublicKey}
import fr.acinq.bitcoin.{Block, ByteVector32, Crypto, Protocol}
import fr.acinq.eclair.Features.{PaymentSecret, _}
import fr.acinq.eclair.payment.PaymentRequest._
import fr.acinq.eclair.{CltvExpiryDelta, LongToBtcAmount, ShortChannelId, ToMilliSatoshiConversion}
import fr.acinq.eclair.{ActivatedFeature, CltvExpiryDelta, FeatureSupport, Features, LongToBtcAmount, ShortChannelId, TestConstants, ToMilliSatoshiConversion}
import org.scalatest.funsuite.AnyFunSuite
import scodec.DecodeResult
import scodec.bits._
Expand Down Expand Up @@ -247,7 +247,7 @@ class PaymentRequestSpec extends AnyFunSuite {
assert(!pr.features.allowMultiPart)
assert(!pr.features.requirePaymentSecret)
assert(!pr.features.allowTrampoline)
assert(pr.features.supported)
assert(pr.features.areSupported(TestConstants.Alice.nodeParams))
assert(PaymentRequest.write(pr.sign(priv)) === ref.toLowerCase)
}
}
Expand All @@ -267,7 +267,7 @@ class PaymentRequestSpec extends AnyFunSuite {
assert(!pr.features.allowMultiPart)
assert(!pr.features.requirePaymentSecret)
assert(!pr.features.allowTrampoline)
assert(!pr.features.supported)
assert(!pr.features.areSupported(TestConstants.Alice.nodeParams))
assert(PaymentRequest.write(pr.sign(priv)) === ref)
}

Expand All @@ -284,7 +284,7 @@ class PaymentRequestSpec extends AnyFunSuite {
assert(pr.expiry === Some(604800L))
assert(pr.minFinalCltvExpiryDelta === Some(CltvExpiryDelta(10)))
assert(pr.routingInfo === Seq(Seq(ExtraHop(PublicKey(hex"03d06758583bb5154774a6eb221b1276c9e82d65bbaceca806d90e20c108f4b1c7"), ShortChannelId("589390x3312x1"), 1000 msat, 2500, CltvExpiryDelta(40)))))
assert(pr.features.supported)
assert(pr.features.areSupported(TestConstants.Alice.nodeParams))
assert(PaymentRequest.write(pr.sign(priv)) === ref)
}

Expand Down Expand Up @@ -382,6 +382,7 @@ class PaymentRequestSpec extends AnyFunSuite {
}

test("supported payment request features") {
val nodeParams = TestConstants.Alice.nodeParams.copy(features = Features(knownFeatures.map(ActivatedFeature(_, FeatureSupport.Optional))))
case class Result(allowMultiPart: Boolean, requirePaymentSecret: Boolean, areSupported: Boolean) // "supported" is based on the "it's okay to be odd" rule"
val featureBits = Map(
PaymentRequestFeatures(bin" 00000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = true),
Expand All @@ -393,9 +394,9 @@ class PaymentRequestSpec extends AnyFunSuite {
PaymentRequestFeatures(bin" 01000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = true),
PaymentRequestFeatures(bin" 0000010000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = true),
PaymentRequestFeatures(bin" 0000011000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = true),
PaymentRequestFeatures(bin" 0000110000001000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = false),
PaymentRequestFeatures(bin" 0000110000001000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = true),
PaymentRequestFeatures(bin" 0000100000001000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = true),
// those are useful for nonreg testing of the areSupported method (which needs to be updated with every new supported mandatory bit)
PaymentRequestFeatures(bin" 0000100000001000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = false),
PaymentRequestFeatures(bin" 0010000000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = false),
PaymentRequestFeatures(bin" 000001000000000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = false),
PaymentRequestFeatures(bin" 000100000000000000000000000000") -> Result(allowMultiPart = false, requirePaymentSecret = false, areSupported = false),
Expand All @@ -405,7 +406,7 @@ class PaymentRequestSpec extends AnyFunSuite {

for ((features, res) <- featureBits) {
val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(123 msat), ByteVector32.One, priv, "Some invoice", CltvExpiryDelta(18), features = Some(features))
assert(Result(pr.features.allowMultiPart, pr.features.requirePaymentSecret, pr.features.supported) === res)
assert(Result(pr.features.allowMultiPart, pr.features.requirePaymentSecret, pr.features.areSupported(nodeParams)) === res)
assert(PaymentRequest.read(PaymentRequest.write(pr)) === pr)
}
}
Expand Down