TQ: Add support for alarms in the protocol (#8753)

This builds on #8741

An alarm represents a protocol invariant violation. It's unclear exactly
what should be done about these other than recording them and allowing
them to be reported upstack, which is what is done in this PR. An
argument could be made for "freezing" the state machine such that trust
quorum nodes stop working and the only thing they can do is report alarm
status. However, that would block the trust quorum from operating at
all, and it's unclear if this should cause an outage on that node.

I'm also somewhat hesitant to put the alarms into the persistent state
as that would prevent unlock in the case of a sled/rack reboot.

On the flip side of just recording is the possible danger resulting from
operating with an invariant violation. This could potentially be risky,
and since we shouldn't ever see these maybe pausing for a support call
is the right thing. TBD, once more work is done on the protocol.

Author: andrewjstone

trust-quorum/gfss/src/shamir.rs

@@ -23,7 +23,17 @@ pub enum SplitError {
     TooFewTotalShares { n: u8, k: u8 },
 }
 
-#[derive(Debug, Clone, thiserror::Error, PartialEq, Eq)]
+#[derive(
+    Debug,
+    Clone,
+    thiserror::Error,
+    PartialEq,
+    Eq,
+    PartialOrd,
+    Ord,
+    Serialize,
+    Deserialize,
+)]
 pub enum CombineError {
     #[error("must be at least 2 shares to combine")]
     TooFewShares,

trust-quorum/src/alarm.rs

@@ -0,0 +1,36 @@
+// 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/.
+
+//! Mechanism for reporting protocol invariant violations
+
+use serde::{Deserialize, Serialize};
+
+use crate::{Configuration, Epoch, PlatformId};
+
+#[allow(clippy::large_enum_variant)]
+#[derive(
+    Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize,
+)]
+pub enum Alarm {
+    /// Different configurations found for the same epoch
+    ///
+    /// Reason: Nexus creates configurations and stores them in CRDB before
+    /// sending them to a coordinator of its choosing. Nexus will not send the
+    /// same reconfiguration request to different coordinators. If it does those
+    /// coordinators will generate different key shares. However, since Nexus
+    /// will not tell different nodes to coordinate the same configuration, this
+    /// state should be impossible to reach.
+    MismatchedConfigurations {
+        config1: Configuration,
+        config2: Configuration,
+        from: PlatformId,
+    },
+
+    /// The `keyShareComputer` could not compute this node's share
+    ///
+    /// Reason: A threshold of valid key shares were received based on the the
+    /// share digests in the Configuration. However, computation of the share
+    /// still failed. This should be impossible.
+    ShareComputationFailed { epoch: Epoch, err: gfss::shamir::CombineError },
+}

trust-quorum/src/compute_key_share.rs

@@ -9,7 +9,9 @@
 //! other nodes so  that it can compute its own key share.
 
 use crate::crypto::Sha3_256Digest;
-use crate::{Configuration, Epoch, NodeHandlerCtx, PeerMsgKind, PlatformId};
+use crate::{
+    Alarm, Configuration, Epoch, NodeHandlerCtx, PeerMsgKind, PlatformId,
+};
 use gfss::gf256::Gf256;
 use gfss::shamir::{self, Share};
 use slog::{Logger, error, o, warn};
@@ -101,6 +103,7 @@ impl KeyShareComputer {
                 "epoch" => %epoch,
                 "from" => %from
             );
+            return false;
         }
 
         // A valid share was received. Is it new?
@@ -116,12 +119,23 @@ impl KeyShareComputer {
         // What index are we in the configuration? This is our "x-coordinate"
         // for our key share calculation. We always start indexing from 1, since
         // 0 is the rack secret.
-        let index = self
-            .config
-            .members
-            .keys()
-            .position(|id| id == ctx.platform_id())
-            .expect("node exists");
+        let index =
+            self.config.members.keys().position(|id| id == ctx.platform_id());
+
+        let Some(index) = index else {
+            let msg = concat!(
+                "Failed to get index for ourselves in current configuration. ",
+                "We are not a member, and must have been expunged."
+            );
+            error!(
+                self.log,
+                "{msg}";
+                "platform_id" => %ctx.platform_id(),
+                "config" => ?self.config
+            );
+            return false;
+        };
+
         let x_coordinate =
             Gf256::new(u8::try_from(index + 1).expect("index fits in u8"));
 
@@ -137,11 +151,9 @@ impl KeyShareComputer {
                 });
                 true
             }
