Merge pull request #970 from TheBlueMatt/2021-06-no-confirmed-csv-delay
[rust-lightning] / lightning / src / ln / channelmanager.rs
index a9e714cea7bf245b9bc536f87221ffbcfa57a183..1f16781603ab43e5dc6f6ed0ae8a65b567a1c2d3 100644 (file)
@@ -626,13 +626,13 @@ pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER + 3;
 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).
@@ -677,8 +677,7 @@ pub struct ChannelDetails {
        /// 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,
@@ -3346,14 +3345,15 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                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) {
@@ -3363,7 +3363,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                // 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));
                                }
@@ -3371,11 +3371,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        },
                        hash_map::Entry::Vacant(_) => unreachable!()
                }
-               Ok(())
+               Ok(NotifyOption::DoPersist)
        }
 
        fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
-               let (htlcs_failed_forward, chan_restoration_res) = {
+               let (htlcs_failed_forward, need_lnd_workaround, chan_restoration_res) = {
                        let mut channel_state_lock = self.channel_state.lock().unwrap();
                        let channel_state = &mut *channel_state_lock;
 
@@ -3396,13 +3396,19 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        msg,
                                                });
                                        }
-                                       (htlcs_failed_forward, handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked))
+                                       let need_lnd_workaround = chan.get_mut().workaround_lnd_bug_4006.take();
+                                       (htlcs_failed_forward, need_lnd_workaround,
+                                               handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked))
                                },
                                hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
                        }
                };
                post_handle_chan_restoration!(self, chan_restoration_res);
                self.fail_holding_cell_htlcs(htlcs_failed_forward, msg.channel_id);
+
+               if let Some(funding_locked_msg) = need_lnd_workaround {
+                       self.internal_funding_locked(counterparty_node_id, &funding_locked_msg)?;
+               }
                Ok(())
        }
 
@@ -4110,8 +4116,13 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: 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) {
@@ -4331,22 +4342,22 @@ const MIN_SERIALIZATION_VERSION: u8 = 1;
 
 impl_writeable_tlv_based_enum!(PendingHTLCRouting,
        (0, Forward) => {
-               (0, onion_packet),
-               (2, short_channel_id),
-       }, {}, {},
+               (0, onion_packet, required),
+               (2, short_channel_id, required),
+       },
        (1, Receive) => {
-               (0, payment_data),
-               (2, incoming_cltv_expiry),
-       }, {}, {}
+               (0, payment_data, required),
+               (2, incoming_cltv_expiry, required),
+       }
 ;);
 
 impl_writeable_tlv_based!(PendingHTLCInfo, {
-       (0, routing),
-       (2, incoming_shared_secret),
-       (4, payment_hash),
-       (6, amt_to_forward),
-       (8, outgoing_cltv_value)
-}, {}, {});
+       (0, routing, required),
+       (2, incoming_shared_secret, required),
+       (4, payment_hash, required),
+       (6, amt_to_forward, required),
+       (8, outgoing_cltv_value, required)
+});
 
 impl_writeable_tlv_based_enum!(HTLCFailureMsg, ;
        (0, Relay),
@@ -4358,60 +4369,58 @@ impl_writeable_tlv_based_enum!(PendingHTLCStatus, ;
 );
 
 impl_writeable_tlv_based!(HTLCPreviousHopData, {
-       (0, short_channel_id),
-       (2, outpoint),
-       (4, htlc_id),
-       (6, incoming_packet_shared_secret)
-}, {}, {});
+       (0, short_channel_id, required),
+       (2, outpoint, required),
+       (4, htlc_id, required),
+       (6, incoming_packet_shared_secret, required)
+});
 
 impl_writeable_tlv_based!(ClaimableHTLC, {
-       (0, prev_hop),
-       (2, value),
-       (4, payment_data),
-       (6, cltv_expiry),
-}, {}, {});
+       (0, prev_hop, required),
+       (2, value, required),
+       (4, payment_data, required),
+       (6, cltv_expiry, required),
+});
 
 impl_writeable_tlv_based_enum!(HTLCSource,
        (0, OutboundRoute) => {
-               (0, session_priv),
-               (2, first_hop_htlc_msat),
-       }, {}, {
-               (4, path),
-       };
+               (0, session_priv, required),
+               (2, first_hop_htlc_msat, required),
+               (4, path, vec_type),
+       }, ;
        (1, PreviousHopData)
 );
 
 impl_writeable_tlv_based_enum!(HTLCFailReason,
        (0, LightningError) => {
-               (0, err),
-       }, {}, {},
+               (0, err, required),
+       },
        (1, Reason) => {
-               (0, failure_code),
-       }, {}, {
-               (2, data),
+               (0, failure_code, required),
+               (2, data, vec_type),
        },
 ;);
 
 impl_writeable_tlv_based_enum!(HTLCForwardInfo,
        (0, AddHTLC) => {
-               (0, forward_info),
-               (2, prev_short_channel_id),
-               (4, prev_htlc_id),
-               (6, prev_funding_outpoint),
-       }, {}, {},
+               (0, forward_info, required),
+               (2, prev_short_channel_id, required),
+               (4, prev_htlc_id, required),
+               (6, prev_funding_outpoint, required),
+       },
        (1, FailHTLC) => {
-               (0, htlc_id),
-               (2, err_packet),
-       }, {}, {},
+               (0, htlc_id, required),
+               (2, err_packet, required),
+       },
 ;);
 
 impl_writeable_tlv_based!(PendingInboundPayment, {
-       (0, payment_secret),
-       (2, expiry_time),
-       (4, user_payment_id),
-       (6, payment_preimage),
-       (8, min_value_msat),
-}, {}, {});
+       (0, payment_secret, required),
+       (2, expiry_time, required),
+       (4, user_payment_id, required),
+       (6, payment_preimage, required),
+       (8, min_value_msat, required),
+});
 
 impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable for ChannelManager<Signer, M, T, K, F, L>
        where M::Target: chain::Watch<Signer>,
@@ -4506,7 +4515,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
                        session_priv.write(writer)?;
                }
 
-               write_tlv_fields!(writer, {}, {});
+               write_tlv_fields!(writer, {});
 
                Ok(())
        }
@@ -4658,6 +4667,8 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                                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() ||
@@ -4677,6 +4688,8 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                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);
                        }
                }
@@ -4759,7 +4772,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                        }
                }
 
-               read_tlv_fields!(reader, {}, {});
+               read_tlv_fields!(reader, {});
 
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&args.keys_manager.get_secure_random_bytes());
@@ -4819,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() {
@@ -4861,6 +4877,53 @@ mod tests {
                        }
                }
        }
+
+       #[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"))]