Merge pull request #1678 from TheBlueMatt/2022-08-funding-locked-mon-persist-fail
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Wed, 19 Oct 2022 16:55:01 +0000 (16:55 +0000)
committerGitHub <noreply@github.com>
Wed, 19 Oct 2022 16:55:01 +0000 (16:55 +0000)
Handle async initial ChannelMonitor persistence failing on restart

1  2 
lightning/src/ln/channelmanager.rs
lightning/src/util/events.rs

index 95771449ba47b1cf5e2001bf9e17b90b88776690,8f0f3811ba3b2bca0b6ac55167fea2fdfd890fcb..fa98e8e9b3fe3a859837c89bbcc468059594b7e5
@@@ -1971,7 -1971,7 +1971,7 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
                log_debug!(self.logger, "Finishing force-closure of channel with {} HTLCs to fail", failed_htlcs.len());
                for htlc_source in failed_htlcs.drain(..) {
                        let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source;
 -                      let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id: channel_id };
 +                      let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id };
                        self.fail_htlc_backwards_internal(source, &payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() }, receiver);
                }
                if let Some((funding_txo, monitor_update)) = monitor_update_option {
                                                                }
                                                        } else {
                                                                events::Event::ProbeFailed {
 -                                                                      payment_id: payment_id,
 +                                                                      payment_id,
                                                                        payment_hash: payment_hash.clone(),
                                                                        path: path.clone(),
                                                                        short_channel_id,
  
                                                if self.payment_is_probe(payment_hash, &payment_id) {
                                                        events::Event::ProbeFailed {
 -                                                              payment_id: payment_id,
 +                                                              payment_id,
                                                                payment_hash: payment_hash.clone(),
                                                                path: path.clone(),
                                                                short_channel_id: Some(scid),
@@@ -6499,7 -6499,7 +6499,7 @@@ impl Readable for HTLCSource 
                                }
                                Ok(HTLCSource::OutboundRoute {
                                        session_priv: session_priv.0.unwrap(),
 -                                      first_hop_htlc_msat: first_hop_htlc_msat,
 +                                      first_hop_htlc_msat,
                                        path: path.unwrap(),
                                        payment_id: payment_id.unwrap(),
                                        payment_secret,
@@@ -6921,6 -6921,16 +6921,16 @@@ impl<'a, Signer: Sign, M: Deref, T: Der
                                        }
                                        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,");
index 20f1c5b786cf66d4c90fbe0a28c6dca3d351f8dc,d2aa076bd21193b1c0d5526af51b2843de2ae88c..83e861889bc356bff2cd2305cfd92077e13b6ded
@@@ -15,7 -15,6 +15,7 @@@
  //! few other things.
  
  use chain::keysinterface::SpendableOutputDescriptor;
 +use ln::chan_utils::HTLCOutputInCommitment;
  use ln::channelmanager::PaymentId;
  use ln::channel::FUNDING_CONF_DEADLINE_BLOCKS;
  use ln::features::ChannelTypeFeatures;
@@@ -26,7 -25,7 +26,7 @@@ use routing::gossip::NetworkUpdate
  use util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper};
  use routing::router::{RouteHop, RouteParameters};
  
 -use bitcoin::{PackedLockTime, Transaction};
 +use bitcoin::{PackedLockTime, Transaction, OutPoint};
  use bitcoin::blockdata::script::Script;
  use bitcoin::hashes::Hash;
  use bitcoin::hashes::sha256::Hash as Sha256;
@@@ -75,7 -74,7 +75,7 @@@ impl_writeable_tlv_based_enum!(PaymentP
        (2, SpontaneousPayment)
  );
  
 -#[derive(Clone, Debug, PartialEq)]
 +#[derive(Clone, Debug, PartialEq, Eq)]
  /// The reason the channel was closed. See individual variants more details.
  pub enum ClosureReason {
        /// Closure generated from receiving a peer error message.
        /// 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
  }
  
@@@ -154,7 -161,7 +162,7 @@@ impl_writeable_tlv_based_enum_upgradabl
  );
  
  /// Intended destination of a failed HTLC as indicated in [`Event::HTLCHandlingFailed`].
 -#[derive(Clone, Debug, PartialEq)]
 +#[derive(Clone, Debug, PartialEq, Eq)]
  pub enum HTLCDestination {
        /// We tried forwarding to a channel but failed to do so. An example of such an instance is when
        /// there is insufficient capacity in our outbound channel.
@@@ -197,84 -204,6 +205,84 @@@ impl_writeable_tlv_based_enum_upgradabl
        }
  );
  
 +/// A descriptor used to sign for a commitment transaction's anchor output.
 +#[derive(Clone, Debug)]
 +pub struct AnchorDescriptor {
 +      /// A unique identifier used along with `channel_value_satoshis` to re-derive the
 +      /// [`InMemorySigner`] required to sign `input`.
 +      ///
 +      /// [`InMemorySigner`]: crate::chain::keysinterface::InMemorySigner
 +      pub channel_keys_id: [u8; 32],
 +      /// The value in satoshis of the channel we're attempting to spend the anchor output of. This is
 +      /// used along with `channel_keys_id` to re-derive the [`InMemorySigner`] required to sign
 +      /// `input`.
 +      ///
 +      /// [`InMemorySigner`]: crate::chain::keysinterface::InMemorySigner
 +      pub channel_value_satoshis: u64,
 +      /// The transaction input's outpoint corresponding to the commitment transaction's anchor
 +      /// output.
 +      pub outpoint: OutPoint,
 +}
 +
 +/// Represents the different types of transactions, originating from LDK, to be bumped.
 +#[derive(Clone, Debug)]
 +pub enum BumpTransactionEvent {
 +      /// Indicates that a channel featuring anchor outputs is to be closed by broadcasting the local
 +      /// commitment transaction. Since commitment transactions have a static feerate pre-agreed upon,
 +      /// they may need additional fees to be attached through a child transaction using the popular
 +      /// [Child-Pays-For-Parent](https://bitcoinops.org/en/topics/cpfp) fee bumping technique. This
 +      /// child transaction must include the anchor input described within `anchor_descriptor` along
 +      /// with additional inputs to meet the target feerate. Failure to meet the target feerate
 +      /// decreases the confirmation odds of the transaction package (which includes the commitment
 +      /// and child anchor transactions), possibly resulting in a loss of funds. Once the transaction
 +      /// is constructed, it must be fully signed for and broadcasted by the consumer of the event
 +      /// along with the `commitment_tx` enclosed. Note that the `commitment_tx` must always be
 +      /// broadcast first, as the child anchor transaction depends on it.
 +      ///
 +      /// The consumer should be able to sign for any of the additional inputs included within the
 +      /// child anchor transaction. To sign its anchor input, an [`InMemorySigner`] should be
 +      /// re-derived through [`KeysManager::derive_channel_keys`] with the help of
 +      /// [`AnchorDescriptor::channel_keys_id`] and [`AnchorDescriptor::channel_value_satoshis`].
 +      ///
 +      /// It is possible to receive more than one instance of this event if a valid child anchor
 +      /// transaction is never broadcast or is but not with a sufficient fee to be mined. Care should
 +      /// be taken by the consumer of the event to ensure any future iterations of the child anchor
 +      /// transaction adhere to the [Replace-By-Fee
 +      /// rules](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md)
 +      /// for fee bumps to be accepted into the mempool, and eventually the chain. As the frequency of
 +      /// these events is not user-controlled, users may ignore/drop the event if they are no longer
 +      /// able to commit external confirmed funds to the child anchor transaction.
 +      ///
 +      /// The set of `pending_htlcs` on the commitment transaction to be broadcast can be inspected to
 +      /// determine whether a significant portion of the channel's funds are allocated to HTLCs,
 +      /// enabling users to make their own decisions regarding the importance of the commitment
 +      /// transaction's confirmation. Note that this is not required, but simply exists as an option
 +      /// for users to override LDK's behavior. On commitments with no HTLCs (indicated by those with
 +      /// an empty `pending_htlcs`), confirmation of the commitment transaction can be considered to
 +      /// be not urgent.
 +      ///
 +      /// [`InMemorySigner`]: crate::chain::keysinterface::InMemorySigner
 +      /// [`KeysManager::derive_channel_keys`]: crate::chain::keysinterface::KeysManager::derive_channel_keys
 +      ChannelClose {
 +              /// The target feerate that the transaction package, which consists of the commitment
 +              /// transaction and the to-be-crafted child anchor transaction, must meet.
 +              package_target_feerate_sat_per_1000_weight: u32,
 +              /// The channel's commitment transaction to bump the fee of. This transaction should be
 +              /// broadcast along with the anchor transaction constructed as a result of consuming this
 +              /// event.
 +              commitment_tx: Transaction,
 +              /// The absolute fee in satoshis of the commitment transaction. This can be used along the
 +              /// with weight of the commitment transaction to determine its feerate.
 +              commitment_tx_fee_satoshis: u64,
 +              /// The descriptor to sign the anchor input of the anchor transaction constructed as a
 +              /// result of consuming this event.
 +              anchor_descriptor: AnchorDescriptor,
 +              /// The set of pending HTLCs on the commitment transaction that need to be resolved once the
 +              /// commitment transaction confirms.
 +              pending_htlcs: Vec<HTLCOutputInCommitment>,
 +      },
 +}
 +
  /// An Event which you should probably take some action in response to.
  ///
  /// Note that while Writeable and Readable are implemented for Event, you probably shouldn't use
@@@ -681,13 -610,6 +689,13 @@@ pub enum Event 
                /// Destination of the HTLC that failed to be processed.
                failed_next_destination: HTLCDestination,
        },
 +      #[cfg(anchors)]
 +      /// Indicates that a transaction originating from LDK needs to have its fee bumped. This event
 +      /// requires confirmed external funds to be readily available to spend.
 +      ///
 +      /// LDK does not currently generate this event. It is limited to the scope of channels with
 +      /// anchor outputs, which will be introduced in a future release.
 +      BumpTransaction(BumpTransactionEvent),
  }
  
  impl Writeable for Event {
                                        (2, failed_next_destination, required),
                                })
                        },
 +                      #[cfg(anchors)]
 +                      &Event::BumpTransaction(ref event)=> {
 +                              27u8.write(writer)?;
 +                              match event {
 +                                      // We never write the ChannelClose events as they'll be replayed upon restarting
 +                                      // anyway if the commitment transaction remains unconfirmed.
 +                                      BumpTransactionEvent::ChannelClose { .. } => {}
 +                              }
 +                      }
                        // Note that, going forward, all new events must only write data inside of
                        // `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write
                        // data via `write_tlv_fields`.