Skip to content

Stop sending channel_update in onion failures #3345

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Stop sending channel_update in onion failures #3345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2142,6 +2142,14 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
self.is_usable() && !self.channel_state.is_peer_disconnected()
}

/// Returns false if our last broadcasted channel_update message has the "channel disabled" bit set
pub fn is_enabled(&self) -> bool {
self.is_usable() && match self.channel_update_status {
ChannelUpdateStatus::Enabled | ChannelUpdateStatus::DisabledStaged(_) => true,
ChannelUpdateStatus::Disabled | ChannelUpdateStatus::EnabledStaged(_) => false,
}
}

// Public utilities:

pub fn channel_id(&self) -> ChannelId {
Expand Down
157 changes: 52 additions & 105 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError};
#[cfg(test)]
use crate::ln::outbound_payment;
use crate::ln::outbound_payment::{OutboundPayments, PendingOutboundPayment, RetryableInvoiceRequest, SendAlongPathArgs, StaleExpiration};
use crate::ln::wire::Encode;
use crate::offers::invoice::{Bolt12Invoice, DEFAULT_RELATIVE_EXPIRY, DerivedSigningPubkey, ExplicitSigningPubkey, InvoiceBuilder, UnsignedBolt12Invoice};
use crate::offers::invoice_error::InvoiceError;
use crate::offers::invoice_request::{DerivedPayerSigningPubkey, InvoiceRequest, InvoiceRequestBuilder};
Expand Down Expand Up @@ -677,7 +676,7 @@ pub enum FailureCode {
}

