Use Amount in calculate_change_output_value
What the developer wrote
Authored by Jeffrey Czyz
Use Amount in calculate_change_output_value
What changed, and why it matters
This commit refactors a Bitcoin transaction fee and change-calculation routine to use the strongly-typed Amount type instead of raw u64 satoshi values. The main functional change is that input/output totals now use checked_add/checked_sub, which prevents silent overflow/underflow wraparound when summing large amounts. It also tightens one subtraction so an unexpected negative value panics instead of silently wrapping. This is a defensive hardening change rather than a fix for a known exploitable bug.
Treat as routine defensive hardening. Reviewers should verify that the new Amount arithmetic preserves all prior invariants (e.g., dust-limit behavior, fee calculations, and test expectations) and that no new panic paths are reachable from untrusted peer input. No urgent action required absent additional context.
Security signals we found
Arithmetic hardening: replaced saturating_add/saturating_sub on raw u64 with checked_add/checked_sub on Amount
One subtraction changed from saturating_sub to expect-guarded checked_sub, turning a silent wraparound into a panic
Type-system migration from u64 to Amount reduces unit-confusion bugs
No explicit security bug, CVE, or exploit described in commit message
Evidence from the diff
In rust-lightning’s interactive transaction construction, calculate_change_output_value and its caller in channel.rs were converted from u64 satoshis to bitcoin::Amount. Summations of inputs and outputs now use checked_add with Amount::MAX fallback, and the net-total-less-fees calculation uses checked_sub with Amount::ZERO fallback. The remaining_value after subtracting our contribution now uses checked_sub(…).expect(), replacing a previous saturating_sub. The change-output dust comparison now compares remaining_value.to_sat() against the dust limit. These changes reduce arithmetic overflow/underflow risk during funding/splicing transaction construction.
Changed components
lightning/src/ln/interactivetxs.rslightning/src/ln/channel.rsInspect captured patch +27 / −19
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 659735c..a1c48b6 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -6707,12 +6707,12 @@ impl FundingNegotiationContext {
},
}
};
- let mut change_output =
- TxOut { value: Amount::from_sat(change_value), script_pubkey: change_script };
+ let mut change_output = TxOut { value: change_value, script_pubkey: change_script };
let change_output_weight = get_output_weight(&change_output.script_pubkey).to_wu();
let change_output_fee =
fee_for_weight(self.funding_feerate_sat_per_1000_weight, change_output_weight);
- let change_value_decreased_with_fee = change_value.saturating_sub(change_output_fee);
+ let change_value_decreased_with_fee =
+ change_value.to_sat().saturating_sub(change_output_fee);
// Check dust limit again
if change_value_decreased_with_fee > context.holder_dust_limit_satoshis {
change_output.value = Amount::from_sat(change_value_decreased_with_fee);
diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index 4340aad..1ab9c6c 100644
--- a/lightning/src/ln/interactivetxs.rs
+++ b/lightning/src/ln/interactivetxs.rs
@@ -2337,22 +2337,23 @@ impl InteractiveTxConstructor {
pub(super) fn calculate_change_output_value(
context: &FundingNegotiationContext, is_splice: bool, shared_output_funding_script: &ScriptBuf,
change_output_dust_limit: u64,
-) -> Result<Option<u64>, AbortReason> {
+) -> Result<Option<Amount>, AbortReason> {
assert!(context.our_funding_contribution > SignedAmount::ZERO);
- let our_funding_contribution_satoshis = context.our_funding_contribution.to_sat() as u64;
+ let our_funding_contribution = context.our_funding_contribution.to_unsigned().unwrap();
- let mut total_input_satoshis = 0u64;
+ let mut total_input_value = Amount::ZERO;
let mut our_funding_inputs_weight = 0u64;
for FundingTxInput { utxo, .. } in context.our_funding_inputs.iter() {
- total_input_satoshis = total_input_satoshis.saturating_add(utxo.output.value.to_sat());
+ total_input_value = total_input_value.checked_add(utxo.output.value).unwrap_or(Amount::MAX);
let weight = BASE_INPUT_WEIGHT + utxo.satisfaction_weight;
our_funding_inputs_weight = our_funding_inputs_weight.saturating_add(weight);
}
let funding_outputs = &context.our_funding_outputs;
- let total_output_satoshis =
- funding_outputs.iter().fold(0u64, |total, out| total.saturating_add(out.value.to_sat()));
+ let total_output_value = funding_outputs
+ .iter()
+ .fold(Amount::ZERO, |total, out| total.checked_add(out.value).unwrap_or(Amount::MAX));
let our_funding_outputs_weight = funding_outputs.iter().fold(0u64, |weight, out| {
weight.saturating_add(get_output_weight(&out.script_pubkey).to_wu())
});
@@ -2376,15 +2377,22 @@ pub(super) fn calculate_change_output_value(
}
}
- let fees_sats = fee_for_weight(context.funding_feerate_sat_per_1000_weight, weight);
- let net_total_less_fees =
- total_input_satoshis.saturating_sub(total_output_satoshis).saturating_sub(fees_sats);
- if net_total_less_fees < our_funding_contribution_satoshis {
+ let contributed_fees =
+ Amount::from_sat(fee_for_weight(context.funding_feerate_sat_per_1000_weight, weight));
+ let net_total_less_fees = total_input_value
+ .checked_sub(total_output_value)
+ .unwrap_or(Amount::ZERO)
+ .checked_sub(contributed_fees)
+ .unwrap_or(Amount::ZERO);
+ if net_total_less_fees < our_funding_contribution {
// Not enough to cover contribution plus fees
return Err(AbortReason::InsufficientFees);
}
- let remaining_value = net_total_less_fees.saturating_sub(our_funding_contribution_satoshis);
- if remaining_value < change_output_dust_limit {
+
+ let remaining_value = net_total_less_fees
+ .checked_sub(our_funding_contribution)
+ .expect("remaining_value should not be negative");
+ if remaining_value.to_sat() < change_output_dust_limit {
// Enough to cover contribution plus fees, but leftover is below dust limit; no change
Ok(None)
} else {
@@ -3440,14 +3448,14 @@ mod tests {
total_inputs - total_outputs - context.our_funding_contribution.to_unsigned().unwrap();
assert_eq!(
calculate_change_output_value(&context, false, &ScriptBuf::new(), 300),
- Ok(Some((gross_change - fees - common_fees).to_sat())),
+ Ok(Some(gross_change - fees - common_fees)),
);
// There is leftover for change, without common fees
let context = FundingNegotiationContext { is_initiator: false, ..context };
assert_eq!(
calculate_change_output_value(&context, false, &ScriptBuf::new(), 300),
- Ok(Some((gross_change - fees).to_sat())),
+ Ok(Some(gross_change - fees)),
);
// Insufficient inputs, no leftover
@@ -3482,7 +3490,7 @@ mod tests {
total_inputs - total_outputs - context.our_funding_contribution.to_unsigned().unwrap();
assert_eq!(
calculate_change_output_value(&context, false, &ScriptBuf::new(), 100),
- Ok(Some((gross_change - fees).to_sat())),
+ Ok(Some(gross_change - fees)),
);
// Larger fee, smaller change
@@ -3496,7 +3504,7 @@ mod tests {
total_inputs - total_outputs - context.our_funding_contribution.to_unsigned().unwrap();
assert_eq!(
calculate_change_output_value(&context, false, &ScriptBuf::new(), 300),
- Ok(Some((gross_change - fees * 3 - common_fees * 3).to_sat())),
+ Ok(Some(gross_change - fees * 3 - common_fees * 3)),
);
}
Why this scored 26/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.