Add AvailableBalances::next_splice_out_maximum_sat#4550
Add AvailableBalances::next_splice_out_maximum_sat#4550tankyleo wants to merge 5 commits intolightningdevkit:mainfrom
AvailableBalances::next_splice_out_maximum_sat#4550Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
724acf9 to
3bd7a76
Compare
|
After a thorough review of every file and hunk in the diff, including examination of the surrounding codebase for context, I have no new issues to flag beyond what was already covered in my prior review comments. Review SummaryNo new issues found beyond what was previously flagged. Corrections to Prior CommentsMy prior comments about unit mismatches were incorrect — I'm noting this so reviewers don't waste time on them:
Previously Flagged Issues (Still Valid)
Notes on Positive Changes
|
tankyleo
requested review from
TheBlueMatt and
wpaulino
and removed request for
jkczyz
|
Seems the fuzz failure produces this error on my machine Hex: Target: chanmon_consistency Error: |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
Curious about |
Thanks for taking a look ! 183sats is the amount allocated to the fee of the splice out transaction at the default feerate of 253sat/kw, and will be added on top of the splice amount. Will clarify this in the test. |
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| their_contribution_candidate: SignedAmount, addl_nondust_htlc_count: usize, | ||
| feerate_per_kw: u32, | ||
| ) -> Result<(Amount, Amount), String> { | ||
| let htlc_candidate = None; |
There was a problem hiding this comment.
This really feels like it could/should be substantially DRYd with get_next_local/remote_commitment_stats - what we're really asking at the callsites is "is this splice even valid" which istm should really be done by just saying "did get_next_local_commitment_stats return an Error or not" (with appropriate balance modification flags, maybe a wrapper method calling an internal impl method rather than modifying the existing methods).
| /// retains at least one output after the splice, which is particularly relevant for | ||
| /// zero-reserve channels. | ||
| // | ||
| // The equation to determine `percent_max_splice_out_sat` is: |
There was a problem hiding this comment.
discussed offline but this feels super unclear to me because "percent_" as a prefix implies to me, that the value is a percent, but its not.
|
Needs rebase, sorry about that. Do think its worth doing this for 0.3, yes, given it fixed splice <-> 0 reserve compat. Its also not a big deal to add a field rn. |
Makes sense, thanks for clarifying! |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
3bd7a76 to
245edbe
Compare
| } | ||
| max_splice_out_sat | ||
| } else { | ||
| // In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat` |
There was a problem hiding this comment.
Nit: This comment is backwards — it reads as if post_splice_delta_above_reserve_sat is the maximum the holder can withdraw, but the code computes local_balance - delta, meaning the holder can withdraw everything except the delta. Consider:
| // In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat` | |
| // In a zero-reserve channel, the holder is free to withdraw up to its balance minus `post_splice_delta_above_reserve_sat` |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
245edbe to
bb91e11
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4550 +/- ##
==========================================
+ Coverage 86.99% 87.02% +0.02%
==========================================
Files 163 161 -2
Lines 109008 109019 +11
Branches 109008 109019 +11
==========================================
+ Hits 94828 94870 +42
+ Misses 11696 11662 -34
- Partials 2484 2487 +3
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:
|
As a result, we now validate that both commitments retain at least one output under the new funding scope, which is crucial for zero-reserve channels.
We previously determined this value by subtracting the htlcs, the anchors, and the commitment transaction fee. This ignored the reserve, as well as the at-least-one-output requirement in zero-reserve channels. This new field now accounts for both of these constraints.
This is equivalent to the previous commit, see the debug assertions added in the previous commit. We now also get to communicate the exact maximum back to the user, instead of some "balance is lower than our reserve" message, which is hard to react to.
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
bb91e11 to
ccde0da
Compare
5 participants
This is the maximum value the holder can splice out of their channel balance, accounting for the updated v2 reserves after the splice, and the "must have at least one output" constraint on zero-reserve channels.