Add missing break when scoring a path with a missing channel

[TheBlueMatt]

If we send payments over a path where a channel ended up being closed, we'll remove it before we call
ProbabilisticPaymentScorer::payment_path_failed. This should be fine, except that payment_path_failed does not break out of its scoring loop if a channel is missing, causing it to assign a minimum available-liquidity of the payment amount even to channels which our attempt never arrived at.

The fix is simple - add the missing check and break.

[TheBlueMatt]

Needs a test, probably, but I'm not gonna do that right now.

[codecov-commenter]

Codecov Report

Base: 90.77% // Head: 91.30% // Increases project coverage by +0.52% 🎉

Coverage data is based on head (00607a5) compared to base (6ca4994).
Patch coverage: 98.11% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1817      +/-   ##
==========================================
+ Coverage   90.77%   91.30%   +0.52%     
==========================================
  Files          87       87              
  Lines       47371    49634    +2263     
  Branches    47371    49634    +2263     
==========================================
+ Hits        43003    45317    +2314     
+ Misses       4368     4317      -51     

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	96.89% <98.11%> (+0.28%)	⬆️
lightning-net-tokio/src/lib.rs	76.73% <0.00%> (-0.31%)	⬇️
lightning/src/util/events.rs	38.40% <0.00%> (+0.26%)	⬆️
lightning/src/chain/onchaintx.rs	96.17% <0.00%> (+0.88%)	⬆️
lightning/src/ln/functional_tests.rs	98.08% <0.00%> (+1.03%)	⬆️
lightning/src/routing/gossip.rs	94.45% <0.00%> (+2.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tnull]

Makes sense. Just want to note that we'd probably want to solve this differently if we'd ever have to deal with paths in which the unannounced channels are located somewhere other than the beginning or end. I imagine this could be the case for trampoline nodes, but scoring for that probably requires rethinking a lot of things anyways.

Not sure why benchmark CI is failing, couldn't reproduce locally.

[jkczyz]

Needs a test, probably, but I'm not gonna do that right now.

Yeah, test is definitely needed here.

[TheBlueMatt]

Just want to note that we'd probably want to solve this differently if we'd ever have to deal with paths in which the unannounced channels are located somewhere other than the beginning or end.

I'm not sure why that's the case? We don't want to break until we find the channel that caused the failure, if there's some channels in the middle that don't have a graph entry we should just ignore them. I believe that's what this change does.

[tnull]

I'm not sure why that's the case? We don't want to break until we find the channel that caused the failure, if there's some channels in the middle that don't have a graph entry we should just ignore them. I believe that's what this change does.

I was under the impression that we'd break whenever we hit the first unknown hop on the failing path, but you're right, the hop.short_channel_id == short_channel_id check should ensure that we only stop penalizing hops after the failed one. So nevermind then.

[TheBlueMatt]

Squashed without further changes.