Merge pull request #1451 from TheBlueMatt/2022-04-moar-mpp-fail-test
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Fri, 29 Apr 2022 19:50:37 +0000 (19:50 +0000)
committerGitHub <noreply@github.com>
Fri, 29 Apr 2022 19:50:37 +0000 (19:50 +0000)
Add test coverage for failure of inconsistent MPP parts

lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/features.rs
lightning/src/ln/functional_tests.rs
lightning/src/util/config.rs

index 3a12a2c066d0bb5e7580d5ef160a78dc5c890433..1cb7a689a21a1b710413e93afbba8e1881d48e90 100644 (file)
@@ -745,9 +745,13 @@ pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
 
 pub const ANCHOR_OUTPUT_VALUE_SATOSHI: u64 = 330;
 
-/// Maximum `funding_satoshis` value, according to the BOLT #2 specification
-/// it's 2^24.
-pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24;
+/// Maximum `funding_satoshis` value according to the BOLT #2 specification, if
+/// `option_support_large_channel` (aka wumbo channels) is not supported.
+/// It's 2^24 - 1.
+pub const MAX_FUNDING_SATOSHIS_NO_WUMBO: u64 = (1 << 24) - 1;
+
+/// Total bitcoin supply in satoshis.
+pub const TOTAL_BITCOIN_SUPPLY_SATOSHIS: u64 = 21_000_000 * 1_0000_0000;
 
 /// The maximum network dust limit for standard script formats. This currently represents the
 /// minimum output value for a P2SH output before Bitcoin Core 22 considers the entire
