Make `channel_parameters` explicit to `build_counterparty_commitment_tx`
What the developer wrote
Authored by Wilmer Paulino
Make `channel_parameters` explicit to `build_counterparty_commitment_tx`
It's best to let the caller decide what the appropriate
`ChannelTransactionParameters` are as it has the most context.
What changed, and why it matters
This commit is a small internal refactoring in the Lightning Dev Kit's channel monitoring code. It changes a helper function so that the caller explicitly passes in the channel parameters, rather than the helper always grabbing them from a fixed internal field. The commit also adds safety comments and debug-only checks (debug_assert!) confirming this path is only used when there is a single active funding scope. There is no direct evidence in the commit or supplied references that this fixes an active security vulnerability; it appears to be a defensive code-quality change that makes future multi-funding-scope logic safer.
Treat as a normal code-quality / defensive refactoring patch. Reviewers should verify that all call sites of build_counterparty_commitment_tx now pass the correct channel parameters, especially in any follow-up work introducing multiple FundingScope support. No urgent security action is indicated by this commit alone.
Security signals we found
Refactoring to make channel parameter selection explicit rather than implicit
Added debug_assert! preconditions about single FundingScope
Added comments explaining invariants around initial monitor persistence and counterparty commitment updates
No direct security claim or CVE reference in commit message or diff
Evidence from the diff
In lightning/src/chain/channelmonitor.rs, build_counterparty_commitment_tx is modified to take channel_parameters: &ChannelTransactionParameters as an explicit parameter instead of reading &self.funding.channel_parameters internally. Both call sites are updated to pass &self.funding.channel_parameters, and debug_assert!(self.pending_funding.is_empty()) plus comments are added to document the precondition that this code path is only valid with a single FundingScope. The functional behavior is unchanged for the current single-funding case; the change gives callers control over which parameters to use, which is groundwork for handling multiple concurrent funding scopes correctly.
Changed components
lightning/src/chain/channelmonitor.rsChannelMonitorImpl::build_counterparty_commitment_txInitial counterparty commitment generationCounterparty commitment update handlingInspect captured patch +24 / −6
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 04e81c9..15bd651 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -4187,8 +4187,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
to_countersignatory_value,
)| {
let nondust_htlcs = vec![];
+ // Since we're expected to only reach here during the initial persistence of a
+ // monitor (i.e., via [`Persist::persist_new_channel`]), we expect to only have
+ // one `FundingScope` present.
+ debug_assert!(self.pending_funding.is_empty());
+ let channel_parameters = &self.funding.channel_parameters;
let commitment_tx = self.build_counterparty_commitment_tx(
+ channel_parameters,
INITIAL_COMMITMENT_NUMBER,
&their_per_commitment_point,
to_broadcaster_value,
@@ -4206,11 +4212,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
#[rustfmt::skip]
fn build_counterparty_commitment_tx(
- &self, commitment_number: u64, their_per_commitment_point: &PublicKey,
- to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32,
+ &self, channel_parameters: &ChannelTransactionParameters, commitment_number: u64,
+ their_per_commitment_point: &PublicKey, to_broadcaster_value: u64,
+ to_countersignatory_value: u64, feerate_per_kw: u32,
nondust_htlcs: Vec<HTLCOutputInCommitment>
) -> CommitmentTransaction {
- let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable();
+ let channel_parameters = &channel_parameters.as_counterparty_broadcastable();
CommitmentTransaction::new(commitment_number, their_per_commitment_point,
to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
}
@@ -4232,9 +4239,20 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
htlc.transaction_output_index.map(|_| htlc).cloned()
}).collect::<Vec<_>>();
- let commitment_tx = self.build_counterparty_commitment_tx(commitment_number,
- &their_per_commitment_point, to_broadcaster_value,
- to_countersignatory_value, feerate_per_kw, nondust_htlcs);
+ // This monitor update variant is only applicable while there's a single
+ // `FundingScope` active, otherwise we expect to see
+ // `LatestCounterpartyCommitment` instead.
+ debug_assert!(self.pending_funding.is_empty());
+ let channel_parameters = &self.funding.channel_parameters;
+ let commitment_tx = self.build_counterparty_commitment_tx(
+ channel_parameters,
+ commitment_number,
+ &their_per_commitment_point,
+ to_broadcaster_value,
+ to_countersignatory_value,
+ feerate_per_kw,
+ nondust_htlcs,
+ );
debug_assert_eq!(commitment_tx.trust().txid(), commitment_txid);
Why this scored 25/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.