Skip to content

More flexible fee rate estimates #2660

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
Oct 20, 2023
Merged

More flexible fee rate estimates #2660

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
7 changes: 4 additions & 3 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,10 @@ impl FeeEstimator for FuzzEstimator {
// always return a HighPriority feerate here which is >= the maximum Normal feerate and a
// Background feerate which is <= the minimum Normal feerate.
match conf_target {
ConfirmationTarget::HighPriority => MAX_FEE,
ConfirmationTarget::Background|ConfirmationTarget::MempoolMinimum => 253,
ConfirmationTarget::Normal => cmp::min(self.ret_val.load(atomic::Ordering::Acquire), MAX_FEE),
ConfirmationTarget::MaxAllowedNonAnchorChannelRemoteFee => MAX_FEE * 10,
ConfirmationTarget::OnChainSweep => MAX_FEE,
ConfirmationTarget::ChannelCloseMinimum|ConfirmationTarget::AnchorChannelFee|ConfirmationTarget::MinAllowedAnchorChannelRemoteFee|ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee => 253,
ConfirmationTarget::NonAnchorChannelFee => cmp::min(self.ret_val.load(atomic::Ordering::Acquire), MAX_FEE),
}
}
}
Expand Down
116 changes: 99 additions & 17 deletions lightning/src/chain/chaininterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,103 @@ pub trait BroadcasterInterface {
/// estimation.
#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
pub enum ConfirmationTarget {
/// We'd like a transaction to confirm in the future, but don't want to commit most of the fees
/// required to do so yet. The remaining fees will come via a Child-Pays-For-Parent (CPFP) fee
/// bump of the transaction.
///
/// The feerate returned should be the absolute minimum feerate required to enter most node
/// mempools across the network. Note that if you are not able to obtain this feerate estimate,
/// you should likely use the furthest-out estimate allowed by your fee estimator.
MempoolMinimum,
/// We are happy with a transaction confirming slowly, at least within a day or so worth of
/// blocks.
Background,
/// We'd like a transaction to confirm without major delayed, i.e., within the next 12-24 blocks.
Normal,
/// We'd like a transaction to confirm in the next few blocks.
HighPriority,
/// We have some funds available on chain which we need to spend prior to some expiry time at
/// which point our counterparty may be able to steal them. Generally we have in the high tens
/// to low hundreds of blocks to get our transaction on-chain, but we shouldn't risk too low a
/// fee - this should be a relatively high priority feerate.
OnChainSweep,
/// The highest feerate we will allow our channel counterparty to have in a non-anchor channel.
///
/// This is the feerate on the transaction which we (or our counterparty) will broadcast in
/// order to close the channel unilaterally. Because our counterparty must ensure they can
/// always broadcast the latest state, this value being too low will cause immediate
/// force-closures.
///
/// Allowing this value to be too high can allow our counterparty to burn our HTLC outputs to
/// dust, which can result in HTLCs failing or force-closures (when the dust HTLCs exceed
/// [`ChannelConfig::max_dust_htlc_exposure`]).
///
/// Because most nodes use a feerate estimate which is based on a relatively high priority
/// transaction entering the current mempool, setting this to a small multiple of your current
/// high priority feerate estimate should suffice.
///
/// [`ChannelConfig::max_dust_htlc_exposure`]: crate::util::config::ChannelConfig::max_dust_htlc_exposure
MaxAllowedNonAnchorChannelRemoteFee,
/// This is the lowest feerate we will allow our channel counterparty to have in an anchor
/// channel in order to close the channel if a channel party goes away. Because our counterparty
/// must ensure they can always broadcast the latest state, this value being too high will cause
/// immediate force-closures.
///
/// This needs to be sufficient to get into the mempool when the channel needs to
/// be force-closed. Setting too low may result in force-closures. Because this is for anchor
/// channels, we can always bump the feerate later, the feerate here only needs to suffice to
/// enter the mempool.
///
/// A good estimate is the expected mempool minimum at the time of force-closure. Obviously this
/// is not an estimate which is very easy to calculate because we do not know the future. Using
/// a simple long-term fee estimate or tracking of the mempool minimum is a good approach to
/// ensure you can always close the channel. A future change to Bitcoin's P2P network
/// (package relay) may obviate the need for this entirely.
MinAllowedAnchorChannelRemoteFee,
/// The lowest feerate we will allow our channel counterparty to have in a non-anchor channel.
/// This needs to be sufficient to get confirmed when the channel needs to be force-closed.
/// Setting too low may result in force-closures.
///
/// This is the feerate on the transaction which we (or our counterparty) will broadcast in
/// order to close the channel if a channel party goes away. Because our counterparty must
/// ensure they can always broadcast the latest state, this value being too high will cause
/// immediate force-closures.
///
/// This feerate represents the fee we pick now, which must be sufficient to enter a block at an
/// arbitrary time in the future. Obviously this is not an estimate which is very easy to
/// calculate. This can leave channels subject to being unable to close if feerates rise, and in
/// general you should prefer anchor channels to ensure you can increase the feerate when the
/// transactions need broadcasting.
///
/// Do note some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw),
/// causing occasional issues with feerate disagreements between an initiator that wants a
/// feerate of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. If your fee
/// estimator rounds subtracting 250 to your desired feerate here can help avoid this issue.
///
/// [`ChannelConfig::max_dust_htlc_exposure`]: crate::util::config::ChannelConfig::max_dust_htlc_exposure
MinAllowedNonAnchorChannelRemoteFee,
/// This is the feerate on the transaction which we (or our counterparty) will broadcast in
/// order to close the channel if a channel party goes away.
///
/// This needs to be sufficient to get into the mempool when the channel needs to
/// be force-closed. Setting too low may result in force-closures. Because this is for anchor
/// channels, it can be a low value as we can always bump the feerate later.
///
/// A good estimate is the expected mempool minimum at the time of force-closure. Obviously this
/// is not an estimate which is very easy to calculate because we do not know the future. Using
/// a simple long-term fee estimate or tracking of the mempool minimum is a good approach to
/// ensure you can always close the channel. A future change to Bitcoin's P2P network
/// (package relay) may obviate the need for this entirely.
AnchorChannelFee,
/// Lightning is built around the ability to broadcast a transaction in the future to close our
/// channel and claim all pending funds. In order to do so, non-anchor channels are built with
/// transactions which we need to be able to broadcast at some point in the future.
///
/// This feerate represents the fee we pick now, which must be sufficient to enter a block at an
/// arbitrary time in the future. Obviously this is not an estimate which is very easy to
/// calculate, so most lightning nodes use some relatively high-priority feerate using the
/// current mempool. This leaves channels subject to being unable to close if feerates rise, and
/// in general you should prefer anchor channels to ensure you can increase the feerate when the
/// transactions need broadcasting.
///
/// Since this should represent the feerate of a channel close that does not need fee
/// bumping, this is also used as an upper bound for our attempted feerate when doing cooperative
/// closure of any channel.
NonAnchorChannelFee,
/// When cooperatively closing a channel, this is the minimum feerate we will accept.
/// Recommended at least within a day or so worth of blocks.
///
/// This will also be used when initiating a cooperative close of a channel. When closing a
/// channel you can override this fee by using
/// [`ChannelManager::close_channel_with_feerate_and_script`].
///
/// [`ChannelManager::close_channel_with_feerate_and_script`]: crate::ln::channelmanager::ChannelManager::close_channel_with_feerate_and_script
ChannelCloseMinimum,
}

/// A trait which should be implemented to provide feerate information on a number of time
Expand Down Expand Up @@ -135,7 +217,7 @@ mod tests {
let test_fee_estimator = &TestFeeEstimator { sat_per_kw };
let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator);

assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background), FEERATE_FLOOR_SATS_PER_KW);
assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::AnchorChannelFee), FEERATE_FLOOR_SATS_PER_KW);
}

