diff --git a/lightning/src/ln/async_signer_tests.rs b/lightning/src/ln/async_signer_tests.rs index d66ba79cc81..e732343bf11 100644 --- a/lightning/src/ln/async_signer_tests.rs +++ b/lightning/src/ln/async_signer_tests.rs @@ -350,6 +350,10 @@ fn do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack( assert!(events.is_empty(), "expected no message, got {}", events.len()); } } + + // Enable SignerOp::GetPerCommitmentPoint since testing the node serialization round-trip + // involves using the signer to get the previous holder commitment point. + dst.enable_channel_signer_op(&src_node_id, &chan_id, SignerOp::GetPerCommitmentPoint); } #[test] diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 56d38d5545c..55662e6089b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1249,14 +1249,10 @@ pub(crate) struct ShutdownResult { /// This consolidates the logic to advance our commitment number and request new /// commitment points from our signer. #[derive(Debug, Copy, Clone)] -enum HolderCommitmentPoint { - /// We've advanced our commitment number and are waiting on the next commitment point. - /// - /// We should retry advancing to `Available` via `try_resolve_pending` once our - /// signer is ready to provide the next commitment point. - PendingNext { transaction_number: u64, current: PublicKey }, - /// Our current commitment point is ready and we've cached our next point. - Available { transaction_number: u64, current: PublicKey, next: PublicKey }, +struct HolderCommitmentPoint { + transaction_number: u64, + point: PublicKey, + next_point: Option, } impl HolderCommitmentPoint { @@ -1264,101 +1260,81 @@ impl HolderCommitmentPoint { pub fn new(signer: &ChannelSignerType, secp_ctx: &Secp256k1) -> Option where SP::Target: SignerProvider { - let current = signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx).ok()?; - let next = signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx).ok(); - let point = if let Some(next) = next { - HolderCommitmentPoint::Available { transaction_number: INITIAL_COMMITMENT_NUMBER, current, next } - } else { - HolderCommitmentPoint::PendingNext { transaction_number: INITIAL_COMMITMENT_NUMBER, current } - }; - Some(point) + Some(HolderCommitmentPoint { + transaction_number: INITIAL_COMMITMENT_NUMBER, + point: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, secp_ctx).ok()?, + next_point: signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, secp_ctx).ok(), + }) } - #[rustfmt::skip] - pub fn is_available(&self) -> bool { - if let HolderCommitmentPoint::Available { .. } = self { true } else { false } + pub fn can_advance(&self) -> bool { + self.next_point.is_some() } pub fn transaction_number(&self) -> u64 { - match self { - HolderCommitmentPoint::PendingNext { transaction_number, .. } => *transaction_number, - HolderCommitmentPoint::Available { transaction_number, .. } => *transaction_number, - } + self.transaction_number } - pub fn current_point(&self) -> PublicKey { - match self { - HolderCommitmentPoint::PendingNext { current, .. } => *current, - HolderCommitmentPoint::Available { current, .. } => *current, - } + pub fn point(&self) -> PublicKey { + self.point } pub fn next_point(&self) -> Option { - match self { - HolderCommitmentPoint::PendingNext { .. } => None, - HolderCommitmentPoint::Available { next, .. } => Some(*next), - } + self.next_point } - /// If we are pending the next commitment point, this method tries asking the signer again, - /// and transitions to the next state if successful. - /// - /// This method is used for the following transitions: - /// - `PendingNext` -> `Available` + /// If we are pending the next commitment point, this method tries asking the signer again. pub fn try_resolve_pending( &mut self, signer: &ChannelSignerType, secp_ctx: &Secp256k1, logger: &L, ) where SP::Target: SignerProvider, L::Target: Logger, { - if let HolderCommitmentPoint::PendingNext { transaction_number, current } = self { - let next = signer.as_ref().get_per_commitment_point(*transaction_number - 1, secp_ctx); + if !self.can_advance() { + let next = + signer.as_ref().get_per_commitment_point(self.transaction_number - 1, secp_ctx); if let Ok(next) = next { log_trace!( logger, "Retrieved next per-commitment point {}", - *transaction_number - 1 + self.transaction_number - 1 ); - *self = HolderCommitmentPoint::Available { - transaction_number: *transaction_number, - current: *current, - next, - }; + self.next_point = Some(next); } else { - log_trace!(logger, "Next per-commitment point {} is pending", transaction_number); + log_trace!( + logger, + "Next per-commitment point {} is pending", + self.transaction_number + ); } } } - /// If we are not pending the next commitment point, this method advances the commitment number - /// and requests the next commitment point from the signer. Returns `Ok` if we were at - /// `Available` and were able to advance our commitment number (even if we are still pending - /// the next commitment point). + /// Returns the next [`HolderCommitmentPoint`] if it is available (i.e., was previously obtained + /// from `signer` and cached), leaving the callee unchanged. /// - /// If our signer is not ready to provide the next commitment point, we will - /// only advance to `PendingNext`, and should be tried again later in `signer_unblocked` - /// via `try_resolve_pending`. + /// Otherwise, returns an `Err` indicating that the signer wasn't previously ready and that the + /// caller must invoke `try_resolve_pending` once it is. /// - /// If our signer is ready to provide the next commitment point, we will advance all the - /// way to `Available`. - /// - /// This method is used for the following transitions: - /// - `Available` -> `PendingNext` - /// - `Available` -> `PendingNext` -> `Available` (in one fell swoop) + /// Attempts to resolve the next point on the *returned* [`HolderCommitmentPoint`], if `signer` + /// is ready, allowing *it* to be advanced later. Otherwise, `try_resolve_pending` must be + /// called on it, typically via [`Channel::signer_maybe_unblocked`]. pub fn advance( - &mut self, signer: &ChannelSignerType, secp_ctx: &Secp256k1, logger: &L, - ) -> Result<(), ()> + &self, signer: &ChannelSignerType, secp_ctx: &Secp256k1, logger: &L, + ) -> Result where SP::Target: SignerProvider, L::Target: Logger, { - if let HolderCommitmentPoint::Available { transaction_number, next, .. } = self { - *self = HolderCommitmentPoint::PendingNext { - transaction_number: *transaction_number - 1, - current: *next, + if let Some(next_point) = self.next_point { + let mut advanced_point = Self { + transaction_number: self.transaction_number - 1, + point: next_point, + next_point: None, }; - self.try_resolve_pending(signer, secp_ctx, logger); - return Ok(()); + + advanced_point.try_resolve_pending(signer, secp_ctx, logger); + return Ok(advanced_point); } Err(()) } @@ -1813,7 +1789,7 @@ where &mut funding, &mut signing_session, true, - chan.holder_commitment_point.transaction_number(), + chan.next_holder_commitment_point.transaction_number(), &&logger, )?; @@ -1859,7 +1835,7 @@ where let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined); match phase { ChannelPhase::UnfundedV2(chan) => { - let holder_commitment_point = match chan.unfunded_context.holder_commitment_point { + let initial_holder_commitment_point = match chan.unfunded_context.initial_holder_commitment_point { Some(point) => point, None => { let channel_id = chan.context.channel_id(); @@ -1874,7 +1850,8 @@ where pending_funding: vec![], context: chan.context, interactive_tx_signing_session: chan.interactive_tx_signing_session, - holder_commitment_point, + current_holder_commitment_point: None, + next_holder_commitment_point: initial_holder_commitment_point, #[cfg(splicing)] pending_splice: None, }; @@ -2011,7 +1988,7 @@ pub(super) struct UnfundedChannelContext { /// in a timely manner. unfunded_channel_age_ticks: usize, /// Tracks the commitment number and commitment point before the channel is funded. - holder_commitment_point: Option, + initial_holder_commitment_point: Option, } impl UnfundedChannelContext { @@ -2025,7 +2002,7 @@ impl UnfundedChannelContext { } fn transaction_number(&self) -> u64 { - self.holder_commitment_point + self.initial_holder_commitment_point .as_ref() .map(|point| point.transaction_number()) .unwrap_or(INITIAL_COMMITMENT_NUMBER) @@ -2771,7 +2748,7 @@ where let funding_script = self.funding().get_funding_redeemscript(); let commitment_data = self.context().build_commitment_transaction(self.funding(), - holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), + holder_commitment_point.transaction_number(), &holder_commitment_point.point(), true, false, logger); let initial_commitment_tx = commitment_data.tx; let trusted_tx = initial_commitment_tx.trust(); @@ -2789,9 +2766,9 @@ where #[rustfmt::skip] fn initial_commitment_signed( - &mut self, channel_id: ChannelId, counterparty_signature: Signature, holder_commitment_point: &mut HolderCommitmentPoint, + &mut self, channel_id: ChannelId, counterparty_signature: Signature, holder_commitment_point: &HolderCommitmentPoint, best_block: BestBlock, signer_provider: &SP, logger: &L, - ) -> Result<(ChannelMonitor<::EcdsaSigner>, CommitmentTransaction), ChannelError> + ) -> Result<(ChannelMonitor<::EcdsaSigner>, CommitmentTransaction, HolderCommitmentPoint), ChannelError> where L::Target: Logger { @@ -2847,14 +2824,16 @@ where context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); } } - if holder_commitment_point.advance(&context.holder_signer, &context.secp_ctx, logger).is_err() { - // We only fail to advance our commitment point/number if we're currently - // waiting for our signer to unblock and provide a commitment point. - // We cannot send accept_channel/open_channel before this has occurred, so if we - // err here by the time we receive funding_created/funding_signed, something has gone wrong. - debug_assert!(false, "We should be ready to advance our commitment point by the time we receive {}", self.received_msg()); - return Err(ChannelError::close("Failed to advance holder commitment point".to_owned())); - } + let advanced_holder_commitment_point = holder_commitment_point + .advance(&context.holder_signer, &context.secp_ctx, logger) + .map_err(|()| { + // We only fail to advance our commitment point/number if we're currently + // waiting for our signer to unblock and provide a commitment point. + // We cannot send accept_channel/open_channel before this has occurred, so if we + // err here by the time we receive funding_created/funding_signed, something has gone wrong. + debug_assert!(false, "We should be ready to advance our commitment point by the time we receive {}", self.received_msg()); + ChannelError::close("Failed to advance holder commitment point".to_owned()) + })?; let context = self.context(); let funding = self.funding(); @@ -2875,7 +2854,7 @@ where self.context_mut().cur_counterparty_commitment_transaction_number -= 1; - Ok((channel_monitor, counterparty_initial_commitment_tx)) + Ok((channel_monitor, counterparty_initial_commitment_tx, advanced_holder_commitment_point)) } fn is_v2_established(&self) -> bool; @@ -4218,7 +4197,7 @@ where let funding_script = funding.get_funding_redeemscript(); let commitment_data = self.build_commitment_transaction(funding, - holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), + holder_commitment_point.transaction_number(), &holder_commitment_point.point(), true, false, logger); let commitment_txid = { let trusted_tx = commitment_data.tx.trust(); @@ -5604,10 +5583,19 @@ where SP::Target: SignerProvider, L::Target: Logger, { + let mut commitment_number = self.cur_counterparty_commitment_transaction_number; + let mut commitment_point = self.counterparty_cur_commitment_point.unwrap(); + + // Use the previous commitment number and point when splicing since they shouldn't change. + if commitment_number != INITIAL_COMMITMENT_NUMBER { + commitment_number += 1; + commitment_point = self.counterparty_prev_commitment_point.unwrap(); + } + let commitment_data = self.build_commitment_transaction( funding, - self.cur_counterparty_commitment_transaction_number, - &self.counterparty_cur_commitment_point.unwrap(), + commitment_number, + &commitment_point, false, false, logger, @@ -6088,7 +6076,13 @@ where /// /// This field is cleared once our counterparty sends a `channel_ready`. pub interactive_tx_signing_session: Option, - holder_commitment_point: HolderCommitmentPoint, + + /// The commitment point used for the current holder commitment transaction. + current_holder_commitment_point: Option, + + /// The commitment point used for the next holder commitment transaction. + next_holder_commitment_point: HolderCommitmentPoint, + /// Info about an in-progress, pending splice (if any), on the pre-splice channel #[cfg(splicing)] pending_splice: Option, @@ -6967,12 +6961,13 @@ where return Err(ChannelError::Close((msg.to_owned(), reason))); } - let holder_commitment_point = &mut self.holder_commitment_point.clone(); + let holder_commitment_point = self.next_holder_commitment_point.clone(); self.context.assert_no_commitment_advancement(holder_commitment_point.transaction_number(), "initial commitment_signed"); - let (channel_monitor, _) = self.initial_commitment_signed( - self.context.channel_id(), msg.signature, holder_commitment_point, best_block, signer_provider, logger)?; - self.holder_commitment_point = *holder_commitment_point; + let (channel_monitor, _, next_holder_commitment_point) = self.initial_commitment_signed( + self.context.channel_id(), msg.signature, &holder_commitment_point, best_block, signer_provider, logger)?; + self.current_holder_commitment_point = Some(holder_commitment_point); + self.next_holder_commitment_point = next_holder_commitment_point; log_info!(logger, "Received initial commitment_signed from peer for channel {}", &self.context.channel_id()); @@ -7017,9 +7012,13 @@ where }) .and_then(|funding_negotiation| funding_negotiation.as_funding()) .expect("Funding must exist for negotiated pending splice"); + let holder_commitment_point = self + .current_holder_commitment_point + .as_ref() + .expect("current should be set after receiving the initial commitment_signed"); let (holder_commitment_tx, _) = self.context.validate_commitment_signed( pending_splice_funding, - &self.holder_commitment_point, + holder_commitment_point, msg, logger, )?; @@ -7029,8 +7028,8 @@ where .context .build_commitment_transaction( pending_splice_funding, - self.context.cur_counterparty_commitment_transaction_number, - &self.context.counterparty_cur_commitment_point.unwrap(), + self.context.cur_counterparty_commitment_transaction_number + 1, + &self.context.counterparty_prev_commitment_point.unwrap(), false, false, logger, @@ -7103,9 +7102,10 @@ where )); } + let holder_commitment_point = &self.next_holder_commitment_point; let update = self .context - .validate_commitment_signed(&self.funding, &self.holder_commitment_point, msg, logger) + .validate_commitment_signed(&self.funding, holder_commitment_point, msg, logger) .map(|(commitment_tx, htlcs_included)| { let (nondust_htlc_sources, dust_htlcs) = Self::get_commitment_htlc_data(&htlcs_included); @@ -7169,7 +7169,7 @@ where })?; let (commitment_tx, htlcs_included) = self.context.validate_commitment_signed( funding, - &self.holder_commitment_point, + &self.next_holder_commitment_point, msg, logger, )?; @@ -7227,23 +7227,24 @@ where where L::Target: Logger, { - if self - .holder_commitment_point + let next_holder_commitment_point = self + .next_holder_commitment_point .advance(&self.context.holder_signer, &self.context.secp_ctx, logger) - .is_err() - { - // We only fail to advance our commitment point/number if we're currently - // waiting for our signer to unblock and provide a commitment point. - // During post-funding channel operation, we only advance our point upon - // receiving a commitment_signed, and our counterparty cannot send us - // another commitment signed until we've provided a new commitment point - // in revoke_and_ack, which requires unblocking our signer and completing - // the advance to the next point. This should be unreachable since - // a new commitment_signed should fail at our signature checks in - // validate_commitment_signed. - debug_assert!(false, "We should be ready to advance our commitment point by the time we receive commitment_signed"); - return Err(ChannelError::close("Failed to advance our commitment point".to_owned())); - } + .map_err(|()| { + // We only fail to advance our commitment point/number if we're currently + // waiting for our signer to unblock and provide a commitment point. + // During post-funding channel operation, we only advance our point upon + // receiving a commitment_signed, and our counterparty cannot send us + // another commitment signed until we've provided a new commitment point + // in revoke_and_ack, which requires unblocking our signer and completing + // the advance to the next point. This should be unreachable since + // a new commitment_signed should fail at our signature checks in + // validate_commitment_signed. + debug_assert!(false, "We should be ready to advance our commitment point by the time we receive commitment_signed"); + ChannelError::close("Failed to advance our commitment point".to_owned()) + })?; + self.current_holder_commitment_point = Some(self.next_holder_commitment_point); + self.next_holder_commitment_point = next_holder_commitment_point; // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; @@ -8412,9 +8413,9 @@ where /// blocked. #[rustfmt::skip] pub fn signer_maybe_unblocked(&mut self, logger: &L) -> SignerResumeUpdates where L::Target: Logger { - if !self.holder_commitment_point.is_available() { + if !self.next_holder_commitment_point.can_advance() { log_trace!(logger, "Attempting to update holder per-commitment point..."); - self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); + self.next_holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); } let funding_signed = if self.context.signer_pending_funding && !self.funding.is_outbound() { let commitment_data = self.context.build_commitment_transaction(&self.funding, @@ -8509,38 +8510,39 @@ where #[rustfmt::skip] fn get_last_revoke_and_ack(&mut self, logger: &L) -> Option where L::Target: Logger { - debug_assert!(self.holder_commitment_point.transaction_number() <= INITIAL_COMMITMENT_NUMBER - 2); - self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); + debug_assert!(self.next_holder_commitment_point.transaction_number() <= INITIAL_COMMITMENT_NUMBER - 2); + self.next_holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); let per_commitment_secret = self.context.holder_signer.as_ref() - .release_commitment_secret(self.holder_commitment_point.transaction_number() + 2).ok(); - if let (HolderCommitmentPoint::Available { current, .. }, Some(per_commitment_secret)) = - (self.holder_commitment_point, per_commitment_secret) { - self.context.signer_pending_revoke_and_ack = false; - return Some(msgs::RevokeAndACK { - channel_id: self.context.channel_id, - per_commitment_secret, - next_per_commitment_point: current, - #[cfg(taproot)] - next_local_nonce: None, - }) + .release_commitment_secret(self.next_holder_commitment_point.transaction_number() + 2).ok(); + if let Some(per_commitment_secret) = per_commitment_secret { + if self.next_holder_commitment_point.can_advance() { + self.context.signer_pending_revoke_and_ack = false; + return Some(msgs::RevokeAndACK { + channel_id: self.context.channel_id, + per_commitment_secret, + next_per_commitment_point: self.next_holder_commitment_point.point(), + #[cfg(taproot)] + next_local_nonce: None, + }) + } } - if !self.holder_commitment_point.is_available() { + if !self.next_holder_commitment_point.can_advance() { log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available", - &self.context.channel_id(), self.holder_commitment_point.transaction_number()); + &self.context.channel_id(), self.next_holder_commitment_point.transaction_number()); } if per_commitment_secret.is_none() { log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment secret for {} is not available", - &self.context.channel_id(), self.holder_commitment_point.transaction_number(), - self.holder_commitment_point.transaction_number() + 2); + &self.context.channel_id(), self.next_holder_commitment_point.transaction_number(), + self.next_holder_commitment_point.transaction_number() + 2); } - // Technically if we're at HolderCommitmentPoint::PendingNext, + // Technically if HolderCommitmentPoint::can_advance is false, // we have a commitment point ready to send in an RAA, however we // choose to wait since if we send RAA now, we could get another // CS before we have any commitment point available. Blocking our // RAA here is a convenient way to make sure that post-funding // we're only ever waiting on one commitment point at a time. log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available", - &self.context.channel_id(), self.holder_commitment_point.transaction_number()); + &self.context.channel_id(), self.next_holder_commitment_point.transaction_number()); self.context.signer_pending_revoke_and_ack = true; None } @@ -8688,7 +8690,7 @@ where return Err(ChannelError::close("Peer sent an invalid channel_reestablish to force close in a non-standard way".to_owned())); } - let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number() - 1; + let our_commitment_transaction = INITIAL_COMMITMENT_NUMBER - self.next_holder_commitment_point.transaction_number() - 1; if msg.next_remote_commitment_number > 0 { let expected_point = self.context.holder_signer.as_ref() .get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.context.secp_ctx) @@ -8790,7 +8792,7 @@ where let is_awaiting_remote_revoke = self.context.channel_state.is_awaiting_remote_revoke(); let next_counterparty_commitment_number = INITIAL_COMMITMENT_NUMBER - self.context.cur_counterparty_commitment_transaction_number + if is_awaiting_remote_revoke { 1 } else { 0 }; - let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number() == 1 { + let channel_ready = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.next_holder_commitment_point.transaction_number() == 1 { // We should never have to worry about MonitorUpdateInProgress resending ChannelReady self.get_channel_ready(logger) } else { None }; @@ -9615,7 +9617,7 @@ where } pub fn get_cur_holder_commitment_transaction_number(&self) -> u64 { - self.holder_commitment_point.transaction_number() + 1 + self.next_holder_commitment_point.transaction_number() + 1 } pub fn get_cur_counterparty_commitment_transaction_number(&self) -> u64 { @@ -9739,7 +9741,7 @@ where debug_assert!(self.context.minimum_depth.unwrap_or(1) > 0); return true; } - if self.holder_commitment_point.transaction_number() == INITIAL_COMMITMENT_NUMBER - 1 && + if self.next_holder_commitment_point.transaction_number() == INITIAL_COMMITMENT_NUMBER - 1 && self.context.cur_counterparty_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 { // If we're a 0-conf channel, we'll move beyond AwaitingChannelReady immediately even while // waiting for the initial monitor persistence. Thus, we check if our commitment @@ -9873,11 +9875,11 @@ where fn get_channel_ready( &mut self, logger: &L ) -> Option where L::Target: Logger { - if let HolderCommitmentPoint::Available { current, .. } = self.holder_commitment_point { + if self.next_holder_commitment_point.can_advance() { self.context.signer_pending_channel_ready = false; Some(msgs::ChannelReady { channel_id: self.context.channel_id(), - next_per_commitment_point: current, + next_per_commitment_point: self.next_holder_commitment_point.point(), short_channel_id_alias: Some(self.context.outbound_scid_alias), }) } else { @@ -10549,8 +10551,8 @@ where assert_ne!(self.context.cur_counterparty_commitment_transaction_number, INITIAL_COMMITMENT_NUMBER); // This is generally the first function which gets called on any given channel once we're // up and running normally. Thus, we take this opportunity to attempt to resolve the - // `holder_commitment_point` to get any keys which we are currently missing. - self.holder_commitment_point.try_resolve_pending( + // `next_holder_commitment_point` to get any keys which we are currently missing. + self.next_holder_commitment_point.try_resolve_pending( &self.context.holder_signer, &self.context.secp_ctx, logger, ); // Prior to static_remotekey, my_current_per_commitment_point was critical to claiming @@ -10580,7 +10582,7 @@ where // next_local_commitment_number is the next commitment_signed number we expect to // receive (indicating if they need to resend one that we missed). - next_local_commitment_number: INITIAL_COMMITMENT_NUMBER - self.holder_commitment_point.transaction_number(), + next_local_commitment_number: INITIAL_COMMITMENT_NUMBER - self.next_holder_commitment_point.transaction_number(), // We have to set next_remote_commitment_number to the next revoke_and_ack we expect to // receive, however we track it by the next commitment number for a remote transaction // (which is one further, as they always revoke previous commitment transaction, not @@ -10606,6 +10608,15 @@ where our_funding_inputs: Vec<(TxIn, Transaction, Weight)>, change_script: Option, funding_feerate_per_kw: u32, locktime: u32, ) -> Result { + if self.current_holder_commitment_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() { @@ -10707,6 +10718,13 @@ where // TODO(splicing): Add check that we are the quiescence acceptor + if self.current_holder_commitment_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!( @@ -11854,7 +11872,7 @@ where )?; let unfunded_context = UnfundedChannelContext { unfunded_channel_age_ticks: 0, - holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), + initial_holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), }; // We initialize `signer_pending_open_channel` to false, and leave setting the flag @@ -11985,10 +12003,10 @@ where panic!("Tried to send an open_channel for a channel that has already advanced"); } - let first_per_commitment_point = match self.unfunded_context.holder_commitment_point { - Some(holder_commitment_point) if holder_commitment_point.is_available() => { + let first_per_commitment_point = match self.unfunded_context.initial_holder_commitment_point { + Some(holder_commitment_point) if holder_commitment_point.can_advance() => { self.signer_pending_open_channel = false; - holder_commitment_point.current_point() + holder_commitment_point.point() }, _ => { log_trace!(_logger, "Unable to generate open_channel message, waiting for commitment point"); @@ -12056,15 +12074,15 @@ where if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(_)) { return Err((self, ChannelError::close("Received funding_signed in strange state!".to_owned()))); } - let mut holder_commitment_point = match self.unfunded_context.holder_commitment_point { + let initial_holder_commitment_point = match self.unfunded_context.initial_holder_commitment_point { Some(point) => point, None => return Err((self, ChannelError::close("Received funding_signed before our first commitment point was available".to_owned()))), }; - self.context.assert_no_commitment_advancement(holder_commitment_point.transaction_number(), "funding_signed"); + self.context.assert_no_commitment_advancement(initial_holder_commitment_point.transaction_number(), "funding_signed"); - let (channel_monitor, _) = match self.initial_commitment_signed( + let (channel_monitor, _, next_holder_commitment_point) = match self.initial_commitment_signed( self.context.channel_id(), msg.signature, - &mut holder_commitment_point, best_block, signer_provider, logger + &initial_holder_commitment_point, best_block, signer_provider, logger ) { Ok(channel_monitor) => channel_monitor, Err(err) => return Err((self, err)), @@ -12077,7 +12095,8 @@ where pending_funding: vec![], context: self.context, interactive_tx_signing_session: None, - holder_commitment_point, + current_holder_commitment_point: Some(initial_holder_commitment_point), + next_holder_commitment_point, #[cfg(splicing)] pending_splice: None, }; @@ -12096,11 +12115,11 @@ where ) -> (Option, Option) where L::Target: Logger { // If we were pending a commitment point, retry the signer and advance to an // available state. - if self.unfunded_context.holder_commitment_point.is_none() { - self.unfunded_context.holder_commitment_point = HolderCommitmentPoint::new(&self.context.holder_signer, &self.context.secp_ctx); + if self.unfunded_context.initial_holder_commitment_point.is_none() { + self.unfunded_context.initial_holder_commitment_point = HolderCommitmentPoint::new(&self.context.holder_signer, &self.context.secp_ctx); } - if let Some(ref mut point) = self.unfunded_context.holder_commitment_point { - if !point.is_available() { + if let Some(ref mut point) = self.unfunded_context.initial_holder_commitment_point { + if !point.can_advance() { point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); } } @@ -12220,7 +12239,7 @@ where )?; let unfunded_context = UnfundedChannelContext { unfunded_channel_age_ticks: 0, - holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), + initial_holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), }; let chan = Self { funding, context, unfunded_context, signer_pending_accept_channel: false }; Ok(chan) @@ -12259,10 +12278,10 @@ where fn generate_accept_channel_message( &mut self, _logger: &L ) -> Option where L::Target: Logger { - let first_per_commitment_point = match self.unfunded_context.holder_commitment_point { - Some(holder_commitment_point) if holder_commitment_point.is_available() => { + let first_per_commitment_point = match self.unfunded_context.initial_holder_commitment_point { + Some(holder_commitment_point) if holder_commitment_point.can_advance() => { self.signer_pending_accept_channel = false; - holder_commitment_point.current_point() + holder_commitment_point.point() }, _ => { log_trace!(_logger, "Unable to generate accept_channel message, waiting for commitment point"); @@ -12332,18 +12351,18 @@ where // channel. return Err((self, ChannelError::close("Received funding_created after we got the channel!".to_owned()))); } - let mut holder_commitment_point = match self.unfunded_context.holder_commitment_point { + let initial_holder_commitment_point = match self.unfunded_context.initial_holder_commitment_point { Some(point) => point, None => return Err((self, ChannelError::close("Received funding_created before our first commitment point was available".to_owned()))), }; - self.context.assert_no_commitment_advancement(holder_commitment_point.transaction_number(), "funding_created"); + self.context.assert_no_commitment_advancement(initial_holder_commitment_point.transaction_number(), "funding_created"); let funding_txo = OutPoint { txid: msg.funding_txid, index: msg.funding_output_index }; self.funding.channel_transaction_parameters.funding_outpoint = Some(funding_txo); - let (channel_monitor, counterparty_initial_commitment_tx) = match self.initial_commitment_signed( + let (channel_monitor, counterparty_initial_commitment_tx, next_holder_commitment_point) = match self.initial_commitment_signed( ChannelId::v1_from_funding_outpoint(funding_txo), msg.signature, - &mut holder_commitment_point, best_block, signer_provider, logger + &initial_holder_commitment_point, best_block, signer_provider, logger ) { Ok(channel_monitor) => channel_monitor, Err(err) => return Err((self, err)), @@ -12363,7 +12382,8 @@ where pending_funding: vec![], context: self.context, interactive_tx_signing_session: None, - holder_commitment_point, + current_holder_commitment_point: Some(initial_holder_commitment_point), + next_holder_commitment_point, #[cfg(splicing)] pending_splice: None, }; @@ -12380,11 +12400,11 @@ where pub fn signer_maybe_unblocked( &mut self, logger: &L ) -> Option where L::Target: Logger { - if self.unfunded_context.holder_commitment_point.is_none() { - self.unfunded_context.holder_commitment_point = HolderCommitmentPoint::new(&self.context.holder_signer, &self.context.secp_ctx); + if self.unfunded_context.initial_holder_commitment_point.is_none() { + self.unfunded_context.initial_holder_commitment_point = HolderCommitmentPoint::new(&self.context.holder_signer, &self.context.secp_ctx); } - if let Some(ref mut point) = self.unfunded_context.holder_commitment_point { - if !point.is_available() { + if let Some(ref mut point) = self.unfunded_context.initial_holder_commitment_point { + if !point.can_advance() { point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); } } @@ -12464,7 +12484,7 @@ where )?; let unfunded_context = UnfundedChannelContext { unfunded_channel_age_ticks: 0, - holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), + initial_holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), }; let funding_negotiation_context = FundingNegotiationContext { is_initiator: true, @@ -12659,7 +12679,7 @@ where let unfunded_context = UnfundedChannelContext { unfunded_channel_age_ticks: 0, - holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), + initial_holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), }; Ok(Self { funding, @@ -12891,7 +12911,7 @@ where } self.context.destination_script.write(writer)?; - self.holder_commitment_point.transaction_number().write(writer)?; + self.next_holder_commitment_point.transaction_number().write(writer)?; self.context.cur_counterparty_commitment_transaction_number.write(writer)?; self.funding.value_to_self_msat.write(writer)?; @@ -13222,9 +13242,11 @@ where } let is_manual_broadcast = Some(self.context.is_manual_broadcast); - // `current_point` will become optional when async signing is implemented. - let cur_holder_commitment_point = Some(self.holder_commitment_point.current_point()); - let next_holder_commitment_point = self.holder_commitment_point.next_point(); + let current_holder_commitment_point = + self.current_holder_commitment_point.map(|p| p.point()); + let next_holder_commitment_point = self.next_holder_commitment_point.point(); + let next_holder_commitment_point_next_advance = + self.next_holder_commitment_point.next_point(); write_tlv_fields!(writer, { (0, self.context.announcement_sigs, option), @@ -13262,8 +13284,8 @@ where (39, pending_outbound_blinding_points, optional_vec), (41, holding_cell_blinding_points, optional_vec), (43, malformed_htlcs, optional_vec), // Added in 0.0.119 - (45, cur_holder_commitment_point, option), - (47, next_holder_commitment_point, option), + (45, next_holder_commitment_point, required), + (47, next_holder_commitment_point_next_advance, option), (49, self.context.local_initiated_shutdown, option), // Added in 0.0.122 (51, is_manual_broadcast, option), // Added in 0.0.124 (53, funding_tx_broadcast_safe_event_emitted, option), // Added in 0.0.124 @@ -13274,6 +13296,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, current_holder_commitment_point, option), // Added in 0.2 }); Ok(()) @@ -13320,7 +13343,7 @@ where }; let destination_script = Readable::read(reader)?; - let cur_holder_commitment_transaction_number = Readable::read(reader)?; + let next_holder_commitment_transaction_number = Readable::read(reader)?; let cur_counterparty_commitment_transaction_number = Readable::read(reader)?; let value_to_self_msat = Readable::read(reader)?; @@ -13623,8 +13646,9 @@ where let mut malformed_htlcs: Option> = None; let mut monitor_pending_update_adds: Option> = None; - let mut cur_holder_commitment_point_opt: Option = None; + let mut current_holder_commitment_point_opt: Option = None; let mut next_holder_commitment_point_opt: Option = None; + let mut next_holder_commitment_point_next_advance_opt: Option = None; let mut is_manual_broadcast = None; let mut pending_funding = Some(Vec::new()); @@ -13664,8 +13688,8 @@ where (39, pending_outbound_blinding_points_opt, optional_vec), (41, holding_cell_blinding_points_opt, optional_vec), (43, malformed_htlcs, optional_vec), // Added in 0.0.119 - (45, cur_holder_commitment_point_opt, option), - (47, next_holder_commitment_point_opt, option), + (45, next_holder_commitment_point_opt, option), + (47, next_holder_commitment_point_next_advance_opt, option), (49, local_initiated_shutdown, option), (51, is_manual_broadcast, option), (53, funding_tx_broadcast_safe_event_emitted, option), @@ -13676,6 +13700,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, current_holder_commitment_point_opt, option), // Added in 0.2 }); let holder_signer = signer_provider.derive_channel_signer(channel_keys_id); @@ -13853,38 +13878,42 @@ where // If we're restoring this channel for the first time after an upgrade, then we require that the // signer be available so that we can immediately populate the current commitment point. Channel // restoration will fail if this is not possible. - let holder_commitment_point = match ( - cur_holder_commitment_point_opt, + let next_holder_commitment_point = match ( next_holder_commitment_point_opt, + next_holder_commitment_point_next_advance_opt, ) { - (Some(current), Some(next)) => HolderCommitmentPoint::Available { - transaction_number: cur_holder_commitment_transaction_number, - current, - next, - }, - (Some(current), _) => HolderCommitmentPoint::PendingNext { - transaction_number: cur_holder_commitment_transaction_number, - current, + (Some(point), next_point) => HolderCommitmentPoint { + transaction_number: next_holder_commitment_transaction_number, + point, + next_point, }, (_, _) => { - let current = holder_signer.get_per_commitment_point(cur_holder_commitment_transaction_number, &secp_ctx) - .expect("Must be able to derive the current commitment point upon channel restoration"); - let next = holder_signer + let point = holder_signer.get_per_commitment_point(next_holder_commitment_transaction_number, &secp_ctx) + .expect("Must be able to derive the current commitment point upon channel restoration"); + let next_point = holder_signer .get_per_commitment_point( - cur_holder_commitment_transaction_number - 1, + next_holder_commitment_transaction_number - 1, &secp_ctx, ) .expect( "Must be able to derive the next commitment point upon channel restoration", ); - HolderCommitmentPoint::Available { - transaction_number: cur_holder_commitment_transaction_number, - current, - next, + HolderCommitmentPoint { + transaction_number: next_holder_commitment_transaction_number, + point, + next_point: Some(next_point), } }, }; + let current_holder_commitment_point = + current_holder_commitment_point_opt.map(|point| HolderCommitmentPoint { + transaction_number: next_holder_commitment_point.transaction_number() + 1, + point, + // Not needed since current_holder_commitment_point is never advanced + next_point: None, + }); + Ok(FundedChannel { funding: FundingScope { value_to_self_msat, @@ -14022,7 +14051,8 @@ where is_holder_quiescence_initiator: None, }, interactive_tx_signing_session, - holder_commitment_point, + current_holder_commitment_point, + next_holder_commitment_point, #[cfg(splicing)] pending_splice: None, })