Merge pull request #3268 from TheBlueMatt/2024-08-moar-feerate-categories

Split up `ConfirmationTarget` even more

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -97,7 +97,9 @@ 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::OnChainSweep => MAX_FEE,
+			ConfirmationTarget::MaximumFeeEstimate | ConfirmationTarget::UrgentOnChainSweep => {
+				MAX_FEE
+			},
 			ConfirmationTarget::ChannelCloseMinimum
 			| ConfirmationTarget::AnchorChannelFee
 			| ConfirmationTarget::MinAllowedAnchorChannelRemoteFee

lightning-background-processor/src/lib.rs

@@ -1107,7 +1107,7 @@ mod tests {
 	use std::collections::VecDeque;
 	use std::path::PathBuf;
 	use std::sync::mpsc::SyncSender;
-	use std::sync::{Arc, Mutex};
+	use std::sync::Arc;
 	use std::time::Duration;
 	use std::{env, fs};
 
@@ -1126,7 +1126,7 @@ mod tests {
 	#[cfg(c_bindings)]
 	type LockingWrapper<T> = lightning::routing::scoring::MultiThreadedLockableScore<T>;
 	#[cfg(not(c_bindings))]
-	type LockingWrapper<T> = Mutex<T>;
+	type LockingWrapper<T> = std::sync::Mutex<T>;
 
 	type ChannelManager = channelmanager::ChannelManager<
 		Arc<ChainMonitor>,
@@ -1532,8 +1532,7 @@ mod tests {
 		let mut nodes = Vec::new();
 		for i in 0..num_nodes {
 			let tx_broadcaster = Arc::new(test_utils::TestBroadcaster::new(network));
-			let fee_estimator =
-				Arc::new(test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) });
+			let fee_estimator = Arc::new(test_utils::TestFeeEstimator::new(253));
 			let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", i)));
 			let genesis_block = genesis_block(network);
 			let network_graph = Arc::new(NetworkGraph::new(network, logger.clone()));

lightning/src/chain/chaininterface.rs

@@ -49,11 +49,19 @@ pub trait BroadcasterInterface {
 /// estimation.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub enum ConfirmationTarget {
+	/// The most aggressive (i.e. highest) feerate estimate available.
+	///
+	/// This is used to sanity-check our counterparty's feerates and should be as conservative as
+	/// possible to ensure that we don't confuse a peer using a very conservative estimator for one
+	/// trying to burn channel balance to dust.
+	MaximumFeeEstimate,
 	/// 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,
+	/// 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 (it doesn't have to happen in the next few blocks!), but we shouldn't risk too low
+	/// a fee - this should be a relatively high priority feerate.
+	UrgentOnChainSweep,
 	/// 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.
 	///
@@ -124,14 +132,18 @@ pub enum ConfirmationTarget {
 	///
 	/// [`ChannelManager::close_channel_with_feerate_and_script`]: crate::ln::channelmanager::ChannelManager::close_channel_with_feerate_and_script
 	ChannelCloseMinimum,
-	/// The feerate [`OutputSweeper`] will use on transactions spending
-	/// [`SpendableOutputDescriptor`]s after a channel closure.
+	/// The feerate used to claim on-chain funds when there is no particular urgency to do so.
+	///
+	/// It is used to get commitment transactions without any HTLCs confirmed in [`ChannelMonitor`]
+	/// and by  [`OutputSweeper`] on transactions spending [`SpendableOutputDescriptor`]s after a
+	/// channel closure.
 	///
 	/// Generally spending these outputs is safe as long as they eventually confirm, so a value
 	/// (slightly above) the mempool minimum should suffice. However, as this value will influence
 	/// how long funds will be unavailable after channel closure, [`FeeEstimator`] implementors
 	/// might want to choose a higher feerate to regain control over funds faster.
 	///
+	/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
 	/// [`OutputSweeper`]: crate::util::sweep::OutputSweeper
 	/// [`SpendableOutputDescriptor`]: crate::sign::SpendableOutputDescriptor
 	OutputSpendingFee,

lightning/src/chain/channelmonitor.rs

@@ -40,7 +40,7 @@ use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSe
 use crate::ln::channelmanager::{HTLCSource, SentHTLCId};
 use crate::chain;
 use crate::chain::{BestBlock, WatchedOutput};
-use crate::chain::chaininterface::{BroadcasterInterface, FeeEstimator, LowerBoundedFeeEstimator};
+use crate::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator};
 use crate::chain::transaction::{OutPoint, TransactionData};
 use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, SpendableOutputDescriptor, StaticPaymentOutputDescriptor, DelayedPaymentOutputDescriptor, ecdsa::EcdsaChannelSigner, SignerProvider, EntropySource};
 use crate::chain::onchaintx::{ClaimEvent, FeerateStrategy, OnchainTxHandler};
@@ -1902,8 +1902,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		let mut inner = self.inner.lock().unwrap();
 		let logger = WithChannelMonitor::from_impl(logger, &*inner, None);
 		let current_height = inner.best_block.height;
+		let conf_target = inner.closure_conf_target();
 		inner.onchain_tx_handler.rebroadcast_pending_claims(
-			current_height, FeerateStrategy::HighestOfPreviousOrNew, &broadcaster, &fee_estimator, &logger,
+			current_height, FeerateStrategy::HighestOfPreviousOrNew, &broadcaster, conf_target, &fee_estimator, &logger,
 		);
 	}
 
@@ -1927,8 +1928,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
 		let mut inner = self.inner.lock().unwrap();
 		let logger = WithChannelMonitor::from_impl(logger, &*inner, None);
 		let current_height = inner.best_block.height;
+		let conf_target = inner.closure_conf_target();
 		inner.onchain_tx_handler.rebroadcast_pending_claims(
-			current_height, FeerateStrategy::RetryPrevious, &broadcaster, &fee_estimator, &logger,
+			current_height, FeerateStrategy::RetryPrevious, &broadcaster, conf_target, &fee_estimator, &logger,
 		);
 	}
 
