fix(validator): slashing-protection review follow-ups: attestation index check, exit on a poisoned record

crates/validator/Cargo.toml

@@ -31,6 +31,10 @@ tracing.workspace = true
 # Declared here rather than relied on through workspace feature unification,
 # so this crate still builds if it is ever compiled on its own.
 tokio = { workspace = true, features = ["time", "macros", "rt"] }
+# `CancellationToken`: the signal that stops the client when the
+# slashing-protection record becomes unusable, and the one that tells the
+# keymanager server to finish its requests in flight.
+tokio-util.workspace = true
 reqwest = { workspace = true, features = ["json"] }
 
 # BLS12-381. Same version and backend the state transition verifies with, so a

crates/validator/src/attestation.rs

@@ -135,7 +135,7 @@ impl<B: BeaconNodeApi> AttestationService<B> {
         // returned; failing here loses the slot rather than trying the next
         // node. That is why the same check now lives in the implementation,
         // where an `Err` is something failover can act on.
-        validate_attestation_data(slot, &data)?;
+        validate_attestation_data(slot, slot_fork, &data)?;
 
         // Consistent with the check just above: `data.target.epoch` is now
         // known to equal `expected_target_epoch`, so this is the fork this
@@ -427,7 +427,7 @@ mod tests {
         keys: &[ethlambda_types::beacon::primitives::BlsPubkey],
     ) -> SharedProtection {
         let protection = SlashingProtection::open(dir, Root::ZERO, keys, true).expect("opens");
-        Arc::new(std::sync::Mutex::new(protection))
+        crate::slashing_protection::SharedProtection::new(protection)
     }
 
     /// The restart shape the old in-memory guard could not cover: a fresh
@@ -705,7 +705,14 @@ mod tests {
         let store = RwLock::new(store);
         let pubkey_hex = encode_hex(&pubkey.0);
 
-        let node = Arc::new(MockBeaconNode::new().with_attestation_data(96));
+        // A node that breaks the trait's contract and hands the stale data
+        // back unchecked, so the refusal below is `attest`'s own, the last
+        // one before a signature, and not the implementation's.
+        let node = Arc::new(
+            MockBeaconNode::new()
+                .with_attestation_data(96)
+                .without_attestation_data_check(),
+        );
         let service =
             AttestationService::new(node.clone(), context(), SlashingProtection::in_memory());
 
@@ -734,7 +741,8 @@ mod tests {
         let store = RwLock::new(store);
         let pubkey_hex = encode_hex(&pubkey.0);
 
-        let mut node = MockBeaconNode::new();
+        // Unchecked by the node, so `attest`'s own check is what refuses.
+        let mut node = MockBeaconNode::new().without_attestation_data_check();
         node.attestation_data = Some(AttestationData {
             slot: 96,
             index: 0,
@@ -763,6 +771,49 @@ mod tests {
         assert!(node.submitted().is_empty());
     }
 
+    /// A liveness check rather than a slashing one: an index that cannot be
+    /// valid at the slot's fork is refused before anything is recorded, so
+    /// the validator's one vote for the epoch is not spent on an attestation
+    /// every peer would reject, and a correct answer for the same slot is
+    /// still signed. `context()` is mainnet's schedule, where slot 96 is long
+    /// before gloas, so 1 is not an index a vote there can carry.
+    ///
+    /// The node hands its answer back unchecked, so what refuses it is
+    /// `attest`'s own check, under the fork `attest` derives for the slot:
+    /// with that check gone, or run under gloas, the first call would sign.
+    #[tokio::test]
+    async fn an_index_invalid_at_the_slots_fork_is_refused_and_records_nothing() {
+        let mut store = ValidatorStore::new();
+        let pubkey = store.insert_secret("test", &secret()).expect("inserts");
+        let duties = vec![duty(&encode_hex(&pubkey.0), 1337, 96, 3)];
+        let store = RwLock::new(store);
+        let protection = SlashingProtection::in_memory();
+
+        let wrong = Arc::new(
+            MockBeaconNode::new()
+                .with_attestation_data(96)
+                .with_attestation_index(1)
+                .without_attestation_data_check(),
+        );
+        let err = AttestationService::new(wrong.clone(), context(), protection.clone())
+            .attest(96, &duties, &store)
+            .await
+            .expect_err("must reject an index of 1 before gloas");
+        assert!(matches!(err, Error::InconsistentResponse(_)), "got {err:?}");
+        assert!(wrong.submitted().is_empty(), "nothing may be signed");
+
+        let right = Arc::new(MockBeaconNode::new().with_attestation_data(96));
+        let attested = AttestationService::new(right.clone(), context(), protection)
+            .attest(96, &duties, &store)
+            .await
+            .expect("attests");
+        assert_eq!(
+            (attested.published, attested.refused),
+            (1, 0),
+            "the refused answer must not have recorded the target"
+        );
+    }
+
     /// The regression test for why `attest` derives the `Eth-Consensus-Version`
     /// header from `data.target.epoch` rather than accepting it from the
     /// caller: the two slots below are one epoch apart and straddle mainnet's

crates/validator/src/beacon_node/fallback.rs

@@ -436,6 +436,14 @@ mod tests {
     use crate::beacon_node::mock::MockBeaconNode;
     use ethlambda_types::beacon::primitives::Root;
 
+    /// The fork a caller asks about `slot` under: the one mainnet's schedule
+    /// puts it in, as `AttestationService` derives it, rather than one made
+    /// up here that the slot does not belong to.
+    fn fork_of(slot: Slot) -> ForkName {
+        let epoch = ethlambda_types::beacon::signing::compute_epoch_at_slot(slot);
+        ethlambda_types::beacon::config::Config::mainnet().fork_at_epoch(epoch)
+    }
+
     fn healthy() -> MockBeaconNode {
         let mut node = MockBeaconNode::new();
         node.genesis = Some(Genesis {
@@ -474,7 +482,7 @@ mod tests {
         let fallback = FallbackBeaconNode::new(vec![stale, fresh]);
 
         let data = fallback
-            .attestation_data(96, ForkName::Electra)
+            .attestation_data(96, fork_of(96))
             .await
             .expect("the second node answers correctly");
 
@@ -494,13 +502,37 @@ mod tests {
         ]);
 
         let err = fallback
-            .attestation_data(96, ForkName::Electra)
+            .attestation_data(96, fork_of(96))
             .await
             .expect_err("no node answered about the requested slot");
 
         assert!(matches!(err, Error::AllBeaconNodesFailed(_)), "got {err:?}");
     }
 
+    /// The same arrangement for the index: a node answering with one no vote
+    /// at the slot's fork can carry is failed over from, rather than its
+    /// answer being signed, recorded and then rejected by every peer.
+    #[tokio::test]
+    async fn a_node_answering_with_an_index_invalid_at_the_fork_is_failed_over_from() {
+        // Slot 96 is before gloas, where the index is zero; node 1 answers 1
+        // regardless.
+        let wrong = MockBeaconNode::new()
+            .with_attestation_data(96)
+            .with_attestation_index(1);
+        let right = MockBeaconNode::new().with_attestation_data(96);
+        let fallback = FallbackBeaconNode::new(vec![wrong, right]);
+
+        let data = fallback
+            .attestation_data(96, fork_of(96))
+            .await
+            .expect("the second node answers correctly");
+
+        assert_eq!(
+            data.index, 0,
+            "the invalid answer must not be the one returned"
+        );
+    }
+
     /// A syncing node at the head of the list must not stop duties being
     /// refreshed when a healthy node sits behind it.
     ///

crates/validator/src/beacon_node/http.rs

@@ -589,7 +589,7 @@ impl BeaconNodeApi for HttpBeaconNode {
         // is what lets `FallbackBeaconNode` move to the next node; the same
         // check one layer up runs only after this node's answer was already
         // accepted.
-        validate_attestation_data(slot, &data)?;
+        validate_attestation_data(slot, fork, &data)?;
         Ok(data)
     }
 

crates/validator/src/beacon_node/mock.rs

@@ -64,6 +64,9 @@ pub struct MockBeaconNode {
     pub duties: Mutex<Vec<(Epoch, AttesterDuties)>>,
     pub proposers: Mutex<Vec<(Epoch, ProposerDuties)>>,
     pub attestation_data: Option<AttestationData>,
+    /// Hand `attestation_data` back without checking it, as an implementation
+    /// that failed to honour the trait's contract would.
+    pub attestation_data_unchecked: bool,
     /// The slot and proposer the mock will produce a block for. `None` makes
     /// `produce_block` fail, the way an unset `attestation_data` does.
     pub produces_block: Option<(Slot, ValidatorIndex)>,
@@ -310,6 +313,15 @@ impl MockBeaconNode {
             .clone()
     }
 
+    /// Break the contract on `BeaconNodeApi::attestation_data`: answer with
+    /// whatever data is set, unchecked. For tests of the caller's own check,
+    /// which a mock that honours the contract never lets a wrong answer
+    /// reach.
+    pub fn without_attestation_data_check(mut self) -> Self {
+        self.attestation_data_unchecked = true;
+        self
+    }
+
     /// Give `attestation_data` answers a nonzero `index`, the way a gloas node
     /// signals a full payload.
     pub fn with_attestation_index(mut self, index: u64) -> Self {
@@ -598,8 +610,11 @@ impl BeaconNodeApi for MockBeaconNode {
         // with data for another slot behaves like a node stuck on a stale
         // head: it errors, and failover moves past it. Without this the mock
         // would be a more permissive node than any real one, and tests built
-        // on it would not reflect what happens in production.
-        crate::beacon_node::validate_attestation_data(slot, &data)?;
+        // on it would not reflect what happens in production. Unless a test
+        // asks for exactly that, to reach the caller's own check.
+        if !self.attestation_data_unchecked {
+            crate::beacon_node::validate_attestation_data(slot, fork, &data)?;
+        }
         Ok(data)
     }