Fix commitment signed

When monitor updating is restored, we may have a situation where the signer
still has not returned a signature for the counterparty commitment. If this is
the case, note that the _signer_ is still pending a commitment update event
though the _monitor_ is not.

Similarly, only allow the revoke-and-ack to be generated in the case that
we're not still pending the counterparty commitment signature. If the monitor
was pending a revoke-and-ack and the commitment update is not available, then
note that _signer_ is still pending a revoke-and-ack.

Ensure that we honor the ordering specified by the channel.

Author: waterson

lightning/src/ln/async_signer_tests.rs

@@ -344,7 +344,7 @@ fn test_async_commitment_signature_for_peer_disconnect() {
 	dst.node.peer_disconnected(&src.node.get_our_node_id());
 	let mut reconnect_args = ReconnectArgs::new(&nodes[0], &nodes[1]);
 	reconnect_args.send_channel_ready = (false, false);
-	reconnect_args.pending_raa = (true, false);
+	reconnect_args.pending_raa = (false, false);
 	reconnect_nodes(reconnect_args);
 
 	// Mark dst's signer as available and retry: we now expect to see dst's `commitment_signed`.

lightning/src/ln/channel.rs

@@ -518,6 +518,8 @@ pub(super) struct MonitorRestoreUpdates {
 /// The return value of `signer_maybe_unblocked`
 pub(super) struct SignerResumeUpdates {
 	pub commitment_update: Option<msgs::CommitmentUpdate>,
+	pub raa: Option<msgs::RevokeAndACK>,
+	pub order: RAACommitmentOrder,
 	pub funding_signed: Option<msgs::FundingSigned>,
 	pub funding_created: Option<msgs::FundingCreated>,
 	pub channel_ready: Option<msgs::ChannelReady>,
@@ -744,6 +746,9 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
 	/// This flag is set in such a case. Note that we don't need to persist this as we'll end up
 	/// setting it again as a side-effect of [`Channel::channel_reestablish`].
 	signer_pending_commitment_update: bool,
+	/// Similar to [`Self::signer_pending_commitment_update`]: indicates that we've deferred sending a
+	/// `revoke_and_ack`, and should do so once the signer has become unblocked.
+	signer_pending_revoke_and_ack: bool,
 	/// Similar to [`Self::signer_pending_commitment_update`] but we're waiting to send either a
 	/// [`msgs::FundingCreated`] or [`msgs::FundingSigned`] depending on if this channel is
 	/// outbound or inbound.
@@ -3143,6 +3148,7 @@ impl<SP: Deref> Channel<SP> where
 		self.context.cur_holder_commitment_transaction_number -= 1;
 		// Note that if we need_commitment & !AwaitingRemoteRevoke we'll call
 		// build_commitment_no_status_check() next which will reset this to RAAFirst.
+		log_debug!(logger, "setting resend_order to CommitmentFirst");
 		self.context.resend_order = RAACommitmentOrder::CommitmentFirst;
 
 		if (self.context.channel_state & ChannelState::MonitorUpdateInProgress as u32) != 0 {
@@ -3160,8 +3166,8 @@ impl<SP: Deref> Channel<SP> where
 				self.context.latest_monitor_update_id = monitor_update.update_id;
 				monitor_update.updates.append(&mut additional_update.updates);
 			}
-			log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updated HTLC state but awaiting a monitor update resolution to reply.",
-				&self.context.channel_id);
+			log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updated HTLC state and set sequence to {} but awaiting a monitor update resolution to reply.",
+				&self.context.channel_id, self.context.cur_holder_commitment_transaction_number);
 			return Ok(self.push_ret_blockable_mon_update(monitor_update));
 		}
 
@@ -3177,8 +3183,8 @@ impl<SP: Deref> Channel<SP> where
 			true
 		} else { false };
 
-		log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updating HTLC state and responding with{} a revoke_and_ack.",
-			&self.context.channel_id(), if need_commitment_signed { " our own commitment_signed and" } else { "" });
+		log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updating HTLC state and set sequence to {}; responding with{} a revoke_and_ack.",
+			&self.context.channel_id(), self.context.cur_holder_commitment_transaction_number, if need_commitment_signed { " our own commitment_signed and" } else { "" });
 		self.monitor_updating_paused(true, need_commitment_signed, false, Vec::new(), Vec::new(), Vec::new());
 		return Ok(self.push_ret_blockable_mon_update(monitor_update));
 	}
