-
Notifications
You must be signed in to change notification settings - Fork 962
Xpay bad nodes #8608
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
base: master
Are you sure you want to change the base?
Xpay bad nodes #8608
Conversation
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 <[email protected]>
Changelog-Added: askrene-bias-node: an RPC command to set a bias on node's outgoing or incoming channels. Signed-off-by: Lagrang3 <[email protected]>
Changelog-None Signed-off-by: Lagrang3 <[email protected]>
Changelog-None Signed-off-by: Lagrang3 <[email protected]>
9585fe2 to
9e297ce
Compare
rustyrussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have evidence to support this approach? We've effectively biased it already by lowering the capacity, after all.
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,