Force-close channels if their feerate gets stale without any update #3037

TheBlueMatt · 2024-05-06T00:46:11Z

For quite some time, LDK has force-closed channels if the peer
sends us a feerate update which is below our FeeEstimator's
concept of a channel lower-bound. This is intended to ensure that
channel feerates are always sufficient to get our commitment
transaction confirmed on-chain if we do need to force-close.

However, we've never checked our channel feerate regularly - if a
peer is offline (or just uninterested in updating the channel
feerate) and the prevailing feerates on-chain go up, we'll simply
ignore it and allow our commitment transaction to sit around with a
feerate too low to get confirmed.

Here we rectify this oversight by force-closing channels with stale
feerates, checking after each block. However, because fee
estimators are often buggy and force-closures piss off users, we
only do so rather conservatively. Specifically, we only force-close
if a channel's feerate is below the minimum FeeEstimator-provided
minimum across the last day.

Further, because fee estimators are often especially buggy on
startup (and because peers haven't had a chance to update the
channel feerates yet), we don't force-close channels until we have
a full day of feerate lower-bound history.

This should reduce the incidence of force-closures substantially,
but it is expected this will still increase force-closures somewhat
substantially depending on the users' FeeEstimator.

Fixes #993
Previous attempt was #1056

I thought about just closing #993 entirely without addressing it as we really don't want to do this, we really want Bitcoin Core 28 to ship with package relay, users to use anchor channels, and never force-close a channel for too-low-feerate issues again.

However, if we don't do this there's kinda no point in force-closing for too-low-feerate at all - it means an honest node that just has a bad fee estimator will get force-closed on but a malicious node will just be mum as feerates go up and leave us high and dry.

codecov-commenter · 2024-05-06T00:53:21Z

Codecov Report

Attention: Patch coverage is 55.80524% with 118 lines in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (1d0c6c6) to head (17b77e0).

Files	Patch %	Lines
lightning/src/ln/channel.rs	35.22%	97 Missing and 6 partials ⚠️
lightning/src/ln/channelmanager.rs	70.58%	14 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3037      +/-   ##
==========================================
- Coverage   89.84%   89.81%   -0.04%     
==========================================
  Files         119      119              
  Lines       97811    97914     +103     
  Branches    97811    97914     +103     
==========================================
+ Hits        87883    87942      +59     
- Misses       7364     7396      +32     
- Partials     2564     2576      +12

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

benthecarman

Would be good if there was a way to disable this functionality, I assume a lot of people will not want their channels randomly force closing

TheBlueMatt · 2024-05-06T16:52:16Z

Because this uses the existing FeeEstimator MinAllowedAnchorChannelRemoteFee and MinAllowedNonAnchorChannelRemoteFee values, setting them to 0/253 will disable this entirely including the existing force-closes from peers sending us low values. Are you anticipating a desire to disable this without disabling the checking when peers do send update_fee?

valentinewallace · 2024-05-30T15:35:21Z

Because this uses the existing FeeEstimator MinAllowedAnchorChannelRemoteFee and MinAllowedNonAnchorChannelRemoteFee values, setting them to 0/253 will disable this entirely including the existing force-closes from peers sending us low values. Are you anticipating a desire to disable this without disabling the checking when peers do send update_fee?

Thoughts on this @benthecarman?

benthecarman · 2024-05-30T15:47:40Z

yeah i guess that is fine

valentinewallace · 2024-05-30T16:52:30Z

lightning/src/ln/channelmanager.rs

+	/// We only keep this in memory as we assume any feerates we receive immediately after startup
+	/// may be bunk (as they often are if Bitcoin Core crashes) and want to delay taking any


Are we likely to receive a very low feerate right after startup if bitcoin core crashed, setting the bar too low for FC? Might we want to wait 1-3 blocks after startup before starting to store any feerates?

That's okay, after a few extra hours the min will fall off our horizon and we'll FC.

FWIW, rereading some chat logs it looks like Cash App LDK node pods automatically restart every ~24 hours, other users may have a similar setup. Not sure if 144 blocks might be a bit on the conservative side.

How bad is it for Cash App? If they never fill up 144 blocks' worth of estimation data, their FC behavior will simply remain the same as it is today, which should be fine for this PR?

Right, it simply won't change. Really people should be using anchor channels, anything we do for non-anchor channels is super best-effort...

valentinewallace · 2024-05-30T17:35:06Z

Basically LGTM pending 2nd reviewer. Needs rebase as well.

