Don't rebroadcast channel updates from update_fail_htlc (#2775)

When we receive a `channel_update` in `update_fail_htlc`, we should take
it into account to exclude channels or correctly retry with updated fees,
but we shouldn't apply it to our routing table.

If we did, that could be exploited to deanonymize our payments.
It shouldn't be necessary anyway, as honest nodes should broadcast
those `channel_update`s publicly, so we would receive them through the
normal gossip mechanism.

Fixes #2767

eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala

@@ -24,7 +24,7 @@ import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey
 import fr.acinq.eclair._
 import fr.acinq.eclair.channel._
 import fr.acinq.eclair.crypto.{Sphinx, TransportHandler}
-import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus, PaymentType}
+import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus}
 import fr.acinq.eclair.payment.Invoice.ExtraEdge
 import fr.acinq.eclair.payment.Monitoring.{Metrics, Tags}
 import fr.acinq.eclair.payment.OutgoingPaymentPacket.Upstream
@@ -272,7 +272,16 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
   }
 
   /**
-   * Apply the channel update to our routing table.
+   * Analyze the channel_update we received and update our routing state accordingly.
+   *
+   * Note that we don't forward the channel update to the router, because it could leak that we were the payer:
+   *  - a malicious intermediate node would fail the payment with a custom channel_update
+   *  - they would *not* send this channel_update to the rest of the network
+   *  - they would then directly connect to us and ask for our latest channel_update for that channel
+   *  - if we sent them that channel_update, they'd know we were the payer
+   *
+   * We don't need to send that update to the router anyway: if the sending node is honest, we should receive it with
+   * the standard gossip mechanism.
    *
    * @return updated routing hints if applicable.
    */
@@ -318,9 +327,6 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A
         log.error(s"couldn't find node=$nodeId in the route, this should never happen")
         data.recipient.extraEdges
     }
-    // in all cases, we forward the update to the router: if the channel is disabled, the router will remove it from its routing table
-    // if the channel is not announced (e.g. was from a hint), the router will simply ignore the update
-    router ! failure.update
     // we update the recipient's assisted routes: they take precedence over the router's routing table
     data.recipient match {
       case recipient: ClearRecipient => recipient.copy(extraEdges = extraEdges1)

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

@@ -499,8 +499,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     assert(routerForwarder.expectMsgType[ChannelCouldNotRelay].hop.shortChannelId == update_bc.shortChannelId)
     routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c), Some(nodeParams.routerConf.channelExcludeDuration)))
     routerForwarder.forward(routerFixture.router)
-    // payment lifecycle forwards the embedded channelUpdate to the router
-    routerForwarder.expectMsg(update_bc)
     awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE)
     routerForwarder.expectMsg(defaultRouteRequest(a, cfg).copy(ignore = Ignore(Set.empty, Set(ChannelDesc(update_bc.shortChannelId, b, c)))))
     routerForwarder.forward(routerFixture.router)
@@ -530,8 +528,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     // and node replies with a failure containing a new channel update
     sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure)))))
 
-    // payment lifecycle forwards the embedded channelUpdate to the router
-    routerForwarder.expectMsg(channelUpdate_bc_modified)
     awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE && nodeParams.db.payments.getOutgoingPayment(id).exists(_.status == OutgoingPaymentStatus.Pending)) // 1 failure but not final, the payment is still PENDING
     routerForwarder.expectMsg(defaultRouteRequest(a, cfg))
     routerForwarder.forward(routerFixture.router)
@@ -550,8 +546,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     // this time the payment lifecycle will ask the router to temporarily exclude this channel from its route calculations
     routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c), Some(nodeParams.routerConf.channelExcludeDuration)))
     routerForwarder.forward(routerFixture.router)
-    // but it will still forward the embedded channelUpdate to the router
-    routerForwarder.expectMsg(channelUpdate_bc_modified_2)
     awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE)
     routerForwarder.expectMsg(defaultRouteRequest(a, cfg))
     routerForwarder.forward(routerFixture.router)
@@ -581,7 +575,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     // we should temporarily exclude that channel
     assert(routerForwarder.expectMsgType[ChannelCouldNotRelay].hop.shortChannelId == update_bc.shortChannelId)
     routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c), Some(nodeParams.routerConf.channelExcludeDuration)))
-    routerForwarder.expectMsg(update_bc)
 
     // this was a single attempt payment
     sender.expectMsgType[PaymentFailed]
@@ -614,8 +607,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     // and node replies with a failure containing a new channel update
     sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure)))))
 
-    // payment lifecycle forwards the embedded channelUpdate to the router
-    routerForwarder.expectMsg(channelUpdate_bc_modified)
     awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE && nodeParams.db.payments.getOutgoingPayment(id).exists(_.status == OutgoingPaymentStatus.Pending)) // 1 failure but not final, the payment is still PENDING
     val extraEdges1 = Seq(
       recipient.extraEdges(0).update(channelUpdate_bc_modified),
@@ -658,7 +649,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
 
     assert(routerForwarder.expectMsgType[RouteCouldRelay].route.hops.map(_.shortChannelId) == Seq(update_ab, update_bc).map(_.shortChannelId))
     routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_cd.shortChannelId, c, d), Some(nodeParams.routerConf.channelExcludeDuration)))
-    routerForwarder.expectMsg(channelUpdate_cd_disabled)
   }
 
   def testPermanentFailure(router: ActorRef, failure: FailureMessage): Unit = {
@@ -960,11 +950,9 @@ class PaymentLifecycleSpec extends BaseRouterSpec {
     // and node replies with a failure containing a new channel update
     sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure)))))
 
-    // payment lifecycle forwards the embedded channelUpdate to the router
-    routerForwarder.expectMsg(channelUpdate_bc_modified)
-
     // The payment fails without retrying
     sender.expectMsgType[PaymentFailed]
+    routerForwarder.expectNoMessage(100 millis) // we don't forward the channel_update to the router
   }
 
 }