Skip to content

Commit d09338d

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 2a3254d commit d09338d

File tree

1 file changed

+178
-146
lines changed

1 file changed

+178
-146
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 178 additions & 146 deletions
Original file line numberDiff line numberDiff line change
@@ -2512,12 +2512,14 @@ where
25122512
})
25132513
}
25142514

2515-
fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> PendingHTLCStatus {
2515+
fn decode_update_add_htlc_onion(
2516+
&self, msg: &msgs::UpdateAddHTLC
2517+
) -> Result<(onion_utils::Hop, [u8; 32]), HTLCFailureMsg> {
25162518
macro_rules! return_malformed_err {
25172519
($msg: expr, $err_code: expr) => {
25182520
{
25192521
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2520-
return PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
2522+
return Err(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
25212523
channel_id: msg.channel_id,
25222524
htlc_id: msg.htlc_id,
25232525
sha256_of_onion: Sha256::hash(&msg.onion_routing_packet.hop_data).into_inner(),
@@ -2548,7 +2550,7 @@ where
25482550
($msg: expr, $err_code: expr, $data: expr) => {
25492551
{
25502552
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2551-
return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
2553+
return Err(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
25522554
channel_id: msg.channel_id,
25532555
htlc_id: msg.htlc_id,
25542556
reason: HTLCFailReason::reason($err_code, $data.to_vec())
@@ -2567,8 +2569,174 @@ where
25672569
return_err!(err_msg, err_code, &[0; 0]);
25682570
},
25692571
};
2572+
let (outgoing_scid, outgoing_amt_msat, outgoing_cltv_value) = match next_hop {
2573+
onion_utils::Hop::Forward {
2574+
next_hop_data: msgs::OnionHopData {
2575+
format: msgs::OnionHopDataFormat::NonFinalNode { short_channel_id }, amt_to_forward,
2576+
outgoing_cltv_value,
2577+
}, ..
2578+
} => (short_channel_id, amt_to_forward, outgoing_cltv_value),
2579+
// We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the
2580+
// inbound channel's state.
2581+
onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret)),
2582+
_ => {
2583+
return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]);
2584+
}
2585+
};
25702586

2571-
let pending_forward_info = match next_hop {
2587+
// Perform outbound checks here instead of in [`Self::construct_pending_htlc_info`] because we
2588+
// can't hold the outbound peer state lock at the same time as the inbound peer state lock.
2589+
if let Some((err, mut code, chan_update)) = loop {
2590+
let id_option = self.short_to_chan_info.read().unwrap().get(&outgoing_scid).cloned();
2591+
let forwarding_chan_info_opt = match id_option {
2592+
None => { // unknown_next_peer
2593+
// Note that this is likely a timing oracle for detecting whether an scid is a
2594+
// phantom or an intercept.
2595+
if (self.default_configuration.accept_intercept_htlcs &&
2596+
fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, outgoing_scid, &self.genesis_hash)) ||
2597+
fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, outgoing_scid, &self.genesis_hash)
2598+
{
2599+
None
2600+
} else {
2601+
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2602+
}
2603+
},
2604+
Some((cp_id, id)) => Some((cp_id.clone(), id.clone())),
2605+
};
2606+
let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
2607+
let per_peer_state = self.per_peer_state.read().unwrap();
2608+
let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
2609+
if peer_state_mutex_opt.is_none() {
2610+
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2611+
}
2612+
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
2613+
let peer_state = &mut *peer_state_lock;
2614+
let chan = match peer_state.channel_by_id.get_mut(&forwarding_id) {
2615+
None => {
2616+
// Channel was removed. The short_to_chan_info and channel_by_id maps
2617+
// have no consistency guarantees.
2618+
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2619+
},
2620+
Some(chan) => chan
2621+
};
2622+
if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
2623+
// Note that the behavior here should be identical to the above block - we
2624+
// should NOT reveal the existence or non-existence of a private channel if
2625+
// we don't allow forwards outbound over them.
2626+
break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None));
2627+
}
2628+
if chan.get_channel_type().supports_scid_privacy() && outgoing_scid != chan.outbound_scid_alias() {
2629+
// `option_scid_alias` (referred to in LDK as `scid_privacy`) means
2630+
// "refuse to forward unless the SCID alias was used", so we pretend
2631+
// we don't have the channel here.
2632+
break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None));
2633+
}
2634+
let chan_update_opt = self.get_channel_update_for_onion(outgoing_scid, chan).ok();
2635+
2636+
// Note that we could technically not return an error yet here and just hope
2637+
// that the connection is reestablished or monitor updated by the time we get
2638+
// around to doing the actual forward, but better to fail early if we can and
2639+
// hopefully an attacker trying to path-trace payments cannot make this occur
2640+
// on a small/per-node/per-channel scale.
2641+
if !chan.is_live() { // channel_disabled
2642+
// If the channel_update we're going to return is disabled (i.e. the
2643+
// peer has been disabled for some time), return `channel_disabled`,
2644+
// otherwise return `temporary_channel_failure`.
2645+
if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
2646+
break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
2647+
} else {
2648+
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
2649+
}
2650+
}
2651+
if outgoing_amt_msat < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
2652+
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
2653+
}
2654+
if let Err((err, code)) = chan.htlc_satisfies_config(&msg, outgoing_amt_msat, outgoing_cltv_value) {
2655+
break Some((err, code, chan_update_opt));
2656+
}
2657+
chan_update_opt
2658+
} else {
2659+
if (msg.cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
2660+
// We really should set `incorrect_cltv_expiry` here but as we're not
2661+
// forwarding over a real channel we can't generate a channel_update
2662+
// for it. Instead we just return a generic temporary_node_failure.
2663+
break Some((
2664+
"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
2665+
0x2000 | 2, None,
2666+
));
2667+
}
2668+
None
2669+
};
2670+
2671+
let cur_height = self.best_block.read().unwrap().height() + 1;
2672+
// Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
2673+
// but we want to be robust wrt to counterparty packet sanitization (see
2674+
// HTLC_FAIL_BACK_BUFFER rationale).
2675+
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
2676+
break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt));
2677+
}
2678+
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
2679+
break Some(("CLTV expiry is too far in the future", 21, None));
2680+
}
2681+
// If the HTLC expires ~now, don't bother trying to forward it to our
2682+
// counterparty. They should fail it anyway, but we don't want to bother with
2683+
// the round-trips or risk them deciding they definitely want the HTLC and
2684+
// force-closing to ensure they get it if we're offline.
2685+
// We previously had a much more aggressive check here which tried to ensure
2686+
// our counterparty receives an HTLC which has *our* risk threshold met on it,
2687+
// but there is no need to do that, and since we're a bit conservative with our
2688+
// risk threshold it just results in failing to forward payments.
2689+
if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
2690+
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt));
2691+
}
2692+
2693+
break None;
2694+
}
2695+
{
2696+
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2));
2697+
if let Some(chan_update) = chan_update {
2698+
if code == 0x1000 | 11 || code == 0x1000 | 12 {
2699+
msg.amount_msat.write(&mut res).expect("Writes cannot fail");
2700+
}
2701+
else if code == 0x1000 | 13 {
2702+
msg.cltv_expiry.write(&mut res).expect("Writes cannot fail");
2703+
}
2704+
else if code == 0x1000 | 20 {
2705+
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791
2706+
0u16.write(&mut res).expect("Writes cannot fail");
2707+
}
2708+
(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
2709+
msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
2710+
chan_update.write(&mut res).expect("Writes cannot fail");
2711+
} else if code & 0x1000 == 0x1000 {
2712+
// If we're trying to return an error that requires a `channel_update` but
2713+
// we're forwarding to a phantom or intercept "channel" (i.e. cannot
2714+
// generate an update), just use the generic "temporary_node_failure"
2715+
// instead.
2716+
code = 0x2000 | 2;
2717+
}
2718+
return_err!(err, code, &res.0[..]);
2719+
}
2720+
Ok((next_hop, shared_secret))
2721+
}
2722+
2723+
fn construct_pending_htlc_info<'a>(
2724+
&self, msg: &msgs::UpdateAddHTLC, shared_secret: [u8; 32], decoded_hop: onion_utils::Hop,
2725+
) -> PendingHTLCStatus {
2726+
macro_rules! return_err {
2727+
($msg: expr, $err_code: expr, $data: expr) => {
2728+
{
2729+
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2730+
return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
2731+
channel_id: msg.channel_id,
2732+
htlc_id: msg.htlc_id,
2733+
reason: HTLCFailReason::reason($err_code, $data.to_vec())
2734+
.get_encrypted_failure_packet(&shared_secret, &None),
2735+
}));
2736+
}
2737+
}
2738+
}
2739+
match decoded_hop {
25722740
onion_utils::Hop::Receive(next_hop_data) => {
25732741
// OUR PAYMENT!
25742742
match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry, None) {
@@ -2610,148 +2778,7 @@ where
26102778
outgoing_cltv_value: next_hop_data.outgoing_cltv_value,
26112779
})
26122780
}
2613-
};
2614-
2615-
if let &PendingHTLCStatus::Forward(PendingHTLCInfo { ref routing, ref outgoing_amt_msat, ref outgoing_cltv_value, .. }) = &pending_forward_info {
2616-
// If short_channel_id is 0 here, we'll reject the HTLC as there cannot be a channel
2617-
// with a short_channel_id of 0. This is important as various things later assume
2618-
// short_channel_id is non-0 in any ::Forward.
2619-
if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
2620-
if let Some((err, mut code, chan_update)) = loop {
2621-
let id_option = self.short_to_chan_info.read().unwrap().get(short_channel_id).cloned();
2622-
let forwarding_chan_info_opt = match id_option {
2623-
None => { // unknown_next_peer
2624-
// Note that this is likely a timing oracle for detecting whether an scid is a
2625-
// phantom or an intercept.
2626-
if (self.default_configuration.accept_intercept_htlcs &&
2627-
fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)) ||
2628-
fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)
2629-
{
2630-
None
2631-
} else {
2632-
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2633-
}
2634-
},
2635-
Some((cp_id, id)) => Some((cp_id.clone(), id.clone())),
2636-
};
2637-
let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
2638-
let per_peer_state = self.per_peer_state.read().unwrap();
2639-
let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
2640-
if peer_state_mutex_opt.is_none() {
2641-
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2642-
}
2643-
let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
2644-
let peer_state = &mut *peer_state_lock;
2645-
let chan = match peer_state.channel_by_id.get_mut(&forwarding_id) {
2646-
None => {
2647-
// Channel was removed. The short_to_chan_info and channel_by_id maps
2648-
// have no consistency guarantees.
2649-
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
2650-
},
2651-
Some(chan) => chan
2652-
};
2653-
if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
2654-
// Note that the behavior here should be identical to the above block - we
2655-
// should NOT reveal the existence or non-existence of a private channel if
2656-
// we don't allow forwards outbound over them.
2657-
break Some(("Refusing to forward to a private channel based on our config.", 0x4000 | 10, None));
2658-
}
2659-
if chan.get_channel_type().supports_scid_privacy() && *short_channel_id != chan.outbound_scid_alias() {
2660-
// `option_scid_alias` (referred to in LDK as `scid_privacy`) means
2661-
// "refuse to forward unless the SCID alias was used", so we pretend
2662-
// we don't have the channel here.
2663-
break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None));
2664-
}
2665-
let chan_update_opt = self.get_channel_update_for_onion(*short_channel_id, chan).ok();
2666-
2667-
// Note that we could technically not return an error yet here and just hope
2668-
// that the connection is reestablished or monitor updated by the time we get
2669-
// around to doing the actual forward, but better to fail early if we can and
2670-
// hopefully an attacker trying to path-trace payments cannot make this occur
2671-
// on a small/per-node/per-channel scale.
2672-
if !chan.is_live() { // channel_disabled
2673-
// If the channel_update we're going to return is disabled (i.e. the
2674-
// peer has been disabled for some time), return `channel_disabled`,
2675-
// otherwise return `temporary_channel_failure`.
2676-
if chan_update_opt.as_ref().map(|u| u.contents.flags & 2 == 2).unwrap_or(false) {
2677-
break Some(("Forwarding channel has been disconnected for some time.", 0x1000 | 20, chan_update_opt));
2678-
} else {
2679-
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 7, chan_update_opt));
2680-
}
2681-
}
2682-
if *outgoing_amt_msat < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
2683-
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
2684-
}
2685-
if let Err((err, code)) = chan.htlc_satisfies_config(&msg, *outgoing_amt_msat, *outgoing_cltv_value) {
2686-
break Some((err, code, chan_update_opt));
2687-
}
2688-
chan_update_opt
2689-
} else {
2690-
if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
2691-
// We really should set `incorrect_cltv_expiry` here but as we're not
2692-
// forwarding over a real channel we can't generate a channel_update
2693-
// for it. Instead we just return a generic temporary_node_failure.
2694-
break Some((
2695-
"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
2696-
0x2000 | 2, None,
2697-
));
2698-
}
2699-
None
2700-
};
2701-
2702-
let cur_height = self.best_block.read().unwrap().height() + 1;
2703-
// Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
2704-
// but we want to be robust wrt to counterparty packet sanitization (see
2705-
// HTLC_FAIL_BACK_BUFFER rationale).
2706-
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
2707-
break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt));
2708-
}
2709-
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
2710-
break Some(("CLTV expiry is too far in the future", 21, None));
2711-
}
2712-
// If the HTLC expires ~now, don't bother trying to forward it to our
2713-
// counterparty. They should fail it anyway, but we don't want to bother with
2714-
// the round-trips or risk them deciding they definitely want the HTLC and
2715-
// force-closing to ensure they get it if we're offline.
2716-
// We previously had a much more aggressive check here which tried to ensure
2717-
// our counterparty receives an HTLC which has *our* risk threshold met on it,
2718-
// but there is no need to do that, and since we're a bit conservative with our
2719-
// risk threshold it just results in failing to forward payments.
2720-
if (*outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
2721-
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt));
2722-
}
2723-
2724-
break None;
2725-
}
2726-
{
2727-
let mut res = VecWriter(Vec::with_capacity(chan_update.serialized_length() + 2 + 8 + 2));
2728-
if let Some(chan_update) = chan_update {
2729-
if code == 0x1000 | 11 || code == 0x1000 | 12 {
2730-
msg.amount_msat.write(&mut res).expect("Writes cannot fail");
2731-
}
2732-
else if code == 0x1000 | 13 {
2733-
msg.cltv_expiry.write(&mut res).expect("Writes cannot fail");
2734-
}
2735-
else if code == 0x1000 | 20 {
2736-
// TODO: underspecified, follow https://github.com/lightning/bolts/issues/791
2737-
0u16.write(&mut res).expect("Writes cannot fail");
2738-
}
2739-
(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
2740-
msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
2741-
chan_update.write(&mut res).expect("Writes cannot fail");
2742-
} else if code & 0x1000 == 0x1000 {
2743-
// If we're trying to return an error that requires a `channel_update` but
2744-
// we're forwarding to a phantom or intercept "channel" (i.e. cannot
2745-
// generate an update), just use the generic "temporary_node_failure"
2746-
// instead.
2747-
code = 0x2000 | 2;
2748-
}
2749-
return_err!(err, code, &res.0[..]);
2750-
}
2751-
}
27522781
}
2753-
2754-
pending_forward_info
27552782
}
27562783