TheBlueMatt · 2024-05-30T21:01:29Z

Rebased and added a test.

vincenzopalazzo

LGTM and thanks to try address this issue before core v28

CI looks like it is failing, but It looks like unrelated?

thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(1)`', src/full_stack.rs:1320:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    full_stack::tests::test_no_existing_test_breakage

test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

Maybe I am wrong, the fuzz testing of ldk is still a black-box to me

TheBlueMatt · 2024-05-31T17:45:13Z

Oops, the fuzz failure was not unrelated. The full_stack_target fuzzer has a sanity check to ensure we get notified if the fuzz format changes implying our existing corpus is less useful, which it did here.

fuzz/src/full_stack.rs

arik-so

looks good, feel free to squash the fuzz fixup commit

In the next commit we'll add a second field to `ChannelError::Close` so here we prep by converting existing calls to the constructor function, which is almost a full-file sed.

This will allow us to add more granular failure reasons when returning the general `ChannelError::Close`.

Closure due to feerate disagreements are a specific closure reason which admins can understand and tune their config (in the form of their `FeeEstimator`) to avoid, so having a separate `ClosureReason` for it is useful.

When we connect 100 blocks in a row, requiring the fuzz input to contain 100 fee estimator results is uneccessary, so add a bool that lets us skip those reads.

TheBlueMatt · 2024-06-10T15:13:09Z

Rebased to address conflicts with upstream and squashed the fixup.

For quite some time, LDK has force-closed channels if the peer sends us a feerate update which is below our `FeeEstimator`'s concept of a channel lower-bound. This is intended to ensure that channel feerates are always sufficient to get our commitment transaction confirmed on-chain if we do need to force-close. However, we've never checked our channel feerate regularly - if a peer is offline (or just uninterested in updating the channel feerate) and the prevailing feerates on-chain go up, we'll simply ignore it and allow our commitment transaction to sit around with a feerate too low to get confirmed. Here we rectify this oversight by force-closing channels with stale feerates, checking after each block. However, because fee estimators are often buggy and force-closures piss off users, we only do so rather conservatively. Specifically, we only force-close if a channel's feerate is below the minimum `FeeEstimator`-provided minimum across the last day. Further, because fee estimators are often especially buggy on startup (and because peers haven't had a chance to update the channel feerates yet), we don't force-close channels until we have a full day of feerate lower-bound history. This should reduce the incidence of force-closures substantially, but it is expected this will still increase force-closures somewhat substantially depending on the users' `FeeEstimator`. Fixes lightningdevkit#993

TheBlueMatt · 2024-06-11T14:19:54Z

Merging, the diff since @arik-so's ACK is mostly just a squash:

$ git range-diff -U1 7d2d04798daa9c78f183424f73f3fea6c8564573...9cc99b27e0b6471629d35df79518b7d282041e17 1d0c6c60c6802126b3b29d6a2aa026c1aa33db02...17b77e0bcf0fd6721ac820df07a92ff697b3f50f
1:  eecf12b38 = 1:  73bc0f61b Add `ChannelError::close` constructor
2:  6aa0d3004 ! 2:  3e09d9937 Use `ChannelError::close` constructor when building a close variant
    @@ lightning/src/ln/channel.rs: impl<SP: Deref> Channel<SP> where
                for outp in closing_tx.trust().built_transaction().output.iter() {
    -                   if !outp.script_pubkey.is_witness_program() && outp.value < MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS {
    +                   if !outp.script_pubkey.is_witness_program() && outp.value < Amount::from_sat(MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS) {
     -                          return Err(ChannelError::Close("Remote sent us a closing_signed with a dust output. Always use segwit closing scripts!".to_owned()));
3:  85fd47023 = 3:  93011c377 Allow `ChannelError` to specify the `ClosureReason`
4:  9f4d907b9 = 4:  88c291a9b Add a new `ClosureReason::PeerFeerateTooLow`
5:  3c7bfd7f8 ! 5:  66e6ee563 Skip fee reads in `full_stack_target` when connecting many blocks
    @@ fuzz/src/full_stack.rs: use std::cell::RefCell;
     +use std::sync::atomic::{AtomicU64,AtomicUsize,AtomicBool,Ordering};
    - use bitcoin::bech32::u5;
    + use bech32::u5;

6:  898f6716a ! 6:  5a1cc288b Force-close channels if their feerate gets stale without any update
    @@ Commit message

    + ## fuzz/src/full_stack.rs ##
    +@@ fuzz/src/full_stack.rs: mod tests {
    +           ext_from_hex("0c005e", &mut test);
    +           // the funding transaction
    +           ext_from_hex("020000000100000000000000000000000000000000000000000000000000000000000000000000000000ffffffff0150c3000000000000220020ae0000000000000000000000000000000000000000000000000000000000000000000000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // connect a block with no transactions, one per line
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // by now client should have sent a channel_ready (CHECK 3: SendChannelReady to 03000000 for chan 3d000000)
    +
    +           // inbound read from peer id 0 of len 18
    +@@ fuzz/src/full_stack.rs: mod tests {
    +           ext_from_hex("0c007d", &mut test);
    +           // the commitment transaction for channel 3f00000000000000000000000000000000000000000000000000000000000000
    +           ext_from_hex("02000000013a000000000000000000000000000000000000000000000000000000000000000000000000000000800258020000000000002200204b0000000000000000000000000000000000000000000000000000000000000014c0000000000000160014280000000000000000000000000000000000000005000020", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           //
    +           // connect a block with one transaction of len 94
    +           ext_from_hex("0c005e", &mut test);
    +           // the HTLC timeout transaction
    +           ext_from_hex("0200000001730000000000000000000000000000000000000000000000000000000000000000000000000000000001a701000000000000220020b20000000000000000000000000000000000000000000000000000000000000000000000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // connect a block with no transactions
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // connect a block with no transactions
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // connect a block with no transactions
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // connect a block with no transactions
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +           // connect a block with no transactions
    +           ext_from_hex("0c0000", &mut test);
    ++          ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
    +
    +           // process the now-pending HTLC forward
    +           ext_from_hex("07", &mut test);
    +
      ## lightning/src/ln/channel.rs ##
7:  518261f93 < -:  --------- f
8:  9cc99b27e = 7:  17b77e0bc Add a test of stale-feerate-force-closure behavior
$ git show 518261f93
commit 518261f9382417923d1b5f3ad10b6c38ba2baa70
Author: Matt Corallo <[email protected]>
Date:   Fri May 31 17:44:01 2024 +0000

    f

diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index f9db2540e..7ab980f34 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -911,23 +911,38 @@ mod tests {
                ext_from_hex("0022 ff4f00f805273c1b203bb5ebf8436bfde57b3be8c2f5e95d9491dbb181909679 3d00000000000000000000000000000000000000000000000000000000000000 0000 00000000000000000000000000000000000000000000000000000000000000210100000000000000000000000000000000000000000000000000000000000000 03000000000000000000000000000000", &mut test);
                // client should now respond with funding_signed (CHECK 2: type 35 to peer 03000000)

+
+
                // connect a block with one transaction of len 94
                ext_from_hex("0c005e", &mut test);
                // the funding transaction
                ext_from_hex("020000000100000000000000000000000000000000000000000000000000000000000000000000000000ffffffff0150c3000000000000220020ae0000000000000000000000000000000000000000000000000000000000000000000000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // connect a block with no transactions, one per line
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // by now client should have sent a channel_ready (CHECK 3: SendChannelReady to 03000000 for chan 3d000000)

                // inbound read from peer id 0 of len 18
@@ -1297,21 +1312,28 @@ mod tests {
                ext_from_hex("0c007d", &mut test);
                // the commitment transaction for channel 3f00000000000000000000000000000000000000000000000000000000000000
                ext_from_hex("02000000013a000000000000000000000000000000000000000000000000000000000000000000000000000000800258020000000000002200204b0000000000000000000000000000000000000000000000000000000000000014c0000000000000160014280000000000000000000000000000000000000005000020", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                //
                // connect a block with one transaction of len 94
                ext_from_hex("0c005e", &mut test);
                // the HTLC timeout transaction
                ext_from_hex("0200000001730000000000000000000000000000000000000000000000000000000000000000000000000000000001a701000000000000220020b20000000000000000000000000000000000000000000000000000000000000000000000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // connect a block with no transactions
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // connect a block with no transactions
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // connect a block with no transactions
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // connect a block with no transactions
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection
                // connect a block with no transactions
                ext_from_hex("0c0000", &mut test);
+               ext_from_hex("00fd00fd", &mut test); // Two feerate requests during block connection

                // process the now-pending HTLC forward
                ext_from_hex("07", &mut test);

TheBlueMatt added this to the 0.1 milestone May 6, 2024

benthecarman reviewed May 6, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-05-close-on-stale-fees branch from 986bc4a to 6a9dd2b Compare May 9, 2024 15:14

TheBlueMatt added the Seeking Code Review label May 30, 2024

valentinewallace reviewed May 30, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-05-close-on-stale-fees branch from 6a9dd2b to 8b320be Compare May 30, 2024 21:00

vincenzopalazzo approved these changes May 31, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-05-close-on-stale-fees branch from 8b320be to 9cc99b2 Compare May 31, 2024 17:44

arik-so reviewed Jun 8, 2024

View reviewed changes

fuzz/src/full_stack.rs Outdated Show resolved Hide resolved

arik-so approved these changes Jun 8, 2024

View reviewed changes

arik-so previously approved these changes Jun 8, 2024

View reviewed changes

TheBlueMatt added 5 commits June 10, 2024 15:12

Add ChannelError::close constructor

73bc0f6

Use ChannelError::close constructor when building a close variant

3e09d99

In the next commit we'll add a second field to `ChannelError::Close` so here we prep by converting existing calls to the constructor function, which is almost a full-file sed.

Allow ChannelError to specify the ClosureReason

93011c3

This will allow us to add more granular failure reasons when returning the general `ChannelError::Close`.

Add a new ClosureReason::PeerFeerateTooLow

88c291a

Closure due to feerate disagreements are a specific closure reason which admins can understand and tune their config (in the form of their `FeeEstimator`) to avoid, so having a separate `ClosureReason` for it is useful.

Skip fee reads in full_stack_target when connecting many blocks

66e6ee5

When we connect 100 blocks in a row, requiring the fuzz input to contain 100 fee estimator results is uneccessary, so add a bool that lets us skip those reads.

TheBlueMatt dismissed arik-so’s stale review via 1c3ce0c June 10, 2024 15:13

TheBlueMatt force-pushed the 2024-05-close-on-stale-fees branch from 9cc99b2 to 1c3ce0c Compare June 10, 2024 15:13

TheBlueMatt added 2 commits June 10, 2024 15:17

Add a test of stale-feerate-force-closure behavior

17b77e0

TheBlueMatt force-pushed the 2024-05-close-on-stale-fees branch from 1c3ce0c to 17b77e0 Compare June 10, 2024 15:18

valentinewallace approved these changes Jun 10, 2024

View reviewed changes

TheBlueMatt merged commit a666401 into lightningdevkit:main Jun 11, 2024
14 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force-close channels if their feerate gets stale without any update #3037

Force-close channels if their feerate gets stale without any update #3037

TheBlueMatt commented May 6, 2024

codecov-commenter commented May 6, 2024 •

edited

Loading

benthecarman left a comment

TheBlueMatt commented May 6, 2024

valentinewallace commented May 30, 2024

benthecarman commented May 30, 2024

valentinewallace May 30, 2024

TheBlueMatt May 30, 2024

valentinewallace May 31, 2024

arik-so Jun 8, 2024

TheBlueMatt Jun 10, 2024

valentinewallace commented May 30, 2024

TheBlueMatt commented May 30, 2024

vincenzopalazzo left a comment

TheBlueMatt commented May 31, 2024

arik-so left a comment

TheBlueMatt commented Jun 10, 2024

TheBlueMatt commented Jun 11, 2024

		/// We only keep this in memory as we assume any feerates we receive immediately after startup
		/// may be bunk (as they often are if Bitcoin Core crashes) and want to delay taking any

Force-close channels if their feerate gets stale without any update #3037

Force-close channels if their feerate gets stale without any update #3037

Conversation

TheBlueMatt commented May 6, 2024

codecov-commenter commented May 6, 2024 • edited Loading

Codecov Report

benthecarman left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 6, 2024

valentinewallace commented May 30, 2024

benthecarman commented May 30, 2024

valentinewallace May 30, 2024

Choose a reason for hiding this comment

TheBlueMatt May 30, 2024

Choose a reason for hiding this comment

valentinewallace May 31, 2024

Choose a reason for hiding this comment

arik-so Jun 8, 2024

Choose a reason for hiding this comment

TheBlueMatt Jun 10, 2024

Choose a reason for hiding this comment

valentinewallace commented May 30, 2024

TheBlueMatt commented May 30, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 31, 2024

arik-so left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 10, 2024

TheBlueMatt commented Jun 11, 2024

codecov-commenter commented May 6, 2024 •

edited

Loading