Skip to content

Add config support for 'their_channel_reserve_proportional_millionths' [#1498] #1619

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.

Sign up for GitHub

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 1 commit into from
Aug 3, 2022
Merged

Add config support for 'their_channel_reserve_proportional_millionths' [#1498] #1619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ lightning-c-bindings/a.out
Cargo.lock
.idea
lightning/target
lightning/ldk-net_graph-*.bin
no-std-check/target

99 changes: 87 additions & 12 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,9 @@ pub const MAX_CHAN_DUST_LIMIT_SATOSHIS: u64 = MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS
/// See https://github.com/lightning/bolts/issues/905 for more details.
pub const MIN_CHAN_DUST_LIMIT_SATOSHIS: u64 = 354;

// Just a reasonable implementation-specific safe lower bound, higher than the dust limit.
pub const MIN_THEIR_CHAN_RESERVE_SATOSHIS: u64 = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have some docs, including explaining why 1000 was chosen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have an exact answer for this, current minimum was 1000, i just extracted it to constant and made it bit more explicit.
I think its just a reasonable value greater than dust limit.


/// Used to return a simple Error back to ChannelManager. Will get converted to a
/// msgs::ErrorAction::SendErrorMessage or msgs::ErrorAction::IgnoreError as appropriate with our
/// channel_id in ChannelManager.
Expand Down Expand Up @@ -843,16 +846,25 @@ impl<Signer: Sign> Channel<Signer> {
}

/// Returns a minimum channel reserve value the remote needs to maintain,
/// required by us.
/// required by us according to the configured or default
/// [`ChannelHandshakeConfig::their_channel_reserve_proportional_millionths`]
///
/// Guaranteed to return a value no larger than channel_value_satoshis
///
/// This is used both for new channels and to figure out what reserve value we sent to peers
/// for channels serialized before we included our selected reserve value in the serialized
/// data explicitly.
pub(crate) fn get_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
/// This is used both for outbound and inbound channels and has lower bound
/// of `MIN_THEIR_CHAN_RESERVE_SATOSHIS`.
pub(crate) fn get_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64, config: &UserConfig) -> u64 {
let calculated_reserve = channel_value_satoshis.saturating_mul(config.channel_handshake_config.their_channel_reserve_proportional_millionths as u64) / 1_000_000;
cmp::min(channel_value_satoshis, cmp::max(calculated_reserve, MIN_THEIR_CHAN_RESERVE_SATOSHIS))
}

/// This is for legacy reasons, present for forward-compatibility.
/// LDK versions older than 0.0.104 don't know how read/handle values other than default
/// from storage. Hence, we use this function to not persist default values of
/// `holder_selected_channel_reserve_satoshis` for channels into storage.
pub(crate) fn get_legacy_default_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
let (q, _) = channel_value_satoshis.overflowing_div(100);
cmp::min(channel_value_satoshis, cmp::max(q, 1000)) //TODO
cmp::min(channel_value_satoshis, cmp::max(q, 1000))
}

pub(crate) fn opt_anchors(&self) -> bool {
Expand Down Expand Up @@ -912,8 +924,10 @@ impl<Signer: Sign> Channel<Signer> {
if holder_selected_contest_delay < BREAKDOWN_TIMEOUT {
return Err(APIError::APIMisuseError {err: format!("Configured with an unreasonable our_to_self_delay ({}) putting user funds at risks", holder_selected_contest_delay)});
}
let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis);
let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis, config);
if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
// Protocol level safety check in place, although it should never happen because
// of `MIN_THEIR_CHAN_RESERVE_SATOSHIS`
return Err(APIError::APIMisuseError { err: format!("Holder selected channel reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) });
}

Expand Down Expand Up @@ -1204,12 +1218,14 @@ impl<Signer: Sign> Channel<Signer> {
}
}

let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis, config);
if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
// Protocol level safety check in place, although it should never happen because
// of `MIN_THEIR_CHAN_RESERVE_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)));
return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({})msats. Channel value is ({} - {})msats.", holder_selected_channel_reserve_satoshis * 1000, 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.",
Expand Down Expand Up @@ -6106,7 +6122,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
// a different percentage of the channel value then 10%, which older versions of LDK used
// to set it to before the percentage was made configurable.
let serialized_holder_selected_reserve =
if self.holder_selected_channel_reserve_satoshis != Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis)
if self.holder_selected_channel_reserve_satoshis != Self::get_legacy_default_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis)
{ Some(self.holder_selected_channel_reserve_satoshis) } else { None };