-            Err(e) => {
-                // TODO: put the node into into an `Alarm` state similar to
-                // https://github.com/oxidecomputer/omicron/pull/8062 once we
-                // have alarms?
-                error!(self.log, "Failed to compute share: {}", e);
+            Err(err) => {
+                error!(self.log, "Failed to compute share: {}", err);
+                ctx.raise_alarm(Alarm::ShareComputationFailed { epoch, err });
                 false
             }
         }

trust-quorum/src/configuration.rs

@@ -30,7 +30,9 @@ pub enum ConfigurationError {
 /// The configuration for a given epoch.
 ///
 /// Only valid for non-lrtq configurations
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(
+    Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize,
+)]
 pub struct Configuration {
     /// Unique Id of the rack
     pub rack_id: RackUuid,

trust-quorum/src/lib.rs

@@ -23,7 +23,9 @@ mod persistent_state;
 mod validators;
 pub use configuration::Configuration;
 pub use coordinator_state::{CoordinatorOperation, CoordinatorState};
+mod alarm;
 
+pub use alarm::Alarm;
 pub use crypto::RackSecret;
 pub use messages::*;
 pub use node::Node;

trust-quorum/src/node.rs

@@ -20,7 +20,7 @@ use crate::validators::{
     MismatchedRackIdError, ReconfigurationError, ValidatedReconfigureMsg,
 };
 use crate::{
-    Configuration, CoordinatorState, Epoch, NodeHandlerCtx, PlatformId,
+    Alarm, Configuration, CoordinatorState, Epoch, NodeHandlerCtx, PlatformId,
     messages::*,
 };
 use gfss::shamir::Share;
@@ -308,28 +308,33 @@ impl Node {
                 "epoch" => %config.epoch
             );
             ctx.update_persistent_state(|ps| ps.commits.insert(config.epoch));
+            return;
         }
 
         // Do we have the configuration in our persistent state? If not save it.
