Accept feerate increases even if they aren't high enough for us

[TheBlueMatt]

LND nodes have very broken fee estimators, causing them to suggest feerates that don't even meet a current mempool minimum feerate when fees go up over the course of hours. This can cause us to reject their feerate estimates as they're not high enough, even though their new feerate is higher than what we had already (which is the feerate we'll use to broadcast a closing transaction). This implies we force-close the channel and broadcast something with a feerate lower than our counterparty was offering.

Here we simply accept such feerates as they are better than what we had. We really should also close the channel, but only after we get their signature on the new feerate. That should happen by checking channel feerates every time we see a new block so is orthogonal to this code.

Ultimately the fix is anchor outputs plus package-based relay in Bitcoin Core, however we're still quite some ways from that, so worth needlessly closing channels for now.

[codecov-commenter]

Codecov Report

Base: 90.71% // Head: 90.63% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (a1404aa) compared to base (838d486).
Patch coverage: 93.54% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1852      +/-   ##
==========================================
- Coverage   90.71%   90.63%   -0.09%     
==========================================
  Files          89       89              
  Lines       47923    49507    +1584     
  Branches    47923    49507    +1584     
==========================================
+ Hits        43473    44870    +1397     
- Misses       4450     4637     +187     

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	89.77% <91.66%> (+1.04%)	⬆️
lightning/src/ln/functional_tests.rs	96.94% <93.87%> (-0.10%)	⬇️
lightning/src/ln/channelmanager.rs	86.55% <100.00%> (+1.13%)	⬆️
lightning/src/util/ser.rs	91.56% <0.00%> (-0.24%)	⬇️
lightning/src/util/events.rs	25.89% <0.00%> (+1.14%)	⬆️
lightning/src/util/ser_macros.rs	90.00% <0.00%> (+1.59%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ariard]

Here we simply accept such feerates as they are better than what we had. We really should also close the channel, but only after we get their signature on the new feerate. That should happen by checking channel feerates every time we see a new block so is orthogonal to this code

I don't know about introducing automatic channel force-close in case of pre-signed feerate under the current mempool min fee, ideally the best time to force-close, if we don't expect a cooperative peer, would be few blocks even before it does happen. I think we had this discussion back-and-forth with #993 and I believe we conclude is the lack of current predictive fee-estimator, at least yielding risks of mempool min fee overflowing our pre-signed feerates. Of course, the situation should be improved by package relay, however I think such smarter fee API will still be useful as our fee-bumping reserves stay bounded.

One more action we could take if we detect that type of situation where the current mempool fee levels are too high would be to stop accepting offered/received HTLC on the channels at risks. I think this is more interesting to revisit this once we support anchor output.

Otherwise Concept ACK eb83763

[dunxen]

Concept ACK. Just a nit and comment, otherwise LGTM.

[wpaulino]

Test is failing with:

thread 'ln::functional_tests::accept_busted_but_better_fee' panicked at 'Had excess message events on node node 1: [BroadcastChannelUpdate { msg: ChannelUpdate { signature: 304402201f75f91778d9d1d20a88b87a0dc69a9ce02d9cfaee454a6daa794336ca37aa80022013ab43970c2cb3af6412539d39e4a62ca174c8b18eb5df151d52057b4e41c93c, contents: UnsignedChannelUpdate { chain_hash: 000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943, short_channel_id: 1099511627776, timestamp: 14, flags: 3, cltv_expiry_delta: 42, htlc_minimum_msat: 1000, htlc_maximum_msat: 10000000, fee_base_msat: 1000, fee_proportional_millionths: 0, excess_data: [] } } }, HandleError { node_id: PublicKey(88ce8f35acfc83a200d0b3bf998a4bd18ffdecad1061e3707c0cacf28515927f4e1b189ad34db624e1e906128da9b9428d73532b728d4d44dfd97ab21ac33459), action: SendErrorMessage { msg: ErrorMessage { channel_id: [54, 1, 27, 147, 208, 68, 174, 120, 153, 166, 147, 131, 196, 197, 137, 125, 20, 169, 146, 130, 47, 96, 90, 106, 226, 118, 1, 231, 93, 113, 103, 164], data: "Peer's feerate much too low. Actual: 1000. Our expected lower limit: 5000 (- 250)" } } }]', lightning/src/ln/functional_test_utils.rs:309:17

[wpaulino]

LGTM after squash.

[TheBlueMatt]

I don't know about introducing automatic channel force-close in case of pre-signed feerate under the current mempool min fee, ideally the best time to force-close, if we don't expect a cooperative peer, would be few blocks even before it does happen. I think we had this discussion back-and-forth with #993 and I believe we conclude is the lack of current predictive fee-estimator, at least yielding risks of mempool min fee overflowing our pre-signed feerates. Of course, the situation should be improved by package relay, however I think such smarter fee API will still be useful as our fee-bumping reserves stay bounded.

Yea, fair, its....complicated, to say the least.

[TheBlueMatt]

Squashed and included a quick fix for 1.41:

$ git diff-tree -U1 aa44f4a69 f7c2ff76a
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 8d976a2e4..4c735462c 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -10498,3 +10498,3 @@ fn accept_busted_but_better_fee() {
 	// later, but for now we only accept the update.
-	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let mut chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);

[TheBlueMatt]

Ugh, okay, attempt number 2:

$ git diff-tree -U1 f7c2ff76a a1404aac6
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 4c735462c..f9b9e75bf 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -10506,6 +10506,12 @@ fn accept_busted_but_better_fee() {
 	// Set nodes[1] to expect 5,000 sat/kW.
-	*chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap() = 5000;
+	{
+		let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 5000;
+	}
 
 	// If nodes[0] increases their feerate, even if its not enough, nodes[1] should accept it.
-	*chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap() = 1000;
+	{
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 1000;
+	}
 	nodes[0].node.timer_tick_occurred();
@@ -10525,3 +10531,6 @@ fn accept_busted_but_better_fee() {
 	// it.
-	*chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap() = 2000;
+	{
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 2000;
+	}
 	nodes[0].node.timer_tick_occurred();
@@ -10541,3 +10550,6 @@ fn accept_busted_but_better_fee() {
 	// channel.
-	*chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap() = 1000;
+	{
+		let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+		*feerate_lock = 1000;
+	}
 	nodes[0].node.timer_tick_occurred();

[ariard]

Tested ACK a1404aa

Modifying the check (both equality and superior) breaks the new test accept_busted_but_better_fee as expected.