@@ -2684,6 +2686,30 @@ pub fn deliberately_bogus_accepted_htlc_witness() -> Vec<Vec<u8>> {
 }
 
 impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
+	/// Gets the [`ConfirmationTarget`] we should use when selecting feerates for channel closure
+	/// transactions for this channel right now.
+	fn closure_conf_target(&self) -> ConfirmationTarget {
+		// Treat the sweep as urgent as long as there is at least one HTLC which is pending on a
+		// valid commitment transaction.
+		if !self.current_holder_commitment_tx.htlc_outputs.is_empty() {
+			return ConfirmationTarget::UrgentOnChainSweep;
+		}
+		if self.prev_holder_signed_commitment_tx.as_ref().map(|t| !t.htlc_outputs.is_empty()).unwrap_or(false) {
+			return ConfirmationTarget::UrgentOnChainSweep;
+		}
+		if let Some(txid) = self.current_counterparty_commitment_txid {
+			if !self.counterparty_claimable_outpoints.get(&txid).unwrap().is_empty() {
+				return ConfirmationTarget::UrgentOnChainSweep;
+			}
+		}
+		if let Some(txid) = self.prev_counterparty_commitment_txid {
+			if !self.counterparty_claimable_outpoints.get(&txid).unwrap().is_empty() {
+				return ConfirmationTarget::UrgentOnChainSweep;
+			}
+		}
+		ConfirmationTarget::OutputSpendingFee
+	}
+
 	/// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither
 	/// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen
 	/// counterparty commitment transaction's secret, they are de facto pruned (we can use revocation key).
@@ -2928,7 +2954,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		macro_rules! claim_htlcs {
 			($commitment_number: expr, $txid: expr, $htlcs: expr) => {
 				let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info($commitment_number, $txid, None, $htlcs);
-				self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, fee_estimator, logger);
+				let conf_target = self.closure_conf_target();
+				self.onchain_tx_handler.update_claims_view_from_requests(htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger);
 			}
 		}
 		if let Some(txid) = self.current_counterparty_commitment_txid {
@@ -2976,7 +3003,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 				// block. Even if not, its a reasonable metric for the bump criteria on the HTLC
 				// transactions.
 				let (claim_reqs, _) = self.get_broadcasted_holder_claims(&holder_commitment_tx, self.best_block.height);
-				self.onchain_tx_handler.update_claims_view_from_requests(claim_reqs, self.best_block.height, self.best_block.height, broadcaster, fee_estimator, logger);
+				let conf_target = self.closure_conf_target();
+				self.onchain_tx_handler.update_claims_view_from_requests(claim_reqs, self.best_block.height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger);
 			}
 		}
 	}
