Merge pull request #3457 from arik-so/min_relay_fee_fix

Fix min relay fee to be 1s/vB

Author: TheBlueMatt

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:
 /// <https://github.com/ElementsProject/lightning/commit/2e687b9b352c9092b5e8bd4a688916ac50b44af0>

lightning/src/chain/onchaintx.rs

@@ -215,6 +215,7 @@ pub(crate) enum OnchainClaim {
 }
 
 /// Represents the different feerate strategies a pending request can use when generating a claim.
+#[derive(Debug)]
 pub(crate) enum FeerateStrategy {
 	/// We must reuse the most recently used feerate, if any.
 	RetryPrevious,

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,10 +1117,10 @@ 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));
+				return Some((input_amounts.saturating_sub(new_fee), feerate));
 			}
 		} else {
 			if let Some((new_fee, feerate)) = compute_fee_from_spent_amounts(input_amounts, predicted_weight, conf_target, fee_estimator, logger) {
@@ -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<F: Deref, L: Logger>(
 	input_amounts: u64, predicted_weight: u64, conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L
 ) -> Option<(u64, u64)>
@@ -1270,16 +1270,20 @@ fn compute_fee_from_spent_amounts<F: Deref, L: Logger>(
 /// respect BIP125 rules 3) and 4) and if required adjust the new fee to meet the RBF policy
 /// requirement.
 fn feerate_bump<F: Deref, L: Logger>(
-	predicted_weight: u64, input_amounts: u64, previous_feerate: u64, feerate_strategy: &FeerateStrategy,
-	conf_target: ConfirmationTarget, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L,
+	predicted_weight: u64, input_amounts: u64, dust_limit_sats: u64, previous_feerate: u64,
+	feerate_strategy: &FeerateStrategy, conf_target: ConfirmationTarget,
+	fee_estimator: &LowerBoundedFeeEstimator<F>, 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_debug!(logger, "Initiating fee rate bump from {} s/kWU ({} s) to {} s/kWU ({} s) using {:?} strategy", previous_feerate, previous_fee, new_feerate, new_fee, feerate_strategy);
 		match feerate_strategy {
 			FeerateStrategy::RetryPrevious => {
 				let previous_fee = previous_feerate * predicted_weight / 1000;
@@ -1297,15 +1301,12 @@ 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;
-				if input_amounts <= bumped_fee {
-					log_warn!(logger, "Can't 25% bump new claiming tx, amount {} is too small", input_amounts);
-					return None;
-				}
+
 				(bumped_fee, bumped_feerate)
 			},
 		}
 	} else {
-		log_warn!(logger, "Can't new-estimation bump new claiming tx, amount {} is too small", input_amounts);
+		log_warn!(logger, "Can't bump new claiming tx, input amount {} is too small", input_amounts);
 		return None;
 	};
 
@@ -1316,22 +1317,31 @@ 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 new_fee = cmp::max(new_fee, previous_fee + min_relay_fee);
+
+	if new_fee > naive_new_fee {
+		log_debug!(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.saturating_sub(new_fee);
+	if remaining_output_amount < dust_limit_sats {
+		log_warn!(logger, "Can't 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_debug!(logger, "Fee rate bumped by {}s from {} s/KWU ({} s) to {} s/KWU ({} s)", new_fee - previous_fee, previous_feerate, previous_fee, new_feerate, 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 +1359,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 +1672,84 @@ 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;
+
+		{
+			// Check underflow doesn't occur
+			let predicted_weight_units = 1000;
+			let input_satoshis = 505;
+
+			let logger = TestLogger::new();
+			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 bump new claiming tx, input amount 505 is too small").unwrap(), 1);
+		}
+
+		{
+			// Check post-25%-bump-underflow scenario satisfying the following constraints:
+			// input - fee = 546
+			// input - fee * 1.25 = -1
+
+			// We accomplish that scenario with the following values:
+			// input = 2734
+			// fee = 2188
+
+			let predicted_weight_units = 1000;
+			let input_satoshis = 2734;
+
+			let logger = TestLogger::new();
+			let bumped_fee_rate = feerate_bump(predicted_weight_units, input_satoshis, 546, 2188, &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 bump new claiming tx, output amount 0 would end up below dust threshold 546").unwrap(), 1);
+		}
+
+		{
+			// Check that an output amount of 0 is caught
+			let predicted_weight_units = 1000;
+			let input_satoshis = 506;
+
+			let logger = TestLogger::new();
+			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 bump new claiming tx, output amount 0 would end up below dust threshold 546").unwrap(), 1);
+		}
+
+		{
+			// Check that dust_threshold - 1 is blocked
+			let predicted_weight_units = 1000;
+			let input_satoshis = 1051;
+
+			let logger = TestLogger::new();
+			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 bump new claiming tx, output amount 545 would end up below dust threshold 546").unwrap(), 1);
+		}
+
+		{
+			let predicted_weight_units = 1000;
+			let input_satoshis = 1052;
+
+			let logger = TestLogger::new();
+			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(), 1);
+		}
+	}
 }

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;