From 9c2a050a35831565e1be895b0967076dbf9e83f8 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Tue, 18 Jun 2024 14:48:23 -0700 Subject: [PATCH] Return an error if we fail to advance our commitment number --- lightning/src/ln/channel.rs | 41 ++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ccba85c60..3f7d01bb8 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1008,7 +1008,9 @@ impl HolderCommitmentPoint { } /// If we are not pending the next commitment point, this method advances the commitment number - /// and requests the next commitment point from the signer. + /// 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). /// /// 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` @@ -1020,7 +1022,9 @@ impl HolderCommitmentPoint { /// This method is used for the following transitions: /// - `Available` -> `PendingNext` /// - `Available` -> `PendingNext` -> `Available` (in one fell swoop) - pub fn advance(&mut self, signer: &ChannelSignerType, secp_ctx: &Secp256k1, logger: &L) + pub fn advance( + &mut self, signer: &ChannelSignerType, secp_ctx: &Secp256k1, logger: &L + ) -> Result<(), ()> where SP::Target: SignerProvider, L::Target: Logger { if let HolderCommitmentPoint::Available { transaction_number, next, .. } = self { @@ -1029,7 +1033,9 @@ impl HolderCommitmentPoint { current: *next, }; self.try_resolve_pending(signer, secp_ctx, logger); + return Ok(()); } + Err(()) } } @@ -4618,7 +4624,18 @@ impl Channel where channel_id: Some(self.context.channel_id()), }; - self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger); + if self.context.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 above. + 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())); + } self.context.expecting_peer_commitment_signed = false; // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call // build_commitment_no_status_check() next which will reset this to RAAFirst. @@ -7799,7 +7816,14 @@ impl OutboundV1Channel where SP::Target: SignerProvider { } else { self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); } - self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger); + if self.context.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. + // We cannot send open_channel before this has occurred, so if we + // err here by the time we receive funding_signed, something has gone wrong. + debug_assert!(false, "We should be ready to advance our commitment point by the time we receive funding_signed"); + return Err((self, ChannelError::close("Failed to advance holder commitment point".to_owned()))); + } self.context.cur_counterparty_commitment_transaction_number -= 1; log_info!(logger, "Received funding_signed from peer for channel {}", &self.context.channel_id()); @@ -8066,7 +8090,14 @@ impl InboundV1Channel where SP::Target: SignerProvider { self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); self.context.channel_id = ChannelId::v1_from_funding_outpoint(funding_txo); self.context.cur_counterparty_commitment_transaction_number -= 1; - self.context.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger); + if self.context.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. + // We cannot send accept_channel before this has occurred, so if we + // err here by the time we receive funding_created, something has gone wrong. + debug_assert!(false, "We should be ready to advance our commitment point by the time we receive funding_created"); + return Err((self, ChannelError::close("Failed to advance holder commitment point".to_owned()))); + } let (counterparty_initial_commitment_tx, funding_signed) = self.context.get_funding_signed_msg(logger); -- 2.39.5