Skip to content

Commit e1ab066

Browse files
committed
Never generate a BroadcastChannelUpdate for priv channels
Currently we always generate a `MessageSendEvent::BroadcastChannelUpdate` when a channel is closed even if the channel is private. Our immediate peers should ignore such messages as they haven't seen a corresponding `channel_announcement`, but we are still giving up some privacy by informing our immediate peers of which channels were ours. Here we split `ChannelManager::get_channel_update` into a `get_channel_update_for_broadcast` and `get_channel_update_for_unicast`. The first is used when we are broadcasting a `channel_update`, allowing us to refuse to do so for private channels. The second is used when failing a payment (in which case the recipient has already shown that they are aware of the channel so no such privacy concerns exist).
1 parent 41bb42a commit e1ab066

File tree

1 file changed

+48
-29
lines changed

1 file changed

+48
-29
lines changed

lightning/src/ln/channelmanager.rs

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -782,15 +782,15 @@ macro_rules! convert_chan_err {
782782
$short_to_id.remove(&short_id);
783783
}
784784
let shutdown_res = $channel.force_shutdown(true);
785-
(true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update(&$channel).ok()))
785+
(true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok()))
786786
},
787787
ChannelError::CloseDelayBroadcast(msg) => {
788788
log_error!($self.logger, "Channel {} need to be shutdown but closing transactions not broadcast due to {}", log_bytes!($channel_id[..]), msg);
789789
if let Some(short_id) = $channel.get_short_channel_id() {
790790
$short_to_id.remove(&short_id);
791791
}
792792
let shutdown_res = $channel.force_shutdown(false);
793-
(true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update(&$channel).ok()))
793+
(true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok()))
794794
}
795795
}
796796
}
@@ -846,7 +846,8 @@ macro_rules! handle_monitor_err {
846846
// splitting hairs we'd prefer to claim payments that were to us, but we haven't
847847
// given up the preimage yet, so might as well just wait until the payment is
848848
// retried, avoiding the on-chain fees.
849-
let res: Result<(), _> = Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure".to_owned(), *$chan_id, $chan.force_shutdown(true), $self.get_channel_update(&$chan).ok()));
849+
let res: Result<(), _> = Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure".to_owned(), *$chan_id,
850+
$chan.force_shutdown(true), $self.get_channel_update_for_broadcast(&$chan).ok() ));
850851
(res, true)
851852
},
852853
ChannelMonitorUpdateErr::TemporaryFailure => {
@@ -1226,9 +1227,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
12261227
self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source.0, &htlc_source.1, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
12271228
}
12281229
let chan_update = if let Some(chan) = chan_option {
1229-
if let Ok(update) = self.get_channel_update(&chan) {
1230-
Some(update)
1231-
} else { None }
1230+
self.get_channel_update_for_broadcast(&chan).ok()
12321231
} else { None };
12331232

12341233
if let Some(update) = chan_update {
@@ -1277,7 +1276,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
12771276
};
12781277
log_trace!(self.logger, "Force-closing channel {}", log_bytes!(channel_id[..]));
12791278
self.finish_force_close_channel(chan.force_shutdown(true));
1280-
if let Ok(update) = self.get_channel_update(&chan) {
1279+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
12811280
let mut channel_state = self.channel_state.lock().unwrap();
12821281
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
12831282
msg: update
@@ -1537,31 +1536,31 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
15371536
// hopefully an attacker trying to path-trace payments cannot make this occur
15381537
// on a small/per-node/per-channel scale.
15391538
if !chan.is_live() { // channel_disabled
1540-
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
1539+
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update_for_unicast(chan).unwrap())));
15411540
}
15421541
if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
1543-
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
1542+
break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update_for_unicast(chan).unwrap())));
15441543
}
15451544
let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_holder_fee_base_msat(&self.fee_estimator) as u64) });
15461545
if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
1547-
break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
1546+
break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update_for_unicast(chan).unwrap())));
15481547
}
15491548
if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + chan.get_cltv_expiry_delta() as u64 { // incorrect_cltv_expiry
1550-
break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update(chan).unwrap())));
1549+
break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update_for_unicast(chan).unwrap())));
15511550
}
15521551
let cur_height = self.best_block.read().unwrap().height() + 1;
15531552
// Theoretically, channel counterparty shouldn't send us a HTLC expiring now, but we want to be robust wrt to counterparty
15541553
// packet sanitization (see HTLC_FAIL_BACK_BUFFER rational)
15551554
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
1556-
break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update(chan).unwrap())));
1555+
break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
15571556
}
15581557
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
15591558
break Some(("CLTV expiry is too far in the future", 21, None));
15601559
}
15611560
// In theory, we would be safe against unitentional channel-closure, if we only required a margin of LATENCY_GRACE_PERIOD_BLOCKS.
15621561
// But, to be safe against policy reception, we use a longuer delay.
15631562
if (*outgoing_cltv_value) as u64 <= (cur_height + HTLC_FAIL_BACK_BUFFER) as u64 {
1564-
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update(chan).unwrap())));
1563+
break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
15651564
}
15661565

