diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs index 84281df1d7b..ebef21657b7 100644 --- a/lightning/src/chain/chaininterface.rs +++ b/lightning/src/chain/chaininterface.rs @@ -176,7 +176,7 @@ pub trait FeeEstimator { } /// Minimum relay fee as required by bitcoin network mempool policy. -pub const MIN_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 4000; +pub const INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT: u64 = 253; /// Minimum feerate that takes a sane approach to bitcoind weight-to-vbytes rounding. /// See the following Core Lightning commit for an explanation: /// diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs index 53bba3a754b..62d95b3a66e 100644 --- a/lightning/src/chain/package.rs +++ b/lightning/src/chain/package.rs @@ -31,7 +31,7 @@ use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA; use crate::ln::msgs::DecodeError; use crate::chain::channelmonitor::COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE; -use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, MIN_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW}; +use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT, compute_feerate_sat_per_1000_weight, FEERATE_FLOOR_SATS_PER_KW}; use crate::chain::transaction::MaybeSignedTransaction; use crate::sign::ecdsa::EcdsaChannelSigner; use crate::chain::onchaintx::{FeerateStrategy, ExternalHTLCClaim, OnchainTxHandler}; @@ -1117,8 +1117,8 @@ impl PackageTemplate { // If old feerate is 0, first iteration of this claim, use normal fee calculation if self.feerate_previous != 0 { if let Some((new_fee, feerate)) = feerate_bump( - predicted_weight, input_amounts, self.feerate_previous, feerate_strategy, - conf_target, fee_estimator, logger, + predicted_weight, input_amounts, dust_limit_sats, self.feerate_previous, + feerate_strategy, conf_target, fee_estimator, logger, ) { return Some((cmp::max(input_amounts as i64 - new_fee as i64, dust_limit_sats as i64) as u64, feerate)); } @@ -1243,7 +1243,7 @@ impl Readable for PackageTemplate { /// fee and the corresponding updated feerate. If fee is under [`FEERATE_FLOOR_SATS_PER_KW`], we /// return nothing. /// -/// [`FEERATE_FLOOR_SATS_PER_KW`]: crate::chain::chaininterface::MIN_RELAY_FEE_SAT_PER_1000_WEIGHT +/// [`FEERATE_FLOOR_SATS_PER_KW`]: crate::chain::chaininterface::INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT fn compute_fee_from_spent_amounts( input_amounts: u64, predicted_weight: u64, conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator, logger: &L ) -> Option<(u64, u64)> @@ -1270,16 +1270,20 @@ fn compute_fee_from_spent_amounts( /// respect BIP125 rules 3) and 4) and if required adjust the new fee to meet the RBF policy /// requirement. fn feerate_bump( - predicted_weight: u64, input_amounts: u64, previous_feerate: u64, feerate_strategy: &FeerateStrategy, - conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator, logger: &L, + predicted_weight: u64, input_amounts: u64, dust_limit_sats: u64, previous_feerate: u64, + feerate_strategy: &FeerateStrategy, conf_target: ConfirmationTarget, + fee_estimator: &LowerBoundedFeeEstimator, logger: &L, ) -> Option<(u64, u64)> where F::Target: FeeEstimator, { + let previous_fee = previous_feerate * predicted_weight / 1000; + // If old feerate inferior to actual one given back by Fee Estimator, use it to compute new fee... let (new_fee, new_feerate) = if let Some((new_fee, new_feerate)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, conf_target, fee_estimator, logger) { + log_trace!(logger, "Initiating fee rate bump from {} s/kWU ({} s) to {} s/kWU ({} s)", previous_feerate, previous_fee, new_feerate, new_fee); match feerate_strategy { FeerateStrategy::RetryPrevious => { let previous_fee = previous_feerate * predicted_weight / 1000; @@ -1297,10 +1301,19 @@ where // ...else just increase the previous feerate by 25% (because that's a nice number) let bumped_feerate = previous_feerate + (previous_feerate / 4); let bumped_fee = bumped_feerate * predicted_weight / 1000; + log_trace!(logger, "Attempting forced 25% fee rate bump from {} s/kWU ({} s) to {} s/kWU ({} s)", previous_feerate, previous_fee, bumped_feerate, bumped_fee); + if input_amounts <= bumped_fee { log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts); return None; } + + let remaining_output_amount = input_amounts - bumped_fee; + if remaining_output_amount < dust_limit_sats { + log_warn!(logger, "Can't new-estimation bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); + return None; + } + (bumped_fee, bumped_feerate) }, } @@ -1316,22 +1329,32 @@ where return Some((new_fee, new_feerate)); } - let previous_fee = previous_feerate * predicted_weight / 1000; - let min_relay_fee = MIN_RELAY_FEE_SAT_PER_1000_WEIGHT * predicted_weight / 1000; + let min_relay_fee = INCREMENTAL_RELAY_FEE_SAT_PER_1000_WEIGHT * predicted_weight / 1000; // BIP 125 Opt-in Full Replace-by-Fee Signaling // * 3. The replacement transaction pays an absolute fee of at least the sum paid by the original transactions. // * 4. The replacement transaction must also pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting. - let new_fee = if new_fee < previous_fee + min_relay_fee { - new_fee + previous_fee + min_relay_fee - new_fee - } else { - new_fee - }; - Some((new_fee, new_fee * 1000 / predicted_weight)) + let naive_new_fee = new_fee; + let naive_new_feerate = new_feerate; + let new_fee = cmp::max(new_fee, previous_fee + min_relay_fee); + + if new_fee > naive_new_fee { + log_trace!(logger, "Naive fee bump of {}s does not meet min relay fee requirements of {}s", naive_new_fee - previous_fee, min_relay_fee); + } + + let remaining_output_amount = input_amounts - new_fee; + if remaining_output_amount < dust_limit_sats { + log_warn!(logger, "Can't new-estimation bump new claiming tx, output amount {} would end up below dust threshold {}", remaining_output_amount, dust_limit_sats); + return None; + } + + let new_feerate = new_fee * 1000 / predicted_weight; + log_trace!(logger, "Fee rate bumped by {}s from {} s/KWU ({} s) to {} s/KWU ({} s) (naive: {} s/KWU ({} s))", new_fee - previous_fee, previous_feerate, previous_fee, new_feerate, new_fee, naive_new_feerate, naive_new_fee); + Some((new_fee, new_feerate)) } #[cfg(test)] mod tests { - use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedHTLCOutput, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc}; + use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageTemplate, PackageSolvingData, RevokedHTLCOutput, RevokedOutput, WEIGHT_REVOKED_OUTPUT, weight_offered_htlc, weight_received_htlc, feerate_bump}; use crate::chain::Txid; use crate::ln::chan_utils::HTLCOutputInCommitment; use crate::types::payment::{PaymentPreimage, PaymentHash}; @@ -1349,7 +1372,10 @@ mod tests { use bitcoin::secp256k1::{PublicKey,SecretKey}; use bitcoin::secp256k1::Secp256k1; + use crate::chain::chaininterface::{ConfirmationTarget, FeeEstimator, FEERATE_FLOOR_SATS_PER_KW, LowerBoundedFeeEstimator}; + use crate::chain::onchaintx::FeerateStrategy; use crate::types::features::ChannelTypeFeatures; + use crate::util::test_utils::TestLogger; fn fake_txid(n: u64) -> Txid { Transaction { @@ -1659,4 +1685,53 @@ mod tests { } } } + + struct TestFeeEstimator { + sat_per_kw: u32, + } + + impl FeeEstimator for TestFeeEstimator { + fn get_est_sat_per_1000_weight(&self, _: ConfirmationTarget) -> u32 { + self.sat_per_kw + } + } + + #[test] + fn test_feerate_bump() { + let sat_per_kw = FEERATE_FLOOR_SATS_PER_KW; + let test_fee_estimator = &TestFeeEstimator { sat_per_kw }; + let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator); + let fee_rate_strategy = FeerateStrategy::ForceBump; + let confirmation_target = ConfirmationTarget::UrgentOnChainSweep; + let logger = TestLogger::new(); + + { + let predicted_weight_units = 1000; + let input_satoshis = 861; + + let bumped_fee_rate = feerate_bump(predicted_weight_units, input_satoshis, 546, 253, &fee_rate_strategy, confirmation_target, &fee_estimator, &logger); + assert!(bumped_fee_rate.is_none()); + logger.assert_log_regex("lightning::chain::package", regex::Regex::new(r"Can't new-estimation bump new claiming tx, output amount 545 would end up below dust threshold 546").unwrap(), 1); + } + + { + let predicted_weight_units = 1000; + let input_satoshis = 862; + + let bumped_fee_rate = feerate_bump(predicted_weight_units, input_satoshis, 546, 253, &fee_rate_strategy, confirmation_target, &fee_estimator, &logger); + assert!(bumped_fee_rate.is_none()); + logger.assert_log_regex("lightning::chain::package", regex::Regex::new(r"Naive fee bump of 63s does not meet min relay fee requirements of 253s").unwrap(), 1); + logger.assert_log_regex("lightning::chain::package", regex::Regex::new(r"Can't new-estimation bump new claiming tx, output amount 356 would end up below dust threshold 546").unwrap(), 1); + } + + { + let predicted_weight_units = 1000; + let input_satoshis = 2000; + + let bumped_fee_rate = feerate_bump(predicted_weight_units, input_satoshis, 546, 253, &fee_rate_strategy, confirmation_target, &fee_estimator, &logger).unwrap(); + assert_eq!(bumped_fee_rate, (506, 506)); + logger.assert_log_regex("lightning::chain::package", regex::Regex::new(r"Naive fee bump of 63s does not meet min relay fee requirements of 253s").unwrap(), 2); + logger.assert_log_regex("lightning::chain::package", regex::Regex::new(r"Fee rate bumped by 253s from 253 s/KWU \(253 s\) to 506 s/KWU \(506 s\) \(naive: 316 s/KWU \(316 s\)\)").unwrap(), 1); + } + } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index f1bc11d421c..2a0550a5474 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1307,18 +1307,22 @@ fn test_duplicate_htlc_different_direction_onchain() { let chan_1 = create_announced_chan_between_nodes(&nodes, 0, 1); + // post-bump fee (288 satoshis) + dust threshold for output type (294 satoshis) = 582 + let payment_value_sats = 582; + let payment_value_msats = payment_value_sats * 1000; + // balancing send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000); let (payment_preimage, payment_hash, ..) = route_payment(&nodes[0], &vec!(&nodes[1])[..], 900_000); - let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[0], 800_000); + let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[0], payment_value_msats); let node_a_payment_secret = nodes[0].node.create_inbound_payment_for_hash(payment_hash, None, 7200, None).unwrap(); - send_along_route_with_secret(&nodes[1], route, &[&[&nodes[0]]], 800_000, payment_hash, node_a_payment_secret); + send_along_route_with_secret(&nodes[1], route, &[&[&nodes[0]]], payment_value_msats, payment_hash, node_a_payment_secret); // Provide preimage to node 0 by claiming payment nodes[0].node.claim_funds(payment_preimage); - expect_payment_claimed!(nodes[0], payment_hash, 800_000); + expect_payment_claimed!(nodes[0], payment_hash, payment_value_msats); check_added_monitors!(nodes[0], 1); // Broadcast node 1 commitment txn @@ -1327,7 +1331,7 @@ fn test_duplicate_htlc_different_direction_onchain() { assert_eq!(remote_txn[0].output.len(), 4); // 1 local, 1 remote, 1 htlc inbound, 1 htlc outbound let mut has_both_htlcs = 0; // check htlcs match ones committed for outp in remote_txn[0].output.iter() { - if outp.value.to_sat() == 800_000 / 1000 { + if outp.value.to_sat() == payment_value_sats { has_both_htlcs += 1; } else if outp.value.to_sat() == 900_000 / 1000 { has_both_htlcs += 1; @@ -1347,18 +1351,15 @@ fn test_duplicate_htlc_different_direction_onchain() { check_spends!(claim_txn[1], remote_txn[0]); check_spends!(claim_txn[2], remote_txn[0]); let preimage_tx = &claim_txn[0]; - let (preimage_bump_tx, timeout_tx) = if claim_txn[1].input[0].previous_output == preimage_tx.input[0].previous_output { - (&claim_txn[1], &claim_txn[2]) - } else { - (&claim_txn[2], &claim_txn[1]) - }; + let timeout_tx = claim_txn.iter().skip(1).find(|t| t.input[0].previous_output != preimage_tx.input[0].previous_output).unwrap(); + let preimage_bump_tx = claim_txn.iter().skip(1).find(|t| t.input[0].previous_output == preimage_tx.input[0].previous_output).unwrap(); assert_eq!(preimage_tx.input.len(), 1); assert_eq!(preimage_bump_tx.input.len(), 1); assert_eq!(preimage_tx.input.len(), 1); assert_eq!(preimage_tx.input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); // HTLC 1 <--> 0, preimage tx - assert_eq!(remote_txn[0].output[preimage_tx.input[0].previous_output.vout as usize].value.to_sat(), 800); + assert_eq!(remote_txn[0].output[preimage_tx.input[0].previous_output.vout as usize].value.to_sat(), payment_value_sats); assert_eq!(timeout_tx.input.len(), 1); assert_eq!(timeout_tx.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT); // HTLC 0 <--> 1, timeout tx @@ -7923,22 +7924,31 @@ fn test_bump_penalty_txn_on_remote_commitment() { let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000); - let (payment_preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], 3_000_000); - route_payment(&nodes[1], &vec!(&nodes[0])[..], 3000000).0; - - // Remote commitment txn with 4 outputs : to_local, to_remote, 1 outgoing HTLC, 1 incoming HTLC - let remote_txn = get_local_commitment_txn!(nodes[0], chan.2); - assert_eq!(remote_txn[0].output.len(), 4); - assert_eq!(remote_txn[0].input.len(), 1); - assert_eq!(remote_txn[0].input[0].previous_output.txid, chan.3.compute_txid()); - - // Claim a HTLC without revocation (provide B monitor with preimage) - nodes[1].node.claim_funds(payment_preimage); - expect_payment_claimed!(nodes[1], payment_hash, 3_000_000); - mine_transaction(&nodes[1], &remote_txn[0]); - check_added_monitors!(nodes[1], 2); - connect_blocks(&nodes[1], TEST_FINAL_CLTV); // Confirm blocks until the HTLC expires + let remote_txn = { + // post-bump fee (288 satoshis) + dust threshold for output type (294 satoshis) = 582 + let htlc_value_a_msats = 582_000; + let htlc_value_b_msats = 583_000; + + let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000); + let (payment_preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], htlc_value_a_msats); + route_payment(&nodes[1], &vec!(&nodes[0])[..], htlc_value_b_msats); + + // Remote commitment txn with 4 outputs : to_local, to_remote, 1 outgoing HTLC, 1 incoming HTLC + let remote_txn = get_local_commitment_txn!(nodes[0], chan.2); + assert_eq!(remote_txn[0].output.len(), 4); + assert_eq!(remote_txn[0].input.len(), 1); + assert_eq!(remote_txn[0].input[0].previous_output.txid, chan.3.compute_txid()); + + // Claim a HTLC without revocation (provide B monitor with preimage) + nodes[1].node.claim_funds(payment_preimage); + expect_payment_claimed!(nodes[1], payment_hash, htlc_value_a_msats); + mine_transaction(&nodes[1], &remote_txn[0]); + check_added_monitors!(nodes[1], 2); + connect_blocks(&nodes[1], TEST_FINAL_CLTV); // Confirm blocks until the HTLC expires + // depending on the block connection style, node 1 may have broadcast either 3 or 10 txs + + remote_txn + }; // One or more claim tx should have been broadcast, check it let timeout;