#[test]
Expand All @@ -144,6 +226,6 @@ mod tests {
let test_fee_estimator = &TestFeeEstimator { sat_per_kw };
let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator);

assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background), sat_per_kw);
assert_eq!(fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::AnchorChannelFee), sat_per_kw);
}
}
4 changes: 2 additions & 2 deletions lightning/src/chain/onchaintx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
if cached_request.is_malleable() {
if cached_request.requires_external_funding() {
let target_feerate_sat_per_1000_weight = cached_request.compute_package_feerate(
fee_estimator, ConfirmationTarget::HighPriority, force_feerate_bump
fee_estimator, ConfirmationTarget::OnChainSweep, force_feerate_bump
);
if let Some(htlcs) = cached_request.construct_malleable_package_with_external_funding(self) {
return Some((
Expand Down Expand Up @@ -631,7 +631,7 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
debug_assert_eq!(tx.txid(), self.holder_commitment.trust().txid(),
"Holder commitment transaction mismatch");

let conf_target = ConfirmationTarget::HighPriority;
let conf_target = ConfirmationTarget::OnChainSweep;
let package_target_feerate_sat_per_1000_weight = cached_request
.compute_package_feerate(fee_estimator, conf_target, force_feerate_bump);
if let Some(input_amount_sat) = output.funding_amount {
Expand Down
46 changes: 18 additions & 28 deletions lightning/src/chain/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::ln::PaymentPreimage;
use crate::ln::chan_utils::{TxCreationKeys, HTLCOutputInCommitment};
use crate::ln::chan_utils;
use crate::ln::msgs::DecodeError;
use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT};
use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, fee_for_weight, FEERATE_FLOOR_SATS_PER_KW};
use crate::sign::WriteableEcdsaChannelSigner;
use crate::chain::onchaintx::{ExternalHTLCClaim, OnchainTxHandler};
use crate::util::logger::Logger;
Expand Down Expand Up @@ -1101,40 +1101,30 @@ impl Readable for PackageTemplate {
}

/// Attempt to propose a bumping fee for a transaction from its spent output's values and predicted
/// weight. We start with the highest priority feerate returned by the node's fee estimator then
/// fall-back to lower priorities until we have enough value available to suck from.
/// weight. We first try our [`OnChainSweep`] feerate, if it's not enough we try to sweep half of
/// the input amounts.
///
/// If the proposed fee is less than the available spent output's values, we return the proposed
/// fee and the corresponding updated feerate. If the proposed fee is equal or more than the
/// available spent output's values, we return nothing
/// fee and the corresponding updated feerate. If fee is under [`FEERATE_FLOOR_SATS_PER_KW`], we
/// return nothing.
///
/// [`OnChainSweep`]: crate::chain::chaininterface::ConfirmationTarget::OnChainSweep
/// [`FEERATE_FLOOR_SATS_PER_KW`]: crate::chain::chaininterface::MIN_RELAY_FEE_SAT_PER_1000_WEIGHT
fn compute_fee_from_spent_amounts<F: Deref, L: Deref>(input_amounts: u64, predicted_weight: usize, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(u64, u64)>
where F::Target: FeeEstimator,
L::Target: Logger,
{
let mut updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64;
let mut fee = updated_feerate * (predicted_weight as u64) / 1000;
if input_amounts <= fee {
updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal) as u64;
fee = updated_feerate * (predicted_weight as u64) / 1000;
if input_amounts <= fee {
updated_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background) as u64;
fee = updated_feerate * (predicted_weight as u64) / 1000;
if input_amounts <= fee {
log_error!(logger, "Failed to generate an on-chain punishment tx as even low priority fee ({} sat) was more than the entire claim balance ({} sat)",
fee, input_amounts);
None
} else {
log_warn!(logger, "Used low priority fee for on-chain punishment tx as high priority fee was more than the entire claim balance ({} sat)",
input_amounts);
Some((fee, updated_feerate))
}
} else {
log_warn!(logger, "Used medium priority fee for on-chain punishment tx as high priority fee was more than the entire claim balance ({} sat)",
input_amounts);
Some((fee, updated_feerate))
}
let sweep_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::OnChainSweep);
let fee_rate = cmp::min(sweep_feerate, compute_feerate_sat_per_1000_weight(input_amounts / 2, predicted_weight as u64)) as u64;
let fee = fee_rate * (predicted_weight as u64) / 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use fee_for_weight here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using that breaks a test, it has an extra -1 in it

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because fee_for_weight will round up. That test isn't very accurate to begin with because it's estimating the witness weight, which can be off by a few units.


// if the fee rate is below the floor, we don't sweep
if fee_rate < FEERATE_FLOOR_SATS_PER_KW as u64 {
log_error!(logger, "Failed to generate an on-chain tx with fee ({} sat/kw) was less than the floor ({} sat/kw)",
fee_rate, FEERATE_FLOOR_SATS_PER_KW);
None
} else {
Some((fee, updated_feerate))
Some((fee, fee_rate))
}
}

Expand Down
34 changes: 14 additions & 20 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
match self.config.options.max_dust_htlc_exposure {
MaxDustHTLCExposure::FeeRateMultiplier(multiplier) => {
let feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(
ConfirmationTarget::HighPriority);
ConfirmationTarget::OnChainSweep);
feerate_per_kw as u64 * multiplier
},
MaxDustHTLCExposure::FixedLimitMsat(limit) => limit,
Expand Down Expand Up @@ -2155,28 +2155,20 @@ impl<SP: Deref> Channel<SP> where
// apply to channels supporting anchor outputs since HTLC transactions are pre-signed with a
// zero fee, so their fee is no longer considered to determine dust limits.
if !channel_type.supports_anchors_zero_fee_htlc_tx() {
let upper_limit = cmp::max(250 * 25,
fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10);
let upper_limit =
fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::MaxAllowedNonAnchorChannelRemoteFee) as u64;
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)));
}
}

