Uh oh!
There was an error while loading. Please reload this page.
Add HTLCsTimedOut closing reason#2887
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -50,7 +50,7 @@ use crate::chain::Filter; | ||
| use crate::util::logger::{Logger, Record}; | ||
| use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, UpgradableRequired, Writer, Writeable, U48}; | ||
| use crate::util::byte_utils; | ||
| use crate::events::{ClosureReason, Event, EventHandler}; | ||
| use crate::events::bump_transaction::{AnchorDescriptor, BumpTransactionEvent}; | ||
| use crate::prelude::*; | ||
| @@ -155,6 +155,17 @@ pub enum MonitorEvent { | ||
| /// A monitor event containing an HTLCUpdate. | ||
| HTLCEvent(HTLCUpdate), | ||
| /// Indicates we broadcasted the channel's latest commitment transaction and thus closed the | ||
| /// channel. Holds information about the channel and why it was closed. | ||
| HolderForceClosedWithInfo { | ||
| /// The reason the channel was closed. | ||
| reason: ClosureReason, | ||
| /// The funding outpoint of the channel. | ||
| outpoint: OutPoint, | ||
| /// The channel ID of the channel. | ||
| channel_id: ChannelId, | ||
| }, | ||
| /// Indicates we broadcasted the channel's latest commitment transaction and thus closed the | ||
| /// channel. | ||
| HolderForceClosed(OutPoint), | ||
| @@ -184,6 +195,11 @@ impl_writeable_tlv_based_enum_upgradable!(MonitorEvent, | ||
| (2, monitor_update_id, required), | ||
| (4, channel_id, required), | ||
| }, | ||
| (5, HolderForceClosedWithInfo) => { | ||
| (0, reason, upgradable_required), | ||
| (2, outpoint, required), | ||
| (4, channel_id, required), | ||
| }, | ||
jkczyz marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| ; | ||
| (2, HTLCEvent), | ||
| (4, HolderForceClosed), | ||
| @@ -1059,6 +1075,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe | ||
| writer.write_all(&(self.pending_monitor_events.iter().filter(|ev| match ev { | ||
| MonitorEvent::HTLCEvent(_) => true, | ||
| MonitorEvent::HolderForceClosed(_) => true, | ||
| MonitorEvent::HolderForceClosedWithInfo { .. } => true, | ||
| _ => false, | ||
| }).count() as u64).to_be_bytes())?; | ||
| for event in self.pending_monitor_events.iter() { | ||
| @@ -1068,6 +1085,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe | ||
| upd.write(writer)?; | ||
| }, | ||
| MonitorEvent::HolderForceClosed(_) => 1u8.write(writer)?, | ||
| // `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. To keep | ||
| // backwards compatibility, we write a `HolderForceClosed` event along with the | ||
| // `HolderForceClosedWithInfo` event. This is deduplicated in the reader. | ||
| MonitorEvent::HolderForceClosedWithInfo { .. } => 1u8.write(writer)?, | ||
wpaulino marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| _ => {}, // Covered in the TLV writes below | ||
| } | ||
| } | ||
| @@ -1099,10 +1120,23 @@ impl<Signer: WriteableEcdsaChannelSigner> Writeable for ChannelMonitorImpl<Signe | ||
| self.lockdown_from_offchain.write(writer)?; | ||
| self.holder_tx_signed.write(writer)?; | ||
| // If we have a `HolderForceClosedWithInfo` event, we need to write the `HolderForceClosed` for backwards compatibility. | ||
| let pending_monitor_events = match self.pending_monitor_events.iter().find(|ev| match ev { | ||
| MonitorEvent::HolderForceClosedWithInfo { .. } => true, | ||
| _ => false, | ||
| }) { | ||
| Some(MonitorEvent::HolderForceClosedWithInfo { outpoint, .. }) => { | ||
| let mut pending_monitor_events = self.pending_monitor_events.clone(); | ||
| pending_monitor_events.push(MonitorEvent::HolderForceClosed(*outpoint)); | ||
| pending_monitor_events | ||
| } | ||
| _ => self.pending_monitor_events.clone(), | ||
| }; | ||
| write_tlv_fields!(writer, { | ||
| (1, self.funding_spend_confirmed, option), | ||
| (3, self.htlcs_resolved_on_chain, required_vec), | ||
| (5, pending_monitor_events, required_vec), | ||
| (7, self.funding_spend_seen, required), | ||
| (9, self.counterparty_node_id, option), | ||
| (11, self.confirmed_commitment_tx_counterparty_output, option), | ||
| @@ -2727,7 +2761,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | ||
| } | ||
| } | ||
| fn generate_claimable_outpoints_and_watch_outputs(&mut self, reason: ClosureReason) -> (Vec<PackageTemplate>, Vec<TransactionOutputs>) { | ||
| let funding_outp = HolderFundingOutput::build( | ||
| self.funding_redeemscript.clone(), | ||
| self.channel_value_satoshis, | ||
| @@ -2739,7 +2773,13 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | ||
| self.best_block.height, self.best_block.height | ||
| ); | ||
| let mut claimable_outpoints = vec![commitment_package]; | ||
| let event = MonitorEvent::HolderForceClosedWithInfo { | ||
| reason, | ||
| outpoint: self.funding_info.0, | ||
| channel_id: self.channel_id, | ||
| }; | ||
| self.pending_monitor_events.push(event); | ||
| // Although we aren't signing the transaction directly here, the transaction will be signed | ||
| // in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject | ||
| // new channel updates. | ||
| @@ -2775,7 +2815,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | ||
| F::Target: FeeEstimator, | ||
| L::Target: Logger, | ||
| { | ||
| let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs(ClosureReason::HolderForceClosed); | ||
| self.onchain_tx_handler.update_claims_view_from_requests( | ||
| claimable_outpoints, self.best_block.height, self.best_block.height, broadcaster, | ||
| fee_estimator, logger | ||
| @@ -3778,7 +3818,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> { | ||
| let should_broadcast = self.should_broadcast_holder_commitment_txn(logger); | ||
| if should_broadcast { | ||
| let (mut new_outpoints, mut new_outputs) = self.generate_claimable_outpoints_and_watch_outputs(ClosureReason::HTLCsTimedOut); | ||
| claimable_outpoints.append(&mut new_outpoints); | ||
| watch_outputs.append(&mut new_outputs); | ||
| } | ||
| @@ -4605,6 +4645,16 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP | ||
| (19, channel_id, option), | ||
| }); | ||
| // `HolderForceClosedWithInfo` replaced `HolderForceClosed` in v0.0.122. If we have both | ||
| // events, we can remove the `HolderForceClosed` event and just keep the `HolderForceClosedWithInfo`. | ||
| if let Some(ref mut pending_monitor_events) = pending_monitor_events { | ||
| if pending_monitor_events.iter().any(|e| matches!(e, MonitorEvent::HolderForceClosed(_))) && | ||
| pending_monitor_events.iter().any(|e| matches!(e, MonitorEvent::HolderForceClosedWithInfo { .. })) | ||
| { | ||
| pending_monitor_events.retain(|e| !matches!(e, MonitorEvent::HolderForceClosed(_))); | ||
| } | ||
| } | ||
| // Monitors for anchor outputs channels opened in v0.0.116 suffered from a bug in which the | ||
| // wrong `counterparty_payment_script` was being tracked. Fix it now on deserialization to | ||
| // give them a chance to recognize the spendable output. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -232,6 +232,8 @@ pub enum ClosureReason { | ||
| /// Another channel in the same funding batch closed before the funding transaction | ||
| /// was ready to be broadcast. | ||
| FundingBatchClosure, | ||
| /// One of our HTLCs timed out in a channel, causing us to force close the channel. | ||
| HTLCsTimedOut, | ||
| } | ||
| impl core::fmt::Display for ClosureReason { | ||
| @@ -241,7 +243,7 @@ impl core::fmt::Display for ClosureReason { | ||
| ClosureReason::CounterpartyForceClosed { peer_msg } => { | ||
| f.write_fmt(format_args!("counterparty force-closed with message: {}", peer_msg)) | ||
| }, | ||
| ClosureReason::HolderForceClosed => f.write_str("user force-closed the channel"), | ||
| ClosureReason::LegacyCooperativeClosure => f.write_str("the channel was cooperatively closed"), | ||
| ClosureReason::CounterpartyInitiatedCooperativeClosure => f.write_str("the channel was cooperatively closed by our peer"), | ||
| ClosureReason::LocallyInitiatedCooperativeClosure => f.write_str("the channel was cooperatively closed by us"), | ||
| @@ -255,6 +257,7 @@ impl core::fmt::Display for ClosureReason { | ||
| ClosureReason::OutdatedChannelManager => f.write_str("the ChannelManager read from disk was stale compared to ChannelMonitor(s)"), | ||
| ClosureReason::CounterpartyCoopClosedUnfundedChannel => f.write_str("the peer requested the unfunded channel be closed"), | ||
| ClosureReason::FundingBatchClosure => f.write_str("another channel in the same funding batch closed"), | ||
| ClosureReason::HTLCsTimedOut => f.write_str("htlcs on the channel timed out"), | ||
wpaulino marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| } | ||
| } | ||
| @@ -272,6 +275,7 @@ impl_writeable_tlv_based_enum_upgradable!(ClosureReason, | ||
| (15, FundingBatchClosure) => {}, | ||
| (17, CounterpartyInitiatedCooperativeClosure) => {}, | ||
| (19, LocallyInitiatedCooperativeClosure) => {}, | ||
| (21, HTLCsTimedOut) => {}, | ||
| ); | ||
| /// Intended destination of a failed HTLC as indicated in [`Event::HTLCHandlingFailed`]. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -7374,7 +7374,7 @@ where | ||
| self.fail_htlc_backwards_internal(&htlc_update.source, &htlc_update.payment_hash, &reason, receiver); | ||
| } | ||
| }, | ||
| MonitorEvent::HolderForceClosed(_) | MonitorEvent::HolderForceClosedWithInfo { .. } => { | ||
| let counterparty_node_id_opt = match counterparty_node_id { | ||
| Some(cp_id) => Some(cp_id), | ||
| None => { | ||
| @@ -7392,7 +7392,12 @@ where | ||
| let pending_msg_events = &mut peer_state.pending_msg_events; | ||
| if let hash_map::Entry::Occupied(chan_phase_entry) = peer_state.channel_by_id.entry(channel_id) { | ||
| if let ChannelPhase::Funded(mut chan) = remove_channel_phase!(self, chan_phase_entry) { | ||
| let reason = if let MonitorEvent::HolderForceClosedWithInfo { reason, .. } = monitor_event { | ||
| reason | ||
benthecarman marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } else { | ||
| ClosureReason::HolderForceClosed | ||
| }; | ||
Comment on lines
+7395
to
+7399
Contributor There was a problem hiding this comment. Could you expand upon this change in the commit message? Seems the previous reason used was incorrect? | ||
| failed_channels.push(chan.context.force_shutdown(false, reason.clone())); | ||
| if let Ok(update) = self.get_channel_update_for_broadcast(&chan) { | ||
| pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { | ||
| msg: update | ||
| @@ -7401,7 +7406,7 @@ where | ||
| pending_msg_events.push(events::MessageSendEvent::HandleError { | ||
| node_id: chan.context.get_counterparty_node_id(), | ||
| action: msgs::ErrorAction::DisconnectPeer { | ||
| msg: Some(msgs::ErrorMessage { channel_id: chan.context.channel_id(), data: reason.to_string() }) | ||
G8XSU marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| }, | ||
| }); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name this
HolderForceClosedand rename the existing variant toHolderForceClosedWithoutInfoinstead? I'm somewhat indifferent on doing this now but seems like the one used going forward shouldn't have a suffix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it makes more sense like this and it can be renamed when
HolderForceClosedis removed