-        ctx.update_persistent_state(|ps| {
-            if let Err(e) = ps.configs.insert_unique(config.clone()) {
-                let existing =
-                    e.duplicates().first().expect("duplicate exists");
-                if *existing != &config {
-                    error!(
-                        self.log,
-                        "Received a configuration mismatch";
-                        "from" => %from,
-                        "existing_config" => #?existing,
-                        "received_config" => #?config
-                    );
-                    // TODO: Alarm
-                }
-                false
-            } else {
-                true
+        if let Some(existing) =
+            ctx.persistent_state().configuration(config.epoch)
+        {
+            if existing != &config {
+                error!(
+                    self.log,
+                    "Received a configuration mismatch";
+                    "from" => %from,
+                    "existing_config" => #?existing,
+                    "received_config" => #?config
+                );
+                ctx.raise_alarm(Alarm::MismatchedConfigurations {
+                    config1: (*existing).clone(),
+                    config2: config.clone(),
+                    from: from.clone(),
+                });
             }
-        });
+        } else {
+            ctx.update_persistent_state(|ps| {
+                ps.configs.insert_unique(config.clone()).expect("new config");
+                true
+            });
+        }
 
         // Are we coordinating for an older epoch? If so, cancel.
         if let Some(cs) = &self.coordinator_state {
@@ -343,14 +348,14 @@ impl Node {
                     "received_epoch" => %config.epoch
                 );
                 self.coordinator_state = None;
+                // Intentionally fall through
             } else if coordinating_epoch == config.epoch {
-                error!(
+                info!(
                     self.log,
                     "Received CommitAdvance while coordinating for same epoch!";
                     "from" => %from,
                     "epoch" => %config.epoch
                 );
-                // TODO: Alarm
                 return;
             } else {
                 info!(
@@ -399,7 +404,8 @@ impl Node {
             }
         }
 
-        // We either were collectiong shares for an old epoch or haven't started yet.
+        // We either were collectiong shares for an old epoch or haven't started
+        // yet.
         self.key_share_computer =
             Some(KeyShareComputer::new(&self.log, ctx, config));
     }
@@ -414,6 +420,18 @@ impl Node {
             ctx.persistent_state().latest_committed_configuration()
         {
             if latest_committed_config.epoch > epoch {
+                if !latest_committed_config.members.contains_key(&from) {
+                    info!(
+                        self.log,
+                        "Received a GetShare message from expunged node";
+                        "from" => %from,
+                        "latest_committed_epoch" =>
+                            %latest_committed_config.epoch,
+                        "requested_epoch" => %epoch
+                    );
+                    // TODO: Send an expunged message
+                    return;
+                }
                 info!(
                     self.log,
                     concat!(
@@ -432,6 +450,20 @@ impl Node {
             }
         }
 
+        // Do we have the configuration? Is the requesting peer a member?
+        if let Some(config) = ctx.persistent_state().configuration(epoch) {
+            if !config.members.contains_key(&from) {
+                info!(
+                    self.log,
+                    "Received a GetShare message from expunged node";
+                    "from" => %from,
+                    "epoch" => %epoch
+                );
+                // TODO: Send an expunged message
+                return;
+            }
+        }
+
         // If we have the share for the requested epoch, we always return it. We
         // know that it is at least as new as the last committed epoch. We might
         // not have learned about the configuration being committed yet, but

trust-quorum/src/node_ctx.rs

@@ -4,14 +4,17 @@
 
 //! Parameter to Node API calls that allows interaction with the system at large
 
-use crate::{Envelope, PeerMsg, PeerMsgKind, PersistentState, PlatformId};
+use crate::{
+    Alarm, Envelope, PeerMsg, PeerMsgKind, PersistentState, PlatformId,
+};
 use std::collections::BTreeSet;
 
 /// An API shared by [`NodeCallerCtx`] and [`NodeHandlerCtx`]
 pub trait NodeCommonCtx {
     fn platform_id(&self) -> &PlatformId;
     fn persistent_state(&self) -> &PersistentState;
     fn connected(&self) -> &BTreeSet<PlatformId>;
+    fn alarms(&self) -> &BTreeSet<Alarm>;
 }
 
 /// An API for an [`NodeCtx`] usable from a [`crate::Node`]
@@ -54,6 +57,9 @@ pub trait NodeHandlerCtx: NodeCommonCtx {
 
     /// Remove a peer from the connected set
     fn remove_connection(&mut self, id: &PlatformId);
+
+    /// Record (in-memory) that an alarm has occurred
+    fn raise_alarm(&mut self, alarm: Alarm);
 }
 
 /// Common parameter to [`crate::Node`] methods
@@ -79,6 +85,9 @@ pub struct NodeCtx {
 
     /// Connected peer nodes
     connected: BTreeSet<PlatformId>,
+
+    /// Any alarms that have occurred
+    alarms: BTreeSet<Alarm>,
 }
 
 impl NodeCtx {
@@ -89,6 +98,7 @@ impl NodeCtx {
             persistent_state_changed: false,
             outgoing: Vec::new(),
             connected: BTreeSet::new(),
+            alarms: BTreeSet::new(),
         }
     }
 }
@@ -105,6 +115,10 @@ impl NodeCommonCtx for NodeCtx {
     fn connected(&self) -> &BTreeSet<PlatformId> {
         &self.connected
     }
+
+    fn alarms(&self) -> &BTreeSet<Alarm> {
+        &self.alarms
+    }
 }
 
 impl NodeHandlerCtx for NodeCtx {
@@ -138,6 +152,10 @@ impl NodeHandlerCtx for NodeCtx {
     fn remove_connection(&mut self, id: &PlatformId) {
         self.connected.remove(id);
     }
+
+    fn raise_alarm(&mut self, alarm: Alarm) {
+        self.alarms.insert(alarm);
+    }
 }
 
 impl NodeCallerCtx for NodeCtx {

trust-quorum/tests/cluster.proptest-regressions

@@ -834,6 +834,7 @@ impl TestState {
         self.invariant_nodes_have_prepared_if_coordinator_has_acks()?;
         self.invariant_nodes_have_committed_if_nexus_has_acks()?;
         self.invariant_nodes_not_coordinating_and_computing_key_share_simultaneously()?;
+        self.invariant_no_alarms()?;
         Ok(())
     }
 
@@ -953,6 +954,20 @@ impl TestState {
 
         Ok(())
     }
+
+    // Ensure there has been no alarm at any node
+    fn invariant_no_alarms(&self) -> Result<(), TestCaseError> {
+        for (id, (_, ctx)) in &self.sut.nodes {
+            let alarms = ctx.alarms();
+            prop_assert!(
+                alarms.is_empty(),
+                "Alarms found for {}: {:#?}",
+                id,
+                alarms
+            );
+        }
+        Ok(())
+    }
 }
 
 /// Broken out of `TestState` to alleviate borrow checker woes