Skip to content

Makes ChannelManager::force_close_channel fail for unknown chan_ids #777

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 1 commit into from
Jan 25, 2021
Merged

Makes ChannelManager::force_close_channel fail for unknown chan_ids #777

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
let channel_id = get_slice!(1)[0] as usize;
if channel_id >= channels.len() { return; }
channels.sort_by(|a, b| { a.channel_id.cmp(&b.channel_id) });
channelmanager.force_close_channel(&channels[channel_id].channel_id);
channelmanager.force_close_channel(&channels[channel_id].channel_id).unwrap();
},
// 15 is above
_ => return,
Expand Down
6 changes: 3 additions & 3 deletions lightning-persister/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ mod tests {

// Force close because cooperative close doesn't result in any persisted
// updates.
nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id);
nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id).unwrap();
check_closed_broadcast!(nodes[0], false);
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -354,7 +354,7 @@ mod tests {
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
nodes[1].node.force_close_channel(&chan.2);
nodes[1].node.force_close_channel(&chan.2).unwrap();
let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();

// Set the persister's directory to read-only, which should result in
Expand Down Expand Up @@ -390,7 +390,7 @@ mod tests {
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
nodes[1].node.force_close_channel(&chan.2);
nodes[1].node.force_close_channel(&chan.2).unwrap();
let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();

// Create the persister with an invalid directory name and test that the
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool, persister_fail
}

// ...and make sure we can force-close a frozen channel
nodes[0].node.force_close_channel(&channel_id);
nodes[0].node.force_close_channel(&channel_id).unwrap();
check_added_monitors!(nodes[0], 1);
check_closed_broadcast!(nodes[0], false);

Expand Down
16 changes: 10 additions & 6 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,8 +917,8 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
}

/// Force closes a channel, immediately broadcasting the latest local commitment transaction to
/// the chain and rejecting new HTLCs on the given channel.
pub fn force_close_channel(&self, channel_id: &[u8; 32]) {
/// the chain and rejecting new HTLCs on the given channel. Fails if channel_id is unknown to the manager.
pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError>{
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to update the docs here to state that we only fail in case the channel is not found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased in 0e83723

let _consistency_lock = self.total_consistency_lock.read().unwrap();

let mut chan = {
Expand All @@ -930,7 +930,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
}
chan
} else {
return;
return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
}
};
log_trace!(self.logger, "Force-closing channel {}", log_bytes!(channel_id[..]));
Expand All @@ -941,13 +941,15 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
msg: update
});
}

Ok(())
}

/// Force close all channels, immediately broadcasting the latest local commitment transaction
/// for each to the chain and rejecting new HTLCs on each.
pub fn force_close_all_channels(&self) {
for chan in self.list_channels() {
self.force_close_channel(&chan.channel_id);
let _ = self.force_close_channel(&chan.channel_id);
}
}

Expand Down Expand Up @@ -3471,11 +3473,13 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
if msg.channel_id == [0; 32] {
for chan in self.list_channels() {
if chan.remote_network_id == *counterparty_node_id {
self.force_close_channel(&chan.channel_id);
// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
let _ = self.force_close_channel(&msg.channel_id);
Copy link

Choose a reason for hiding this comment

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

Can you add a code comment here and below : "Untrusted messages from peer, we throw away the error if id points to a non-existent channel". A good warning to avoid vulns in in case of new work around this code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 821f6cd

}
}
} else {
self.force_close_channel(&msg.channel_id);
// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
let _ = self.force_close_channel(&msg.channel_id);
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3396,7 +3396,7 @@ fn test_htlc_ignore_latest_remote_commitment() {
create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());

route_payment(&nodes[0], &[&nodes[1]], 10000000);
nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id);
nodes[0].node.force_close_channel(&nodes[0].node.list_channels()[0].channel_id).unwrap();
check_closed_broadcast!(nodes[0], false);
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -3457,7 +3457,7 @@ fn test_force_close_fail_back() {
// state or updated nodes[1]' state. Now force-close and broadcast that commitment/HTLC
// transaction and ensure nodes[1] doesn't fail-backwards (this was originally a bug!).

nodes[2].node.force_close_channel(&payment_event.commitment_msg.channel_id);
nodes[2].node.force_close_channel(&payment_event.commitment_msg.channel_id).unwrap();
check_closed_broadcast!(nodes[2], false);
check_added_monitors!(nodes[2], 1);
let tx = {
Expand Down Expand Up @@ -4783,7 +4783,7 @@ fn test_claim_sizeable_push_msat() {
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 99000000, InitFeatures::known(), InitFeatures::known());
nodes[1].node.force_close_channel(&chan.2);
nodes[1].node.force_close_channel(&chan.2).unwrap();
check_closed_broadcast!(nodes[1], false);
check_added_monitors!(nodes[1], 1);
let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
Expand All @@ -4810,7 +4810,7 @@ fn test_claim_on_remote_sizeable_push_msat() {
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 99000000, InitFeatures::known(), InitFeatures::known());
nodes[0].node.force_close_channel(&chan.2);
nodes[0].node.force_close_channel(&chan.2).unwrap();
check_closed_broadcast!(nodes[0], false);
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -8544,7 +8544,7 @@ fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain
// responds by (1) broadcasting a channel update and (2) adding a new ChannelMonitor.
let mut force_closing_node = 0; // Alice force-closes
if !broadcast_alice { force_closing_node = 1; } // Bob force-closes
nodes[force_closing_node].node.force_close_channel(&chan_ab.2);
nodes[force_closing_node].node.force_close_channel(&chan_ab.2).unwrap();
check_closed_broadcast!(nodes[force_closing_node], false);
check_added_monitors!(nodes[force_closing_node], 1);
if go_onchain_before_fulfill {
Expand Down