Skip to content

Wumbo! #1425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 28, 2022
Merged

Wumbo! #1425

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -734,9 +734,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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny nitpick on naming (not blocking): I feel like "total" can be misinterpreted in various ways such as representing the total supply so far and not necessarily year 2140. If the intention is to communicate the maximum number of sats possible in any one channel, then maybe a name like MAX_SATOSHI_LIMIT would be more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Total bitcoin supply" is referred to quite a few times in the codebase as 21 million bitcoin, so I like that it's consistent with that


/// 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
Expand Down Expand Up @@ -850,8 +854,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 {
Expand Down Expand Up @@ -1076,8 +1083,11 @@ 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)));
Expand Down Expand Up @@ -4110,7 +4120,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()));
}

Expand Down Expand Up @@ -6298,7 +6308,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;
Expand Down Expand Up @@ -6334,9 +6344,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]
Expand Down
2 changes: 0 additions & 2 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1498,8 +1498,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`.
Expand Down
25 changes: 21 additions & 4 deletions lightning/src/ln/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ mod sealed {
// Byte 1
,
// Byte 2
BasicMPP,
BasicMPP | Wumbo,
// Byte 3
ShutdownAnySegwit,
// Byte 4
Expand Down Expand Up @@ -169,7 +169,7 @@ mod sealed {
// Byte 1
,
// Byte 2
BasicMPP,
BasicMPP | Wumbo,
// Byte 3
ShutdownAnySegwit,
// Byte 4
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
28 changes: 25 additions & 3 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any need to use TOTAL_BITCOIN_SUPPLY_SATOSHIS here? Seems strange to use an invalid value when any valid value would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allows us to cover both the cases of config.max_funding_satoshis and the TOTAL_BITCOIN_SUPPLY check without recreating the channels with a new config. If it were lower, we'd never get to the TOTAL_BITCOIN_SUPPLY check I think

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
Expand Down Expand Up @@ -92,11 +95,11 @@ 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 });

Expand All @@ -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
Expand Down
9 changes: 8 additions & 1 deletion lightning/src/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have a config parameter? If we advertise support for wumbo, wouldn't it be surprising to a peer if they couldn't open a channel >= 2^24?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users are likely to implement it anyway via the accept_channel hooks, plus its a really important security parameter - what level of risk are you willing to tolerate if the channel funds somehow get lost because you went offline too long?

/// 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.
///
Expand Down Expand Up @@ -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(),
Expand Down