Skip to content

Implement KeyError for bad key in channel announcement process #99

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

Closed
wants to merge 1 commit into from
Closed

Implement KeyError for bad key in channel announcement process #99

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 10 additions & 6 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ macro_rules! secp_call {
match $res {
Ok(key) => key,
//TODO: Make the err a parameter!
Err(_) => return Err(HandleError{err: "Key error", action: None})
Err(_) => return Err(HandleError{err: "Key error", action: None}),
}
};
}
Expand Down Expand Up @@ -1370,7 +1370,7 @@ impl ChainListener for ChannelManager {
Ok(res) => res,
Err(e) => {
log_error!(self, "Got error handling message: {}!", e.err);
//TODO: push e on events and blow up the channel (it has bad keys)
//TODO: handle WaitNewState or IgnoreMessage
Copy link
Author

Choose a reason for hiding this comment

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

We don't need KeyError anymore here ! Just to fulfill the downstream HandleError with some WaitNewState (waiting for ChannelState::ChannelFunded) or other

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think WaitNewState makes sense - maybe there's some confusion here but if you see a "bad key" from something the remote generated they are clearly misbehaving - if you could randomly hit bad keys on accident it'd be a sign that the underlying ECC crypto were broken, so we should treat it as any other clearly-misbehaving node.

Copy link
Author

Choose a reason for hiding this comment

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

Not targeting the "bad key" err here but more the "Cannot get a ChannelAnnouncement until the channel funding has been locked" from get_channel_announcement which can be return upstream in block_connected. How should we handle this kind of case ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, I missed context, in any case, we can simplify this further, see #133.

return true;
}
};
Expand Down Expand Up @@ -1540,7 +1540,9 @@ impl ChannelMessageHandler for ChannelManager {

fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<msgs::FundingSigned, HandleError> {
let (chan, funding_msg, monitor_update) = {
let mut channel_state = self.channel_state.lock().unwrap();
let mut channel_lock = self.channel_state.lock().unwrap();
let channel_state = channel_lock.borrow_parts();
let short_to_id = channel_state.short_to_id;
match channel_state.by_id.entry(msg.temporary_channel_id.clone()) {
hash_map::Entry::Occupied(mut chan) => {
if chan.get().get_their_node_id() != *their_node_id {
Expand All @@ -1551,9 +1553,11 @@ impl ChannelMessageHandler for ChannelManager {
(chan.remove(), funding_msg, monitor_update)
},
Err(e) => {
//TODO: Possibly remove the channel depending on e.action
return Err(e);
}
let failure = chan.get_mut().force_shutdown();
short_to_id.remove(&chan.get_mut().get_short_channel_id().unwrap());
self.finish_force_close_channel(failure);
return Err(HandleError{err: e.err, action: e.action});
Copy link
Author

Choose a reason for hiding this comment

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

Here the errors are bad keys or shows strange behavior of peer so we close and disconnect. But pushing event here is pretty messy with channel and events locks so we give error upstream (ugly I admit still thinking on another way)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're gonna need to handle this in a number of places, so it may make sense to start thinking about splitting out the "Channel returned an Err, maybe call force_close and then return" function and wrap all the handle_MSG functions so that we can handle things appropriately.

Copy link
Author

Choose a reason for hiding this comment

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

Having a handle_channel_msg function in channel_manager which pass message and manage errors (remove channel and call force_close) ? Okay but it's gonna to get the lock again that some handle_MSG just dropped..

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking more that the ChannelMessageHandler impl for ChannelManager would mostly consist of wrapper functions that called into "real" handler functions and then, on error, called force_close channel. The ChannelMessageHandler requirements say that we'll never have paralell message handler calls for the same peer, so locking should be fine in that we'll handle the error in order for the given peer.

},
}
},
hash_map::Entry::Vacant(_) => return Err(HandleError{err: "Failed to find corresponding channel", action: None})
Expand Down