TQ: cluster proptest Action/Event cleanup (#8899)

This builds on #8874 

To make using using tqdb easier, we put all necessary information inside
events. This works because of deterministic replay using the same
infrastucture as the proptest itself. To ensure we are truly
deterministically replaying we must assert that any Event in our log has
actually been generated when we go to apply it in tqdb. In the common
case case this is equivalent to asserting that the `DeliveredEnvelope`
is actually the same as the one pulled off the `bootstrap_network` vec
during runs. In order to guarantee this a few changes needed to be made:

1. Determinism exists, except for in the crypto code because we don't
seed the various random number generators, and therefore different key
shares and rack secrets get generated in different runs. We could seed
them, but then this makes it possible that we accidentally seed them in
production code. More importantly for our current purposes, though, is
that it's tedious and unnecessary. Instead we just implement comparison
methods for messages that ignore things like key shares. This works fine
because if the shares are not self-consistent with the rack secret and
the parameters such as threshold that we don't change the crypto code
itself will fail immediately.

2. We had a few actions that generated multiple events. Unfortunately,
applying these events would result in mutating the test networks
multiple times, resulting in a difference between which message was
recorded and which was actually pulled out to be delivered. This was
fixed by changing each action to only do a single thing at a time, like
deliver one envelope or commit a configuration at one node. This also
allows for finer grain interleaving and matches better with our TLA+
spec/model checking. This was observable by looking at the event logs.

The change to an action typically generating a single event means,
however, that we need to generate more actions per run to get the
equivalent functionality. A single action won't result in N commits or N
envelopes delivered. Therefore, each test run now needs to have
significantly more actions generated. Unfortunately this can make test
runs even longer. In order to help alleviate this we made three other
changes:

1. We change the invariant checks to only look at nodes that could
possibly be mutated by an event application. We call these the "affected
nodes". This prevents looping over every node in each check.
2. We reduce the member universe, and hence the size of the state space.
3. We reduce the total number of test cases per run.

Author: andrewjstone

Cargo.toml

@@ -842,6 +842,11 @@ opt-level = 3
 [profile.dev.package.bootstore]
 opt-level = 3
 
+[profile.dev.package.trust-quorum]
+opt-level = 3
+[profile.dev.package.trust-quorum-test-utils]
+opt-level = 3
+
 # Crypto stuff always needs optimizations
 [profile.dev.package.sha3]
 opt-level = 3

trust-quorum/Cargo.toml

@@ -52,3 +52,4 @@ trust-quorum-test-utils.workspace = true
 # subtle when we do this. On the other hand its very useful for testing and
 # debugging outside of production.
 danger_partial_eq_ct_wrapper = ["gfss/danger_partial_eq_ct_wrapper"]
+testing = []

trust-quorum/src/configuration.rs

@@ -13,6 +13,7 @@ use iddqd::{IdOrdItem, id_upcast};
 use omicron_uuid_kinds::RackUuid;
 use secrecy::ExposeSecret;
 use serde::{Deserialize, Serialize};
+use serde_with::serde_as;
 use slog_error_chain::SlogInlineError;
 use std::collections::BTreeMap;
 
@@ -31,6 +32,7 @@ pub enum ConfigurationError {
 /// The configuration for a given epoch.
 ///
 /// Only valid for non-lrtq configurations
+#[serde_as]
 #[derive(
     Debug,
     Clone,
@@ -53,6 +55,7 @@ pub struct Configuration {
     pub coordinator: PlatformId,
 
     // All members of the current configuration and the hash of their key shares
+    #[serde_as(as = "Vec<(_, _)>")]
     pub members: BTreeMap<PlatformId, Sha3_256Digest>,
 
     /// The number of sleds required to reconstruct the rack secret
@@ -121,4 +124,25 @@ impl Configuration {
             shares,
         ))
     }
+
+    #[cfg(feature = "testing")]
+    pub fn equal_except_for_crypto_data(&self, other: &Self) -> bool {
+        let encrypted_rack_secrets_match =
+            match (&self.encrypted_rack_secrets, &other.encrypted_rack_secrets)
+            {
+                (None, None) => true,
+                (Some(_), Some(_)) => true,
+                _ => false,
+            };
+        self.rack_id == other.rack_id
+            && self.epoch == other.epoch
+            && self.coordinator == other.coordinator
+            && self
+                .members
+                .keys()
+                .zip(other.members.keys())
+                .all(|(id1, id2)| id1 == id2)
+            && self.threshold == other.threshold
+            && encrypted_rack_secrets_match
+    }
 }

