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.

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

MPP routing improvements #1219

Merged
merged 6 commits into from
Nov 21, 2019
Merged

MPP routing improvements #1219

merged 6 commits into from
Nov 21, 2019

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Nov 20, 2019

I found a few things than can be improved in the MPP retry behavior.
Those will be particularly helpful for Trampoline-MPP to a remote peer (take into account the fact that a Trampoline node will likely have many channels).

Improvement 1: sending to a direct peer should not retry

It doesn't make any sense to let the PaymentLifecycle retry when we're forcing the choice of the first hop and the recipient is on the other end of that channel.
This is fixed by setting maxAttempts=1 when sending child payments via direct channels to the recipient.

Improvement 2: only use public channels for remote recipients

When the recipient isn't a direct peer, it's very unlikely that choosing a route prefix with an unannounced channel will succeed. Most unannounced channels Endurance/Horizon has are with mobile wallets. We ignore them when sending to remote nodes.

Improvement 3: handle RouteNotFound

We're choosing the first hop without looking at the graph. That means we may choose a first hop that can't yield a complete route to our recipient. When that happens, we should avoid retrying with that same hop.

If we have a lot of channels, it's likely that we'll often choose a bad first hop. Since we only retry 5 times, this will lead to many payment failures which is bad when we're relaying (Trampoline). So we add some randomness on whether to count a LocalFailure as a complete attempt or "refund" that attempt.

Note that this hack won't be necessary once we move the MPP logic inside the router: when that happens we will only select first hops that are on a potentially valid route to the recipient.

Unrelated

I also fixed the random failure that was happening in one of the MPP IntegrationSpec test.
With the code that's running on master, the PaymentInitiator doesn't guarantee that it will send the PaymentId before payment events.
Now we guarantee that.

* Only use public channels when sending to remote node
* Don't retry when sending to direct peer
* Blacklist channels that are a bad route prefix
We now guarantee that a sender receives the PaymentId before any payment event.
@t-bast t-bast requested a review from pm47 November 20, 2019 13:38
@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c76cc5b). Click here to learn what that means.
The diff coverage is 94.11%.

@@            Coverage Diff            @@
##             master    #1219   +/-   ##
=========================================
  Coverage          ?   76.41%           
=========================================
  Files             ?      140           
  Lines             ?     9513           
  Branches          ?      375           
=========================================
  Hits              ?     7269           
  Misses            ?     2244           
  Partials          ?        0
Impacted Files Coverage Δ
...r/acinq/eclair/payment/send/PaymentLifecycle.scala 90% <100%> (ø)
...r/acinq/eclair/payment/send/PaymentInitiator.scala 100% <100%> (ø)
...clair/payment/send/MultiPartPaymentLifecycle.scala 96.36% <92.3%> (ø)

@t-bast t-bast requested a review from pm47 November 20, 2019 16:49
@t-bast t-bast merged commit 321ecef into master Nov 21, 2019
@t-bast t-bast deleted the mpp-routing-improvements branch November 21, 2019 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants