Trust Quorum: Handle prepare messages + Alarms

andrewjstone · andrewjstone · commit 10ae04e4e9d3 · 2025-04-29T05:01:29.000Z
This builds on #8052. Node's now handle `PrepareMsg`s from coordinators. The coordinator proptest was updated to generate prepares from non-existent test only nodes and send them to the coordinator. Additionally, protocol invariant violations are now detected in a few cases and recorded to the `PersistentState`. This is for debugging and support purposes. The goal is to test the code well enough that we never actually see an alarm in production.
diff --git a/trust-quorum/src/alarm.rs b/trust-quorum/src/alarm.rs
@@ -0,0 +1,60 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//! A mechanism for reporting protocol invariant violations
+//!
+//! Invariant violations should _never_ occur. They represent a critical bug in
+//! the implementation of the system. In certain scenarios we can detect these
+//! invariant violations and record them. This allows reporting them to higher
+//! levels of control plane software so that we can debug them and fix them in
+//! future releases, as well as rectify outstanding issues on systems where such
+//! an alarm arose.
+
+use crate::{Epoch, PlatformId};
+use serde::{Deserialize, Serialize};
+
+/// A critical invariant violation that should never occur.
+///
+/// Many invariant violations are only possible on receipt of peer messages,
+/// and are _not_ a result of API calls. This means that there isn't a good
+/// way to directly inform the rest of the control plane. Instead we provide a
+/// queryable API for `crate::Node` status that includes alerts.
+///
+/// If an `Alarm` is ever seen by an operator then support should be contacted
+/// immediately.
+#[derive(
+    Debug, Clone, thiserror::Error, PartialEq, Eq, Serialize, Deserialize,
+)]
+pub enum Alarm {
+    #[error(
+        "prepare for a later configuration exists: \
+        last_prepared_epoch = {last_prepared_epoch:?}, \
+        commit_epoch = {commit_epoch}"
+    )]
+    OutOfOrderCommit { last_prepared_epoch: Option<Epoch>, commit_epoch: Epoch },
+
+    #[error("commit attempted, but missing prepare message: epoch = {epoch}")]
+    MissingPrepare { epoch: Epoch },
+
+    #[error(
+        "prepare received with mismatched last_committed_epoch: prepare's last \
+        committed epoch = {prepare_last_committed_epoch:?}, persisted \
+        prepare's last_committed_epoch = \
+        {persisted_prepare_last_committed_epoch:?}"
+    )]
+    PrepareLastCommittedEpochMismatch {
+        prepare_last_committed_epoch: Option<Epoch>,
+        persisted_prepare_last_committed_epoch: Option<Epoch>,
+    },
+
+    #[error(
+        "different nodes coordinating same epoch = {epoch}: \
+        them = {them}, us = {us}"
+    )]
+    DifferentNodesCoordinatingSameEpoch {
+        epoch: Epoch,
+        them: PlatformId,
+        us: PlatformId,
+    },
+}
diff --git a/trust-quorum/src/crypto.rs b/trust-quorum/src/crypto.rs
@@ -29,8 +29,8 @@ use zeroize::{Zeroize, ZeroizeOnDrop, Zeroizing};
 const LRTQ_SHARE_SIZE: usize = 33;
 
 /// We don't distinguish whether this is an Ed25519 Scalar or set of GF(256)
-/// polynomials points with an x-coordinate of 0. Both can be treated as 32 byte
-/// blobs when decrypted, as they are immediately fed into HKDF.
+/// polynomials' points with an x-coordinate of 0. Both can be treated as 32
+/// byte blobs when decrypted, as they are immediately fed into HKDF.
 #[derive(
     Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize,
 )]
diff --git a/trust-quorum/src/lib.rs b/trust-quorum/src/lib.rs
@@ -12,6 +12,7 @@
 use derive_more::Display;
 use serde::{Deserialize, Serialize};
 
+mod alarm;
 mod configuration;
 mod coordinator_state;
 pub(crate) mod crypto;
@@ -20,9 +21,10 @@ mod messages;
 mod node;
 mod persistent_state;
 mod validators;
-pub use configuration::Configuration;
+pub use alarm::Alarm;
+pub use configuration::{Configuration, PreviousConfiguration};
 pub(crate) use coordinator_state::CoordinatorState;
-pub use crypto::RackSecret;
+pub use crypto::{EncryptedRackSecret, RackSecret, Salt, Sha3_256Digest};
 pub use messages::*;
 pub use node::Node;
 pub use persistent_state::{PersistentState, PersistentStateSummary};
diff --git a/trust-quorum/src/node.rs b/trust-quorum/src/node.rs
@@ -7,7 +7,8 @@
 use crate::errors::{CommitError, MismatchedRackIdError, ReconfigurationError};
 use crate::validators::ValidatedReconfigureMsg;
 use crate::{
-    CoordinatorState, Envelope, Epoch, PersistentState, PlatformId, messages::*,
+    Alarm, CoordinatorState, Envelope, Epoch, PersistentState, PlatformId,
+    messages::*,
 };
 use gfss::shamir::Share;
 use omicron_uuid_kinds::RackUuid;
