Skip to content
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

Complete route blinding support #2812

Merged
31 changes: 28 additions & 3 deletions lightning/src/ln/blinded_payment_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,29 @@ fn two_hop_blinded_path_success() {
claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);
}

#[test]
fn three_hop_blinded_path_success() {
let chanmon_cfgs = create_chanmon_cfgs(5);
let node_cfgs = create_node_cfgs(5, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(5, &node_cfgs, &[None, None, None, None, None]);
let mut nodes = create_network(5, &node_cfgs, &node_chanmgrs);
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0);
let chan_upd_2_3 = create_announced_chan_between_nodes_with_value(&nodes, 2, 3, 1_000_000, 0).0.contents;
let chan_upd_3_4 = create_announced_chan_between_nodes_with_value(&nodes, 3, 4, 1_000_000, 0).0.contents;

let amt_msat = 5000;
let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[4], Some(amt_msat), None);
let route_params = get_blinded_route_parameters(amt_msat, payment_secret,
nodes.iter().skip(2).map(|n| n.node.get_our_node_id()).collect(),
&[&chan_upd_2_3, &chan_upd_3_4], &chanmon_cfgs[4].keys_manager);

nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
check_added_monitors(&nodes[0], 1);
pass_along_route(&nodes[0], &[&[&nodes[1], &nodes[2], &nodes[3], &nodes[4]]], amt_msat, payment_hash, payment_secret);
claim_payment(&nodes[0], &[&nodes[1], &nodes[2], &nodes[3], &nodes[4]], payment_preimage);
}

#[derive(PartialEq)]
enum ReceiveCheckFail {
// The recipient fails the payment upon `PaymentClaimable`.
Expand Down Expand Up @@ -537,19 +560,20 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
};

let amt_msat = 5000;
let final_cltv_delta = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
let excess_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
Some(TEST_FINAL_CLTV as u16 - 2)
} else { None };
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), final_cltv_delta);
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), excess_final_cltv_delta_opt);
Comment on lines +563 to +567
Copy link

Choose a reason for hiding this comment

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

The variable excess_final_cltv_delta_opt is used to set a CLTV delta that is too low, but the name does not clearly reflect its purpose. Consider renaming it to more accurately describe its function, such as insufficient_final_cltv_delta_opt.

- let excess_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
+ let insufficient_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
let excess_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
Some(TEST_FINAL_CLTV as u16 - 2)
} else { None };
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), final_cltv_delta);
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), excess_final_cltv_delta_opt);
let insufficient_final_cltv_delta_opt = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
Some(TEST_FINAL_CLTV as u16 - 2)
} else { None };
let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), insufficient_final_cltv_delta_opt);

let mut route_params = get_blinded_route_parameters(amt_msat, payment_secret,
nodes.iter().skip(1).map(|n| n.node.get_our_node_id()).collect(), &[&chan_upd_1_2],
&chanmon_cfgs[2].keys_manager);

let route = if check == ReceiveCheckFail::ProcessPendingHTLCsCheck {
let mut route = get_route(&nodes[0], &route_params).unwrap();
// Set the final CLTV expiry too low to trigger the failure in process_pending_htlc_forwards.
route.paths[0].blinded_tail.as_mut().map(|bt| bt.excess_final_cltv_expiry_delta = TEST_FINAL_CLTV - 2);
route.paths[0].hops.last_mut().map(|h| h.cltv_expiry_delta += excess_final_cltv_delta_opt.unwrap() as u32);
route.paths[0].blinded_tail.as_mut().map(|bt| bt.excess_final_cltv_expiry_delta = excess_final_cltv_delta_opt.unwrap() as u32);
route
} else if check == ReceiveCheckFail::PaymentConstraints {
// Create a blinded path where the receiver's encrypted payload has an htlc_minimum_msat that is
Expand Down Expand Up @@ -657,6 +681,7 @@ fn do_multi_hop_receiver_fail(check: ReceiveCheckFail) {
commitment_signed_dance!(nodes[2], nodes[1], (), false, true, false, false);
},
ReceiveCheckFail::ProcessPendingHTLCsCheck => {
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32);
Copy link

Choose a reason for hiding this comment

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

The assertion on line 684 checks the cltv_expiry against a value derived from the block height and an excess_final_cltv_delta_opt. However, the name excess_final_cltv_delta_opt suggests an excess value, while the context implies it is insufficient. This could lead to confusion. The variable should be renamed to reflect its purpose more clearly, as suggested in the previous comment.

- assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32);
+ assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + insufficient_final_cltv_delta_opt.unwrap() as u32);

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + excess_final_cltv_delta_opt.unwrap() as u32);
assert_eq!(payment_event_1_2.msgs[0].cltv_expiry, nodes[0].best_block_info().1 + 1 + insufficient_final_cltv_delta_opt.unwrap() as u32);

nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event_1_2.msgs[0]);
check_added_monitors!(nodes[2], 0);
do_commitment_signed_dance(&nodes[2], &nodes[1], &payment_event_1_2.commitment_msg, true, true);
Expand Down
25 changes: 17 additions & 8 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,15 +202,16 @@ pub struct BlindedForward {
/// onion payload if we're the introduction node. Useful for calculating the next hop's
/// [`msgs::UpdateAddHTLC::blinding_point`].
pub inbound_blinding_point: PublicKey,
// Another field will be added here when we support forwarding as a non-intro node.
/// If needed, this determines how this HTLC should be failed backwards, based on whether we are
/// the introduction node.
pub failure: BlindedFailure,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... maybe this should be named BlindedSource now that it is no longer just use in a failure context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, it should only be used in a failure context. Could you point me to where you're seeing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

By "used" I meant more that it is used in this struct even though described "if needed". I suppose the struct name is fine given how it is actually used, but the variable name failure makes it seem like something has already failed when in fact in this context it might not ever. I'm fine if there's not a better name.

}

impl PendingHTLCRouting {
// Used to override the onion failure code and data if the HTLC is blinded.
fn blinded_failure(&self) -> Option<BlindedFailure> {
// TODO: needs update when we support forwarding blinded HTLCs as non-intro node
match self {
Self::Forward { blinded: Some(_), .. } => Some(BlindedFailure::FromIntroductionNode),
Self::Forward { blinded: Some(BlindedForward { failure, .. }), .. } => Some(*failure),
Self::Receive { requires_blinded_error: true, .. } => Some(BlindedFailure::FromBlindedNode),
_ => None,
}
Expand Down Expand Up @@ -305,10 +306,15 @@ pub(super) enum HTLCForwardInfo {
},
}

// Used for failing blinded HTLCs backwards correctly.
/// Whether this blinded HTLC is being failed backwards by the introduction node or a blinded node,
/// which determines the failure message that should be used.
#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
enum BlindedFailure {
pub enum BlindedFailure {
/// This HTLC is being failed backwards by the introduction node, and thus should be failed with
/// [`msgs::UpdateFailHTLC`] and error code `0x8000|0x4000|24`.
FromIntroductionNode,
/// This HTLC is being failed backwards by a blinded node within the path, and thus should be
/// failed with [`msgs::UpdateFailMalformedHTLC`] and error code `0x8000|0x4000|24`.
FromBlindedNode,
}

Expand Down Expand Up @@ -3024,8 +3030,9 @@ where

let is_intro_node_forward = match next_hop {
onion_utils::Hop::Forward {
// TODO: update this when we support blinded forwarding as non-intro node
next_hop_data: msgs::InboundOnionPayload::BlindedForward { .. }, ..
next_hop_data: msgs::InboundOnionPayload::BlindedForward {
intro_node_blinding_point: Some(_), ..
}, ..
} => true,
_ => false,
};
Expand Down Expand Up @@ -4370,7 +4377,7 @@ where
incoming_packet_shared_secret: incoming_shared_secret,
// Phantom payments are only PendingHTLCRouting::Receive.
phantom_shared_secret: None,
blinded_failure: blinded.map(|_| BlindedFailure::FromIntroductionNode),
blinded_failure: blinded.map(|b| b.failure),
});
let next_blinding_point = blinded.and_then(|b| {
let encrypted_tlvs_ss = self.node_signer.ecdh(
Expand Down Expand Up @@ -9350,6 +9357,7 @@ pub fn provided_init_features(config: &UserConfig) -> InitFeatures {
features.set_channel_type_optional();
features.set_scid_privacy_optional();
features.set_zero_conf_optional();
features.set_route_blinding_optional();
if config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx {
features.set_anchors_zero_fee_htlc_tx_optional();
}
Expand Down Expand Up @@ -9495,6 +9503,7 @@ impl_writeable_tlv_based!(PhantomRouteHints, {

impl_writeable_tlv_based!(BlindedForward, {
(0, inbound_blinding_point, required),
(1, failure, (default_value, BlindedFailure::FromIntroductionNode)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't we previously only support forwarding as an intermediate node, so shouldn't the default here be FromBlindedNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no we previously only supported intro node forwarding.

});

impl_writeable_tlv_based_enum!(PendingHTLCRouting,
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/msgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1714,7 +1714,7 @@ mod fuzzy_internal_msgs {
payment_relay: PaymentRelay,
payment_constraints: PaymentConstraints,
features: BlindedHopFeatures,
intro_node_blinding_point: PublicKey,
intro_node_blinding_point: Option<PublicKey>,
},
BlindedReceive {
sender_intended_htlc_amt_msat: u64,
Expand Down Expand Up @@ -2394,7 +2394,7 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, &NS)> for InboundOnionPayload w
payment_relay,
payment_constraints,
features,
intro_node_blinding_point: intro_node_blinding_point.ok_or(DecodeError::InvalidValue)?,
intro_node_blinding_point,
})
},
ChaChaPolyReadAdapter { readable: BlindedPaymentTlvs::Receive(ReceiveTlvs {
Expand Down
14 changes: 10 additions & 4 deletions lightning/src/ln/onion_payment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::blinded_path;
use crate::blinded_path::payment::{PaymentConstraints, PaymentRelay};
use crate::chain::channelmonitor::{HTLC_FAIL_BACK_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS};
use crate::ln::PaymentHash;
use crate::ln::channelmanager::{BlindedForward, CLTV_FAR_FAR_AWAY, HTLCFailureMsg, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting};
use crate::ln::channelmanager::{BlindedFailure, BlindedForward, CLTV_FAR_FAR_AWAY, HTLCFailureMsg, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting};
use crate::ln::features::BlindedHopFeatures;
use crate::ln::msgs;
use crate::ln::onion_utils;
Expand Down Expand Up @@ -73,7 +73,7 @@ pub(super) fn create_fwd_pending_htlc_info(
};

let (
short_channel_id, amt_to_forward, outgoing_cltv_value, inbound_blinding_point
short_channel_id, amt_to_forward, outgoing_cltv_value, intro_node_blinding_point
) = match hop_data {
msgs::InboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } =>
(short_channel_id, amt_to_forward, outgoing_cltv_value, None),
Expand All @@ -91,7 +91,7 @@ pub(super) fn create_fwd_pending_htlc_info(
err_data: vec![0; 32],
}
})?;
(short_channel_id, amt_to_forward, outgoing_cltv_value, Some(intro_node_blinding_point))
(short_channel_id, amt_to_forward, outgoing_cltv_value, intro_node_blinding_point)
},
msgs::InboundOnionPayload::Receive { .. } | msgs::InboundOnionPayload::BlindedReceive { .. } =>
return Err(InboundHTLCErr {
Expand All @@ -105,7 +105,13 @@ pub(super) fn create_fwd_pending_htlc_info(
routing: PendingHTLCRouting::Forward {
onion_packet: outgoing_packet,
short_channel_id,
blinded: inbound_blinding_point.map(|bp| BlindedForward { inbound_blinding_point: bp }),
blinded: intro_node_blinding_point.or(msg.blinding_point)
.map(|bp| BlindedForward {
inbound_blinding_point: bp,
failure: intro_node_blinding_point
.map(|_| BlindedFailure::FromIntroductionNode)
.unwrap_or(BlindedFailure::FromBlindedNode),
}),
},
payment_hash: msg.payment_hash,
incoming_shared_secret: shared_secret,
Expand Down
3 changes: 1 addition & 2 deletions lightning/src/ln/onion_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,10 @@ pub(super) fn build_onion_payloads(path: &Path, total_msat: u64, mut recipient_o
for (i, blinded_hop) in hops.iter().enumerate() {
if i == hops.len() - 1 {
cur_value_msat += final_value_msat;
cur_cltv += excess_final_cltv_expiry_delta;
res.push(msgs::OutboundOnionPayload::BlindedReceive {
sender_intended_htlc_amt_msat: *final_value_msat,
total_msat,
cltv_expiry_height: cltv,
cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,
Copy link

Choose a reason for hiding this comment

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

The cltv_expiry_height calculation has been modified to include excess_final_cltv_expiry_delta. This change could potentially introduce an off-by-one error if cur_cltv is not already inclusive of the current block height. It's important to ensure that cur_cltv represents the correct starting point before the addition of excess_final_cltv_expiry_delta.

To address this, verify that cur_cltv is calculated correctly elsewhere in the codebase, taking into account the current block height. If cur_cltv is already inclusive of the current block height, then this change is correct. Otherwise, adjust the calculation to ensure the correct cltv_expiry_height is set.

- cltv_expiry_height: cur_cltv,
+ cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,

Committable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,
cltv_expiry_height: cur_cltv + excess_final_cltv_expiry_delta,

encrypted_tlvs: blinded_hop.encrypted_payload.clone(),
intro_node_blinding_point: blinding_point.take(),
});
Expand Down
1 change: 1 addition & 0 deletions lightning/src/ln/peer_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ impl ChannelMessageHandler for ErroringMessageHandler {
features.set_channel_type_optional();
features.set_scid_privacy_optional();
features.set_zero_conf_optional();
features.set_route_blinding_optional();
features
}

Expand Down