Skip to content
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

Add new config setting max_balance_dust_htlc_msat #1009

Merged
Merged
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
1 change: 1 addition & 0 deletions fuzz/src/chanmon_consistency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ fn check_api_err(api_err: APIError) {
_ if err.starts_with("Cannot send value that would put counterparty balance under holder-announced channel reserve value") => {},
_ if err.starts_with("Cannot send value that would overdraw remaining funds.") => {},
_ if err.starts_with("Cannot send value that would not leave enough to pay for fees.") => {},
_ if err.starts_with("Cannot send value that would put our exposure to dust HTLCs at") => {},
_ => panic!("{}", err),
}
},
Expand Down
142 changes: 117 additions & 25 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,14 @@ enum HTLCInitiator {
RemoteOffered,
}

/// An enum gathering stats on pending HTLCs, either inbound or outbound side.
struct HTLCStats {
pending_htlcs: u32,
pending_htlcs_value_msat: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggested nit: s/value/balance

Copy link
Author

@ariard ariard Aug 10, 2021

Choose a reason for hiding this comment

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

I think balance includes either the to_local or to_remote output so would like to avoid blurring meanings here.

on_counterparty_tx_dust_exposure_msat: u64,
on_holder_tx_dust_exposure_msat: u64,
}

/// Used when calculating whether we or the remote can afford an additional HTLC.
struct HTLCCandidate {
amount_msat: u64,
Expand Down Expand Up @@ -1816,32 +1824,63 @@ impl<Signer: Sign> Channel<Signer> {
Ok(())
}

/// Returns (inbound_htlc_count, htlc_inbound_value_msat)
fn get_inbound_pending_htlc_stats(&self) -> (u32, u64) {
let mut htlc_inbound_value_msat = 0;
/// Returns a HTLCStats about inbound pending htlcs
fn get_inbound_pending_htlc_stats(&self) -> HTLCStats {
let mut stats = HTLCStats {
pending_htlcs: self.pending_inbound_htlcs.len() as u32,
pending_htlcs_value_msat: 0,
on_counterparty_tx_dust_exposure_msat: 0,
on_holder_tx_dust_exposure_msat: 0,
};

let counterparty_dust_limit_timeout_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
let holder_dust_limit_success_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
for ref htlc in self.pending_inbound_htlcs.iter() {
htlc_inbound_value_msat += htlc.amount_msat;
stats.pending_htlcs_value_msat += htlc.amount_msat;
if htlc.amount_msat / 1000 < counterparty_dust_limit_timeout_sat {
stats.on_counterparty_tx_dust_exposure_msat += htlc.amount_msat;
}
if htlc.amount_msat / 1000 < holder_dust_limit_success_sat {
stats.on_holder_tx_dust_exposure_msat += htlc.amount_msat;
}
}
(self.pending_inbound_htlcs.len() as u32, htlc_inbound_value_msat)
stats
}

/// Returns (outbound_htlc_count, htlc_outbound_value_msat) *including* pending adds in our
/// holding cell.
fn get_outbound_pending_htlc_stats(&self) -> (u32, u64) {
let mut htlc_outbound_value_msat = 0;
/// Returns a HTLCStats about pending outbound htlcs, *including* pending adds in our holding cell.
fn get_outbound_pending_htlc_stats(&self) -> HTLCStats {
let mut stats = HTLCStats {
pending_htlcs: self.pending_outbound_htlcs.len() as u32,
pending_htlcs_value_msat: 0,
on_counterparty_tx_dust_exposure_msat: 0,
on_holder_tx_dust_exposure_msat: 0,
};

let counterparty_dust_limit_success_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
let holder_dust_limit_timeout_sat = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
for ref htlc in self.pending_outbound_htlcs.iter() {
htlc_outbound_value_msat += htlc.amount_msat;
stats.pending_htlcs_value_msat += htlc.amount_msat;
if htlc.amount_msat / 1000 < counterparty_dust_limit_success_sat {
stats.on_counterparty_tx_dust_exposure_msat += htlc.amount_msat;
}
if htlc.amount_msat / 1000 < holder_dust_limit_timeout_sat {
stats.on_holder_tx_dust_exposure_msat += htlc.amount_msat;
}
}

let mut htlc_outbound_count = self.pending_outbound_htlcs.len();
for update in self.holding_cell_htlc_updates.iter() {
if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update {
htlc_outbound_count += 1;
htlc_outbound_value_msat += amount_msat;
stats.pending_htlcs += 1;
stats.pending_htlcs_value_msat += amount_msat;
if *amount_msat / 1000 < counterparty_dust_limit_success_sat {
stats.on_counterparty_tx_dust_exposure_msat += amount_msat;
}
if *amount_msat / 1000 < holder_dust_limit_timeout_sat {
stats.on_holder_tx_dust_exposure_msat += amount_msat;
}
}
}

(htlc_outbound_count as u32, htlc_outbound_value_msat)
stats
}

/// Get the available (ie not including pending HTLCs) inbound and outbound balance in msat.
Expand All @@ -1853,11 +1892,11 @@ impl<Signer: Sign> Channel<Signer> {
(
cmp::max(self.channel_value_satoshis as i64 * 1000
- self.value_to_self_msat as i64
- self.get_inbound_pending_htlc_stats().1 as i64
- self.get_inbound_pending_htlc_stats().pending_htlcs_value_msat as i64
- Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) as i64 * 1000,
0) as u64,
cmp::max(self.value_to_self_msat as i64
- self.get_outbound_pending_htlc_stats().1 as i64
- self.get_outbound_pending_htlc_stats().pending_htlcs_value_msat as i64
- self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000,
0) as u64
)
Expand Down Expand Up @@ -2069,12 +2108,13 @@ impl<Signer: Sign> Channel<Signer> {
return Err(ChannelError::Close(format!("Remote side tried to send less than our minimum HTLC value. Lower limit: ({}). Actual: ({})", self.holder_htlc_minimum_msat, msg.amount_msat)));
}

let (inbound_htlc_count, htlc_inbound_value_msat) = self.get_inbound_pending_htlc_stats();
if inbound_htlc_count + 1 > OUR_MAX_HTLCS as u32 {
let inbound_stats = self.get_inbound_pending_htlc_stats();
let outbound_stats = self.get_outbound_pending_htlc_stats();
if inbound_stats.pending_htlcs + 1 > OUR_MAX_HTLCS as u32 {
return Err(ChannelError::Close(format!("Remote tried to push more than our max accepted HTLCs ({})", OUR_MAX_HTLCS)));
}
let holder_max_htlc_value_in_flight_msat = Channel::<Signer>::get_holder_max_htlc_value_in_flight_msat(self.channel_value_satoshis);
if htlc_inbound_value_msat + msg.amount_msat > holder_max_htlc_value_in_flight_msat {
if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > holder_max_htlc_value_in_flight_msat {
return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", holder_max_htlc_value_in_flight_msat)));
}
// Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
Expand All @@ -2098,8 +2138,28 @@ impl<Signer: Sign> Channel<Signer> {
}
}

let exposure_dust_limit_timeout_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
if msg.amount_msat / 1000 < exposure_dust_limit_timeout_sats {
let on_counterparty_tx_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat + msg.amount_msat;
if on_counterparty_tx_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx",
on_counterparty_tx_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat());
pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|7);
}
}

let exposure_dust_limit_success_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
if msg.amount_msat / 1000 < exposure_dust_limit_success_sats {
let on_holder_tx_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat + msg.amount_msat;
if on_holder_tx_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
log_info!(logger, "Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
on_holder_tx_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat());
pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|7);
}
}

