Skip to content

Refactor ChannelManager handle functions into a Channel-closing macro #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 5, 2018
Merged

Refactor ChannelManager handle functions into a Channel-closing macro #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c578e4a
Add ChannelManager-specific HandleError type and macro to use it
TheBlueMatt Sep 5, 2018
4afbc12
Refactor handle_open_channel to wrapper error handling function
Sep 5, 2018
8c709d1
Dont return an Err from Channel::get_accept_channel that can't fail
TheBlueMatt Sep 4, 2018
f60b5d9
Ensure Channel::new_from_req always returns an ErrorMessage on Err
TheBlueMatt Sep 5, 2018
2202d13
Simplify secp_call! macro in ChannelManager
TheBlueMatt Sep 5, 2018
7a234b6
Fill out IgnoreError actions in get_channel_announcement
TheBlueMatt Sep 5, 2018
a71abac
Move announcement_signatures handling into new force-close macro
TheBlueMatt Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fuzz/fuzz_targets/channel_target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 8 additions & 8 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -2200,7 +2200,7 @@ impl Channel {
})
}

pub fn get_accept_channel(&self) -> Result<msgs::AcceptChannel, HandleError> {
pub fn get_accept_channel(&self) -> msgs::AcceptChannel {
if self.channel_outbound {
panic!("Tried to send accept_channel for an outbound channel?");
}
Expand All @@ -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),
Expand All @@ -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> {
Expand Down Expand Up @@ -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()[..];
Expand Down
231 changes: 160 additions & 71 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,51 @@ enum PendingOutboundHTLC {
}
}

struct MsgHandleErrInternal {
err: msgs::HandleError,
needs_channel_force_close: bool,
}
impl MsgHandleErrInternal {
Copy link

@ariard ariard Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm having a builder is great to simplify errors filling but I think it's force us to specify a method for each case as a71abac shows it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe, though I also don't want to have ErrBuilder::new().msg().action().needs_close() every time we want to return an Err cause then we spend all our charachters on a line on error handling :/. Not sure if there's a great solution here, really, but I'm open to patches to play around with it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, c-lightning doesn't have a concept of general handle error like us.
Still not delighted by this design, but don't see better for now so will go ahead with this for channel refactoring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, well nothing stopping us from cleaning up things with macros or flattening errors in a future PR.

#[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 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 }
}
}

/// 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
Expand Down Expand Up @@ -189,10 +234,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),
}
};
}
Expand Down Expand Up @@ -914,7 +959,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)?;

Expand Down Expand Up @@ -1357,6 +1403,83 @@ impl ChannelManager {
pub fn test_restore_channel_monitor(&self) {
unimplemented!();
}

fn internal_open_channel(&self, their_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<msgs::AcceptChannel, MsgHandleErrInternal> {
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();
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 {
Expand Down Expand Up @@ -1483,41 +1606,42 @@ 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<msgs::AcceptChannel, HandleError> {
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> {
Expand Down Expand Up @@ -1978,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::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 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) {
Expand Down