lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 367 forks source link

Split up `ConfirmationTarget` even more #3268

Closed TheBlueMatt closed 2 months ago

TheBlueMatt commented 2 months ago

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[bot] commented 2 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jkczyz commented 2 months ago

cc: @Beige-Coffee

TheBlueMatt commented 2 months ago

Squashed.

valentinewallace commented 2 months ago

CI is sad

TheBlueMatt commented 2 months ago

:facepalm:

$ 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,
$ 
valentinewallace commented 2 months ago

Benchmark is failing

TheBlueMatt commented 2 months ago

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());
$ 
TheBlueMatt commented 2 months ago

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>;
$