← Watch feed

Low 29 Bitcoin Committed Apr 15, 2026 · 02:51 UTC

Make PriorContribution::holder_balance non-optional

LDK

Lightning Dev Kit/rust-lightning

41a2baac View source ↗

Public commit record

What the developer wrote

Authored by Wilmer Paulino

85/100 · Strong

Make PriorContribution::holder_balance non-optional

The `holder_balance` is computed by
`FundedChannel::get_holder_counterparty_balances_floor_incl_fee`, which
may unexpectedly fail due to the balance either being too high or too
low. These cases are highly unlikely to happen given we have validation
to ensure we never enter such a state to begin with. If they were to
happen, something has gone wrong with the channel and it doesn't make
sense to allow splicing anyway. Therefore, we opt to make
`PriorContribution::holder_balance` non-optional and return an error
that the channel cannot be spliced at the moment.

✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode

The short version

What changed, and why it matters

This commit tightens error handling during Bitcoin Lightning channel splicing. Previously, if the software could not compute the user's current channel balance, it would silently skip a fee-rate optimization and re-run coin selection. Now it treats that balance-computation failure as a hard error and refuses to splice. This prevents the code from proceeding with potentially incorrect fee calculations when the channel is in an unexpected state, but it is a defensive hardening change rather than a fix for a known exploitable bug.

Recommended action

Treat as a defensive hardening commit. Reviewers should verify that get_holder_counterparty_balances_floor_incl_fee can only fail in genuinely inconsistent channel states and that returning ChannelUnavailable does not introduce a denial-of-service vector for legitimate splice operations. No immediate security response is indicated absent additional context.

Security signals we found

Defensive hardening: converts silent fallback to explicit error

Removes Option wrapper around balance used in fee-rate adjustment

Propagates balance-computation failure as APIError::ChannelUnavailable

Prevents proceeding with splicing when channel state is inconsistent

No explicit CVE, advisory, or security disclosure referenced

Technical analysis

Evidence from the diff

The patch removes the Option wrapper around PriorContribution::holder_balance, making it a required Amount. The helper build_prior_contribution() is replaced with inline logic that calls get_holder_counterparty_balances_floor_incl_fee() and propagates any error as APIError::ChannelUnavailable instead of mapping it to None. Call sites in funding.rs no longer branch on Some/None holder_balance; they always use the concrete value. Tests are updated to supply Amount::ZERO or Amount::MAX where they previously passed None. The change is defensive: it converts a silent fallback path into an explicit failure when balance computation fails.

Changed components

lightning/src/ln/channel.rslightning/src/ln/funding.rsPriorContribution structFundingTemplate::rbf and FundingTemplate::rbf_syncChannel splicing / RBF fee adjustment path

Inspect captured patch +44 / −46

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 32c0e94..b99b2a1 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -12324,7 +12324,25 @@ where
 			);
 			let min_rbf_feerate = prev_feerate.map(min_rbf_feerate);
 			let prior = if pending_splice.last_funding_feerate_sat_per_1000_weight.is_some() {
-				self.build_prior_contribution()
+				if let Some(prior) = self
+					.pending_splice
+					.as_ref()
+					.and_then(|pending_splice| pending_splice.contributions.last())
+				{
+					let holder_balance = self
+						.get_holder_counterparty_balances_floor_incl_fee(&self.funding)
+						.map(|(h, _)| h)
+						.map_err(|e| APIError::ChannelUnavailable {
+							err: format!(
+								"Channel {} cannot be spliced at this time: {}",
+								self.context.channel_id(),
+								e
+							),
+						})?;
+					Some(PriorContribution::new(prior.clone(), holder_balance))
+				} else {
+					None
+				}
 			} else {
 				None
 			};
@@ -12346,21 +12364,6 @@ where
 		Ok(FundingTemplate::new(Some(shared_input), min_rbf_feerate, prior_contribution))
 	}
 
