From 84fa127661ff09de94ac6129dd0790d6de79f8b7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 15 Feb 2022 23:27:07 +0000 Subject: [PATCH] Provide our peers with SCID aliases and forward payments with them This creates an SCID alias for all of our outbound channels, which we send to our counterparties as a part of the `funding_locked` message and then recognize in any HTLC forwarding instructions. Note that we generate an SCID alias for all channels, including already open ones, even though we currently have no way of communicating to our peers the SCID alias for already-open channels. --- lightning/src/ln/channel.rs | 56 +++++-- lightning/src/ln/channelmanager.rs | 177 +++++++++++++++++----- lightning/src/ln/functional_tests.rs | 20 ++- lightning/src/ln/onion_route_tests.rs | 2 +- lightning/src/ln/priv_short_conf_tests.rs | 34 +++++ lightning/src/ln/reorg_tests.rs | 2 +- lightning/src/util/scid_utils.rs | 15 +- 7 files changed, 236 insertions(+), 70 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 99fb345be..65f1be000 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -702,6 +702,12 @@ pub(super) struct Channel { // We only bother storing the most recent SCID alias at any time, though our counterparty has // to store all of them. latest_inbound_scid_alias: Option, + + // We always offer our counterparty a static SCID alias, which we recognize as for this channel + // if we see it in HTLC forwarding instructions. We don't bother rotating the alias given we + // don't currently support node id aliases and eventually privacy should be provided with + // blinded paths instead of simple scid+node_id aliases. + outbound_scid_alias: u64, } #[cfg(any(test, fuzzing))] @@ -807,7 +813,8 @@ impl Channel { // Constructors: pub fn new_outbound( fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures, - channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig, current_chain_height: u32 + channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig, current_chain_height: u32, + outbound_scid_alias: u64 ) -> Result, APIError> where K::Target: KeysInterface, F::Target: FeeEstimator, @@ -955,6 +962,7 @@ impl Channel { workaround_lnd_bug_4006: None, latest_inbound_scid_alias: None, + outbound_scid_alias, #[cfg(any(test, fuzzing))] historical_inbound_htlc_fulfills: HashSet::new(), @@ -993,7 +1001,8 @@ impl Channel { /// Assumes chain_hash has already been checked and corresponds with what we expect! pub fn new_from_req( fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures, - msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig, current_chain_height: u32, logger: &L + msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig, current_chain_height: u32, logger: &L, + outbound_scid_alias: u64 ) -> Result, ChannelError> where K::Target: KeysInterface, F::Target: FeeEstimator, @@ -1262,6 +1271,7 @@ impl Channel { workaround_lnd_bug_4006: None, latest_inbound_scid_alias: None, + outbound_scid_alias, #[cfg(any(test, fuzzing))] historical_inbound_htlc_fulfills: HashSet::new(), @@ -3477,7 +3487,7 @@ impl Channel { Some(msgs::FundingLocked { channel_id: self.channel_id(), next_per_commitment_point, - short_channel_id_alias: None, + short_channel_id_alias: Some(self.outbound_scid_alias), }) } else { None }; @@ -3699,7 +3709,7 @@ impl Channel { funding_locked: Some(msgs::FundingLocked { channel_id: self.channel_id(), next_per_commitment_point, - short_channel_id_alias: None, + short_channel_id_alias: Some(self.outbound_scid_alias), }), raa: None, commitment_update: None, mon_update: None, order: RAACommitmentOrder::CommitmentFirst, @@ -3735,7 +3745,7 @@ impl Channel { Some(msgs::FundingLocked { channel_id: self.channel_id(), next_per_commitment_point, - short_channel_id_alias: None, + short_channel_id_alias: Some(self.outbound_scid_alias), }) } else { None }; @@ -4223,6 +4233,17 @@ impl Channel { self.latest_inbound_scid_alias } + /// Allowed in any state (including after shutdown) + pub fn outbound_scid_alias(&self) -> u64 { + self.outbound_scid_alias + } + /// Only allowed immediately after deserialization if get_outbound_scid_alias returns 0, + /// indicating we were written by an old LDK which did not set outbound SCID aliases. + pub fn set_outbound_scid_alias(&mut self, outbound_scid_alias: u64) { + assert_eq!(self.outbound_scid_alias, 0); + self.outbound_scid_alias = outbound_scid_alias; + } + /// Returns the funding_txo we either got from our peer, or were given by /// get_outbound_funding_created. pub fn get_funding_txo(&self) -> Option { @@ -4475,7 +4496,7 @@ impl Channel { return Some(msgs::FundingLocked { channel_id: self.channel_id, next_per_commitment_point, - short_channel_id_alias: None, + short_channel_id_alias: Some(self.outbound_scid_alias), }); } } else { @@ -5795,6 +5816,7 @@ impl Writeable for Channel { (15, preimages, vec_type), (17, self.announcement_sigs_state, required), (19, self.latest_inbound_scid_alias, option), + (21, self.outbound_scid_alias, required), }); Ok(()) @@ -6051,6 +6073,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel // AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine. let mut announcement_sigs_state = Some(AnnouncementSigsState::NotSent); let mut latest_inbound_scid_alias = None; + let mut outbound_scid_alias = None; read_tlv_fields!(reader, { (0, announcement_sigs, option), @@ -6067,6 +6090,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel (15, preimages_opt, vec_type), (17, announcement_sigs_state, option), (19, latest_inbound_scid_alias, option), + (21, outbound_scid_alias, option), }); if let Some(preimages) = preimages_opt { @@ -6202,6 +6226,8 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel workaround_lnd_bug_4006: None, latest_inbound_scid_alias, + // Later in the ChannelManager deserialization phase we scan for channels and assign scid aliases if its missing + outbound_scid_alias: outbound_scid_alias.unwrap_or(0), #[cfg(any(test, fuzzing))] historical_inbound_htlc_fulfills, @@ -6325,7 +6351,7 @@ mod tests { let secp_ctx = Secp256k1::new(); let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - match Channel::::new_outbound(&&fee_estimator, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0) { + match Channel::::new_outbound(&&fee_estimator, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0, 42) { Err(APIError::IncompatibleShutdownScript { script }) => { assert_eq!(script.into_inner(), non_v0_segwit_shutdown_script.into_inner()); }, @@ -6347,7 +6373,7 @@ mod tests { let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let node_a_chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); + let node_a_chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap(); // Now change the fee so we can check that the fee in the open_channel message is the // same as the old fee. @@ -6373,13 +6399,13 @@ mod tests { // Create Node A's channel pointing to Node B's pubkey let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap(); // Create Node B's channel by receiving Node A's open_channel message // Make sure A's dust limit is as we expect. let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash()); let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap()); - let mut node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger).unwrap(); + let mut node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap(); // Node B --> Node A: accept channel, explicitly setting B's dust limit. let mut accept_channel_msg = node_b_chan.accept_inbound_channel(); @@ -6443,7 +6469,7 @@ mod tests { let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); + let mut chan = Channel::::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap(); let commitment_tx_fee_0_htlcs = Channel::::commit_tx_fee_msat(chan.feerate_per_kw, 0, chan.opt_anchors()); let commitment_tx_fee_1_htlc = Channel::::commit_tx_fee_msat(chan.feerate_per_kw, 1, chan.opt_anchors()); @@ -6492,12 +6518,12 @@ mod tests { // Create Node A's channel pointing to Node B's pubkey let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap(); // Create Node B's channel by receiving Node A's open_channel message let open_channel_msg = node_a_chan.get_open_channel(chain_hash); let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap()); - let mut node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger).unwrap(); + let mut node_b_chan = Channel::::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap(); // Node B --> Node A: accept channel let accept_channel_msg = node_b_chan.accept_inbound_channel(); @@ -6554,7 +6580,7 @@ mod tests { // Create a channel. let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let config = UserConfig::default(); - let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap(); + let mut node_a_chan = Channel::::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap(); assert!(node_a_chan.counterparty_forwarding_info.is_none()); assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); // the default assert!(node_a_chan.counterparty_forwarding_info().is_none()); @@ -6619,7 +6645,7 @@ mod tests { let counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let mut config = UserConfig::default(); config.channel_options.announced_channel = false; - let mut chan = Channel::::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, &InitFeatures::known(), 10_000_000, 100000, 42, &config, 0).unwrap(); // Nothing uses their network key in this test + let mut chan = Channel::::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, &InitFeatures::known(), 10_000_000, 100000, 42, &config, 0, 42).unwrap(); // Nothing uses their network key in this test chan.holder_dust_limit_satoshis = 546; chan.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 94d78e798..699941586 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -660,8 +660,16 @@ pub(super) enum RAACommitmentOrder { // Note this is only exposed in cfg(test): pub(super) struct ChannelHolder { pub(super) by_id: HashMap<[u8; 32], Channel>, + /// SCIDs (and outbound SCID aliases) to the real channel id. Outbound SCID aliases are added + /// here once the channel is available for normal use, with SCIDs being added once the funding + /// transaction is confirmed at the channel's required confirmation depth. pub(super) short_to_id: HashMap, - /// short channel id -> forward infos. Key of 0 means payments received + /// SCID/SCID Alias -> forward infos. Key of 0 means payments received. + /// + /// Note that because we may have an SCID Alias as the key we can have two entries per channel, + /// though in practice we probably won't be receiving HTLCs for a channel both via the alias + /// and via the classic SCID. + /// /// Note that while this is held in the same mutex as the channels themselves, no consistency /// guarantees are made about the existence of a channel with the short id here, nor the short /// ids in the PendingHTLCInfo! @@ -971,6 +979,12 @@ pub struct ChannelManager>, + /// The set of outbound SCID aliases across all our channels, including unconfirmed channels + /// and some closed channels which reached a usable state prior to being closed. This is used + /// only to avoid duplicates, and is not persisted explicitly to disk, but rebuilt from the + /// active channel list on load. + outbound_scid_aliases: Mutex>, + our_network_key: SecretKey, our_network_pubkey: PublicKey, @@ -1406,10 +1420,20 @@ macro_rules! handle_error { } macro_rules! update_maps_on_chan_removal { - ($short_to_id: expr, $channel: expr) => { + ($self: expr, $short_to_id: expr, $channel: expr) => { if let Some(short_id) = $channel.get_short_channel_id() { $short_to_id.remove(&short_id); + } else { + // If the channel was never confirmed on-chain prior to its closure, remove the + // outbound SCID alias we used for it from the collision-prevention set. While we + // generally want to avoid ever re-using an outbound SCID alias across all channels, we + // also don't want a counterparty to be able to trivially cause a memory leak by simply + // opening a million channels with us which are closed before we ever reach the funding + // stage. + let alias_removed = $self.outbound_scid_aliases.lock().unwrap().remove(&$channel.outbound_scid_alias()); + debug_assert!(alias_removed); } + $short_to_id.remove(&$channel.outbound_scid_alias()); } } @@ -1425,14 +1449,14 @@ macro_rules! convert_chan_err { }, ChannelError::Close(msg) => { log_error!($self.logger, "Closing channel {} due to close-required error: {}", log_bytes!($channel_id[..]), msg); - update_maps_on_chan_removal!($short_to_id, $channel); + update_maps_on_chan_removal!($self, $short_to_id, $channel); let shutdown_res = $channel.force_shutdown(true); (true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, $channel.get_user_id(), shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok())) }, ChannelError::CloseDelayBroadcast(msg) => { log_error!($self.logger, "Channel {} need to be shutdown but closing transactions not broadcast due to {}", log_bytes!($channel_id[..]), msg); - update_maps_on_chan_removal!($short_to_id, $channel); + update_maps_on_chan_removal!($self, $short_to_id, $channel); let shutdown_res = $channel.force_shutdown(false); (true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, $channel.get_user_id(), shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok())) @@ -1472,10 +1496,10 @@ macro_rules! try_chan_entry { } macro_rules! remove_channel { - ($channel_state: expr, $entry: expr) => { + ($self: expr, $channel_state: expr, $entry: expr) => { { let channel = $entry.remove_entry().1; - update_maps_on_chan_removal!($channel_state.short_to_id, channel); + update_maps_on_chan_removal!($self, $channel_state.short_to_id, channel); channel } } @@ -1486,7 +1510,7 @@ macro_rules! handle_monitor_err { match $err { ChannelMonitorUpdateErr::PermanentFailure => { log_error!($self.logger, "Closing channel {} due to monitor update ChannelMonitorUpdateErr::PermanentFailure", log_bytes!($chan_id[..])); - update_maps_on_chan_removal!($short_to_id, $chan); + update_maps_on_chan_removal!($self, $short_to_id, $chan); // TODO: $failed_fails is dropped here, which will cause other channels to hit the // chain in a confused state! We need to move them into the ChannelMonitor which // will be responsible for failing backwards once things confirm on-chain. @@ -1568,15 +1592,34 @@ macro_rules! maybe_break_monitor_err { } } +macro_rules! send_funding_locked { + ($short_to_id: expr, $pending_msg_events: expr, $channel: expr, $funding_locked_msg: expr) => { + $pending_msg_events.push(events::MessageSendEvent::SendFundingLocked { + node_id: $channel.get_counterparty_node_id(), + msg: $funding_locked_msg, + }); + // Note that we may send a funding locked multiple times for a channel if we reconnect, so + // we allow collisions, but we shouldn't ever be updating the channel ID pointed to. + let outbound_alias_insert = $short_to_id.insert($channel.outbound_scid_alias(), $channel.channel_id()); + assert!(outbound_alias_insert.is_none() || outbound_alias_insert.unwrap() == $channel.channel_id(), + "SCIDs should never collide - ensure you weren't behind the chain tip by a full month when creating channels"); + if let Some(real_scid) = $channel.get_short_channel_id() { + let scid_insert = $short_to_id.insert(real_scid, $channel.channel_id()); + assert!(scid_insert.is_none() || scid_insert.unwrap() == $channel.channel_id(), + "SCIDs should never collide - ensure you weren't behind the chain tip by a full month when creating channels"); + } + } +} + macro_rules! handle_chan_restoration_locked { ($self: ident, $channel_lock: expr, $channel_state: expr, $channel_entry: expr, $raa: expr, $commitment_update: expr, $order: expr, $chanmon_update: expr, $pending_forwards: expr, $funding_broadcastable: expr, $funding_locked: expr, $announcement_sigs: expr) => { { let mut htlc_forwards = None; - let counterparty_node_id = $channel_entry.get().get_counterparty_node_id(); let chanmon_update: Option = $chanmon_update; // Force type-checking to resolve let chanmon_update_is_none = chanmon_update.is_none(); + let counterparty_node_id = $channel_entry.get().get_counterparty_node_id(); let res = loop { let forwards: Vec<(PendingHTLCInfo, u64)> = $pending_forwards; // Force type-checking to resolve if !forwards.is_empty() { @@ -1602,11 +1645,7 @@ macro_rules! handle_chan_restoration_locked { // Similar to the above, this implies that we're letting the funding_locked fly // before it should be allowed to. assert!(chanmon_update.is_none()); - $channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingLocked { - node_id: counterparty_node_id, - msg, - }); - $channel_state.short_to_id.insert($channel_entry.get().get_short_channel_id().unwrap(), $channel_entry.get().channel_id()); + send_funding_locked!($channel_state.short_to_id, $channel_state.pending_msg_events, $channel_entry.get(), msg); } if let Some(msg) = $announcement_sigs { $channel_state.pending_msg_events.push(events::MessageSendEvent::SendAnnouncementSignatures { @@ -1735,6 +1774,7 @@ impl ChannelMana claimable_htlcs: HashMap::new(), pending_msg_events: Vec::new(), }), + outbound_scid_aliases: Mutex::new(HashSet::new()), pending_inbound_payments: Mutex::new(HashMap::new()), pending_outbound_payments: Mutex::new(HashMap::new()), @@ -1766,6 +1806,25 @@ impl ChannelMana &self.default_configuration } + fn create_and_insert_outbound_scid_alias(&self) -> u64 { + let height = self.best_block.read().unwrap().height(); + let mut outbound_scid_alias = 0; + let mut i = 0; + loop { + if cfg!(fuzzing) { // fuzzing chacha20 doesn't use the key at all so we always get the same alias + outbound_scid_alias += 1; + } else { + outbound_scid_alias = fake_scid::Namespace::OutboundAlias.get_fake_scid(height, &self.genesis_hash, &self.fake_scid_rand_bytes, &self.keys_manager); + } + if outbound_scid_alias != 0 && self.outbound_scid_aliases.lock().unwrap().insert(outbound_scid_alias) { + break; + } + i += 1; + if i > 1_000_000 { panic!("Your RNG is busted or we ran out of possible outbound SCID aliases (which should never happen before we run out of memory to store channels"); } + } + outbound_scid_alias + } + /// Creates a new outbound channel to the given remote node and with the given value. /// /// `user_channel_id` will be provided back as in @@ -1801,11 +1860,20 @@ impl ChannelMana let per_peer_state = self.per_peer_state.read().unwrap(); match per_peer_state.get(&their_network_key) { Some(peer_state) => { + let outbound_scid_alias = self.create_and_insert_outbound_scid_alias(); let peer_state = peer_state.lock().unwrap(); let their_features = &peer_state.latest_features; let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration }; - Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, - channel_value_satoshis, push_msat, user_channel_id, config, self.best_block.read().unwrap().height())? + match Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, + their_features, channel_value_satoshis, push_msat, user_channel_id, config, + self.best_block.read().unwrap().height(), outbound_scid_alias) + { + Ok(res) => res, + Err(e) => { + self.outbound_scid_aliases.lock().unwrap().remove(&outbound_scid_alias); + return Err(e); + }, + } }, None => return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }), } @@ -1943,7 +2011,7 @@ impl ChannelMana let (result, is_permanent) = handle_monitor_err!(self, e, channel_state.short_to_id, chan_entry.get_mut(), RAACommitmentOrder::CommitmentFirst, chan_entry.key(), NO_UPDATE); if is_permanent { - remove_channel!(channel_state, chan_entry); + remove_channel!(self, channel_state, chan_entry); break result; } } @@ -1955,7 +2023,7 @@ impl ChannelMana }); if chan_entry.get().is_shutdown() { - let channel = remove_channel!(channel_state, chan_entry); + let channel = remove_channel!(self, channel_state, chan_entry); if let Ok(channel_update) = self.get_channel_update_for_broadcast(&channel) { channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { msg: channel_update @@ -2056,7 +2124,7 @@ impl ChannelMana } else { self.issue_channel_close_events(chan.get(),ClosureReason::HolderForceClosed); } - remove_channel!(channel_state, chan) + remove_channel!(self, channel_state, chan) } else { return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()}); } @@ -3203,7 +3271,7 @@ impl ChannelMana } ChannelError::Close(msg) => { log_trace!(self.logger, "Closing channel {} due to Close-required error: {}", log_bytes!(chan.key()[..]), msg); - let mut channel = remove_channel!(channel_state, chan); + let mut channel = remove_channel!(self, channel_state, chan); // ChannelClosed event is generated by handle_error for us. Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel.channel_id(), channel.get_user_id(), channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok())) }, @@ -4215,13 +4283,24 @@ impl ChannelMana return Err(MsgHandleErrInternal::send_err_msg_no_close("No inbound channels accepted".to_owned(), msg.temporary_channel_id.clone())); } - let mut channel = Channel::new_from_req(&self.fee_estimator, &self.keys_manager, counterparty_node_id.clone(), - &their_features, msg, 0, &self.default_configuration, self.best_block.read().unwrap().height(), &self.logger) - .map_err(|e| MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id))?; + let outbound_scid_alias = self.create_and_insert_outbound_scid_alias(); + let mut channel = match Channel::new_from_req(&self.fee_estimator, &self.keys_manager, + counterparty_node_id.clone(), &their_features, msg, 0, &self.default_configuration, + self.best_block.read().unwrap().height(), &self.logger, outbound_scid_alias) + { + Err(e) => { + self.outbound_scid_aliases.lock().unwrap().remove(&outbound_scid_alias); + return Err(MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id)); + }, + Ok(res) => res + }; let mut channel_state_lock = self.channel_state.lock().unwrap(); let channel_state = &mut *channel_state_lock; match channel_state.by_id.entry(channel.channel_id()) { - hash_map::Entry::Occupied(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("temporary_channel_id collision!".to_owned(), msg.temporary_channel_id.clone())), + hash_map::Entry::Occupied(_) => { + self.outbound_scid_aliases.lock().unwrap().remove(&outbound_scid_alias); + return Err(MsgHandleErrInternal::send_err_msg_no_close("temporary_channel_id collision!".to_owned(), msg.temporary_channel_id.clone())) + }, hash_map::Entry::Vacant(entry) => { if !self.default_configuration.manually_accept_inbound_channels { channel_state.pending_msg_events.push(events::MessageSendEvent::SendAcceptChannel { @@ -4425,7 +4504,7 @@ impl ChannelMana let (result, is_permanent) = handle_monitor_err!(self, e, channel_state.short_to_id, chan_entry.get_mut(), RAACommitmentOrder::CommitmentFirst, chan_entry.key(), NO_UPDATE); if is_permanent { - remove_channel!(channel_state, chan_entry); + remove_channel!(self, channel_state, chan_entry); break result; } } @@ -4473,7 +4552,7 @@ impl ChannelMana // also implies there are no pending HTLCs left on the channel, so we can // fully delete it from tracking (the channel monitor is still around to // watch for old state broadcasts)! - (tx, Some(remove_channel!(channel_state, chan_entry))) + (tx, Some(remove_channel!(self, channel_state, chan_entry))) } else { (tx, None) } }, hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) @@ -4913,7 +4992,7 @@ impl ChannelMana let by_id = &mut channel_state.by_id; let pending_msg_events = &mut channel_state.pending_msg_events; if let hash_map::Entry::Occupied(chan_entry) = by_id.entry(funding_outpoint.to_channel_id()) { - let mut chan = remove_channel!(channel_state, chan_entry); + let mut chan = remove_channel!(self, channel_state, chan_entry); failed_channels.push(chan.force_shutdown(false)); if let Ok(update) = self.get_channel_update_for_broadcast(&chan) { pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { @@ -5052,7 +5131,7 @@ impl ChannelMana log_info!(self.logger, "Broadcasting {}", log_tx!(tx)); self.tx_broadcaster.broadcast_transaction(&tx); - update_maps_on_chan_removal!(short_to_id, chan); + update_maps_on_chan_removal!(self, short_to_id, chan); false } else { true } }, @@ -5249,7 +5328,7 @@ impl ChannelMana let mut channel_state = self.channel_state.lock().unwrap(); let best_block = self.best_block.read().unwrap(); loop { - let scid_candidate = fake_scid::get_phantom_scid(&self.fake_scid_rand_bytes, best_block.height(), &self.genesis_hash, &self.keys_manager); + let scid_candidate = fake_scid::Namespace::Phantom.get_fake_scid(best_block.height(), &self.genesis_hash, &self.fake_scid_rand_bytes, &self.keys_manager); // Ensure the generated scid doesn't conflict with a real channel. match channel_state.short_to_id.entry(scid_candidate) { hash_map::Entry::Occupied(_) => continue, @@ -5537,10 +5616,7 @@ where })); } if let Some(funding_locked) = funding_locked_opt { - pending_msg_events.push(events::MessageSendEvent::SendFundingLocked { - node_id: channel.get_counterparty_node_id(), - msg: funding_locked, - }); + send_funding_locked!(short_to_id, pending_msg_events, channel, funding_locked); if channel.is_usable() { log_trace!(self.logger, "Sending funding_locked with private initial channel_update for our counterparty on channel {}", log_bytes!(channel.channel_id())); pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate { @@ -5550,7 +5626,6 @@ where } else { log_trace!(self.logger, "Sending funding_locked WITHOUT channel_update for {}", log_bytes!(channel.channel_id())); } - short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id()); } if let Some(announcement_sigs) = announcement_sigs { log_trace!(self.logger, "Sending announcement_signatures for channel {}", log_bytes!(channel.channel_id())); @@ -5570,7 +5645,7 @@ where } } } else if let Err(reason) = res { - update_maps_on_chan_removal!(short_to_id, channel); + update_maps_on_chan_removal!(self, short_to_id, channel); // It looks like our counterparty went on-chain or funding transaction was // reorged out of the main chain. Close the channel. failed_channels.push(channel.force_shutdown(true)); @@ -5760,13 +5835,13 @@ impl { let mut channel_state_lock = self.channel_state.lock().unwrap(); let channel_state = &mut *channel_state_lock; - let short_to_id = &mut channel_state.short_to_id; let pending_msg_events = &mut channel_state.pending_msg_events; + let short_to_id = &mut channel_state.short_to_id; if no_connection_possible { log_debug!(self.logger, "Failing all channels with {} due to no_connection_possible", log_pubkey!(counterparty_node_id)); channel_state.by_id.retain(|_, chan| { if chan.get_counterparty_node_id() == *counterparty_node_id { - update_maps_on_chan_removal!(short_to_id, chan); + update_maps_on_chan_removal!(self, short_to_id, chan); failed_channels.push(chan.force_shutdown(true)); if let Ok(update) = self.get_channel_update_for_broadcast(&chan) { pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { @@ -5785,7 +5860,7 @@ impl if chan.get_counterparty_node_id() == *counterparty_node_id { chan.remove_uncommitted_htlcs_and_mark_paused(&self.logger); if chan.is_shutdown() { - update_maps_on_chan_removal!(short_to_id, chan); + update_maps_on_chan_removal!(self, short_to_id, chan); self.issue_channel_close_events(chan, ClosureReason::DisconnectedPeer); return false; } else { @@ -6785,6 +6860,32 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> } } + let mut outbound_scid_aliases = HashSet::new(); + for (chan_id, chan) in by_id.iter_mut() { + if chan.outbound_scid_alias() == 0 { + let mut outbound_scid_alias; + loop { + outbound_scid_alias = fake_scid::Namespace::OutboundAlias + .get_fake_scid(best_block_height, &genesis_hash, fake_scid_rand_bytes.as_ref().unwrap(), &args.keys_manager); + if outbound_scid_aliases.insert(outbound_scid_alias) { break; } + } + chan.set_outbound_scid_alias(outbound_scid_alias); + } else if !outbound_scid_aliases.insert(chan.outbound_scid_alias()) { + // Note that in rare cases its possible to hit this while reading an older + // channel if we just happened to pick a colliding outbound alias above. + log_error!(args.logger, "Got duplicate outbound SCID alias; {}", chan.outbound_scid_alias()); + return Err(DecodeError::InvalidValue); + } + if chan.is_usable() { + if short_to_id.insert(chan.outbound_scid_alias(), *chan_id).is_some() { + // Note that in rare cases its possible to hit this while reading an older + // channel if we just happened to pick a colliding outbound alias above. + log_error!(args.logger, "Got duplicate outbound SCID alias; {}", chan.outbound_scid_alias()); + return Err(DecodeError::InvalidValue); + } + } + } + let inbound_pmt_key_material = args.keys_manager.get_inbound_payment_key_material(); let expanded_inbound_key = inbound_payment::ExpandedKey::new(&inbound_pmt_key_material); let channel_manager = ChannelManager { @@ -6805,6 +6906,8 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> inbound_payment_key: expanded_inbound_key, pending_inbound_payments: Mutex::new(pending_inbound_payments), pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()), + + outbound_scid_aliases: Mutex::new(outbound_scid_aliases), fake_scid_rand_bytes: fake_scid_rand_bytes.unwrap(), our_network_key, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index ed220a518..598a51b0b 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5916,7 +5916,7 @@ fn bolt2_open_channel_sending_node_checks_part1() { //This test needs to be on i let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - //Force duplicate channel ids + // Force duplicate randomness for every get-random call for node in nodes.iter() { *node.keys_manager.override_random_bytes.lock().unwrap() = Some([0; 32]); } @@ -5929,7 +5929,8 @@ fn bolt2_open_channel_sending_node_checks_part1() { //This test needs to be on i nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &node0_to_1_send_open_channel); get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()); - //Create a second channel with a channel_id collision + // Create a second channel with the same random values. This used to panic due to a colliding + // channel_id, but now panics due to a colliding outbound SCID alias. assert!(nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), channel_value_satoshis, push_msat, 42, None).is_err()); } @@ -7179,7 +7180,10 @@ fn test_user_configurable_csv_delay() { let nodes = create_network(2, &node_cfgs, &node_chanmgrs); // We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound() - if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), 1000000, 1000000, 0, &low_our_to_self_config, 0) { + if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, + &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), 1000000, 1000000, 0, + &low_our_to_self_config, 0, 42) + { match error { APIError::APIMisuseError { err } => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7190,7 +7194,10 @@ fn test_user_configurable_csv_delay() { nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap(); let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id()); open_channel.to_self_delay = 200; - if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, &low_our_to_self_config, 0, &nodes[0].logger) { + if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, + &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, + &low_our_to_self_config, 0, &nodes[0].logger, 42) + { match error { ChannelError::Close(err) => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7219,7 +7226,10 @@ fn test_user_configurable_csv_delay() { nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap(); let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id()); open_channel.to_self_delay = 200; - if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, &high_their_to_self_config, 0, &nodes[0].logger) { + if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, + &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0, + &high_their_to_self_config, 0, &nodes[0].logger, 42) + { match error { ChannelError::Close(err) => { assert!(regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index d67abeefd..af82a275d 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -319,10 +319,10 @@ fn test_onion_failure() { let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config), Some(config), Some(node_2_cfg)]); let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())]; for node in nodes.iter() { *node.keys_manager.override_random_bytes.lock().unwrap() = Some([3; 32]); } - let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())]; let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 40000); // positive case send_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 40000); diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index f8a1a70e2..4fadfc915 100644 --- a/lightning/src/ln/priv_short_conf_tests.rs +++ b/lightning/src/ln/priv_short_conf_tests.rs @@ -237,3 +237,37 @@ fn test_1_conf_open() { do_test_1_conf_open(ConnectStyle::TransactionsFirst); do_test_1_conf_open(ConnectStyle::FullBlockViaListen); } + +#[test] +fn test_routed_scid_alias() { + // Trivially test sending a payment which is routed through an SCID alias. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let mut no_announce_cfg = test_default_channel_config(); + no_announce_cfg.accept_forwards_to_priv_channels = true; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(no_announce_cfg), None]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 500_000_000, InitFeatures::known(), InitFeatures::known()).2; + create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 500_000_000, InitFeatures::known(), InitFeatures::known()); + + let last_hop = nodes[2].node.list_usable_channels(); + let hop_hints = vec![RouteHint(vec![RouteHintHop { + src_node_id: nodes[1].node.get_our_node_id(), + short_channel_id: last_hop[0].inbound_scid_alias.unwrap(), + fees: RoutingFees { + base_msat: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_base_msat, + proportional_millionths: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_proportional_millionths, + }, + cltv_expiry_delta: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().cltv_expiry_delta, + htlc_maximum_msat: None, + htlc_minimum_msat: None, + }])]; + let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], hop_hints, 100_000, 42); + assert_eq!(route.paths[0][1].short_channel_id, last_hop[0].inbound_scid_alias.unwrap()); + nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + + pass_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], 100_000, payment_hash, payment_secret); + claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); +} diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 6c39efb87..8eb39cfe9 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -201,7 +201,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_ let channel_state = nodes[0].node.channel_state.lock().unwrap(); assert_eq!(channel_state.by_id.len(), 1); - assert_eq!(channel_state.short_to_id.len(), 1); + assert_eq!(channel_state.short_to_id.len(), 2); mem::drop(channel_state); if !reorg_after_reload { diff --git a/lightning/src/util/scid_utils.rs b/lightning/src/util/scid_utils.rs index 11b8d0c95..8552358c3 100644 --- a/lightning/src/util/scid_utils.rs +++ b/lightning/src/util/scid_utils.rs @@ -61,7 +61,7 @@ pub fn scid_from_parts(block: u64, tx_index: u64, vout_index: u64) -> Result(&self, highest_seen_blockheight: u32, genesis_hash: &BlockHash, fake_scid_rand_bytes: &[u8; 32], keys_manager: &K) -> u64 + pub(crate) fn get_fake_scid(&self, highest_seen_blockheight: u32, genesis_hash: &BlockHash, fake_scid_rand_bytes: &[u8; 32], keys_manager: &K) -> u64 where K::Target: KeysInterface, { // Ensure we haven't created a namespace that doesn't fit into the 3 bits we've allocated for @@ -138,13 +138,6 @@ pub(crate) mod fake_scid { } } - pub fn get_phantom_scid(fake_scid_rand_bytes: &[u8; 32], highest_seen_blockheight: u32, genesis_hash: &BlockHash, keys_manager: &K) -> u64 - where K::Target: KeysInterface, - { - let namespace = Namespace::Phantom; - namespace.get_fake_scid(highest_seen_blockheight, genesis_hash, fake_scid_rand_bytes, keys_manager) - } - fn segwit_activation_height(genesis: &BlockHash) -> u32 { const MAINNET_GENESIS_STR: &'static str = "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"; if BlockHash::from_hex(MAINNET_GENESIS_STR).unwrap() == *genesis { -- 2.39.5