Skip to content

Commit f0b3acb

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 1e10250 commit f0b3acb

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
@@ -2502,12 +2502,14 @@ where
25022502
})
25032503
}
25042504

2505-
fn decode_update_add_htlc_onion(&self, msg: &msgs::UpdateAddHTLC) -> PendingHTLCStatus {
2505+
fn decode_update_add_htlc_onion(
2506+
&self, msg: &msgs::UpdateAddHTLC
2507+
) -> Result<(onion_utils::Hop, [u8; 32]), HTLCFailureMsg> {
25062508
macro_rules! return_malformed_err {
25072509
($msg: expr, $err_code: expr) => {
25082510
{
25092511
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2510-
return PendingHTLCStatus::Fail(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
2512+
return Err(HTLCFailureMsg::Malformed(msgs::UpdateFailMalformedHTLC {
25112513
channel_id: msg.channel_id,
25122514
htlc_id: msg.htlc_id,
25132515
sha256_of_onion: Sha256::hash(&msg.onion_routing_packet.hop_data).into_inner(),
@@ -2538,7 +2540,7 @@ where
25382540
($msg: expr, $err_code: expr, $data: expr) => {
25392541
{
25402542
log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg);
2541-
return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
2543+
return Err(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
25422544
channel_id: msg.channel_id,
25432545
htlc_id: msg.htlc_id,
25442546
reason: HTLCFailReason::reason($err_code, $data.to_vec())
@@ -2557,8 +2559,174 @@ where
25572559
return_err!(err_msg, err_code, &[0; 0]);
25582560
},
25592561
};
2562+
let (outgoing_scid, outgoing_amt_msat, outgoing_cltv_value) = match next_hop {
2563+
onion_utils::Hop::Forward {
2564+
next_hop_data: msgs::OnionHopData {
2565+
format: msgs::OnionHopDataFormat::NonFinalNode { short_channel_id }, amt_to_forward,
2566+
outgoing_cltv_value,
2567+
}, ..
2568+
} => (short_channel_id, amt_to_forward, outgoing_cltv_value),
2569+
// We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the
2570+
// inbound channel's state.
2571+
onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret)),
2572+
_ => {
2573+
return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]);
2574+
}
2575+
};
25602576

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

27472774
/// Gets the current [`channel_update`] for the given channel. This first checks if the channel is
@@ -5258,7 +5285,7 @@ where
52585285
//encrypted with the same key. It's not immediately obvious how to usefully exploit that,
52595286
//but we should prevent it anyway.
52605287

5261-
let pending_forward_info = self.decode_update_add_htlc_onion(msg);
5288+
let decoded_hop_res = self.decode_update_add_htlc_onion(msg);
52625289
let per_peer_state = self.per_peer_state.read().unwrap();
52635290
let peer_state_mutex = per_peer_state.get(counterparty_node_id)
52645291
.ok_or_else(|| {
@@ -5270,6 +5297,11 @@ where
52705297
match peer_state.channel_by_id.entry(msg.channel_id) {
52715298
hash_map::Entry::Occupied(mut chan) => {
52725299

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

0 commit comments

Comments
 (0)