Merge pull request #970 from TheBlueMatt/2021-06-no-confirmed-csv-delay
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Fri, 2 Jul 2021 17:55:17 +0000 (17:55 +0000)
committerGitHub <noreply@github.com>
Fri, 2 Jul 2021 17:55:17 +0000 (17:55 +0000)
Create SpendableOutputs events no matter the chain::Confirm order

1  2 
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs

index 1d3e264a3fcbf263eec75e3b85e8d0b65c81e024,403744c4f6ef38208d4e87f814d175e814155f5e..4051ebf3fbf6cc0d07dd29c4132c2dab3510bb43
@@@ -114,8 -114,8 +114,8 @@@ enum InboundHTLCState 
        /// commitment transaction without it as otherwise we'll have to force-close the channel to
        /// claim it before the timeout (obviously doesn't apply to revoked HTLCs that we can't claim
        /// anyway). That said, ChannelMonitor does this for us (see
-       /// ChannelMonitor::would_broadcast_at_height) so we actually remove the HTLC from our own
-       /// local state before then, once we're sure that the next commitment_signed and
+       /// ChannelMonitor::should_broadcast_holder_commitment_txn) so we actually remove the HTLC from
+       /// our own local state before then, once we're sure that the next commitment_signed and
        /// ChannelMonitor::provide_latest_local_commitment_tx will not include this HTLC.
        LocalRemoved(InboundHTLCRemovalReason),
  }
@@@ -288,7 -288,7 +288,7 @@@ impl HTLCCandidate 
  }
  
  /// Information needed for constructing an invoice route hint for this channel.
 -#[derive(Clone)]
 +#[derive(Clone, Debug, PartialEq)]
  pub struct CounterpartyForwardingInfo {
        /// Base routing fee in millisatoshis.
        pub fee_base_msat: u32,
@@@ -3513,7 -3513,7 +3513,7 @@@ impl<Signer: Sign> Channel<Signer> 
        /// is_usable() and considers things like the channel being temporarily disabled.
        /// Allowed in any state (including after shutdown)
        pub fn is_live(&self) -> bool {
 -              self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) == 0)
 +              self.is_usable() && (self.channel_state & (ChannelState::PeerDisconnected as u32) == 0)
        }
  
        /// Returns true if this channel has been marked as awaiting a monitor update to move forward.
  
        /// Adds a pending outbound HTLC to this channel, note that you probably want
        /// send_htlc_and_commit instead cause you'll want both messages at once.
 -      /// This returns an option instead of a pure UpdateAddHTLC as we may be in a state where we are
 -      /// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
 -      /// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
 -      /// You MUST call send_commitment prior to any other calls on this Channel
 +      ///
 +      /// This returns an optional UpdateAddHTLC as we may be in a state where we cannot add HTLCs on
 +      /// the wire:
 +      /// * 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.
 +      ///
 +      /// You MUST call send_commitment prior to calling any other methods on this Channel!
 +      ///
        /// If an Err is returned, it's a ChannelError::Ignore!
        pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
                        return Err(ChannelError::Ignore(format!("Cannot send less than their minimum HTLC value ({})", self.counterparty_htlc_minimum_msat)));
                }
  
 -              if (self.channel_state & (ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
 +              if (self.channel_state & (ChannelState::PeerDisconnected as u32)) != 0 {
                        // Note that this should never really happen, if we're !is_live() on receipt of an
                        // incoming HTLC for relay will result in us rejecting the HTLC and we won't allow
                        // the user to send directly into a !is_live() channel. However, if we
                        // disconnected during the time the previous hop was doing the commitment dance we may
                        // end up getting here after the forwarding delay. In any case, returning an
                        // IgnoreError will get ChannelManager to do the right thing and fail backwards now.
 -                      return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected/frozen for channel monitor update".to_owned()));
 +                      return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
                }
  
                let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
                }
  
                // Now update local state:
 -              if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 +              if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::MonitorUpdateFailed as u32)) != 0 {
                        self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
                                amount_msat,
                                payment_hash,
index a2eab3f5556de3a56247f3a9da3d071af03853cd,766b2394b7ad38e3566f56b72906f4efe96800b1..1f16781603ab43e5dc6f6ed0ae8a65b567a1c2d3
@@@ -626,13 -626,13 +626,13 @@@ pub const MIN_FINAL_CLTV_EXPIRY: u32 = 
  const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;
  
  // Check for ability of an attacker to make us fail on-chain by delaying an HTLC claim. See
- // ChannelMontior::would_broadcast_at_height for a description of why this is needed.
+ // ChannelMonitor::should_broadcast_holder_commitment_txn for a description of why this is needed.
  #[deny(const_err)]
  #[allow(dead_code)]
  const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
  
  /// Details of a channel, as returned by ChannelManager::list_channels and ChannelManager::list_usable_channels
 -#[derive(Clone)]
 +#[derive(Clone, Debug, PartialEq)]
  pub struct ChannelDetails {
        /// The channel's ID (prior to funding transaction generation, this is a random 32 bytes,
        /// thereafter this is the txid of the funding transaction xor the funding transaction output).
        /// point after the funding transaction received enough confirmations).
        pub is_funding_locked: bool,
        /// True if the channel is (a) confirmed and funding_locked messages have been exchanged, (b)
 -      /// the peer is connected, (c) no monitor update failure is pending resolution, and (d) the
 -      /// channel is not currently negotiating a shutdown.
 +      /// the peer is connected, and (c) the channel is not currently negotiating a shutdown.
        ///
        /// This is a strict superset of `is_funding_locked`.
        pub is_usable: bool,
@@@ -3345,15 -3346,14 +3345,15 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
                Ok(())
        }
  
 -      fn internal_channel_update(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelUpdate) -> Result<(), MsgHandleErrInternal> {
 +      /// Returns ShouldPersist if anything changed, otherwise either SkipPersist or an Err.
 +      fn internal_channel_update(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelUpdate) -> Result<NotifyOption, MsgHandleErrInternal> {
                let mut channel_state_lock = self.channel_state.lock().unwrap();
                let channel_state = &mut *channel_state_lock;
                let chan_id = match channel_state.short_to_id.get(&msg.contents.short_channel_id) {
                        Some(chan_id) => chan_id.clone(),
                        None => {
                                // It's not a local channel
 -                              return Ok(())
 +                              return Ok(NotifyOption::SkipPersist)
                        }
                };
                match channel_state.by_id.entry(chan_id) {
                                                // If the announcement is about a channel of ours which is public, some
                                                // other peer may simply be forwarding all its gossip to us. Don't provide
                                                // a scary-looking error message and return Ok instead.
 -                                              return Ok(());
 +                                              return Ok(NotifyOption::SkipPersist);
                                        }
                                        return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a channel_update for a channel from the wrong node - it shouldn't know about our private channels!".to_owned(), chan_id));
                                }
                        },
                        hash_map::Entry::Vacant(_) => unreachable!()
                }
 -              Ok(())
 +              Ok(NotifyOption::DoPersist)
        }
  
        fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
