Skip to content

Commit a2a7fef

Browse files
valentinewallacevalentinewallace
committed
Move PendingHTLCStatus construction inside channel lock
We need the channel lock for constructing a pending HTLC's status because we need to know if the channel accepts underpaying HTLCs in upcoming commits.
1 parent ae9e96e commit a2a7fef

File tree

1 file changed

+180
-146
lines changed

1 file changed

+180
-146
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 180 additions & 146 deletions
Original file line numberDiff line numberDiff line change
@@ -2619,12 +2619,14 @@ where
26192619
})
26202620
}
26212621

2622-
fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> PendingHTLCStatus {
2622+
fn decode_update_add_htlc_onion(
2623+
&self, msg: &msgs::UpdateAddHTLC
2624+
) -> Result<(onion_utils::Hop, [u8; 32]), HTLCFailureMsg> {
26232625
macro_rules! return_malformed_err {
26242626
($msg: expr, $err_code: expr) => {
26252627
{
26262628
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2627-
return PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
2629+
return Err(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
26282630
channel_id: msg.channel_id,
26292631
htlc_id: msg.htlc_id,
26302632
sha256_of_onion: Sha256::hash(&msg.onion_routing_packet.hop_data).into_inner(),
@@ -2655,7 +2657,7 @@ where
26552657
($msg: expr, $err_code: expr, $data: expr) => {
26562658
{
26572659
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2658-
return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
2660+
return Err(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
26592661
channel_id: msg.channel_id,
26602662
htlc_id: msg.htlc_id,
26612663
reason: HTLCFailReason::reason($err_code, $data.to_vec())
@@ -2674,8 +2676,176 @@ where
26742676
return_err!(err_msg, err_code, &[0; 0]);
26752677
},
26762678
};
2679+
let (outgoing_scid, outgoing_amt_msat, outgoing_cltv_value) = match next_hop {
2680+
onion_utils::Hop::Forward {
2681+
next_hop_data: msgs::OnionHopData {
2682+
format: msgs::OnionHopDataFormat::NonFinalNode { short_channel_id }, amt_to_forward,
2683+
outgoing_cltv_value,
2684+
}, ..
2685+
} => (short_channel_id, amt_to_forward, outgoing_cltv_value),
2686+
// We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the
2687+
// inbound channel's state.
2688+
onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret)),
2689+
onion_utils::Hop::Forward {
2690+
next_hop_data: msgs::OnionHopData { format: msgs::OnionHopDataFormat::FinalNode { .. }, .. }, ..
2691+
} => {
2692+
return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]);
2693+
}
2694+
};
26772695

2678-
let pending_forward_info = match next_hop {
2696+
// Perform outbound checks here instead of in [`Self::construct_pending_htlc_info`] because we
2697+
// can't hold the outbound peer state lock at the same time as the inbound peer state lock.
2698+
if let Some((err, mut code, chan_update)) = loop {
2699+
let id_option = self.short_to_chan_info.read().unwrap().get(&outgoing_scid).cloned();
2700+
let forwarding_chan_info_opt = match id_option {
2701+
None => { // unknown_next_peer
2702+
// Note that this is likely a timing oracle for detecting whether an scid is a
2703+
// phantom or an intercept.
2704+
if (self.default_configuration.accept_intercept_htlcs &&
2705+
fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, outgoing_scid, &self.genesis_hash)) ||
2706+
fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, outgoing_scid, &self.genesis_hash)
2707+
{
2708+
None
2709+
} else {
2710+
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2711+
}
2712+
},
2713+
Some((cp_id, id)) => Some((cp_id.clone(), id.clone())),
2714+
};
2715+
let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
2716+
let per_peer_state = self.per_peer_state.read().unwrap();
2717+
let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
2718+
if peer_state_mutex_opt.is_none() {
2719+
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2720+
}
2721+
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
2722+
let peer_state = &mut *peer_state_lock;
2723+
let chan = match peer_state.channel_by_id.get_mut(&forwarding_id) {
2724+
None => {
2725+
// Channel was removed. The short_to_chan_info and channel_by_id maps
2726+
// have no consistency guarantees.
2727+
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2728+
},
2729+
Some(chan) => chan
2730+
};
2731+
if !chan.context.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
2732+
// Note that the behavior here should be identical to the above block - we
2733+
// should NOT reveal the existence or non-existence of a private channel if
2734+
// we don't allow forwards outbound over them.
2735+
break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None));
2736+
}
2737+
if chan.context.get_channel_type().supports_scid_privacy() && outgoing_scid != chan.context.outbound_scid_alias() {
2738+
// `option_scid_alias` (referred to in LDK as `scid_privacy`) means
2739+
// "refuse to forward unless the SCID alias was used", so we pretend
2740+
// we don't have the channel here.
2741+
break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None));
2742+
}
2743+
let chan_update_opt = self.get_channel_update_for_onion(outgoing_scid, chan).ok();
2744+
2745+
// Note that we could technically not return an error yet here and just hope
2746+
// that the connection is reestablished or monitor updated by the time we get
2747+
// around to doing the actual forward, but better to fail early if we can and
2748+
// hopefully an attacker trying to path-trace payments cannot make this occur
2749+
// on a small/per-node/per-channel scale.
2750+
if !chan.context.is_live() { // channel_disabled
2751+
// If the channel_update we're going to return is disabled (i.e. the
2752+
// peer has been disabled for some time), return `channel_disabled`,
2753+
// otherwise return `temporary_channel_failure`.
2754+
if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
2755+
break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
2756+
} else {
2757+
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
2758+
}
2759+
}
2760+
if outgoing_amt_msat < chan.context.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
2761+
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
2762+
}
2763+
if let Err((err, code)) = chan.htlc_satisfies_config(&msg, outgoing_amt_msat, outgoing_cltv_value) {
2764+
break Some((err, code, chan_update_opt));
2765+
}
2766+
chan_update_opt
2767+
} else {
2768+
if (msg.cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
2769+
// We really should set `incorrect_cltv_expiry` here but as we're not
2770+
// forwarding over a real channel we can't generate a channel_update
2771+
// for it. Instead we just return a generic temporary_node_failure.
2772+
break Some((
2773+
"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
2774+
0x2000 | 2, None,
2775+
));
2776+
}
2777+
None
2778+
};
2779+
2780+
let cur_height = self.best_block.read().unwrap().height() + 1;
2781+
// Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
2782+
// but we want to be robust wrt to counterparty packet sanitization (see
2783+
// HTLC_FAIL_BACK_BUFFER rationale).
2784+
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
2785+
break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt));
2786+
}
2787+
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
2788+
break Some(("CLTV expiry is too far in the future", 21, None));
2789+
}
2790+
// If the HTLC expires ~now, don't bother trying to forward it to our
2791+
// counterparty. They should fail it anyway, but we don't want to bother with
2792+
// the round-trips or risk them deciding they definitely want the HTLC and
2793+
// force-closing to ensure they get it if we're offline.
2794+
// We previously had a much more aggressive check here which tried to ensure
2795+
// our counterparty receives an HTLC which has *our* risk threshold met on it,
2796+
// but there is no need to do that, and since we're a bit conservative with our
2797+
// risk threshold it just results in failing to forward payments.
2798+
if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
2799+
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt));
2800+
}
2801+
2802+
break None;
2803+
}
2804+
{
2805+
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2));
2806+
if let Some(chan_update) = chan_update {
2807+
if code == 0x1000 | 11 || code == 0x1000 | 12 {
2808+
msg.amount_msat.write(&mut res).expect("Writes cannot fail");
2809+
}
2810+
else if code == 0x1000 | 13 {
2811+
msg.cltv_expiry.write(&mut res).expect("Writes cannot fail");
2812+
}
2813+
else if code == 0x1000 | 20 {
2814+
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791
2815+
0u16.write(&mut res).expect("Writes cannot fail");
2816+
}
2817+
(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
2818+
msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
2819+
chan_update.write(&mut res).expect("Writes cannot fail");
2820+
} else if code & 0x1000 == 0x1000 {
2821+
// If we're trying to return an error that requires a `channel_update` but
2822+
// we're forwarding to a phantom or intercept "channel" (i.e. cannot
2823+
// generate an update), just use the generic "temporary_node_failure"
2824+
// instead.
2825+
code = 0x2000 | 2;
2826+
}
2827+
return_err!(err, code, &res.0[..]);
2828+
}
2829+
Ok((next_hop, shared_secret))
2830+
}
2831+
2832+
fn construct_pending_htlc_status<'a>(
2833+
&self, msg: &msgs::UpdateAddHTLC, shared_secret: [u8; 32], decoded_hop: onion_utils::Hop,
2834+
) -> PendingHTLCStatus {
2835+
macro_rules! return_err {
2836+
($msg: expr, $err_code: expr, $data: expr) => {
2837+
{
2838+
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2839+
return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
2840+
channel_id: msg.channel_id,
2841+
htlc_id: msg.htlc_id,
2842+
reason: HTLCFailReason::reason($err_code, $data.to_vec())
2843+
.get_encrypted_failure_packet(&shared_secret, &None),
2844+
}));
2845+
}
2846+
}
2847+
}
2848+
match decoded_hop {
26792849
onion_utils::Hop::Receive(next_hop_data) => {
26802850
// OUR PAYMENT!
26812851
match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry, None) {
@@ -2717,148 +2887,7 @@ where
27172887
outgoing_cltv_value: next_hop_data.outgoing_cltv_value,
27182888
})
27192889
}
2720-
};
2721-
2722-
if let &PendingHTLCStatus::Forward(PendingHTLCInfo { ref routing, ref outgoing_amt_msat, ref outgoing_cltv_value, .. }) = &pending_forward_info {
2723-
// If short_channel_id is 0 here, we'll reject the HTLC as there cannot be a channel
2724-
// with a short_channel_id of 0. This is important as various things later assume
2725-
// short_channel_id is non-0 in any ::Forward.
2726-
if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
2727-
if let Some((err, mut code, chan_update)) = loop {
2728-
let id_option = self.short_to_chan_info.read().unwrap().get(short_channel_id).cloned();
2729-
let forwarding_chan_info_opt = match id_option {
2730-
None => { // unknown_next_peer
2731-
// Note that this is likely a timing oracle for detecting whether an scid is a
2732-
// phantom or an intercept.
2733-
if (self.default_configuration.accept_intercept_htlcs &&
2734-
fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)) ||
2735-
fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)
2736-
{
2737-
None
2738-
} else {
2739-
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2740-
}
2741-
},
2742-
Some((cp_id, id)) => Some((cp_id.clone(), id.clone())),
2743-
};
2744-
let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
2745-
let per_peer_state = self.per_peer_state.read().unwrap();
2746-
let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
2747-
if peer_state_mutex_opt.is_none() {
2748-
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2749-
}
2750-
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
2751-
let peer_state = &mut *peer_state_lock;
2752-
let chan = match peer_state.channel_by_id.get_mut(&forwarding_id) {
2753-
None => {
2754-
// Channel was removed. The short_to_chan_info and channel_by_id maps
2755-
// have no consistency guarantees.
2756-
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2757-
},
2758-
Some(chan) => chan
2759-
};
2760-
if !chan.context.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
2761-
// Note that the behavior here should be identical to the above block - we
2762-
// should NOT reveal the existence or non-existence of a private channel if
2763-
// we don't allow forwards outbound over them.
2764-
break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None));
2765-
}
2766-
if chan.context.get_channel_type().supports_scid_privacy() && *short_channel_id != chan.context.outbound_scid_alias() {
2767-
// `option_scid_alias` (referred to in LDK as `scid_privacy`) means
2768-
// "refuse to forward unless the SCID alias was used", so we pretend
2769-
// we don't have the channel here.
2770-
break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None));
2771-
}
2772-
let chan_update_opt = self.get_channel_update_for_onion(*short_channel_id, chan).ok();
2773-
2774-
// Note that we could technically not return an error yet here and just hope
2775-
// that the connection is reestablished or monitor updated by the time we get
2776-
// around to doing the actual forward, but better to fail early if we can and
2777-
// hopefully an attacker trying to path-trace payments cannot make this occur
2778-
// on a small/per-node/per-channel scale.
2779-
if !chan.context.is_live() { // channel_disabled
2780-
// If the channel_update we're going to return is disabled (i.e. the
2781-
// peer has been disabled for some time), return `channel_disabled`,
2782-
// otherwise return `temporary_channel_failure`.
2783-
if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
2784-
break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
2785-
} else {
2786-
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
2787-
}
2788-
}
2789-
if *outgoing_amt_msat < chan.context.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
2790-
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
2791-
}
2792-
if let Err((err, code)) = chan.htlc_satisfies_config(&msg, *outgoing_amt_msat, *outgoing_cltv_value) {
2793-
break Some((err, code, chan_update_opt));
2794-
}
2795-
chan_update_opt
2796-
} else {
2797-
if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
2798-
// We really should set `incorrect_cltv_expiry` here but as we're not
2799-
// forwarding over a real channel we can't generate a channel_update
2800-
// for it. Instead we just return a generic temporary_node_failure.
2801-
break Some((
2802-
"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
2803-
0x2000 | 2, None,
2804-
));
2805-
}
2806-
None
2807-
};
2808-
2809-
let cur_height = self.best_block.read().unwrap().height() + 1;
2810-
// Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
2811-
// but we want to be robust wrt to counterparty packet sanitization (see
2812-
// HTLC_FAIL_BACK_BUFFER rationale).
2813-
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
2814-
break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt));
2815-
}
2816-
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
2817-
break Some(("CLTV expiry is too far in the future", 21, None));
2818-
}
2819-
// If the HTLC expires ~now, don't bother trying to forward it to our
2820-
// counterparty. They should fail it anyway, but we don't want to bother with
2821-
// the round-trips or risk them deciding they definitely want the HTLC and
2822-
// force-closing to ensure they get it if we're offline.
2823-
// We previously had a much more aggressive check here which tried to ensure
2824-
// our counterparty receives an HTLC which has *our* risk threshold met on it,
2825-
// but there is no need to do that, and since we're a bit conservative with our
2826-
// risk threshold it just results in failing to forward payments.
2827-
if (*outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
2828-
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt));
2829-
}
2830-
2831-
break None;
2832-
}
2833-
{
2834-
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2));
2835-
if let Some(chan_update) = chan_update {
2836-
if code == 0x1000 | 11 || code == 0x1000 | 12 {
2837-
msg.amount_msat.write(&mut res).expect("Writes cannot fail");
2838-
}
2839-
else if code == 0x1000 | 13 {
2840-
msg.cltv_expiry.write(&mut res).expect("Writes cannot fail");
2841-
}
2842-
else if code == 0x1000 | 20 {
2843-
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791
2844-
0u16.write(&mut res).expect("Writes cannot fail");
2845-
}
2846-
(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
2847-
msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
2848-
chan_update.write(&mut res).expect("Writes cannot fail");
2849-
} else if code & 0x1000 == 0x1000 {
2850-
// If we're trying to return an error that requires a `channel_update` but
2851-
// we're forwarding to a phantom or intercept "channel" (i.e. cannot
2852-
// generate an update), just use the generic "temporary_node_failure"
2853-
// instead.
2854-
code = 0x2000 | 2;
2855-
}
2856-
return_err!(err, code, &res.0[..]);
2857-
}
2858-
}
28592890
}
2860-
2861-
pending_forward_info
28622891
}
28632892

