Skip to content

Intercept HTLC forwards for JIT channels #1835

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

Prev Previous commit
Next Next commit
Allow failing back intercepted HTLCs
Co-authored-by: John Cantrell <johncantrell97@gmail.com>
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
  • Loading branch information
valentinewallace and johncantrell97 committed Nov 30, 2022
commit f79ad2efb161d8ea9ccda9d67cf0ee2d63586cbe
32 changes: 31 additions & 1 deletion lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3059,7 +3059,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
///
/// To make use of intercepted HTLCs, use [`ChannelManager::get_intercept_scid`] to generate short
/// channel id(s) to put in the receiver's invoice route hints. These route hints will signal to
/// LDK to generate an [`HTLCIntercepted`] event when it receives the forwarded HTLC.
/// LDK to generate an [`HTLCIntercepted`] event when it receives the forwarded HTLC, and this
/// method or [`ChannelManager::fail_intercepted_htlc`] MUST be called in response to the event.
///
/// Note that LDK does not enforce fee requirements in `amt_to_forward_msat`, and will not stop
/// you from forwarding more than you received.
Expand Down Expand Up @@ -3102,6 +3103,35 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
Ok(())
}

/// Fails the intercepted HTLC indicated by intercept_id. Should only be called in response to
/// an [`HTLCIntercepted`] event. See [`ChannelManager::forward_intercepted_htlc`].
///
/// [`HTLCIntercepted`]: events::Event::HTLCIntercepted
pub fn fail_intercepted_htlc(&self, intercept_id: InterceptId) -> Result<(), APIError> {
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);

let payment = self.pending_intercepted_htlcs.lock().unwrap().remove(&intercept_id)
.ok_or_else(|| APIError::APIMisuseError {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating this line does not break intercepted_payment

err: format!("Payment with InterceptId {:?} not found", intercept_id)
})?;

if let PendingHTLCRouting::Forward { short_channel_id, .. } = payment.forward_info.routing {
let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData {
short_channel_id: payment.prev_short_channel_id,
outpoint: payment.prev_funding_outpoint,
htlc_id: payment.prev_htlc_id,
incoming_packet_shared_secret: payment.forward_info.incoming_shared_secret,
phantom_shared_secret: None,
});

let failure_reason = HTLCFailReason::Reason { failure_code: 0x4000 | 10, data: Vec::new() };
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this value choice of an onion error message be standardized, otherwise if Eclair use PERM|1 (invalid_realm) and LND use UPDATE|20 (channel_disabled), we could have some implementation fingerprint in the support of offline-receive.

Do you think it could be worthy to introduce a new PERM|23 (custom_routing_policy_unsatisfied) at the spec-level as a catch-all for HTLC intercepting application ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are easier ways to fingerprint implementations as-is, but it would be good for the LSP spec (which this feature is based on) to specify what error should be returned.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can add a comment, suggesting the error could be a custom one and as a reminder to update if/when the LSP spec adopts one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to do anything else here, and it definitely doesn't make sense to have a custom error. The error we return here absolutely should be the same error as if we didn't have intercepts enabled, which makes unknown_next_peer correct.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After understanding this PR is only about JIT channel, I think effectively we shouldn't introduce a custom error. We can revisit in the future, when offline receive at the receiver's LSP or things like "delay my HTLC", introduces HTLC delay processing and it makes sense to return new onion errors.

let destination = HTLCDestination::UnknownNextHop { requested_forward_scid: short_channel_id };
self.fail_htlc_backwards_internal(htlc_source, &payment.forward_info.payment_hash, failure_reason, destination);
} else { unreachable!() } // Only `PendingHTLCRouting::Forward`s are intercepted

Ok(())
}

/// Processes HTLCs which are pending waiting on random forward delay.
///
/// Should only really ever be called in response to a PendingHTLCsForwardable event.
Expand Down
116 changes: 71 additions & 45 deletions lightning/src/ln/payment_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1374,9 +1374,15 @@ fn test_holding_cell_inflight_htlcs() {
}