let mut old_max_in_flight_percent_config = UserConfig::default().channel_handshake_config;
Expand Down Expand Up @@ -6381,7 +6397,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
let mut announcement_sigs = None;
let mut target_closing_feerate_sats_per_kw = None;
let mut monitor_pending_finalized_fulfills = Some(Vec::new());
let mut holder_selected_channel_reserve_satoshis = Some(Self::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis));
let mut holder_selected_channel_reserve_satoshis = Some(Self::get_legacy_default_holder_selected_channel_reserve_satoshis(channel_value_satoshis));
let mut holder_max_htlc_value_in_flight_msat = Some(Self::get_holder_max_htlc_value_in_flight_msat(channel_value_satoshis, &UserConfig::default().channel_handshake_config));
// Prior to supporting channel type negotiation, all of our channels were static_remotekey
// only, so we default to that if none was written.
Expand Down Expand Up @@ -6561,6 +6577,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>

#[cfg(test)]
mod tests {
use std::cmp;
use bitcoin::blockdata::script::{Script, Builder};
use bitcoin::blockdata::transaction::{Transaction, TxOut};
use bitcoin::blockdata::constants::genesis_block;
Expand All @@ -6570,7 +6587,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_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
use ln::features::{InitFeatures, ChannelTypeFeatures};
use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
use ln::script::ShutdownScript;
Expand Down Expand Up @@ -6962,6 +6979,64 @@ mod tests {
assert_eq!(chan_8.holder_max_htlc_value_in_flight_msat, chan_8_value_msat);
}

#[test]
fn test_configured_holder_selected_channel_reserve_satoshis() {

// Test that `new_outbound` and `new_from_req` create a channel with the correct
// channel reserves, when `their_channel_reserve_proportional_millionths` is configured.
test_self_and_counterparty_channel_reserve(10_000_000, 0.02, 0.02);

// Test with valid but unreasonably high channel reserves
// Requesting and accepting parties have requested for 49%-49% and 60%-30% channel reserve
test_self_and_counterparty_channel_reserve(10_000_000, 0.49, 0.49);
test_self_and_counterparty_channel_reserve(10_000_000, 0.60, 0.30);

// Test with calculated channel reserve less than lower bound
// i.e `MIN_THEIR_CHAN_RESERVE_SATOSHIS`
test_self_and_counterparty_channel_reserve(100_000, 0.00002, 0.30);

// Test with invalid channel reserves since sum of both is greater than or equal
// to channel value
test_self_and_counterparty_channel_reserve(10_000_000, 0.50, 0.50);
test_self_and_counterparty_channel_reserve(10_000_000, 0.60, 0.50);
}

fn test_self_and_counterparty_channel_reserve(channel_value_satoshis: u64, outbound_selected_channel_reserve_perc: f64, inbound_selected_channel_reserve_perc: f64) {
let fee_est = LowerBoundedFeeEstimator::new(&TestFeeEstimator { fee_est: 15_000 });
let logger = test_utils::TestLogger::new();
let secp_ctx = Secp256k1::new();
let seed = [42; 32];
let network = Network::Testnet;
let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
let outbound_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
let inbound_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());


let mut outbound_node_config = UserConfig::default();
outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
let chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, outbound_node_id, &InitFeatures::known(), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42).unwrap();

let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.channel_value_satoshis as f64 * outbound_selected_channel_reserve_perc) as u64);
assert_eq!(chan.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve);

let chan_open_channel_msg = chan.get_open_channel(genesis_block(network).header.block_hash());
let mut inbound_node_config = UserConfig::default();
inbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (inbound_selected_channel_reserve_perc * 1_000_000.0) as u32;

if outbound_selected_channel_reserve_perc + inbound_selected_channel_reserve_perc < 1.0 {
let chan_inbound_node = Channel::<EnforcingSigner>::new_from_req(&&fee_est, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, 42).unwrap();

let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.channel_value_satoshis as f64 * inbound_selected_channel_reserve_perc) as u64);

assert_eq!(chan_inbound_node.holder_selected_channel_reserve_satoshis, expected_inbound_selected_chan_reserve);
assert_eq!(chan_inbound_node.counterparty_selected_channel_reserve_satoshis.unwrap(), expected_outbound_selected_chan_reserve);
} else {
// Channel Negotiations failed
let result = Channel::<EnforcingSigner>::new_from_req(&&fee_est, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, 42);
assert!(result.is_err());
}
}

#[test]
fn channel_update() {
let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1635,7 +1635,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
}

