Merge pull request #976 from TheBlueMatt/2021-06-actionable-errors
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Thu, 1 Jul 2021 03:33:15 +0000 (03:33 +0000)
committerGitHub <noreply@github.com>
Thu, 1 Jul 2021 03:33:15 +0000 (03:33 +0000)
Make errors actionable when failing to deserialize a ChannelManager

1  2 
lightning/src/ln/channelmanager.rs

index 267f97cfa1521232be709a73bbfe361cbd7fa0a3,dc3654927e98003ad0cdc38f01b16fdbc8a05cd4..a2eab3f5556de3a56247f3a9da3d071af03853cd
@@@ -632,7 -632,7 +632,7 @@@ const CHECK_CLTV_EXPIRY_SANITY: u32 = M
  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,6 -4662,8 +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);
                        }
                }
@@@ -4828,9 -4827,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"))]