let pending_value_to_self_msat =
self.value_to_self_msat + htlc_inbound_value_msat - removed_outbound_total_msat;
self.value_to_self_msat + inbound_stats.pending_htlcs_value_msat - removed_outbound_total_msat;
let pending_remote_value_msat =
self.channel_value_satoshis * 1000 - pending_value_to_self_msat;
if pending_remote_value_msat < msg.amount_msat {
Expand Down Expand Up @@ -3502,11 +3562,24 @@ impl<Signer: Sign> Channel<Signer> {
cmp::max(self.config.cltv_expiry_delta, MIN_CLTV_EXPIRY_DELTA)
}

pub fn get_max_dust_htlc_exposure_msat(&self) -> u64 {
self.config.max_dust_htlc_exposure_msat
}

#[cfg(test)]
pub fn get_feerate(&self) -> u32 {
self.feerate_per_kw
}

pub fn get_dust_buffer_feerate(&self) -> u32 {
ariard marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: s/feerate/feerate_per_kw

Copy link
Author

Choose a reason for hiding this comment

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

Right, but we express sat per kw in the whole codebase so kind of implicit I think.

// When calculating our exposure to dust HTLCs, we assume that the channel feerate
// may, at any point, increase by at least 10 sat/vB (i.e 2530 sat/kWU) or 25%,
// whichever is higher. This ensures that we aren't suddenly exposed to significantly
// more dust balance if the feerate increases when we have several HTLCs pending
// which are near the dust limit.
cmp::max(2530, self.feerate_per_kw * 1250 / 1000)
ariard marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 {
self.cur_holder_commitment_transaction_number + 1
}
Expand Down Expand Up @@ -4145,12 +4218,13 @@ impl<Signer: Sign> Channel<Signer> {
return Err(ChannelError::Ignore("Cannot send an HTLC while disconnected from channel counterparty".to_owned()));
}

let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
if outbound_htlc_count + 1 > self.counterparty_max_accepted_htlcs as u32 {
let inbound_stats = self.get_inbound_pending_htlc_stats();
let outbound_stats = self.get_outbound_pending_htlc_stats();
if outbound_stats.pending_htlcs + 1 > self.counterparty_max_accepted_htlcs as u32 {
return Err(ChannelError::Ignore(format!("Cannot push more than their max accepted HTLCs ({})", self.counterparty_max_accepted_htlcs)));
}
// Check their_max_htlc_value_in_flight_msat
if htlc_outbound_value_msat + amount_msat > self.counterparty_max_htlc_value_in_flight_msat {
if outbound_stats.pending_htlcs_value_msat + amount_msat > self.counterparty_max_htlc_value_in_flight_msat {
return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat)));
}

Expand All @@ -4165,7 +4239,25 @@ impl<Signer: Sign> Channel<Signer> {
}
}

