Fix CommitAdvance behavior for expunged nodes

Author: andrewjstone

trust-quorum/src/alarm.rs

@@ -8,6 +8,7 @@ use serde::{Deserialize, Serialize};
 
 use crate::{Configuration, Epoch, PlatformId};
 
+#[allow(clippy::large_enum_variant)]
 #[derive(
     Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize,
 )]

trust-quorum/src/compute_key_share.rs

@@ -103,6 +103,7 @@ impl KeyShareComputer {
                 "epoch" => %epoch,
                 "from" => %from
             );
+            return false;
         }
 
         // A valid share was received. Is it new?
@@ -118,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"));
 

trust-quorum/src/node.rs

@@ -308,6 +308,7 @@ 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.
@@ -347,6 +348,7 @@ impl Node {
                     "received_epoch" => %config.epoch
                 );
                 self.coordinator_state = None;
+                // Intentionally fall through
             } else if coordinating_epoch == config.epoch {
                 info!(
                     self.log,
@@ -402,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));
     }
@@ -417,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!(
@@ -435,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