Skip to content

Correct initial commitment tx fee affordability checks on open #1162

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 4 commits into from
Nov 23, 2021
Merged

Correct initial commitment tx fee affordability checks on open #1162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a33d3b9
Make Channel::commit_tx_fee_msat static and take fee explicitly
TheBlueMatt Nov 15, 2021
1d30e06
Rewrite test_update_fee_that_funder_cannot_afford to avoid magic
TheBlueMatt Nov 10, 2021
940ef05
Correct initial commitment tx fee affordability checks on open
TheBlueMatt Nov 10, 2021
13e4fd5
Test fixed channel reserve checks on channel open
TheBlueMatt Nov 10, 2021
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
4 changes: 2 additions & 2 deletions fuzz/src/full_stack.rs
View file
Open in desktop

Large diffs are not rendered by default.

56 changes: 33 additions & 23 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,12 @@ pub(crate) const FUNDING_CONF_DEADLINE_BLOCKS: u32 = 2016;
/// size 2. However, if the number of concurrent update_add_htlc is higher, this still
/// leads to a channel force-close. Ultimately, this is an issue coming from the
/// design of LN state machines, allowing asynchronous updates.
const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
pub(crate) const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;

/// When a channel is opened, we check that the funding amount is enough to pay for relevant
/// commitment transaction fees, with at least this many HTLCs present on the commitment
/// transaction (not counting the value of the HTLCs themselves).
pub(crate) const MIN_AFFORDABLE_HTLC_COUNT: usize = 4;

// TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
// has been completed, and then turn into a Channel to get compiler-time enforcement of things like
Expand Down Expand Up @@ -708,6 +713,12 @@ impl<Signer: Sign> Channel<Signer> {

let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);

let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
let commitment_tx_fee = Self::commit_tx_fee_msat(feerate, MIN_AFFORDABLE_HTLC_COUNT);
if value_to_self_msat < commitment_tx_fee {
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) });
}

let mut secp_ctx = Secp256k1::new();
secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes());

Expand Down Expand Up @@ -738,7 +749,7 @@ impl<Signer: Sign> Channel<Signer> {

cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
value_to_self_msat,

pending_inbound_htlcs: Vec::new(),
pending_outbound_htlcs: Vec::new(),
Expand Down Expand Up @@ -952,8 +963,6 @@ impl<Signer: Sign> Channel<Signer> {
// we either accept their preference or the preferences match
local_config.announced_channel = announce;

let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);

let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_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)));
Expand All @@ -966,17 +975,18 @@ impl<Signer: Sign> Channel<Signer> {
}

// check if the funder's amount for the initial commitment tx is sufficient
// for full fee payment
// for full fee payment plus a few HTLCs to ensure the channel will be useful.
let funders_amount_msat = msg.funding_satoshis * 1000 - msg.push_msat;
let lower_limit = background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
if funders_amount_msat < lower_limit {
return Err(ChannelError::Close(format!("Insufficient funding amount ({}) for initial commitment. Must be at least {}", funders_amount_msat, lower_limit)));
let commitment_tx_fee = Self::commit_tx_fee_msat(msg.feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT) / 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)));
}

let to_local_msat = msg.push_msat;
let to_remote_msat = funders_amount_msat - background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
if to_local_msat <= msg.channel_reserve_satoshis * 1000 && to_remote_msat <= holder_selected_channel_reserve_satoshis * 1000 {
return Err(ChannelError::Close("Insufficient funding amount for initial commitment".to_owned()));
let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee;
// 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 {
return Err(ChannelError::Close("Insufficient funding amount for initial reserve".to_owned()));
}

let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
Expand Down Expand Up @@ -2109,10 +2119,10 @@ impl<Signer: Sign> Channel<Signer> {

// 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(&self, num_htlcs: usize) -> u64 {
fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize) -> 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 + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * self.feerate_per_kw as u64 / 1000 * 1000
(COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000
}

// Get the fee cost in SATS of a commitment tx with a given number of HTLC outputs.
Expand Down Expand Up @@ -2186,12 +2196,12 @@ impl<Signer: Sign> Channel<Signer> {
}

let num_htlcs = included_htlcs + addl_htlcs;
let res = self.commit_tx_fee_msat(num_htlcs);
let res = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs);
#[cfg(any(test, feature = "fuzztarget"))]
{
let mut fee = res;
if fee_spike_buffer_htlc.is_some() {
fee = self.commit_tx_fee_msat(num_htlcs - 1);
fee = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs - 1);
}
let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len()
+ self.holding_cell_htlc_updates.len();
Expand Down Expand Up @@ -2264,12 +2274,12 @@ impl<Signer: Sign> Channel<Signer> {
}

let num_htlcs = included_htlcs + addl_htlcs;
let res = self.commit_tx_fee_msat(num_htlcs);
let res = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs);
#[cfg(any(test, feature = "fuzztarget"))]
{
let mut fee = res;
if fee_spike_buffer_htlc.is_some() {
fee = self.commit_tx_fee_msat(num_htlcs - 1);
fee = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs - 1);
}
let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
let commitment_tx_info = CommitmentTxInfoCached {
Expand Down Expand Up @@ -4896,7 +4906,7 @@ impl<Signer: Sign> Channel<Signer> {
&& info.next_holder_htlc_id == self.next_holder_htlc_id
&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
&& info.feerate == self.feerate_per_kw {
let actual_fee = self.commit_tx_fee_msat(commitment_stats.num_nondust_htlcs);
let actual_fee = Self::commit_tx_fee_msat(self.feerate_per_kw, commitment_stats.num_nondust_htlcs);
assert_eq!(actual_fee, info.fee);
}
}
Expand Down Expand Up @@ -5921,13 +5931,13 @@ mod tests {
// the dust limit check.
let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
let local_commit_fee_0_htlcs = node_a_chan.commit_tx_fee_msat(0);
let local_commit_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(node_a_chan.feerate_per_kw, 0);
assert_eq!(local_commit_tx_fee, local_commit_fee_0_htlcs);

// Finally, make sure that when Node A calculates the remote's commitment transaction fees, all
// of the HTLCs are seen to be above the dust limit.
node_a_chan.channel_transaction_parameters.is_outbound_from_holder = false;
let remote_commit_fee_3_htlcs = node_a_chan.commit_tx_fee_msat(3);
let remote_commit_fee_3_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(node_a_chan.feerate_per_kw, 3);
let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
let remote_commit_tx_fee = node_a_chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
Expand All @@ -5949,8 +5959,8 @@ mod tests {
let config = UserConfig::default();
let mut chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).unwrap();

let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0);
let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1);
let commitment_tx_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 0);
let commitment_tx_fee_1_htlc = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 1);

// If HTLC_SUCCESS_TX_WEIGHT and HTLC_TIMEOUT_TX_WEIGHT were swapped: then this HTLC would be
// counted as dust when it shouldn't be.
Expand Down
Loading