@@ -3828,7 +3834,7 @@ impl<SP: Deref> Channel<SP> where
 		}
 
 		let raa = if self.context.monitor_pending_revoke_and_ack {
-			Some(self.get_last_revoke_and_ack())
+			self.get_last_revoke_and_ack(logger)
 		} else { None };
 		let commitment_update = if self.context.monitor_pending_commitment_signed {
 			self.get_last_commitment_update_for_send(logger).ok()
@@ -3837,13 +3843,25 @@ impl<SP: Deref> Channel<SP> where
 			self.mark_awaiting_response();
 		}
 
+		if self.context.monitor_pending_commitment_signed && commitment_update.is_none() {
+			log_debug!(logger, "Monitor was pending_commitment_signed with no commitment update available; setting signer_pending_commitment_update = true");
+			self.context.signer_pending_commitment_update = true;
+		}
+		if self.context.monitor_pending_revoke_and_ack && raa.is_none() {
+			log_debug!(logger, "Monitor was pending_revoke_and_ack with no RAA available; setting signer_pending_revoke_and_ack = true");
+			self.context.signer_pending_revoke_and_ack = true;
+		}
+
 		self.context.monitor_pending_revoke_and_ack = false;
 		self.context.monitor_pending_commitment_signed = false;
+
 		let order = self.context.resend_order.clone();
-		log_debug!(logger, "Restored monitor updating in channel {} resulting in {}{} commitment update and {} RAA, with {} first",
+		log_debug!(logger, "Restored monitor updating in channel {} resulting in {}{} commitment update and {} RAA{}",
 			&self.context.channel_id(), if funding_broadcastable.is_some() { "a funding broadcastable, " } else { "" },
 			if commitment_update.is_some() { "a" } else { "no" }, if raa.is_some() { "an" } else { "no" },
-			match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
+			if commitment_update.is_some() && raa.is_some() {
+				match order { RAACommitmentOrder::CommitmentFirst => ", with commitment first", RAACommitmentOrder::RevokeAndACKFirst => ", with RAA first"}
+			} else { "" });
 		MonitorRestoreUpdates {
 			raa, commitment_update, order, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, funding_broadcastable, channel_ready, announcement_sigs
 		}
@@ -3890,6 +3908,9 @@ impl<SP: Deref> Channel<SP> where
 		let commitment_update = if self.context.signer_pending_commitment_update {
 			self.get_last_commitment_update_for_send(logger).ok()
 		} else { None };
+		let raa = if self.context.signer_pending_revoke_and_ack {
+			self.get_last_revoke_and_ack(logger)
+		} else { None };
 		let funding_signed = if self.context.signer_pending_funding && !self.context.is_outbound() {
 			self.context.get_funding_signed_msg(logger).1
 		} else { None };
@@ -3899,24 +3920,48 @@ impl<SP: Deref> Channel<SP> where
 		let funding_created = if self.context.signer_pending_funding && self.context.is_outbound() {
 			self.context.get_funding_created_msg(logger)
 		} else { None };
+		let order = self.context.resend_order.clone();
+
+		log_debug!(logger, "Signing unblocked in channel {} at sequence {} resulting in {} commitment update, {} RAA{}, {} funding signed, and {} funding created",
+			&self.context.channel_id(), self.context.cur_holder_commitment_transaction_number,
+			if commitment_update.is_some() { "a" } else { "no" },
+			if raa.is_some() { "an" } else { "no" },
+			if commitment_update.is_some() && raa.is_some() {
+				if order == RAACommitmentOrder::CommitmentFirst { " (commitment first)" } else { " (RAA first)" }
+			} else { "" },
+			if funding_signed.is_some() { "a" } else { "no" },
+			if funding_created.is_some() { "a" } else { "no" });
+
 		SignerResumeUpdates {
 			commitment_update,
+			raa,
+			order,
 			funding_signed,
 			funding_created,
 			channel_ready,
 		}
 	}
 