15671566
break None;
@@ -1589,9 +1588,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
15891588
(pending_forward_info, channel_state.unwrap())
15901589
}
15911590

1592-
/// only fails if the channel does not yet have an assigned short_id
1591+
/// Gets the current channel_update for the given channel. This first checks if the channel is
1592+
/// public, and thus should be called whenever the result is going to be passed out in a
1593+
/// [`MessageSendEvent::BroadcastChannelUpdate`] event.
1594+
///
1595+
/// May be called with channel_state already locked!
1596+
fn get_channel_update_for_broadcast(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
1597+
if !chan.should_announce() {
1598+
return Err(LightningError {
1599+
err: "Cannot broadcast a channel_update for a private channel".to_owned(),
1600+
action: msgs::ErrorAction::IgnoreError
1601+
});
1602+
}
1603+
self.get_channel_update_for_unicast(chan)
1604+
}
1605+
1606+
/// Gets the current channel_update for the given channel. This does not check if the channel
1607+
/// is public (only returning an Err if the channel does not yet have an assigned short_id),
1608+
/// and thus MUST NOT be called unless the recipient of the resulting message has already
1609+
/// provided evidence that they know about the existence of the channel.
15931610
/// May be called with channel_state already locked!
1594-
fn get_channel_update(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
1611+
fn get_channel_update_for_unicast(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
15951612
let short_channel_id = match chan.get_short_channel_id() {
15961613
None => return Err(LightningError{err: "Channel not yet established".to_owned(), action: msgs::ErrorAction::IgnoreError}),
15971614
Some(id) => id,
@@ -1983,7 +2000,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
19832000
if let Some(msg) = chan.get_signed_channel_announcement(&self.our_network_key, self.get_our_node_id(), self.genesis_hash.clone()) {
19842001
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
19852002
msg,
1986-
update_msg: match self.get_channel_update(chan) {
2003+
update_msg: match self.get_channel_update_for_broadcast(chan) {
19872004
Ok(msg) => msg,
19882005
Err(_) => continue,
19892006
},
@@ -2075,7 +2092,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
20752092
} else {
20762093
panic!("Stated return value requirements in send_htlc() were not met");
20772094
}
2078-
let chan_update = self.get_channel_update(chan.get()).unwrap();
2095+
let chan_update = self.get_channel_update_for_unicast(chan.get()).unwrap();
20792096
failed_forwards.push((htlc_source, payment_hash,
20802097
HTLCFailReason::Reason { failure_code: 0x1000 | 7, data: chan_update.encode_with_len() }
20812098
));
@@ -2147,7 +2164,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
21472164
if let Some(short_id) = channel.get_short_channel_id() {
21482165
channel_state.short_to_id.remove(&short_id);
21492166
}
2150-
Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(true), self.get_channel_update(&channel).ok()))
2167+
Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok()))
21512168
},
21522169
ChannelError::CloseDelayBroadcast(_) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
21532170
};
@@ -2348,7 +2365,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
23482365
ChannelUpdateStatus::DisabledStaged if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
23492366
ChannelUpdateStatus::EnabledStaged if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
23502367
ChannelUpdateStatus::DisabledStaged if !chan.is_live() => {
2351-
if let Ok(update) = self.get_channel_update(&chan) {
2368+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
23522369
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
23532370
msg: update
23542371
});
@@ -2357,7 +2374,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
23572374
chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
23582375
},
23592376
ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
2360-
if let Ok(update) = self.get_channel_update(&chan) {
2377+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
23612378
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
23622379
msg: update
23632380
});
@@ -2407,7 +2424,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
24072424
let (failure_code, onion_failure_data) =
24082425
match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
24092426
hash_map::Entry::Occupied(chan_entry) => {
2410-
if let Ok(upd) = self.get_channel_update(&chan_entry.get()) {
2427+
if let Ok(upd) = self.get_channel_update_for_unicast(&chan_entry.get()) {
24112428
(0x1000|7, upd.encode_with_len())
24122429
} else {
24132430
(0x4000|10, Vec::new())
@@ -2988,7 +3005,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
29883005
self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source.0, &htlc_source.1, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
29893006
}
29903007
if let Some(chan) = chan_option {
2991-
if let Ok(update) = self.get_channel_update(&chan) {
3008+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
29923009
let mut channel_state = self.channel_state.lock().unwrap();
29933010
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
29943011
msg: update
@@ -3034,7 +3051,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
30343051
self.tx_broadcaster.broadcast_transaction(&broadcast_tx);
30353052
}
30363053
if let Some(chan) = chan_option {
3037-
if let Ok(update) = self.get_channel_update(&chan) {
3054+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
30383055
let mut channel_state = self.channel_state.lock().unwrap();
30393056
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
30403057
msg: update
@@ -3072,7 +3089,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
30723089
// want to reject the new HTLC and fail it backwards instead of forwarding.
30733090
match pending_forward_info {
30743091
PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => {
3075-
let reason = if let Ok(upd) = self.get_channel_update(chan) {
3092+
let reason = if let Ok(upd) = self.get_channel_update_for_unicast(chan) {
30763093
onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
30773094
let mut res = Vec::with_capacity(8 + 128);
30783095
// TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
@@ -3334,7 +3351,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
33343351

33353352
channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
33363353
msg: try_chan_entry!(self, chan.get_mut().announcement_signatures(&self.our_network_key, self.get_our_node_id(), self.genesis_hash.clone(), msg), channel_state, chan),
3337-
update_msg: self.get_channel_update(chan.get()).unwrap(), // can only fail if we're not in a ready state
3354+
// Note that announcement_signatures fails if the channel cannot be announced,
3355+
// so get_channel_update_for_broadcast will never fail by the time we get here.
3356+
update_msg: self.get_channel_update_for_broadcast(chan.get()).unwrap(),
33383357
});
33393358
},
33403359
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
@@ -3479,7 +3498,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
34793498
short_to_id.remove(&short_id);
34803499
}
34813500
failed_channels.push(chan.force_shutdown(false));
3482-
if let Ok(update) = self.get_channel_update(&chan) {
3501+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
34833502
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
34843503
msg: update
34853504
});
@@ -3918,7 +3937,7 @@ where
39183937
let res = f(channel);
39193938
if let Ok((chan_res, mut timed_out_pending_htlcs)) = res {
39203939
for (source, payment_hash) in timed_out_pending_htlcs.drain(..) {
3921-
let chan_update = self.get_channel_update(&channel).map(|u| u.encode_with_len()).unwrap(); // Cannot add/recv HTLCs before we have a short_id so unwrap is safe
3940+
let chan_update = self.get_channel_update_for_unicast(&channel).map(|u| u.encode_with_len()).unwrap(); // Cannot add/recv HTLCs before we have a short_id so unwrap is safe
39223941
timed_out_htlcs.push((source, payment_hash, HTLCFailReason::Reason {
39233942
failure_code: 0x1000 | 14, // expiry_too_soon, or at least it is now
39243943
data: chan_update,
@@ -3947,7 +3966,7 @@ where
39473966
// It looks like our counterparty went on-chain or funding transaction was
39483967
// reorged out of the main chain. Close the channel.
39493968
failed_channels.push(channel.force_shutdown(true));
3950-
if let Ok(update) = self.get_channel_update(&channel) {
3969+
if let Ok(update) = self.get_channel_update_for_broadcast(&channel) {
39513970
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
39523971
msg: update
39533972
});
@@ -4126,7 +4145,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
41264145
short_to_id.remove(&short_id);
41274146
}
41284147
failed_channels.push(chan.force_shutdown(true));
4129-
if let Ok(update) = self.get_channel_update(&chan) {
4148+
if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
41304149
pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
41314150
msg: update
41324151
});

0 commit comments

Comments
 (0)