Honor ordering when resuming monitor

When we resume the monitor, ensure that we provide the commitment update and
revoke-and-ack messages in the order that our counterparty expects them. With
an asynchronous signer, it's possible that the message may become available in
an order other than what's expected.

Author: waterson

lightning/src/ln/async_signer_tests.rs

@@ -786,12 +786,62 @@ fn channel_update_fee_test() {
 
 	alice.node.timer_tick_occurred();
 
+	assert!(alice.node.get_and_clear_pending_msg_events().is_empty());
+
+	alice.set_channel_signer_ops_available(
+		&bob.node.get_our_node_id(), &chan_id, ops::GET_PER_COMMITMENT_POINT, true);
+	alice.node.signer_unblocked(None);
+
 	let events = alice.node.get_and_clear_pending_msg_events();
-	assert_eq!(events.len(), 1);
-	let (_update_msg, _commitment_signed) = match events[0] { // (1)
-		MessageSendEvent::UpdateHTLCs { updates: msgs::CommitmentUpdate { ref update_fee, ref commitment_signed, .. }, .. } => {
-			(update_fee.as_ref(), commitment_signed)
-		},
-		_ => panic!("Unexpected event"),
+	assert_eq!(events.len(), 2);
+	match (&events[0], &events[1]) {
+		(MessageSendEvent::SendRevokeAndACK { .. }, MessageSendEvent::UpdateHTLCs { .. }) => {
+			// TODO(waterson) we'd kind of expect to see an update_fee here, but we actually don't because
+			// signer_maybe_unblocked doesn't create that. It probably should.
+		}
+		(a, b) => {
+			panic!("Expected SendRevokeAndACK and UpdateHTLCs, got {:?} and {:?}", a, b);
+		}
+	}
+}
+
+#[test]
+fn monitor_honors_commitment_raa_order() {
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+	let (_, _, chan_id, _) = create_announced_chan_between_nodes(&nodes, 0, 1);
+
+	let alice = &nodes[0];
+	let bob = &nodes[1];
+
+	let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(alice, bob, 8_000_000);
+
+	alice.node.send_payment_with_route(&route, payment_hash,
+		RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)).unwrap();
+	check_added_monitors!(alice, 1);
+
+	let payment_event = {
+		let mut events = alice.node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		SendEvent::from_event(events.remove(0))
 	};
+
+	// Make the commitment secret be unavailable. The expectation here is that Bob should send the
+	// revoke-and-ack first. So even though he can generate a commitment update, he should hold onto
+	// that until he's ready to revoke.
+	bob.set_channel_signer_ops_available(&alice.node.get_our_node_id(),  &chan_id, ops::RELEASE_COMMITMENT_SECRET, false);
+
+	bob.node.handle_update_add_htlc(&alice.node.get_our_node_id(), &payment_event.msgs[0]);
+	bob.node.handle_commitment_signed(&alice.node.get_our_node_id(), &payment_event.commitment_msg);
+	check_added_monitors(bob, 1);
+
+	assert!(bob.node.get_and_clear_pending_msg_events().is_empty());
+
+	// Now make the commitment secret available and restart the channel.
+	bob.set_channel_signer_ops_available(&alice.node.get_our_node_id(),  &chan_id, ops::RELEASE_COMMITMENT_SECRET, true);
+	bob.node.signer_unblocked(None);
+
+	get_revoke_commit_msgs(bob, &alice.node.get_our_node_id());
 }

lightning/src/ln/channel.rs

@@ -4107,7 +4107,29 @@ impl<SP: Deref> Channel<SP> where
 		self.context.monitor_pending_revoke_and_ack = false;
 		self.context.monitor_pending_commitment_signed = false;
 
+		// Enforce the ordering between commitment update and revoke-and-ack: with an asynchronous
+		// signer, they may have become available in an order that violates the ordering that our
+		// counterparty expects. If that happens, suppress the out-of-order message and mark it as
+		// pending. When the signer becomes unblocked we can provide the messages in the proper order.
 		let order = self.context.resend_order.clone();
+		let (commitment_update, raa) = match order {
+			RAACommitmentOrder::CommitmentFirst => {
+				if self.context.signer_pending_commitment_update && raa.is_some() {
+					log_trace!(logger, "RAA is available, but we can't deliver it because ordering requires CommitmentFirst; setting signer_pending_revoke_and_ack = true");
+					self.context.signer_pending_revoke_and_ack = true;
+				}
+				(commitment_update, if !self.context.signer_pending_commitment_update { raa } else { None })
+			}
+
+			RAACommitmentOrder::RevokeAndACKFirst => {
+				if self.context.signer_pending_revoke_and_ack && commitment_update.is_some() {
+					log_trace!(logger, "commitment_update is available, but we can't deliver it because ordering requires RevokeAndACKFirst; setting signer_pending_commitment_update = true");
+					self.context.signer_pending_commitment_update = true;
+				}
+				(if !self.context.signer_pending_revoke_and_ack { commitment_update } else { None }, raa)
+			}
+		};
+
 		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" },