#[test]
fn forward_intercepted_payment() {
fn intercepted_payment() {
// Test that detecting an intercept scid on payment forward will signal LDK to generate an
// intercept event, which the LSP can then use to open a JIT channel to forward the payment.
// intercept event, which the LSP can then use to either (a) open a JIT channel to forward the
// payment or (b) fail the payment.
do_test_intercepted_payment(false);
do_test_intercepted_payment(true);
}

fn do_test_intercepted_payment(fail_intercept: bool) {
let chanmon_cfgs = create_chanmon_cfgs(3);
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
let mut chan_config = test_default_channel_config();
Expand Down Expand Up @@ -1449,49 +1455,69 @@ fn forward_intercepted_payment() {
let unknown_chan_id_err = nodes[1].node.forward_intercepted_htlc(intercept_id, &[42; 32], nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
assert_eq!(unknown_chan_id_err , APIError::APIMisuseError { err: format!("Channel with id {:?} not found", [42; 32]) });

// Open the just-in-time channel so the payment can then be forwarded.
let (_, channel_id) = open_zero_conf_channel(&nodes[1], &nodes[2], None);

// Check for unknown intercept id error.
let unknown_intercept_id = InterceptId([42; 32]);
let unknown_intercept_id_err = nodes[1].node.forward_intercepted_htlc(unknown_intercept_id, &channel_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
assert_eq!(unknown_intercept_id_err , APIError::APIMisuseError { err: format!("Payment with intercept id {:?} not found", unknown_intercept_id.0) });

// Finally, forward the intercepted payment through and claim it.
nodes[1].node.forward_intercepted_htlc(intercept_id, &channel_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap();
expect_pending_htlcs_forwardable!(nodes[1]);

let payment_event = {
{
let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
assert_eq!(added_monitors.len(), 1);
added_monitors.clear();
if fail_intercept {
// Ensure we can fail the intercepted payment back.
nodes[1].node.fail_intercepted_htlc(intercept_id).unwrap();
expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[1], vec![HTLCDestination::UnknownNextHop { requested_forward_scid: intercept_scid }]);
nodes[1].node.process_pending_htlc_forwards();
let update_fail = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
check_added_monitors!(&nodes[1], 1);
assert!(update_fail.update_fail_htlcs.len() == 1);
let fail_msg = update_fail.update_fail_htlcs[0].clone();
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
commitment_signed_dance!(nodes[0], nodes[1], update_fail.commitment_signed, false);

// Ensure the payment fails with the expected error.
let fail_conditions = PaymentFailedConditions::new()
.blamed_scid(intercept_scid)
.blamed_chan_closed(true)
.expected_htlc_error_data(0x4000 | 10, &[]);
expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
} else {
// Open the just-in-time channel so the payment can then be forwarded.
let (_, channel_id) = open_zero_conf_channel(&nodes[1], &nodes[2], None);

// Check for unknown intercept id error.
let unknown_intercept_id = InterceptId([42; 32]);
let unknown_intercept_id_err = nodes[1].node.forward_intercepted_htlc(unknown_intercept_id, &channel_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
assert_eq!(unknown_intercept_id_err , APIError::APIMisuseError { err: format!("Payment with intercept id {:?} not found", unknown_intercept_id.0) });

// Finally, forward the intercepted payment through and claim it.
nodes[1].node.forward_intercepted_htlc(intercept_id, &channel_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap();
expect_pending_htlcs_forwardable!(nodes[1]);

let payment_event = {
{
let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
assert_eq!(added_monitors.len(), 1);
added_monitors.clear();
}
let mut events = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
SendEvent::from_event(events.remove(0))
};
nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
commitment_signed_dance!(nodes[2], nodes[1], &payment_event.commitment_msg, false, true);
expect_pending_htlcs_forwardable!(nodes[2]);

let payment_preimage = nodes[2].node.get_payment_preimage(payment_hash, payment_secret).unwrap();
expect_payment_received!(&nodes[2], payment_hash, payment_secret, amt_msat, Some(payment_preimage), nodes[2].node.get_our_node_id());
do_claim_payment_along_route(&nodes[0], &vec!(&vec!(&nodes[1], &nodes[2])[..]), false, payment_preimage);
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 2);
match events[0] {
Event::PaymentSent { payment_preimage: ref ev_preimage, payment_hash: ref ev_hash, ref fee_paid_msat, .. } => {
assert_eq!(payment_preimage, *ev_preimage);
assert_eq!(payment_hash, *ev_hash);
assert_eq!(fee_paid_msat, &Some(1000));
},
_ => panic!("Unexpected event")
}
match events[1] {
Event::PaymentPathSuccessful { payment_hash: hash, .. } => {
assert_eq!(hash, Some(payment_hash));
},
_ => panic!("Unexpected event")
}
let mut events = nodes[1].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
SendEvent::from_event(events.remove(0))
};
nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
commitment_signed_dance!(nodes[2], nodes[1], &payment_event.commitment_msg, false, true);
expect_pending_htlcs_forwardable!(nodes[2]);

let payment_preimage = nodes[2].node.get_payment_preimage(payment_hash, payment_secret).unwrap();
expect_payment_received!(&nodes[2], payment_hash, payment_secret, amt_msat, Some(payment_preimage), nodes[2].node.get_our_node_id());
do_claim_payment_along_route(&nodes[0], &vec!(&vec!(&nodes[1], &nodes[2])[..]), false, payment_preimage);
let events = nodes[0].node.get_and_clear_pending_events();
assert_eq!(events.len(), 2);
match events[0] {
Event::PaymentSent { payment_preimage: ref ev_preimage, payment_hash: ref ev_hash, ref fee_paid_msat, .. } => {
assert_eq!(payment_preimage, *ev_preimage);
assert_eq!(payment_hash, *ev_hash);
assert_eq!(fee_paid_msat, &Some(1000));
},
_ => panic!("Unexpected event")
}
match events[1] {
Event::PaymentPathSuccessful { payment_hash: hash, .. } => {
assert_eq!(hash, Some(payment_hash));
},
_ => panic!("Unexpected event")
}
}
6 changes: 6 additions & 0 deletions lightning/src/util/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,13 @@ pub enum Event {
/// you've encoded an intercept scid in the receiver's invoice route hints using
/// [`ChannelManager::get_intercept_scid`].
///
/// [`ChannelManager::forward_intercepted_htlc`] or
/// [`ChannelManager::fail_intercepted_htlc`] MUST be called in response to this event. See
/// their docs for more information.
///
/// [`ChannelManager::get_intercept_scid`]: crate::ln::channelmanager::ChannelManager::get_intercept_scid
/// [`ChannelManager::forward_intercepted_htlc`]: crate::ln::channelmanager::ChannelManager::forward_intercepted_htlc
/// [`ChannelManager::fail_intercepted_htlc`]: crate::ln::channelmanager::ChannelManager::fail_intercepted_htlc
HTLCIntercepted {
/// An id to help LDK identify which HTLC is being forwarded or failed.
intercept_id: InterceptId,
Expand Down