27572784
/// Gets the current [`channel_update`] for the given channel. This first checks if the channel is
@@ -5228,7 +5255,7 @@ where
52285255
//encrypted with the same key. It's not immediately obvious how to usefully exploit that,
52295256
//but we should prevent it anyway.
52305257

5231-
let pending_forward_info = self.decode_update_add_htlc_onion(msg);
5258+
let decoded_hop_res = self.decode_update_add_htlc_onion(msg);
52325259
let per_peer_state = self.per_peer_state.read().unwrap();
52335260
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
52345261
.ok_or_else(|| {
@@ -5240,6 +5267,11 @@ where
52405267
match peer_state.channel_by_id.entry(msg.channel_id) {
52415268
hash_map::Entry::Occupied(mut chan) => {
52425269

5270+
let pending_forward_info = match decoded_hop_res {
5271+
Ok((next_hop, shared_secret)) =>
5272+
self.construct_pending_htlc_info(msg, shared_secret, next_hop),
5273+
Err(e) => PendingHTLCStatus::Fail(e)
5274+
};
52435275
let create_pending_htlc_status = |chan: &Channel<<SP::Target as SignerProvider>::Signer>, pending_forward_info: PendingHTLCStatus, error_code: u16| {
52445276
// If the update_add is completely bogus, the call will Err and we will close,
52455277
// but if we've sent a shutdown and they haven't acknowledged it yet, we just

0 commit comments

Comments
 (0)