Make payment_secret mandatory (#1810)

This is a security feature that has been introduced a long time ago and is
widely supported across the network.

We can safely make it mandatory which closes probing attack vectors.

eclair-core/src/main/resources/reference.conf

@@ -44,9 +44,9 @@ eclair {
     option_data_loss_protect = optional
     gossip_queries = optional
     gossip_queries_ex = optional
-    var_onion_optin = optional
+    var_onion_optin = mandatory
     option_static_remotekey = optional
-    payment_secret = optional
+    payment_secret = mandatory
     basic_mpp = optional
     option_support_large_channel = optional
     option_anchor_outputs = disabled

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

@@ -236,7 +236,8 @@ object NodeParams extends Logging {
     def validateFeatures(features: Features): Unit = {
       val featuresErr = Features.validateFeatureGraph(features)
       require(featuresErr.isEmpty, featuresErr.map(_.message))
-      require(features.hasFeature(Features.VariableLengthOnion), s"${Features.VariableLengthOnion.rfcName} must be enabled")
+      require(features.hasFeature(Features.VariableLengthOnion, Some(FeatureSupport.Mandatory)), s"${Features.VariableLengthOnion.rfcName} must be enabled and mandatory")
+      require(features.hasFeature(Features.PaymentSecret, Some(FeatureSupport.Mandatory)), s"${Features.PaymentSecret.rfcName} must be enabled and mandatory")
       require(!features.hasFeature(Features.InitialRoutingSync), s"${Features.InitialRoutingSync.rfcName} is not supported anymore, use ${Features.ChannelRangeQueries.rfcName} instead")
     }
 

eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentRequest.scala

@@ -135,7 +135,7 @@ object PaymentRequest {
             expirySeconds: Option[Long] = None,
             extraHops: List[List[ExtraHop]] = Nil,
             timestamp: Long = System.currentTimeMillis() / 1000L,
-            features: Option[PaymentRequestFeatures] = Some(PaymentRequestFeatures(Features.VariableLengthOnion.optional, Features.PaymentSecret.optional))): PaymentRequest = {
+            features: Option[PaymentRequestFeatures] = Some(PaymentRequestFeatures(Features.VariableLengthOnion.mandatory, Features.PaymentSecret.mandatory))): PaymentRequest = {
 
     val prefix = prefixes(chainHash)
     val tags = {

eclair-core/src/main/scala/fr/acinq/eclair/payment/receive/MultiPartHandler.scala

@@ -57,10 +57,8 @@ class MultiPartHandler(nodeParams: NodeParams, register: ActorRef, db: IncomingP
         val paymentPreimage = paymentPreimage_opt.getOrElse(randomBytes32)
         val paymentHash = Crypto.sha256(paymentPreimage)
         val expirySeconds = expirySeconds_opt.getOrElse(nodeParams.paymentRequestExpiry.toSeconds)
-        // We currently only optionally support payment secrets (to allow legacy clients to pay invoices).
-        // Once we're confident most of the network has upgraded, we should switch to mandatory payment secrets.
         val features = {
-          val f1 = Seq(Features.PaymentSecret.optional, Features.VariableLengthOnion.optional)
+          val f1 = Seq(Features.PaymentSecret.mandatory, Features.VariableLengthOnion.mandatory)
           val allowMultiPart = nodeParams.features.hasFeature(Features.BasicMultiPartPayment)
           val f2 = if (allowMultiPart) Seq(Features.BasicMultiPartPayment.optional) else Nil
           val f3 = if (nodeParams.enableTrampolinePayment) Seq(Features.TrampolinePayment.optional) else Nil
@@ -99,9 +97,9 @@ class MultiPartHandler(nodeParams: NodeParams, register: ActorRef, db: IncomingP
               val paymentHash = Crypto.sha256(paymentPreimage)
               val desc = "Donation"
               val features = if (nodeParams.features.hasFeature(Features.BasicMultiPartPayment)) {
-                PaymentRequestFeatures(Features.BasicMultiPartPayment.optional, Features.PaymentSecret.optional, Features.VariableLengthOnion.optional)
+                PaymentRequestFeatures(Features.BasicMultiPartPayment.optional, Features.PaymentSecret.mandatory, Features.VariableLengthOnion.mandatory)
               } else {
-                PaymentRequestFeatures(Features.PaymentSecret.optional, Features.VariableLengthOnion.optional)
+                PaymentRequestFeatures(Features.PaymentSecret.mandatory, Features.VariableLengthOnion.mandatory)
               }
 
               // Insert a fake invoice and then restart the incoming payment handler

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

@@ -94,8 +94,8 @@ class StartupSpec extends AnyFunSuite {
       s"features.${OptionDataLossProtect.rfcName}" -> "optional",
       s"features.${ChannelRangeQueries.rfcName}" -> "optional",
       s"features.${ChannelRangeQueriesExtended.rfcName}" -> "optional",
-      s"features.${VariableLengthOnion.rfcName}" -> "optional",
-      s"features.${PaymentSecret.rfcName}" -> "optional",
+      s"features.${VariableLengthOnion.rfcName}" -> "mandatory",
+      s"features.${PaymentSecret.rfcName}" -> "mandatory",
       s"features.${BasicMultiPartPayment.rfcName}" -> "optional"
     ).asJava)
 
@@ -106,22 +106,39 @@ class StartupSpec extends AnyFunSuite {
       s"features.${ChannelRangeQueriesExtended.rfcName}" -> "optional"
     ).asJava)
 
+    // var_onion_optin cannot be optional
+    val optionalVarOnionOptinConf = ConfigFactory.parseMap(Map(
+      s"features.${OptionDataLossProtect.rfcName}" -> "optional",
+      s"features.${VariableLengthOnion.rfcName}" -> "optional"
+    ).asJava)
+
+    // payment_secret cannot be optional
+    val optionalPaymentSecretConf = ConfigFactory.parseMap(Map(
+      s"features.${OptionDataLossProtect.rfcName}" -> "optional",
+      s"features.${VariableLengthOnion.rfcName}" -> "mandatory",
+      s"features.${PaymentSecret.rfcName}" -> "optional",
+    ).asJava)
+
     // initial_routing_sync cannot be enabled
     val initialRoutingSyncConf = ConfigFactory.parseMap(Map(
       s"features.${OptionDataLossProtect.rfcName}" -> "optional",
       s"features.${InitialRoutingSync.rfcName}" -> "optional",
       s"features.${ChannelRangeQueries.rfcName}" -> "optional",
       s"features.${ChannelRangeQueriesExtended.rfcName}" -> "optional",
-      s"features.${VariableLengthOnion.rfcName}" -> "optional"
+      s"features.${VariableLengthOnion.rfcName}" -> "mandatory",
+      s"features.${PaymentSecret.rfcName}" -> "mandatory",
     ).asJava)
 
-    // basic_mpp without payment_secret
+    // extended channel queries without channel queries
     val illegalFeaturesConf = ConfigFactory.parseMap(Map(
-      s"features.${BasicMultiPartPayment.rfcName}" -> "optional"
+      s"features.${OptionDataLossProtect.rfcName}" -> "optional",
+      s"features.${ChannelRangeQueriesExtended.rfcName}" -> "optional"
     ).asJava)
 
     assert(Try(makeNodeParamsWithDefaults(finalizeConf(legalFeaturesConf))).isSuccess)
     assert(Try(makeNodeParamsWithDefaults(finalizeConf(noVariableLengthOnionConf))).isFailure)
+    assert(Try(makeNodeParamsWithDefaults(finalizeConf(optionalVarOnionOptinConf))).isFailure)
+    assert(Try(makeNodeParamsWithDefaults(finalizeConf(optionalPaymentSecretConf))).isFailure)
     assert(Try(makeNodeParamsWithDefaults(finalizeConf(initialRoutingSyncConf))).isFailure)
     assert(Try(makeNodeParamsWithDefaults(finalizeConf(illegalFeaturesConf))).isFailure)
   }
@@ -133,7 +150,7 @@ class StartupSpec extends AnyFunSuite {
         |      {
         |        nodeid = "02aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
         |          features {
-        |            var_onion_optin = optional
+        |            var_onion_optin = mandatory
         |            payment_secret = mandatory
         |            basic_mpp = mandatory
         |          }
@@ -144,7 +161,7 @@ class StartupSpec extends AnyFunSuite {
 
     val nodeParams = makeNodeParamsWithDefaults(perNodeConf.withFallback(defaultConf))
     val perNodeFeatures = nodeParams.featuresFor(PublicKey(ByteVector.fromValidHex("02aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")))
-    assert(perNodeFeatures === Features(VariableLengthOnion -> Optional, PaymentSecret -> Mandatory, BasicMultiPartPayment -> Mandatory))
+    assert(perNodeFeatures === Features(VariableLengthOnion -> Mandatory, PaymentSecret -> Mandatory, BasicMultiPartPayment -> Mandatory))
   }
 
   test("override feerate mismatch tolerance") {

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

@@ -18,7 +18,7 @@ package fr.acinq.eclair
 
 import fr.acinq.bitcoin.Crypto.PrivateKey
 import fr.acinq.bitcoin.{Block, ByteVector32, Satoshi, SatoshiLong, Script}
-import fr.acinq.eclair.FeatureSupport.Optional
+import fr.acinq.eclair.FeatureSupport.{Mandatory, Optional}
 import fr.acinq.eclair.Features._
 import fr.acinq.eclair.blockchain.fee.{FeeEstimator, FeeTargets, FeeratesPerKw, OnChainFeeConf, _}
 import fr.acinq.eclair.channel.LocalParams
@@ -88,8 +88,8 @@ object TestConstants {
           OptionDataLossProtect -> Optional,
           ChannelRangeQueries -> Optional,
           ChannelRangeQueriesExtended -> Optional,
-          VariableLengthOnion -> Optional,
-          PaymentSecret -> Optional,
+          VariableLengthOnion -> Mandatory,
+          PaymentSecret -> Mandatory,
           BasicMultiPartPayment -> Optional
         ),
         Set(UnknownFeature(TestFeature.optional))
@@ -194,8 +194,8 @@ object TestConstants {
         OptionDataLossProtect -> Optional,
         ChannelRangeQueries -> Optional,
         ChannelRangeQueriesExtended -> Optional,
-        VariableLengthOnion -> Optional,
-        PaymentSecret -> Optional,
+        VariableLengthOnion -> Mandatory,
+        PaymentSecret -> Mandatory,
         BasicMultiPartPayment -> Optional
       ),
       pluginParams = Nil,

eclair-core/src/test/scala/fr/acinq/eclair/integration/IntegrationSpec.scala

@@ -86,8 +86,8 @@ abstract class IntegrationSpec extends TestKitBaseClass with BitcoindService wit
     s"eclair.features.${OptionDataLossProtect.rfcName}" -> "optional",
     s"eclair.features.${ChannelRangeQueries.rfcName}" -> "optional",
     s"eclair.features.${ChannelRangeQueriesExtended.rfcName}" -> "optional",
-    s"eclair.features.${VariableLengthOnion.rfcName}" -> "optional",
-    s"eclair.features.${PaymentSecret.rfcName}" -> "optional",
+    s"eclair.features.${VariableLengthOnion.rfcName}" -> "mandatory",
+    s"eclair.features.${PaymentSecret.rfcName}" -> "mandatory",
     s"eclair.features.${BasicMultiPartPayment.rfcName}" -> "optional"
   ).asJava)
 

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

@@ -20,8 +20,8 @@ import akka.actor.PoisonPill
 import akka.testkit.{TestFSMRef, TestProbe}
 import fr.acinq.bitcoin.Block
 import fr.acinq.bitcoin.Crypto.{PrivateKey, PublicKey}
-import fr.acinq.eclair.FeatureSupport.Optional
-import fr.acinq.eclair.Features.{BasicMultiPartPayment, ChannelRangeQueries, VariableLengthOnion}
+import fr.acinq.eclair.FeatureSupport.{Mandatory, Optional}
+import fr.acinq.eclair.Features.{BasicMultiPartPayment, ChannelRangeQueries, PaymentSecret, VariableLengthOnion}
 import fr.acinq.eclair.TestConstants._
 import fr.acinq.eclair._
 import fr.acinq.eclair.crypto.TransportHandler
@@ -190,7 +190,7 @@ class PeerConnectionSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike wi
 
   test("sync when requested") { f =>
     import f._
-    val remoteInit = protocol.Init(Features(ChannelRangeQueries -> Optional))
+    val remoteInit = protocol.Init(Features(ChannelRangeQueries -> Optional, VariableLengthOnion -> Mandatory, PaymentSecret -> Mandatory))
     connect(nodeParams, remoteNodeId, switchboard, router, connection, transport, peerConnection, peer, remoteInit, doSync = true)
   }
 

eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala

@@ -20,7 +20,7 @@ import akka.actor.ActorRef
 import akka.actor.Status.Failure
 import akka.testkit.{TestActorRef, TestProbe}
 import fr.acinq.bitcoin.{ByteVector32, Crypto}
-import fr.acinq.eclair.FeatureSupport.Optional
+import fr.acinq.eclair.FeatureSupport.{Mandatory, Optional}
 import fr.acinq.eclair.Features._
 import fr.acinq.eclair.TestConstants.Alice
 import fr.acinq.eclair.channel.{CMD_FAIL_HTLC, CMD_FULFILL_HTLC, Register}
@@ -45,18 +45,19 @@ import scala.concurrent.duration._
 class MultiPartHandlerSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike {
 
   val featuresWithoutMpp = Features(
-    VariableLengthOnion -> Optional,
-    PaymentSecret -> Optional,
+    VariableLengthOnion -> Mandatory,
+    PaymentSecret -> Mandatory,
   )
 
   val featuresWithMpp = Features(
-    VariableLengthOnion -> Optional,
-    PaymentSecret -> Optional,
+    VariableLengthOnion -> Mandatory,
+    PaymentSecret -> Mandatory,
     BasicMultiPartPayment -> Optional
   )
 
   val featuresWithKeySend = Features(
-    VariableLengthOnion -> Optional,
+    VariableLengthOnion -> Mandatory,
+    PaymentSecret -> Mandatory,
     KeySend -> Optional
   )
 

eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentInitiatorSpec.scala

@@ -19,7 +19,7 @@ package fr.acinq.eclair.payment
 import akka.actor.{ActorContext, ActorRef}
 import akka.testkit.{TestActorRef, TestProbe}
 import fr.acinq.bitcoin.Block
-import fr.acinq.eclair.FeatureSupport.Optional
+import fr.acinq.eclair.FeatureSupport.{Mandatory, Optional}
 import fr.acinq.eclair.Features._
 import fr.acinq.eclair.UInt64.Conversions._
 import fr.acinq.eclair.channel.Channel
@@ -53,13 +53,13 @@ class PaymentInitiatorSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
   case class FixtureParam(nodeParams: NodeParams, initiator: TestActorRef[PaymentInitiator], payFsm: TestProbe, multiPartPayFsm: TestProbe, sender: TestProbe, eventListener: TestProbe)
 
   val featuresWithoutMpp = Features(
-    VariableLengthOnion -> Optional,
-    PaymentSecret -> Optional
+    VariableLengthOnion -> Mandatory,
+    PaymentSecret -> Mandatory
   )
 
   val featuresWithMpp = Features(
-    VariableLengthOnion -> Optional,
-    PaymentSecret -> Optional,
+    VariableLengthOnion -> Mandatory,
+    PaymentSecret -> Mandatory,
     BasicMultiPartPayment -> Optional,
   )
 
@@ -143,7 +143,7 @@ class PaymentInitiatorSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
   test("forward single-part payment when multi-part deactivated", Tag("mpp_disabled")) { f =>
     import f._
     val finalExpiryDelta = CltvExpiryDelta(24)
-    val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(finalAmount), paymentHash, randomKey, "Some MPP invoice", finalExpiryDelta, features = Some(PaymentRequestFeatures(VariableLengthOnion.optional, PaymentSecret.optional, BasicMultiPartPayment.optional)))
+    val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(finalAmount), paymentHash, randomKey, "Some MPP invoice", finalExpiryDelta, features = Some(PaymentRequestFeatures(VariableLengthOnion.mandatory, PaymentSecret.mandatory, BasicMultiPartPayment.optional)))
     val req = SendPaymentRequest(finalAmount, paymentHash, c, 1, /* ignored since the invoice provides it */ CltvExpiryDelta(12), Some(pr))
     assert(req.finalExpiry(nodeParams.currentBlockHeight) === (finalExpiryDelta + 1).toCltvExpiry(nodeParams.currentBlockHeight))
     sender.send(initiator, req)
@@ -154,7 +154,7 @@ class PaymentInitiatorSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
 
   test("forward multi-part payment") { f =>
     import f._
-    val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(finalAmount), paymentHash, randomKey, "Some invoice", CltvExpiryDelta(18), features = Some(PaymentRequestFeatures(VariableLengthOnion.optional, PaymentSecret.optional, BasicMultiPartPayment.optional)))
+    val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(finalAmount), paymentHash, randomKey, "Some invoice", CltvExpiryDelta(18), features = Some(PaymentRequestFeatures(VariableLengthOnion.mandatory, PaymentSecret.mandatory, BasicMultiPartPayment.optional)))
     val req = SendPaymentRequest(finalAmount + 100.msat, paymentHash, c, 1, CltvExpiryDelta(42), Some(pr))
     sender.send(initiator, req)
     val id = sender.expectMsgType[UUID]
@@ -245,7 +245,7 @@ class PaymentInitiatorSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike
     assert(trampolinePayload.outgoingCltv.toLong === currentBlockCount + 9 + 1)
     assert(trampolinePayload.outgoingNodeId === c)
     assert(trampolinePayload.paymentSecret === pr.paymentSecret)
-    assert(trampolinePayload.invoiceFeatures === Some(hex"8200")) // var_onion_optin, payment_secret
+    assert(trampolinePayload.invoiceFeatures === Some(hex"4100")) // var_onion_optin, payment_secret
   }
 
   test("reject trampoline to legacy payment for 0-value invoice") { f =>

eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala

@@ -216,7 +216,7 @@ class PaymentPacketSpec extends AnyFunSuite with BeforeAndAfterAll {
     // a -> b -> c      d -> e
 
     val routingHints = List(List(PaymentRequest.ExtraHop(randomKey.publicKey, ShortChannelId(42), 10 msat, 100, CltvExpiryDelta(144))))
-    val invoiceFeatures = PaymentRequestFeatures(VariableLengthOnion.optional, PaymentSecret.optional, BasicMultiPartPayment.optional)
+    val invoiceFeatures = PaymentRequestFeatures(VariableLengthOnion.mandatory, PaymentSecret.mandatory, BasicMultiPartPayment.optional)
     val invoice = PaymentRequest(Block.RegtestGenesisBlock.hash, Some(finalAmount), paymentHash, priv_a.privateKey, "#reckless", CltvExpiryDelta(18), None, None, routingHints, features = Some(invoiceFeatures))
     val (amount_ac, expiry_ac, trampolineOnion) = buildTrampolineToLegacyPacket(invoice, trampolineHops, FinalLegacyPayload(finalAmount, finalExpiry))
     assert(amount_ac === amount_bc)
@@ -257,7 +257,7 @@ class PaymentPacketSpec extends AnyFunSuite with BeforeAndAfterAll {
     assert(inner_d.outgoingNodeId === e)
     assert(inner_d.totalAmount === finalAmount)
     assert(inner_d.paymentSecret === invoice.paymentSecret)
-    assert(inner_d.invoiceFeatures === Some(hex"028200")) // var_onion_optin, payment_secret, basic_mpp
+    assert(inner_d.invoiceFeatures === Some(hex"024100")) // var_onion_optin, payment_secret, basic_mpp
     assert(inner_d.invoiceRoutingInfo === Some(routingHints))
   }
 

eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentRequestSpec.scala

@@ -431,8 +431,8 @@ class PaymentRequestSpec extends AnyFunSuite {
   test("payment secret") {
     val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(123 msat), ByteVector32.One, priv, "Some invoice", CltvExpiryDelta(18))
     assert(pr.paymentSecret.isDefined)
-    assert(pr.features === PaymentRequestFeatures(PaymentSecret.optional, VariableLengthOnion.optional))
-    assert(!pr.features.requirePaymentSecret)
+    assert(pr.features === PaymentRequestFeatures(PaymentSecret.mandatory, VariableLengthOnion.mandatory))
+    assert(pr.features.requirePaymentSecret)
 
     val pr1 = PaymentRequest.read(PaymentRequest.write(pr))
     assert(pr1.paymentSecret === pr.paymentSecret)

eclair-core/src/test/scala/fr/acinq/eclair/payment/relay/NodeRelayerSpec.scala

@@ -575,7 +575,7 @@ class NodeRelayerSpec extends ScalaTestWithActorTestKit(ConfigFactory.load("appl
 
     // Receive an upstream multi-part payment.
     val hints = List(List(ExtraHop(outgoingNodeId, ShortChannelId(42), feeBase = 10 msat, feeProportionalMillionths = 1, cltvExpiryDelta = CltvExpiryDelta(12))))
-    val features = PaymentRequestFeatures(VariableLengthOnion.optional, PaymentSecret.mandatory, BasicMultiPartPayment.optional)
+    val features = PaymentRequestFeatures(VariableLengthOnion.mandatory, PaymentSecret.mandatory, BasicMultiPartPayment.optional)
     val pr = PaymentRequest(Block.LivenetGenesisBlock.hash, Some(outgoingAmount * 3), paymentHash, randomKey, "Some invoice", CltvExpiryDelta(18), extraHops = hints, features = Some(features))
     val incomingPayments = incomingMultiPart.map(incoming => incoming.copy(innerPayload = Onion.createNodeRelayToNonTrampolinePayload(
       incoming.innerPayload.amountToForward, outgoingAmount * 3, outgoingExpiry, outgoingNodeId, pr

eclair-core/src/test/scala/fr/acinq/eclair/router/AnnouncementsSpec.scala

@@ -53,7 +53,7 @@ class AnnouncementsSpec extends AnyFunSuite {
 
   test("create valid signed node announcement") {
     val ann = makeNodeAnnouncement(Alice.nodeParams.privateKey, Alice.nodeParams.alias, Alice.nodeParams.color, Alice.nodeParams.publicAddresses, Alice.nodeParams.features)
-    assert(ann.features.hasFeature(Features.VariableLengthOnion, Some(FeatureSupport.Optional)))
+    assert(ann.features.hasFeature(Features.VariableLengthOnion, Some(FeatureSupport.Mandatory)))
     assert(checkSig(ann))
     assert(checkSig(ann.copy(timestamp = 153)) === false)
   }