Use struct for HolderCommitmentPoint

The only difference between the two variants is the next point, which
can be stored using an Option for simplicity. The naming of the
Available variant is also confusing as it refers to the next commitment
point. But HolderCommitmentPoint is typically used to represent the next
point, which is actually stored in the current field. Drop the "current"
nomenclature to avoid confusion.

Author: jkczyz

lightning/src/ln/channel.rs

@@ -1249,114 +1249,91 @@ 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<PublicKey>,
 }
 
 impl HolderCommitmentPoint {
 	#[rustfmt::skip]
 	pub fn new<SP: Deref>(signer: &ChannelSignerType<SP>, secp_ctx: &Secp256k1<secp256k1::All>) -> Option<Self>
 		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<PublicKey> {
-		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<SP: Deref, L: Deref>(
 		&mut self, signer: &ChannelSignerType<SP>, secp_ctx: &Secp256k1<secp256k1::All>, 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).
-	///
-	/// 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`.
+	/// and requests the next commitment point from the signer. Returns `Ok` if we were able to
+	/// advance our commitment number (even if we are still pending the next commitment point).
 	///
-	/// If our signer is ready to provide the next commitment point, we will advance all the
-	/// way to `Available`.
+	/// If our signer is not ready to provide the next commitment point, we will advance but won't
+	/// be able to advance again immediately. Instead, this hould be tried again later in
+	/// `signer_unblocked` via `try_resolve_pending`.
 	///
-	/// This method is used for the following transitions:
-	/// - `Available` -> `PendingNext`
-	/// - `Available` -> `PendingNext` -> `Available` (in one fell swoop)
+	/// If our signer is ready to provide the next commitment point, the next call to `advance` will
+	/// succeed.
 	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 {
-			*self = HolderCommitmentPoint::PendingNext {
-				transaction_number: *transaction_number - 1,
-				current: *next,
+		if let Some(next_point) = self.next_point {
+			*self = Self {
+				transaction_number: self.transaction_number - 1,
+				point: next_point,
+				next_point: None,
 			};
+
 			self.try_resolve_pending(signer, secp_ctx, logger);
 			return Ok(());
 		}
@@ -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();
@@ -4218,7 +4195,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();
@@ -8412,7 +8389,7 @@ where
 	/// blocked.
 	#[rustfmt::skip]
 	pub fn signer_maybe_unblocked<L: Deref>(&mut self, logger: &L) -> SignerResumeUpdates where L::Target: Logger {
-		if !self.holder_commitment_point.is_available() {
+		if !self.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);
 		}
@@ -8513,18 +8490,19 @@ where
 		self.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,
-			})
+		if let Some(per_commitment_secret) = per_commitment_secret {
+			if self.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.holder_commitment_point.point(),
+					#[cfg(taproot)]
+					next_local_nonce: None,
+				})
+			}
 		}
-		if !self.holder_commitment_point.is_available() {
+		if !self.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());
 		}
@@ -8533,7 +8511,7 @@ where
 				&self.context.channel_id(), self.holder_commitment_point.transaction_number(),
 				self.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
@@ -9873,11 +9851,11 @@ where
 	fn get_channel_ready<L: Deref>(
 		&mut self, logger: &L
 	) -> Option<msgs::ChannelReady> where L::Target: Logger {
-		if let HolderCommitmentPoint::Available { current, .. } = self.holder_commitment_point {
+		if self.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.holder_commitment_point.point(),
 				short_channel_id_alias: Some(self.context.outbound_scid_alias),
 			})
 		} else {
@@ -11986,9 +11964,9 @@ where
 		}
 
 		let first_per_commitment_point = match self.unfunded_context.holder_commitment_point {
-			Some(holder_commitment_point) if holder_commitment_point.is_available() => {
+			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");
@@ -12100,7 +12078,7 @@ where
 			self.unfunded_context.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 !point.can_advance() {
 				point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
 			}
 		}
@@ -12260,9 +12238,9 @@ where
 		&mut self, _logger: &L
 	) -> Option<msgs::AcceptChannel> 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() => {
+			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");
@@ -12384,7 +12362,7 @@ where
 			self.unfunded_context.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 !point.can_advance() {
 				point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
 			}
 		}
@@ -13222,9 +13200,9 @@ 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();
+		// `HolderCommitmentPoint::point` will become optional when async signing is implemented.
+		let holder_commitment_point = Some(self.holder_commitment_point.point());
+		let holder_commitment_point_next_advance = self.holder_commitment_point.next_point();
 
 		write_tlv_fields!(writer, {
 			(0, self.context.announcement_sigs, option),
@@ -13262,8 +13240,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, holder_commitment_point, option),
+			(47, 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
@@ -13320,7 +13298,7 @@ where
 		};
 		let destination_script = Readable::read(reader)?;
 
-		let cur_holder_commitment_transaction_number = Readable::read(reader)?;
+		let 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 +13601,8 @@ 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 cur_holder_commitment_point_opt: Option<PublicKey> = None;
-		let mut next_holder_commitment_point_opt: Option<PublicKey> = None;
+		let mut holder_commitment_point_opt: Option<PublicKey> = None;
+		let mut holder_commitment_point_next_advance_opt: Option<PublicKey> = None;
 		let mut is_manual_broadcast = None;
 
 		let mut pending_funding = Some(Vec::new());
@@ -13664,8 +13642,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, holder_commitment_point_opt, option),
+			(47, 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),
@@ -13853,37 +13831,31 @@ 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,
-			next_holder_commitment_point_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,
-			},
-			(_, _) => {
-				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
-					.get_per_commitment_point(
-						cur_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,
-				}
-			},
-		};
+		let holder_commitment_point =
+			match (holder_commitment_point_opt, holder_commitment_point_next_advance_opt) {
+				(Some(point), next_point) => HolderCommitmentPoint {
+					transaction_number: holder_commitment_transaction_number,
+					point,
+					next_point,
+				},
+				(_, _) => {
+					let point = holder_signer.get_per_commitment_point(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(
+							holder_commitment_transaction_number - 1,
+							&secp_ctx,
+						)
+						.expect(
+							"Must be able to derive the next commitment point upon channel restoration",
+						);
+					HolderCommitmentPoint {
+						transaction_number: holder_commitment_transaction_number,
+						point,
+						next_point: Some(next_point),
+					}
+				},
+			};
 
 		Ok(FundedChannel {
 			funding: FundingScope {