Account for missing balance in splice max commitment output tracking

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks

[wpaulino]

When a splice creates new funding, the monotonicity debug assertion trackers were initialized to the raw post-splice balance without accounting for pending HTLCs or anchor costs. Since splices can have in-flight HTLCs (unlike fresh channel opens), the first commitment transaction's actual balance was lower than the initialized max, causing the debug assertion in ChannelContext::build_commitment_transaction to fire.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.91%. Comparing base (153e57e) to head (034892b).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #4417 +/- ##
=======================================
Coverage 85.90% 85.91% =======================================
Files 156 156 Lines 103965 103975 +10 Branches 103965 103975 +10 =======================================
+ Hits 89316 89332 +16 + Misses 12128 12119 -9 - Partials 2521 2524 +3 

Flag	Coverage Δ
tests	85.91% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tankyleo]

This is the direction I have in mind what do you think ? We'd also assert that a balance that is under the new reserve did not decrease across the splice. Probably want to rename the state variables to highlight that once we are above the reserve, these track the balance on the previous commitment.

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b8dc90ef9..8de424beb 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -2772,55 +2772,18 @@ impl FundingScope {
context.counterparty_dust_limit_satoshis,
);
- // Account for in-flight HTLCs and anchor outputs when initializing the max- // commitment tx output trackers. Unlike a fresh channel open (which has no HTLCs),- // a splice may have pending HTLCs whose amounts are subtracted from the balances- // in the commitment transaction. Without this adjustment, the monotonicity debug- // assertion in `build_commitment_transaction` would fire on the first commitment.- #[cfg(debug_assertions)]- let (local_balance_msat, remote_balance_msat) = {- let pending_outbound_htlcs_value_msat: u64 =- context.pending_outbound_htlcs.iter().map(|h| h.amount_msat).sum();- let pending_inbound_htlcs_value_msat: u64 =- context.pending_inbound_htlcs.iter().map(|h| h.amount_msat).sum();- let channel_type = &post_channel_transaction_parameters.channel_type_features;- let total_anchors_sat = if channel_type.supports_anchors_zero_fee_htlc_tx() {- ANCHOR_OUTPUT_VALUE_SATOSHI * 2- } else {- 0- };- let post_value_to_remote_msat =- (post_channel_value * 1000).saturating_sub(post_value_to_self_msat);- if post_channel_transaction_parameters.is_outbound_from_holder {- (- post_value_to_self_msat- .saturating_sub(pending_outbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- post_value_to_remote_msat.saturating_sub(pending_inbound_htlcs_value_msat),- )- } else {- (- post_value_to_self_msat.saturating_sub(pending_outbound_htlcs_value_msat),- post_value_to_remote_msat- .saturating_sub(pending_inbound_htlcs_value_msat)- .saturating_sub(total_anchors_sat * 1000),- )- }- };-
Self {
channel_transaction_parameters: post_channel_transaction_parameters,
value_to_self_msat: post_value_to_self_msat,
funding_transaction: None,
counterparty_selected_channel_reserve_satoshis,
holder_selected_channel_reserve_satoshis,
+ // Here we copy over these values; if the party is below the reserve under the new funding+ // scope, their balance MUST NOT decrease.
#[cfg(debug_assertions)]
- holder_max_commitment_tx_output: Mutex::new((local_balance_msat, remote_balance_msat)),+ holder_max_commitment_tx_output: Mutex::new(prev_funding.holder_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(debug_assertions)]
- counterparty_max_commitment_tx_output: Mutex::new((- local_balance_msat,- remote_balance_msat,- )),+ counterparty_max_commitment_tx_output: Mutex::new(prev_funding.counterparty_max_commitment_tx_output.lock().unwrap().clone()),
#[cfg(any(test, fuzzing))]
next_local_fee: Mutex::new(PredictedNextFee::default()),
#[cfg(any(test, fuzzing))]
@@ -5544,12 +5507,21 @@ impl<SP: SignerProvider> ChannelContext<SP> {
} else {
funding.counterparty_max_commitment_tx_output.lock().unwrap()
};
- debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat || stats.local_balance_before_fee_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap());- broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, stats.local_balance_before_fee_msat);- debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat || stats.remote_balance_before_fee_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis);- broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, stats.remote_balance_before_fee_msat);- }+ if stats.local_balance_before_fee_msat / 1000 < funding.counterparty_selected_channel_reserve_satoshis.unwrap() {+ // If local is below the reserve on this new commitment, local balance MUST be greater than+ // or equal to local balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.0 <= stats.local_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.0 = stats.local_balance_before_fee_msat;++ if stats.remote_balance_before_fee_msat / 1000 < funding.holder_selected_channel_reserve_satoshis {+ // If remote is below the reserve on this new commitment, remote balance MUST be greater than+ // or equal to remote balance on the previous commitment, even across a splice.+ debug_assert!(broadcaster_max_commitment_tx_output.1 <= stats.remote_balance_before_fee_msat);+ }+ broadcaster_max_commitment_tx_output.1 = stats.remote_balance_before_fee_msat;+ }
// This populates the HTLC-source table with the indices from the HTLCs in the commitment
// transaction.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[TheBlueMatt]

thanks