Skip to content

Consider anchor outputs value throughout balance checks and computations #2674

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 5 commits into from
Oct 20, 2023
Merged

Consider anchor outputs value throughout balance checks and computations #2674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37a3a03
Run chanmon_consistency_test with anchor outputs channels
wpaulino Oct 19, 2023
d7672d4
Consider anchor outputs value in get_available_balances
wpaulino Oct 19, 2023
297390a
Consider anchor outputs value on inbound HTLCs
wpaulino Oct 19, 2023
834f4d7
Consider anchor outputs value on channel open
wpaulino Oct 19, 2023
27fba2d
Only account for fee spike buffer multiple on non-anchor channels
wpaulino Oct 19, 2023
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
53 changes: 44 additions & 9 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ fn send_hop_payment(source: &ChanMan, middle: &ChanMan, middle_chan_id: u64, des
}

#[inline]
pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) {
let out = SearchingOutput::new(underlying_out);
let broadcast = Arc::new(TestBroadcaster{});
let router = FuzzRouter {};
Expand All @@ -450,6 +450,10 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
let mut config = UserConfig::default();
config.channel_config.forwarding_fee_proportional_millionths = 0;
config.channel_handshake_config.announced_channel = true;
if anchors {
config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
config.manually_accept_inbound_channels = true;
}
let network = Network::Bitcoin;
let best_block_timestamp = genesis_block(network).header.time;
let params = ChainParameters {
Expand All @@ -473,6 +477,10 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
let mut config = UserConfig::default();
config.channel_config.forwarding_fee_proportional_millionths = 0;
config.channel_handshake_config.announced_channel = true;
if anchors {
config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
config.manually_accept_inbound_channels = true;
}

let mut monitors = HashMap::new();
let mut old_monitors = $old_monitors.latest_monitors.lock().unwrap();
Expand Down Expand Up @@ -509,7 +517,7 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {

let mut channel_txn = Vec::new();
macro_rules! make_channel {
($source: expr, $dest: expr, $chan_id: expr) => { {
($source: expr, $dest: expr, $dest_keys_manager: expr, $chan_id: expr) => { {
$source.peer_connected(&$dest.get_our_node_id(), &Init {
features: $dest.init_features(), networks: None, remote_network_address: None
}, true).unwrap();
Expand All @@ -528,6 +536,22 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {

$dest.handle_open_channel(&$source.get_our_node_id(), &open_channel);
let accept_channel = {
if anchors {
let events = $dest.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
if let events::Event::OpenChannelRequest {
ref temporary_channel_id, ref counterparty_node_id, ..
} = events[0] {
let mut random_bytes = [0u8; 16];
random_bytes.copy_from_slice(&$dest_keys_manager.get_secure_random_bytes()[..16]);
let user_channel_id = u128::from_be_bytes(random_bytes);
$dest.accept_inbound_channel(
temporary_channel_id,
counterparty_node_id,
user_channel_id,
).unwrap();
} else { panic!("Wrong event type"); }
}
let events = $dest.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
if let events::MessageSendEvent::SendAcceptChannel { ref msg, .. } = events[0] {
Expand Down Expand Up @@ -639,8 +663,8 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {

let mut nodes = [node_a, node_b, node_c];

let chan_1_funding = make_channel!(nodes[0], nodes[1], 0);
let chan_2_funding = make_channel!(nodes[1], nodes[2], 1);
let chan_1_funding = make_channel!(nodes[0], nodes[1], keys_manager_b, 0);
let chan_2_funding = make_channel!(nodes[1], nodes[2], keys_manager_c, 1);

for node in nodes.iter() {
confirm_txn!(node);
Expand Down Expand Up @@ -1199,7 +1223,10 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
0x6d => { send_hop_payment(&nodes[2], &nodes[1], chan_b, &nodes[0], chan_a, 1, &mut payment_id, &mut payment_idx); },

0x80 => {
let max_feerate = last_htlc_clear_fee_a * FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
let mut max_feerate = last_htlc_clear_fee_a;
if !anchors {
max_feerate *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
}
if fee_est_a.ret_val.fetch_add(250, atomic::Ordering::AcqRel) + 250 > max_feerate {
fee_est_a.ret_val.store(max_feerate, atomic::Ordering::Release);
}
Expand All @@ -1208,7 +1235,10 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
0x81 => { fee_est_a.ret_val.store(253, atomic::Ordering::Release); nodes[0].maybe_update_chan_fees(); },

0x84 => {
let max_feerate = last_htlc_clear_fee_b * FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
let mut max_feerate = last_htlc_clear_fee_b;
if !anchors {
max_feerate *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
}
if fee_est_b.ret_val.fetch_add(250, atomic::Ordering::AcqRel) + 250 > max_feerate {
fee_est_b.ret_val.store(max_feerate, atomic::Ordering::Release);
}
Expand All @@ -1217,7 +1247,10 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
0x85 => { fee_est_b.ret_val.store(253, atomic::Ordering::Release); nodes[1].maybe_update_chan_fees(); },

0x88 => {
let max_feerate = last_htlc_clear_fee_c * FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
let mut max_feerate = last_htlc_clear_fee_c;
if !anchors {
max_feerate *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE as u32;
}
if fee_est_c.ret_val.fetch_add(250, atomic::Ordering::AcqRel) + 250 > max_feerate {
fee_est_c.ret_val.store(max_feerate, atomic::Ordering::Release);
}
Expand Down Expand Up @@ -1338,10 +1371,12 @@ impl<O: Output> SearchingOutput<O> {
}

pub fn chanmon_consistency_test<Out: Output>(data: &[u8], out: Out) {
do_test(data, out);
do_test(data, out.clone(), false);
do_test(data, out, true);
}

#[no_mangle]
pub extern "C" fn chanmon_consistency_run(data: *const u8, datalen: usize) {
do_test(unsafe { std::slice::from_raw_parts(data, datalen) }, test_logger::DevNull{});
do_test(unsafe { std::slice::from_raw_parts(data, datalen) }, test_logger::DevNull{}, false);
do_test(unsafe { std::slice::from_raw_parts(data, datalen) }, test_logger::DevNull{}, true);
}
94 changes: 60 additions & 34 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {

cur_holder_commitment_transaction_number: u64,
cur_counterparty_commitment_transaction_number: u64,
value_to_self_msat: u64, // Excluding all pending_htlcs, excluding fees
value_to_self_msat: u64, // Excluding all pending_htlcs, fees, and anchor outputs
pending_inbound_htlcs: Vec<InboundHTLCOutput>,
pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
Expand Down Expand Up @@ -1673,6 +1673,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {

let mut available_capacity_msat = outbound_capacity_msat;

let anchor_outputs_value_msat = if context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
} else {
0
};
if context.is_outbound() {
// We should mind channel commit tx fee when computing how much of the available capacity
// can be used in the next htlc. Mirrors the logic in send_htlc.
Expand All @@ -1687,14 +1692,19 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}

let htlc_above_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000, HTLCInitiator::LocalOffered);
let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * context.next_local_commit_tx_fee_msat(htlc_above_dust, Some(()));
let mut max_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_above_dust, Some(()));
let htlc_dust = HTLCCandidate::new(real_dust_limit_timeout_sat * 1000 - 1, HTLCInitiator::LocalOffered);
let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * context.next_local_commit_tx_fee_msat(htlc_dust, Some(()));
let mut min_reserved_commit_tx_fee_msat = context.next_local_commit_tx_fee_msat(htlc_dust, Some(()));
if !context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
max_reserved_commit_tx_fee_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
min_reserved_commit_tx_fee_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
}

// We will first subtract the fee as if we were above-dust. Then, if the resulting
// value ends up being below dust, we have this fee available again. In that case,
// match the value to right-below-dust.
let mut capacity_minus_commitment_fee_msat: i64 = (available_capacity_msat as i64) - (max_reserved_commit_tx_fee_msat as i64);
let mut capacity_minus_commitment_fee_msat: i64 = available_capacity_msat as i64 -
max_reserved_commit_tx_fee_msat as i64 - anchor_outputs_value_msat as i64;
if capacity_minus_commitment_fee_msat < (real_dust_limit_timeout_sat as i64) * 1000 {
let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat;
debug_assert!(one_htlc_difference_msat != 0);
Expand All @@ -1719,7 +1729,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
let remote_balance_msat = (context.channel_value_satoshis * 1000 - context.value_to_self_msat)
.saturating_sub(inbound_stats.pending_htlcs_value_msat);

if remote_balance_msat < max_reserved_commit_tx_fee_msat + holder_selected_chan_reserve_msat {
if remote_balance_msat < max_reserved_commit_tx_fee_msat + holder_selected_chan_reserve_msat + anchor_outputs_value_msat {
// If another HTLC's fee would reduce the remote's balance below the reserve limit
// we've selected for them, we can only send dust HTLCs.
available_capacity_msat = cmp::min(available_capacity_msat, real_dust_limit_success_sat * 1000 - 1);
Expand Down Expand Up @@ -2119,7 +2129,7 @@ fn commit_tx_fee_sat(feerate_per_kw: u32, num_htlcs: usize, channel_type_feature

// Get the fee cost in MSATS of a commitment tx with a given number of HTLC outputs.
// Note that num_htlcs should not include dust HTLCs.
fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
pub(crate) fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
// Note that we need to divide before multiplying to round properly,
// since the lowest denomination of bitcoin on-chain is the satoshi.
(commitment_tx_base_weight(channel_type_features) + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000
Expand Down Expand Up @@ -2766,6 +2776,7 @@ impl<SP: Deref> Channel<SP> where
if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat {
return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat)));
}

// Check holder_selected_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).
Expand Down Expand Up @@ -2825,30 +2836,40 @@ impl<SP: Deref> Channel<SP> where

// Check that the remote can afford to pay for this HTLC on-chain at the current
// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
};
if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
};

if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < self.context.holder_selected_channel_reserve_satoshis * 1000 {
return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
{
let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
};
let anchor_outputs_value_msat = if !self.context.is_outbound() && self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
} else {
0
};
if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(anchor_outputs_value_msat) < remote_commit_tx_fee_msat {
return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
};
if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(remote_commit_tx_fee_msat).saturating_sub(anchor_outputs_value_msat) < self.context.holder_selected_channel_reserve_satoshis * 1000 {
return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
}
}

let anchor_outputs_value_msat = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
} else {
0
};
if !self.context.is_outbound() {
// `2 *` and `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
// the spec because in the spec, the fee spike buffer requirement doesn't exist on the
// receiver's side, only on the sender's.
// Note that when we eventually remove support for fee updates and switch to anchor output
// fees, we will drop the `2 *`, since we no longer be as sensitive to fee spikes. But, keep
// the extra htlc when calculating the next remote commitment transaction fee as we should
// still be able to afford adding this HTLC plus one more future HTLC, regardless of being
// sensitive to fee spikes.
// `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
// the spec because the fee spike buffer requirement doesn't exist on the receiver's
// side, only on the sender's. Note that with anchor outputs we are no longer as
// sensitive to fee spikes, so we need to account for them.
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
if pending_remote_value_msat - msg.amount_msat - self.context.holder_selected_channel_reserve_satoshis * 1000 < remote_fee_cost_incl_stuck_buffer_msat {
let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
if !self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
remote_fee_cost_incl_stuck_buffer_msat *= FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE;
}
if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(self.context.holder_selected_channel_reserve_satoshis * 1000).saturating_sub(anchor_outputs_value_msat) < remote_fee_cost_incl_stuck_buffer_msat {
// Note that if the pending_forward_status is not updated here, then it's because we're already failing
// the HTLC, i.e. its status is already set to failing.
log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.context.channel_id());
Expand All @@ -2858,7 +2879,7 @@ impl<SP: Deref> Channel<SP> where
// Check that they won't violate our local required channel reserve by adding this HTLC.
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat + anchor_outputs_value_msat {
return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
}
}
Expand Down Expand Up @@ -5721,16 +5742,16 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
let channel_type = Self::get_initial_channel_type(&config, their_features);
debug_assert!(channel_type.is_subset(&channelmanager::provided_channel_type_features(&config)));

let commitment_conf_target = if channel_type.supports_anchors_zero_fee_htlc_tx() {
ConfirmationTarget::AnchorChannelFee
let (commitment_conf_target, anchor_outputs_value_msat) = if channel_type.supports_anchors_zero_fee_htlc_tx() {
(ConfirmationTarget::AnchorChannelFee, ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000)
} else {
ConfirmationTarget::NonAnchorChannelFee
(ConfirmationTarget::NonAnchorChannelFee, 0)
};
let commitment_feerate = fee_estimator.bounded_sat_per_1000_weight(commitment_conf_target);

let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
let commitment_tx_fee = commit_tx_fee_msat(commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, &channel_type);
if value_to_self_msat < commitment_tx_fee {
if value_to_self_msat.saturating_sub(anchor_outputs_value_msat) < commitment_tx_fee {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not adjust for this in commit_tx_fee_msat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

return Err(APIError::APIMisuseError{ err: format!("Funding amount ({}) can't even pay fee for initial commitment transaction fee of {}.", value_to_self_msat / 1000, commitment_tx_fee / 1000) });
}

Expand Down Expand Up @@ -6347,13 +6368,18 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {

// check if the funder's amount for the initial commitment tx is sufficient
// for full fee payment plus a few HTLCs to ensure the channel will be useful.
let anchor_outputs_value = if channel_type.supports_anchors_zero_fee_htlc_tx() {
ANCHOR_OUTPUT_VALUE_SATOSHI * 2
} else {
0
};
let funders_amount_msat = msg.funding_satoshis * 1000 - msg.push_msat;
let commitment_tx_fee = commit_tx_fee_msat(msg.feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT, &channel_type) / 1000;
if funders_amount_msat / 1000 < commitment_tx_fee {
return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", funders_amount_msat / 1000, commitment_tx_fee)));
if (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value) < commitment_tx_fee {
return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", (funders_amount_msat / 1000).saturating_sub(anchor_outputs_value), commitment_tx_fee)));
}

let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee;
let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee - anchor_outputs_value;
// While it's reasonable for us to not meet the channel reserve initially (if they don't
// want to push much to us), our counterparty should always have more than our reserve.
if to_remote_satoshis < holder_selected_channel_reserve_satoshis {
Expand Down
Loading