-	fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
+	fn get_last_revoke_and_ack<L: Deref>(&mut self, logger: &L) -> Option<msgs::RevokeAndACK> where L::Target: Logger {
+		if self.context.signer_pending_commitment_update {
+			log_debug!(logger, "Can't generate revoke-and-ack in channel {} while pending commitment update",
+				self.context.channel_id());
+			return None;
+		}
+
+		log_debug!(logger, "Regenerated last revoke-and-ack in channel {} for next per-commitment point sequence number {}, releasing secret for {}",
+			&self.context.channel_id(), self.context.cur_holder_commitment_transaction_number,
+			self.context.cur_holder_commitment_transaction_number + 2);
+		self.context.signer_pending_revoke_and_ack = false;
 		let next_per_commitment_point = self.context.holder_signer.as_ref().get_per_commitment_point(self.context.cur_holder_commitment_transaction_number, &self.context.secp_ctx);
 		let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.cur_holder_commitment_transaction_number + 2);
-		msgs::RevokeAndACK {
+		Some(msgs::RevokeAndACK {
 			channel_id: self.context.channel_id,
 			per_commitment_secret,
 			next_per_commitment_point,
 			#[cfg(taproot)]
 			next_local_nonce: None,
-		}
+		})
 	}
 
 	/// Gets the last commitment update for immediate sending to our peer.
@@ -3976,8 +4021,8 @@ impl<SP: Deref> Channel<SP> where
 			})
 		} else { None };
 
-		log_trace!(logger, "Regenerated latest commitment update in channel {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds",
-				&self.context.channel_id(), if update_fee.is_some() { " update_fee," } else { "" },
+		log_debug!(logger, "Regenerated latest commitment update in channel {} at {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds",
+				&self.context.channel_id(), self.context.cur_holder_commitment_transaction_number, if update_fee.is_some() { " update_fee," } else { "" },
 				update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len(), update_fail_malformed_htlcs.len());
 		let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger).map(|(cu, _)| cu) {
 			self.context.signer_pending_commitment_update = false;
@@ -4112,7 +4157,7 @@ impl<SP: Deref> Channel<SP> where
 				self.context.monitor_pending_revoke_and_ack = true;
 				None
 			} else {
-				Some(self.get_last_revoke_and_ack())
+				self.get_last_revoke_and_ack(logger)
 			}
 		} else {
 			return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old local commitment transaction".to_owned()));
@@ -5447,6 +5492,7 @@ impl<SP: Deref> Channel<SP> where
 				self.context.pending_update_fee = None;
 			}
 		}
+		log_debug!(logger, "setting resend_order to RevokeAndACKFirst");
 		self.context.resend_order = RAACommitmentOrder::RevokeAndACKFirst;
 
 		let (mut htlcs_ref, counterparty_commitment_tx) =
@@ -5841,6 +5887,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 				monitor_pending_finalized_fulfills: Vec::new(),
 
 				signer_pending_commitment_update: false,
+				signer_pending_revoke_and_ack: false,
 				signer_pending_funding: false,
 
 				#[cfg(debug_assertions)]
@@ -6463,6 +6510,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 				monitor_pending_finalized_fulfills: Vec::new(),
 
 				signer_pending_commitment_update: false,
+				signer_pending_revoke_and_ack: false,
 				signer_pending_funding: false,
 
 				#[cfg(debug_assertions)]
@@ -7531,6 +7579,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 				monitor_pending_finalized_fulfills: monitor_pending_finalized_fulfills.unwrap(),
 
 				signer_pending_commitment_update: false,
+				signer_pending_revoke_and_ack: false,
 				signer_pending_funding: false,
 
 				pending_update_fee,

lightning/src/ln/channelmanager.rs

@@ -6803,12 +6803,19 @@ where
 			let node_id = phase.context().get_counterparty_node_id();
 			if let ChannelPhase::Funded(chan) = phase {
 				let msgs = chan.signer_maybe_unblocked(&self.logger);
-				if let Some(updates) = msgs.commitment_update {
-					pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-						node_id,
-						updates,
-					});
-				}
+				match (msgs.commitment_update, msgs.raa) {
+					(Some(cu), Some(raa)) if msgs.order == RAACommitmentOrder::CommitmentFirst => {
+						pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu });
+						pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa });
+					},
+					(Some(cu), Some(raa)) if msgs.order == RAACommitmentOrder::RevokeAndACKFirst => {
+						pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa });
+						pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu });
+					},
+					(Some(cu), _) => pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs { node_id, updates: cu }),
+					(_, Some(raa)) => pending_msg_events.push(events::MessageSendEvent::SendRevokeAndACK { node_id, msg: raa }),
+					(_, _) => (),
+				};
 				if let Some(msg) = msgs.funding_signed {
 					pending_msg_events.push(events::MessageSendEvent::SendFundingSigned {
 						node_id,