Payee abstraction for use in get_route and PaymentPathFailed #1134
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1134 +/- ##
==========================================
+ Coverage 90.40% 91.49% +1.08%
==========================================
Files 68 68
Lines 34796 40055 +5259
==========================================
+ Hits 31458 36647 +5189
- Misses 3338 3408 +70
Continue to review full report at Codecov.
|
jkczyz
force-pushed
the
2021-10-payee-arg
branch
2 times, most recently
from
ffae900 to
74d8896
Compare
jkczyz
force-pushed
the
2021-10-payee-arg
branch
from
74d8896 to
b57b613
Compare
| } | ||
|
|
||
| /// Includes the payee's features. | ||
| pub fn with_features(self, features: InvoiceFeatures) -> Self { |
There was a problem hiding this comment.
Hmm, I thought based on this discussion this type of constructor wasn't supported in the bindings? Wondering what's different in this case
There was a problem hiding this comment.
Yea, I kinda hate to say it cause its quite nice, but to map this in bindings we have to clone at the ffi. We can keep these constructors, but maybe add a (C-not exported) tag to them and then just make the actual struct fields pub?
There was a problem hiding this comment.
Ah, right... foiled! Took @TheBlueMatt suggestion to maintain expressiveness. I could also make them take &self at the expense of a clone, FWIW. Let me know if you'd prefer that over not exposing them in bindings / making the fields pub.
| @@ -3131,6 +3133,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
| all_paths_failed, | |||
| path: path.clone(), | |||
| short_channel_id: Some(path.first().unwrap().short_channel_id), | |||
| retry: None, | |||
There was a problem hiding this comment.
Hmm, little confused why this isn't set anywhere atm -- maybe I should know this lol, but any TL;DR about how this PR fits into the largest retries picture?
There was a problem hiding this comment.
@TheBlueMatt is working on it. Getting this change in allows me to work on the InvoicePayer side independently.
lightning/src/routing/router.rs
Outdated
| (2, short_channel_id, required), | ||
| (4, fees, required), | ||
| (6, cltv_expiry_delta, required), | ||
| (1, htlc_minimum_msat, option), |
There was a problem hiding this comment.
Don't these need to go in type-order? Is there any test coverage of serializing a RouteHintHop?
There was a problem hiding this comment.
Whoops... was hoping we had some magic to write order. 🙂
Re-ordered. I could make them even instead. Not sure if you have a preference on these as they aren't in the invoice.
I guess we'll have coverage once PaymentPathRetry is populated in PaymentPathFailed.
There was a problem hiding this comment.
Yea, that all sounds fine, I don't have a preference about required or not.
| } | ||
|
|
||
| /// Includes the payee's features. | ||
| pub fn with_features(self, features: InvoiceFeatures) -> Self { |
There was a problem hiding this comment.
Yea, I kinda hate to say it cause its quite nice, but to map this in bindings we have to clone at the ffi. We can keep these constructors, but maybe add a (C-not exported) tag to them and then just make the actual struct fields pub?
|
|
||
| /// The recipient of a payment. | ||
| #[derive(Clone, Debug)] | ||
| pub struct Payee { |
There was a problem hiding this comment.
To make sure we're on the same page - do we intend to pass a Payee to ChannelManager::send_payment explicitly, or should we include it in the Route object as an extra field?
There was a problem hiding this comment.
Hmm... good question. Was thinking we'd pass it in to send_payment. Otherwise, you'd pass it to get_route only to have it cloned to include in the returned Route.
There was a problem hiding this comment.
I think I don't have a strong feelings. It kinda feels cool if its just kinda automagically passed in for you, without users having to do anything to get it there or any chance of setting it wrong or something, but its also a bit of a strange API, agreed there.
There was a problem hiding this comment.
That's a good point. We'll need to copy the payee potentially for each path anyhow. I'd be ok with adding a field to Route.
jkczyz
force-pushed
the
2021-10-payee-arg
branch
from
b57b613 to
d09debd
Compare
jkczyz
force-pushed
the
2021-10-payee-arg
branch
2 times, most recently
from
5b7f044 to
6477442
Compare
jkczyz
force-pushed
the
2021-10-payee-arg
branch
from
6477442 to
b20f5e9
Compare
A payee can be identified by a pubkey and optionally have an associated set of invoice features and route hints. Use this in get_route instead of three separate parameters. This may be included in PaymentPathFailed later to use when finding a new route.
When a payment path fails, it may be retried. Typically, this means re-computing the route after updating the NetworkGraph and channel scores in order to avoid the failing hop. The last hop in PaymentPathFailed's path field contains the pubkey, amount, and CLTV values needed to pass to get_route. However, it does not contain the payee's features and route hints from the invoice. Include the entire set of parameters in PaymentPathRetry and add it to the PaymentPathFailed event. Add a get_retry_route wrapper around get_route that takes PaymentPathRetry. This allows an EventHandler to retry failed payment paths using the payee's route hints and features.
Using ignorable TLV decoding is only applicable for an Option containing an enum, but short_channel_id is an Option<u64>. Use option TLV encoding instead.
jkczyz
force-pushed
the
2021-10-payee-arg
branch
from
b20f5e9 to
8e7ceab
Compare
Refactor's
get_routeto take aPayeeparameter consisting of the payee's pubkey along with invoice features and route hints, if any. This is included inPaymentPathFailedas part ofPaymentPathRetryalong with a final amount and CLTV, used for re-computing aRoutewhen retrying payments.