// We can afford to use a lower bound with anchors than previously since we can now bump
// fees when broadcasting our commitment. However, we must still make sure we meet the
// minimum mempool feerate, until package relay is deployed, such that we can ensure the
// commitment transaction propagates throughout node mempools on its own.
let lower_limit_conf_target = if channel_type.supports_anchors_zero_fee_htlc_tx() {
ConfirmationTarget::MempoolMinimum
ConfirmationTarget::MinAllowedAnchorChannelRemoteFee
} else {
ConfirmationTarget::Background
ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee
};
let lower_limit = fee_estimator.bounded_sat_per_1000_weight(lower_limit_conf_target);
// Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
// occasional issues with feerate disagreements between an initiator that wants a feerate
// of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
// sat/kw before the comparison here.
if feerate_per_kw + 250 < lower_limit {
if feerate_per_kw < lower_limit {
if let Some(cur_feerate) = cur_feerate_per_kw {
if feerate_per_kw > cur_feerate {
log_warn!(logger,
Expand All @@ -2185,7 +2177,7 @@ impl<SP: Deref> Channel<SP> where
return Ok(());
}
}
return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", 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)));
}
Ok(())
}
Expand Down Expand Up @@ -4155,8 +4147,10 @@ impl<SP: Deref> Channel<SP> where
// Propose a range from our current Background feerate to our Normal feerate plus our
// force_close_avoidance_max_fee_satoshis.
// If we fail to come to consensus, we'll have to force-close.
let mut proposed_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background);
let normal_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
let mut proposed_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::ChannelCloseMinimum);
// Use NonAnchorChannelFee because this should be an estimate for a channel close
// that we don't expect to need fee bumping
let normal_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee);
let mut proposed_max_feerate = if self.context.is_outbound() { normal_feerate } else { u32::max_value() };

// The spec requires that (when the channel does not have anchors) we only send absolute
Expand Down Expand Up @@ -5727,9 +5721,9 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
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::MempoolMinimum
ConfirmationTarget::AnchorChannelFee
} else {
ConfirmationTarget::Normal
ConfirmationTarget::NonAnchorChannelFee
};
let commitment_feerate = fee_estimator.bounded_sat_per_1000_weight(commitment_conf_target);

Expand Down Expand Up @@ -6019,7 +6013,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
// whatever reason.
if self.context.channel_type.supports_anchors_zero_fee_htlc_tx() {
self.context.channel_type.clear_anchors_zero_fee_htlc_tx();
self.context.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
self.context.feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::NonAnchorChannelFee);
assert!(!self.context.channel_transaction_parameters.channel_type_features.supports_anchors_nonzero_fee_htlc_tx());
} else if self.context.channel_type.supports_scid_privacy() {
self.context.channel_type.clear_scid_privacy();
Expand Down
Loading