impl Into<u16> for FailureCode {
fn into(self) -> u16 {
fn into(self) -> u16 {
match self {
FailureCode::TemporaryNodeFailure => 0x2000 | 2,
FailureCode::RequiredNodeFeatureMissing => 0x4000 | 0x2000 | 3,
Expand Down Expand Up @@ -3841,43 +3840,39 @@ where

fn can_forward_htlc_to_outgoing_channel(
&self, chan: &mut Channel<SP>, msg: &msgs::UpdateAddHTLC, next_packet: &NextPacketDetails
) -> Result<(), (&'static str, u16, Option<msgs::ChannelUpdate>)> {
) -> Result<(), (&'static str, u16)> {
if !chan.context.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
// Note that the behavior here should be identical to the above block - we
// should NOT reveal the existence or non-existence of a private channel if
// we don't allow forwards outbound over them.
return Err(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None));
return Err(("Refusing to forward to a private channel based on our config.", 0x4000 | 10));
}
if chan.context.get_channel_type().supports_scid_privacy() && next_packet.outgoing_scid != chan.context.outbound_scid_alias() {
// `option_scid_alias` (referred to in LDK as `scid_privacy`) means
// "refuse to forward unless the SCID alias was used", so we pretend
// we don't have the channel here.
return Err(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None));
return Err(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10));
}

// Note that we could technically not return an error yet here and just hope
// that the connection is reestablished or monitor updated by the time we get
// around to doing the actual forward, but better to fail early if we can and
// hopefully an attacker trying to path-trace payments cannot make this occur
// on a small/per-node/per-channel scale.
if !chan.context.is_live() { // channel_disabled
// If the channel_update we're going to return is disabled (i.e. the
// peer has been disabled for some time), return `channel_disabled`,
// otherwise return `temporary_channel_failure`.
let chan_update_opt = self.get_channel_update_for_onion(next_packet.outgoing_scid, chan).ok();
if chan_update_opt.as_ref().map(|u| u.contents.channel_flags & 2 == 2).unwrap_or(false) {
return Err(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
if !chan.context.is_live() {
if !chan.context.is_enabled() {
// channel_disabled
return Err(("Forwarding channel has been disconnected for some time.", 0x1000 | 20));
} else {
return Err(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
// temporary_channel_failure
return Err(("Forwarding channel is not in a ready state.", 0x1000 | 7));
}
}
if next_packet.outgoing_amt_msat < chan.context.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
let chan_update_opt = self.get_channel_update_for_onion(next_packet.outgoing_scid, chan).ok();
return Err(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
return Err(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11));
}
if let Err((err, code)) = chan.htlc_satisfies_config(msg, next_packet.outgoing_amt_msat, next_packet.outgoing_cltv_value) {
let chan_update_opt = self.get_channel_update_for_onion(next_packet.outgoing_scid, chan).ok();
return Err((err, code, chan_update_opt));
return Err((err, code));
}

Ok(())
Expand Down Expand Up @@ -3909,7 +3904,7 @@ where

fn can_forward_htlc(
&self, msg: &msgs::UpdateAddHTLC, next_packet_details: &NextPacketDetails
) -> Result<(), (&'static str, u16, Option<msgs::ChannelUpdate>)> {
) -> Result<(), (&'static str, u16)> {
match self.do_funded_channel_callback(next_packet_details.outgoing_scid, |chan: &mut Channel<SP>| {
self.can_forward_htlc_to_outgoing_channel(chan, msg, next_packet_details)
}) {
Expand All @@ -3922,7 +3917,7 @@ where
fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, next_packet_details.outgoing_scid, &self.chain_hash)) ||
fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, next_packet_details.outgoing_scid, &self.chain_hash)
{} else {
return Err(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
return Err(("Don't have available channel for forwarding as requested.", 0x4000 | 10));
}
}
}
Expand All @@ -3931,23 +3926,20 @@ where
if let Err((err_msg, err_code)) = check_incoming_htlc_cltv(
cur_height, next_packet_details.outgoing_cltv_value, msg.cltv_expiry
) {
let chan_update_opt = self.do_funded_channel_callback(next_packet_details.outgoing_scid, |chan: &mut Channel<SP>| {
self.get_channel_update_for_onion(next_packet_details.outgoing_scid, chan).ok()
}).flatten();
return Err((err_msg, err_code, chan_update_opt));
return Err((err_msg, err_code));
}

Ok(())
}

fn htlc_failure_from_update_add_err(
&self, msg: &msgs::UpdateAddHTLC, counterparty_node_id: &PublicKey, err_msg: &'static str,
mut err_code: u16, chan_update: Option<msgs::ChannelUpdate>, is_intro_node_blinded_forward: bool,
err_code: u16, is_intro_node_blinded_forward: bool,
shared_secret: &[u8; 32]
) -> HTLCFailureMsg {
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2));
if chan_update.is_some() && err_code & 0x1000 == 0x1000 {
let chan_update = chan_update.unwrap();
// at capacity, we write fields `htlc_msat` and `len`
let mut res = VecWriter(Vec::with_capacity(8 + 2));
if err_code & 0x1000 == 0x1000 {
if err_code == 0x1000 | 11 || err_code == 0x1000 | 12 {
msg.amount_msat.write(&mut res).expect("Writes cannot fail");
}
Expand All @@ -3958,15 +3950,8 @@ where
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791
0u16.write(&mut res).expect("Writes cannot fail");
}
(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
chan_update.write(&mut res).expect("Writes cannot fail");
} else if err_code & 0x1000 == 0x1000 {
// If we're trying to return an error that requires a `channel_update` but
// we're forwarding to a phantom or intercept "channel" (i.e. cannot
// generate an update), just use the generic "temporary_node_failure"
// instead.
err_code = 0x2000 | 2;
// See https://github.com/lightning/bolts/blob/247e83d/04-onion-routing.md?plain=1#L1414-L1415
(0u16).write(&mut res).expect("Writes cannot fail");
}

log_info!(
Expand Down Expand Up @@ -4014,9 +3999,9 @@ where
// Perform outbound checks here instead of in [`Self::construct_pending_htlc_info`] because we
// can't hold the outbound peer state lock at the same time as the inbound peer state lock.
self.can_forward_htlc(&msg, &next_packet_details).map_err(|e| {
let (err_msg, err_code, chan_update_opt) = e;
let (err_msg, err_code) = e;
self.htlc_failure_from_update_add_err(
msg, counterparty_node_id, err_msg, err_code, chan_update_opt,
msg, counterparty_node_id, err_msg, err_code,
next_hop.is_intro_node_blinded_forward(), &shared_secret
)
})?;
Expand Down Expand Up @@ -4125,20 +4110,10 @@ where
Some(id) => id,
};

self.get_channel_update_for_onion(short_channel_id, chan)
}

