Skip to content

Automatically Update fees on outbound channels #985

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

Merged
32 changes: 30 additions & 2 deletions lightning/src/ln/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,12 @@ impl<Signer: Sign> Channel<Signer> {
if feerate_per_kw < lower_limit {
return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit)));
}
let upper_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 2;
// We only bound the fee updates on the upper side to prevent completely absurd feerates,
// always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee.
// We generally don't care too much if they set the feerate to something very high, but it
// could result in the channel being useless due to everything being dust.
let upper_limit = cmp::max(250 * 25,
Copy link

Choose a reason for hiding this comment

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

Should be 253 here ? I don't have all the computation back in mind, but iirc if you commit your transactions at 250 sat per kWU they won't meet Core's min relay fee due to the round up at vbytes to weight units conversion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we need to care given its * 25 - you aren't at risk of underestimating :)

fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10);
if feerate_per_kw as u64 > upper_limit {
return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit)));
}
Expand Down Expand Up @@ -3111,8 +3116,27 @@ impl<Signer: Sign> Channel<Signer> {
return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish".to_owned()));
}
Channel::<Signer>::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
let feerate_over_dust_buffer = msg.feerate_per_kw > self.get_dust_buffer_feerate();

self.pending_update_fee = Some((msg.feerate_per_kw, FeeUpdateState::RemoteAnnounced));
self.update_time_counter += 1;
// If the feerate has increased over the previous dust buffer (note that
// `get_dust_buffer_feerate` considers the `pending_update_fee` status), check that we
// won't be pushed over our dust exposure limit by the feerate increase.
if feerate_over_dust_buffer {
let inbound_stats = self.get_inbound_pending_htlc_stats();
let outbound_stats = self.get_outbound_pending_htlc_stats();
let holder_tx_dust_exposure = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat;
let counterparty_tx_dust_exposure = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat;
if holder_tx_dust_exposure > self.get_max_dust_htlc_exposure_msat() {
return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our own transactions (totaling {} msat)",
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, missing test coverage here and a few lines down

Copy link

Choose a reason for hiding this comment

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

I've a big wonder if this doesn't expose us to flood-and-loot style of attacks where a third-party to this link betting on upcoming feerate spikes forward through us a set of trimmed-to-dust HTLCs to trigger a force-close. I think it could be even done at scale across the network if this default configuration is widely deployed...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, but its better than force-closing on any feerate increase. That exposes us to the same set of issues, except instead of requiring some newly-dust HTLCs it just requires a feerate increase lol.

msg.feerate_per_kw, holder_tx_dust_exposure)));
}
if counterparty_tx_dust_exposure > self.get_max_dust_htlc_exposure_msat() {
return Err(ChannelError::Close(format!("Peer sent update_fee with a feerate ({}) which may over-expose us to dust-in-flight on our counterparty's transactions (totaling {} msat)",
msg.feerate_per_kw, counterparty_tx_dust_exposure)));
}
}
Ok(())
}

Expand Down Expand Up @@ -3684,7 +3708,11 @@ impl<Signer: Sign> Channel<Signer> {
// 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)
let mut feerate_per_kw = self.feerate_per_kw;
if let Some((feerate, _)) = self.pending_update_fee {
feerate_per_kw = cmp::max(feerate_per_kw, feerate);
}
cmp::max(2530, feerate_per_kw * 1250 / 1000)
}

pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 {
Expand Down