@@ -861,8 +865,11 @@ impl<Signer: Sign> Channel<Signer> {
                let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
                let pubkeys = holder_signer.pubkeys().clone();
 
-               if channel_value_satoshis >= MAX_FUNDING_SATOSHIS {
-                       return Err(APIError::APIMisuseError{err: format!("funding_value must be smaller than {}, it was {}", MAX_FUNDING_SATOSHIS, channel_value_satoshis)});
+               if !their_features.supports_wumbo() && channel_value_satoshis > MAX_FUNDING_SATOSHIS_NO_WUMBO {
+                       return Err(APIError::APIMisuseError{err: format!("funding_value must not exceed {}, it was {}", MAX_FUNDING_SATOSHIS_NO_WUMBO, channel_value_satoshis)});
+               }
+               if channel_value_satoshis >= TOTAL_BITCOIN_SUPPLY_SATOSHIS {
+                       return Err(APIError::APIMisuseError{err: format!("funding_value must be smaller than the total bitcoin supply, it was {}", channel_value_satoshis)});
                }
                let channel_value_msat = channel_value_satoshis * 1000;
                if push_msat > channel_value_msat {
@@ -1087,20 +1094,22 @@ impl<Signer: Sign> Channel<Signer> {
                }
 
                // Check sanity of message fields:
-               if msg.funding_satoshis >= MAX_FUNDING_SATOSHIS {
-                       return Err(ChannelError::Close(format!("Funding must be smaller than {}. It was {}", MAX_FUNDING_SATOSHIS, msg.funding_satoshis)));
+               if msg.funding_satoshis > config.peer_channel_config_limits.max_funding_satoshis {
+                       return Err(ChannelError::Close(format!("Per our config, funding must be at most {}. It was {}", config.peer_channel_config_limits.max_funding_satoshis, msg.funding_satoshis)));
+               }
+               if msg.funding_satoshis >= TOTAL_BITCOIN_SUPPLY_SATOSHIS {
+                       return Err(ChannelError::Close(format!("Funding must be smaller than the total bitcoin supply. It was {}", msg.funding_satoshis)));
                }
                if msg.channel_reserve_satoshis > msg.funding_satoshis {
                        return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must be not greater than funding_satoshis: {}", msg.channel_reserve_satoshis, msg.funding_satoshis)));
                }
-               let funding_value = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
-               if msg.push_msat > funding_value {
-                       return Err(ChannelError::Close(format!("push_msat {} was larger than funding value {}", msg.push_msat, funding_value)));
+               let full_channel_value_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
+               if msg.push_msat > full_channel_value_msat {
+                       return Err(ChannelError::Close(format!("push_msat {} was larger than channel amount minus reserve ({})", msg.push_msat, full_channel_value_msat)));
                }
                if msg.dust_limit_satoshis > msg.funding_satoshis {
                        return Err(ChannelError::Close(format!("dust_limit_satoshis {} was larger than funding_satoshis {}. Peer never wants payout outputs?", msg.dust_limit_satoshis, msg.funding_satoshis)));
                }
-               let full_channel_value_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
                if msg.htlc_minimum_msat >= full_channel_value_msat {
                        return Err(ChannelError::Close(format!("Minimum htlc value ({}) was larger than full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat)));
                }
@@ -1154,6 +1163,9 @@ impl<Signer: Sign> Channel<Signer> {
                if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
                        return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
                }
+               if holder_selected_channel_reserve_satoshis * 1000 >= full_channel_value_msat {
+                       return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). Channel value is ({} - {}).", holder_selected_channel_reserve_satoshis, full_channel_value_msat, msg.push_msat)));
+               }
                if msg.channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
                        log_debug!(logger, "channel_reserve_satoshis ({}) is smaller than our dust limit ({}). We can broadcast stale states without any risk, implying this channel is very insecure for our counterparty.",
                                msg.channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS);
@@ -4120,7 +4132,7 @@ impl<Signer: Sign> Channel<Signer> {
                if !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() {
                        return Err(ChannelError::Close("Remote end sent us a closing_signed while there were still pending HTLCs".to_owned()));
                }
-               if msg.fee_satoshis > 21_000_000 * 1_0000_0000 { //this is required to stop potential overflow in build_closing_transaction
+               if msg.fee_satoshis > TOTAL_BITCOIN_SUPPLY_SATOSHIS { // this is required to stop potential overflow in build_closing_transaction
                        return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned()));
                }
 
@@ -6327,7 +6339,7 @@ mod tests {
        use ln::PaymentHash;
        use ln::channelmanager::{HTLCSource, PaymentId};
        use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
-       use ln::channel::MAX_FUNDING_SATOSHIS;
+       use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
        use ln::features::InitFeatures;
        use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
        use ln::script::ShutdownScript;
@@ -6363,9 +6375,10 @@ mod tests {
        }
 
        #[test]
-       fn test_max_funding_satoshis() {
-               assert!(MAX_FUNDING_SATOSHIS <= 21_000_000 * 100_000_000,
-                       "MAX_FUNDING_SATOSHIS is greater than all satoshis in existence");
+       fn test_max_funding_satoshis_no_wumbo() {
+               assert_eq!(TOTAL_BITCOIN_SUPPLY_SATOSHIS, 21_000_000 * 100_000_000);
+               assert!(MAX_FUNDING_SATOSHIS_NO_WUMBO <= TOTAL_BITCOIN_SUPPLY_SATOSHIS,
+                       "MAX_FUNDING_SATOSHIS_NO_WUMBO is greater than all satoshis in existence");
        }
 
        #[test]
index d7f2aeb6e4132374cc7a16d1c04361f65a3db358..5990b2adf0863f690fcd6b099390a77de053d197 100644 (file)
@@ -1516,8 +1516,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        ///
        /// Non-proportional fees are fixed according to our risk using the provided fee estimator.
        ///
-       /// panics if channel_value_satoshis is >= `MAX_FUNDING_SATOSHIS`!
-       ///
        /// Users need to notify the new ChannelManager when a new block is connected or
        /// disconnected using its `block_connected` and `block_disconnected` methods, starting
        /// from after `params.latest_hash`.
index 4512ee8002996aced4ba6238c6d7a42233608955..2580874640e438e611bea9b88ddda0384dc9db5d 100644 (file)
@@ -137,7 +137,7 @@ mod sealed {
                        // Byte 1
                        ,
                        // Byte 2
-                       BasicMPP,
+                       BasicMPP | Wumbo,
                        // Byte 3
                        ShutdownAnySegwit,
                        // Byte 4
@@ -169,7 +169,7 @@ mod sealed {
                        // Byte 1
                        ,
                        // Byte 2
-                       BasicMPP,
+                       BasicMPP | Wumbo,
                        // Byte 3
                        ShutdownAnySegwit,
                        // Byte 4
@@ -390,6 +390,9 @@ mod sealed {
        define_feature!(17, BasicMPP, [InitContext, NodeContext, InvoiceContext],
                "Feature flags for `basic_mpp`.", set_basic_mpp_optional, set_basic_mpp_required,
                supports_basic_mpp, requires_basic_mpp);
+       define_feature!(19, Wumbo, [InitContext, NodeContext],
+               "Feature flags for `option_support_large_channel` (aka wumbo channels).", set_wumbo_optional, set_wumbo_required,
+               supports_wumbo, requires_wumbo);
        define_feature!(27, ShutdownAnySegwit, [InitContext, NodeContext],
                "Feature flags for `opt_shutdown_anysegwit`.", set_shutdown_any_segwit_optional,
                set_shutdown_any_segwit_required, supports_shutdown_anysegwit, requires_shutdown_anysegwit);
@@ -740,6 +743,15 @@ impl<T: sealed::ShutdownAnySegwit> Features<T> {
                self
        }
 }
+
+impl<T: sealed::Wumbo> Features<T> {
+       #[cfg(test)]
+       pub(crate) fn clear_wumbo(mut self) -> Self {
+               <T as sealed::Wumbo>::clear_bits(&mut self.flags);
+               self
+       }
+}
+
 macro_rules! impl_feature_len_prefixed_write {
        ($features: ident) => {
                impl Writeable for $features {
@@ -843,6 +855,11 @@ mod tests {
                assert!(!InitFeatures::known().requires_scid_privacy());
                assert!(!NodeFeatures::known().requires_scid_privacy());
 
+               assert!(InitFeatures::known().supports_wumbo());
+               assert!(NodeFeatures::known().supports_wumbo());
+               assert!(!InitFeatures::known().requires_wumbo());
+               assert!(!NodeFeatures::known().requires_wumbo());
+
                let mut init_features = InitFeatures::known();
                assert!(init_features.initial_routing_sync());
                init_features.clear_initial_routing_sync();
@@ -878,14 +895,14 @@ mod tests {
                        // Check that the flags are as expected:
                        // - option_data_loss_protect
                        // - var_onion_optin (req) | static_remote_key (req) | payment_secret(req)
-                       // - basic_mpp
+                       // - basic_mpp | wumbo
                        // - opt_shutdown_anysegwit
                        // -
                        // - option_channel_type | option_scid_alias
                        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[2], 0b00001010);
                        assert_eq!(node_features.flags[3], 0b00001000);
                        assert_eq!(node_features.flags[4], 0b00000000);
                        assert_eq!(node_features.flags[5], 0b10100000);
index f915248b0b0e983c098c4ce846887a0f4edb5389..4defbaaa2931d061ca7bb399d45cb0178a7240ee 100644 (file)
@@ -58,9 +58,12 @@ use ln::chan_utils::CommitmentTransaction;
 #[test]
 fn test_insane_channel_opens() {
        // Stand up a network of 2 nodes
+       use ln::channel::TOTAL_BITCOIN_SUPPLY_SATOSHIS;
+       let mut cfg = UserConfig::default();
+       cfg.peer_channel_config_limits.max_funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS + 1;
        let chanmon_cfgs = create_chanmon_cfgs(2);
        let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(cfg)]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
        // Instantiate channel parameters where we push the maximum msats given our
@@ -92,15 +95,15 @@ fn test_insane_channel_opens() {
                } else { assert!(false); }
        };
 
-       use ln::channel::MAX_FUNDING_SATOSHIS;
        use ln::channelmanager::MAX_LOCAL_BREAKDOWN_TIMEOUT;
 
        // Test all mutations that would make the channel open message insane
-       insane_open_helper(format!("Funding must be smaller than {}. It was {}", MAX_FUNDING_SATOSHIS, MAX_FUNDING_SATOSHIS).as_str(), |mut msg| { msg.funding_satoshis = MAX_FUNDING_SATOSHIS; msg });
+       insane_open_helper(format!("Per our config, funding must be at most {}. It was {}", TOTAL_BITCOIN_SUPPLY_SATOSHIS + 1, TOTAL_BITCOIN_SUPPLY_SATOSHIS + 2).as_str(), |mut msg| { msg.funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS + 2; msg });
+       insane_open_helper(format!("Funding must be smaller than the total bitcoin supply. It was {}", TOTAL_BITCOIN_SUPPLY_SATOSHIS).as_str(), |mut msg| { msg.funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS; msg });
 
        insane_open_helper("Bogus channel_reserve_satoshis", |mut msg| { msg.channel_reserve_satoshis = msg.funding_satoshis + 1; msg });
 
-       insane_open_helper(r"push_msat \d+ was larger than funding value \d+", |mut msg| { msg.push_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 + 1; msg });
+       insane_open_helper(r"push_msat \d+ was larger than channel amount minus reserve \(\d+\)", |mut msg| { msg.push_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 + 1; msg });
 
        insane_open_helper("Peer never wants payout outputs?", |mut msg| { msg.dust_limit_satoshis = msg.funding_satoshis + 1 ; msg });
 
@@ -113,6 +116,25 @@ fn test_insane_channel_opens() {
        insane_open_helper("max_accepted_htlcs was 484. It must not be larger than 483", |mut msg| { msg.max_accepted_htlcs = 484; msg });
 }
 
+#[test]
+fn test_funding_exceeds_no_wumbo_limit() {
+       // Test that if a peer does not support wumbo channels, we'll refuse to open a wumbo channel to
+       // them.
+       use ln::channel::MAX_FUNDING_SATOSHIS_NO_WUMBO;
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       node_cfgs[1].features = InitFeatures::known().clear_wumbo();
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       match nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), MAX_FUNDING_SATOSHIS_NO_WUMBO + 1, 0, 42, None) {
+               Err(APIError::APIMisuseError { err }) => {
+                       assert_eq!(format!("funding_value must not exceed {}, it was {}", MAX_FUNDING_SATOSHIS_NO_WUMBO, MAX_FUNDING_SATOSHIS_NO_WUMBO + 1), err);
+               },
+               _ => panic!()
+       }
+}
+
 fn do_test_counterparty_no_reserve(send_from_initiator: bool) {
        // A peer providing a channel_reserve_satoshis of 0 (or less than our dust limit) is insecure,
        // but only for them. Because some LSPs do it with some level of trust of the clients (for a
index bd8b40b66567e8a5346e8e551568aaf1d539eae5..0f625098f79f9030ae25ddc1979cc6b5fcba30ae 100644 (file)
@@ -10,6 +10,7 @@
 //! Various user-configurable channel limits and settings which ChannelManager
 //! applies for you.
 
+use ln::channel::MAX_FUNDING_SATOSHIS_NO_WUMBO;
 use ln::channelmanager::{BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 
 /// Configuration we set when applicable.
@@ -95,11 +96,16 @@ impl Default for ChannelHandshakeConfig {
 /// are applied mostly only to incoming channels that's not much of a problem.
 #[derive(Copy, Clone, Debug)]
 pub struct ChannelHandshakeLimits {
-       /// Minimum allowed satoshis when a channel is funded, this is supplied by the sender and so
+       /// Minimum allowed satoshis when a channel is funded. This is supplied by the sender and so
        /// only applies to inbound channels.
        ///
        /// Default value: 0.
        pub min_funding_satoshis: u64,
+       /// Maximum allowed satoshis when a channel is funded. This is supplied by the sender and so
+       /// only applies to inbound channels.
+       ///
+       /// Default value: 2^24 - 1.
+       pub max_funding_satoshis: u64,
        /// The remote node sets a limit on the minimum size of HTLCs we can send to them. This allows
        /// you to limit the maximum minimum-size they can require.
        ///
@@ -151,6 +157,7 @@ impl Default for ChannelHandshakeLimits {
        fn default() -> Self {
                ChannelHandshakeLimits {
                        min_funding_satoshis: 0,
+                       max_funding_satoshis: MAX_FUNDING_SATOSHIS_NO_WUMBO,
                        max_htlc_minimum_msat: <u64>::max_value(),
                        min_max_htlc_value_in_flight_msat: 0,
                        max_channel_reserve_satoshis: <u64>::max_value(),