add expiry_time to PendingOutboundPayment::StaticInvoiceReceived
#3918
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
|
It is weird that mutants modify the constant to check for tests. Is it because we want to test based on the constant ? 🤔 fuzz failed because of a memory issue on the vm |
lightning/src/ln/outbound_payment.rs
Outdated
| @@ -2661,6 +2684,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, | |||
| (6, route_params, required), | |||
| (8, invoice_request, required), | |||
| (10, static_invoice, required), | |||
| (12, expiry_time, required), | |||
There was a problem hiding this comment.
This variant probably hasn't been written, but IMO we should still set the TLV type to an odd value to support downgrading from 0.2+ to 0.1. We can also set a default_value of 0, I think, so downgrading to 0.1 then upgrading would result in immediately timing out the payment, though it would be good to include a release note for that.
Suggestion:
| (12, expiry_time, required), | |
| (11, expiry_time, (default_value, StaleExpiration::AbsoluteTimeout(Duration::from_secs(0))), |
+ a release note for the downgrade/upgrade behavior. Let me know if that doesn't make sense.
There was a problem hiding this comment.
oh would that mean that when upgrading from 0.1 to 0.2+ and default value is zero, everything from previous version would be timed out too?
Maybe being optional is better in this case and manage the None case as no expiry_time 🤔
There was a problem hiding this comment.
Added a commit using None I'm not loving it but kinda of works
There was a problem hiding this comment.
No one's using this feature yet almost certainly, so honestly I'd prefer to just time out payments when going from 0.1 to 0.2+ and having a release note noting that this will happen, so we can avoid the Option.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Previous this commit if the StaticInvoice has an expiration time of months or years would make our HTLC to hold for that time until is abandoned. This patch adds a defaults of 1 week of expiry time that the HTLC will be held waiting for the often-offline node comes online.
a-mpch
force-pushed
the
2025-07-expiry-time-static-invoice-received
branch
from
9b282a4 to
58b51a5
Compare
|
So I rebased it and added fixups for all comments beside the TLVs. That probable the default value being |
IMO we're alright on upgrade/downgrade tests, I don't think anyone's using this feature |
a-mpch
force-pushed
the
2025-07-expiry-time-static-invoice-received
branch
from
e723094 to
2a7c9a2
Compare
a-mpch
force-pushed
the
2025-07-expiry-time-static-invoice-received
branch
from
2a7c9a2 to
e3bc8c3
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
great, all comments should be addressed. I leave the fixup so changes are clear. |
Context
This is a follow up to Matt's comment from #3140
Whenever paying to an often-offline node and invoice has an expiration time of weeks / months / years and node does not come online soonish we would hold the HTLC for the entire time until "we remember that have to do something about it"
Solution
This sets a
expiry_timetoStaticInvoiceReceivedusingStaleExpirationthat is defaulted (hardcoded) to one week. Then is used to time out any HTLC pending after that expiration time.Some decisions over the way:
Not really sure about not being configurable but maybe is worth changing it in a follow up when being used.