@@ -3036,9 +3064,10 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		L::Target: Logger,
 	{
 		let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs(ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) });
+		let conf_target = self.closure_conf_target();
 		self.onchain_tx_handler.update_claims_view_from_requests(
 			claimable_outpoints, self.best_block.height, self.best_block.height, broadcaster,
-			fee_estimator, logger
+			conf_target, fee_estimator, logger,
 		);
 	}
 
@@ -3851,7 +3880,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			self.best_block = BestBlock::new(block_hash, height);
 			log_trace!(logger, "Best block re-orged, replaced with new block {} at height {}", block_hash, height);
 			self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height <= height);
-			self.onchain_tx_handler.block_disconnected(height + 1, broadcaster, fee_estimator, logger);
+			let conf_target = self.closure_conf_target();
+			self.onchain_tx_handler.block_disconnected(height + 1, broadcaster, conf_target, fee_estimator, logger);
 			Vec::new()
 		} else { Vec::new() }
 	}
@@ -4116,8 +4146,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 			}
 		}
 
-		self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, conf_height, self.best_block.height, broadcaster, fee_estimator, logger);
-		self.onchain_tx_handler.update_claims_view_from_matched_txn(&txn_matched, conf_height, conf_hash, self.best_block.height, broadcaster, fee_estimator, logger);
+		let conf_target = self.closure_conf_target();
+		self.onchain_tx_handler.update_claims_view_from_requests(claimable_outpoints, conf_height, self.best_block.height, broadcaster, conf_target, fee_estimator, logger);
+		self.onchain_tx_handler.update_claims_view_from_matched_txn(&txn_matched, conf_height, conf_hash, self.best_block.height, broadcaster, conf_target, fee_estimator, logger);
 
 		// Determine new outputs to watch by comparing against previously known outputs to watch,
 		// updating the latter in the process.
@@ -4156,7 +4187,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 		self.onchain_events_awaiting_threshold_conf.retain(|ref entry| entry.height < height);
 
 		let bounded_fee_estimator = LowerBoundedFeeEstimator::new(fee_estimator);
-		self.onchain_tx_handler.block_disconnected(height, broadcaster, &bounded_fee_estimator, logger);
+		let conf_target = self.closure_conf_target();
+		self.onchain_tx_handler.block_disconnected(height, broadcaster, conf_target, &bounded_fee_estimator, logger);
 
 		self.best_block = BestBlock::new(header.prev_blockhash, height - 1);
 	}
@@ -4190,7 +4222,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 
 		debug_assert!(!self.onchain_events_awaiting_threshold_conf.iter().any(|ref entry| entry.txid == *txid));
 
-		self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, fee_estimator, logger);
+		let conf_target = self.closure_conf_target();
+		self.onchain_tx_handler.transaction_unconfirmed(txid, broadcaster, conf_target, fee_estimator, logger);
 	}
 
 	/// Filters a block's `txdata` for transactions spending watched outputs or for any child
@@ -4983,7 +5016,7 @@ mod tests {
 	use crate::util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
 	use crate::util::ser::{ReadableArgs, Writeable};
 	use crate::util::logger::Logger;
-	use crate::sync::{Arc, Mutex};
+	use crate::sync::Arc;
 	use crate::io;
 	use crate::ln::features::ChannelTypeFeatures;
 
@@ -5083,7 +5116,7 @@ mod tests {
 		let secp_ctx = Secp256k1::new();
 		let logger = Arc::new(TestLogger::new());
 		let broadcaster = Arc::new(TestBroadcaster::new(Network::Testnet));
-		let fee_estimator = TestFeeEstimator { sat_per_kw: Mutex::new(253) };
+		let fee_estimator = TestFeeEstimator::new(253);
 
 		let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());