let pending_value_to_self_msat = self.value_to_self_msat - htlc_outbound_value_msat;
let exposure_dust_limit_success_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
if amount_msat / 1000 < exposure_dust_limit_success_sats {
let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat + amount_msat;
if on_counterparty_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx",
on_counterparty_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat())));
}
}

let exposure_dust_limit_timeout_sats = (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
if amount_msat / 1000 < exposure_dust_limit_timeout_sats {
let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat + amount_msat;
if on_holder_dust_htlc_exposure_msat > self.get_max_dust_htlc_exposure_msat() {
return Err(ChannelError::Ignore(format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx",
on_holder_dust_htlc_exposure_msat, self.get_max_dust_htlc_exposure_msat())));
}
}

let pending_value_to_self_msat = self.value_to_self_msat - outbound_stats.pending_htlcs_value_msat;
if pending_value_to_self_msat < amount_msat {
return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, pending_value_to_self_msat)));
}
Expand Down
3 changes: 3 additions & 0 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1384,6 +1384,9 @@ pub fn test_default_channel_config() -> UserConfig {
// When most of our tests were written, the default HTLC minimum was fixed at 1000.
// It now defaults to 1, so we simply set it to the expected value here.
default_config.own_channel_config.our_htlc_minimum_msat = 1000;
// When most of our tests were written, we didn't have the notion of a `max_dust_htlc_exposure_msat`,
// It now defaults to 5_000_000 msat; to avoid interfering with tests we bump it to 50_000_000 msat.
default_config.channel_options.max_dust_htlc_exposure_msat = 50_000_000;
default_config
}

Expand Down
113 changes: 113 additions & 0 deletions lightning/src/ln/functional_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9660,3 +9660,116 @@ fn test_keysend_payments_to_private_node() {
pass_along_path(&nodes[0], &path, 10000, payment_hash, None, event, true, Some(test_preimage));
claim_payment(&nodes[0], &path, test_preimage);
}

fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, at_forward: bool, on_holder_tx: bool) {
// Test that we properly reject dust HTLC violating our `max_dust_htlc_exposure_msat` policy.
//
// At HTLC forward (`send_payment()`), if the sum of the trimmed-to-dust HTLC inbound and
// trimmed-to-dust HTLC outbound balance and this new payment as included on next counterparty
// commitment are above our `max_dust_htlc_exposure_msat`, we'll reject the update.
// At HTLC reception (`update_add_htlc()`), if the sum of the trimmed-to-dust HTLC inbound
// and trimmed-to-dust HTLC outbound balance and this new received HTLC as included on next
// counterparty commitment are above our `max_dust_htlc_exposure_msat`, we'll fail the update.
// Note, we return a `temporary_channel_failure` (0x1000 | 7), as the channel might be
// available again for HTLC processing once the dust bandwidth has cleared up.

let chanmon_cfgs = create_chanmon_cfgs(2);
let mut config = test_default_channel_config();
config.channel_options.max_dust_htlc_exposure_msat = 5_000_000; // default setting value
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(config)]);
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);

nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 1_000_000, 500_000_000, 42, None).unwrap();
let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
open_channel.max_htlc_value_in_flight_msat = 50_000_000;
open_channel.max_accepted_htlcs = 60;
nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel);
let mut accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
if on_holder_tx {
accept_channel.dust_limit_satoshis = 660;
}
nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &accept_channel);

let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], 1_000_000, 42);

if on_holder_tx {
if let Some(mut chan) = nodes[1].node.channel_state.lock().unwrap().by_id.get_mut(&temporary_channel_id) {
chan.holder_dust_limit_satoshis = 660;
}
}

nodes[0].node.funding_transaction_generated(&temporary_channel_id, tx.clone()).unwrap();
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()));
check_added_monitors!(nodes[1], 1);

nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id()));
check_added_monitors!(nodes[0], 1);

let (funding_locked, _) = create_chan_between_nodes_with_value_confirm(&nodes[0], &nodes[1], &tx);
let (announcement, as_update, bs_update) = create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked);
update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update);

if on_holder_tx {
if dust_outbound_balance {
for i in 0..2 {
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 2_300_000);
if let Err(_) = nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)) { panic!("Unexpected event at dust HTLC {}", i); }
}
} else {
for _ in 0..2 {
route_payment(&nodes[0], &[&nodes[1]], 2_300_000);
}
}
} else {
if dust_outbound_balance {
for i in 0..25 {
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 200_000); // + 177_000 msat of HTLC-success tx at 253 sats/kWU
if let Err(_) = nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)) { panic!("Unexpected event at dust HTLC {}", i); }
}
} else {
for _ in 0..25 {
route_payment(&nodes[0], &[&nodes[1]], 200_000); // + 167_000 msat of HTLC-timeout tx at 253 sats/kWU
}
}
}

if at_forward {
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], if on_holder_tx { 2_300_000 } else { 200_000 });
let mut config = UserConfig::default();
if on_holder_tx {
unwrap_send_err!(nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)), true, APIError::ChannelUnavailable { ref err }, assert_eq!(err, &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", 6_900_000, config.channel_options.max_dust_htlc_exposure_msat)));
} else {
unwrap_send_err!(nodes[1].node.send_payment(&route, payment_hash, &Some(payment_secret)), true, APIError::ChannelUnavailable { ref err }, assert_eq!(err, &format!("Cannot send value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", 5_200_000, config.channel_options.max_dust_htlc_exposure_msat)));
}
} else {
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1 ], if on_holder_tx { 2_300_000 } else { 200_000 });
nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
check_added_monitors!(nodes[0], 1);
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
let payment_event = SendEvent::from_event(events.remove(0));
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
if on_holder_tx {
nodes[1].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on holder commitment tx", 6_900_000, config.channel_options.max_dust_htlc_exposure_msat), 1);
} else {
nodes[1].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot accept value that would put our exposure to dust HTLCs at {} over the limit {} on counterparty commitment tx", 5_200_000, config.channel_options.max_dust_htlc_exposure_msat), 1);
}
}

let _ = nodes[1].node.get_and_clear_pending_msg_events();
let mut added_monitors = nodes[1].chain_monitor.added_monitors.lock().unwrap();
added_monitors.clear();
}

#[test]
fn test_max_dust_htlc_exposure() {
do_test_max_dust_htlc_exposure(true, true, true);
do_test_max_dust_htlc_exposure(false, true, true);
do_test_max_dust_htlc_exposure(false, false, true);
do_test_max_dust_htlc_exposure(false, false, false);
do_test_max_dust_htlc_exposure(true, true, false);
do_test_max_dust_htlc_exposure(true, false, false);
do_test_max_dust_htlc_exposure(true, false, true);
do_test_max_dust_htlc_exposure(false, true, false);
}
Loading