Merge pull request #1202 from TheBlueMatt/2021-12-fix-retries-races
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Wed, 15 Dec 2021 04:58:46 +0000 (04:58 +0000)
committerGitHub <noreply@github.com>
Wed, 15 Dec 2021 04:58:46 +0000 (04:58 +0000)
Fix payment retry races and inform users when a payment fails

.github/workflows/build.yml
fuzz/src/router.rs
lightning-background-processor/src/lib.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/features.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/msgs.rs
lightning/src/ln/peer_handler.rs
lightning/src/routing/network_graph.rs
lightning/src/routing/router.rs

index 260767333f3c599d878eec70683e8875f5195e7d..4b9f23ff091b718d1d918f9e6b43922cf4070685 100644 (file)
@@ -172,7 +172,7 @@ jobs:
           done
       - name: Upload coverage
         if: matrix.coverage
-        uses: codecov/codecov-action@v1
+        uses: codecov/codecov-action@v2
         with:
           # Could you use this to fake the coverage report for your PR? Sure.
           # Will anyone be impressed by your amazing coverage? No
index 149d40134f5102165aaaa00440079352fcbcd737..d6aa97e42e7fc3939c327501017080b03e2552cb 100644 (file)
@@ -223,6 +223,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                                                                force_close_spend_delay: None,
                                                                is_outbound: true, is_funding_locked: true,
                                                                is_usable: true, is_public: true,
+                                                               balance_msat: 0,
                                                                outbound_capacity_msat: 0,
                                                        });
                                                }
