Skip to content

Make channel reserve variable names less confusing. #613

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
May 6, 2020
Merged

Make channel reserve variable names less confusing. #613

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
64 changes: 33 additions & 31 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,9 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> {
#[cfg(not(test))]
their_max_htlc_value_in_flight_msat: u64,
//get_our_max_htlc_value_in_flight_msat(): u64,
/// minimum channel reserve for **self** to maintain - set by them.
their_channel_reserve_satoshis: u64,
//get_our_channel_reserve_satoshis(): u64,
/// minimum channel reserve for self to maintain - set by them.
local_channel_reserve_satoshis: u64,
// get_remote_channel_reserve_satoshis(channel_value_sats: u64): u64
their_htlc_minimum_msat: u64,
our_htlc_minimum_msat: u64,
their_to_self_delay: u16,
Expand Down Expand Up @@ -420,10 +420,11 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
channel_value_satoshis * 1000 / 10 //TODO
}

/// Returns a minimum channel reserve value **they** need to maintain
/// Returns a minimum channel reserve value the remote needs to maintain,
/// required by us.
///
/// Guaranteed to return a value no larger than channel_value_satoshis
pub(crate) fn get_our_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
pub(crate) fn get_remote_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
}
Expand Down Expand Up @@ -452,7 +453,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {


let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
if Channel::<ChanSigner>::get_our_channel_reserve_satoshis(channel_value_satoshis) < Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate) {
if Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(channel_value_satoshis) < Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate) {
return Err(APIError::FeeRateTooHigh{err: format!("Not enough reserve above dust limit can be found at current fee rate({})", background_feerate), feerate: background_feerate});
}

Expand Down Expand Up @@ -512,7 +513,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
their_dust_limit_satoshis: 0,
our_dust_limit_satoshis: Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate),
their_max_htlc_value_in_flight_msat: 0,
their_channel_reserve_satoshis: 0,
local_channel_reserve_satoshis: 0,
their_htlc_minimum_msat: 0,
our_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
their_to_self_delay: 0,
Expand Down Expand Up @@ -638,15 +639,15 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);

let our_dust_limit_satoshis = Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate);
let our_channel_reserve_satoshis = Channel::<ChanSigner>::get_our_channel_reserve_satoshis(msg.funding_satoshis);
if our_channel_reserve_satoshis < our_dust_limit_satoshis {
let remote_channel_reserve_satoshis = Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(msg.funding_satoshis);
if remote_channel_reserve_satoshis < our_dust_limit_satoshis {
return Err(ChannelError::Close("Suitable channel reserve not found. aborting"));
}
if msg.channel_reserve_satoshis < our_dust_limit_satoshis {
return Err(ChannelError::Close("channel_reserve_satoshis too small"));
}
if our_channel_reserve_satoshis < msg.dust_limit_satoshis {
return Err(ChannelError::Close("Dust limit too high for our channel reserve"));
if remote_channel_reserve_satoshis < msg.dust_limit_satoshis {
return Err(ChannelError::Close("Dust limit too high for the channel reserve we require the remote to keep"));
}

// check if the funder's amount for the initial commitment tx is sufficient
Expand All @@ -658,7 +659,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {

let to_local_msat = msg.push_msat;
let to_remote_msat = funders_amount_msat - background_feerate * COMMITMENT_TX_BASE_WEIGHT;
if to_local_msat <= msg.channel_reserve_satoshis * 1000 && to_remote_msat <= our_channel_reserve_satoshis * 1000 {
if to_local_msat <= msg.channel_reserve_satoshis * 1000 && to_remote_msat <= remote_channel_reserve_satoshis * 1000 {
return Err(ChannelError::Close("Insufficient funding amount for initial commitment"));
}

Expand Down Expand Up @@ -737,7 +738,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
their_dust_limit_satoshis: msg.dust_limit_satoshis,
our_dust_limit_satoshis: our_dust_limit_satoshis,
their_max_htlc_value_in_flight_msat: cmp::min(msg.max_htlc_value_in_flight_msat, msg.funding_satoshis * 1000),
their_channel_reserve_satoshis: msg.channel_reserve_satoshis,
local_channel_reserve_satoshis: msg.channel_reserve_satoshis,
their_htlc_minimum_msat: msg.htlc_minimum_msat,
our_htlc_minimum_msat: if config.own_channel_config.our_htlc_minimum_msat == 0 { 1 } else { config.own_channel_config.our_htlc_minimum_msat },
their_to_self_delay: msg.to_self_delay,
Expand Down Expand Up @@ -952,9 +953,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
} else {
self.max_commitment_tx_output_remote.lock().unwrap()
};
debug_assert!(max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= self.their_channel_reserve_satoshis as i64);
debug_assert!(max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= self.local_channel_reserve_satoshis as i64);
max_commitment_tx_output.0 = cmp::max(max_commitment_tx_output.0, value_to_self_msat as u64);
debug_assert!(max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis) as i64);
debug_assert!(max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis) as i64);
max_commitment_tx_output.1 = cmp::max(max_commitment_tx_output.1, value_to_remote_msat as u64);
}

