Refactor handle_accept_channel to wrapper error handling function #151
Conversation
src/ln/channelmanager.rs
Outdated
| match channel_state.by_id.get_mut(&msg.temporary_channel_id) { | ||
| Some(chan) => { | ||
| if chan.get_their_node_id() != *their_node_id { | ||
| return Err(MsgHandleErrInternal::disconnect_peer_with_err_msg("Got a message for a channel from the wrong node!", msg.temporary_channel_id.clone())); |
There was a problem hiding this comment.
A node giving us message for a channel it doesn't own seems to me either a malicious node or a busted one. Disconnect it?
There was a problem hiding this comment.
I'd be ok with that, but we need to be consistent and I know we do it differently elsewhere. I think we need to come up with some vaguely consistent concept of disconnect vs just error message and I really have no idea what it should be. I was thinking earlier today there's probably situations (like maybe this) where we'd want to disconnect if they do something obnoxious but not DoS-y but only if we dont have an open channel with them. It wouldnt be worth breaking a channel over, but if they're a random peer then maybe it would be worth disconnecting. Not really sure how to handle it, but it may make sense to think about removing the distinction between Disconnect and error message for an Error type that is used in Channel/ChannelManager and then convert that to HandleError in the ChannelManager macro when returning.
In any case, all of that is obviously out-of-scope for this PR and probably only makes sense as a refactor after all the moves-into-internal-function refactors here are done. Maybe just open an issue for it, put it on the TODO list and not worry about it now?
There was a problem hiding this comment.
Yes, out-of-scope for now, done with #153
src/ln/channelmanager.rs
Outdated
| @@ -155,6 +155,23 @@ impl MsgHandleErrInternal { | |||
| needs_channel_force_close: true, | |||
| } | |||
| } | |||
|
|
|||
| #[inline] | |||
| fn disconnect_peer_with_err_msg(err: &'static str, channel_id: [u8; 32]) -> Self { | |||
There was a problem hiding this comment.
Can we keep the naming consistent and mention close/no-close in the fn name as thats the most important distinction (also in your use-case here you need no-close, cause a random third-party sent us a message for the wrong channel so we shouldn't close it on the actual peer's channel).
There was a problem hiding this comment.
Remove it but I keep the comment on my side, if I need it again in a coming commits
|
Looks good aside from the two comments there. As an aside I was thinking that while we go through these refactors we should go ahead and (in separate commits) make sure we fill in all the cases with action: None with Some() so that we can drop the Option<> soon, since we're re-reading it all anyway. I think this PR qualifies but just making note in case you feel up to that. |
ariard
force-pushed
the
channel_manager_refactor
branch
from
b172ba5
to
b6bbce5
Compare
|
Splitted in two commits. Still feeling than disconnect_peer, channel_closure could be a combination of flags on top of an error message? |
src/ln/channelmanager.rs
Outdated
| Some(chan) => { | ||
| if chan.get_their_node_id() != *their_node_id { | ||
| //TODO: see issue #153 , need a consistent behavior on obnoxious behavior from random node | ||
| return Err(MsgHandleErrInternal::from_no_close(HandleError{err: "Got a message for a channel from the wrong node!", action: Some(msgs::ErrorAction::IgnoreError)})); |
There was a problem hiding this comment.
Oops, well it needs to be consistent but either between SendErrorMessage or Disconnect with an ErrorMessage specified. IgnoreError isnt really correct either way.
src/ln/channelmanager.rs
Outdated
| chan.accept_channel(&msg).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?; | ||
| (chan.get_value_satoshis(), chan.get_funding_redeemscript().to_v0_p2wsh(), chan.get_user_id()) | ||
| }, | ||
| None => return Err(MsgHandleErrInternal::from_no_close(HandleError{err: "Failed to find corresponding channel", action: Some(msgs::ErrorAction::IgnoreError)})) |
There was a problem hiding this comment.
This should also be the ErrorMessage/Disconnect you had earlier, same as above (and also covered in #153).
ariard
force-pushed
the
channel_manager_refactor
branch
from
b6bbce5
to
c09669f
Compare
Refactor handle_accept_channel to wrapper error handling function
|
Merged with the commits squashed so we dont have commits that fail to build. |
Fill in HandleError actions