TQ: Add support for alarms in the protocol

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,42 @@
+// 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};
+
+#[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 },
+
+    /// We received a `CommitAdvance` while coordinating for the same epoch
+    ///
+    /// Reason: `CommitAdvance` is a reply for a key share request in an
+    /// old epoch that we don't have the latest committed coordination.
+    /// However we are actually the coordinator for the configuration in the
+    /// `CommitAdvance`. While it's possible that another node could learn
+    /// of the commit from Nexus before the coordinator, the coordinator will
+    /// never ask for a key share for that epoch. Therefore this state should be
+    /// impossible to reach.
+    CommitAdvanceForCoordinatingEpoch { config: Configuration },
+
+    /// 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};
@@ -137,11 +139,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;
@@ -311,25 +311,28 @@ impl Node {
         }
 
         // 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(),
+                });
             }
-        });
+        } 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 {
@@ -350,7 +353,9 @@ impl Node {
                     "from" => %from,
                     "epoch" => %config.epoch
                 );
-                // TODO: Alarm
+                ctx.raise_alarm(Alarm::CommitAdvanceForCoordinatingEpoch {
+                    config,
+                });
                 return;
             } else {
                 info!(

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.rs

@@ -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