Expand Down Expand Up @@ -1362,7 +1363,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
if msg.channel_reserve_satoshis < self.our_dust_limit_satoshis {
return Err(ChannelError::Close("Peer never wants payout outputs?"));
}
if msg.dust_limit_satoshis > Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis) {
if msg.dust_limit_satoshis > Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis) {
return Err(ChannelError::Close("Dust limit is bigger than our channel reverse"));
}
if msg.htlc_minimum_msat >= (self.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000 {
Expand Down Expand Up @@ -1424,7 +1425,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {

self.their_dust_limit_satoshis = msg.dust_limit_satoshis;
self.their_max_htlc_value_in_flight_msat = cmp::min(msg.max_htlc_value_in_flight_msat, self.channel_value_satoshis * 1000);
self.their_channel_reserve_satoshis = msg.channel_reserve_satoshis;
self.local_channel_reserve_satoshis = msg.channel_reserve_satoshis;
self.their_htlc_minimum_msat = msg.htlc_minimum_msat;
self.their_to_self_delay = msg.to_self_delay;
self.their_max_accepted_htlcs = msg.max_accepted_htlcs;
Expand Down Expand Up @@ -1696,7 +1697,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
if htlc_inbound_value_msat + msg.amount_msat > Channel::<ChanSigner>::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis) {
return Err(ChannelError::Close("Remote HTLC add would put them over our max HTLC value"));
}
// Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
// Check remote_channel_reserve_satoshis (we're getting paid, so they have to at least meet
// the reserve_satoshis we told them to always have as direct payment so that they lose
// something if we punish them for broadcasting an old state).
// Note that we don't really care about having a small/no to_remote output in our local
Expand All @@ -1716,8 +1717,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
removed_outbound_total_msat += htlc.amount_msat;
}
}
if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
return Err(ChannelError::Close("Remote HTLC add would put them over their reserve value"));
if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
return Err(ChannelError::Close("Remote HTLC add would put them under their reserve value"));
}
if self.next_remote_htlc_id != msg.htlc_id {
return Err(ChannelError::Close("Remote skipped HTLC ID"));
Expand Down Expand Up @@ -1847,7 +1848,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let num_htlcs = local_commitment_tx.1;
let total_fee: u64 = feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;

if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + self.their_channel_reserve_satoshis {
let remote_reserve_we_require = Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future, best to keep bugfixes to separate commits from pure-renaming. Otherwise I'll think you're trying to slip something in :p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha noted, thanks

if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + remote_reserve_we_require {
return Err((None, ChannelError::Close("Funding remote cannot afford proposed new fee")));
}
}
Expand Down Expand Up @@ -3033,7 +3035,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
ChannelValueStat {
value_to_self_msat: self.value_to_self_msat,
channel_value_msat: self.channel_value_satoshis * 1000,
channel_reserve_msat: self.their_channel_reserve_satoshis * 1000,
channel_reserve_msat: self.local_channel_reserve_satoshis * 1000,
pending_outbound_htlcs_amount_msat: self.pending_outbound_htlcs.iter().map(|ref h| h.amount_msat).sum::<u64>(),
pending_inbound_htlcs_amount_msat: self.pending_inbound_htlcs.iter().map(|ref h| h.amount_msat).sum::<u64>(),
holding_cell_outbound_amount_msat: {
Expand Down Expand Up @@ -3321,7 +3323,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
push_msat: self.channel_value_satoshis * 1000 - self.value_to_self_msat,
dust_limit_satoshis: self.our_dust_limit_satoshis,
max_htlc_value_in_flight_msat: Channel::<ChanSigner>::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
channel_reserve_satoshis: Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
channel_reserve_satoshis: Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis),
htlc_minimum_msat: self.our_htlc_minimum_msat,
feerate_per_kw: fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) as u32,
to_self_delay: self.our_to_self_delay,
Expand Down Expand Up @@ -3354,7 +3356,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
temporary_channel_id: self.channel_id,
dust_limit_satoshis: self.our_dust_limit_satoshis,
max_htlc_value_in_flight_msat: Channel::<ChanSigner>::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis),
channel_reserve_satoshis: Channel::<ChanSigner>::get_our_channel_reserve_satoshis(self.channel_value_satoshis),
channel_reserve_satoshis: Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis),
htlc_minimum_msat: self.our_htlc_minimum_msat,
minimum_depth: self.minimum_depth,
to_self_delay: self.our_to_self_delay,
Expand Down Expand Up @@ -3550,10 +3552,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
}

