Disallow holders from selecting 0-reserve in legacy channels#4613
Disallow holders from selecting 0-reserve in legacy channels#4613tankyleo wants to merge 5 commits into
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
tankyleo
force-pushed
the
2026-05-ban-0reserve-legacy-chans
branch
4 times, most recently
from
e90865f to
603754d
Compare
tankyleo
changed the title
| use crate::sign::ChannelSigner; | ||
| use crate::types::features::ChannelTypeFeatures; | ||
| use crate::types::payment::{PaymentHash, PaymentPreimage}; | ||
| use crate::types::payment::PaymentPreimage; |
There was a problem hiding this comment.
Nit: PaymentHash is imported from bolt11_invoice which re-exports it from lightning_types::payment. Every other file in this crate imports it from crate::types::payment::PaymentHash. Consider keeping it consistent:
| use crate::types::payment::PaymentPreimage; | |
| use crate::types::payment::{PaymentHash, PaymentPreimage}; |
(and removing the bolt11_invoice::PaymentHash import on line 30)
| connect_peers(&nodes[0], &nodes[1]); | ||
| connect_peers(&nodes[1], &nodes[2]); | ||
|
|
||
| let set_0reserve = if chan_type == ChanType::Legacy { false } else { true }; |
There was a problem hiding this comment.
Nit: This can be simplified to chan_type != ChanType::Legacy (which also explains why PartialEq was derived above).
| // A-B: channel 2 A and B have 0-reserve (trusted open + trusted accept), | ||
| // channel 3 A has 0-reserve (trusted accept). |
There was a problem hiding this comment.
The comments on lines 2090-2091 and 2095-2096 still describe channel 2/3/4/5 as having 0-reserve unconditionally, but that's no longer the case for ChanType::Legacy. Consider qualifying these with "if non-legacy" or restructuring the comments to be accurate for both code paths.
|
I've re-reviewed the entire PR diff against the current codebase, verified my prior findings, and confirmed my analysis of the key changes. My prior review covered all the issues. No new issues found. The three nits from my prior review remain the only findings:
All enforcement points are correctly implemented. The relaxation of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4613 +/- ##
==========================================
- Coverage 86.40% 86.40% -0.01%
==========================================
Files 158 158
Lines 109293 109294 +1
Branches 109293 109294 +1
==========================================
- Hits 94439 94432 -7
- Misses 12309 12313 +4
- Partials 2545 2549 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM, but please reorder the commits so that the "remove tests of legacy + 0-reserve" change happens before we remove the legacy reserve change, that way at each commit tests pass.
tankyleo
force-pushed
the
2026-05-ban-0reserve-legacy-chans
branch
from
603754d to
96a9e7b
Compare
|
Thank you done, no other changes |
| let spiked_feerate = feerate_per_kw.saturating_mul( | ||
| if is_outbound_from_holder && !channel_type.supports_anchors_zero_fee_htlc_tx() { | ||
| let spiked_feerate = | ||
| feerate_per_kw.saturating_mul(if !channel_type.supports_anchors_zero_fee_htlc_tx() { |
There was a problem hiding this comment.
Why are we dropping is_outbound_from_holder?
There was a problem hiding this comment.
Would rather have the variable spiked_feerate be solely based on whether we are in a legacy channel or not.
No functional change, all the dependents on this variable are only used if is_outbound_from_holder is true.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
wpaulino
left a comment
wpaulino
left a comment
There was a problem hiding this comment.
Would be nice to fix the review bot nits
We still allow counterparties to set 0-reserve legacy channels, as in prior releases.
We only assume fee spikes in legacy channels, and we do not allow `holder_selected_channel_reserve_satoshis` to be set to zero in such channels. It is nonetheless still possible to reach the no-outputs case in a fee spike with solely the counterparty selected reserve set to zero, so we still guard against this case in `get_next_commitment_stats`. We don't guard against no-outputs under fee spikes `get_available_balances`; in the worst case, the receiver of the HTLC we just sent fails it back.
tankyleo
force-pushed
the
2026-05-ban-0reserve-legacy-chans
branch
from
96a9e7b to
257033f
Compare
Thanks diff from last review: diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 1f9e42ce8..4ff0e4a4a 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -2082,18 +2082,18 @@
connect_peers(&nodes[0], &nodes[1]);
connect_peers(&nodes[1], &nodes[2]);
- let set_0reserve = if chan_type == ChanType::Legacy { false } else { true };
+ let set_0reserve = chan_type != ChanType::Legacy;
// Create 3 channels between A-B and 3 channels between B-C (6 total).
//
// Use distinct version numbers for each funding transaction so each test
// channel gets its own txid and funding outpoint.
// A-B: channel 2 A and B have 0-reserve (trusted open + trusted accept),
- // channel 3 A has 0-reserve (trusted accept).
+ // channel 3 A has 0-reserve (trusted accept), if channels are non-legacy.
make_channel(&nodes[0], &nodes[1], 1, false, false, &mut chain_state);
make_channel(&nodes[0], &nodes[1], 2, set_0reserve, set_0reserve, &mut chain_state);
make_channel(&nodes[0], &nodes[1], 3, false, set_0reserve, &mut chain_state);
// B-C: channel 4 B has 0-reserve (via trusted accept),
- // channel 5 C has 0-reserve (via trusted open).
+ // channel 5 C has 0-reserve (via trusted open), if channels are non-legacy.
make_channel(&nodes[1], &nodes[2], 4, false, set_0reserve, &mut chain_state);
make_channel(&nodes[1], &nodes[2], 5, set_0reserve, false, &mut chain_state);
make_channel(&nodes[1], &nodes[2], 6, false, false, &mut chain_state);
diff --git a/lightning/src/ln/htlc_reserve_unit_tests.rs b/lightning/src/ln/htlc_reserve_unit_tests.rs
index 17b0ffee4..86d98b788 100644
--- a/lightning/src/ln/htlc_reserve_unit_tests.rs
+++ b/lightning/src/ln/htlc_reserve_unit_tests.rs
@@ -23,11 +23,10 @@
use crate::sign::tx_builder::{SpecTxBuilder, TxBuilder};
use crate::sign::ChannelSigner;
use crate::types::features::ChannelTypeFeatures;
-use crate::types::payment::PaymentPreimage;
+use crate::types::payment::{PaymentHash, PaymentPreimage};
use crate::util::config::UserConfig;
use crate::util::errors::APIError;
-use bolt11_invoice::PaymentHash;
use lightning_macros::xtest;
use bitcoin::secp256k1::{Secp256k1, SecretKey}; |
5 participants
We still allow counterparties to set 0-reserve legacy channels, as in prior releases.