trust-quorum/src/lib.rs

@@ -139,6 +139,15 @@ pub struct Envelope {
     pub msg: PeerMsg,
 }
 
+#[cfg(feature = "testing")]
+impl Envelope {
+    pub fn equal_except_for_crypto_data(&self, other: &Self) -> bool {
+        self.to == other.to
+            && self.from == other.from
+            && self.msg.equal_except_for_crypto_data(&other.msg)
+    }
+}
+
 /// Check if a received share is valid for a given configuration
 ///
 /// Return true if valid, false otherwise.

trust-quorum/src/messages.rs

@@ -30,6 +30,14 @@ pub struct PeerMsg {
     pub kind: PeerMsgKind,
 }
 
+impl PeerMsg {
+    #[cfg(feature = "testing")]
+    pub fn equal_except_for_crypto_data(&self, other: &Self) -> bool {
+        self.rack_id == other.rack_id
+            && self.kind.equal_except_for_crypto_data(&other.kind)
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[cfg_attr(feature = "danger_partial_eq_ct_wrapper", derive(PartialEq, Eq))]
 pub enum PeerMsgKind {
@@ -92,4 +100,28 @@ impl PeerMsgKind {
             Self::CommitAdvance(_) => "commit_advance",
         }
     }
+
+    /// This is useful for our replay tests without having to worry about seeding
+    /// the various random number generators in our production code.
+    #[cfg(feature = "testing")]
+    pub fn equal_except_for_crypto_data(&self, other: &Self) -> bool {
+        match (self, other) {
+            (
+                Self::Prepare { config: config1, .. },
+                Self::Prepare { config: config2, .. },
+            ) => config1.equal_except_for_crypto_data(config2),
+            (Self::Config(config1), Self::Config(config2)) => {
+                config1.equal_except_for_crypto_data(config2)
+            }
+            (
+                Self::Share { epoch: epoch1, .. },
+                Self::Share { epoch: epoch2, .. },
+            ) => epoch1 == epoch2,
+            (Self::LrtqShare(_), Self::LrtqShare(_)) => true,
+            (Self::CommitAdvance(config1), Self::CommitAdvance(config2)) => {
+                config1.equal_except_for_crypto_data(config2)
+            }
+            (s, o) => s == o,
+        }
+    }
 }

trust-quorum/test-utils/Cargo.toml

@@ -16,6 +16,6 @@ omicron-uuid-kinds.workspace = true
 serde.workspace = true
 serde_json.workspace = true
 slog.workspace = true
-trust-quorum = { workspace = true, features = ["danger_partial_eq_ct_wrapper"] }
+trust-quorum = { workspace = true, features = ["danger_partial_eq_ct_wrapper", "testing"] }
 
 omicron-workspace-hack.workspace = true

trust-quorum/test-utils/src/event.rs

@@ -7,7 +7,7 @@
 use crate::nexus::{NexusConfig, NexusReply};
 use serde::{Deserialize, Serialize};
 use std::collections::BTreeSet;
-use trust_quorum::{Epoch, PlatformId};
+use trust_quorum::{Envelope, Epoch, PlatformId};
 
 /// An event that can be fed into our system under test (SUT)
 ///
@@ -22,12 +22,30 @@ pub enum Event {
     },
     AbortConfiguration(Epoch),
     SendNexusReplyOnUnderlay(NexusReply),
-    /// Pull an envelope off the bootstrap network and call `Node::handle`
-    DeliverEnvelope {
-        destination: PlatformId,
-    },
+    /// Call `Node::handle` with the given Envelope.
+    ///
+    /// Since replay is deterministic, we actually know what this value is,
+    /// even though a prior event may not have yet sent the message.
+    DeliverEnvelope(Envelope),
     /// Pull a `NexusReply` off the underlay network and update the `NexusState`
-    DeliverNexusReply,
+    DeliverNexusReply(NexusReply),
     CommitConfiguration(PlatformId),
     Reconfigure(NexusConfig),
 }
