diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index 15d46b04688..11434ce3a89 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -2238,11 +2238,12 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { let (route, payment_hash_1, payment_preimage_1, payment_secret_1) = get_route_and_payment_hash!(&nodes[0], nodes[1], 100000); let (payment_preimage_2, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(&nodes[1]); - // Do a really complicated dance to get an HTLC into the holding cell, with MonitorUpdateFailed - // set but AwaitingRemoteRevoke unset. When this test was written, any attempts to send an HTLC - // while MonitorUpdateFailed is set are immediately failed-backwards. Thus, the only way to get - // an AddHTLC into the holding cell is to add it while AwaitingRemoteRevoke is set but - // MonitorUpdateFailed is unset, and then swap the flags. + // Do a really complicated dance to get an HTLC into the holding cell, with + // MonitorUpdateInProgress set but AwaitingRemoteRevoke unset. When this test was written, any + // attempts to send an HTLC while MonitorUpdateInProgress is set are immediately + // failed-backwards. Thus, the only way to get an AddHTLC into the holding cell is to add it + // while AwaitingRemoteRevoke is set but MonitorUpdateInProgress is unset, and then swap the + // flags. // // We do this by: // a) routing a payment from node B to node A, @@ -2255,8 +2256,8 @@ fn do_channel_holding_cell_serialize(disconnect: bool, reload_a: bool) { // e) delivering A's commitment_signed from (b) and the resulting B revoke_and_ack message, // clearing AwaitingRemoteRevoke on node A. // - // Note that because, at the end, MonitorUpdateFailed is still set, the HTLC generated in (c) - // will not be freed from the holding cell. + // Note that because, at the end, MonitorUpdateInProgress is still set, the HTLC generated in + // (c) will not be freed from the holding cell. let (payment_preimage_0, payment_hash_0, _) = route_payment(&nodes[1], &[&nodes[0]], 100_000); nodes[0].node.send_payment(&route, payment_hash_1, &Some(payment_secret_1)).unwrap(); @@ -2745,3 +2746,221 @@ fn double_temp_error() { commitment_signed_dance!(nodes[0], nodes[1], commitment_signed_b2, false); expect_payment_sent!(nodes[0], payment_preimage_2); } + +fn do_test_outbound_reload_without_init_mon(use_0conf: bool) { + // Test that if the monitor update generated in funding_signed is stored async and we restart + // with the latest ChannelManager but the ChannelMonitor persistence never completed we happily + // drop the channel and move on. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + + let persister: test_utils::TestPersister; + let new_chain_monitor: test_utils::TestChainMonitor; + let nodes_0_deserialized: ChannelManager; + + let mut chan_config = test_default_channel_config(); + chan_config.manually_accept_inbound_channels = true; + chan_config.channel_handshake_limits.trust_own_funding_0conf = true; + + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(chan_config), Some(chan_config)]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 43, None).unwrap(); + nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), channelmanager::provided_init_features(), &get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id())); + + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::OpenChannelRequest { temporary_channel_id, .. } => { + if use_0conf { + nodes[1].node.accept_inbound_channel_from_trusted_peer_0conf(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 0).unwrap(); + } else { + nodes[1].node.accept_inbound_channel(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 0).unwrap(); + } + }, + _ => panic!("Unexpected event"), + }; + + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), channelmanager::provided_init_features(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())); + + let (temporary_channel_id, funding_tx, ..) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 43); + + nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), funding_tx.clone()).unwrap(); + check_added_monitors!(nodes[0], 0); + + let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()); + nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg); + check_added_monitors!(nodes[1], 1); + + let bs_signed_locked = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(bs_signed_locked.len(), if use_0conf { 2 } else { 1 }); + match &bs_signed_locked[0] { + MessageSendEvent::SendFundingSigned { msg, .. } => { + chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &msg); + check_added_monitors!(nodes[0], 1); + } + _ => panic!("Unexpected event"), + } + if use_0conf { + match &bs_signed_locked[1] { + MessageSendEvent::SendChannelReady { msg, .. } => { + nodes[0].node.handle_channel_ready(&nodes[1].node.get_our_node_id(), &msg); + } + _ => panic!("Unexpected event"), + } + } + + assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty()); + assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); + assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); + + // nodes[0] is now waiting on the first ChannelMonitor persistence to complete in order to + // broadcast the funding transaction. If nodes[0] restarts at this point with the + // ChannelMonitor lost, we should simply discard the channel. + + // The test framework checks that watched_txn/outputs match the monitor set, which they will + // not, so we have to clear them here. + nodes[0].chain_source.watched_txn.lock().unwrap().clear(); + nodes[0].chain_source.watched_outputs.lock().unwrap().clear(); + + let nodes_0_serialized = nodes[0].node.encode(); + persister = test_utils::TestPersister::new(); + let keys_manager = &chanmon_cfgs[0].keys_manager; + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), nodes[0].logger, node_cfgs[0].fee_estimator, &persister, keys_manager); + nodes[0].chain_monitor = &new_chain_monitor; + + let mut nodes_0_read = &nodes_0_serialized[..]; + let config = UserConfig::default(); + nodes_0_deserialized = { + <(BlockHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { + default_config: config, + keys_manager, + fee_estimator: node_cfgs[0].fee_estimator, + chain_monitor: nodes[0].chain_monitor, + tx_broadcaster: nodes[0].tx_broadcaster.clone(), + logger: nodes[0].logger, + channel_monitors: HashMap::new(), + }).unwrap().1 + }; + nodes[0].node = &nodes_0_deserialized; + assert!(nodes_0_read.is_empty()); + + check_closed_event!(nodes[0], 1, ClosureReason::DisconnectedPeer); + assert!(nodes[0].node.list_channels().is_empty()); +} + +#[test] +fn test_outbound_reload_without_init_mon() { + do_test_outbound_reload_without_init_mon(true); + do_test_outbound_reload_without_init_mon(false); +} + +fn do_test_inbound_reload_without_init_mon(use_0conf: bool, lock_commitment: bool) { + // Test that if the monitor update generated by funding_transaction_generated is stored async + // and we restart with the latest ChannelManager but the ChannelMonitor persistence never + // completed we happily drop the channel and move on. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + + let persister: test_utils::TestPersister; + let new_chain_monitor: test_utils::TestChainMonitor; + let nodes_1_deserialized: ChannelManager; + + let mut chan_config = test_default_channel_config(); + chan_config.manually_accept_inbound_channels = true; + chan_config.channel_handshake_limits.trust_own_funding_0conf = true; + + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(chan_config), Some(chan_config)]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 43, None).unwrap(); + nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), channelmanager::provided_init_features(), &get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id())); + + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::OpenChannelRequest { temporary_channel_id, .. } => { + if use_0conf { + nodes[1].node.accept_inbound_channel_from_trusted_peer_0conf(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 0).unwrap(); + } else { + nodes[1].node.accept_inbound_channel(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 0).unwrap(); + } + }, + _ => panic!("Unexpected event"), + }; + + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), channelmanager::provided_init_features(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())); + + let (temporary_channel_id, funding_tx, ..) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 43); + + nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), funding_tx.clone()).unwrap(); + check_added_monitors!(nodes[0], 0); + + let funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()); + chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress); + nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg); + check_added_monitors!(nodes[1], 1); + + // nodes[1] happily sends its funding_signed even though its awaiting the persistence of the + // initial ChannelMonitor, but it will decline to send its channel_ready even if the funding + // transaction is confirmed. + let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed_msg); + check_added_monitors!(nodes[0], 1); + + let as_funding_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); + if lock_commitment { + confirm_transaction(&nodes[0], &as_funding_tx[0]); + confirm_transaction(&nodes[1], &as_funding_tx[0]); + } + if use_0conf || lock_commitment { + let as_ready = get_event_msg!(nodes[0], MessageSendEvent::SendChannelReady, nodes[1].node.get_our_node_id()); + nodes[1].node.handle_channel_ready(&nodes[0].node.get_our_node_id(), &as_ready); + } + assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + + // nodes[1] is now waiting on the first ChannelMonitor persistence to complete in order to + // move the channel to ready (or is waiting on the funding transaction to confirm). If nodes[1] + // restarts at this point with the ChannelMonitor lost, we should simply discard the channel. + + // The test framework checks that watched_txn/outputs match the monitor set, which they will + // not, so we have to clear them here. + nodes[1].chain_source.watched_txn.lock().unwrap().clear(); + nodes[1].chain_source.watched_outputs.lock().unwrap().clear(); + + let nodes_1_serialized = nodes[1].node.encode(); + persister = test_utils::TestPersister::new(); + let keys_manager = &chanmon_cfgs[1].keys_manager; + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[1].chain_source), nodes[1].tx_broadcaster.clone(), nodes[1].logger, node_cfgs[1].fee_estimator, &persister, keys_manager); + nodes[1].chain_monitor = &new_chain_monitor; + + let mut nodes_1_read = &nodes_1_serialized[..]; + let config = UserConfig::default(); + nodes_1_deserialized = { + <(BlockHash, ChannelManager)>::read(&mut nodes_1_read, ChannelManagerReadArgs { + default_config: config, + keys_manager, + fee_estimator: node_cfgs[1].fee_estimator, + chain_monitor: nodes[1].chain_monitor, + tx_broadcaster: nodes[1].tx_broadcaster.clone(), + logger: nodes[1].logger, + channel_monitors: HashMap::new(), + }).unwrap().1 + }; + nodes[1].node = &nodes_1_deserialized; + assert!(nodes_1_read.is_empty()); + + check_closed_event!(nodes[1], 1, ClosureReason::DisconnectedPeer); + assert!(nodes[1].node.list_channels().is_empty()); +} + +#[test] +fn test_inbound_reload_without_init_mon() { + do_test_inbound_reload_without_init_mon(true, true); + do_test_inbound_reload_without_init_mon(true, false); + do_test_inbound_reload_without_init_mon(false, true); + do_test_inbound_reload_without_init_mon(false, false); +} diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 48a701e3b04..cbd04ccf2a0 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -273,9 +273,9 @@ enum ChannelState { /// dance. PeerDisconnected = 1 << 7, /// Flag which is set on ChannelFunded, FundingCreated, and FundingSent indicating the user has - /// told us they failed to update our ChannelMonitor somewhere and we should pause sending any - /// outbound messages until they've managed to do so. - MonitorUpdateFailed = 1 << 8, + /// told us a ChannelMonitor update is pending async persistence somewhere and we should pause + /// sending any outbound messages until they've managed to finish. + MonitorUpdateInProgress = 1 << 8, /// Flag which implies that we have sent a commitment_signed but are awaiting the responding /// revoke_and_ack message. During this time period, we can't generate new commitment_signed /// messages as then we will be unable to determine which HTLCs they included in their @@ -295,7 +295,7 @@ enum ChannelState { ShutdownComplete = 4096, } const BOTH_SIDES_SHUTDOWN_MASK: u32 = ChannelState::LocalShutdownSent as u32 | ChannelState::RemoteShutdownSent as u32; -const MULTI_STATE_FLAGS: u32 = BOTH_SIDES_SHUTDOWN_MASK | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32; +const MULTI_STATE_FLAGS: u32 = BOTH_SIDES_SHUTDOWN_MASK | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32; pub const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1; @@ -1776,7 +1776,7 @@ impl Channel { where L::Target: Logger { // Assert that we'll add the HTLC claim to the holding cell in `get_update_fulfill_htlc` // (see equivalent if condition there). - assert!(self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) != 0); + assert!(self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32) != 0); let mon_update_id = self.latest_monitor_update_id; // Forget the ChannelMonitor update let fulfill_resp = self.get_update_fulfill_htlc(htlc_id_arg, payment_preimage_arg, logger); self.latest_monitor_update_id = mon_update_id; @@ -1846,7 +1846,7 @@ impl Channel { }], }; - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 { + if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 { // Note that this condition is the same as the assertion in // `claim_htlc_while_disconnected_dropping_mon_update` and must match exactly - // `claim_htlc_while_disconnected_dropping_mon_update` would not work correctly if we @@ -1971,7 +1971,7 @@ impl Channel { } // Now update local state: - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 { + if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 { for pending_update in self.holding_cell_htlc_updates.iter() { match pending_update { &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { @@ -2258,7 +2258,7 @@ impl Channel { if !self.is_outbound() { return Err(ChannelError::Close("Received funding_signed for an inbound channel?".to_owned())); } - if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 { + if self.channel_state & !(ChannelState::MonitorUpdateInProgress as u32) != ChannelState::FundingCreated as u32 { return Err(ChannelError::Close("Received funding_signed in strange state!".to_owned())); } if self.commitment_secrets.get_min_seen_secret() != (1 << 48) || @@ -2316,7 +2316,7 @@ impl Channel { channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_bitcoin_tx.txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger); - assert_eq!(self.channel_state & (ChannelState::MonitorUpdateFailed as u32), 0); // We have no had any monitor(s) yet to fail update! + assert_eq!(self.channel_state & (ChannelState::MonitorUpdateInProgress as u32), 0); // We have no had any monitor(s) yet to fail update! self.channel_state = ChannelState::FundingSent as u32; self.cur_holder_commitment_transaction_number -= 1; self.cur_counterparty_commitment_transaction_number -= 1; @@ -3078,7 +3078,7 @@ impl Channel { // send_commitment_no_status_check() next which will reset this to RAAFirst. self.resend_order = RAACommitmentOrder::CommitmentFirst; - if (self.channel_state & ChannelState::MonitorUpdateFailed as u32) != 0 { + if (self.channel_state & ChannelState::MonitorUpdateInProgress as u32) != 0 { // In case we initially failed monitor updating without requiring a response, we need // to make sure the RAA gets sent first. self.monitor_pending_revoke_and_ack = true; @@ -3125,7 +3125,7 @@ impl Channel { /// returns `(None, Vec::new())`. pub fn maybe_free_holding_cell_htlcs(&mut self, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger { if self.channel_state >= ChannelState::ChannelFunded as u32 && - (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) == 0 { + (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)) == 0 { self.free_holding_cell_htlcs(logger) } else { Ok((None, Vec::new())) } } @@ -3133,7 +3133,7 @@ impl Channel { /// Used to fulfill holding_cell_htlcs when we get a remote ack (or implicitly get it by them /// fulfilling or failing the last pending HTLC) fn free_holding_cell_htlcs(&mut self, logger: &L) -> Result<(Option<(msgs::CommitmentUpdate, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>), ChannelError> where L::Target: Logger { - assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0); + assert_eq!(self.channel_state & ChannelState::MonitorUpdateInProgress as u32, 0); if self.holding_cell_htlc_updates.len() != 0 || self.holding_cell_update_fee.is_some() { log_trace!(logger, "Freeing holding cell with {} HTLC updates{} in channel {}", self.holding_cell_htlc_updates.len(), if self.holding_cell_update_fee.is_some() { " and a fee update" } else { "" }, log_bytes!(self.channel_id())); @@ -3428,7 +3428,7 @@ impl Channel { } } - if (self.channel_state & ChannelState::MonitorUpdateFailed as u32) == ChannelState::MonitorUpdateFailed as u32 { + if (self.channel_state & ChannelState::MonitorUpdateInProgress as u32) == ChannelState::MonitorUpdateInProgress as u32 { // We can't actually generate a new commitment transaction (incl by freeing holding // cells) while we can't update the monitor, so we just return what we have. if require_commitment { @@ -3557,7 +3557,7 @@ impl Channel { return None; } - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 { + if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 { self.holding_cell_update_fee = Some(feerate_per_kw); return None; } @@ -3677,15 +3677,15 @@ impl Channel { self.monitor_pending_forwards.append(&mut pending_forwards); self.monitor_pending_failures.append(&mut pending_fails); self.monitor_pending_finalized_fulfills.append(&mut pending_finalized_claimed_htlcs); - self.channel_state |= ChannelState::MonitorUpdateFailed as u32; + self.channel_state |= ChannelState::MonitorUpdateInProgress as u32; } /// Indicates that the latest ChannelMonitor update has been committed by the client /// successfully and we should restore normal operation. Returns messages which should be sent /// to the remote side. pub fn monitor_updating_restored(&mut self, logger: &L, node_pk: PublicKey, genesis_block_hash: BlockHash, best_block_height: u32) -> MonitorRestoreUpdates where L::Target: Logger { - assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32); - self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32); + assert_eq!(self.channel_state & ChannelState::MonitorUpdateInProgress as u32, ChannelState::MonitorUpdateInProgress as u32); + self.channel_state &= !(ChannelState::MonitorUpdateInProgress as u32); // If we're past (or at) the FundingSent stage on an outbound channel, try to // (re-)broadcast the funding transaction as we may have declined to broadcast it when we @@ -3700,9 +3700,9 @@ impl Channel { funding_broadcastable = None; } - // We will never broadcast the funding transaction when we're in MonitorUpdateFailed (and - // we assume the user never directly broadcasts the funding transaction and waits for us to - // do it). Thus, we can only ever hit monitor_pending_channel_ready when we're + // We will never broadcast the funding transaction when we're in MonitorUpdateInProgress + // (and we assume the user never directly broadcasts the funding transaction and waits for + // us to do it). Thus, we can only ever hit monitor_pending_channel_ready when we're // * an inbound channel that failed to persist the monitor on funding_created and we got // the funding transaction confirmed before the monitor was persisted, or // * a 0-conf channel and intended to send the channel_ready before any broadcast at all. @@ -3941,7 +3941,7 @@ impl Channel { if self.channel_state & (ChannelState::FundingSent as u32) == ChannelState::FundingSent as u32 { // If we're waiting on a monitor update, we shouldn't re-send any channel_ready's. if self.channel_state & (ChannelState::OurChannelReady as u32) == 0 || - self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 { + self.channel_state & (ChannelState::MonitorUpdateInProgress as u32) != 0 { if msg.next_remote_commitment_number != 0 { return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent channel_ready yet".to_owned())); } @@ -3975,7 +3975,7 @@ impl Channel { // Note that if we need to repeat our ChannelReady we'll do that in the next if block. None } else if msg.next_remote_commitment_number + 1 == (INITIAL_COMMITMENT_NUMBER - 1) - self.cur_holder_commitment_transaction_number { - if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 { + if self.channel_state & (ChannelState::MonitorUpdateInProgress as u32) != 0 { self.monitor_pending_revoke_and_ack = true; None } else { @@ -3992,7 +3992,7 @@ impl Channel { let next_counterparty_commitment_number = INITIAL_COMMITMENT_NUMBER - self.cur_counterparty_commitment_transaction_number + if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) != 0 { 1 } else { 0 }; let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number == 1 { - // We should never have to worry about MonitorUpdateFailed resending ChannelReady + // We should never have to worry about MonitorUpdateInProgress resending ChannelReady let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx); Some(msgs::ChannelReady { channel_id: self.channel_id(), @@ -4008,7 +4008,7 @@ impl Channel { log_debug!(logger, "Reconnected channel {} with no loss", log_bytes!(self.channel_id())); } - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) == 0 { + if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) == 0 { // We're up-to-date and not waiting on a remote revoke (if we are our // channel_reestablish should result in them sending a revoke_and_ack), but we may // have received some updates while we were disconnected. Free the holding cell @@ -4055,7 +4055,7 @@ impl Channel { log_debug!(logger, "Reconnected channel {} with only lost remote commitment tx", log_bytes!(self.channel_id())); } - if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 { + if self.channel_state & (ChannelState::MonitorUpdateInProgress as u32) != 0 { self.monitor_pending_commitment_signed = true; Ok(ReestablishResponses { channel_ready, shutdown_msg, announcement_sigs, @@ -4137,7 +4137,7 @@ impl Channel { self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32 | - ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) + ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32) == BOTH_SIDES_SHUTDOWN_MASK && self.pending_update_fee.is_none() } @@ -4333,7 +4333,7 @@ impl Channel { return Err(ChannelError::Close("Remote tried to send a closing_signed when we were supposed to propose the first one".to_owned())); } - if self.channel_state & ChannelState::MonitorUpdateFailed as u32 != 0 { + if self.channel_state & ChannelState::MonitorUpdateInProgress as u32 != 0 { self.pending_counterparty_closing_signed = Some(msg.clone()); return Ok((None, None)); } @@ -4780,7 +4780,7 @@ impl Channel { /// Returns true if this channel has been marked as awaiting a monitor update to move forward. /// Allowed in any state (including after shutdown) pub fn is_awaiting_monitor_update(&self) -> bool { - (self.channel_state & ChannelState::MonitorUpdateFailed as u32) != 0 + (self.channel_state & ChannelState::MonitorUpdateInProgress as u32) != 0 } /// Returns true if funding_created was sent/received. @@ -4788,6 +4788,42 @@ impl Channel { self.channel_state >= ChannelState::FundingSent as u32 } + /// Returns true if the channel is awaiting the persistence of the initial ChannelMonitor. + /// If the channel is outbound, this implies we have not yet broadcasted the funding + /// transaction. If the channel is inbound, this implies simply that the channel has not + /// advanced state. + pub fn is_awaiting_initial_mon_persist(&self) -> bool { + if !self.is_awaiting_monitor_update() { return false; } + if self.channel_state & + !(ChannelState::TheirChannelReady as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32) + == ChannelState::FundingSent as u32 { + // If we're not a 0conf channel, we'll be waiting on a monitor update with only + // FundingSent set, though our peer could have sent their channel_ready. + debug_assert!(self.minimum_depth.unwrap_or(1) > 0); + return true; + } + if self.cur_holder_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 && + self.cur_counterparty_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 { + // If we're a 0-conf channel, we'll move beyond FundingSent immediately even while + // waiting for the initial monitor persistence. Thus, we check if our commitment + // transaction numbers have both been iterated only exactly once (for the + // funding_signed), and we're awaiting monitor update. + // + // If we got here, we shouldn't have yet broadcasted the funding transaction (as the + // only way to get an awaiting-monitor-update state during initial funding is if the + // initial monitor persistence is still pending). + // + // Because deciding we're awaiting initial broadcast spuriously could result in + // funds-loss (as we don't have a monitor, but have the funding transaction confirmed), + // we hard-assert here, even in production builds. + if self.is_outbound() { assert!(self.funding_transaction.is_some()); } + assert!(self.monitor_pending_channel_ready); + assert_eq!(self.latest_monitor_update_id, 0); + return true; + } + false + } + /// Returns true if our channel_ready has been sent pub fn is_our_channel_ready(&self) -> bool { (self.channel_state & ChannelState::OurChannelReady as u32) != 0 || self.channel_state >= ChannelState::ChannelFunded as u32 @@ -4866,7 +4902,7 @@ impl Channel { }; if need_commitment_update { - if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 { + if self.channel_state & (ChannelState::MonitorUpdateInProgress as u32) == 0 { if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 { let next_per_commitment_point = self.holder_signer.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &self.secp_ctx); @@ -5440,9 +5476,9 @@ impl Channel { /// * In cases where we're waiting on the remote peer to send us a revoke_and_ack, we /// wouldn't be able to determine what they actually ACK'ed if we have two sets of updates /// awaiting ACK. - /// * In cases where we're marked MonitorUpdateFailed, we cannot commit to a new state as we - /// may not yet have sent the previous commitment update messages and will need to regenerate - /// them. + /// * In cases where we're marked MonitorUpdateInProgress, we cannot commit to a new state as + /// we may not yet have sent the previous commitment update messages and will need to + /// regenerate them. /// /// You MUST call send_commitment prior to calling any other methods on this Channel! /// @@ -5543,7 +5579,7 @@ impl Channel { } // Now update local state: - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 { + if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateInProgress as u32)) != 0 { self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC { amount_msat, payment_hash, @@ -5590,7 +5626,7 @@ impl Channel { if (self.channel_state & (ChannelState::PeerDisconnected as u32)) == (ChannelState::PeerDisconnected as u32) { panic!("Cannot create commitment tx while disconnected, as send_htlc will have returned an Err so a send_commitment precondition has been violated"); } - if (self.channel_state & (ChannelState::MonitorUpdateFailed as u32)) == (ChannelState::MonitorUpdateFailed as u32) { + if (self.channel_state & (ChannelState::MonitorUpdateInProgress as u32)) == (ChannelState::MonitorUpdateInProgress as u32) { panic!("Cannot create commitment tx while awaiting monitor update unfreeze, as send_htlc will have returned an Err so a send_commitment precondition has been violated"); } let mut have_updates = self.is_outbound() && self.pending_update_fee.is_some(); @@ -5782,7 +5818,7 @@ impl Channel { } } assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0); - if self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) != 0 { + if self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32) != 0 { return Err(APIError::ChannelUnavailable{err: "Cannot begin shutdown while peer is disconnected or we're waiting on a monitor update, maybe force-close instead?".to_owned()}); } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f234ad9f91f..8f0f3811ba3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6921,6 +6921,16 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> } by_id.insert(channel.channel_id(), channel); } + } else if channel.is_awaiting_initial_mon_persist() { + // If we were persisted and shut down while the initial ChannelMonitor persistence + // was in-progress, we never broadcasted the funding transaction and can still + // safely discard the channel. + let _ = channel.force_shutdown(false); + channel_closures.push(events::Event::ChannelClosed { + channel_id: channel.channel_id(), + user_channel_id: channel.get_user_id(), + reason: ClosureReason::DisconnectedPeer, + }); } else { log_error!(args.logger, "Missing ChannelMonitor for channel {} needed by ChannelManager.", log_bytes!(channel.channel_id())); log_error!(args.logger, " The chain::Watch API *requires* that monitors are persisted durably before returning,"); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 8ddd762e970..d2aa076bd21 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -111,11 +111,19 @@ pub enum ClosureReason { /// The peer disconnected prior to funding completing. In this case the spec mandates that we /// forget the channel entirely - we can attempt again if the peer reconnects. /// + /// This includes cases where we restarted prior to funding completion, including prior to the + /// initial [`ChannelMonitor`] persistence completing. + /// /// In LDK versions prior to 0.0.107 this could also occur if we were unable to connect to the /// peer because of mutual incompatibility between us and our channel counterparty. + /// + /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor DisconnectedPeer, - /// Closure generated from `ChannelManager::read` if the ChannelMonitor is newer than - /// the ChannelManager deserialized. + /// Closure generated from `ChannelManager::read` if the [`ChannelMonitor`] is newer than + /// the [`ChannelManager`] deserialized. + /// + /// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor + /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager OutdatedChannelManager }