diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index 5372b8470..9de0b770b 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -135,7 +135,7 @@ impl AttestationService { // 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 @@ -763,6 +763,44 @@ 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. + #[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), + ); + 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 diff --git a/crates/validator/src/beacon_node/fallback.rs b/crates/validator/src/beacon_node/fallback.rs index f05107ab0..918386c50 100644 --- a/crates/validator/src/beacon_node/fallback.rs +++ b/crates/validator/src/beacon_node/fallback.rs @@ -501,6 +501,29 @@ mod tests { 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() { + // Electra fixed the index at 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, ForkName::Electra) + .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. /// diff --git a/crates/validator/src/beacon_node/http.rs b/crates/validator/src/beacon_node/http.rs index 013964c74..f32f39451 100644 --- a/crates/validator/src/beacon_node/http.rs +++ b/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) } diff --git a/crates/validator/src/beacon_node/mock.rs b/crates/validator/src/beacon_node/mock.rs index fe4a952b9..b5d46c907 100644 --- a/crates/validator/src/beacon_node/mock.rs +++ b/crates/validator/src/beacon_node/mock.rs @@ -599,7 +599,7 @@ impl BeaconNodeApi for MockBeaconNode { // 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)?; + crate::beacon_node::validate_attestation_data(slot, fork, &data)?; Ok(data) } diff --git a/crates/validator/src/beacon_node/mod.rs b/crates/validator/src/beacon_node/mod.rs index 49c1c54dd..5399d7ab3 100644 --- a/crates/validator/src/beacon_node/mod.rs +++ b/crates/validator/src/beacon_node/mod.rs @@ -32,8 +32,8 @@ pub mod http; #[cfg(test)] pub mod mock; -/// Check that `data` is an answer about `slot`, as the contract on -/// [`BeaconNodeApi::attestation_data`] requires. +/// Check that `data` is an answer about `slot`, which is in `fork`, as the +/// contract on [`BeaconNodeApi::attestation_data`] requires. /// /// One function rather than two copies, because it is enforced in two places /// for two different reasons and they must not drift apart. Each @@ -42,12 +42,21 @@ pub mod mock; /// before signing, since the trait is public and nothing stops an /// implementation from being wrong. /// -/// Both fields are checked because both are signable. `slot` is the obvious -/// one. `target.epoch` matters independently: it selects the signing domain -/// (see `SigningContext::attestation_signing_root`), so a node answering with -/// the right slot and a wrong target epoch is a second, distinct way to make -/// this client sign something it should not. -pub fn validate_attestation_data(slot: Slot, data: &AttestationData) -> Result<()> { +/// `slot` and `target.epoch` are checked because both are signable. `slot` is +/// the obvious one. `target.epoch` matters independently: it selects the +/// signing domain (see `SigningContext::attestation_signing_root`), so a node +/// answering with the right slot and a wrong target epoch is a second, +/// distinct way to make this client sign something it should not. +/// +/// `index` is checked for liveness rather than safety. A vote whose index +/// cannot be valid at `fork` is one every peer rejects (gossip's +/// `attestation.data.index == 0` from electra, `< 2` from gloas), and by +/// then slashing protection has recorded its target, so the validator has no +/// second vote to cast in that epoch. Refused here, nothing is recorded for +/// it, and the `Err` is one failover moves past to a node that may answer +/// properly. Lodestar's validator client refuses the same before signing +/// (`validateAttestationDuty`). +pub fn validate_attestation_data(slot: Slot, fork: ForkName, data: &AttestationData) -> Result<()> { if data.slot != slot { return Err(crate::error::Error::InconsistentResponse(format!( "requested attestation data for slot {slot}, node answered for slot {}", @@ -74,9 +83,51 @@ pub fn validate_attestation_data(slot: Slot, data: &AttestationData) -> Result<( data.source.epoch, data.target.epoch ))); } + let highest_index = highest_attestation_index(fork)?; + if data.index > highest_index { + return Err(crate::error::Error::InconsistentResponse(format!( + "attestation data for slot {slot} carries index {}, and a {} attestation's is at \ + most {highest_index}", + data.index, + fork.as_str() + ))); + } Ok(()) } +/// The highest `AttestationData.index` a vote at `fork` can carry. +/// +/// Every fork is named rather than compared against, so a fork added after +/// gloas has to say what its index means instead of inheriting gloas's +/// answer. +fn highest_attestation_index(fork: ForkName) -> Result { + match fork { + // A committee, and the one the query names: this client only ever + // asks about committee 0 (see `BeaconNodeApi::attestation_data`), so + // any other index is not an answer to its question. + ForkName::Phase0 + | ForkName::Altair + | ForkName::Bellatrix + | ForkName::Capella + | ForkName::Deneb => Ok(0), + // EIP-7549 moved the committee out of the signed data and left the + // field fixed at zero. + ForkName::Electra | ForkName::Fulu => Ok(0), + // The payload signal: 0 for a same-slot block or an empty one, 1 for + // a block whose payload was seen. Which of the two is right depends + // on the node's view of the block, which this client does not have, + // so only the range is checked. + ForkName::Gloas => Ok(1), + // Not a Beacon Chain fork: `Config::fork_at_epoch` never names it, so + // a caller that does is wrong, and nothing is signed for it. + ForkName::Lean => Err(crate::error::Error::InconsistentResponse( + "attestation data was asked for under the lean fork, which is not a Beacon Chain \ + fork and has no attestation of this shape" + .to_string(), + )), + } +} + /// The chain's genesis, as the beacon node reports it. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Genesis { @@ -339,6 +390,12 @@ pub trait BeaconNodeApi: Send + Sync { /// path: a target from the future would raise the validator's watermark /// and cost it every attestation until the chain caught up. /// + /// It must also reject an `index` that cannot be valid at `fork`: anything + /// but 0 before gloas, anything above 1 from it. Such a vote is rejected + /// by every peer, after slashing protection has spent the validator's one + /// vote for the epoch on it. [`validate_attestation_data`] is every one of + /// these checks. + /// /// It matters that this is the *implementation's* job rather than the /// caller's, because of where failover sits. /// [`fallback::FallbackBeaconNode`] wraps this method: a check performed @@ -609,21 +666,74 @@ mod tests { /// Slot 100 is in epoch 3 on the mainnet preset. #[test] fn attestation_data_with_a_source_at_or_past_its_target_is_refused() { - assert!(validate_attestation_data(100, &data(100, 2, 3)).is_ok()); - assert!(validate_attestation_data(100, &data(100, 0, 3)).is_ok()); - let err = validate_attestation_data(100, &data(100, 3, 3)).expect_err("source == target"); + let fork = ForkName::Fulu; + assert!(validate_attestation_data(100, fork, &data(100, 2, 3)).is_ok()); + assert!(validate_attestation_data(100, fork, &data(100, 0, 3)).is_ok()); + let err = + validate_attestation_data(100, fork, &data(100, 3, 3)).expect_err("source == target"); assert!(err.to_string().contains("source epoch 3"), "{err}"); - assert!(validate_attestation_data(100, &data(100, 4, 3)).is_err()); + assert!(validate_attestation_data(100, fork, &data(100, 4, 3)).is_err()); } /// At genesis both checkpoints are epoch 0, and that is the one valid /// shape where the source does not precede the target. #[test] fn genesis_attestation_data_is_accepted() { - assert!(validate_attestation_data(0, &data(0, 0, 0)).is_ok()); + let fork = ForkName::Phase0; + assert!(validate_attestation_data(0, fork, &data(0, 0, 0)).is_ok()); assert!( - validate_attestation_data(0, &data(0, 1, 0)).is_err(), + validate_attestation_data(0, fork, &data(0, 1, 0)).is_err(), "a source past a genesis target is not the genesis shape" ); } + + fn with_index(index: u64) -> AttestationData { + AttestationData { + index, + ..data(100, 2, 3) + } + } + + /// Before gloas the index is zero: the committee this client asks about + /// up to deneb, and EIP-7549's fixed zero from electra. Anything else is + /// a vote no peer accepts. + #[test] + fn a_nonzero_index_is_refused_before_gloas() { + let before_gloas = ForkName::ALL + .iter() + .copied() + .filter(|fork| *fork < ForkName::Gloas); + for fork in before_gloas { + assert!( + validate_attestation_data(100, fork, &with_index(0)).is_ok(), + "{fork:?} takes index 0" + ); + let err = validate_attestation_data(100, fork, &with_index(1)) + .expect_err("an index of 1 before gloas"); + assert!( + matches!(err, crate::error::Error::InconsistentResponse(_)), + "{fork:?}: failover must be able to move past it, got {err}" + ); + assert!(err.to_string().contains("carries index 1"), "{err}"); + } + } + + /// From gloas the index is the payload signal, 0 or 1, and passes through + /// as the node gave it. Only a value outside that range is refused. + #[test] + fn a_gloas_index_is_zero_or_one() { + let fork = ForkName::Gloas; + assert!(validate_attestation_data(100, fork, &with_index(0)).is_ok()); + assert!(validate_attestation_data(100, fork, &with_index(1)).is_ok()); + let err = validate_attestation_data(100, fork, &with_index(2)).expect_err("an index of 2"); + assert!(err.to_string().contains("at most 1"), "{err}"); + assert!(validate_attestation_data(100, fork, &with_index(u64::MAX)).is_err()); + } + + /// Lean is not on the Beacon Chain's fork timeline, so no index is valid + /// for it and nothing is signed. + #[test] + fn the_lean_fork_has_no_valid_index() { + assert!(validate_attestation_data(100, ForkName::Lean, &with_index(0)).is_err()); + } } diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index ae65aee92..0aacfe313 100644 --- a/crates/validator/src/error.rs +++ b/crates/validator/src/error.rs @@ -115,15 +115,17 @@ pub enum Error { #[error("slashing protection {path}: {reason}")] SlashingProtection { path: String, reason: String }, /// The record cannot be reached at all, as opposed to a record that - /// answered: its lock is poisoned because an earlier access panicked - /// while holding it, or the runtime is shutting down under the blocking - /// task. + /// answered: its lock is poisoned because an access panicked while + /// holding it, it stayed held past the wait, or the runtime is shutting + /// down under the blocking task. /// /// Kept apart from `SlashingProtection` and `Io` because the remedy /// differs: those name a file to look at, this one names a process to /// restart. A poisoned lock is a bug, never a disk, and it stays poisoned - /// for the life of the process, so every duty is refused until it is - /// restarted. Never retryable. + /// for the life of the process, so every duty is refused from then on + /// and the duty loop stops the client at the next slot, for its + /// supervisor to restart (see `slashing_protection::ensure_usable`). + /// Never retryable. #[error("slashing protection is unusable: {reason}")] SlashingProtectionUnusable { reason: String }, diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index c8356decb..72dbbe77e 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -407,31 +407,31 @@ pub async fn import( .map(|result| match (result, ®istration) { (Ok((_, secret, _)), Ok(_)) => finish_import(&mut store, &secret), (Ok((pubkey, _, _)), Err(err)) => { - if before.contains_key(&pubkey) { - // Its keystore was rewritten; its entry is back as it was - // and nothing about it changed in memory, so it stays - // whatever it was, loaded since startup in the ordinary - // case. - let entry = if rolled_back.is_ok() { - "its definitions entry is back as it was" - } else { - "its definitions entry could not be put back" - }; - KeyStatus::error(format!( + match (before.contains_key(&pubkey), rolled_back.is_ok()) { + // A key the file already listed, undone: its keystore, + // its password and its entry are what they were before + // the request, and nothing about it changed in memory, so + // it stays whatever it was, loaded since startup in the + // ordinary case. + (true, true) => KeyStatus::error(format!( + "not imported: its slashing-protection record could not be written: \ + {err}; its keystore and definitions entry are back as they were" + )), + // The undo failed as well, so what this request wrote + // over the key's files and entry may still be there. + (true, false) => KeyStatus::error(format!( "keystore rewritten, but the slashing-protection record could not be \ - written: {err}; {entry}" - )) - } else if rolled_back.is_err() { + written: {err}; its definitions entry could not be put back" + )), + (false, true) => KeyStatus::error(format!( + "not imported: its slashing-protection record could not be written: {err}" + )), // Still on disk, not loaded now, and refused at the next // start until it has a record. - KeyStatus::error(format!( + (false, false) => KeyStatus::error(format!( "saved, but not loaded: its slashing-protection record could not be \ written: {err}" - )) - } else { - KeyStatus::error(format!( - "not imported: its slashing-protection record could not be written: {err}" - )) + )), } } (Err(status), _) => status, @@ -1508,11 +1508,12 @@ mod tests { ) .await; assert_eq!(body["data"][0]["status"], "error", "got {body}"); + // Not "keystore rewritten": the undo put the files back too, so the + // message must not send the operator looking for a new keystore. + let message = body["data"][0]["message"].as_str().unwrap_or_default(); + assert!(message.starts_with("not imported"), "got {body}"); assert!( - body["data"][0]["message"] - .as_str() - .unwrap_or_default() - .contains("back as it was"), + message.ends_with("its keystore and definitions entry are back as they were"), "got {body}" ); let definitions = ValidatorDefinitions::open(dir.path()).expect("opens"); @@ -1637,9 +1638,12 @@ mod tests { assert_eq!(definitions.0, vec![previous]); } - /// A request that carries history is refused whole, importing nothing. + /// A request whose history does not parse as an interchange is refused + /// whole, importing nothing: `{}` has neither metadata nor data. History + /// that does parse is imported with its keys (see + /// `an_import_brings_its_slashing_protection_history`). #[tokio::test] - async fn an_import_carrying_history_is_refused_whole() { + async fn an_import_whose_history_does_not_parse_is_refused_whole() { let dir = tempfile::tempdir().expect("temp dir"); let context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); let store = context.store.clone(); @@ -1658,6 +1662,14 @@ mod tests { ) .await; assert_eq!(body["data"][0]["status"], "error", "got {body}"); + // Refused for the history, and for its shape: not for the keystore, + // and not for some later step. + let message = body["data"][0]["message"].as_str().unwrap_or_default(); + assert!( + message.starts_with("slashing_protection was not imported") + && message.contains("malformed"), + "got {body}" + ); assert!(store.read().await.pubkeys().is_empty()); assert!( ValidatorDefinitions::open(dir.path()) diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index 24fc99dfd..4b583c16a 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -265,6 +265,18 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { tokio::time::sleep(delay).await; served = Some(slot); + // A poisoned record refuses every duty until the process restarts, + // and nothing in the process restarts it. Carrying on would leave a + // client that answers `/health` and signs nothing, which no + // supervisor has a reason to touch; stopping hands systemd or + // Kubernetes the restart the refusals ask for. Asked once a slot, at + // the boundary, which is soon enough: in between, the record refuses + // every duty on its own. + if let Err(err) = slashing_protection::ensure_usable(&protection) { + error!(%slot, %err, "Slashing protection is unusable; stopping so the client can be restarted"); + return Err(err); + } + let epoch = clock.epoch_of(slot); if last_epoch != Some(epoch) { diff --git a/crates/validator/src/slashing_protection/file.rs b/crates/validator/src/slashing_protection/file.rs index 865a8237c..9018e4aba 100644 --- a/crates/validator/src/slashing_protection/file.rs +++ b/crates/validator/src/slashing_protection/file.rs @@ -42,10 +42,13 @@ pub const FILE_NAME: &str = "slashing_protection.json"; /// The only EIP-3076 interchange version this client reads or writes. pub const INTERCHANGE_VERSION: &str = "5"; -/// This client's own file, read strictly: a field it did not write is an -/// error, since nothing else should have written here. Not for reading -/// other clients' interchange files, which may carry fields this refuses; -/// an importer needs its own, lenient, types. +/// An EIP-3076 version 5 interchange: the shape of this client's own record +/// and of every file it imports. The record reader ([`read`]) and the +/// importer ([`parse`], which `SlashingProtection::stage` calls) share these +/// types, and both read strictly: a field the format does not define is an +/// error rather than ignored, as it is in Lighthouse's EIP-3076 types, so a +/// file that is not the interchange it claims to be is refused whole instead +/// of half read. #[derive(Debug, Serialize, Deserialize)] #[serde(deny_unknown_fields)] struct Interchange { diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 04f7c861d..d462d490e 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -96,9 +96,11 @@ const RECORD_POLL: std::time::Duration = std::time::Duration::from_millis(5); /// could not be reached at all (`Error::SlashingProtectionUnusable`): the /// lock is poisoned, because an operation panicked while holding it, and /// stays so until the process restarts, since nothing here recovers a record -/// that a panic may have left half-updated in memory. The panic that poisons -/// it is reported the same way on the call that raised it, which sees it as -/// the blocking task's `JoinError`. +/// that a panic may have left half-updated in memory. The call whose own +/// operation panicked sees it as the blocking task's `JoinError` and is told +/// so, with the panic's message; every later call is told an earlier access +/// panicked. The duty loop asks [`ensure_usable`] once a slot and stops the +/// client on a poisoned record, so the restart comes from its supervisor. /// /// The operation runs to completion even if the caller is dropped, as a duty /// abandoned at its deadline is: that is safe, since a record written for a @@ -113,11 +115,6 @@ pub async fn blocking( protection: &SharedProtection, operation: impl FnOnce(&mut SlashingProtection) -> Result + Send + 'static, ) -> Result { - let poisoned = || Error::SlashingProtectionUnusable { - reason: "an earlier access panicked while holding the record, so nothing is signed \ - until the validator client is restarted" - .to_string(), - }; let protection = protection.clone(); tokio::task::spawn_blocking(move || { // A bounded wait rather than `lock()`: a write normally holds the @@ -129,7 +126,7 @@ pub async fn blocking( let mut protection = loop { match protection.try_lock() { Ok(guard) => break guard, - Err(std::sync::TryLockError::Poisoned(_)) => return Err(poisoned()), + Err(std::sync::TryLockError::Poisoned(_)) => return Err(poisoned_earlier()), Err(std::sync::TryLockError::WouldBlock) => { if std::time::Instant::now() >= deadline { return Err(Error::SlashingProtectionUnusable { @@ -148,16 +145,58 @@ pub async fn blocking( }) .await .map_err(|err| { - if err.is_panic() { - poisoned() - } else { - Error::SlashingProtectionUnusable { - reason: format!("the record could not be consulted: {err}"), - } - } + let reason = match err.try_into_panic() { + // This call's own operation panicked, and it held the record when + // it did: nothing else in the task can panic. Said as this + // access's failure, with the panic's message, since "an earlier + // access" would send whoever reads it looking for another event. + Ok(panic) => format!( + "this access panicked while holding the record ({}), so nothing is signed until \ + the validator client is restarted", + panic_message(panic.as_ref()) + ), + Err(err) => format!("the record could not be consulted: {err}"), + }; + Error::SlashingProtectionUnusable { reason } })? } +/// What a caller is told when it finds the record's lock already poisoned. +fn poisoned_earlier() -> Error { + Error::SlashingProtectionUnusable { + reason: "an earlier access panicked while holding the record, so nothing is signed \ + until the validator client is restarted" + .to_string(), + } +} + +/// A panic's message, when it carried one: `panic!` with a literal gives a +/// `&str` and with arguments a `String`. Anything else was raised with +/// `panic_any` and has no text to show. +fn panic_message(panic: &(dyn std::any::Any + Send)) -> &str { + panic + .downcast_ref::<&'static str>() + .copied() + .or_else(|| panic.downcast_ref::().map(String::as_str)) + .unwrap_or("no message") +} + +/// Refuse to go on with a record whose lock is poisoned. +/// +/// A poisoned record refuses every duty and never recovers (see +/// [`blocking`]), so a client left running on one looks alive to whatever +/// supervises it while signing nothing. The duty loop asks this once a slot +/// and stops on an `Err`, which ends the process with a failure status: +/// systemd or Kubernetes then restarts it, and the restart reads the record +/// back from disk, which every write either replaced whole or left alone. +/// That is the remedy the refusals ask for. +pub fn ensure_usable(protection: &SharedProtection) -> Result<()> { + if protection.is_poisoned() { + return Err(poisoned_earlier()); + } + Ok(()) +} + /// Whether an attestation's source epoch is at or past its target, which is /// never valid outside genesis: with unsigned epochs, `(0, 0)` is the only /// such pair that is. One predicate for the three places that refuse it, @@ -1459,6 +1498,31 @@ mod tests { assert!(propose(&mut fresh, 1, 97).is_err()); } + /// An export writes nothing to the validators directory, and the lock is + /// an `flock` on the directory itself, which a read-only descriptor can + /// take: keys mounted read-only can still hand their history over. + #[cfg(unix)] + #[test] + fn an_export_needs_no_writable_directory() { + use std::os::unix::fs::PermissionsExt as _; + + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + propose(&mut protection, 1, 97).expect("propose"); + drop(protection); + + let read_only = std::fs::Permissions::from_mode(0o500); + std::fs::set_permissions(dir.path(), read_only).expect("chmod"); + let exported = SlashingProtection::open_offline(dir.path(), None) + .and_then(|protection| protection.export(&[pubkey(1)])); + let writable = std::fs::Permissions::from_mode(0o700); + std::fs::set_permissions(dir.path(), writable).expect("chmod back"); + + let exported = exported.expect("locks, reads and exports with no write access"); + let json: serde_json::Value = serde_json::from_str(&exported).expect("json"); + assert_eq!(json["data"][0]["signed_blocks"][0]["slot"], "97"); + } + #[test] fn opening_offline_needs_a_record_or_a_chain_to_create_one_for() { let dir = tempfile::tempdir().expect("temp dir"); @@ -1517,4 +1581,67 @@ mod tests { Err(Refusal::Unregistered) ); } + + // A poisoned record. + + /// The duty loop's once-a-slot question: a healthy record lets it go on, + /// a poisoned one stops the client, and the error is not one to retry. + #[test] + fn a_poisoned_record_stops_the_client() { + let protection = SlashingProtection::in_memory(); + ensure_usable(&protection).expect("a healthy record is usable"); + + // Poisoned the way a bug would: a panic while the lock is held. + let held = protection.clone(); + std::thread::spawn(move || { + let _guard = held.lock().expect("not poisoned yet"); + panic!("a bug while holding the record"); + }) + .join() + .expect_err("the thread panics"); + + let err = ensure_usable(&protection).expect_err("a poisoned record is not"); + assert!( + matches!(err, Error::SlashingProtectionUnusable { .. }), + "got {err}" + ); + assert!(!err.is_retryable(), "the loop must stop, not try again"); + } + + /// The access whose own operation panics is told it was its panic, with + /// the message, and only the ones after it that an earlier access + /// panicked: the same wording for both points the first at an event that + /// never happened. + #[tokio::test] + async fn the_access_that_panics_is_told_so_and_later_ones_that_an_earlier_one_did() { + let protection = SlashingProtection::in_memory(); + + let own = blocking(&protection, |_| -> Result<()> { + panic!("a bug in the record") + }) + .await + .expect_err("the operation panicked") + .to_string(); + assert!(own.contains("this access panicked"), "{own}"); + assert!(own.contains("a bug in the record"), "{own}"); + + let later = blocking(&protection, |_| Ok(())) + .await + .expect_err("the record is poisoned") + .to_string(); + assert!(later.contains("an earlier access panicked"), "{later}"); + ensure_usable(&protection).expect_err("and the duty loop stops on it"); + } + + /// A panic raised with arguments carries a `String`, not a `&str`, and + /// one raised with `panic_any` carries neither. + #[test] + fn a_panics_message_is_read_whatever_it_was_raised_with() { + let literal: Box = Box::new("a literal"); + assert_eq!(panic_message(literal.as_ref()), "a literal"); + let formatted: Box = Box::new(format!("slot {}", 7)); + assert_eq!(panic_message(formatted.as_ref()), "slot 7"); + let neither: Box = Box::new(7_u64); + assert_eq!(panic_message(neither.as_ref()), "no message"); + } } diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs index a338d8b73..4823dd51d 100644 --- a/crates/validator/tests/interchange_vectors.rs +++ b/crates/validator/tests/interchange_vectors.rs @@ -128,9 +128,17 @@ fn run(case: &Case) -> Result<(), String> { // The README lets an importer refuse an interchange holding // slashable data, in which case the rest of the case is skipped. // This one absorbs all of it but an attestation whose source is at - // or past its target, so a refusal on such a step ends the case, - // as it does in Lighthouse; on any other step it is a failure. - (false, true) if step.contains_slashable_data => return Ok(()), + // or past its target, so only a step whose interchange holds one + // may be refused, which ends the case, as it does in Lighthouse. + // Every other slashable step must import: skipping on the flag + // alone would pass a suite whose importer had started refusing, + // say, a duplicated pubkey or a surround, with that case's blocks + // and attestations never attempted. + (false, true) + if step.contains_slashable_data && holds_impossible_source(&step.interchange) => + { + return Ok(()); + } (false, true) => { return Err(format!( "step {index}: import failed but should succeed (slashable data: {}): {}", @@ -182,6 +190,26 @@ fn run(case: &Case) -> Result<(), String> { Ok(()) } +/// Whether `interchange` holds an attestation whose source is at or past its +/// target outside genesis: the one shape this importer refuses rather than +/// absorbs. The rule is restated here rather than borrowed from the crate, so +/// that the importer widening what it refuses cannot widen what this runner +/// forgives along with it. +fn holds_impossible_source(interchange: &serde_json::Value) -> bool { + let epoch = + |value: &serde_json::Value| value.as_str().and_then(|text| text.parse::().ok()); + interchange["data"] + .as_array() + .into_iter() + .flatten() + .flat_map(|entry| entry["signed_attestations"].as_array().into_iter().flatten()) + .any(|attestation| { + let source = epoch(&attestation["source_epoch"]); + let target = epoch(&attestation["target_epoch"]); + matches!((source, target), (Some(source), Some(target)) if source >= target && source != 0) + }) +} + /// Only the epochs are judged; the rest of the data is filler. fn data(source: u64, target: u64) -> AttestationData { AttestationData { diff --git a/docs/cli.md b/docs/cli.md index e77aa426a..172d96c8b 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -277,7 +277,12 @@ implementation's, so none of the [common flags](#common-flags) apply to it. > regular file, since a dangling one would read as "no record"; and > the directory should be a local disk, since every signature waits for the > record to be flushed to it, and the lock that keeps a second process off it -> is an `flock`, which a network filesystem may not honour. Unix only. A key with no record is refused at startup unless +> is an `flock`, which a network filesystem may not honour. Unix only. Run the +> client under a supervisor that restarts it (systemd's `Restart=on-failure`, +> a Kubernetes pod): if a bug ever panics while the record is held, the record +> is unusable for the rest of that process, every duty is refused, and the +> client exits with a failure at the next slot instead of idling, so that the +> restart rereads the record from disk. A key with no record is refused at startup unless > `--init-slashing-protection` is passed. A key moving here from another client > brings its history with [`slashing-protection import`](#slashing-protection), > or in the same keymanager API request that imports the key. See @@ -311,8 +316,12 @@ advance so it can collect the votes. Each slot it wakes at the boundary. If one of its validators proposes that slot, it signs the RANDAO reveal, asks the beacon node for a block, checks that the block is for the slot and proposer it asked about, signs it and publishes -it. One third into the slot it fetches the attestation data, signs for every -validator due that slot, and submits the batch. Two thirds in, for any duty it +it. One third into the slot it fetches the attestation data, checks that it is +for the slot it asked about and that its `index` is one the slot's fork allows +(0 before gloas, 0 or 1 from it, where it is the payload signal), signs for +every validator due that slot, and submits the batch. A node whose answer fails +either check is passed over for the next one in `--beacon-nodes`, before +anything is recorded or signed. Two thirds in, for any duty it was selected to aggregate, it asks the node for the aggregate covering that committee's votes on the data it just signed, wraps it with the selection proof and publishes it. @@ -474,7 +483,7 @@ ethlambda slashing-protection export --validators-dir ./validators --pubkeys 0xa | Sub-command | Does | |---|---| | `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused, except an attestation whose source is at or past its target outside genesis, which is refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing; the client refuses the record at its next start if that is not its beacon node's chain | -| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record fails the export, so a typo or the wrong directory cannot produce a file that reads as complete. Takes the record's lock, so the directory must be writable. Refuses to overwrite an existing file | +| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record fails the export, so a typo or the wrong directory cannot produce a file that reads as complete. Writes nothing to the directory, and the lock is on the directory itself, so a read-only one will do. Refuses to overwrite an existing file | ## `benchmark import` flags