28642893
/// Gets the current [`channel_update`] for the given channel. This first checks if the channel is
@@ -5358,7 +5387,7 @@ where
53585387
//encrypted with the same key. It's not immediately obvious how to usefully exploit that,
53595388
//but we should prevent it anyway.
53605389

5361-
let pending_forward_info = self.decode_update_add_htlc_onion(msg);
5390+
let decoded_hop_res = self.decode_update_add_htlc_onion(msg);
53625391
let per_peer_state = self.per_peer_state.read().unwrap();
53635392
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
53645393
.ok_or_else(|| {
@@ -5370,6 +5399,11 @@ where
53705399
match peer_state.channel_by_id.entry(msg.channel_id) {
53715400
hash_map::Entry::Occupied(mut chan) => {
53725401

5402+
let pending_forward_info = match decoded_hop_res {
5403+
Ok((next_hop, shared_secret)) =>
5404+
self.construct_pending_htlc_status(msg, shared_secret, next_hop),
5405+
Err(e) => PendingHTLCStatus::Fail(e)
5406+
};
53735407
let create_pending_htlc_status = |chan: &Channel<<SP::Target as SignerProvider>::Signer>, pending_forward_info: PendingHTLCStatus, error_code: u16| {
53745408
// If the update_add is completely bogus, the call will Err and we will close,
53755409
// but if we've sent a shutdown and they haven't acknowledged it yet, we just

0 commit comments

Comments
 (0)