-	/// Clones the prior contribution and fetches the holder balance for deferred feerate
-	/// adjustment.
-	fn build_prior_contribution(&self) -> Option<PriorContribution> {
-		debug_assert!(
-			self.pending_splice.is_some(),
-			"build_prior_contribution requires pending_splice"
-		);
-		let prior = self.pending_splice.as_ref()?.contributions.last()?;
-		let holder_balance = self
-			.get_holder_counterparty_balances_floor_incl_fee(&self.funding)
-			.map(|(h, _)| h)
-			.ok();
-		Some(PriorContribution::new(prior.clone(), holder_balance))
-	}
-
 	/// Returns whether this channel can ever RBF, independent of splice state.
 	fn is_rbf_compatible(&self) -> Result<(), String> {
 		if self.context.minimum_depth(&self.funding) == Some(0) {
diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs
index 470e8bc..6341b10 100644
--- a/lightning/src/ln/funding.rs
+++ b/lightning/src/ln/funding.rs
@@ -174,8 +174,7 @@ impl core::fmt::Display for FundingContributionError {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub(super) struct PriorContribution {
 	contribution: FundingContribution,
-	/// The holder's balance, used for feerate adjustment. `None` when the balance computation
-	/// fails, in which case adjustment is skipped and coin selection is re-run.
+	/// The holder's balance, used for feerate adjustment.
 	///
 	/// This value is captured at [`ChannelManager::splice_channel`] time and may become stale
 	/// if balances change before the contribution is used. Staleness is acceptable here because
@@ -186,11 +185,11 @@ pub(super) struct PriorContribution {
 	///
 	/// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel
 	/// [`ChannelManager::funding_contributed`]: crate::ln::channelmanager::ChannelManager::funding_contributed
-	holder_balance: Option<Amount>,
+	holder_balance: Amount,
 }
 
 impl PriorContribution {
-	pub(super) fn new(contribution: FundingContribution, holder_balance: Option<Amount>) -> Self {
+	pub(super) fn new(contribution: FundingContribution, holder_balance: Amount) -> Self {
 		Self { contribution, holder_balance }
 	}
 }
@@ -562,17 +561,15 @@ impl FundingTemplate {
 				// buffer is insufficient (splice-in), or if the prior's feerate is already
 				// above rbf_feerate (e.g., from a counterparty-initiated RBF that locked
 				// at a higher feerate). In all cases, fall through to re-run coin selection.
-				if let Some(holder_balance) = holder_balance {
-					if contribution
-						.net_value_for_initiator_at_feerate(rbf_feerate, holder_balance)
-						.is_ok()
-					{
-						let mut adjusted = contribution
-							.for_initiator_at_feerate(rbf_feerate, holder_balance)
-							.expect("feerate compatibility already checked");
-						adjusted.max_feerate = max_feerate;
-						return Ok(adjusted);
-					}
+				if contribution
+					.net_value_for_initiator_at_feerate(rbf_feerate, holder_balance)
+					.is_ok()
+				{
+					let mut adjusted = contribution
+						.for_initiator_at_feerate(rbf_feerate, holder_balance)
+						.expect("feerate compatibility already checked");
+					adjusted.max_feerate = max_feerate;
+					return Ok(adjusted);
 				}
 				build_funding_contribution!(
 					contribution.value_added,
@@ -620,17 +617,15 @@ impl FundingTemplate {
 		match prior_contribution {
 			Some(PriorContribution { contribution, holder_balance }) => {
 				// See comment in `rbf` for details on when this adjustment fails.
-				if let Some(holder_balance) = holder_balance {
-					if contribution
-						.net_value_for_initiator_at_feerate(rbf_feerate, holder_balance)
-						.is_ok()
-					{
-						let mut adjusted = contribution
-							.for_initiator_at_feerate(rbf_feerate, holder_balance)
-							.expect("feerate compatibility already checked");
-						adjusted.max_feerate = max_feerate;
-						return Ok(adjusted);
-					}
+				if contribution
+					.net_value_for_initiator_at_feerate(rbf_feerate, holder_balance)
+					.is_ok()
+				{
+					let mut adjusted = contribution
+						.for_initiator_at_feerate(rbf_feerate, holder_balance)
+						.expect("feerate compatibility already checked");
+					adjusted.max_feerate = max_feerate;
+					return Ok(adjusted);
 				}
 				build_funding_contribution!(
 					contribution.value_added,
@@ -2355,7 +2350,7 @@ mod tests {
 		let template = FundingTemplate::new(
 			None,
 			Some(min_rbf_feerate),
-			Some(PriorContribution::new(prior, None)),
+			Some(PriorContribution::new(prior, Amount::MAX)),
 		);
 		assert!(matches!(
 			template.rbf_sync(max_feerate, UnreachableWallet),
@@ -2390,7 +2385,7 @@ mod tests {
 		let template = FundingTemplate::new(
 			None,
 			Some(min_rbf_feerate),
-			Some(PriorContribution::new(prior, Some(Amount::MAX))),
+			Some(PriorContribution::new(prior, Amount::MAX)),
 		);
 		let contribution = template.rbf_sync(max_feerate, UnreachableWallet).unwrap();
 		assert_eq!(contribution.feerate, min_rbf_feerate);
@@ -2452,7 +2447,7 @@ mod tests {
 		let template = FundingTemplate::new(
 			Some(shared_input(100_000)),
 			Some(min_rbf_feerate),
-			Some(PriorContribution::new(prior, None)),
+			Some(PriorContribution::new(prior, Amount::ZERO)),
 		);
 
 		let wallet = SingleUtxoWallet {
@@ -2513,7 +2508,7 @@ mod tests {
 		let template = FundingTemplate::new(
 			Some(shared_input(100_000)),
 			Some(min_rbf_feerate),
-			Some(PriorContribution::new(prior, None)),
+			Some(PriorContribution::new(prior, Amount::MAX)),
 		);
 
 		let wallet = SingleUtxoWallet {

Risk score

Why this scored 29/100

Our methodology →

Potential impact 8/30

Exploitability 4/25

Stealth signal 3/15

Affected reach 5/15

Confidence 6/10

Evidence quality 3/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 29/100

Community notes

Related watches

Merge PR 'tx-sync: Parallelize esplora status queries' (#4913)

Merge PR 'Skip Electrum creator transaction downloads' (#4992)

Merge PR 'Use preferred sPK of watched txn in electrum, not rand ones' (#4867)