Prioritize private channels when relaying payments (#3248)

When relaying payments, we want to select private channels first and
keep as much liquidity available as possible in public channels, to
ensure that we don't send a `channel_update` that would otherwise
disable the public channel (and thus make the private channels also
unusable since they aren't visible by path-finding algorithms) or
limit the `htlc_maximum_msat` of this public channel (which also
indirectly applies to private channels).

We also change the order in which we select channels that have the
same visibility: we prioritize channels with smaller balances, to
ensure that we keep our larger balances for larger payments. When
balances are equal, we prioritize the largest channel, which creates
a larger inbound liquidity to allow receiving larger payments.

Author: t-bast

eclair-core/src/main/scala/fr/acinq/eclair/payment/relay/ChannelRelay.scala

@@ -382,24 +382,20 @@ class ChannelRelay private(nodeParams: NodeParams,
           })
         (channel, relayResult)
       }
-      .collect {
-        // we only keep channels that have enough balance to handle this payment
-        case (channel, _: RelaySuccess) if channel.commitments.availableBalanceForSend > r.amountToForward => channel
-      }
+      // we only keep channels that have enough balance to handle this payment
+      .collect { case (channel, _: RelaySuccess) if channel.commitments.availableBalanceForSend > r.amountToForward => channel }
       .toList // needed for ordering