index 39ecc316a5a88c858144ed6d3b65a8ee3889997a..4e98ba9ea0fc81b032f75b29a31f7612e8722bde 100644 (file)
@@ -493,9 +493,10 @@ mod tests {
 
                macro_rules! check_persisted_data {
                        ($node: expr, $filepath: expr, $expected_bytes: expr) => {
-                               match $node.write(&mut $expected_bytes) {
-                                       Ok(()) => {
-                                               loop {
+                               loop {
+                                       $expected_bytes.clear();
+                                       match $node.write(&mut $expected_bytes) {
+                                               Ok(()) => {
                                                        match std::fs::read($filepath) {
                                                                Ok(bytes) => {
                                                                        if bytes == $expected_bytes {
@@ -506,9 +507,9 @@ mod tests {
                                                                },
                                                                Err(_) => continue
                                                        }
-                                               }
-                                       },
-                                       Err(e) => panic!("Unexpected error: {}", e)
+                                               },
+                                               Err(e) => panic!("Unexpected error: {}", e)
+                                       }
                                }
                        }
                }
index ae5df9a1507c78058d4d94d5ed06e79ef272668d..0cdcb0c14c2b0665f45c272e244f714208e29001 100644 (file)
@@ -481,9 +481,14 @@ pub(super) struct Channel<Signer: Sign> {
        holding_cell_update_fee: Option<u32>,
        next_holder_htlc_id: u64,
        next_counterparty_htlc_id: u64,
-       update_time_counter: u32,
        feerate_per_kw: u32,
 
+       /// The timestamp set on our latest `channel_update` message for this channel. It is updated
+       /// when the channel is updated in ways which may impact the `channel_update` message or when a
+       /// new block is received, ensuring it's always at least moderately close to the current real
+       /// time.
+       update_time_counter: u32,
+
        #[cfg(debug_assertions)]
        /// Max to_local and to_remote outputs in a locally-generated commitment transaction
        holder_max_commitment_tx_output: Mutex<(u64, u64)>,
@@ -862,8 +867,12 @@ impl<Signer: Sign> Channel<Signer> {
                where F::Target: FeeEstimator
        {
                let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-               if feerate_per_kw < lower_limit {
-                       return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit)));
+               // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
+               // occasional issues with feerate disagreements between an initiator that wants a feerate
+               // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
+               // sat/kw before the comparison here.
+               if feerate_per_kw + 250 < lower_limit {
+                       return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
                }
                // We only bound the fee updates on the upper side to prevent completely absurd feerates,
                // always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee.
@@ -2118,6 +2127,8 @@ impl<Signer: Sign> Channel<Signer> {
        /// Doesn't bother handling the
        /// if-we-removed-it-already-but-haven't-fully-resolved-they-can-still-send-an-inbound-HTLC
        /// corner case properly.
+       /// The channel reserve is subtracted from each balance.
+       /// See also [`Channel::get_balance_msat`]
        pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {
                // Note that we have to handle overflow due to the above case.
                (
@@ -2133,6 +2144,14 @@ impl<Signer: Sign> Channel<Signer> {
                )
        }
 
+       /// Get our total balance in msat.
+       /// This is the amount that would go to us if we close the channel, ignoring any on-chain fees.
+       /// See also [`Channel::get_inbound_outbound_available_balance_msat`]
+       pub fn get_balance_msat(&self) -> u64 {
+               self.value_to_self_msat
+                       - self.get_outbound_pending_htlc_stats(None).pending_htlcs_value_msat
+       }
+
        pub fn get_holder_counterparty_selected_channel_reserve_satoshis(&self) -> (u64, Option<u64>) {
                (self.holder_selected_channel_reserve_satoshis, self.counterparty_selected_channel_reserve_satoshis)
        }
@@ -4178,6 +4197,7 @@ impl<Signer: Sign> Channel<Signer> {
        }
 
        pub fn set_channel_update_status(&mut self, status: ChannelUpdateStatus) {
+               self.update_time_counter += 1;
                self.channel_update_status = status;
        }
 
index bd3b3683e8900dca43f0819d7b4dee9040dca29e..47d705fe625b4889d7d3da19681af0d4272b8807 100644 (file)
@@ -911,17 +911,30 @@ pub struct ChannelDetails {
        pub unspendable_punishment_reserve: Option<u64>,
        /// The `user_channel_id` passed in to create_channel, or 0 if the channel was inbound.
        pub user_channel_id: u64,
+       /// Our total balance.  This is the amount we would get if we close the channel.
+       /// This value is not exact. Due to various in-flight changes and feerate changes, exactly this
+       /// amount is not likely to be recoverable on close.
+       ///
+       /// This does not include any pending HTLCs which are not yet fully resolved (and, thus, whose
+       /// balance is not available for inclusion in new outbound HTLCs). This further does not include
+       /// any pending outgoing HTLCs which are awaiting some other resolution to be sent.
+       /// This does not consider any on-chain fees.
+       ///
+       /// See also [`ChannelDetails::outbound_capacity_msat`]
+       pub balance_msat: u64,
        /// The available outbound capacity for sending HTLCs to the remote peer. This does not include
-       /// any pending HTLCs which are not yet fully resolved (and, thus, who's balance is not
+       /// any pending HTLCs which are not yet fully resolved (and, thus, whose balance is not
        /// available for inclusion in new outbound HTLCs). This further does not include any pending
        /// outgoing HTLCs which are awaiting some other resolution to be sent.
        ///
+       /// See also [`ChannelDetails::balance_msat`]
+       ///
        /// This value is not exact. Due to various in-flight changes, feerate changes, and our
        /// conflict-avoidance policy, exactly this amount is not likely to be spendable. However, we
        /// should be able to spend nearly this amount.
        pub outbound_capacity_msat: u64,
        /// The available inbound capacity for the remote peer to send HTLCs to us. This does not
-       /// include any pending HTLCs which are not yet fully resolved (and, thus, who's balance is not
+       /// include any pending HTLCs which are not yet fully resolved (and, thus, whose balance is not
        /// available for inclusion in new inbound HTLCs).
        /// Note that there are some corner cases not fully handled here, so the actual available
        /// inbound capacity may be slightly higher than this.
@@ -1492,6 +1505,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        res.reserve(channel_state.by_id.len());
                        for (channel_id, channel) in channel_state.by_id.iter().filter(f) {
                                let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
+                               let balance_msat = channel.get_balance_msat();
                                let (to_remote_reserve_satoshis, to_self_reserve_satoshis) =
                                        channel.get_holder_counterparty_selected_channel_reserve_satoshis();
                                res.push(ChannelDetails {
@@ -1506,6 +1520,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        short_channel_id: channel.get_short_channel_id(),
                                        channel_value_satoshis: channel.get_value_satoshis(),
                                        unspendable_punishment_reserve: to_self_reserve_satoshis,
+                                       balance_msat,
                                        inbound_capacity_msat,
                                        outbound_capacity_msat,
                                        user_channel_id: channel.get_user_id(),
index 32ba9de758632036942318c756f64708793e1b1a..dab841327802fc8ae3ee0c869395a7ce378c4148 100644 (file)
@@ -126,6 +126,10 @@ mod sealed {
                        ,
                        // Byte 3
                        ,
+                       // Byte 4
+                       ,
+                       // Byte 5
+                       ,
                ],
                optional_features: [
                        // Byte 0
@@ -136,6 +140,10 @@ mod sealed {
                        BasicMPP,
                        // Byte 3
                        ShutdownAnySegwit,
+                       // Byte 4
+                       ,
+                       // Byte 5
+                       ChannelType,
                ],
        });
        define_context!(NodeContext {
@@ -167,7 +175,7 @@ mod sealed {
                        // Byte 4
                        ,
                        // Byte 5
-                       ,
+                       ChannelType,
                        // Byte 6
                        Keysend,
                ],
@@ -223,7 +231,7 @@ mod sealed {
        /// useful for manipulating feature flags.
        macro_rules! define_feature {
                ($odd_bit: expr, $feature: ident, [$($context: ty),+], $doc: expr, $optional_setter: ident,
-                $required_setter: ident) => {
+                $required_setter: ident, $supported_getter: ident) => {
                        #[doc = $doc]
                        ///
                        /// See [BOLT #9] for details.
@@ -320,6 +328,11 @@ mod sealed {
                                        <T as $feature>::set_required_bit(&mut self.flags);
                                        self
                                }
+
+                               /// Checks if this feature is supported.
+                               pub fn $supported_getter(&self) -> bool {
+                                       <T as $feature>::supports_feature(&self.flags)
+                               }
                        }
 
                        $(
@@ -331,41 +344,60 @@ mod sealed {
                                        const ASSERT_ODD_BIT_PARITY: usize = (<Self as $feature>::ODD_BIT % 2) - 1;
                                }
                        )*
-
+               };
+               ($odd_bit: expr, $feature: ident, [$($context: ty),+], $doc: expr, $optional_setter: ident,
+                $required_setter: ident, $supported_getter: ident, $required_getter: ident) => {
+                       define_feature!($odd_bit, $feature, [$($context),+], $doc, $optional_setter, $required_setter, $supported_getter);
+                       impl <T: $feature> Features<T> {
+                               /// Checks if this feature is required.
+                               pub fn $required_getter(&self) -> bool {
+                                       <T as $feature>::requires_feature(&self.flags)
+                               }
+                       }
                }
        }
 
        define_feature!(1, DataLossProtect, [InitContext, NodeContext],
                "Feature flags for `option_data_loss_protect`.", set_data_loss_protect_optional,
-               set_data_loss_protect_required);
+               set_data_loss_protect_required, supports_data_loss_protect, requires_data_loss_protect);
        // NOTE: Per Bolt #9, initial_routing_sync has no even bit.
        define_feature!(3, InitialRoutingSync, [InitContext], "Feature flags for `initial_routing_sync`.",
-               set_initial_routing_sync_optional, set_initial_routing_sync_required);
+               set_initial_routing_sync_optional, set_initial_routing_sync_required,
+               initial_routing_sync);
        define_feature!(5, UpfrontShutdownScript, [InitContext, NodeContext],
                "Feature flags for `option_upfront_shutdown_script`.", set_upfront_shutdown_script_optional,
-               set_upfront_shutdown_script_required);
+               set_upfront_shutdown_script_required, supports_upfront_shutdown_script,
+               requires_upfront_shutdown_script);
        define_feature!(7, GossipQueries, [InitContext, NodeContext],
-               "Feature flags for `gossip_queries`.", set_gossip_queries_optional, set_gossip_queries_required);
+               "Feature flags for `gossip_queries`.", set_gossip_queries_optional, set_gossip_queries_required,
+               supports_gossip_queries, requires_gossip_queries);
        define_feature!(9, VariableLengthOnion, [InitContext, NodeContext, InvoiceContext],
                "Feature flags for `var_onion_optin`.", set_variable_length_onion_optional,
-               set_variable_length_onion_required);
+               set_variable_length_onion_required, supports_variable_length_onion,
+               requires_variable_length_onion);
        define_feature!(13, StaticRemoteKey, [InitContext, NodeContext, ChannelTypeContext],
                "Feature flags for `option_static_remotekey`.", set_static_remote_key_optional,
-               set_static_remote_key_required);
+               set_static_remote_key_required, supports_static_remote_key, requires_static_remote_key);
        define_feature!(15, PaymentSecret, [InitContext, NodeContext, InvoiceContext],
-               "Feature flags for `payment_secret`.", set_payment_secret_optional, set_payment_secret_required);
+               "Feature flags for `payment_secret`.", set_payment_secret_optional, set_payment_secret_required,
+               supports_payment_secret, requires_payment_secret);
        define_feature!(17, BasicMPP, [InitContext, NodeContext, InvoiceContext],
-               "Feature flags for `basic_mpp`.", set_basic_mpp_optional, set_basic_mpp_required);
+               "Feature flags for `basic_mpp`.", set_basic_mpp_optional, set_basic_mpp_required,
+               supports_basic_mpp, requires_basic_mpp);
        define_feature!(27, ShutdownAnySegwit, [InitContext, NodeContext],
                "Feature flags for `opt_shutdown_anysegwit`.", set_shutdown_any_segwit_optional,
-               set_shutdown_any_segwit_required);
+               set_shutdown_any_segwit_required, supports_shutdown_anysegwit, requires_shutdown_anysegwit);
+       define_feature!(45, ChannelType, [InitContext, NodeContext],
+               "Feature flags for `option_channel_type`.", set_channel_type_optional,
+               set_channel_type_required, supports_channel_type, requires_channel_type);
        define_feature!(55, Keysend, [NodeContext],
-               "Feature flags for keysend payments.", set_keysend_optional, set_keysend_required);
+               "Feature flags for keysend payments.", set_keysend_optional, set_keysend_required,
+               supports_keysend, requires_keysend);
 
        #[cfg(test)]
        define_feature!(123456789, UnknownFeature, [NodeContext, ChannelContext, InvoiceContext],
                "Feature flags for an unknown feature used in testing.", set_unknown_feature_optional,
-               set_unknown_feature_required);
+               set_unknown_feature_required, supports_unknown_test_feature, requires_unknown_test_feature);
 }
 
 /// Tracks the set of features which a node implements, templated by the context in which it