// Check self.their_channel_reserve_satoshis (the amount we must keep as
// reserve for them to have something to claim if we misbehave)
if self.value_to_self_msat < self.their_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
return Err(ChannelError::Ignore("Cannot send value that would put us over their reserve value"));
// Check self.local_channel_reserve_satoshis (the amount we must keep as
// reserve for the remote to have something to claim if we misbehave)
if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
return Err(ChannelError::Ignore("Cannot send value that would put us under local channel reserve value"));
}

// Now update local state:
Expand Down Expand Up @@ -4019,7 +4021,7 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
self.their_dust_limit_satoshis.write(writer)?;
self.our_dust_limit_satoshis.write(writer)?;
self.their_max_htlc_value_in_flight_msat.write(writer)?;
self.their_channel_reserve_satoshis.write(writer)?;
self.local_channel_reserve_satoshis.write(writer)?;
self.their_htlc_minimum_msat.write(writer)?;
self.our_htlc_minimum_msat.write(writer)?;
self.their_to_self_delay.write(writer)?;
Expand Down Expand Up @@ -4175,7 +4177,7 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for Channel<C
let their_dust_limit_satoshis = Readable::read(reader)?;
let our_dust_limit_satoshis = Readable::read(reader)?;
let their_max_htlc_value_in_flight_msat = Readable::read(reader)?;
let their_channel_reserve_satoshis = Readable::read(reader)?;
let local_channel_reserve_satoshis = Readable::read(reader)?;
let their_htlc_minimum_msat = Readable::read(reader)?;
let our_htlc_minimum_msat = Readable::read(reader)?;
let their_to_self_delay = Readable::read(reader)?;
Expand Down Expand Up @@ -4254,7 +4256,7 @@ impl<ChanSigner: ChannelKeys + Readable> ReadableArgs<Arc<Logger>> for Channel<C
their_dust_limit_satoshis,
our_dust_limit_satoshis,
their_max_htlc_value_in_flight_msat,
their_channel_reserve_satoshis,
local_channel_reserve_satoshis,
their_htlc_minimum_msat,
our_htlc_minimum_msat,
their_to_self_delay,
Expand Down
18 changes: 9 additions & 9 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,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::<EnforcingChannelKeys>::get_our_channel_reserve_satoshis(channel_value_sat);
let channel_reserve_satoshis = Channel::<EnforcingChannelKeys>::get_remote_channel_reserve_satoshis(channel_value_sat);
let push_msat = (channel_value_sat - channel_reserve_satoshis) * 1000;

// Have node0 initiate a channel to node1 with aforementioned parameters
Expand Down Expand Up @@ -1578,9 +1578,9 @@ fn do_channel_reserve_test(test_recv: bool) {
// attempt to get channel_reserve violation
let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value + 1);
unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
assert_eq!(err, "Cannot send value that would put us over their reserve value"));
assert_eq!(err, "Cannot send value that would put us under local channel reserve value"));
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us over their reserve value".to_string(), 1);
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 1);
}

// adding pending output
Expand All @@ -1603,9 +1603,9 @@ fn do_channel_reserve_test(test_recv: bool) {
{
let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_2 + 1);
unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
assert_eq!(err, "Cannot send value that would put us over their reserve value"));
assert_eq!(err, "Cannot send value that would put us under local channel reserve value"));
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us over their reserve value".to_string(), 2);
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 2);
}

{
Expand Down Expand Up @@ -1640,7 +1640,7 @@ fn do_channel_reserve_test(test_recv: bool) {
assert_eq!(nodes[1].node.list_channels().len(), 1);
assert_eq!(nodes[1].node.list_channels().len(), 1);
let err_msg = check_closed_broadcast!(nodes[1], true).unwrap();
assert_eq!(err_msg.data, "Remote HTLC add would put them over their reserve value");
assert_eq!(err_msg.data, "Remote HTLC add would put them under their reserve value");
check_added_monitors!(nodes[1], 1);
return;
}
Expand All @@ -1666,9 +1666,9 @@ fn do_channel_reserve_test(test_recv: bool) {
{
let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_22+1);
unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
assert_eq!(err, "Cannot send value that would put us over their reserve value"));
assert_eq!(err, "Cannot send value that would put us under local channel reserve value"));
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us over their reserve value".to_string(), 3);
nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 3);
}

let (route_22, our_payment_hash_22, our_payment_preimage_22) = get_route_and_payment_hash!(recv_value_22);
Expand Down Expand Up @@ -5977,7 +5977,7 @@ fn test_update_add_htlc_bolt2_receiver_sender_can_afford_amount_sent() {

assert!(nodes[1].node.list_channels().is_empty());
let err_msg = check_closed_broadcast!(nodes[1], true).unwrap();
assert_eq!(err_msg.data, "Remote HTLC add would put them over their reserve value");
assert_eq!(err_msg.data, "Remote HTLC add would put them under their reserve value");
check_added_monitors!(nodes[1], 1);
}

Expand Down