-      // we want to use the channel with:
-      //  - the lowest available capacity to ensure we keep high-capacity channels for big payments
-      //  - the lowest available balance to increase our incoming liquidity
-      .sortBy { channel => (channel.commitments.latest.capacity, channel.commitments.availableBalanceForSend) }
-      .headOption match {
-      case Some(channel) =>
-        if (requestedChannelId_opt.contains(channel.channelId)) {
-          context.log.debug("requested short channel id is our preferred channel")
-          Some(channel)
-        } else {
-          context.log.debug("replacing requestedShortChannelId={} by preferredShortChannelId={} with availableBalanceMsat={}", requestedShortChannelId_opt, channel.channelUpdate.shortChannelId, channel.commitments.availableBalanceForSend)
-          Some(channel)
-        }
+      .sortWith {
+        // we always prioritize private channels to avoid exhausting our public channels and disabling them or lowering their htlc_maximum_msat
+        case (channel1, channel2) if channel1.commitments.announceChannel != channel2.commitments.announceChannel => !channel1.commitments.announceChannel
+        // otherwise, we use the channel with the smallest capacity to ensure we keep high-capacity channels enabled
+        // (if we ran out of liquidity in a large channels, we would send a channel_update to disable it, which would
+        // negatively impact our score in path-finding algorithms)
+        case (channel1, channel2) if channel1.commitments.capacity != channel2.commitments.capacity => channel1.commitments.capacity <= channel2.commitments.capacity
+        // otherwise, we use the channel with the smallest balance, to ensure we keep higher balances for larger payments
+        case (channel1, channel2) => channel1.commitments.availableBalanceForSend <= channel2.commitments.availableBalanceForSend
+      }.headOption match {
+      case Some(channel) => Some(channel)
       case None =>
         val requestedChannel_opt = requestedChannelId_opt.flatMap(channels.get)
         requestedChannel_opt match {

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

@@ -575,31 +575,36 @@ class ChannelRelayerSpec extends ScalaTestWithActorTestKit(ConfigFactory.load("a
     import f._
 
     /** This is just a simplified helper function with random values for fields we are not using here */
-    def dummyLocalUpdate(shortChannelId: RealShortChannelId, remoteNodeId: PublicKey, availableBalanceForSend: MilliSatoshi, capacity: Satoshi) = {
+    def dummyLocalUpdate(shortChannelId: RealShortChannelId, remoteNodeId: PublicKey, availableBalanceForSend: MilliSatoshi, capacity: Satoshi, isPublic: Boolean = true): LocalChannelUpdate = {
       val channelId = randomBytes32()
       val update = Announcements.makeChannelUpdate(Block.RegtestGenesisBlock.hash, randomKey(), remoteNodeId, shortChannelId, CltvExpiryDelta(10), 100 msat, 1000 msat, 100, capacity.toMilliSatoshi)
-      val ann = Announcements.makeChannelAnnouncement(Block.RegtestGenesisBlock.hash, shortChannelId, TestConstants.Alice.nodeParams.nodeId, outgoingNodeId, randomKey().publicKey, randomKey().publicKey, randomBytes64(), randomBytes64(), randomBytes64(), randomBytes64())
-      val commitments = PaymentPacketSpec.makeCommitments(channelId, availableBalanceForSend, testCapacity = capacity, announcement_opt = Some(ann))
+      val ann_opt = if (isPublic) {
+        Some(Announcements.makeChannelAnnouncement(Block.RegtestGenesisBlock.hash, shortChannelId, TestConstants.Alice.nodeParams.nodeId, outgoingNodeId, randomKey().publicKey, randomKey().publicKey, randomBytes64(), randomBytes64(), randomBytes64(), randomBytes64()))
+      } else {
+        None
+      }
+      val commitments = PaymentPacketSpec.makeCommitments(channelId, availableBalanceForSend, testCapacity = capacity, announcement_opt = ann_opt)
       val aliases = ShortIdAliases(localAlias = ShortChannelId.generateLocalAlias(), remoteAlias_opt = None)
-      LocalChannelUpdate(null, channelId, aliases, remoteNodeId, Some(AnnouncedCommitment(commitments.latest.commitment, ann)), update, commitments)
+      LocalChannelUpdate(null, channelId, aliases, remoteNodeId, ann_opt.map(ann => AnnouncedCommitment(commitments.latest.commitment, ann)), update, commitments)
     }
 
     val (a, b) = (randomKey().publicKey, randomKey().publicKey)
 
     val channelUpdates = Map(
-      ShortChannelId(11111) -> dummyLocalUpdate(RealShortChannelId(11111), a, 100000000 msat, 200000 sat),
-      ShortChannelId(12345) -> dummyLocalUpdate(RealShortChannelId(12345), a, 10000000 msat, 200000 sat),
-      ShortChannelId(22222) -> dummyLocalUpdate(RealShortChannelId(22222), a, 10000000 msat, 100000 sat),
-      ShortChannelId(22223) -> dummyLocalUpdate(RealShortChannelId(22223), a, 9000000 msat, 50000 sat),
-      ShortChannelId(33333) -> dummyLocalUpdate(RealShortChannelId(33333), a, 100000 msat, 50000 sat),
-      ShortChannelId(44444) -> dummyLocalUpdate(RealShortChannelId(44444), b, 1000000 msat, 10000 sat),
+      ShortChannelId(11111) -> dummyLocalUpdate(RealShortChannelId(11111), a, 100_000_000 msat, 200_000 sat),
+      ShortChannelId(12345) -> dummyLocalUpdate(RealShortChannelId(12345), a, 10_000_000 msat, 200_000 sat),
+      ShortChannelId(22222) -> dummyLocalUpdate(RealShortChannelId(22222), a, 10_000_000 msat, 100_000 sat),
+      ShortChannelId(22223) -> dummyLocalUpdate(RealShortChannelId(22223), a, 9_000_000 msat, 50_000 sat),
+      ShortChannelId(33333) -> dummyLocalUpdate(RealShortChannelId(33333), a, 100_000 msat, 50_000 sat),
+      ShortChannelId(33334) -> dummyLocalUpdate(RealShortChannelId(33334), a, 150_000 msat, 75_000 sat, isPublic = false),
+      ShortChannelId(44444) -> dummyLocalUpdate(RealShortChannelId(44444), b, 1_000_000 msat, 10_000 sat),
     )
 
     channelUpdates.values.foreach(u => channelRelayer ! WrappedLocalChannelUpdate(u))
 
     {
-      val payload = ChannelRelay.Standard(ShortChannelId(12345), 998900 msat, CltvExpiry(60), upgradeAccountability = false)
-      val r = createValidIncomingPacket(payload, 1000000 msat, CltvExpiry(70))
+      val payload = ChannelRelay.Standard(ShortChannelId(12345), 998_900 msat, CltvExpiry(60), upgradeAccountability = false)
+      val r = createValidIncomingPacket(payload, 1_000_000 msat, CltvExpiry(70))
       channelRelayer ! Relay(r, TestConstants.Alice.nodeParams.nodeId, 0.1)
       receiveConfidence(Reputation.Score(1.0, accountable = false))
       // select the channel to the same node, with the lowest capacity and balance but still high enough to handle the payment
@@ -618,41 +623,43 @@ class ChannelRelayerSpec extends ScalaTestWithActorTestKit(ConfigFactory.load("a
       expectFwdFail(register, r.add.channelId, CMD_FAIL_HTLC(r.add.id, FailureReason.LocalFailure(TemporaryChannelFailure(Some(channelUpdates(ShortChannelId(12345)).channelUpdate))), None, commit = true))
     }
     {
-      // higher amount payment (have to increased incoming htlc amount for fees to be sufficient)
-      val payload = ChannelRelay.Standard(ShortChannelId(12345), 50000000 msat, CltvExpiry(60), upgradeAccountability = false)
-      val r = createValidIncomingPacket(payload, 60000000 msat, CltvExpiry(70))
+      // higher amount payment (have to increase incoming htlc amount for fees to be sufficient)
+      val payload = ChannelRelay.Standard(ShortChannelId(12345), 50_000_000 msat, CltvExpiry(60), upgradeAccountability = false)
+      val r = createValidIncomingPacket(payload, 60_000_000 msat, CltvExpiry(70))
       channelRelayer ! Relay(r, TestConstants.Alice.nodeParams.nodeId, 0.1)
       receiveConfidence(Reputation.Score(1.0, accountable = false))
       expectFwdAdd(register, channelUpdates(ShortChannelId(11111)).channelId, r.amountToForward, r.outgoingCltv, outAccountable = false).message
     }
     {
-      // lower amount payment
+      // lower payment amount, which adds more candidate channels: we prioritize private channels regardless of capacity
       val payload = ChannelRelay.Standard(ShortChannelId(12345), 1000 msat, CltvExpiry(60), upgradeAccountability = false)
-      val r = createValidIncomingPacket(payload, 60000000 msat, CltvExpiry(70))
+      val r = createValidIncomingPacket(payload, 60_000_000 msat, CltvExpiry(70))
       channelRelayer ! Relay(r, TestConstants.Alice.nodeParams.nodeId, 0.1)
       receiveConfidence(Reputation.Score(1.0, accountable = false))
+      val cmd1 = expectFwdAdd(register, channelUpdates(ShortChannelId(33334)).channelId, r.amountToForward, r.outgoingCltv, outAccountable = false).message
+      cmd1.replyTo ! RES_ADD_FAILED(cmd1, ChannelUnavailable(randomBytes32()), None)
       expectFwdAdd(register, channelUpdates(ShortChannelId(33333)).channelId, r.amountToForward, r.outgoingCltv, outAccountable = false).message
     }
     {
       // payment too high, no suitable channel found, we keep the requested one
-      val payload = ChannelRelay.Standard(ShortChannelId(12345), 1000000000 msat, CltvExpiry(60), upgradeAccountability = false)
-      val r = createValidIncomingPacket(payload, 1010000000 msat, CltvExpiry(70))
+      val payload = ChannelRelay.Standard(ShortChannelId(12345), 1_000_000_000 msat, CltvExpiry(60), upgradeAccountability = false)
+      val r = createValidIncomingPacket(payload, 1_010_000_000 msat, CltvExpiry(70))
       channelRelayer ! Relay(r, TestConstants.Alice.nodeParams.nodeId, 0.1)
       receiveConfidence(Reputation.Score(1.0, accountable = false))
       expectFwdAdd(register, channelUpdates(ShortChannelId(12345)).channelId, r.amountToForward, r.outgoingCltv, outAccountable = false).message
     }
     {
       // cltv expiry larger than our requirements
-      val payload = ChannelRelay.Standard(ShortChannelId(12345), 998900 msat, CltvExpiry(50), upgradeAccountability = false)
-      val r = createValidIncomingPacket(payload, 1000000 msat, CltvExpiry(70))
+      val payload = ChannelRelay.Standard(ShortChannelId(12345), 998_900 msat, CltvExpiry(50), upgradeAccountability = false)
+      val r = createValidIncomingPacket(payload, 1_000_000 msat, CltvExpiry(70))
       channelRelayer ! Relay(r, TestConstants.Alice.nodeParams.nodeId, 0.1)
       receiveConfidence(Reputation.Score(1.0, accountable = false))
       expectFwdAdd(register, channelUpdates(ShortChannelId(22223)).channelId, r.amountToForward, r.outgoingCltv, outAccountable = false).message
     }
     {
       // cltv expiry too small, no suitable channel found
-      val payload = ChannelRelay.Standard(ShortChannelId(12345), 998900 msat, CltvExpiry(61), upgradeAccountability = false)
-      val r = createValidIncomingPacket(payload, 1000000 msat, CltvExpiry(70))
+      val payload = ChannelRelay.Standard(ShortChannelId(12345), 998_900 msat, CltvExpiry(61), upgradeAccountability = false)
+      val r = createValidIncomingPacket(payload, 1_000_000 msat, CltvExpiry(70))
       channelRelayer ! Relay(r, TestConstants.Alice.nodeParams.nodeId, 0.1)
       receiveConfidence(Reputation.Score(1.0, accountable = false))
       expectFwdFail(register, r.add.channelId, CMD_FAIL_HTLC(r.add.id, FailureReason.LocalFailure(IncorrectCltvExpiry(CltvExpiry(61), Some(channelUpdates(ShortChannelId(12345)).channelUpdate))), None, commit = true))