@@ -662,25 +694,7 @@ impl<T: sealed::Context> Features<T> {
        }
 }
 
-impl<T: sealed::DataLossProtect> Features<T> {
-       #[cfg(test)]
-       pub(crate) fn requires_data_loss_protect(&self) -> bool {
-               <T as sealed::DataLossProtect>::requires_feature(&self.flags)
-       }
-       #[cfg(test)]
-       pub(crate) fn supports_data_loss_protect(&self) -> bool {
-               <T as sealed::DataLossProtect>::supports_feature(&self.flags)
-       }
-}
-
 impl<T: sealed::UpfrontShutdownScript> Features<T> {
-       #[cfg(test)]
-       pub(crate) fn requires_upfront_shutdown_script(&self) -> bool {
-               <T as sealed::UpfrontShutdownScript>::requires_feature(&self.flags)
-       }
-       pub(crate) fn supports_upfront_shutdown_script(&self) -> bool {
-               <T as sealed::UpfrontShutdownScript>::supports_feature(&self.flags)
-       }
        #[cfg(test)]
        pub(crate) fn clear_upfront_shutdown_script(mut self) -> Self {
                <T as sealed::UpfrontShutdownScript>::clear_bits(&mut self.flags);
@@ -690,13 +704,6 @@ impl<T: sealed::UpfrontShutdownScript> Features<T> {
 
 
 impl<T: sealed::GossipQueries> Features<T> {
-       #[cfg(test)]
-       pub(crate) fn requires_gossip_queries(&self) -> bool {
-               <T as sealed::GossipQueries>::requires_feature(&self.flags)
-       }
-       pub(crate) fn supports_gossip_queries(&self) -> bool {
-               <T as sealed::GossipQueries>::supports_feature(&self.flags)
-       }
        #[cfg(test)]
        pub(crate) fn clear_gossip_queries(mut self) -> Self {
                <T as sealed::GossipQueries>::clear_bits(&mut self.flags);
@@ -704,30 +711,7 @@ impl<T: sealed::GossipQueries> Features<T> {
        }
 }
 
-impl<T: sealed::VariableLengthOnion> Features<T> {
-       #[cfg(test)]
-       pub(crate) fn requires_variable_length_onion(&self) -> bool {
-               <T as sealed::VariableLengthOnion>::requires_feature(&self.flags)
-       }
-       pub(crate) fn supports_variable_length_onion(&self) -> bool {
-               <T as sealed::VariableLengthOnion>::supports_feature(&self.flags)
-       }
-}
-
-impl<T: sealed::StaticRemoteKey> Features<T> {
-       pub(crate) fn supports_static_remote_key(&self) -> bool {
-               <T as sealed::StaticRemoteKey>::supports_feature(&self.flags)
-       }
-       #[cfg(test)]
-       pub(crate) fn requires_static_remote_key(&self) -> bool {
-               <T as sealed::StaticRemoteKey>::requires_feature(&self.flags)
-       }
-}
-
 impl<T: sealed::InitialRoutingSync> Features<T> {
-       pub(crate) fn initial_routing_sync(&self) -> bool {
-               <T as sealed::InitialRoutingSync>::supports_feature(&self.flags)
-       }
        // We are no longer setting initial_routing_sync now that gossip_queries
        // is enabled. This feature is ignored by a peer when gossip_queries has 
        // been negotiated.
@@ -737,32 +721,7 @@ impl<T: sealed::InitialRoutingSync> Features<T> {
        }
 }
 
-impl<T: sealed::PaymentSecret> Features<T> {
-       #[cfg(test)]
-       pub(crate) fn requires_payment_secret(&self) -> bool {
-               <T as sealed::PaymentSecret>::requires_feature(&self.flags)
-       }
-       /// Returns whether the `payment_secret` feature is supported.
-       pub fn supports_payment_secret(&self) -> bool {
-               <T as sealed::PaymentSecret>::supports_feature(&self.flags)
-       }
-}
-
-impl<T: sealed::BasicMPP> Features<T> {
-       #[cfg(test)]
-       pub(crate) fn requires_basic_mpp(&self) -> bool {
-               <T as sealed::BasicMPP>::requires_feature(&self.flags)
-       }
-       // We currently never test for this since we don't actually *generate* multipath routes.
-       pub(crate) fn supports_basic_mpp(&self) -> bool {
-               <T as sealed::BasicMPP>::supports_feature(&self.flags)
-       }
-}
-
 impl<T: sealed::ShutdownAnySegwit> Features<T> {
-       pub(crate) fn supports_shutdown_anysegwit(&self) -> bool {
-               <T as sealed::ShutdownAnySegwit>::supports_feature(&self.flags)
-       }
        #[cfg(test)]
        pub(crate) fn clear_shutdown_anysegwit(mut self) -> Self {
                <T as sealed::ShutdownAnySegwit>::clear_bits(&mut self.flags);
@@ -859,6 +818,11 @@ mod tests {
                assert!(!NodeFeatures::known().requires_basic_mpp());
                assert!(!InvoiceFeatures::known().requires_basic_mpp());
 
+               assert!(InitFeatures::known().supports_channel_type());
+               assert!(NodeFeatures::known().supports_channel_type());
+               assert!(!InitFeatures::known().requires_channel_type());
+               assert!(!NodeFeatures::known().requires_channel_type());
+
                assert!(InitFeatures::known().supports_shutdown_anysegwit());
                assert!(NodeFeatures::known().supports_shutdown_anysegwit());
 
@@ -897,11 +861,15 @@ mod tests {
                        // - var_onion_optin (req) | static_remote_key (req) | payment_secret(req)
                        // - basic_mpp
                        // - opt_shutdown_anysegwit
-                       assert_eq!(node_features.flags.len(), 4);
+                       // -
+                       // - option_channel_type
+                       assert_eq!(node_features.flags.len(), 6);
                        assert_eq!(node_features.flags[0], 0b00000010);
                        assert_eq!(node_features.flags[1], 0b01010001);
                        assert_eq!(node_features.flags[2], 0b00000010);
                        assert_eq!(node_features.flags[3], 0b00001000);
+                       assert_eq!(node_features.flags[4], 0b00000000);
+                       assert_eq!(node_features.flags[5], 0b00100000);
                }
 
                // Check that cleared flags are kept blank when converting back:
index d693c41d07e300c8228bda16ffb52d18d213fa11..2fd5e7d286548b92d4784b573f70e6e9e89a4966 100644 (file)
@@ -7390,9 +7390,9 @@ fn test_announce_disable_channels() {
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
-       let short_id_1 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
-       let short_id_2 = create_announced_chan_between_nodes(&nodes, 1, 0, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
-       let short_id_3 = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+       create_announced_chan_between_nodes(&nodes, 1, 0, InitFeatures::known(), InitFeatures::known());
+       create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 
        // Disconnect peers
        nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
@@ -7402,13 +7402,13 @@ fn test_announce_disable_channels() {
        nodes[0].node.timer_tick_occurred(); // DisabledStaged -> Disabled
        let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
        assert_eq!(msg_events.len(), 3);
-       let mut chans_disabled: HashSet<u64> = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect();
+       let mut chans_disabled = HashMap::new();
        for e in msg_events {
                match e {
                        MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
                                assert_eq!(msg.contents.flags & (1<<1), 1<<1); // The "channel disabled" bit should be set
                                // Check that each channel gets updated exactly once
-                               if !chans_disabled.remove(&msg.contents.short_channel_id) {
+                               if chans_disabled.insert(msg.contents.short_channel_id, msg.contents.timestamp).is_some() {
                                        panic!("Generated ChannelUpdate for wrong chan!");
                                }
                        },
@@ -7444,19 +7444,22 @@ fn test_announce_disable_channels() {
        nodes[0].node.timer_tick_occurred();
        let msg_events = nodes[0].node.get_and_clear_pending_msg_events();
        assert_eq!(msg_events.len(), 3);
-       chans_disabled = [short_id_1, short_id_2, short_id_3].iter().map(|a| *a).collect();
        for e in msg_events {
                match e {
                        MessageSendEvent::BroadcastChannelUpdate { ref msg } => {
                                assert_eq!(msg.contents.flags & (1<<1), 0); // The "channel disabled" bit should be off
-                               // Check that each channel gets updated exactly once
-                               if !chans_disabled.remove(&msg.contents.short_channel_id) {
-                                       panic!("Generated ChannelUpdate for wrong chan!");
+                               match chans_disabled.remove(&msg.contents.short_channel_id) {
+                                       // Each update should have a higher timestamp than the previous one, replacing
+                                       // the old one.
+                                       Some(prev_timestamp) => assert!(msg.contents.timestamp > prev_timestamp),
+                                       None => panic!("Generated ChannelUpdate for wrong chan!"),
                                }
                        },
                        _ => panic!("Unexpected event"),
                }
        }
+       // Check that each channel gets updated exactly once
+       assert!(chans_disabled.is_empty());
 }
 
 #[test]
index 3c6300ef8eadc5e517c3fb398c021bba8c53dd31..39c00105f8d675e5cf5ce664b2d7416654c40cfb 100644 (file)
@@ -707,6 +707,10 @@ pub enum ErrorAction {
        /// The peer did something harmless that we weren't able to meaningfully process.
        /// If the error is logged, log it at the given level.
        IgnoreAndLog(logger::Level),
+       /// The peer provided us with a gossip message which we'd already seen. In most cases this
+       /// should be ignored, but it may result in the message being forwarded if it is a duplicate of
+       /// our own channel announcements.
+       IgnoreDuplicateGossip,
        /// The peer did something incorrect. Tell them.
        SendErrorMessage {
                /// The message to send.
index 89f3e9ff5ffa48233086af330cb0fdb6ccf58f15..9157dc87dc2fc5e415de7679974bfe0de3a91f0c 100644 (file)
@@ -811,6 +811,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
                                                                                                        log_given_level!(self.logger, level, "Error handling message{}; ignoring: {}", OptionalFromDebugger(&peer.their_node_id), e.err);
                                                                                                        continue
                                                                                                },
+                                                                                               msgs::ErrorAction::IgnoreDuplicateGossip => continue, // Don't even bother logging these
                                                                                                msgs::ErrorAction::IgnoreError => {
                                                                                                        log_debug!(self.logger, "Error handling message{}; ignoring: {}", OptionalFromDebugger(&peer.their_node_id), e.err);
                                                                                                        continue;
@@ -1351,21 +1352,31 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
                                        },
                                        MessageSendEvent::BroadcastChannelAnnouncement { msg, update_msg } => {
                                                log_debug!(self.logger, "Handling BroadcastChannelAnnouncement event in peer_handler for short channel id {}", msg.contents.short_channel_id);
-                                               if self.message_handler.route_handler.handle_channel_announcement(&msg).is_ok() && self.message_handler.route_handler.handle_channel_update(&update_msg).is_ok() {
-                                                       self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None);
-                                                       self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None);
+                                               match self.message_handler.route_handler.handle_channel_announcement(&msg) {
+                                                       Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
+                                                               self.forward_broadcast_msg(peers, &wire::Message::ChannelAnnouncement(msg), None),
+                                                       _ => {},
+                                               }
+                                               match self.message_handler.route_handler.handle_channel_update(&update_msg) {
+                                                       Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
+                                                               self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(update_msg), None),
+                                                       _ => {},
                                                }
                                        },
                                        MessageSendEvent::BroadcastNodeAnnouncement { msg } => {
                                                log_debug!(self.logger, "Handling BroadcastNodeAnnouncement event in peer_handler");
-                                               if self.message_handler.route_handler.handle_node_announcement(&msg).is_ok() {
-                                                       self.forward_broadcast_msg(peers, &wire::Message::NodeAnnouncement(msg), None);
+                                               match self.message_handler.route_handler.handle_node_announcement(&msg) {
+                                                       Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
+                                                               self.forward_broadcast_msg(peers, &wire::Message::NodeAnnouncement(msg), None),
+                                                       _ => {},
                                                }
                                        },
                                        MessageSendEvent::BroadcastChannelUpdate { msg } => {
                                                log_debug!(self.logger, "Handling BroadcastChannelUpdate event in peer_handler for short channel id {}", msg.contents.short_channel_id);
-                                               if self.message_handler.route_handler.handle_channel_update(&msg).is_ok() {
-                                                       self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None);
+                                               match self.message_handler.route_handler.handle_channel_update(&msg) {
+                                                       Ok(_) | Err(LightningError { action: msgs::ErrorAction::IgnoreDuplicateGossip, .. }) =>
+                                                               self.forward_broadcast_msg(peers, &wire::Message::ChannelUpdate(msg), None),
+                                                       _ => {},
                                                }
                                        },
                                        MessageSendEvent::SendChannelUpdate { ref node_id, ref msg } => {
@@ -1398,6 +1409,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref, CMH: Deref> P
                                                        msgs::ErrorAction::IgnoreAndLog(level) => {
                                                                log_given_level!(self.logger, level, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
                                                        },
+                                                       msgs::ErrorAction::IgnoreDuplicateGossip => {},
                                                        msgs::ErrorAction::IgnoreError => {
                                                                log_debug!(self.logger, "Received a HandleError event to be ignored for node {}", log_pubkey!(node_id));
                                                        },
index 4d33bbdee9278717525c9991805245280bc22137..10c0ba57b58b05e2a6875795773e4a19188c6cb1 100644 (file)
@@ -847,8 +847,13 @@ impl NetworkGraph {
                        None => Err(LightningError{err: "No existing channels for node_announcement".to_owned(), action: ErrorAction::IgnoreError}),
                        Some(node) => {
                                if let Some(node_info) = node.announcement_info.as_ref() {
-                                       if node_info.last_update  >= msg.timestamp {
+                                       // The timestamp field is somewhat of a misnomer - the BOLTs use it to order
+                                       // updates to ensure you always have the latest one, only vaguely suggesting
+                                       // that it be at least the current time.
+                                       if node_info.last_update  > msg.timestamp {
                                                return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)});
+                                       } else if node_info.last_update  == msg.timestamp {
+                                               return Err(LightningError{err: "Update had the same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
                                        }
                                }
 
@@ -977,7 +982,7 @@ impl NetworkGraph {
                                        Self::remove_channel_in_nodes(&mut nodes, &entry.get(), msg.short_channel_id);
                                        *entry.get_mut() = chan_info;
                                } else {
-                                       return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)})
+                                       return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
                                }
                        },
                        BtreeEntry::Vacant(entry) => {
@@ -1082,8 +1087,16 @@ impl NetworkGraph {
                                macro_rules! maybe_update_channel_info {
                                        ( $target: expr, $src_node: expr) => {
                                                if let Some(existing_chan_info) = $target.as_ref() {
-                                                       if existing_chan_info.last_update >= msg.timestamp {
+                                                       // The timestamp field is somewhat of a misnomer - the BOLTs use it to
+                                                       // order updates to ensure you always have the latest one, only
+                                                       // suggesting  that it be at least the current time. For
+                                                       // channel_updates specifically, the BOLTs discuss the possibility of
+                                                       // pruning based on the timestamp field being more than two weeks old,
+                                                       // but only in the non-normative section.
+                                                       if existing_chan_info.last_update > msg.timestamp {
                                                                return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Gossip)});
+                                                       } else if existing_chan_info.last_update == msg.timestamp {
+                                                               return Err(LightningError{err: "Update had same timestamp as last processed update".to_owned(), action: ErrorAction::IgnoreDuplicateGossip});
                                                        }
                                                        chan_was_enabled = existing_chan_info.enabled;
                                                } else {
@@ -1720,7 +1733,7 @@ mod tests {
 
                match net_graph_msg_handler.handle_channel_update(&valid_channel_update) {
                        Ok(_) => panic!(),
-                       Err(e) => assert_eq!(e.err, "Update older than last processed update")
+                       Err(e) => assert_eq!(e.err, "Update had same timestamp as last processed update")
                };
                unsigned_channel_update.timestamp += 500;
 
index cd7d62e6686ed9628d455f5b548ff17a1cfdf091..e49844383bbceee091e9d42b7262416109b2d484 100644 (file)
@@ -1521,6 +1521,7 @@ mod tests {
                        short_channel_id,
                        channel_value_satoshis: 0,
                        user_channel_id: 0,
+                       balance_msat: 0,
                        outbound_capacity_msat,
                        inbound_capacity_msat: 42,
                        unspendable_punishment_reserve: None,