Xpay bad nodes #8608
Xpay bad nodes #8608
Conversation
Lagrang3
force-pushed
the
xpay-bad-nodes
branch
8 times, most recently
from
7f51753 to
9585fe2
Compare
We add one more field to biases: "timestamp". With the timestamp variable old biases can be removed with the askrene-age command. Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Changelog-Added: askrene-bias-node: an RPC command to set a bias on node's outgoing or incoming channels. Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Changelog-None Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Lagrang3
force-pushed
the
xpay-bad-nodes
branch
from
9585fe2 to
9e297ce
Compare
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
Minor API changes, and I need to be convinced of the xpay change.
| for (bias = bias_hash_first(layer->biases, &biasit); bias; | ||
| bias = bias_hash_next(layer->biases, &biasit)) { | ||
| if (bias->timestamp < cutoff) { | ||
| bias_hash_delval(layer->biases, &biasit); | ||
| tal_free(bias); | ||
| num_removed++; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is cool. but
- askrene-age needs to document this new behaviour!
- Changelog-None should be Changelog-Added: Plugins:
askrenechannel biases now have an associated timestamp, and are timed out byaskrene-age.
| "out_direction": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": [ | ||
| "If true the bias applies to outgoing channels otherwise it applies to incoming channels." | ||
| ] | ||
| } |
There was a problem hiding this comment.
Prefer this be called "direction", a string "in" or "out", and a required field.
I think this makes it more explicit.
| // FIXME: is this the correct direction? | ||
| biases[idx] += node_bias->out_bias; | ||
| biases[idx ^ 1] += node_bias->in_bias; |
There was a problem hiding this comment.
To answer your FIXME: yes, it's correct.
The direction for a node pair corresponds to the outgoing channel: that's because the node controls fees only on outgoing. So, biases[index] is indeed this node's outgoing.
|
|
||
| /* We add a negative bias to this channel to penalize it for other | ||
| * payments. | ||
| * Biases are nice way to penalize or encourage the use of | ||
| * channels. But it has some limitations. For example the bias would | ||
| * have no effect if the channel already provides 0 cost routing. */ | ||
| req = payment_ignored_req(aux_cmd, attempt, "askrene-bias-channel"); | ||
| json_add_string(req->js, "layer", XPAY_GLOBAL_LAYER); | ||
| json_add_short_channel_id_dir(req->js, "short_channel_id_dir", | ||
| attempt->hops[index].scidd); | ||
| json_add_s32(req->js, "bias", -5); | ||
| json_add_bool(req->js, "relative", true); | ||
| send_payment_req(aux_cmd, attempt->payment, req); | ||
|
|
||
| /* We add a negative bias to the node that owns this half channel. */ | ||
| if (index) { | ||
| req = | ||
| payment_ignored_req(aux_cmd, attempt, "askrene-bias-node"); | ||
| json_add_string(req->js, "layer", XPAY_GLOBAL_LAYER); | ||
| json_add_pubkey(req->js, "node", | ||
| &attempt->hops[index - 1].next_node); | ||
| json_add_s32(req->js, "bias", -1); | ||
| json_add_bool(req->js, "relative", true); | ||
| json_add_bool(req->js, "out_direction", true); | ||
| send_payment_req(aux_cmd, attempt->payment, req); | ||
| } | ||
| goto check_previous_success; |
There was a problem hiding this comment.
Do we have evidence to support this approach? We've effectively biased it already by lowering the capacity, after all.
There was a problem hiding this comment.
I don't think so. We add a bias to the channel that returned a weird error here, eg. WIRE_FEE_INSUFFICIENT or WIRE_PERMANENT_CHANNEL_FAILURE. We reduce the known liquidity only when we get a WIRE_TEMPORARY_CHANNEL_FAILURE.
There was a problem hiding this comment.
We disable those failing channels for the current payment session but other payments instances can pick them up again.
|
@rustyrussell: regarding the direction of the channels, either "in" or "out", it is true that it gives us more control but I can't think of a practical use case for making that distinction. If a node's out channels are heavily penalized that means we will eventually not going to route through it's in-channels anyways. |
|
I don't quite see a case where we'd ever set This is similar to the Boltz situation, since they keep their channels purposefully unbalanced, with most capacity on their peer's side. We temporarily fixed that by blocklisting all outgoing channels: this way we'd never end up at that node, unless it is the destination, because incoming edges are still available. And I think it can be generalized this way. I'd vote for removing the direction param, and just always using |
3 participants
Several changes to askrene, mainly to introduce penalties (biases) on nodes and channels that
can be used across payments.
It addresses issue #8600.
- [ ] add expiration on biases,- [ ] add expiration on knowledge data,