@@@ -4116,13 -4116,8 +4116,13 @@@ impl<Signer: Sign, M: Deref , T: Deref 
        }
  
        fn handle_channel_update(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelUpdate) {
 -              let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 -              let _ = handle_error!(self, self.internal_channel_update(counterparty_node_id, msg), *counterparty_node_id);
 +              PersistenceNotifierGuard::optionally_notify(&self.total_consistency_lock, &self.persistence_notifier, || {
 +                      if let Ok(persist) = handle_error!(self, self.internal_channel_update(counterparty_node_id, msg), *counterparty_node_id) {
 +                              persist
 +                      } else {
 +                              NotifyOption::SkipPersist
 +                      }
 +              });
        }
  
        fn handle_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) {
@@@ -4667,8 -4662,6 +4667,8 @@@ impl<'a, Signer: Sign, M: Deref, T: Der
                                                log_bytes!(channel.channel_id()), monitor.get_latest_update_id(), channel.get_latest_monitor_update_id());
                                        log_error!(args.logger, " The chain::Watch API *requires* that monitors are persisted durably before returning,");
                                        log_error!(args.logger, " client applications must ensure that ChannelMonitor data is always available and the latest to avoid funds loss!");
 +                                      log_error!(args.logger, " Without the latest ChannelMonitor we cannot continue without risking funds.");
 +                                      log_error!(args.logger, " Please ensure the chain::Watch API requirements are met and file a bug report at https://github.com/rust-bitcoin/rust-lightning");
                                        return Err(DecodeError::InvalidValue);
                                } else if channel.get_cur_holder_commitment_transaction_number() > monitor.get_cur_holder_commitment_number() ||
                                                channel.get_revoked_counterparty_commitment_transaction_number() > monitor.get_min_seen_secret() ||
                                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,");
                                log_error!(args.logger, " client applications must ensure that ChannelMonitor data is always available and the latest to avoid funds loss!");
 +                              log_error!(args.logger, " Without the ChannelMonitor we cannot continue without risking funds.");
 +                              log_error!(args.logger, " Please ensure the chain::Watch API requirements are met and file a bug report at https://github.com/rust-bitcoin/rust-lightning");
                                return Err(DecodeError::InvalidValue);
                        }
                }
@@@ -4832,9 -4823,6 +4832,9 @@@ mod tests 
        use core::sync::atomic::{AtomicBool, Ordering};
        use std::thread;
        use core::time::Duration;
 +      use ln::functional_test_utils::*;
 +      use ln::features::InitFeatures;
 +      use ln::msgs::ChannelMessageHandler;
  
        #[test]
        fn test_wait_timeout() {
                        }
                }
        }
 +
 +      #[test]
 +      fn test_notify_limits() {
 +              // Check that a few cases which don't require the persistence of a new ChannelManager,
 +              // indeed, do not cause the persistence of a new ChannelManager.
 +              let chanmon_cfgs = create_chanmon_cfgs(3);
 +              let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
 +              let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 +              let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 +
 +              let mut chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 +
 +              // We check that the channel info nodes have doesn't change too early, even though we try
 +              // to connect messages with new values
 +              chan.0.contents.fee_base_msat *= 2;
 +              chan.1.contents.fee_base_msat *= 2;
 +              let node_a_chan_info = nodes[0].node.list_channels()[0].clone();
 +              let node_b_chan_info = nodes[1].node.list_channels()[0].clone();
 +
 +              // The first two nodes (which opened a channel) should now require fresh persistence
 +              assert!(nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert!(nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              // ... but the last node should not.
 +              assert!(!nodes[2].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              // After persisting the first two nodes they should no longer need fresh persistence.
 +              assert!(!nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert!(!nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +
 +              // Node 3, unrelated to the only channel, shouldn't care if it receives a channel_update
 +              // about the channel.
 +              nodes[2].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &chan.0);
 +              nodes[2].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &chan.1);
 +              assert!(!nodes[2].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +
 +              // The nodes which are a party to the channel should also ignore messages from unrelated
 +              // parties.
 +              nodes[0].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.0);
 +              nodes[0].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.1);
 +              nodes[1].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.0);
 +              nodes[1].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.1);
 +              assert!(!nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert!(!nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +
 +              // At this point the channel info given by peers should still be the same.
 +              assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info);
 +              assert_eq!(nodes[1].node.list_channels()[0], node_b_chan_info);
 +      }
  }
  
  #[cfg(all(any(test, feature = "_test_utils"), feature = "unstable"))]