diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala index 381ce075e1..809ee8b4ca 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala +++ b/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) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala index a598b0e583..0022597a83 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala +++ b/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 } }