Split up `ConfirmationTarget` even more #3268

TheBlueMatt · 2024-08-22T19:25:08Z

This morning we had a sudden feerate spike where feerates spiked something like 100x between one or two blocks, causing my node (and a few other LDK users) to have a feerate estimate 100x lower than that of some of my peers. This cause the new "feerate expansion is treated as dust" logic to kick in and FC two of my channels.

Here we fix this issue by giving the "feerate expansion is treated as dust" logic its own ConfirmationTarget which can be yet more conservative so that we're always more conservative than our peers.

Further, because I FC'd while fees were incredibly high, I ended up spending nontrivial funds on the FC transactions, even though there was no particular need to rush - neither of the lost channels had HTLCs so they weren't trying to get confirmed immediately.

Here we fix this second issue by breaking up OnChainSweep into an urgent and non-urgent version.

codecov · 2024-08-22T19:59:56Z

Codecov Report

Attention: Patch coverage is 95.27027% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.54%. Comparing base (dced69d) to head (cf97cef).
Report is 45 commits behind head on main.

Files	Patch %	Lines
lightning/src/chain/channelmonitor.rs	82.05%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3268      +/-   ##
==========================================
+ Coverage   89.82%   90.54%   +0.71%     
==========================================
  Files         125      126       +1     
  Lines      102798   108880    +6082     
  Branches   102798   108880    +6082     
==========================================
+ Hits        92337    98580    +6243     
+ Misses       7743     7658      -85     
+ Partials     2718     2642      -76

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz · 2024-08-22T22:51:39Z

cc: @Beige-Coffee

tnull · 2024-08-23T08:14:43Z

lightning/src/chain/chaininterface.rs

