Store current_point in HolderCommitmentPoint
What the developer wrote
Authored by Jeffrey Czyz
Store current_point in HolderCommitmentPoint
When splicing a channel, sending and receiving the initial
commitment_signed message should use the current commitment point
rather then the next one. Store this in HolderCommitmentPoint whenever
it is advanced.
What changed, and why it matters
This commit fixes a state-tracking bug in the experimental splicing feature of a Lightning Network implementation. Previously, during a splice, the code used the wrong commitment point (the 'next' one instead of the 'current' one) when sending or receiving the first commitment_signed message. The fix stores the current commitment point explicitly and adds guards that prevent splicing until the commitment point has been advanced at least once. It also persists the new field across restarts. The change is defensive and corrects protocol behavior, but it is not a clear-cut critical security patch on its own.
Treat as a correctness fix for an in-development splicing feature. Review whether the wrong commitment point could have led to invalid or unenforceable commitment signatures in splice scenarios, and ensure test coverage exists for splice after restart/upgrade when current_point is None. No immediate emergency response is indicated by the commit alone.
Security signals we found
Protocol-state correctness fix for splicing commitment point selection
Addition of defensive API misuse checks before splice operations
Serialization change adding a new persisted field (TLV 63)
Potential downgrade/upgrade state inconsistency: pre-fix code does not store current_point, so after upgrade current_point may be None until next commitment advance
No explicit mention of vulnerability, CVE, security bug, or researcher attribution in commit
Evidence from the diff
The patch modifies HolderCommitmentPoint in lightning/src/ln/channel.rs to track current_point alongside next_point and pending_next_point. It adds current_transaction_number() and current_point() accessors, updates advance() to set current_point to the old next_point, and changes get_cur_holder_commitment_transaction_number() to use the new accessor. Splice initiation and acceptance now return errors if current_point is None, ensuring the commitment point has advanced once. The field is serialized/deserialized in channel persistence. For downgrades, older code reading the new field will ignore unknown TLV 63; for upgrades, current_point starts as None and is populated on the next advance, with the splice guards preventing use before that.
Changed components
lightning/src/ln/channel.rsHolderCommitmentPoint state structChannel splice initiation (splice_init)Channel splice acceptance (splice_ack handling)Channel persistence serialization/deserializationCommitment transaction number reportingInspect captured patch +34 / −1
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 7d4983f..3d035b9 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1251,6 +1251,7 @@ pub(crate) struct ShutdownResult {
#[derive(Debug, Copy, Clone)]
struct HolderCommitmentPoint {
next_transaction_number: u64,
+ current_point: Option<PublicKey>,
next_point: PublicKey,
pending_next_point: Option<PublicKey>,
}
@@ -1262,6 +1263,7 @@ impl HolderCommitmentPoint {
{
Some(HolderCommitmentPoint {
next_transaction_number: INITIAL_COMMITMENT_NUMBER,
+ current_point: None,
next_point: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx).ok()?,
pending_next_point: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx).ok(),
})
@@ -1271,6 +1273,14 @@ impl HolderCommitmentPoint {
self.pending_next_point.is_some()
}
+ pub fn current_transaction_number(&self) -> u64 {
+ self.next_transaction_number + 1
+ }
+
+ pub fn current_point(&self) -> Option<PublicKey> {
+ self.current_point
+ }
+
pub fn next_transaction_number(&self) -> u64 {
self.next_transaction_number
}
@@ -1328,6 +1338,7 @@ impl HolderCommitmentPoint {
if let Some(next_point) = self.pending_next_point {
*self = Self {
next_transaction_number: self.next_transaction_number - 1,
+ current_point: Some(self.next_point),
next_point,
pending_next_point: None,
};
@@ -9591,7 +9602,7 @@ where
}
pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 {
- self.holder_commitment_point.next_transaction_number() + 1
+ self.holder_commitment_point.current_transaction_number()
}
pub fn get_cur_counterparty_commitment_transaction_number(&self) -> u64 {
@@ -10582,6 +10593,15 @@ where
our_funding_inputs: Vec<(TxIn, Transaction, Weight)>, change_script: Option<ScriptBuf>,
funding_feerate_per_kw: u32, locktime: u32,
) -> Result<msgs::SpliceInit, APIError> {
+ if self.holder_commitment_point.current_point().is_none() {
+ return Err(APIError::APIMisuseError {
+ err: format!(
+ "Channel {} cannot be spliced, commitment point needs to be advanced once",
+ self.context.channel_id(),
+ ),
+ });
+ }
+
// Check if a splice has been initiated already.
// Note: only a single outstanding splice is supported (per spec)
if self.pending_splice.is_some() {
@@ -10683,6 +10703,13 @@ where
// TODO(splicing): Add check that we are the quiescence acceptor
+ if self.holder_commitment_point.current_point().is_none() {
+ return Err(ChannelError::Warn(format!(
+ "Channel {} commitment point needs to be advanced once before spliced",
+ self.context.channel_id(),
+ )));
+ }
+
// Check if a splice has been initiated already.
if self.pending_splice.is_some() {
return Err(ChannelError::WarnAndDisconnect(format!(
@@ -13198,6 +13225,7 @@ where
}
let is_manual_broadcast = Some(self.context.is_manual_broadcast);
+ let holder_commitment_point_current = self.holder_commitment_point.current_point();
// `HolderCommitmentPoint::next_point` will become optional when async signing is implemented.
let holder_commitment_point_next = Some(self.holder_commitment_point.next_point());
let holder_commitment_point_pending_next = self.holder_commitment_point.pending_next_point;
@@ -13250,6 +13278,7 @@ where
(59, self.funding.minimum_depth_override, option), // Added in 0.2
(60, self.context.historical_scids, optional_vec), // Added in 0.2
(61, fulfill_attribution_data, optional_vec), // Added in 0.2
+ (63, holder_commitment_point_current, option), // Added in 0.2
});
Ok(())
@@ -13599,6 +13628,7 @@ where
let mut malformed_htlcs: Option<Vec<(u64, u16, [u8; 32])>> = None;
let mut monitor_pending_update_adds: Option<Vec<msgs::UpdateAddHTLC>> = None;
+ let mut holder_commitment_point_current_opt: Option<PublicKey> = None;
let mut holder_commitment_point_next_opt: Option<PublicKey> = None;
let mut holder_commitment_point_pending_next_opt: Option<PublicKey> = None;
let mut is_manual_broadcast = None;
@@ -13652,6 +13682,7 @@ where
(59, minimum_depth_override, option), // Added in 0.2
(60, historical_scids, optional_vec), // Added in 0.2
(61, fulfill_attribution_data, optional_vec), // Added in 0.2
+ (63, holder_commitment_point_current_opt, option), // Added in 0.2
});
let holder_signer = signer_provider.derive_channel_signer(channel_keys_id);
@@ -13833,6 +13864,7 @@ where
match (holder_commitment_point_next_opt, holder_commitment_point_pending_next_opt) {
(Some(next_point), pending_next_point) => HolderCommitmentPoint {
next_transaction_number: holder_commitment_next_transaction_number,
+ current_point: None,
next_point,
pending_next_point,
},
@@ -13852,6 +13884,7 @@ where
);
HolderCommitmentPoint {
next_transaction_number: holder_commitment_next_transaction_number,
+ current_point: holder_commitment_point_current_opt,
next_point,
pending_next_point: Some(pending_next_point),
}
Why this scored 41/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.