Add AvailableBalances::next_splice_out_maximum_sat#4550
Add AvailableBalances::next_splice_out_maximum_sat#4550tankyleo wants to merge 6 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
|
I've completed a thorough re-review of the entire diff, cross-referencing against the current code and my prior comments. All unit-conversion concerns from the prior review were confirmed incorrect — the code is correct in those areas. The remaining prior concerns about dust limit inconsistency, I found no new issues in this review pass. Review SummaryNo new issues found beyond what was flagged in the prior review. Previously Flagged Issues (Still Valid)
Previously Flagged Issues (Confirmed Incorrect — Not Issues)
Cross-Cutting NoteThe semantic change from "holder balance floor" to "splice-out maximum" is incomplete: several variable names remain |
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
ccde0da to
fb777b8
Compare
| // TODO: Skip 0FC channels for now as these always have an output on the commitment, the P2A | ||
| // output. We will be able to withdraw up to the dust limit of the funding script, which | ||
| // is checked in interactivetx. Still need to double check whether that's what we actually | ||
| // want. |
There was a problem hiding this comment.
@TheBlueMatt forgot to mention this TODO here, WDYT ?
There was a problem hiding this comment.
Hmm, it seems like mostly-not-an-issue, no? Like, with 0FC in order to be worried about dust burns we have to actually have dust. Yea, we can't really withdraw much (or its in the P2A at which point it might as well be a burn) but its basically only when we have almost no balance anyway, so it seems like its not really worth worrying too much about?
There was a problem hiding this comment.
do we want to allow people to withdraw to the point where the overall channel value is around 300-400 sats ? the public API for create_channel does not allow users to open channels less than 1000 satoshis
There was a problem hiding this comment.
Yea, maybe not. We can just hard-code the lower-bound for both in the same constant?
|
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8931b05bc..5b165a28b 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -12401,8 +12401,8 @@
"build_prior_contribution requires pending_splice"
);
let prior = self.pending_splice.as_ref()?.contributions.last()?;
- let holder_balance = self.get_next_splice_out_maximum(&self.funding).ok();
- Some(PriorContribution::new(prior.clone(), holder_balance))
+ let next_splice_out_maximum = self.get_next_splice_out_maximum(&self.funding).ok();
+ Some(PriorContribution::new(prior.clone(), next_splice_out_maximum))
}
/// Returns whether this channel can ever RBF, independent of splice state.
@@ -12476,13 +12476,13 @@
return contribution;
}
- let holder_balance = match self.get_next_splice_out_maximum(&self.funding) {
+ let next_splice_out_maximum = match self.get_next_splice_out_maximum(&self.funding) {
Ok(balance) => balance,
Err(_) => return contribution,
};
- if let Err(e) =
- contribution.net_value_for_initiator_at_feerate(min_rbf_feerate, holder_balance)
+ if let Err(e) = contribution
+ .net_value_for_initiator_at_feerate(min_rbf_feerate, next_splice_out_maximum)
{
log_info!(
logger,
@@ -12503,7 +12503,7 @@
min_rbf_feerate,
);
contribution
- .for_initiator_at_feerate(min_rbf_feerate, holder_balance)
+ .for_initiator_at_feerate(min_rbf_feerate, next_splice_out_maximum)
.expect("feerate compatibility already checked")
}
@@ -12876,7 +12876,7 @@
fn resolve_queued_contribution<L: Logger>(
&self, feerate: FeeRate, logger: &L,
) -> Result<(Option<SignedAmount>, Option<Amount>), ChannelError> {
- let holder_balance = self
+ let next_splice_out_maximum = self
.get_next_splice_out_maximum(&self.funding)
.map_err(|e| {
log_info!(
@@ -12889,9 +12889,12 @@
})
.ok();
- let net_value = match holder_balance.and_then(|_| self.queued_funding_contribution()) {
+ let net_value = match next_splice_out_maximum
+ .and_then(|_| self.queued_funding_contribution())
+ {
Some(c) => {
- match c.net_value_for_acceptor_at_feerate(feerate, holder_balance.unwrap()) {
+ match c.net_value_for_acceptor_at_feerate(feerate, next_splice_out_maximum.unwrap())
+ {
Ok(net_value) => Some(net_value),
Err(FeeRateAdjustmentError::FeeRateTooHigh { .. }) => {
return Err(ChannelError::Abort(AbortReason::FeeRateTooHigh));
@@ -12911,7 +12914,7 @@
None => None,
};
- Ok((net_value, holder_balance))
+ Ok((net_value, next_splice_out_maximum))
}
pub(crate) fn splice_init<ES: EntropySource, L: Logger>(
diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs
index d78f65d22..ade54c603 100644
--- a/lightning/src/sign/tx_builder.rs
+++ b/lightning/src/sign/tx_builder.rs
@@ -324,7 +324,7 @@
//
// The equation to determine `max_splice_percentage_constraint_sat` is:
// 1) floor((c - s) / 100) == h - s - d
-// We want the maximum value of s that will satisfy this equation, therefore, we solve:
+// We want the maximum value of s that will satisfy equation 1, therefore, we solve:
// 2) (c - s) / 100 < h - s - d + 1
// where c: `channel_value_satoshis`
// s: `max_splice_percentage_constraint_sat`
@@ -343,16 +343,17 @@
.counterparty_selected_channel_reserve_satoshis
!= 0
{
- let dividend = local_balance_before_fee_sat
+ let dividend_sat = local_balance_before_fee_sat
.saturating_mul(100)
.saturating_add(100)
.saturating_sub(post_splice_delta_above_reserve_sat.saturating_mul(100))
.saturating_sub(channel_value_satoshis);
- let max_splice_percentage_constraint_sat = dividend.saturating_sub(1) / 99;
+ // Calculate the greatest integer that is strictly less than the RHS of inequality 3 above
+ let max_splice_percentage_constraint_sat = dividend_sat.saturating_sub(1) / 99;
let max_splice_dust_limit_constraint_sat = local_balance_before_fee_sat
.saturating_sub(channel_constraints.holder_dust_limit_satoshis)
.saturating_sub(post_splice_delta_above_reserve_sat);
- // Take whichever constraint you hit first as you increase the value of the splice-out
+ // Both constraints must be satisfied, so take the minimum of the two maximums
let max_splice_out_sat =
cmp::min(max_splice_percentage_constraint_sat, max_splice_dust_limit_constraint_sat);
#[cfg(debug_assertions)] |
|
Needs a rebase |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
fb777b8 to
d3b1c31
Compare
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
d3b1c31 to
5b25a09
Compare
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
5b25a09 to
794b9ff
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM but needs another rebase :/
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. It can be seen as the total spliceable balance from the channel.
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.
We did not enforce this minimum when accepting 0-reserve channels. This is because we depended on the `MIN_THEIR_CHAN_RESERVE_SATOSHIS` constant to guarantee this minimum channel value, but this value is no longer read in 0-reserve channels. Note that the user's `min_funding_satoshis` value would still be respected in this case. When splicing 0-reserve channels, we only enforced that the commitment transaction retained at least one output after the splice, which could produce a channel value lower than 1000sats. Along the way, we also now enforce this 1000sat minimum when splicing reserve-enabled channels. We previously correctly enforced the reserves after the splice, but this could still result in a channel value smaller than 1000sats. This case is now rejected during splice validation. Note that the user's `min_funding_satoshis` is not respected when validating splice contributions, we leave this for follow-up work.
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
794b9ff to
89ee5a1
Compare
|
Rebased, and appended one commit to enforce a min channel value satoshis everywhere @TheBlueMatt (create channel, accept channel, and splice channel). |
6 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.