From c578e4a346efb4361b53d18525bdb74de07cbae8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 20:16:06 -0400 Subject: [PATCH 01/10] Add ChannelManager-specific HandleError type and macro to use it Original macro is from Antoine Riard , the error type and additional mappings are from Matt Corallo --- src/ln/channelmanager.rs | 58 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 956a08c98db..f12c3f7ac4a 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -120,6 +120,32 @@ enum PendingOutboundHTLC { } } +struct MsgHandleErrInternal { + err: msgs::HandleError, + needs_channel_force_close: bool, +} +impl MsgHandleErrInternal { + #[inline] + fn send_err_msg_no_close(err: &'static str, channel_id: [u8; 32]) -> Self { + Self { + err: HandleError { + err, + action: Some(msgs::ErrorAction::SendErrorMessage { + msg: msgs::ErrorMessage { + channel_id, + data: err.to_string() + }, + }), + }, + needs_channel_force_close: false, + } + } + #[inline] + fn from_no_close(err: msgs::HandleError) -> Self { + Self { err, needs_channel_force_close: false } + } +} + /// We hold back HTLCs we intend to relay for a random interval in the range (this, 5*this). This /// provides some limited amount of privacy. Ideally this would range from somewhere like 1 second /// to 30 seconds, but people expect lightning to be, you know, kinda fast, sadly. We could @@ -1483,6 +1509,38 @@ impl ChainListener for ChannelManager { } } +macro_rules! handle_error { + ($self: ident, $internal: expr, $their_node_id: expr) => { + match $internal { + Ok(msg) => Ok(msg), + Err(MsgHandleErrInternal { err, needs_channel_force_close }) => { + if needs_channel_force_close { + match &err.action { + &Some(msgs::ErrorAction::DisconnectPeer { msg: Some(ref msg) }) => { + if msg.channel_id == [0; 32] { + $self.peer_disconnected(&$their_node_id, true); + } else { + $self.force_close_channel(&msg.channel_id); + } + }, + &Some(msgs::ErrorAction::DisconnectPeer { msg: None }) => {}, + &Some(msgs::ErrorAction::IgnoreError) => {}, + &Some(msgs::ErrorAction::SendErrorMessage { ref msg }) => { + if msg.channel_id == [0; 32] { + $self.peer_disconnected(&$their_node_id, true); + } else { + $self.force_close_channel(&msg.channel_id); + } + }, + &None => {}, + } + } + Err(err) + }, + } + } +} + impl ChannelMessageHandler for ChannelManager { //TODO: Handle errors and close channel (or so) fn handle_open_channel(&self, their_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result { From 4afbc125688abd0195929994cfa161c7dfb12b0a Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 4 Sep 2018 20:17:45 -0400 Subject: [PATCH 02/10] Refactor handle_open_channel to wrapper error handling function Original version is from Antoine Riard , the error type and some return type fixes are from Matt Corallo --- src/ln/channelmanager.rs | 68 +++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index f12c3f7ac4a..845bced7590 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -1383,6 +1383,41 @@ impl ChannelManager { pub fn test_restore_channel_monitor(&self) { unimplemented!(); } + + fn internal_open_channel(&self, their_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result { + if msg.chain_hash != self.genesis_hash { + return Err(MsgHandleErrInternal::send_err_msg_no_close("Unknown genesis block hash", msg.temporary_channel_id.clone())); + } + let mut channel_state = self.channel_state.lock().unwrap(); + if channel_state.by_id.contains_key(&msg.temporary_channel_id) { + return Err(MsgHandleErrInternal::send_err_msg_no_close("temporary_channel_id collision!", msg.temporary_channel_id.clone())); + } + + let chan_keys = if cfg!(feature = "fuzztarget") { + ChannelKeys { + funding_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0]).unwrap(), + revocation_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0]).unwrap(), + payment_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0]).unwrap(), + delayed_payment_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0]).unwrap(), + htlc_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0]).unwrap(), + channel_close_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0]).unwrap(), + channel_monitor_claim_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 0]).unwrap(), + commitment_seed: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], + } + } else { + let mut key_seed = [0u8; 32]; + rng::fill_bytes(&mut key_seed); + match ChannelKeys::new_from_seed(&key_seed) { + Ok(key) => key, + Err(_) => panic!("RNG is busted!") + } + }; + + let channel = Channel::new_from_req(&*self.fee_estimator, chan_keys, their_node_id.clone(), msg, 0, false, self.announce_channels_publicly, Arc::clone(&self.logger)).map_err(|e| MsgHandleErrInternal::from_no_close(e))?; + let accept_msg = channel.get_accept_channel().map_err(|e| MsgHandleErrInternal::from_no_close(e))?; + channel_state.by_id.insert(channel.channel_id(), channel); + Ok(accept_msg) + } } impl events::EventsProvider for ChannelManager { @@ -1544,38 +1579,7 @@ macro_rules! handle_error { impl ChannelMessageHandler for ChannelManager { //TODO: Handle errors and close channel (or so) fn handle_open_channel(&self, their_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result { - if msg.chain_hash != self.genesis_hash { - return Err(HandleError{err: "Unknown genesis block hash", action: None}); - } - let mut channel_state = self.channel_state.lock().unwrap(); - if channel_state.by_id.contains_key(&msg.temporary_channel_id) { - return Err(HandleError{err: "temporary_channel_id collision!", action: None}); - } - - let chan_keys = if cfg!(feature = "fuzztarget") { - ChannelKeys { - funding_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0]).unwrap(), - revocation_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 0]).unwrap(), - payment_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0]).unwrap(), - delayed_payment_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0]).unwrap(), - htlc_base_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0]).unwrap(), - channel_close_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0]).unwrap(), - channel_monitor_claim_key: SecretKey::from_slice(&self.secp_ctx, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, 0]).unwrap(), - commitment_seed: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], - } - } else { - let mut key_seed = [0u8; 32]; - rng::fill_bytes(&mut key_seed); - match ChannelKeys::new_from_seed(&key_seed) { - Ok(key) => key, - Err(_) => panic!("RNG is busted!") - } - }; - - let channel = Channel::new_from_req(&*self.fee_estimator, chan_keys, their_node_id.clone(), msg, 0, false, self.announce_channels_publicly, Arc::clone(&self.logger))?; - let accept_msg = channel.get_accept_channel()?; - channel_state.by_id.insert(channel.channel_id(), channel); - Ok(accept_msg) + handle_error!(self, self.internal_open_channel(their_node_id, msg), their_node_id) } fn handle_accept_channel(&self, their_node_id: &PublicKey, msg: &msgs::AcceptChannel) -> Result<(), HandleError> { From 8c709d1b6f8bd03d2982b9f94f936a61522f6e4b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 19:28:06 -0400 Subject: [PATCH 03/10] Dont return an Err from Channel::get_accept_channel that can't fail --- fuzz/fuzz_targets/channel_target.rs | 2 +- src/ln/channel.rs | 6 +++--- src/ln/channelmanager.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/fuzz/fuzz_targets/channel_target.rs b/fuzz/fuzz_targets/channel_target.rs index 37a67183696..7b3b7df1ce2 100644 --- a/fuzz/fuzz_targets/channel_target.rs +++ b/fuzz/fuzz_targets/channel_target.rs @@ -230,7 +230,7 @@ pub fn do_test(data: &[u8]) { Ok(chan) => chan, Err(_) => return, }; - chan.get_accept_channel().unwrap(); + chan.get_accept_channel(); tx.output.push(TxOut{ value: open_chan.funding_satoshis, script_pubkey: chan.get_funding_redeemscript().to_v0_p2wsh() }); let funding_output = OutPoint::new(Sha256dHash::from_data(&serialize(&tx).unwrap()[..]), 0); diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 0659f8261af..8dcd23b785e 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -2200,7 +2200,7 @@ impl Channel { }) } - pub fn get_accept_channel(&self) -> Result { + pub fn get_accept_channel(&self) -> msgs::AcceptChannel { if self.channel_outbound { panic!("Tried to send accept_channel for an outbound channel?"); } @@ -2213,7 +2213,7 @@ impl Channel { let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); - Ok(msgs::AcceptChannel { + msgs::AcceptChannel { temporary_channel_id: self.channel_id, dust_limit_satoshis: self.our_dust_limit_satoshis, max_htlc_value_in_flight_msat: Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis), @@ -2229,7 +2229,7 @@ impl Channel { htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key), first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret), shutdown_scriptpubkey: None, - }) + } } fn get_outbound_funding_created_signature(&mut self) -> Result<(Signature, Transaction), HandleError> { diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 845bced7590..ff1f6e53dae 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -1414,7 +1414,7 @@ impl ChannelManager { }; let channel = Channel::new_from_req(&*self.fee_estimator, chan_keys, their_node_id.clone(), msg, 0, false, self.announce_channels_publicly, Arc::clone(&self.logger)).map_err(|e| MsgHandleErrInternal::from_no_close(e))?; - let accept_msg = channel.get_accept_channel().map_err(|e| MsgHandleErrInternal::from_no_close(e))?; + let accept_msg = channel.get_accept_channel(); channel_state.by_id.insert(channel.channel_id(), channel); Ok(accept_msg) } From f60b5d971c3b89af9671c8fbaee61c55fdc79a0c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 20:10:32 -0400 Subject: [PATCH 04/10] Ensure Channel::new_from_req always returns an ErrorMessage on Err --- src/ln/channel.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 8dcd23b785e..e9ba79f0b4d 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -525,10 +525,10 @@ impl Channel { let their_announce = if (msg.channel_flags & 1) == 1 { true } else { false }; if require_announce && !their_announce { - return Err(HandleError{err: "Peer tried to open unannounced channel, but we require public ones", action: Some(msgs::ErrorAction::IgnoreError) }); + return_error_message!("Peer tried to open unannounced channel, but we require public ones"); } if !allow_announce && their_announce { - return Err(HandleError{err: "Peer tried to open announced channel, but we require private ones", action: Some(msgs::ErrorAction::IgnoreError) }); + return_error_message!("Peer tried to open announced channel, but we require private ones"); } let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); From 2202d139c16b7facb100d8da360c375a46544f43 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 20:00:47 -0400 Subject: [PATCH 05/10] Simplify secp_call! macro in ChannelManager --- src/ln/channelmanager.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index ff1f6e53dae..4c588ddbc81 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -215,10 +215,10 @@ pub struct ChannelManager { const CLTV_EXPIRY_DELTA: u16 = 6 * 24 * 2; //TODO? macro_rules! secp_call { - ( $res: expr, $err_msg: expr, $action: expr ) => { + ( $res: expr, $err: expr ) => { match $res { Ok(key) => key, - Err(_) => return Err(HandleError{err: $err_msg, action: Some($action)}) + Err(_) => return Err($err), } }; } @@ -940,7 +940,8 @@ impl ChannelManager { //TODO: This should return something other than HandleError, that's really intended for //p2p-returns only. - let onion_keys = secp_call!(ChannelManager::construct_onion_keys(&self.secp_ctx, &route, &session_priv), "Pubkey along hop was maliciously selected", msgs::ErrorAction::IgnoreError); + let onion_keys = secp_call!(ChannelManager::construct_onion_keys(&self.secp_ctx, &route, &session_priv), + HandleError{err: "Pubkey along hop was maliciously selected", action: Some(msgs::ErrorAction::IgnoreError)}); let (onion_payloads, htlc_msat, htlc_cltv) = ChannelManager::build_onion_payloads(&route, cur_height)?; let onion_packet = ChannelManager::construct_onion_packet(onion_payloads, onion_keys, &payment_hash)?; @@ -2056,9 +2057,9 @@ impl ChannelMessageHandler for ChannelManager { let were_node_one = announcement.node_id_1 == our_node_id; let msghash = Message::from_slice(&Sha256dHash::from_data(&announcement.encode()[..])[..]).unwrap(); - let bad_sig_action = msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id: msg.channel_id.clone(), data: "Invalid signature in announcement_signatures".to_string() } }; - secp_call!(self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }), "Bad announcement_signatures node_signature", bad_sig_action); - secp_call!(self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }), "Bad announcement_signatures bitcoin_signature", bad_sig_action); + let bad_sig_action = msgs::HandleError {err: "Invalid signature in announcement_signatures", action: msgs::ErrorAction::SendErrorMessage {msg: msgs::ErrorMessage {channel_id: msg.channel_id.clone(), data: "Invalid signature in announcement_signatures".to_string()}}}; + secp_call!(self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }), bad_sig_action); + secp_call!(self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }), bad_sig_action); let our_node_sig = self.secp_ctx.sign(&msghash, &self.our_network_key); From 7a234b63857297a074f0268f677aaccaac08ade1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 20:02:33 -0400 Subject: [PATCH 06/10] Fill out IgnoreError actions in get_channel_announcement They are all just "its too early/late to get an announcement" errors so simply ignoring them and not sending an announce is fine --- src/ln/channel.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index e9ba79f0b4d..14ab80c90eb 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -2298,13 +2298,13 @@ impl Channel { /// https://github.com/lightningnetwork/lightning-rfc/issues/468 pub fn get_channel_announcement(&self, our_node_id: PublicKey, chain_hash: Sha256dHash) -> Result<(msgs::UnsignedChannelAnnouncement, Signature), HandleError> { if !self.announce_publicly { - return Err(HandleError{err: "Channel is not available for public announcements", action: None}); + return Err(HandleError{err: "Channel is not available for public announcements", action: Some(msgs::ErrorAction::IgnoreError)}); } if self.channel_state & (ChannelState::ChannelFunded as u32) == 0 { - return Err(HandleError{err: "Cannot get a ChannelAnnouncement until the channel funding has been locked", action: None}); + return Err(HandleError{err: "Cannot get a ChannelAnnouncement until the channel funding has been locked", action: Some(msgs::ErrorAction::IgnoreError)}); } if (self.channel_state & (ChannelState::LocalShutdownSent as u32 | ChannelState::ShutdownComplete as u32)) != 0 { - return Err(HandleError{err: "Cannot get a ChannelAnnouncement once the channel is closing", action: None}); + return Err(HandleError{err: "Cannot get a ChannelAnnouncement once the channel is closing", action: Some(msgs::ErrorAction::IgnoreError)}); } let were_node_one = our_node_id.serialize()[..] < self.their_node_id.serialize()[..]; From a71abac55a07c6802fc6acd6fc934b2813e2a786 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 20:07:29 -0400 Subject: [PATCH 07/10] Move announcement_signatures handling into new force-close macro Because we've separated out channel closure from ErrorMessage returning we can return error messages in a few additional cases, like if the peer sent us a message for a channel they didn't own. --- src/ln/channelmanager.rs | 98 +++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 36 deletions(-) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 4c588ddbc81..e365b048a20 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -141,6 +141,25 @@ impl MsgHandleErrInternal { } } #[inline] + fn send_err_msg_close_chan(err: &'static str, channel_id: [u8; 32]) -> Self { + Self { + err: HandleError { + err, + action: Some(msgs::ErrorAction::SendErrorMessage { + msg: msgs::ErrorMessage { + channel_id, + data: err.to_string() + }, + }), + }, + needs_channel_force_close: true, + } + } + #[inline] + fn from_maybe_close(err: msgs::HandleError) -> Self { + Self { err, needs_channel_force_close: true } + } + #[inline] fn from_no_close(err: msgs::HandleError) -> Self { Self { err, needs_channel_force_close: false } } @@ -1419,6 +1438,48 @@ impl ChannelManager { channel_state.by_id.insert(channel.channel_id(), channel); Ok(accept_msg) } + + fn internal_announcement_signatures(&self, their_node_id: &PublicKey, msg: &msgs::AnnouncementSignatures) -> Result<(), MsgHandleErrInternal> { + let (chan_announcement, chan_update) = { + let mut channel_state = self.channel_state.lock().unwrap(); + match channel_state.by_id.get_mut(&msg.channel_id) { + Some(chan) => { + if chan.get_their_node_id() != *their_node_id { + return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id)); + } + if !chan.is_usable() { + return Err(MsgHandleErrInternal::from_no_close(HandleError{err: "Got an announcement_signatures before we were ready for it", action: Some(msgs::ErrorAction::IgnoreError)})); + } + + let our_node_id = self.get_our_node_id(); + let (announcement, our_bitcoin_sig) = chan.get_channel_announcement(our_node_id.clone(), self.genesis_hash.clone()) + .map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?; + + let were_node_one = announcement.node_id_1 == our_node_id; + let msghash = Message::from_slice(&Sha256dHash::from_data(&announcement.encode()[..])[..]).unwrap(); + let bad_sig_action = MsgHandleErrInternal::send_err_msg_close_chan("Bad announcement_signatures node_signature", msg.channel_id); + secp_call!(self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }), bad_sig_action); + secp_call!(self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }), bad_sig_action); + + let our_node_sig = self.secp_ctx.sign(&msghash, &self.our_network_key); + + (msgs::ChannelAnnouncement { + node_signature_1: if were_node_one { our_node_sig } else { msg.node_signature }, + node_signature_2: if were_node_one { msg.node_signature } else { our_node_sig }, + bitcoin_signature_1: if were_node_one { our_bitcoin_sig } else { msg.bitcoin_signature }, + bitcoin_signature_2: if were_node_one { msg.bitcoin_signature } else { our_bitcoin_sig }, + contents: announcement, + }, self.get_channel_update(chan).unwrap()) // can only fail if we're not in a ready state + }, + None => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id)) + } + }; + let mut pending_events = self.pending_events.lock().unwrap(); + pending_events.push(events::Event::BroadcastChannelAnnouncement { msg: chan_announcement, update_msg: chan_update }); + Ok(()) + } + + } impl events::EventsProvider for ChannelManager { @@ -2041,42 +2102,7 @@ impl ChannelMessageHandler for ChannelManager { } fn handle_announcement_signatures(&self, their_node_id: &PublicKey, msg: &msgs::AnnouncementSignatures) -> Result<(), HandleError> { - let (chan_announcement, chan_update) = { - let mut channel_state = self.channel_state.lock().unwrap(); - match channel_state.by_id.get_mut(&msg.channel_id) { - Some(chan) => { - if chan.get_their_node_id() != *their_node_id { - return Err(HandleError{err: "Got a message for a channel from the wrong node!", action: Some(msgs::ErrorAction::IgnoreError) }) - } - if !chan.is_usable() { - return Err(HandleError{err: "Got an announcement_signatures before we were ready for it", action: Some(msgs::ErrorAction::IgnoreError) }); - } - - let our_node_id = self.get_our_node_id(); - let (announcement, our_bitcoin_sig) = chan.get_channel_announcement(our_node_id.clone(), self.genesis_hash.clone())?; - - let were_node_one = announcement.node_id_1 == our_node_id; - let msghash = Message::from_slice(&Sha256dHash::from_data(&announcement.encode()[..])[..]).unwrap(); - let bad_sig_action = msgs::HandleError {err: "Invalid signature in announcement_signatures", action: msgs::ErrorAction::SendErrorMessage {msg: msgs::ErrorMessage {channel_id: msg.channel_id.clone(), data: "Invalid signature in announcement_signatures".to_string()}}}; - secp_call!(self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }), bad_sig_action); - secp_call!(self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }), bad_sig_action); - - let our_node_sig = self.secp_ctx.sign(&msghash, &self.our_network_key); - - (msgs::ChannelAnnouncement { - node_signature_1: if were_node_one { our_node_sig } else { msg.node_signature }, - node_signature_2: if were_node_one { msg.node_signature } else { our_node_sig }, - bitcoin_signature_1: if were_node_one { our_bitcoin_sig } else { msg.bitcoin_signature }, - bitcoin_signature_2: if were_node_one { msg.bitcoin_signature } else { our_bitcoin_sig }, - contents: announcement, - }, self.get_channel_update(chan).unwrap()) // can only fail if we're not in a ready state - }, - None => return Err(HandleError{err: "Failed to find corresponding channel", action: Some(msgs::ErrorAction::IgnoreError)}) - } - }; - let mut pending_events = self.pending_events.lock().unwrap(); - pending_events.push(events::Event::BroadcastChannelAnnouncement { msg: chan_announcement, update_msg: chan_update }); - Ok(()) + handle_error!(self, self.internal_announcement_signatures(their_node_id, msg), their_node_id) } fn peer_disconnected(&self, their_node_id: &PublicKey, no_connection_possible: bool) { From 91b23a075442107a93d00c6db1d7f5ffe54f715b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 22:24:46 -0400 Subject: [PATCH 08/10] Reject rumors of channels that are from one node back to itself --- src/ln/channelmanager.rs | 8 ++++---- src/ln/router.rs | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index e365b048a20..50899783384 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -3355,9 +3355,9 @@ mod tests { chain_hash: genesis_block(Network::Testnet).header.bitcoin_hash(), short_channel_id: as_chan.get_short_channel_id().unwrap(), node_id_1: if were_node_one { as_network_key } else { bs_network_key }, - node_id_2: if !were_node_one { bs_network_key } else { as_network_key }, + node_id_2: if were_node_one { bs_network_key } else { as_network_key }, bitcoin_key_1: if were_node_one { as_bitcoin_key } else { bs_bitcoin_key }, - bitcoin_key_2: if !were_node_one { bs_bitcoin_key } else { as_bitcoin_key }, + bitcoin_key_2: if were_node_one { bs_bitcoin_key } else { as_bitcoin_key }, excess_data: Vec::new(), }; } @@ -3372,9 +3372,9 @@ mod tests { let bs_node_sig = secp_ctx.sign(&msghash, &nodes[1].node.our_network_key); chan_announcement = msgs::ChannelAnnouncement { node_signature_1 : if were_node_one { as_node_sig } else { bs_node_sig}, - node_signature_2 : if !were_node_one { bs_node_sig } else { as_node_sig}, + node_signature_2 : if were_node_one { bs_node_sig } else { as_node_sig}, bitcoin_signature_1: if were_node_one { as_bitcoin_sig } else { bs_bitcoin_sig }, - bitcoin_signature_2 : if !were_node_one { bs_bitcoin_sig } else { as_bitcoin_sig }, + bitcoin_signature_2 : if were_node_one { bs_bitcoin_sig } else { as_bitcoin_sig }, contents: $unsigned_msg } } diff --git a/src/ln/router.rs b/src/ln/router.rs index e3697147f0a..6bc319d59b8 100644 --- a/src/ln/router.rs +++ b/src/ln/router.rs @@ -199,6 +199,10 @@ impl RoutingMessageHandler for Router { } fn handle_channel_announcement(&self, msg: &msgs::ChannelAnnouncement) -> Result { + if msg.contents.node_id_1 == msg.contents.node_id_2 || msg.contents.bitcoin_key_1 == msg.contents.bitcoin_key_2 { + return Err(HandleError{err: "Channel announcement node had a channel with itself", action: Some(ErrorAction::IgnoreError)}); + } + let msg_hash = Message::from_slice(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]).unwrap(); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_2, &msg.contents.node_id_2); From 3b4c1a366216d714cbcbeb483f6b08b277bde6c7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 22:25:51 -0400 Subject: [PATCH 09/10] Util-func channel removal (fixing a bug in HTLC failure updates) --- src/ln/router.rs | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/ln/router.rs b/src/ln/router.rs index 6bc319d59b8..e91e0c2acd8 100644 --- a/src/ln/router.rs +++ b/src/ln/router.rs @@ -306,12 +306,7 @@ impl RoutingMessageHandler for Router { &msgs::HTLCFailChannelUpdate::ChannelClosed { ref short_channel_id } => { let mut network = self.network_map.write().unwrap(); if let Some(chan) = network.channels.remove(short_channel_id) { - network.nodes.get_mut(&chan.one_to_two.src_node_id).unwrap().channels.retain(|chan_id| { - chan_id != NetworkMap::get_short_id(chan_id) - }); - network.nodes.get_mut(&chan.two_to_one.src_node_id).unwrap().channels.retain(|chan_id| { - chan_id != NetworkMap::get_short_id(chan_id) - }); + Self::remove_channel_in_nodes(&mut network.nodes, &chan, *short_channel_id); } }, } @@ -462,6 +457,25 @@ impl Router { unimplemented!(); } + fn remove_channel_in_nodes(nodes: &mut HashMap, chan: &ChannelInfo, short_channel_id: u64) { + macro_rules! remove_from_node { + ($node_id: expr) => { + if let Entry::Occupied(mut entry) = nodes.entry($node_id) { + entry.get_mut().channels.retain(|chan_id| { + short_channel_id != *NetworkMap::get_short_id(chan_id) + }); + if entry.get().channels.is_empty() { + entry.remove_entry(); + } + } else { + panic!("Had channel that pointed to unknown node (ie inconsistent network map)!"); + } + } + } + remove_from_node!(chan.one_to_two.src_node_id); + remove_from_node!(chan.two_to_one.src_node_id); + } + /// Gets a route from us to the given target node. /// Extra routing hops between known nodes and the target will be used if they are included in /// last_hops. From 227c1d21bcc81513cea3cde9ee7fe2995d98e2dc Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 4 Sep 2018 22:39:04 -0400 Subject: [PATCH 10/10] Handle partial-response UTXO impls or reorgs in chan_announcements Mostly to add a big comment noting why we aren't "spec-compliant" --- src/ln/router.rs | 86 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/src/ln/router.rs b/src/ln/router.rs index e91e0c2acd8..6b20d541e27 100644 --- a/src/ln/router.rs +++ b/src/ln/router.rs @@ -102,7 +102,21 @@ struct NetworkMap { our_node_id: PublicKey, nodes: HashMap, } - +struct MutNetworkMap<'a> { + #[cfg(feature = "non_bitcoin_chain_hash_routing")] + channels: &'a mut HashMap<(u64, Sha256dHash), ChannelInfo>, + #[cfg(not(feature = "non_bitcoin_chain_hash_routing"))] + channels: &'a mut HashMap, + nodes: &'a mut HashMap, +} +impl NetworkMap { + fn borrow_parts(&mut self) -> MutNetworkMap { + MutNetworkMap { + channels: &mut self.channels, + nodes: &mut self.nodes, + } + } +} impl std::fmt::Display for NetworkMap { fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> { write!(f, "Node id {} network map\n[Channels]\n", log_pubkey!(self.our_node_id))?; @@ -213,7 +227,7 @@ impl RoutingMessageHandler for Router { panic!("Unknown-required-features ChannelAnnouncements should never deserialize!"); } - match self.chain_monitor.get_chain_utxo(msg.contents.chain_hash, msg.contents.short_channel_id) { + let checked_utxo = match self.chain_monitor.get_chain_utxo(msg.contents.chain_hash, msg.contents.short_channel_id) { Ok((script_pubkey, _value)) => { let expected_script = Builder::new().push_opcode(opcodes::All::OP_PUSHNUM_2) .push_slice(&msg.contents.bitcoin_key_1.serialize()) @@ -224,9 +238,11 @@ impl RoutingMessageHandler for Router { } //TODO: Check if value is worth storing, use it to inform routing, and compare it //to the new HTLC max field in channel_update + true }, Err(ChainError::NotSupported) => { // Tentatively accept, potentially exposing us to DoS attacks + false }, Err(ChainError::NotWatched) => { return Err(HandleError{err: "Channel announced on an unknown chain", action: Some(ErrorAction::IgnoreError)}); @@ -234,39 +250,55 @@ impl RoutingMessageHandler for Router { Err(ChainError::UnknownTx) => { return Err(HandleError{err: "Channel announced without corresponding UTXO entry", action: Some(ErrorAction::IgnoreError)}); }, - } + }; - let mut network = self.network_map.write().unwrap(); + let mut network_lock = self.network_map.write().unwrap(); + let network = network_lock.borrow_parts(); + + let chan_info = ChannelInfo { + features: msg.contents.features.clone(), + one_to_two: DirectionalChannelInfo { + src_node_id: msg.contents.node_id_1.clone(), + last_update: 0, + enabled: false, + cltv_expiry_delta: u16::max_value(), + htlc_minimum_msat: u64::max_value(), + fee_base_msat: u32::max_value(), + fee_proportional_millionths: u32::max_value(), + }, + two_to_one: DirectionalChannelInfo { + src_node_id: msg.contents.node_id_2.clone(), + last_update: 0, + enabled: false, + cltv_expiry_delta: u16::max_value(), + htlc_minimum_msat: u64::max_value(), + fee_base_msat: u32::max_value(), + fee_proportional_millionths: u32::max_value(), + } + }; match network.channels.entry(NetworkMap::get_key(msg.contents.short_channel_id, msg.contents.chain_hash)) { - Entry::Occupied(_) => { + Entry::Occupied(mut entry) => { //TODO: because asking the blockchain if short_channel_id is valid is only optional //in the blockchain API, we need to handle it smartly here, though its unclear //exactly how... - return Err(HandleError{err: "Already have knowledge of channel", action: Some(ErrorAction::IgnoreError)}) + if checked_utxo { + // Either our UTXO provider is busted, there was a reorg, or the UTXO provider + // only sometimes returns results. In any case remove the previous entry. Note + // that the spec expects us to "blacklist" the node_ids involved, but we can't + // do that because + // a) we don't *require* a UTXO provider that always returns results. + // b) we don't track UTXOs of channels we know about and remove them if they + // get reorg'd out. + // c) it's unclear how to do so without exposing ourselves to massive DoS risk. + Self::remove_channel_in_nodes(network.nodes, &entry.get(), msg.contents.short_channel_id); + *entry.get_mut() = chan_info; + } else { + return Err(HandleError{err: "Already have knowledge of channel", action: Some(ErrorAction::IgnoreError)}) + } }, Entry::Vacant(entry) => { - entry.insert(ChannelInfo { - features: msg.contents.features.clone(), - one_to_two: DirectionalChannelInfo { - src_node_id: msg.contents.node_id_1.clone(), - last_update: 0, - enabled: false, - cltv_expiry_delta: u16::max_value(), - htlc_minimum_msat: u64::max_value(), - fee_base_msat: u32::max_value(), - fee_proportional_millionths: u32::max_value(), - }, - two_to_one: DirectionalChannelInfo { - src_node_id: msg.contents.node_id_2.clone(), - last_update: 0, - enabled: false, - cltv_expiry_delta: u16::max_value(), - htlc_minimum_msat: u64::max_value(), - fee_base_msat: u32::max_value(), - fee_proportional_millionths: u32::max_value(), - } - }); + entry.insert(chan_info); } };