From b288a2739afa18c5aedaacb00ed809c98be47187 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 30 Sep 2021 21:35:40 +0000 Subject: [PATCH] Return `ClosureReason` from `Channel` chain update methods This fixes a few `ClosureReason`s and allows us to have finer-grained user-visible errors when a channel closes due to an on-chain event. --- lightning/src/ln/channel.rs | 25 +++++++++-------------- lightning/src/ln/channelmanager.rs | 12 +++++++---- lightning/src/ln/functional_test_utils.rs | 2 +- lightning/src/ln/functional_tests.rs | 9 ++++---- lightning/src/ln/reorg_tests.rs | 19 +++++++++-------- lightning/src/util/events.rs | 21 +++++++++++++++++++ 6 files changed, 55 insertions(+), 33 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a921f68b9..8703f6c95 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -35,6 +35,7 @@ use chain::chaininterface::{FeeEstimator,ConfirmationTarget}; use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS}; use chain::transaction::{OutPoint, TransactionData}; use chain::keysinterface::{Sign, KeysInterface}; +use util::events::ClosureReason; use util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter}; use util::logger::Logger; use util::errors::APIError; @@ -4117,7 +4118,7 @@ impl Channel { /// In the first case, we store the confirmation height and calculating the short channel id. /// In the second, we simply return an Err indicating we need to be force-closed now. pub fn transactions_confirmed(&mut self, block_hash: &BlockHash, height: u32, txdata: &TransactionData, logger: &L) - -> Result, msgs::ErrorMessage> where L::Target: Logger { + -> Result, ClosureReason> where L::Target: Logger { let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS); for &(index_in_block, tx) in txdata.iter() { if let Some(funding_txo) = self.get_funding_txo() { @@ -4138,10 +4139,8 @@ impl Channel { panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!"); } self.update_time_counter += 1; - return Err(msgs::ErrorMessage { - channel_id: self.channel_id(), - data: "funding tx had wrong script/value or output index".to_owned() - }); + let err_reason = "funding tx had wrong script/value or output index"; + return Err(ClosureReason::ProcessingError { err: err_reason.to_owned() }); } else { if self.is_outbound() { for input in tx.input.iter() { @@ -4172,10 +4171,7 @@ impl Channel { for inp in tx.input.iter() { if inp.previous_output == funding_txo.into_bitcoin_outpoint() { log_info!(logger, "Detected channel-closing tx {} spending {}:{}, closing channel {}", tx.txid(), inp.previous_output.txid, inp.previous_output.vout, log_bytes!(self.channel_id())); - return Err(msgs::ErrorMessage { - channel_id: self.channel_id(), - data: "Commitment or closing transaction was confirmed on chain.".to_owned() - }); + return Err(ClosureReason::CommitmentTxConfirmed); } } } @@ -4195,7 +4191,7 @@ impl Channel { /// May return some HTLCs (and their payment_hash) which have timed out and should be failed /// back. pub fn best_block_updated(&mut self, height: u32, highest_header_time: u32, logger: &L) - -> Result<(Option, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> where L::Target: Logger { + -> Result<(Option, Vec<(HTLCSource, PaymentHash)>), ClosureReason> where L::Target: Logger { let mut timed_out_htlcs = Vec::new(); // This mirrors the check in ChannelManager::decode_update_add_htlc_onion, refusing to // forward an HTLC when our counterparty should almost certainly just fail it for expiring @@ -4236,10 +4232,9 @@ impl Channel { // close the channel and hope we can get the latest state on chain (because presumably // the funding transaction is at least still in the mempool of most nodes). if funding_tx_confirmations < self.minimum_depth.unwrap() as i64 / 2 { - return Err(msgs::ErrorMessage { - channel_id: self.channel_id(), - data: format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.", self.minimum_depth.unwrap(), funding_tx_confirmations), - }); + let err_reason = format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.", + self.minimum_depth.unwrap(), funding_tx_confirmations); + return Err(ClosureReason::ProcessingError { err: err_reason }); } } @@ -4249,7 +4244,7 @@ impl Channel { /// Indicates the funding transaction is no longer confirmed in the main chain. This may /// force-close the channel, but may also indicate a harmless reorganization of a block or two /// before the channel has reached funding_locked and we can just wait for more blocks. - pub fn funding_transaction_unconfirmed(&mut self, logger: &L) -> Result<(), msgs::ErrorMessage> where L::Target: Logger { + pub fn funding_transaction_unconfirmed(&mut self, logger: &L) -> Result<(), ClosureReason> where L::Target: Logger { if self.funding_tx_confirmation_height != 0 { // We handle the funding disconnection by calling best_block_updated with a height one // below where our funding was connected, implying a reorg back to conf_height - 1. diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 631edfe6c..a6c319b6e 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4850,7 +4850,7 @@ where /// Calls a function which handles an on-chain event (blocks dis/connected, transactions /// un/confirmed, etc) on each channel, handling any resulting errors or messages generated by /// the function. - fn do_chain_event) -> Result<(Option, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage>> + fn do_chain_event) -> Result<(Option, Vec<(HTLCSource, PaymentHash)>), ClosureReason>> (&self, height_opt: Option, f: FN) { // Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called // during initialization prior to the chain_monitor being fully configured in some cases. @@ -4895,7 +4895,7 @@ where } short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id()); } - } else if let Err(e) = res { + } else if let Err(reason) = res { if let Some(short_id) = channel.get_short_channel_id() { short_to_id.remove(&short_id); } @@ -4907,10 +4907,14 @@ where msg: update }); } - self.issue_channel_close_events(channel, ClosureReason::CommitmentTxConfirmed); + let reason_message = format!("{}", reason); + self.issue_channel_close_events(channel, reason); pending_msg_events.push(events::MessageSendEvent::HandleError { node_id: channel.get_counterparty_node_id(), - action: msgs::ErrorAction::SendErrorMessage { msg: e }, + action: msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { + channel_id: channel.channel_id(), + data: reason_message, + } }, }); return false; } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e6b246707..e9e36e607 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1737,7 +1737,7 @@ pub fn handle_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec(nodes: &Vec>, a: usize, b: usize) { - handle_announce_close_broadcast_events(nodes, a, b, false, "Commitment or closing transaction was confirmed on chain."); + handle_announce_close_broadcast_events(nodes, a, b, false, "Channel closed because commitment or closing transaction was confirmed on chain."); } #[cfg(test)] diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 118bb1497..b6b3d345e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1207,7 +1207,7 @@ fn test_duplicate_htlc_different_direction_onchain() { MessageSendEvent::BroadcastChannelUpdate { .. } => {}, MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => { assert_eq!(node_id, nodes[1].node.get_our_node_id()); - assert_eq!(msg.data, "Commitment or closing transaction was confirmed on chain."); + assert_eq!(msg.data, "Channel closed because commitment or closing transaction was confirmed on chain."); }, MessageSendEvent::UpdateHTLCs { ref node_id, updates: msgs::CommitmentUpdate { ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs, ref update_fail_malformed_htlcs, .. } } => { assert!(update_add_htlcs.is_empty()); @@ -3017,7 +3017,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use match events[if deliver_bs_raa { 2 } else { 1 }] { MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id, ref data } }, node_id: _ } => { assert_eq!(channel_id, chan_2.2); - assert_eq!(data.as_str(), "Commitment or closing transaction was confirmed on chain."); + assert_eq!(data.as_str(), "Channel closed because commitment or closing transaction was confirmed on chain."); }, _ => panic!("Unexpected event"), } @@ -8831,15 +8831,16 @@ fn test_invalid_funding_tx() { assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx); nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); + let expected_err = "funding tx had wrong script/value or output index"; confirm_transaction_at(&nodes[1], &tx, 1); - check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed); + check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: expected_err.to_string() }); check_added_monitors!(nodes[1], 1); let events_2 = nodes[1].node.get_and_clear_pending_msg_events(); assert_eq!(events_2.len(), 1); if let MessageSendEvent::HandleError { node_id, action } = &events_2[0] { assert_eq!(*node_id, nodes[0].node.get_our_node_id()); if let msgs::ErrorAction::SendErrorMessage { msg } = action { - assert_eq!(msg.data, "funding tx had wrong script/value or output index"); + assert_eq!(msg.data, "Channel closed because of an exception: ".to_owned() + expected_err); } else { panic!(); } } else { panic!(); } assert_eq!(nodes[1].node.list_channels().len(), 0); diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index a9b341b34..5cd1a890c 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -219,9 +219,9 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_ disconnect_all_blocks(&nodes[0]); } if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed { - handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs."); + handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs."); } else { - handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs."); + handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs."); } check_added_monitors!(nodes[1], 1); { @@ -287,9 +287,9 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_ disconnect_all_blocks(&nodes[0]); } if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed { - handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs."); + handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs."); } else { - handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs."); + handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs."); } check_added_monitors!(nodes[1], 1); { @@ -303,12 +303,13 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_ *nodes[0].chain_monitor.expect_channel_force_closed.lock().unwrap() = Some((chan.2, true)); nodes[0].node.test_process_background_events(); // Required to free the pending background monitor update check_added_monitors!(nodes[0], 1); - check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed); - if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed { - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs.".to_string() }); + let expected_err = if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed { + "Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs." } else { - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.".to_string() }); - } + "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs." + }; + check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "Channel closed because of an exception: ".to_owned() + expected_err }); + check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: expected_err.to_owned() }); assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index e0f0a63db..c021fe3c1 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -118,6 +118,27 @@ pub enum ClosureReason { OutdatedChannelManager } +impl core::fmt::Display for ClosureReason { + fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { + f.write_str("Channel closed because ")?; + match self { + ClosureReason::CounterpartyForceClosed { peer_msg } => { + f.write_str("counterparty force-closed with message ")?; + f.write_str(&peer_msg) + }, + ClosureReason::HolderForceClosed => f.write_str("user manually force-closed the channel"), + ClosureReason::CooperativeClosure => f.write_str("the channel was cooperatively closed"), + ClosureReason::CommitmentTxConfirmed => f.write_str("commitment or closing transaction was confirmed on chain."), + ClosureReason::ProcessingError { err } => { + f.write_str("of an exception: ")?; + f.write_str(&err) + }, + ClosureReason::DisconnectedPeer => f.write_str("the peer disconnected prior to the channel being funded"), + ClosureReason::OutdatedChannelManager => f.write_str("the ChannelManager read from disk was stale compared to ChannelMonitor(s)"), + } + } +} + impl_writeable_tlv_based_enum_upgradable!(ClosureReason, (0, CounterpartyForceClosed) => { (1, peer_msg, required) }, (2, HolderForceClosed) => {}, -- 2.39.5