From: Alec Chen <alecchendev@gmail.com>
Date: Tue, 18 Jun 2024 21:48:23 +0000 (-0700)
Subject: Return an error if we fail to advance our commitment number
X-Git-Tag: v0.0.124-beta~49^2~4
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=9c2a050a35831565e1be895b0967076dbf9e83f8;p=rust-lightning

Return an error if we fail to advance our commitment number
---

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<SP: Deref, L: Deref>(&mut self, signer: &ChannelSignerType<SP>, secp_ctx: &Secp256k1<secp256k1::All>, logger: &L)
+	pub fn advance<SP: Deref, L: Deref>(
+		&mut self, signer: &ChannelSignerType<SP>, secp_ctx: &Secp256k1<secp256k1::All>, 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<SP: Deref> Channel<SP> 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<SP: Deref> OutboundV1Channel<SP> 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<SP: Deref> InboundV1Channel<SP> 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);