Repository navigation
xpay: stop retrying a node that repeats unknown_next_peer - #9592
vincenzopalazzo wants to merge 4 commits into
Conversation
unknown_next_peer means the reporting node cannot forward, which is
what lnd returns when the next peer is offline. xpay only disables the
failed channel, so getroutes hands back every other channel through the
same node.
This test fails a hub with four exits using that code. On current xpay
it walks all four:
Failed after 4 attempts. We got a weird error (unknown_next_peer)
for 131x1x0/1 ... 124x1x0/1 ... 117x1x0/1 ... 110x2x0/0
assert 4 <= 2
Marked xfail until the next commit excludes the node after the repeat.
Changelog-None.
The previous commit fails because xpay disables only the channel. A
second unknown_next_peer from the same node now calls
askrene-disable-node on this payment's private layer, so getroutes
stops handing back the rest of its channels.
The first failure still only disables that channel. One stale scid
must not take a well-connected node out of every path.
temporary_channel_failure and fee or CLTV errors are unchanged.
Removes the xfail from test_xpay_unknown_next_peer_excludes_node.
With the fix that test stops after two attempts:
Repeated unknown_next_peer from <hub>: disabling node for this payment
Fixes ElementsProject#9590.
Changelog-Fixed: xpay no longer retries every channel through a node that keeps returning unknown_next_peer.
bab20a2 to
a97e697
Compare
| case WIRE_UNKNOWN_NEXT_PEER: | ||
| /* One hit can be a stale scid, so disable that channel. | ||
| * The same node returning it again is excluded, or we | ||
| * walk every other channel through it. */ | ||
| add_result_summary(attempt, LOG_DBG, | ||
| "We got %s for %s: disabling it for this payment", | ||
| errmsg, | ||
| describe_scidd(attempt, index)); | ||
| maybe_exclude_unknown_next_peer(aux_cmd, attempt, index); | ||
| goto disable_channel; | ||
|
|
There was a problem hiding this comment.
Why disable the reporting node?
We should disable the unreachable node instead.
Alternatively we could at first distrust both the reporter (A) and next node (B), keep that in record and disable the channel. Next time we hit another channel with the same error and the same pair (A->B) we figure that there is something wrong with
these two, so we disable every parallel channel between them.
If we see another report of C failing with unknown next peer B, then we know for sure B was the problem
and we disable B.
Or if we see another report A failing with unknown next peer D, then we know is A the problem, and we disable A.
There was a problem hiding this comment.
Not a bug. unknown_next_peer does not name the next peer. It is PERM|10, channel scoped, and the onion only carries the scid the reporter rejected. hops[index - 1].next_node is that reporter, and that is the node we disable on the second hit. That is the #9590 case: one hub returns unknown_next_peer on every exit, so getroutes keeps handing back the other channels. The first hit still only disables that channel, so one stale scid does not take the node out.
Disabling hops[index].next_node on the first repeat would drop a live peer when the upstream lied or has a stale scid. The A/B record, then disable B only if C also reports B, or A if A reports a different next peer, is the right distinction. It needs new per-payment state and tests for both cases. Happy to do that as a follow-up. Does that address your concern, or did I misunderstand?
a87c380 to
e39a6b8
Compare
This branch has not been deployed
4 participants
Summary
xpaytreatedunknown_next_peeras a single-channel failure. That is the code lnd returns when the next peer is offline, so a well-connected node was retried on every other channel untilretry_forexpired. Fixes Bug:xpayfails because it keeps retrying paths over the same node #9590.unknown_next_peerfrom the same node callsaskrene-disable-nodeon the private payment layer, sogetroutesstops handing back the rest of its channels.Test plan
test_xpay_unknown_next_peer_excludes_nodefails on master: 4 attempts, oneunknown_next_peerper hub channel (assert 4 <= 2)Repeated unknown_next_peer from <hub>: disabling node for this payment