@@ -49,6 +49,12 @@ pub trait BroadcasterInterface {
 /// estimation.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub enum ConfirmationTarget {
+	/// The most conservative feerate estimate available.


I'm not sure I can make sense of these docs. Reading "The most conservative feerate estimate available.", I'd plug in the mempool minimum and be done with it. Can we give some guidance what this actually means and how users would retrieve such a 'conservative estimate'?

Hmm, I can add "(i.e. highest)", but I feel like "the most conservative" is clear (as long as its clarified that it means highest, not lowest). It should be the most conservative you can go on your API, whatever API that is.

Would MaximumAcceptableFee be accurate/clearer? I agree that conservative could be read in the opposite way...

I don't think we want to communicate to folks that this has to be an "acceptable" fee that you might pay, but rather the maximum fee that a peer might reasonably pick without it clearly being an attack.

Fair enough. I tried to come up with something like MaxForceCloseAvoidanceFeerate or MaxFeerateFromPeer but it's a bit wordy and I think this is fine as-is with the docs clarification.

Or maybe just MaximumFeeEstimate?

That seems reasonable!

lightning/src/chain/chaininterface.rs

valentinewallace

Basically LGTM, I'm good with a squash

lightning/src/chain/chaininterface.rs

valentinewallace · 2024-08-26T17:39:09Z

lightning/src/chain/chaininterface.rs

@@ -49,6 +49,12 @@ pub trait BroadcasterInterface {
 /// estimation.
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)]
 pub enum ConfirmationTarget {
+	/// The most conservative feerate estimate available.


Fair enough. I tried to come up with something like MaxForceCloseAvoidanceFeerate or MaxFeerateFromPeer but it's a bit wordy and I think this is fine as-is with the docs clarification.

valentinewallace · 2024-08-26T17:54:20Z

lightning/src/chain/channelmonitor.rs

+		}
+		if let Some(txid) = self.prev_counterparty_commitment_txid {
+			if !self.counterparty_claimable_outpoints.get(&txid).unwrap().is_empty() {
+				return ConfirmationTarget::UrgentOnChainSweep;


FWIW, we don't have test coverage here

Btw, there still isn't test coverage here (won't hold the PR up over it though).

lightning/src/chain/package.rs

tnull

LGTM, feel free to squash.

TheBlueMatt · 2024-08-27T14:15:44Z

Squashed.

valentinewallace · 2024-08-27T14:28:01Z

CI is sad

TheBlueMatt · 2024-08-27T16:33:28Z

🤦

$ git diff-tree -U1 0a452bef6 76f6e9dbf
diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 84cc4bf26..b1b916250 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -99,3 +99,5 @@ impl FeeEstimator for FuzzEstimator {
 		match conf_target {
-			ConfirmationTarget::OnChainSweep => MAX_FEE,
+			ConfirmationTarget::MaximumFeeEstimate | ConfirmationTarget::UrgentOnChainSweep => {
+				MAX_FEE
+			},
 			ConfirmationTarget::ChannelCloseMinimum
diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index 121084b09..84281df1d 100644
--- a/lightning/src/chain/chaininterface.rs
+++ b/lightning/src/chain/chaininterface.rs
@@ -145,2 +145,3 @@ pub enum ConfirmationTarget {
 	///
+	/// [`ChannelMonitor`]: crate::chain::channelmonitor::ChannelMonitor
 	/// [`OutputSweeper`]: crate::util::sweep::OutputSweeper
diff --git a/lightning/src/chain/package.rs b/lightning/src/chain/package.rs
index 973390478..3dedf2914 100644
--- a/lightning/src/chain/package.rs
+++ b/lightning/src/chain/package.rs
@@ -1104,4 +1104,4 @@ impl Readable for PackageTemplate {
 /// Attempt to propose a bumping fee for a transaction from its spent output's values and predicted
-/// weight. We first try our [`OnChainSweep`] feerate, if it's not enough we try to sweep half of
-/// the input amounts.
+/// weight. We first try our estimator's feerate, if it's not enough we try to sweep half of the
+/// input amounts.
 ///
@@ -1111,3 +1111,2 @@ impl Readable for PackageTemplate {
 ///
-/// [`OnChainSweep`]: crate::chain::chaininterface::ConfirmationTarget::OnChainSweep
 /// [`FEERATE_FLOOR_SATS_PER_KW`]: crate::chain::chaininterface::MIN_RELAY_FEE_SAT_PER_1000_WEIGHT
diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs
index 100e2b53e..31c975c9c 100644
--- a/lightning/src/util/config.rs
+++ b/lightning/src/util/config.rs
@@ -516,4 +516,4 @@ pub struct ChannelConfig {
 	/// for anchor channels we expect our counterparty to use a relatively low feerate estimate
-	/// while we use [`ConfirmationTarget::OnChainSweep`] (which should be relatively high) and
-	/// feerate disagreement force-closures should only occur when theirs is higher than ours.
+	/// while we use [`ConfirmationTarget::MaximumFeeEstimate`] (which should be relatively high)
+	/// and feerate disagreement force-closures should only occur when theirs is higher than ours.
 	///
@@ -521,3 +521,3 @@ pub struct ChannelConfig {
 	///
-	/// [`ConfirmationTarget::OnChainSweep`]: crate::chain::chaininterface::ConfirmationTarget::OnChainSweep
+	/// [`ConfirmationTarget::MaximumFeeEstimate`]: crate::chain::chaininterface::ConfirmationTarget::MaximumFeeEstimate
 	pub max_dust_htlc_exposure: MaxDustHTLCExposure,
$

When we broke `ConfirmationTarget` out into task-specific names, we left `MaxDustHTLCExposure::FeeRateMultiplier` as using the "when we broadcast feerate" as we were mostly concerned about the dust thresholds on outbound channels where we pick the fee and drive our own funds to dust. In 51bf78d, that changed to include transaction fees on both inbound and outbound channels in our dust exposure amount, but we continued to use `ConfirmationTarget::OnChainSweep` for the fee estimator threshold. While the `MaxDustHTLCExposure::FeeRateMultiplier` value is quite conservative and shouldn't lead to force-closures unless feerate estimates disagree by something like 500 sat/vB (with only one HTLC active in a channel), this happened on Aug 22 when feerates spiked from 4 sat/vB to over 1000 sat/vB in one block. To avoid simple feerate estimate horizons causing this in the future, here we add a new `ConfirmationTarget::MaximumFeeEstimate` which is used for dust calculations. This allows users to split out the estimates they use for checking counterparty feerates from the estimates used for actual broadcasting.

When we force-close a channel, occasionally its due to feerate disagreements or other non-HTLC-related issues. In those cases, there's no reason to use a very urgent feerate estimate - we don't have any timers expiring soon. Instead, we should give users the information they need to be more economical on fees in this case, which we do here by splitting `OnChainSweep` into `UrgentOnChainSweep` and `NonUrgentOnChainSweep` `ConfirmationTarget`s.

valentinewallace · 2024-08-27T17:57:15Z

Benchmark is failing

TheBlueMatt · 2024-08-27T18:26:38Z

Grrrrr.

$ git diff-tree -U1 76f6e9dbf b0f5c19f4
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 8a07e18a4..ce16b6746 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -14200,3 +14200,3 @@ pub mod bench {
 		let tx_broadcaster = test_utils::TestBroadcaster::new(network);
-		let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
+		let fee_estimator = test_utils::TestFeeEstimator::new(253);
 		let logger_a = test_utils::TestLogger::with_id("node a".to_owned());
$

This will allow us to test `ConfirmationTarget`s used in functional tests by setting an override on just the target we expect to be used.

Our tests should never ignore the events generated as they provide critical context about what's happening in LDK. Here we fix `test_yield_anchors_events` to avoid doing so.

This updates `test_yield_anchors_events` to test both anchor channels with and without HTLCs, and relies on overriding only the singular expected `ConfirmationTarget` used, testing the new `ConfirmationTarget::UrgentOnChainSweep` use.

TheBlueMatt · 2024-08-27T19:58:01Z

Ugh, yet another one...

$ git diff-tree -U1 b0f5c19f4 cf97cefb4
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index dc80ecd70..69511d5ee 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -1109,3 +1109,3 @@ mod tests {
 	use std::sync::mpsc::SyncSender;
-	use std::sync::{Arc, Mutex};
+	use std::sync::Arc;
 	use std::time::Duration;
@@ -1128,3 +1128,3 @@ mod tests {
 	#[cfg(not(c_bindings))]
-	type LockingWrapper<T> = Mutex<T>;
+	type LockingWrapper<T> = std::sync::Mutex<T>;
$

TheBlueMatt added this to the 0.0.124 milestone Aug 22, 2024

tnull reviewed Aug 23, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from 4a0a596 to ef1fff4 Compare August 23, 2024 14:21

valentinewallace reviewed Aug 26, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from ef1fff4 to 5971471 Compare August 26, 2024 20:13

Beige-Coffee mentioned this pull request Aug 26, 2024

Update Fee Estimation Docs lightningdevkit/lightningdevkit.org#277

Merged

tnull reviewed Aug 27, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from d9815d0 to 0a452be Compare August 27, 2024 14:15

tnull previously approved these changes Aug 27, 2024

View reviewed changes

TheBlueMatt dismissed tnull’s stale review via 3bc61b7 August 27, 2024 16:32

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from 0a452be to 3bc61b7 Compare August 27, 2024 16:32

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from 3bc61b7 to 38bdd72 Compare August 27, 2024 16:39

valentinewallace previously approved these changes Aug 27, 2024

View reviewed changes

TheBlueMatt dismissed valentinewallace’s stale review via 76f6e9d August 27, 2024 16:48

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from 38bdd72 to 76f6e9d Compare August 27, 2024 16:48

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from 76f6e9d to b0f5c19 Compare August 27, 2024 18:26

valentinewallace previously approved these changes Aug 27, 2024

View reviewed changes

TheBlueMatt added 3 commits August 27, 2024 19:57

Add a constructor and per-target override to TestFeeEstimator

3f23e3c

This will allow us to test `ConfirmationTarget`s used in functional tests by setting an override on just the target we expect to be used.

Don't ignore events in test_yield_anchors_events

80dd594

Our tests should never ignore the events generated as they provide critical context about what's happening in LDK. Here we fix `test_yield_anchors_events` to avoid doing so.

TheBlueMatt dismissed valentinewallace’s stale review via cf97cef August 27, 2024 19:57

TheBlueMatt force-pushed the 2024-08-moar-feerate-categories branch from b0f5c19 to cf97cef Compare August 27, 2024 19:57

tnull approved these changes Aug 28, 2024

View reviewed changes

TheBlueMatt merged commit b706480 into lightningdevkit:main Aug 28, 2024
19 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split up `ConfirmationTarget` even more #3268

Split up `ConfirmationTarget` even more #3268

TheBlueMatt commented Aug 22, 2024

codecov bot commented Aug 22, 2024 •

edited

Loading

jkczyz commented Aug 22, 2024

tnull Aug 23, 2024

TheBlueMatt Aug 23, 2024

valentinewallace Aug 23, 2024

TheBlueMatt Aug 23, 2024

valentinewallace Aug 26, 2024

TheBlueMatt Aug 26, 2024

valentinewallace Aug 26, 2024

valentinewallace left a comment

valentinewallace Aug 26, 2024

valentinewallace Aug 26, 2024

valentinewallace Aug 27, 2024

tnull left a comment

TheBlueMatt commented Aug 27, 2024

valentinewallace commented Aug 27, 2024

TheBlueMatt commented Aug 27, 2024 •

edited

Loading

valentinewallace commented Aug 27, 2024

TheBlueMatt commented Aug 27, 2024

TheBlueMatt commented Aug 27, 2024

Split up ConfirmationTarget even more #3268

Split up ConfirmationTarget even more #3268

Conversation

TheBlueMatt commented Aug 22, 2024

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

jkczyz commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 27, 2024

valentinewallace commented Aug 27, 2024

TheBlueMatt commented Aug 27, 2024 • edited Loading

valentinewallace commented Aug 27, 2024

TheBlueMatt commented Aug 27, 2024

TheBlueMatt commented Aug 27, 2024

Split up `ConfirmationTarget` even more #3268

Split up `ConfirmationTarget` even more #3268

codecov bot commented Aug 22, 2024 •

edited

Loading

TheBlueMatt commented Aug 27, 2024 •

edited

Loading