Skip to content

Add bolt 2 spec checks for funding messages #190

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 3 commits into from
Closed

Add bolt 2 spec checks for funding messages #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0413f2
Add check for channel id in fundung_created message
SWvheerden Sep 18, 2018
63d6ed2
check id of funding signed
SWvheerden Sep 19, 2018
26b949a
make funding_txo accessible so we can test if the channel id is correct.
SWvheerden Sep 19, 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
6 changes: 6 additions & 0 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,6 +1300,9 @@ impl Channel {
if self.channel_outbound {
return Err(HandleError{err: "Received funding_created for an outbound channel?", action: Some(msgs::ErrorAction::SendErrorMessage {msg: msgs::ErrorMessage {channel_id: self.channel_id, data: "Received funding_created for an outbound channel?".to_string()}})});
}
if self.channel_id != msg.temporary_channel_id {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method is called on self b/c this condition is true. So this if expression will never be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I potentially have this wrong, but isnt msg the incoming message from the remote peer? Which means h=it could have it wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

channelmanager maintains a mapping from channel_id to channel struct, so if an unknown temporary_channel_id is received, it will be disregarded instead of this method being invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome, cwl then we dont have to check these.

return Err(HandleError{err: "temporary channel id in funding_created does not match open_channel message", action: Some(msgs::ErrorAction::SendErrorMessage {msg: msgs::ErrorMessage {channel_id: self.channel_id, data: "temporary channel id in funding_created does not match open_channel message".to_string()}})});
}
if self.channel_state != (ChannelState::OurInitSent as u32 | ChannelState::TheirInitSent as u32) {
// BOLT 2 says that if we disconnect before we send funding_signed we SHOULD NOT
// remember the channel, so its safe to just send an error_message here and drop the
Expand Down Expand Up @@ -1352,6 +1355,9 @@ impl Channel {
self.cur_local_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER {
panic!("Should not have advanced channel commitment tx numbers prior to funding_created");
}
if self.channel_monitor.funding_txo.clone().unwrap().0.to_channel_id() != msg.channel_id {
return Err(HandleError{err: "Received incorrect channel_id in funding_signed message", action: None});
Copy link
Contributor

@yuntai yuntai Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Given how things are structured it'd be very hard for someone to introduce a bug that will make this condition true, but if that ever happens it should be hard panic, not returning Err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it is very likely that it col be caused by a bug. But the remote could accidentally or intentionally send the incorrect id.

Copy link
Contributor

Choose a reason for hiding this comment

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

ChannelManager handles an incorrect channel_id and channel_id collision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome, cwl then we dont have to check these.

}

let funding_script = self.get_funding_redeemscript();

Expand Down
2 changes: 1 addition & 1 deletion src/ln/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ const SERIALIZATION_VERSION: u8 = 1;
const MIN_SERIALIZATION_VERSION: u8 = 1;

pub struct ChannelMonitor {
funding_txo: Option<(OutPoint, Script)>,
pub funding_txo: Option<(OutPoint, Script)>,
commitment_transaction_number_obscure_factor: u64,

key_storage: KeyStorage,
Expand Down