@@ -110,10 +111,14 @@ impl Node {
         if last_prepared_epoch != Some(epoch) {
             error!(
                 self.log,
-                "Commit message occurred out of order";
+                "Protocol invariant violation: commit message out of order";
                 "epoch" => %epoch,
                 "last_prepared_epoch" => ?last_prepared_epoch
             );
+            self.persistent_state.add_alarm(Alarm::OutOfOrderCommit {
+                last_prepared_epoch,
+                commit_epoch: epoch,
+            });
             return Err(CommitError::OutOfOrderCommit);
         }
         if let Some(prepare) = self.persistent_state.prepares.get(&epoch) {
@@ -136,11 +141,19 @@ impl Node {
             //
             // Nexus should instead tell this node to retrieve a `Prepare`
             // from another node that has already committed.
-            error!(
-                self.log,
-                "tried to commit a configuration, but missing prepare msg";
-                "epoch" => %epoch
+            //
+            // This is a protocol bug because nexus must have seen an
+            // acknowledgement that this node had a `PrepareMsg` or erroneously
+            // sent a `Commit` anyway. This is a less serious error than other
+            // invariant violations since it can be recovered from. However, it
+            // is still worthy of an alarm, as the most likely case is a  disk/
+            // ledger failure.
+            let err_msg = concat!(
+                "Protocol invariant violation: triedi to ",
+                "commit a configuration, but missing prepare msg"
             );
+            error!(self.log, "{err_msg}"; "epoch" => %epoch);
+            self.persistent_state.add_alarm(Alarm::MissingPrepare { epoch });
             return Err(CommitError::MissingPrepare);
         }
 
@@ -178,6 +191,7 @@ impl Node {
         msg: PeerMsg,
     ) -> Option<PersistentState> {
         match msg {
+            PeerMsg::Prepare(msg) => self.handle_prepare(outbox, from, msg),
             PeerMsg::PrepareAck(epoch) => {
                 self.handle_prepare_ack(from, epoch);
                 None
@@ -196,6 +210,138 @@ impl Node {
         self.coordinator_state.as_ref()
     }
 
+    fn handle_prepare(
+        &mut self,
+        outbox: &mut Vec<Envelope>,
+        from: PlatformId,
+        msg: PrepareMsg,
+    ) -> Option<PersistentState> {
+        // TODO: check rack_id
+        if let Some(last_prepared_epoch) =
+            self.persistent_state.last_prepared_epoch()
+        {
+            // Is this an old request?
+            if msg.config.epoch < last_prepared_epoch {
+                warn!(self.log, "Received stale prepare";
+                    "from" => %from,
+                    "msg_epoch" => %msg.config.epoch,
+                    "last_prepared_epoch" => %last_prepared_epoch
+                );
+                // TODO: Respond to sender?
+                return None;
+            }
+
+            // Idempotent request
+            if msg.config.epoch == last_prepared_epoch {
+                return None;
+            }
+
+            // Does the last committed epoch match what we have?
+            let msg_last_committed_epoch =
+                msg.config.previous_configuration.as_ref().map(|p| p.epoch);
+            let last_committed_epoch =
+                self.persistent_state.last_committed_epoch();
+            if msg_last_committed_epoch != last_committed_epoch {
+                // If the msg contains an older last_committed_epoch than what
+                // we have, then out of order commits have occurred, as we know
+                // this prepare is later than what we've seen. This is a critical
+                // protocol invariant that has been violated.
+                //
+                // If the msg contains a newer last_committed_epoch than what
+                // we have, then we have likely missed a commit and are behind
+                // by more than one reconfiguration. The protocol currently does
+                // not allow this. Future protocol implementations may provide a
+                // capability to "jump" configurations.
+                //
+                // If there is a `None`/`Some` mismatch, then again, an invariant
+                // has been violated somewhere. The coordinator should know
+                // whether this is a "new" node or not, which would have a "None"
+                // configuration.
+                let err_msg = concat!(
+                    "Protocol invariant violation: ",
+                    "PrepareMsg last_committed_epoch does not match ours"
+                );
+                error!(self.log, "{err_msg}";
+                    "msg_last_committed_epoch" => ?msg_last_committed_epoch,
+                    "last_committed_epoch" => ?last_committed_epoch
+                );
+                self.persistent_state.add_alarm(
+                    Alarm::PrepareLastCommittedEpochMismatch {
+                        prepare_last_committed_epoch: msg_last_committed_epoch,
+                        persisted_prepare_last_committed_epoch:
+                            last_committed_epoch,
+                    },
+                );
+                return Some(self.persistent_state.clone());
+            }
+
+            // The prepare is up-to-date with respect to our persistent state
+        };
+
+        // Are we currently trying to coordinate?
+        if let Some(cs) = &self.coordinator_state {
+            let coordinating_epoch = cs.reconfigure_msg().epoch();
+            if coordinating_epoch > msg.config.epoch {
+                warn!(self.log, "Received stale prepare while coordinating";
+                    "from" => %from,
+                    "msg_epoch" => %msg.config.epoch,
+                    "epoch" => %cs.reconfigure_msg().epoch()
+                );
+                return None;
+            }
+            if coordinating_epoch == msg.config.epoch {
+                // TODO: Track these in an "invariant violation alarm" struct
+                // that can be queried.
+                let err_msg = concat!(
+                    "Protocol invariant violation: Nexus has told different",
+                    "nodes to coordinate the same reconfiguration."
+                );
+                error!(self.log, "{err_msg}";
+                    "epoch" => %coordinating_epoch,
+                    "from" => %from,
+                    "id" => %self.platform_id
+
+                );
+                self.persistent_state.add_alarm(
+                    Alarm::DifferentNodesCoordinatingSameEpoch {
+                        epoch: coordinating_epoch,
+                        them: from,
+                        us: self.platform_id.clone(),
+                    },
+                );
+                return Some(self.persistent_state.clone());
+            }
+
+            // This prepare is for a newer configuration than the one we
+            // are currently coordinating. We must implicitly cancel our
+            // coordination as Nexus has moved on.
+            let cancel_msg = concat!(
+                "Received a prepare for newer configuration.",
+                "Cancelling our coordination."
+            );
+            info!(self.log, "{cancel_msg}";
+                "msg_epoch" => %msg.config.epoch,
+                "epoch" => %coordinating_epoch,
+                "from" => %from
+            );
+        }
+
+        // Acknowledge the PrepareMsg
+        outbox.push(Envelope {
+            to: from,
+            from: self.platform_id.clone(),
+            msg: PeerMsg::PrepareAck(msg.config.epoch),
+        });
+
+        // Add the prepare to our `PersistentState`
+        self.persistent_state.prepares.insert(msg.config.epoch, msg);
+
+        // Stop coordinating
+        self.coordinator_state = None;
+
+        Some(self.persistent_state.clone())
+    }
+
     // Handle receipt of a `PrepareAck` message
     fn handle_prepare_ack(&mut self, from: PlatformId, epoch: Epoch) {
         // Are we coordinating for this epoch?
diff --git a/trust-quorum/src/persistent_state.rs b/trust-quorum/src/persistent_state.rs
@@ -8,13 +8,19 @@
 
 use crate::crypto::LrtqShare;
 use crate::messages::PrepareMsg;
-use crate::{Configuration, Epoch, PlatformId};
+use crate::{Alarm, Configuration, Epoch, PlatformId};
 use bootstore::schemes::v0::SharePkgCommon as LrtqShareData;
 use gfss::shamir::Share;
 use omicron_uuid_kinds::{GenericUuid, RackUuid};
 use serde::{Deserialize, Serialize};
 use std::collections::{BTreeMap, BTreeSet};
 
+// We want to prevent the situation where invariant violations keep replaying
+// due to message receipt and we blow up memory / storage with them. In
+// practice, even one `Alarm` is a critical bug, but we leave room to track a
+// few more.
+const MAX_ALARMS: usize = 10;
+
 /// All the persistent state for this protocol
 #[derive(Debug, Clone, Serialize, Deserialize, Default)]
 pub struct PersistentState {
@@ -30,6 +36,14 @@ pub struct PersistentState {
     // node. The sled corresponding to the node must be factory reset by wiping
     // its storage.
     pub decommissioned: Option<DecommissionedMetadata>,
+
+    // We persist [`Alarm`]s so that they can't be lost
+    //
+    // During a support scenario, any existing alarms must be manually cleared
+    // as part of resolving the issue. In principal, there should be *no* alarms
+    // as they are only for protocol invariant violations, so this should never
+    // be necessary.
+    alarms: Vec<Alarm>,
 }
 
 impl PersistentState {
@@ -39,6 +53,7 @@ impl PersistentState {
             prepares: BTreeMap::new(),
             commits: BTreeSet::new(),
             decommissioned: None,
+            alarms: Vec::new(),
         }
     }
 
@@ -93,6 +108,13 @@ impl PersistentState {
             self.prepares.get(&epoch).expect("missing prepare").share.clone()
         })
     }
+
+    // Add an alarm, if we haven't already reached the max number of alarms.
+    pub fn add_alarm(&mut self, alarm: Alarm) {
+        if self.alarms.len() < MAX_ALARMS {
+            self.alarms.push(alarm)
+        }
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
@@ -115,6 +137,7 @@ pub struct PersistentStateSummary {
     pub last_prepared_epoch: Option<Epoch>,
     pub last_committed_epoch: Option<Epoch>,
     pub decommissioned: Option<DecommissionedMetadata>,
+    pub alarms: Vec<Alarm>,
 }
 
 impl From<&PersistentState> for PersistentStateSummary {
@@ -126,6 +149,7 @@ impl From<&PersistentState> for PersistentStateSummary {
             last_prepared_epoch: value.last_prepared_epoch(),
             last_committed_epoch: value.last_committed_epoch(),
             decommissioned: value.decommissioned.clone(),
+            alarms: value.alarms.clone(),
         }
     }
 }
diff --git a/trust-quorum/src/validators.rs b/trust-quorum/src/validators.rs
diff --git a/trust-quorum/tests/coordinator.rs b/trust-quorum/tests/coordinator.rs