/// Gets the current configuration applied to all new channels, as
/// Gets the current configuration applied to all new channels.
pub fn get_current_default_configuration(&self) -> &UserConfig {
&self.default_configuration
}
Expand Down
28 changes: 16 additions & 12 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ fn test_insane_channel_opens() {
// Instantiate channel parameters where we push the maximum msats given our
// funding satoshis
let channel_value_sat = 31337; // same as funding satoshis
let channel_reserve_satoshis = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(channel_value_sat);
let channel_reserve_satoshis = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(channel_value_sat, &cfg);
let push_msat = (channel_value_sat - channel_reserve_satoshis) * 1000;

// Have node0 initiate a channel to node1 with aforementioned parameters
Expand Down Expand Up @@ -148,13 +148,14 @@ fn do_test_counterparty_no_reserve(send_from_initiator: bool) {
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
let default_config = UserConfig::default();

// Have node0 initiate a channel to node1 with aforementioned parameters
let mut push_amt = 100_000_000;
let feerate_per_kw = 253;
let opt_anchors = false;
push_amt -= feerate_per_kw as u64 * (commitment_tx_base_weight(opt_anchors) + 4 * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 * 1000;
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000, &default_config) * 1000;

let temp_channel_id = nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, if send_from_initiator { 0 } else { push_amt }, 42, None).unwrap();
let mut open_channel_message = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
Expand Down Expand Up @@ -633,7 +634,8 @@ fn test_update_fee_that_funder_cannot_afford() {
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, push_sats * 1000, InitFeatures::known(), InitFeatures::known());
let channel_id = chan.2;
let secp_ctx = Secp256k1::new();
let bs_channel_reserve_sats = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(channel_value);
let default_config = UserConfig::default();
let bs_channel_reserve_sats = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(channel_value, &default_config);

let opt_anchors = false;

Expand Down Expand Up @@ -1520,12 +1522,13 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let default_config = UserConfig::default();
let opt_anchors = false;

let mut push_amt = 100_000_000;
push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64, opt_anchors);
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;

push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000, &default_config) * 1000;

let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt, InitFeatures::known(), InitFeatures::known());

Expand All @@ -1549,15 +1552,15 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let default_config = UserConfig::default();
let opt_anchors = false;

// Set nodes[0]'s balance such that they will consider any above-dust received HTLC to be a
// channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
// transaction fee with 0 HTLCs (183 sats)).
let mut push_amt = 100_000_000;
push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64, opt_anchors);
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000, &default_config) * 1000;
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt, InitFeatures::known(), InitFeatures::known());

// Send four HTLCs to cover the initial push_msat buffer we're required to include
Expand Down Expand Up @@ -1602,15 +1605,15 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let default_config = UserConfig::default();
let opt_anchors = false;

// Set nodes[0]'s balance such that they will consider any above-dust received HTLC to be a
// channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
// transaction fee with 0 HTLCs (183 sats)).
let mut push_amt = 100_000_000;
push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64, opt_anchors);
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000, &default_config) * 1000;
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, push_amt, InitFeatures::known(), InitFeatures::known());

let dust_amt = crate::ln::channel::MIN_CHAN_DUST_LIMIT_SATOSHIS * 1000
Expand Down Expand Up @@ -1640,7 +1643,7 @@ fn test_chan_init_feerate_unaffordability() {
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

let default_config = UserConfig::default();
let opt_anchors = false;

// Set the push_msat amount such that nodes[0] will not be able to afford to add even a single
Expand All @@ -1652,7 +1655,7 @@ fn test_chan_init_feerate_unaffordability() {

// During open, we don't have a "counterparty channel reserve" to check against, so that
// requirement only comes into play on the open_channel handling side.
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000, &default_config) * 1000;
nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, push_amt, 42, None).unwrap();
let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
open_channel_msg.push_msat += 1;
Expand Down Expand Up @@ -1771,10 +1774,11 @@ fn test_inbound_outbound_capacity_is_not_zero() {
let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
let channels0 = node_chanmgrs[0].list_channels();
let channels1 = node_chanmgrs[1].list_channels();
let default_config = UserConfig::default();
assert_eq!(channels0.len(), 1);
assert_eq!(channels1.len(), 1);

let reserve = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100000);
let reserve = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000, &default_config);
assert_eq!(channels0[0].inbound_capacity_msat, 95000000 - reserve*1000);
assert_eq!(channels1[0].outbound_capacity_msat, 95000000 - reserve*1000);

Expand Down
Loading