[Lagrang3]

This is a follow-up PR from #9592.
We keep a record of rare errors from nodes during payment attempts.
Repeated strange errors trigger a negative bias that is applied to the node and if the errors keep coming
above a certain tolerance counter we disable the node for the current payment session.

It addresses a known issue of xpay retrying payment attempts through the same node even after
repeated unlikely error messages: #8600, #9590

[cdecker]

Two minor things that I'd like to see before merging this:

• Blocklist any channel that we got an unknown_next_peer from, otherwise we risk immediately retrying it, causing the tolerance to run through quicker than necessary.
• Implement the fee_insufficient error channel_update extraction, and add it to gossip_store. This could already resolve the issue of repeated fee_insufficient errors, as we now get the real feerate right away, and can try it immediately again if the MCF solver wants to use it. Also that'd remove one "weird" error

All in all I feel like the weird error mechanism can stay, but the two examples that are being used here may not be good matches (unknown_next_peer is an indication that we haven't cleaned the gossip_store using block spends, and fee_insufficient may very well be just the result of discarding the channel_update rather than persisting it).

[cdecker]

We still need to absolutely avoid this channel at all costs, the node told us it's no longer there. By biasing the node rather than banning the channel we risk reusing it and causing the node to be banned on the next attempt due to a repeated error.

[Lagrang3]

I tested a payment to Phoenix wallet:

• got an invoice inv1 from phoenix, routehint is some LSP only scid (16Mx...x...) not revealing the onchain channel,
• turned off the wallet,
• called xpay on core-lightning, payment failed with "unknown_next_peer" at scid,
• created a layer "disable-unknown-next-peer",
• disabled scid on that layer,
• opened up Phoenix,
• retried xpay invoice=inv1 and layer=["disable-unknown-next-peer"], got failure "All 1 channels to the destination are disabled",
• asked Phoenix for a second invoice inv2, same scid in the routehint,
• retried xpay invoice=inv2 and layer=["disable-unknown-next-peer"], got failure "All 1 channels to the destination are disabled",

Lesson learned: If we permanently disable channels that have given us "unknown_next_peer" (who knows about other other permanent errors),
we will shoot ourselves in the foot.

I get the point of doing something permanent if we get a "perm" error message. But that has to be carefully crafted for every message
considering the behavior of the other implementations. Can we do that in a follow up PR?

[nGoline]

CI is red because of the PR itself, not flakes:

• test_pay_exclude_node fails in 4 jobs because the new "adding a global bias to channel ..." line goes through add_result_summary, so it ends up in the error string returned to the user. I would log those two bias messages with attempt_log(..., LOG_DBG, ...) instead; they are internal bookkeeping, not something a pay caller needs to read. That also keeps the second commit bisectable.
• check-doc-examples wants xpay-failing-node-tolerance before xpay-handle-pay in listconfigs.json; make update-doc-examples should sort it (and the msggen schema).

On behaviour:

• The default tolerance of 4 undoes the #9590 fix that is on master but not released yet: that one excluded the node on the second unknown_next_peer. The #9590 test only passes now because it sets the tolerance to 2; with the default, xpay walks all four exits of the hub again, which is what the test docstring calls the bug. I would either default to 2, or keep the #9590 test on defaults and make sure it passes.
• Agree with @cdecker that a final-only error (incorrect_or_unknown_payment_details, mpp_timeout, final_incorrect_*) from an intermediate node should disable the node right away. The NODE-bit codes (temporary_node_failure, permanent_node_failure, required_node_feature_missing) fall in the same bucket: BOLT #4 says to remove all channels of the erring node, and today they need 4 distinct scids first.
• The node bias threshold is hardcoded to 2 while the disable threshold is configurable, and a tolerance of 0 disables on the first error. Worth tying them together, or at least documenting it.
• Minor: in the second commit, test_xpay_fee_insufficient_bias says "we should not need to disable the hub" but asserts that it was disabled.

On the two points in the review summary: for fee_insufficient, xpay already extracts the channel_update (process_channel_update_from_onion_error) and applies it to the payment's private layer, so only errors without a usable update reach this path. Putting it into gossip_store would go against BOLT #4, which says the sender MUST NOT apply an onion error channel_update to its local network graph. For unknown_next_peer, the channel is already disabled for the rest of the payment; your Phoenix experiment is a good argument against making that persistent, and I am fine with handling per-code permanent bans in a follow-up.

🤖 Generated with Claude Code

[Lagrang3]

Minor: in the second commit, test_xpay_fee_insufficient_bias says "we should not need to disable the hub" but asserts that it was disabled.

The assertion is: assert not payer.daemon.is_in_log(...), ie. that the "disabling node" line is not in the logs.

[Lagrang3]

The default tolerance of 4 undoes the #9590 fix that is on master but not released yet: that one excluded the node on the second unknown_next_peer. The #9590 test only passes now because it sets the tolerance to 2; with the default, xpay walks all four exits of the hub again, which is what the test docstring calls the bug. I would either default to 2, or keep the #9590 test on defaults and make sure it passes.

This test wants to prove that we are able to identify repeated "unknown_next_peer" errors from a hub and disable it.
The real bug is to keep trying it ad infinitum without any changes to our routing graph.
The least changes needed to make this test pass after we introduced the failure tolerance was to set it to 2 in this particular case.
Another possibility could have been to assert that we tried 4 times (with a hub that has more than 4 channels) before disabling it.

I don't like to set the default tolerance to 2 because we might be disabling a bridge node to the destination, so first adding a bias to it
would encourage the routing algorithm to search alternative routes, while not excluding the possibility that the only route is through that node.

I do agree that we could have a default tolerance of 3.

[Lagrang3]

The node bias threshold is hardcoded to 2 while the disable threshold is configurable, and a tolerance of 0 disables on the first error. Worth tying them together, or at least documenting it.

Worth tying and documenting it.

[Lagrang3]

Agree with @cdecker that a final-only error (incorrect_or_unknown_payment_details, mpp_timeout, final_incorrect_*) from an intermediate node should disable the node right away. The NODE-bit codes (temporary_node_failure, permanent_node_failure, required_node_feature_missing) fall in the same bucket: BOLT #4 says to remove all channels of the erring node, and today they need 4 distinct scids first.

Agree. Will do.

[Lagrang3]

@cdecker

Implement the fee_insufficient error channel_update extraction, and add it to gossip_store. This could already resolve the issue of repeated fee_insufficient errors, as we now get the real feerate right away, and can try it immediately again if the MCF solver wants to use it. Also that'd remove one "weird" error

We already have a branch that detects a channel update in the onion error. If present this channel update will be used to update the
details of the failing channel through the "askrene-update-channel" rpc instead of feeding the gossip store. There are some checks in place
to prevent a node from feeding up channel updates for a different channel than the one he controls. Renepay instead would call addgossip right away.
Therefore when we log a node that reports a fee_insufficient error it is implied that there either was not channel update or the channel update
was unusable.

[cdecker]

Ah, ok, I had missed that is already being added to the gossip store, if we have one, and mixed the two cases with and without the update 👍

[Lagrang3]

A drawback could be the fact that we don't actually add the channel update to the gossip store, but to askrene instead. I think the rationale is that we will eventually learn of that gossip message.

[Lagrang3]

Ah, ok, I had missed that is already being added to the gossip store, if we have one, and mixed the two cases with and without the update 👍

Though, I am not sure if that branch is covered by the integration tests.
UPD: Yes. Just checked. There are tests to cover channel updates.