-
Notifications
You must be signed in to change notification settings - Fork 401
LSPS2: Fail (or abandon) intercepted HTLCs if LSP channel open fails #3712
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: main
Are you sure you want to change the base?
LSPS2: Fail (or abandon) intercepted HTLCs if LSP channel open fails #3712
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3712 +/- ##
==========================================
- Coverage 89.05% 89.04% -0.02%
==========================================
Files 155 155
Lines 122019 122162 +143
Branches 122019 122162 +143
==========================================
+ Hits 108666 108774 +108
- Misses 10695 10717 +22
- Partials 2658 2671 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add two LSPS2Service methods: 'Abandoned' prunes all channel open state. 'Failed' resets JIT channel to fail HTLCs. It allows a retry on channel open. Closes lightningdevkit#3479.
Add integration tests to verify channel open reset and pruning handlers. Tests cover: - channel_open_failed resetting state to allow retry. - channel_open_failed error on invalid state. - channel_open_abandoned pruning all open state. - error handling for nonexistent channels.
f50496f
to
0d8ba10
Compare
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
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.
Thanks for looking into this, and excuse the delay! We'll probably wait with this until after #3509 landed either way though.
/// without resetting the state for a potential payment retry. It removes the intercept SCID | ||
/// mapping along with any outbound channel state and related channel ID mappings associated with | ||
/// the specified `user_channel_id`. | ||
pub fn channel_open_abandoned( |
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.
I think to make this a bit safer, it should probably fail if we're already at PendingPaymentForward
or later, i.e., we already opened the channel?
Also, what would happen if this is called after the LSPS2ServiceEvent::OpenChannel
has been emitted?
})?; | ||
|
||
let jit_channel = peer_state | ||
.outbound_channels_by_intercept_scid |
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.
nit: indentation seems off.
), | ||
})?; | ||
|
||
jit_channel.state = match &jit_channel.state { |
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.
Maybe use the matches!
macro here?
@@ -24,6 +25,7 @@ use lightning_invoice::{Bolt11Invoice, InvoiceBuilder, RoutingFees}; | |||
use bitcoin::hashes::{sha256, Hash}; | |||
use bitcoin::secp256k1::{PublicKey, Secp256k1}; | |||
use bitcoin::Network; | |||
use lightning_types::payment::PaymentHash; |
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.
nit: move import up.
👋 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. |
Closes #3479
Allows the LSP to signal when a channel open fails or is abandoned.
PendingInitialPayment
, allowing the payer to retry.Inspired by previous work and a suggestion from @tnull to add an abandon variant apart from the reset.