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.
|
Good — confirmed that I've now reviewed every file and every hunk in the diff. The prior review comments already covered all the nits I found. No bugs, security issues, or logic errors beyond those nits. Review SummaryNo bugs or security issues found. The three nits from my prior review remain the only findings:
All enforcement points (outbound creation, inbound acceptance, accept-channel validation, channel-type downgrade) are correctly implemented. The relaxation of No new issues to report. |
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.
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
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.
4 participants
We still allow counterparties to set 0-reserve legacy channels, as in prior releases.