From a2df43d525322812a6081e5ae0a666ce79efc20b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 26 Nov 2018 22:21:28 -0500 Subject: [PATCH] Remove check which makes us sometimes never send closing_signed This is the case pointed out by nayuta-gondo at https://github.com/lightningnetwork/lightning-rfc/issues/499#issuecomment-438623208 though this doesn't actually solve the issue of ensuring we have a consistent fee view when we start shutdown processing. There isn't a clear solution to that however without adding additional state tracking in Channel. This also removes an associated test that tests for the correct behavior (but didn't consider the bug) as we no longer behave correctly. This should be fine as we'll be removing all the update_fee garbage with option_simplified_commitment anyway. --- src/ln/channel.rs | 3 +-- src/ln/channelmanager.rs | 46 ---------------------------------------- 2 files changed, 1 insertion(+), 48 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 296d97c16..f6177e9eb 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -2406,8 +2406,7 @@ impl Channel { fn maybe_propose_first_closing_signed(&mut self, fee_estimator: &FeeEstimator) -> Option { if !self.channel_outbound || !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() || self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) != BOTH_SIDES_SHUTDOWN_MASK || - self.last_sent_closing_fee.is_some() || - self.cur_remote_commitment_transaction_number != self.cur_local_commitment_transaction_number{ + self.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() { return None; } diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index a47236469..fc8af4cca 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -4921,52 +4921,6 @@ mod tests { assert!(nodes[2].node.list_channels().is_empty()); } - #[test] - fn update_fee_async_shutdown() { - // Test update_fee works after shutdown start if messages are delivered out-of-order - let nodes = create_network(2); - let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1); - - let starting_feerate = nodes[0].node.channel_state.lock().unwrap().by_id.get(&chan_1.2).unwrap().get_feerate(); - nodes[0].node.update_fee(chan_1.2.clone(), starting_feerate + 20).unwrap(); - check_added_monitors!(nodes[0], 1); - let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); - assert!(updates.update_add_htlcs.is_empty()); - assert!(updates.update_fulfill_htlcs.is_empty()); - assert!(updates.update_fail_htlcs.is_empty()); - assert!(updates.update_fail_malformed_htlcs.is_empty()); - assert!(updates.update_fee.is_some()); - - nodes[1].node.close_channel(&chan_1.2).unwrap(); - let node_1_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id()); - nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_1_shutdown).unwrap(); - // Note that we don't actually test normative behavior here. The spec indicates we could - // actually send a closing_signed here, but is kinda unclear and could possibly be amended - // to require waiting on the full commitment dance before doing so (see - // https://github.com/lightningnetwork/lightning-rfc/issues/499). In any case, to avoid - // ambiguity, we should wait until after the full commitment dance to send closing_signed. - let node_0_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id()); - - nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &updates.update_fee.unwrap()).unwrap(); - nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &updates.commitment_signed).unwrap(); - check_added_monitors!(nodes[1], 1); - nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &node_0_shutdown).unwrap(); - let node_0_closing_signed = commitment_signed_dance!(nodes[1], nodes[0], (), false, true, true); - - assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); - nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), match node_0_closing_signed.unwrap() { - MessageSendEvent::SendClosingSigned { ref node_id, ref msg } => { - assert_eq!(*node_id, nodes[1].node.get_our_node_id()); - msg - }, - _ => panic!("Unexpected event"), - }).unwrap(); - let (_, node_1_closing_signed) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id()); - nodes[0].node.handle_closing_signed(&nodes[1].node.get_our_node_id(), &node_1_closing_signed.unwrap()).unwrap(); - let (_, node_0_none) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id()); - assert!(node_0_none.is_none()); - } - fn do_test_shutdown_rebroadcast(recv_count: u8) { // Test that shutdown/closing_signed is re-sent on reconnect with a variable number of // messages delivered prior to disconnect -- 2.39.5