+
+impl Event {
+    /// Return which nodes the event may have mutated.
+    pub fn affected_nodes(&self) -> Vec<PlatformId> {
+        match self {
+            Self::InitialSetup { config, crashed_nodes, .. } => {
+                config.members.union(&crashed_nodes).cloned().collect()
+            }
+            Self::AbortConfiguration(_) => vec![],
+            Self::SendNexusReplyOnUnderlay(_) => vec![],
+            Self::DeliverEnvelope(envelope) => vec![envelope.to.clone()],
+            Self::DeliverNexusReply(_) => vec![],
+            Self::CommitConfiguration(id) => vec![id.clone()],
+            Self::Reconfigure(_) => vec![],
+        }
+    }
+}

trust-quorum/test-utils/src/nexus.rs

@@ -113,7 +113,9 @@ pub struct NexusState {
 impl NexusState {
     #[allow(clippy::new_without_default)]
     pub fn new() -> NexusState {
-        NexusState { rack_id: RackUuid::new_v4(), configs: IdOrdMap::new() }
+        // We end up replaying events in tqdb, and can't use a random rack
+        // uuid.
+        NexusState { rack_id: RackUuid::nil(), configs: IdOrdMap::new() }
     }
 
     // Create a `ReconfigureMsg` for the latest nexus config

trust-quorum/test-utils/src/state.rs

@@ -201,11 +201,11 @@ impl TqState {
             Event::SendNexusReplyOnUnderlay(reply) => {
                 self.apply_event_send_nexus_reply_on_underlay(reply)
             }
-            Event::DeliverEnvelope { destination } => {
-                self.apply_event_deliver_envelope(destination);
+            Event::DeliverEnvelope(envelope) => {
+                self.apply_event_deliver_envelope(envelope);
             }
-            Event::DeliverNexusReply => {
-                self.apply_event_deliver_nexus_reply();
+            Event::DeliverNexusReply(reply) => {
+                self.apply_event_deliver_nexus_reply(reply);
             }
             Event::CommitConfiguration(dest) => {
                 self.apply_event_commit(dest);
@@ -273,9 +273,10 @@ impl TqState {
         self.underlay_network.push(reply);
     }
 
-    fn apply_event_deliver_nexus_reply(&mut self) {
+    fn apply_event_deliver_nexus_reply(&mut self, recorded_reply: NexusReply) {
         let mut latest_config = self.nexus.latest_config_mut();
         let reply = self.underlay_network.pop().expect("reply exists");
+        assert_eq!(recorded_reply, reply);
         match reply {
             NexusReply::AckedPreparesFromCoordinator { epoch, acks } => {
                 if epoch == latest_config.epoch {
@@ -301,13 +302,21 @@ impl TqState {
         latest_config.op = NexusOp::Aborted;
     }
 
-    fn apply_event_deliver_envelope(&mut self, destination: PlatformId) {
+    fn apply_event_deliver_envelope(&mut self, recorded_envelope: Envelope) {
         let envelope = self
             .bootstrap_network
-            .get_mut(&destination)
+            .get_mut(&recorded_envelope.to)
             .unwrap()
             .pop()
             .expect("envelope in bootstrap network");
+
+        // The recorded envelope must be exactly the same as the one pulled
+        // off the bootstrap network. We ignore crypto data because we don't
+        // currently seed and track random number generators. For our purposes,
+        // validating the other fields is enough, because the test will fail if
+        // the crypto doesn't work and decrypt to the same plaintext.
+        assert!(recorded_envelope.equal_except_for_crypto_data(&envelope));
+
         let (node, ctx) =
             self.sut.nodes.get_mut(&envelope.to).expect("destination exists");
         node.handle(ctx, envelope.from, envelope.msg);