fn get_channel_update_for_onion(&self, short_channel_id: u64, chan: &Channel<SP>) -> Result<msgs::ChannelUpdate, LightningError> {
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
log_trace!(logger, "Generating channel update for channel {}", chan.context.channel_id());
let were_node_one = self.our_network_pubkey.serialize()[..] < chan.context.get_counterparty_node_id().serialize()[..];

let enabled = chan.context.is_usable() && match chan.channel_update_status() {
ChannelUpdateStatus::Enabled => true,
ChannelUpdateStatus::DisabledStaged(_) => true,
ChannelUpdateStatus::Disabled => false,
ChannelUpdateStatus::EnabledStaged(_) => false,
};
let enabled = chan.context.is_enabled();

let unsigned = msgs::UnsignedChannelUpdate {
chain_hash: self.chain_hash,
Expand Down Expand Up @@ -5326,16 +5301,9 @@ where
}) {
Some(Ok(_)) => {},
Some(Err((err, code))) => {
let outgoing_chan_update_opt = if let Some(outgoing_scid) = outgoing_scid_opt.as_ref() {
self.do_funded_channel_callback(*outgoing_scid, |chan: &mut Channel<SP>| {
self.get_channel_update_for_onion(*outgoing_scid, chan).ok()
}).flatten()
} else {
None
};
let htlc_fail = self.htlc_failure_from_update_add_err(
&update_add_htlc, &incoming_counterparty_node_id, err, code,
outgoing_chan_update_opt, is_intro_node_blinded_forward, &shared_secret,
is_intro_node_blinded_forward, &shared_secret,
);
let htlc_destination = get_failed_htlc_destination(outgoing_scid_opt, update_add_htlc.payment_hash);
htlc_fails.push((htlc_fail, htlc_destination));
Expand All @@ -5347,12 +5315,12 @@ where

// Now process the HTLC on the outgoing channel if it's a forward.
if let Some(next_packet_details) = next_packet_details_opt.as_ref() {
if let Err((err, code, chan_update_opt)) = self.can_forward_htlc(
if let Err((err, code)) = self.can_forward_htlc(
&update_add_htlc, next_packet_details
) {
let htlc_fail = self.htlc_failure_from_update_add_err(
&update_add_htlc, &incoming_counterparty_node_id, err, code,
chan_update_opt, is_intro_node_blinded_forward, &shared_secret,
is_intro_node_blinded_forward, &shared_secret,
);
let htlc_destination = get_failed_htlc_destination(outgoing_scid_opt, update_add_htlc.payment_hash);
htlc_fails.push((htlc_fail, htlc_destination));
Expand Down Expand Up @@ -5633,7 +5601,8 @@ where
}

if let Some(ChannelPhase::Funded(ref mut chan)) = peer_state.channel_by_id.get_mut(&forward_chan_id) {
let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|7, short_chan_id, chan);
let failure_code = 0x1000|7;
let data = self.get_htlc_inbound_temp_fail_data(failure_code);
Comment on lines +5604 to +5605
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO there was no reason to change the function return type, is it?

Copy link
Contributor Author

@tankyleo tankyleo Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In case this function failed to fetch the channel_update, it previously indicated this to the caller by returning an 0x4000 error, and the caller would reassign the 0x1000 code with the 0x4000 code.

In case the channel_update fetch was successful, the returned error code was the same as the one passed in.

This function no longer has this potential fetch failure, so it will never reassign the error code value to a different value. So we don't need to return an error code value any longer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I noted it, and make sense of your explaination. Probably just bias because the previous function was more "elegant" from the code point of view. Thanks for explaination

failed_forwards.push((htlc_source, payment_hash,
HTLCFailReason::reason(failure_code, data),
HTLCDestination::NextHopChannel { node_id: Some(chan.context.get_counterparty_node_id()), channel_id: forward_chan_id }
Expand Down Expand Up @@ -6448,46 +6417,21 @@ where
///
/// This is for failures on the channel on which the HTLC was *received*, not failures
/// forwarding
fn get_htlc_inbound_temp_fail_err_and_data(&self, desired_err_code: u16, chan: &Channel<SP>) -> (u16, Vec<u8>) {
// We can't be sure what SCID was used when relaying inbound towards us, so we have to
// guess somewhat. If its a public channel, we figure best to just use the real SCID (as
// we're not leaking that we have a channel with the counterparty), otherwise we try to use
// an inbound SCID alias before the real SCID.
let scid_pref = if chan.context.should_announce() {
chan.context.get_short_channel_id().or(chan.context.latest_inbound_scid_alias())
} else {
chan.context.latest_inbound_scid_alias().or(chan.context.get_short_channel_id())
};
if let Some(scid) = scid_pref {
self.get_htlc_temp_fail_err_and_data(desired_err_code, scid, chan)
} else {
(0x4000|10, Vec::new())
}
}


/// Gets an HTLC onion failure code and error data for an `UPDATE` error, given the error code
/// that we want to return and a channel.
fn get_htlc_temp_fail_err_and_data(&self, desired_err_code: u16, scid: u64, chan: &Channel<SP>) -> (u16, Vec<u8>) {
debug_assert_eq!(desired_err_code & 0x1000, 0x1000);
if let Ok(upd) = self.get_channel_update_for_onion(scid, chan) {
let mut enc = VecWriter(Vec::with_capacity(upd.serialized_length() + 6));
if desired_err_code == 0x1000 | 20 {
// No flags for `disabled_flags` are currently defined so they're always two zero bytes.
// See https://github.com/lightning/bolts/blob/341ec84/04-onion-routing.md?plain=1#L1008
0u16.write(&mut enc).expect("Writes cannot fail");
}
(upd.serialized_length() as u16 + 2).write(&mut enc).expect("Writes cannot fail");
msgs::ChannelUpdate::TYPE.write(&mut enc).expect("Writes cannot fail");
upd.write(&mut enc).expect("Writes cannot fail");
(desired_err_code, enc.0)
} else {
// If we fail to get a unicast channel_update, it implies we don't yet have an SCID,
// which means we really shouldn't have gotten a payment to be forwarded over this
// channel yet, or if we did it's from a route hint. Either way, returning an error of
// PERM|no_such_channel should be fine.
(0x4000|10, Vec::new())
}
fn get_htlc_inbound_temp_fail_data(&self, err_code: u16) -> Vec<u8> {
debug_assert_eq!(err_code & 0x1000, 0x1000);
debug_assert_ne!(err_code, 0x1000|11);
debug_assert_ne!(err_code, 0x1000|12);
debug_assert_ne!(err_code, 0x1000|13);
// at capacity, we write fields `disabled_flags` and `len`
let mut enc = VecWriter(Vec::with_capacity(4));
if err_code == 0x1000 | 20 {
// No flags for `disabled_flags` are currently defined so they're always two zero bytes.
// See https://github.com/lightning/bolts/blob/341ec84/04-onion-routing.md?plain=1#L1008
0u16.write(&mut enc).expect("Writes cannot fail");
}
// See https://github.com/lightning/bolts/blob/247e83d/04-onion-routing.md?plain=1#L1414-L1415
(0u16).write(&mut enc).expect("Writes cannot fail");
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it seems kind of obvious for no channel update, but could we perhaps link to https://github.com/lightning/bolts/blob/247e83d528a2a380e533e89f31918d7b0ce6a0c1/04-onion-routing.md?plain=1#L1414-L1415 in a comment?

Sorry for only seeing this now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem you are right, I think it's worth adding this reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see below, I added the link in the two places where it is relevant.

I only included the first 7 characters of the commit hash, partly to keep it consistent with the link further above in the location you pointed out.

enc.0
}

// Fail a list of HTLCs that were just freed from the holding cell. The HTLCs need to be
Expand All @@ -6504,8 +6448,10 @@ where
let peer_state = &mut *peer_state_lock;
match peer_state.channel_by_id.entry(channel_id) {
hash_map::Entry::Occupied(chan_phase_entry) => {
if let ChannelPhase::Funded(chan) = chan_phase_entry.get() {
self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan)
if let ChannelPhase::Funded(_chan) = chan_phase_entry.get() {
let failure_code = 0x1000|7;
let data = self.get_htlc_inbound_temp_fail_data(failure_code);
Comment on lines +6452 to +6453
Copy link
Contributor

Choose a reason for hiding this comment

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

👁️

(failure_code, data)
} else {
// We shouldn't be trying to fail holding cell HTLCs on an unfunded channel.
debug_assert!(false);
Expand Down Expand Up @@ -8164,8 +8110,8 @@ where
let reason = if routing.blinded_failure().is_some() {
HTLCFailReason::reason(INVALID_ONION_BLINDING, vec![0; 32])
} else if (error_code & 0x1000) != 0 {
let (real_code, error_data) = self.get_htlc_inbound_temp_fail_err_and_data(error_code, chan);
HTLCFailReason::reason(real_code, error_data)
let error_data = self.get_htlc_inbound_temp_fail_data(error_code);
HTLCFailReason::reason(error_code, error_data)
} else {
HTLCFailReason::from_failure_code(error_code)
}.get_encrypted_failure_packet(incoming_shared_secret, &None);
Expand Down Expand Up @@ -10190,7 +10136,8 @@ where
let res = f(channel);
if let Ok((channel_ready_opt, mut timed_out_pending_htlcs, announcement_sigs)) = res {
for (source, payment_hash) in timed_out_pending_htlcs.drain(..) {
let (failure_code, data) = self.get_htlc_inbound_temp_fail_err_and_data(0x1000|14 /* expiry_too_soon */, &channel);
let failure_code = 0x1000|14; /* expiry_too_soon */
let data = self.get_htlc_inbound_temp_fail_data(failure_code);
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(failure_code, data),
HTLCDestination::NextHopChannel { node_id: Some(channel.context.get_counterparty_node_id()), channel_id: channel.context.channel_id() }));
}
Expand Down
14 changes: 7 additions & 7 deletions lightning/src/ln/onion_route_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,9 +1397,12 @@ fn test_phantom_failure_modified_cltv() {
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);

// Ensure the payment fails with the expected error.
let mut err_data = Vec::new();
err_data.extend_from_slice(&update_add.cltv_expiry.to_be_bytes());
err_data.extend_from_slice(&0u16.to_be_bytes());
let mut fail_conditions = PaymentFailedConditions::new()
.blamed_scid(phantom_scid)
.expected_htlc_error_data(0x2000 | 2, &[]);
.expected_htlc_error_data(0x1000 | 13, &err_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
}

Expand Down Expand Up @@ -1437,9 +1440,10 @@ fn test_phantom_failure_expires_too_soon() {
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);

// Ensure the payment fails with the expected error.
let err_data = 0u16.to_be_bytes();
let mut fail_conditions = PaymentFailedConditions::new()
.blamed_scid(phantom_scid)
.expected_htlc_error_data(0x2000 | 2, &[]);
.expected_htlc_error_data(0x1000 | 14, &err_data);
expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
}

Expand Down Expand Up @@ -1535,11 +1539,7 @@ fn do_test_phantom_dust_exposure_failure(multiplier_dust_limit: bool) {
commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);

// Ensure the payment fails with the expected error.
let mut err_data = Vec::new();
err_data.extend_from_slice(&(channel.1.serialized_length() as u16 + 2).to_be_bytes());
err_data.extend_from_slice(&ChannelUpdate::TYPE.to_be_bytes());
err_data.extend_from_slice(&channel.1.encode());

let err_data = 0u16.to_be_bytes();
let mut fail_conditions = PaymentFailedConditions::new()
.blamed_scid(route.paths[0].hops.last().as_ref().unwrap().short_channel_id)
.blamed_chan_closed(false)
Expand Down
Loading
Loading