From 4b93df7bce8de9f24ed96d77a13d8c982fc80725 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 29 Oct 2026 18:14:38 -0300 Subject: [PATCH 1/3] Make the validator client's slashing protection durable: every block and attestation is checked against a per-validator record of EIP-3076 minimal watermarks (highest block slot, highest attestation source and target) that is written to disk before the signature exists. The record lives in /slashing_protection.json as an EIP-3076 v5 minimal interchange bound to the chain's genesis_validators_root, is written through a temporary file, fsync, rename and directory fsync, and is locked to one process. Reading fails closed, a failed write signs nothing, and a key with no record is refused at startup unless --init-slashing-protection is passed. Keymanager imports register a key before loading it. This replaces the in-memory AttestationGuard and ProposalGuard, whose rule and tests move into the new slashing_protection module, and removes the startup warning that the client kept no record. --- CLAUDE.md | 6 +- README.md | 15 +- bin/ethlambda/src/validator.rs | 14 + .../rpc/src/beacon/validator_client_tests.rs | 2 + crates/validator/Cargo.toml | 9 +- crates/validator/src/attestation.rs | 311 +++++-- crates/validator/src/attestation_guard.rs | 377 -------- crates/validator/src/beacon_node/mod.rs | 5 +- crates/validator/src/error.rs | 10 + crates/validator/src/http_api/keystores.rs | 80 +- crates/validator/src/http_api/mod.rs | 3 + crates/validator/src/http_api/proposer.rs | 1 + crates/validator/src/lib.rs | 100 ++- crates/validator/src/metrics.rs | 59 +- crates/validator/src/proposal.rs | 185 ++-- crates/validator/src/proposal_guard.rs | 271 ------ crates/validator/src/signing.rs | 5 +- .../validator/src/slashing_protection/file.rs | 312 +++++++ .../validator/src/slashing_protection/mod.rs | 848 ++++++++++++++++++ docs/cli.md | 13 +- docs/metrics.md | 6 +- docs/spec_deviations.md | 85 +- 22 files changed, 1780 insertions(+), 937 deletions(-) delete mode 100644 crates/validator/src/attestation_guard.rs delete mode 100644 crates/validator/src/proposal_guard.rs create mode 100644 crates/validator/src/slashing_protection/file.rs create mode 100644 crates/validator/src/slashing_protection/mod.rs diff --git a/CLAUDE.md b/CLAUDE.md index a171704a8..d08ee404f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -846,8 +846,10 @@ transitions are in `ethlambda-types`, per the section above. Nothing above aggregate when `verified_sync_aggregate` fails (see `docs/spec_deviations.md`). `SlotClock` picks the offsets by each slot's own fork, and `/eth/v1/config/spec` supplies them. It - still keeps no slashing-protection record; PTC votes have an in-memory - `(validator, slot)` dedup only, and envelopes are unguarded. + keeps EIP-3076 minimal-watermark slashing protection in + `--validators-dir/slashing_protection.json` (`crate::slashing_protection`), + durable before every block and attestation signature; PTC votes have an + in-memory `(validator, slot)` dedup only, and envelopes are unguarded. `get_head_node` is one bottom-up walk for every fork, with the current slot's fork selecting the payload rules and the boost gate, so a follower on any network that schedules gloas (Sepolia does) pays work diff --git a/README.md b/README.md index 2b3457da7..181915db2 100644 --- a/README.md +++ b/README.md @@ -332,13 +332,14 @@ payload envelope and the payload timeliness vote. It talks to a beacon node over the standard Beacon API, so it works against `ethlambda beacon` (run with the `--execution-*` flags) or any other conformant beacon node. -> **Warning:** the validator client keeps no slashing-protection record yet. -> Slashing protection is a work in progress and will be added soon. Until then, -> do not run the same keys in another client while this one runs, and treat a -> restart with the same care as a manual key move. Read -> [Spec deviations](./docs/spec_deviations.md#the-validator-client-keeps-no-slashing-protection-record) -> before using keys that hold real stake. The client repeats this warning at -> startup. +> **Slashing protection:** every block and attestation is checked against a +> durable per-validator record in `--validators-dir` before it is signed, and a +> second validator client pointed at the same directory refuses to start. A key +> with no record is refused at startup: pass `--init-slashing-protection` once, +> and only for keys that have signed nothing anywhere for at least two epochs. +> Importing history from another client is not supported yet, so never run +> these keys in another client at the same time. See +> [Spec deviations](./docs/spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). ```sh ./target/release/ethlambda validator \ diff --git a/bin/ethlambda/src/validator.rs b/bin/ethlambda/src/validator.rs index f97f82782..16aefe31c 100644 --- a/bin/ethlambda/src/validator.rs +++ b/bin/ethlambda/src/validator.rs @@ -78,6 +78,18 @@ pub(crate) struct ValidatorOptions { /// beyond localhost must front it with TLS. #[arg(long, default_value = "false")] pub(crate) enable_keymanager: bool, + + /// Register every loaded key that has no slashing-protection record as + /// never having signed, instead of refusing to start. + /// + /// DANGEROUS. Use it only for keys that are brand new, or that have signed + /// nothing anywhere for at least two epochs. A key that recently signed in + /// another client must bring that history instead: starting it here as + /// "never signed" can produce a slashable double vote. Keys that already + /// have a record keep it, so the flag never lowers one; remove it once the + /// keys are registered. + #[arg(long, default_value = "false")] + pub(crate) init_slashing_protection: bool, } impl ValidatorOptions { @@ -154,6 +166,7 @@ pub(crate) async fn run(options: ValidatorOptions) -> eyre::Result<()> { keymanager: options .enable_keymanager .then(|| std::net::SocketAddr::new(options.http_address, options.keymanager_port)), + init_slashing_protection: options.init_slashing_protection, }) .await .map_err(Into::into) @@ -174,6 +187,7 @@ mod tests { graffiti: graffiti.to_string(), suggested_fee_recipient: None, enable_keymanager: false, + init_slashing_protection: false, } } diff --git a/crates/net/rpc/src/beacon/validator_client_tests.rs b/crates/net/rpc/src/beacon/validator_client_tests.rs index 693450ebf..b62b61cdc 100644 --- a/crates/net/rpc/src/beacon/validator_client_tests.rs +++ b/crates/net/rpc/src/beacon/validator_client_tests.rs @@ -1488,6 +1488,7 @@ mod gloas { None, ), ), + ethlambda_validator::slashing_protection::SlashingProtection::in_memory(), ); let duty = ProposerDutyDto { pubkey: encode_hex(&pubkey.0), @@ -1606,6 +1607,7 @@ mod gloas { None, ), ), + ethlambda_validator::slashing_protection::SlashingProtection::in_memory(), ); let duty = ProposerDutyDto { pubkey: encode_hex(&pubkey.0), diff --git a/crates/validator/Cargo.toml b/crates/validator/Cargo.toml index c227e995f..fad9b8712 100644 --- a/crates/validator/Cargo.toml +++ b/crates/validator/Cargo.toml @@ -26,10 +26,11 @@ serde_yaml_ng.workspace = true hex.workspace = true thiserror.workspace = true tracing.workspace = true -# `time` for the duty loop's sleep between slots. 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"] } +# `time` for the duty loop's sleep between slots, `rt` for `spawn_blocking`, +# which keeps the slashing-protection record's disk flush off the executor. +# 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"] } reqwest = { workspace = true, features = ["json"] } # BLS12-381. Same version and backend the state transition verifies with, so a diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index a5984e590..50189d6df 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -3,12 +3,11 @@ use std::sync::Arc; use ethlambda_types::beacon::containers::shared::AttestationData; -use ethlambda_types::beacon::primitives::Slot; +use ethlambda_types::beacon::primitives::{BlsPubkey, Slot}; use ethlambda_types::beacon::signing::compute_epoch_at_slot; use tokio::sync::RwLock; use tracing::{error, info, warn}; -use crate::attestation_guard::AttestationGuard; use crate::beacon_node::dto::{ AttestationDataOutDto, AttesterDutyDto, SingleAttestationDto, encode_hex, parse_pubkey, }; @@ -16,6 +15,7 @@ use crate::beacon_node::{BeaconNodeApi, validate_attestation_data}; use crate::error::Result; use crate::keys::ValidatorStore; use crate::signing::SigningContext; +use crate::slashing_protection::SharedProtection; /// What one slot's attestation duty produced. /// @@ -43,25 +43,21 @@ pub struct Attested { pub struct AttestationService { beacon_node: Arc, context: Arc, - /// What this process has already signed, per validator. - /// - /// A `std::sync::Mutex` rather than a `tokio` one on purpose: the critical - /// section is a hash lookup and an insert with no `.await` inside it, so an - /// async mutex would buy nothing and cost a scheduling point. The guard is - /// taken and dropped per validator inside the signing loop, which already - /// holds the store's read lock, so it must not be the thing that blocks. - /// - /// Not slashing protection. See [`AttestationGuard`] for exactly what it - /// does and does not cover. - guard: std::sync::Mutex, + /// What every validator has signed, shared with the proposal service and + /// the keymanager. See [`crate::slashing_protection`]. + protection: SharedProtection, } impl AttestationService { - pub fn new(beacon_node: Arc, context: Arc) -> Self { + pub fn new( + beacon_node: Arc, + context: Arc, + protection: SharedProtection, + ) -> Self { Self { beacon_node, context, - guard: std::sync::Mutex::new(AttestationGuard::new()), + protection, } } @@ -80,9 +76,9 @@ impl AttestationService { /// On a submission failure every signature in the batch has already been /// produced. Calling this again re-fetches the attestation data, and if the /// head moved in between it signs a *different* message for the same - /// validator and slot. This client keeps no slashing-protection record, so - /// nothing would catch that, and two distinct attestations for one slot from - /// one validator is a slashable offence. + /// validator and slot. Slashing protection refuses it, since the first call + /// already recorded that target, so a retry here publishes nothing: the + /// attestations are lost rather than doubled. /// /// A caller that wants to retry must resubmit the batch this call already /// built, not call this again. Today's duty loop does neither: it logs the @@ -142,6 +138,49 @@ impl AttestationService { let data_dto = AttestationDataOutDto::from(&data); + let candidates: Vec<(&AttesterDutyDto, BlsPubkey)> = duties + .iter() + .filter_map(|duty| match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => Some((duty, pubkey)), + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); + None + } + }) + .collect(); + + // Check and durably record the whole batch before any signature exists: + // a signature that exists is a signature that can escape, whatever the + // code after it intended. One write for the slot, off the executor, + // and before the store's read lock below is taken, so the disk flush + // never holds up a keymanager writer queued behind it. + let pubkeys: Vec = candidates.iter().map(|(_, pubkey)| *pubkey).collect(); + let protection = self.protection.clone(); + let recorded_data = data; + let verdicts = tokio::task::spawn_blocking(move || { + let mut protection = protection + .lock() + .map_err(|_| "slashing protection is poisoned".to_string())?; + protection + .check_and_record_attestations(&pubkeys, &recorded_data) + .map_err(|err| err.to_string()) + }) + .await + .map_err(|err| err.to_string()) + .and_then(|verdicts| verdicts); + let verdicts = match verdicts { + Ok(verdicts) => verdicts, + Err(err) => { + // Nothing was recorded, so nothing may be signed. + error!(%slot, %err, "Slashing protection could not record this slot's attestations; refusing to sign any"); + crate::metrics::inc_attestations_refused(); + return Ok(Attested { + published: 0, + data: Some(data), + }); + } + }; + // The read guard is scoped to this block alone and must never be widened to cover an // await. `tokio::sync::RwLock` is write-preferring, so a keymanager // import or delete queuing for the write lock blocks every read behind @@ -149,50 +188,20 @@ impl AttestationService { // import holds its own write lock for the inserts alone, doing the slow // EIP-2335 derivation outside it (see `http_api::keystores::import`), // so this is the duty path keeping its half of the same bargain. - let mut attestations = Vec::with_capacity(duties.len()); + let mut attestations = Vec::with_capacity(candidates.len()); { let _timing = crate::metrics::time_signing(); let store = store.read().await; - for duty in duties { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - - // Refuse before signing, not after: a signature that exists is - // a signature that can escape, whatever the code after it - // intended. This closes the double-vote shapes reachable - // within one run (a backward clock step, a schedule replaced - // mid-epoch); it is not slashing protection and does not - // pretend to be. See `AttestationGuard`. - // - // Held per validator, and dropped before the next iteration. - // The lock is poisoned only by a panic while it is held, and - // nothing inside can panic; `unwrap` on it would still be a - // crash in the duty path, so a poisoned lock is treated as a - // refusal for every remaining validator instead. - match self.guard.lock() { - Ok(mut guard) => { - if let Err(refusal) = guard.check_and_record(&pubkey, &data) { - warn!( - %slot, - validator = duty.validator_index, - %refusal, - "Refusing to sign: this process already signed a conflicting \ - attestation for this validator" - ); - crate::metrics::inc_attestations_refused(); - continue; - } - } - Err(err) => { - error!(%slot, %err, "Attestation guard is poisoned; refusing to sign"); - crate::metrics::inc_attestations_refused(); - continue; - } + for ((duty, pubkey), verdict) in candidates.into_iter().zip(verdicts) { + if let Err(refusal) = verdict { + warn!( + %slot, + validator = duty.validator_index, + %refusal, + "Refusing to sign: slashing protection judged this attestation unsafe" + ); + crate::metrics::inc_attestations_refused(); + continue; } // One validator failing must not cost the others their attestation. @@ -250,6 +259,7 @@ mod tests { // error, since `validate_attestation_data` produces them now. use crate::beacon_node::mock::MockBeaconNode; use crate::error::Error; + use crate::slashing_protection::SlashingProtection; use ethlambda_types::beacon::config::Config; use ethlambda_types::beacon::containers::shared::{AttestationData, Checkpoint}; use ethlambda_types::beacon::preset; @@ -268,8 +278,8 @@ mod tests { /// one key used twice. A pubkey resolves to exactly one validator index on /// chain, and `ValidatorStore` is keyed by pubkey so a duplicate collapses, /// so one key under two indices is a shape that cannot occur. It also now - /// trips `AttestationGuard`, correctly: two duties for one validator in one - /// epoch is the double-vote shape the guard exists to refuse. + /// trips slashing protection, correctly: two duties for one validator in + /// one epoch is the double-vote shape it exists to refuse. fn other_secret() -> [u8; 32] { let mut bytes = secret(); // Perturb a low byte: still a valid scalar, comfortably below the @@ -311,7 +321,8 @@ mod tests { .expect("inserts"); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![ duty(&encode_hex(&first.0), 1337, 96, 3), @@ -346,7 +357,8 @@ mod tests { let pubkey = store.insert_secret("test", &secret()).expect("inserts"); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![duty(&encode_hex(&pubkey.0), 1337, 96, 3)]; let store = RwLock::new(store); @@ -366,6 +378,80 @@ mod tests { ); } + /// A record kept in `dir`, with every key in `keys` registered. + fn persisted( + dir: &std::path::Path, + 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)) + } + + /// The restart shape the old in-memory guard could not cover: a fresh + /// service over the same directory still refuses the double vote, and + /// reaches the beacon node with nothing. + #[tokio::test] + async fn a_restart_does_not_forget_what_was_attested() { + let mut store = ValidatorStore::new(); + let pubkey = store.insert_secret("test", &secret()).expect("inserts"); + let store = RwLock::new(store); + let duties = vec![duty(&encode_hex(&pubkey.0), 1337, 96, 3)]; + let dir = tempfile::tempdir().expect("temp dir"); + + let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); + let first = + AttestationService::new(node.clone(), context(), persisted(dir.path(), &[pubkey])) + .attest(96, &duties, &store) + .await + .expect("attests"); + assert_eq!(first.published, 1); + + // The first service, and with it the lock on the record, is gone. + let node_again = Arc::new(MockBeaconNode::new().with_attestation_data(96)); + let second = AttestationService::new( + node_again.clone(), + context(), + persisted(dir.path(), &[pubkey]), + ) + .attest(96, &duties, &store) + .await + .expect("not an error, simply signs nothing"); + assert_eq!( + second.published, 0, + "the double vote must be refused after a restart" + ); + assert!(node_again.submitted().is_empty()); + } + + /// A record that cannot be written means nothing is signed: the write is + /// what makes a signature safe to exist. + #[cfg(unix)] + #[tokio::test] + async fn a_record_that_cannot_be_written_signs_nothing() { + use std::os::unix::fs::PermissionsExt as _; + + let mut store = ValidatorStore::new(); + let pubkey = store.insert_secret("test", &secret()).expect("inserts"); + let store = RwLock::new(store); + let dir = tempfile::tempdir().expect("temp dir"); + let protection = persisted(dir.path(), &[pubkey]); + + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o500)) + .expect("chmod"); + let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); + let attested = AttestationService::new(node.clone(), context(), protection) + .attest(96, &[duty(&encode_hex(&pubkey.0), 1337, 96, 3)], &store) + .await; + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)) + .expect("chmod back"); + + assert_eq!(attested.expect("not an error").published, 0); + assert!( + node.submitted().is_empty(), + "nothing may reach the beacon node" + ); + } + /// The mid-epoch schedule replacement shape: the same validator moved to a /// different slot within one epoch. Both slots are in epoch 3, so the /// second is a double vote even though the slot differs. @@ -377,7 +463,8 @@ mod tests { // 96 and 101 are both in epoch 3 (96 / 32 == 101 / 32 == 3). let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let store = RwLock::new(store); let first = service @@ -387,12 +474,12 @@ mod tests { assert_eq!(first.published, 1); let node_again = Arc::new(MockBeaconNode::new().with_attestation_data(101)); - // Same service, so the same guard; a new mock only because the mock + // Same slashing-protection record; a new mock only because the mock // pins one slot's data at a time. let service = AttestationService { beacon_node: node_again.clone(), context: context(), - guard: service.guard, + protection: service.protection, }; let second = service .attest(101, &[duty(&pubkey_hex, 1337, 101, 3)], &store) @@ -420,7 +507,8 @@ mod tests { .expect("inserts"); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![ duty(&encode_hex(&first.0), 1337, 96, 3), @@ -446,7 +534,8 @@ mod tests { let known = store.insert_secret("test", &secret()).expect("inserts"); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![ duty(&encode_hex(&[0x99; 48]), 1, 96, 3), @@ -467,7 +556,8 @@ mod tests { // after the signing loop, where the loop produced nothing to send. let store = RwLock::new(ValidatorStore::new()); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![ duty(&encode_hex(&[0x99; 48]), 1, 96, 3), @@ -494,7 +584,7 @@ mod tests { let pubkey = store.insert_secret("test", &secret()).expect("inserts"); let store = RwLock::new(store); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node, context()); + let service = AttestationService::new(node, context(), SlashingProtection::in_memory()); let attested = service .attest(96, &[duty(&encode_hex(&pubkey.0), 1337, 96, 3)], &store) @@ -511,7 +601,11 @@ mod tests { /// aggregate of an attestation nobody made. #[tokio::test] async fn no_duties_means_no_data_to_aggregate_against() { - let service = AttestationService::new(Arc::new(MockBeaconNode::new()), context()); + let service = AttestationService::new( + Arc::new(MockBeaconNode::new()), + context(), + SlashingProtection::in_memory(), + ); let attested = service .attest(96, &[], &RwLock::new(ValidatorStore::new())) .await @@ -522,7 +616,8 @@ mod tests { #[tokio::test] async fn no_duties_means_no_request_at_all() { let node = Arc::new(MockBeaconNode::new()); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let published = service .attest(96, &[], &RwLock::new(ValidatorStore::new())) .await @@ -534,7 +629,7 @@ mod tests { #[tokio::test] async fn a_failing_attestation_data_fetch_is_reported() { let node = Arc::new(MockBeaconNode::new()); - let service = AttestationService::new(node, context()); + let service = AttestationService::new(node, context(), SlashingProtection::in_memory()); let mut store = ValidatorStore::new(); let pubkey = store.insert_secret("test", &secret()).expect("inserts"); let store = RwLock::new(store); @@ -558,7 +653,8 @@ mod tests { let pubkey_hex = encode_hex(&pubkey.0); let node = Arc::new(MockBeaconNode::new().with_attestation_data(96)); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![duty(&pubkey_hex, 1337, 97, 3)]; let err = service @@ -601,7 +697,8 @@ mod tests { }, }); let node = Arc::new(node); - let service = AttestationService::new(node.clone(), context()); + let service = + AttestationService::new(node.clone(), context(), SlashingProtection::in_memory()); let duties = vec![duty(&pubkey_hex, 1337, 96, 3)]; let err = service @@ -632,24 +729,32 @@ mod tests { let first_altair_slot = altair_fork_epoch * preset::SLOTS_PER_EPOCH; let phase0_node = Arc::new(MockBeaconNode::new().with_attestation_data(last_phase0_slot)); - AttestationService::new(phase0_node.clone(), context()) - .attest( - last_phase0_slot, - &[duty(&pubkey_hex, 1337, last_phase0_slot, 3)], - &store, - ) - .await - .expect("attests"); + AttestationService::new( + phase0_node.clone(), + context(), + SlashingProtection::in_memory(), + ) + .attest( + last_phase0_slot, + &[duty(&pubkey_hex, 1337, last_phase0_slot, 3)], + &store, + ) + .await + .expect("attests"); let altair_node = Arc::new(MockBeaconNode::new().with_attestation_data(first_altair_slot)); - AttestationService::new(altair_node.clone(), context()) - .attest( - first_altair_slot, - &[duty(&pubkey_hex, 1337, first_altair_slot, 3)], - &store, - ) - .await - .expect("attests"); + AttestationService::new( + altair_node.clone(), + context(), + SlashingProtection::in_memory(), + ) + .attest( + first_altair_slot, + &[duty(&pubkey_hex, 1337, first_altair_slot, 3)], + &store, + ) + .await + .expect("attests"); assert_eq!( phase0_node.last_submitted_fork_name().as_deref(), @@ -688,10 +793,14 @@ mod tests { .with_attestation_data(slot) .with_attestation_index(1), ); - let attested = AttestationService::new(node.clone(), gloas_context()) - .attest(slot, &[duty(&pubkey_hex, 1337, slot, 3)], &store) - .await - .expect("attests"); + let attested = AttestationService::new( + node.clone(), + gloas_context(), + SlashingProtection::in_memory(), + ) + .attest(slot, &[duty(&pubkey_hex, 1337, slot, 3)], &store) + .await + .expect("attests"); assert_eq!(attested.data.expect("data").index, 1); let submitted = node.submitted(); @@ -744,10 +853,14 @@ mod tests { let slot = 9 * preset::SLOTS_PER_EPOCH + 3; let node = Arc::new(MockBeaconNode::new().with_attestation_data(slot)); - AttestationService::new(node.clone(), gloas_context()) - .attest(slot, &[duty(&pubkey_hex, 1337, slot, 3)], &store) - .await - .expect("attests"); + AttestationService::new( + node.clone(), + gloas_context(), + SlashingProtection::in_memory(), + ) + .attest(slot, &[duty(&pubkey_hex, 1337, slot, 3)], &store) + .await + .expect("attests"); assert_eq!(node.attestation_data_forks(), vec![ForkName::Phase0]); } } diff --git a/crates/validator/src/attestation_guard.rs b/crates/validator/src/attestation_guard.rs deleted file mode 100644 index 5a5ebc3e4..000000000 --- a/crates/validator/src/attestation_guard.rs +++ /dev/null @@ -1,377 +0,0 @@ -//! An in-process guard against signing two conflicting attestations. -//! -//! # What this is not -//! -//! **This is not slashing protection.** It holds no history on disk, so it -//! knows nothing about what a previous run of this process signed, and nothing -//! about what another process holding the same keys is signing right now. A -//! restart empties it. Its records are gone the moment the process is. -//! -//! This client keeps no signing history by design (see the crate -//! documentation). That decision is not reversed here and this module is not a -//! step toward reversing it: durable slashing protection is a different thing, -//! with a durability requirement this deliberately does not have. -//! -//! # What it is -//! -//! A cheap check that closes the double-vote shapes this client can reach -//! *within one run*, which were otherwise reachable through ordinary -//! operation rather than through operator error: -//! -//! - **A backward wall-clock step.** The duty loop derives its slot from -//! `SystemTime::now()`, which is not monotonic. An NTP correction of a few -//! seconds re-enters a slot already attested, and the re-fetched attestation -//! data can differ if a block arrived in between. Two different attestations -//! under one target epoch is a double vote. -//! - **A schedule replaced mid-epoch.** A duty refresh that fails at an epoch -//! boundary and succeeds a few slots later can move a validator to a -//! different slot in the *same* epoch, and the loop then attests a second -//! time under that target. -//! -//! Neither needs a hostile beacon node or a second instance. Both are ordinary -//! things that happen to running systems. -//! -//! # The rule -//! -//! EIP-3076's minimal variant, per validator: remember the highest source and -//! target epoch signed, and refuse anything that does not strictly advance the -//! target or that would reach back below the source. -//! -//! That is deliberately more conservative than the full slashing conditions, -//! which would need the whole history to evaluate. With one record per -//! validator it cannot distinguish "surrounds an attestation from six epochs -//! ago" from "surrounds nothing"; requiring monotonic progress makes the -//! question unnecessary. An honest validator's attestations already advance -//! this way, so the conservatism costs nothing in normal operation. -//! -//! Refusing is always safe here: a skipped attestation is a missed reward, and -//! a double vote is a slashing. - -use std::collections::HashMap; - -use ethlambda_types::beacon::containers::shared::AttestationData; -use ethlambda_types::beacon::primitives::{BlsPubkey, Epoch}; - -/// The highest source and target epoch signed for one validator. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -struct Signed { - source: Epoch, - target: Epoch, -} - -/// Why an attestation was refused. -/// -/// Carried out of [`AttestationGuard::check`] so the caller can log which rule -/// fired rather than a bare "refused". The two are different operational -/// situations: a repeated target usually means the clock moved or a schedule -/// was replaced, while a regressed source means something stranger and is -/// worth looking at. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Refusal { - /// The target epoch is at or below one already signed for this validator. - /// - /// The double-vote shape. Equality is refused as well as regression: the - /// same target signed twice is a double vote unless the two attestations - /// are byte-identical, and this guard deliberately does not keep enough to - /// tell those apart. Re-signing identical data would be harmless but is - /// also pointless, since the client does not retry within a slot. - TargetNotAdvanced { signed: Epoch, proposed: Epoch }, - /// The source epoch is below one already signed for this validator. - /// - /// The surround shape: a lower source with a higher target surrounds the - /// earlier attestation. - SourceRegressed { signed: Epoch, proposed: Epoch }, -} - -impl std::fmt::Display for Refusal { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::TargetNotAdvanced { signed, proposed } => write!( - f, - "target epoch {proposed} does not advance past {signed}, already signed this run" - ), - Self::SourceRegressed { signed, proposed } => write!( - f, - "source epoch {proposed} is below {signed}, already signed this run" - ), - } - } -} - -/// Per-validator record of the highest attestation signed in this run. -/// -/// Not `Clone`: one guard per process is the point. A copy would be a second -/// opinion about what has been signed, which is worse than no opinion. -#[derive(Debug, Default)] -pub struct AttestationGuard { - signed: HashMap, -} - -impl AttestationGuard { - pub fn new() -> Self { - Self::default() - } - - /// Whether `data` may be signed for `pubkey`, without recording anything. - /// - /// Separate from [`Self::record`] so a caller can decide before paying for - /// a signature, and so this stays a pure function over the guard's state - /// and therefore directly testable. - pub fn check(&self, pubkey: &BlsPubkey, data: &AttestationData) -> Result<(), Refusal> { - let Some(signed) = self.signed.get(pubkey) else { - // Nothing signed for this validator this run. Note what this does - // *not* mean: a previous run may have signed anything at all. That - // gap is the crate-level scope decision, not an oversight here. - return Ok(()); - }; - - if data.target.epoch <= signed.target { - return Err(Refusal::TargetNotAdvanced { - signed: signed.target, - proposed: data.target.epoch, - }); - } - if data.source.epoch < signed.source { - return Err(Refusal::SourceRegressed { - signed: signed.source, - proposed: data.source.epoch, - }); - } - Ok(()) - } - - /// Record that `data` was signed for `pubkey`. - /// - /// Takes the maximum of each epoch rather than overwriting, so that a - /// caller recording out of order cannot lower the bar. Nothing does that - /// today; the guard is cheap to make order-independent and expensive to - /// debug if it ever is not. - pub fn record(&mut self, pubkey: BlsPubkey, data: &AttestationData) { - let entry = self.signed.entry(pubkey).or_insert(Signed { - source: data.source.epoch, - target: data.target.epoch, - }); - entry.source = entry.source.max(data.source.epoch); - entry.target = entry.target.max(data.target.epoch); - } - - /// Check and record in one step, for the common call site. - /// - /// Recording only on success is what makes a refusal idempotent: a - /// refused attestation must not move the bar it was measured against. - pub fn check_and_record( - &mut self, - pubkey: &BlsPubkey, - data: &AttestationData, - ) -> Result<(), Refusal> { - self.check(pubkey, data)?; - self.record(*pubkey, data); - Ok(()) - } - - /// How many validators have signed something this run. For metrics. - pub fn len(&self) -> usize { - self.signed.len() - } - - pub fn is_empty(&self) -> bool { - self.signed.is_empty() - } -} - -#[cfg(test)] -mod tests { - use ethlambda_types::beacon::containers::shared::Checkpoint; - use ethlambda_types::beacon::primitives::{H256, Root}; - - use super::*; - - fn pubkey(byte: u8) -> BlsPubkey { - BlsPubkey([byte; 48]) - } - - /// An attestation at `slot` voting source `source` and target `target`. - /// - /// `beacon_block_root` is a parameter because the double-vote case that - /// matters most is two attestations with one target epoch and *different* - /// roots, which is what a re-fetch after a block arrival produces. - fn data(slot: u64, source: Epoch, target: Epoch, root: u8) -> AttestationData { - AttestationData { - slot, - index: 0, - beacon_block_root: H256([root; 32]), - source: Checkpoint { - epoch: source, - root: Root::ZERO, - }, - target: Checkpoint { - epoch: target, - root: Root::ZERO, - }, - } - } - - #[test] - fn a_first_attestation_is_allowed() { - let mut guard = AttestationGuard::new(); - assert!( - guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xaa)) - .is_ok() - ); - } - - #[test] - fn advancing_the_target_is_allowed() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xaa)) - .expect("first"); - - assert!( - guard - .check_and_record(&pubkey(1), &data(128, 3, 4, 0xbb)) - .is_ok() - ); - } - - /// The backward-clock-step case. Same validator, same target epoch, a - /// different block root because the data was re-fetched after a block - /// arrived. This is the double vote the guard exists to stop. - #[test] - fn re_attesting_one_target_with_different_data_is_refused() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xaa)) - .expect("first"); - - let err = guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xbb)) - .expect_err("a second vote for one target must be refused"); - - assert_eq!( - err, - Refusal::TargetNotAdvanced { - signed: 3, - proposed: 3 - } - ); - } - - /// The mid-epoch schedule replacement case: a different *slot* in the same - /// epoch, which is still one target epoch and still a double vote. - #[test] - fn a_different_slot_in_the_same_target_epoch_is_refused() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xaa)) - .expect("first"); - - assert!( - guard - .check_and_record(&pubkey(1), &data(101, 2, 3, 0xbb)) - .is_err(), - "a second slot under one target epoch is a double vote" - ); - } - - #[test] - fn a_regressed_target_is_refused() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(128, 3, 4, 0xaa)) - .expect("first"); - - assert!( - guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xbb)) - .is_err() - ); - } - - /// The surround shape: a later target with an earlier source surrounds the - /// attestation already signed. - #[test] - fn a_regressed_source_under_an_advancing_target_is_refused() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(128, 3, 4, 0xaa)) - .expect("first"); - - let err = guard - .check_and_record(&pubkey(1), &data(160, 1, 5, 0xbb)) - .expect_err("a surrounding vote must be refused"); - - assert_eq!( - err, - Refusal::SourceRegressed { - signed: 3, - proposed: 1 - } - ); - } - - /// The property that makes this per-validator rather than per-slot: two - /// validators in one epoch attest at different slots, and the second must - /// not be blocked by the first. - #[test] - fn one_validator_does_not_block_another_in_the_same_epoch() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(96, 2, 3, 0xaa)) - .expect("first validator"); - - assert!( - guard - .check_and_record(&pubkey(2), &data(101, 2, 3, 0xaa)) - .is_ok(), - "a second validator attesting later in the same epoch must be allowed" - ); - } - - /// A refusal must not move the bar, or one refused attestation would - /// change what the next check compares against. - #[test] - fn a_refusal_records_nothing() { - let mut guard = AttestationGuard::new(); - guard - .check_and_record(&pubkey(1), &data(128, 3, 4, 0xaa)) - .expect("first"); - - guard - .check_and_record(&pubkey(1), &data(160, 1, 5, 0xbb)) - .expect_err("surround refused"); - - // The recorded source must still be 3, not 1: a valid attestation - // advancing from the real record is still accepted. - assert!( - guard - .check_and_record(&pubkey(1), &data(160, 3, 5, 0xcc)) - .is_ok(), - "the refused attempt must not have lowered the recorded source" - ); - } - - #[test] - fn check_alone_does_not_record() { - let guard = AttestationGuard::new(); - let data = data(96, 2, 3, 0xaa); - - guard.check(&pubkey(1), &data).expect("allowed"); - guard - .check(&pubkey(1), &data) - .expect("still allowed, because check records nothing"); - } - - #[test] - fn recording_out_of_order_does_not_lower_the_bar() { - let mut guard = AttestationGuard::new(); - guard.record(pubkey(1), &data(128, 3, 4, 0xaa)); - guard.record(pubkey(1), &data(96, 2, 3, 0xbb)); - - assert!( - guard.check(&pubkey(1), &data(101, 2, 3, 0xcc)).is_err(), - "the earlier record must not have displaced the later one" - ); - } -} diff --git a/crates/validator/src/beacon_node/mod.rs b/crates/validator/src/beacon_node/mod.rs index c689d2a1d..9ed3f7bc9 100644 --- a/crates/validator/src/beacon_node/mod.rs +++ b/crates/validator/src/beacon_node/mod.rs @@ -322,9 +322,10 @@ pub trait BeaconNodeApi: Send + Sync { /// /// An implementation must reject data whose `slot` is not the one asked /// for, and whose `target.epoch` is not that slot's epoch. Both are - /// signable material, and this client keeps no slashing-protection record, + /// signable material, and slashing protection records whatever it allows, /// so a node answering about the wrong slot must never reach the signing - /// path. + /// path: a target from the future would raise the validator's watermark + /// and cost it every attestation until the chain caught up. /// /// It matters that this is the *implementation's* job rather than the /// caller's, because of where failover sits. diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index 55b82a0ff..ee6df5afe 100644 --- a/crates/validator/src/error.rs +++ b/crates/validator/src/error.rs @@ -104,6 +104,15 @@ pub enum Error { #[error("signing failed for {pubkey:?}: {reason}")] Signing { pubkey: BlsPubkey, reason: String }, + /// The slashing-protection record could not be opened, read or written, + /// or refuses to start: a key with no record, another chain's record, + /// another process holding it. + /// + /// Never retryable: every one of these means nothing may be signed until + /// an operator looks. + #[error("slashing protection {path}: {reason}")] + SlashingProtection { path: String, reason: String }, + #[error("{path}: {source}")] Io { path: String, @@ -136,6 +145,7 @@ impl Error { | Self::KeystoreBadPassword | Self::UnknownValidator(_) | Self::Signing { .. } + | Self::SlashingProtection { .. } | Self::Io { .. } => false, } } diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 2232679b3..b6a9272ce 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -2,8 +2,8 @@ //! //! # Deviation //! -//! This client keeps no slashing-protection record, so the interchange data the -//! specification requires cannot be produced. `DELETE` declares +//! This client does not yet import or export EIP-3076 history, so the +//! interchange data the specification requires is not produced. `DELETE` declares //! `slashing_protection` a required response field, so an empty but well-formed //! EIP-3076 interchange is returned; `POST` accepts the optional //! `slashing_protection` field and ignores it. A caller migrating keys between @@ -12,8 +12,8 @@ //! The empty interchange [`DELETE`] returns is deliberately unusable as one: //! its `genesis_validators_root` is all-zero rather than the chain's real //! value. A real root would make the file look like legitimate, if empty, -//! history, which is worse than what it actually is (no history at all, -//! because this client never recorded any). The all-zero root is chosen so a +//! history, which is worse than what it actually is (none of the history this +//! client recorded, since it does not export it yet). The all-zero root is chosen so a //! conformant importer rejects the file outright instead of trusting it, on //! the theory that a loud, obvious failure at import time beats a tool //! silently believing a validator has never signed. @@ -72,7 +72,7 @@ pub struct ImportRequest { /// memory until decryption runs, and a plain `Vec` would leave /// those bytes sitting in the allocator's freed memory afterwards. pub passwords: Zeroizing>, - /// Accepted and ignored: this client keeps no slashing-protection record. + /// Accepted and ignored: importing EIP-3076 history is not supported yet. #[serde(default)] pub slashing_protection: Option, } @@ -145,7 +145,10 @@ pub async fn import( Json(request): Json, ) -> Json { if request.slashing_protection.is_some() { - warn!("Ignoring slashing_protection on import: this client keeps no signing history"); + warn!( + "Ignoring slashing_protection on import: importing EIP-3076 history is not \ + supported yet, so every new key starts as never having signed" + ); } if request.keystores.len() > MAX_KEYSTORES_PER_IMPORT { @@ -243,6 +246,25 @@ fn prepare_import( let secret = keystore.decrypt(password).map_err(KeyStatus::error)?; let pubkey = ValidatorStore::derive_pubkey(&secret).map_err(KeyStatus::error)?; + // Registered before the key is written to disk or reachable through the + // store, so the duty path never meets a key it has no record for, and a + // restart finds a record for every key on disk. A key that already has a + // record keeps it. The history the request may carry is not imported yet, + // so a key arriving from another client starts as never having signed: + // see the warning in `import`. + let registered = context + .protection + .lock() + .map_err(|_| KeyStatus::error("slashing protection is poisoned"))? + .register(&[pubkey]) + .map_err(KeyStatus::error)?; + if registered > 0 { + warn!( + pubkey = %encode_hex(&pubkey.0), + "Imported a validator key with no slashing-protection history" + ); + } + // Persisted before the key is reachable through the store: a validator // this process is willing to sign with must already be durable, or a // crash right after "imported" is reported silently loses it, with @@ -321,9 +343,9 @@ fn persist_import( // and re-saves it. Leaving a failed key's mutation in place would let the // *next* key's successful save write it to disk, so a key this request // reports as `error` would be enabled on the next restart and start - // signing. With no slashing-protection record, a validator the operator - // believes was never imported is exactly the kind that ends up running in - // two places. + // signing. A validator the operator believes was never imported is exactly + // the kind that ends up running in two places, which no local record can + // see. enum Undo { Appended, Replaced { @@ -564,6 +586,7 @@ mod tests { validators_dir: dir.to_path_buf(), secrets_dir: dir.to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + protection: crate::slashing_protection::SlashingProtection::in_memory(), } } @@ -900,6 +923,45 @@ mod tests { ); } + /// An imported key is registered in the slashing-protection record before + /// it is loaded, so the next start finds a record for it instead of + /// refusing it. + #[tokio::test] + async fn an_imported_key_is_registered_for_slashing_protection() { + let dir = tempfile::tempdir().expect("temp dir"); + let root = ethlambda_types::beacon::primitives::Root::ZERO; + let protection = + crate::slashing_protection::SlashingProtection::open(dir.path(), root, &[], false) + .expect("opens with no keys"); + let mut context = + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + let protection = context.protection.clone(); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + assert_eq!(protection.lock().expect("not poisoned").len(), 1); + + // The next start: same directory, the imported key loaded, no + // --init-slashing-protection. It must open, because the key is + // registered. + drop(protection); + let keys = ValidatorStore::load(dir.path()).expect("loads").pubkeys(); + crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false) + .expect("the imported key has a record"); + } + #[tokio::test] async fn deleting_an_unknown_key_reports_not_found() { let (_, body) = send( diff --git a/crates/validator/src/http_api/mod.rs b/crates/validator/src/http_api/mod.rs index 26e250b6e..2a7abb54c 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -58,6 +58,9 @@ pub struct KeymanagerContext { /// it takes this one first and `store` second, consistently, so the two /// never deadlock against each other. pub definitions_lock: Arc>, + /// The record every key must be registered in before it is loaded, so the + /// duty path never meets a key it has no record for. + pub protection: crate::slashing_protection::SharedProtection, } /// The file the generated token is written to, inside the validators directory. diff --git a/crates/validator/src/http_api/proposer.rs b/crates/validator/src/http_api/proposer.rs index beec84d3c..b9f98aed3 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -367,6 +367,7 @@ mod tests { validators_dir: dir.path().to_path_buf(), secrets_dir: dir.path().to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + protection: crate::slashing_protection::SlashingProtection::in_memory(), }; Harness { app: router(context, TOKEN.to_string()), diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index ebec73b1f..1d708bc6d 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -7,23 +7,20 @@ //! everything it knows about the chain arrives through the `BeaconNodeApi` //! trait, so it runs against any conformant node. //! -//! # Deviations from the specifications +//! # Slashing protection //! -//! This client implements **no slashing protection**. It keeps no durable -//! record of what it has signed, so a restart, or a second instance sharing the -//! same keystores, can double-vote or propose twice for one slot. On a live -//! network either is a slashable offence. This is a deliberate, recorded scope decision, not an oversight; -//! see [`docs/spec_deviations.md`] for the full entry, including what is and is -//! not covered. -//! -//! [`crate::attestation_guard`] and [`crate::proposal_guard`] close the part of -//! this that is reachable within a single run: a backward wall-clock step, or a -//! duty schedule replaced mid-epoch. Both are in-memory only and neither is a -//! substitute for the real thing. +//! Every block and attestation is checked against a durable, per-validator +//! record before it is signed, and the record is on disk before the signature +//! exists. The record keeps EIP-3076's minimal watermarks, lives next to the +//! keys, names the chain it belongs to, and is locked to one process. See +//! [`crate::slashing_protection`] for the rule and its guarantees, and +//! [`docs/spec_deviations.md`] for where the minimal rule is stricter than the +//! specification. //! //! [`docs/spec_deviations.md`]: https://github.com/lambdaclass/ethlambda/blob/main/docs/spec_deviations.md //! -//! The keymanager API is affected by the same decision. Its specification makes +//! The keymanager API registers a key it imports before loading it. It does +//! not yet carry history in or out: its specification makes //! `slashing_protection` a required field of the `DELETE /eth/v1/keystores` //! response, so this implementation returns a well-formed but empty EIP-3076 //! interchange, and accepts and ignores the optional `slashing_protection` @@ -32,7 +29,6 @@ pub mod aggregation; pub mod aggregation_selection; pub mod attestation; -pub mod attestation_guard; pub mod beacon_node; pub mod duties; pub mod error; @@ -41,10 +37,10 @@ pub mod keys; pub mod metrics; pub mod payload_attestation; pub mod proposal; -pub mod proposal_guard; pub mod proposer_settings; pub(crate) mod secure_fs; pub mod signing; +pub mod slashing_protection; pub mod slot_clock; pub mod subscriptions; pub mod sync_committee; @@ -95,29 +91,17 @@ pub struct ValidatorConfig { /// Bind address for the keymanager API. `None` when `--enable-keymanager` /// was not passed, in which case it is never spawned. pub keymanager: Option, + /// Register keys that have no slashing-protection record as never having + /// signed, instead of refusing to start. See + /// [`slashing_protection::SlashingProtection::open`]. + pub init_slashing_protection: bool, } /// Run the validator client until the process is stopped. pub async fn run(config: ValidatorConfig) -> Result<()> { metrics::init(); - // Said once, at startup, before anything is loaded or signed. - // - // Until now this client's most consequential property was recorded only in - // a rustdoc comment on this module, which nobody running a binary reads. - // An operator can build this, point it at mainnet keys, and never meet the - // warning. `warn!` rather than `info!` because the consequence is losing - // stake, and it is unconditional rather than behind a flag because a - // warning an operator can silence is one they will. - warn!( - "This validator client keeps NO slashing-protection record. It cannot detect that a \ - previous run, or another client holding these keys, already signed. Running the same \ - keys here and anywhere else, or restarting into a state where the chain has moved, can \ - produce a slashable double vote or a double block proposal. Do not use these keys in \ - any other client while this one runs. See docs/spec_deviations.md." - ); - - // Said at startup for the same reason the warning above is: an operator + // Said at startup, where an operator reads it: an operator // who never sets this proposes blocks that pay their execution-layer // rewards to whatever address their beacon node was configured with, which // on a default configuration is nobody useful. It is silent money, and @@ -191,6 +175,23 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { "Validator client starting" ); + // After the beacon node names the chain, which the record must belong to, + // and before any service that signs exists. Every loaded key must already + // have a record, unless the operator said otherwise. + let keys = store.read().await.pubkeys(); + let protection = slashing_protection::SlashingProtection::open( + &config.validators_dir, + genesis.genesis_validators_root, + &keys, + config.init_slashing_protection, + )?; + info!( + validators = protection.len(), + path = %protection.path().map(|path| path.display().to_string()).unwrap_or_default(), + "Slashing protection loaded" + ); + let protection = Arc::new(std::sync::Mutex::new(protection)); + let clock = SlotClock::from_config(genesis.genesis_time, &spec); let context = Arc::new(SigningContext { config: spec, @@ -198,11 +199,17 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { }); let mut duties = DutiesService::new(beacon_node.clone(), Vec::new()); - let attestation = AttestationService::new(beacon_node.clone(), context.clone()); + let attestation = + AttestationService::new(beacon_node.clone(), context.clone(), protection.clone()); let aggregation = AggregationService::new(beacon_node.clone(), context.clone()); let payload_attestation = PayloadAttestationService::new(beacon_node.clone(), context.clone()); let sync_committee = SyncCommitteeService::new(beacon_node.clone(), context.clone()); - let proposal = ProposalService::new(beacon_node.clone(), context.clone(), settings.clone()); + let proposal = ProposalService::new( + beacon_node.clone(), + context.clone(), + settings.clone(), + protection.clone(), + ); if let Some(address) = config.keymanager { let token = http_api::load_or_create_token(&config.validators_dir)?; @@ -212,6 +219,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { validators_dir: config.validators_dir.clone(), secrets_dir: config.secrets_dir.clone(), definitions_lock: Arc::new(Mutex::new(())), + protection: protection.clone(), }; let router = http_api::router(context, token); let listener = tokio::net::TcpListener::bind(address) @@ -462,7 +470,7 @@ async fn attest_and_aggregate( // also delays the next slot's is worth less than nothing. // // Dropping the future mid-flight is safe here specifically because of - // `AttestationGuard`: it records at signing time, so a duty abandoned + // slashing protection: it records before signing, so a duty abandoned // between signing and submission cannot be re-signed next slot under // the same target. That is the intended outcome and matches `attest`'s // own "do not retry within a slot" rule; the attestation is simply @@ -620,8 +628,8 @@ async fn aggregate( /// So: a proposal that overruns is abandoned, and the slot's attesters still /// vote. Dropping the future mid-flight is safe for the same reason it is on /// the attestation path, and only for that reason: `ProposalService` records -/// the slot in its guard at signing time, so a block abandoned between signing -/// and publication cannot be signed a second time. +/// the slot in slashing protection before signing, so a block abandoned between +/// signing and publication cannot be signed a second time. /// /// Failures are logged and counted rather than propagated. A proposal is one /// slot's work; losing it must not stop the client attesting for the rest of @@ -1197,7 +1205,11 @@ mod tests { }]; serve_slot_duties( - &AttestationService::new(node.clone(), context.clone()), + &AttestationService::new( + node.clone(), + context.clone(), + slashing_protection::SlashingProtection::in_memory(), + ), &AggregationService::new(node.clone(), context.clone()), &PayloadAttestationService::new(node.clone(), context.clone()), &SyncCommitteeService::new(node.clone(), context.clone()), @@ -1335,7 +1347,11 @@ mod tests { }]; serve_slot_duties( - &AttestationService::new(node.clone(), context.clone()), + &AttestationService::new( + node.clone(), + context.clone(), + slashing_protection::SlashingProtection::in_memory(), + ), &AggregationService::new(node.clone(), context.clone()), &PayloadAttestationService::new(node.clone(), context.clone()), &SyncCommitteeService::new(node.clone(), context.clone()), @@ -1371,7 +1387,11 @@ mod tests { let store = empty_store(); serve_slot_duties( - &AttestationService::new(node.clone(), context.clone()), + &AttestationService::new( + node.clone(), + context.clone(), + slashing_protection::SlashingProtection::in_memory(), + ), &AggregationService::new(node.clone(), context.clone()), &PayloadAttestationService::new(node.clone(), context.clone()), &SyncCommitteeService::new(node.clone(), context.clone()), diff --git a/crates/validator/src/metrics.rs b/crates/validator/src/metrics.rs index 2da746bb3..b77f8b2b6 100644 --- a/crates/validator/src/metrics.rs +++ b/crates/validator/src/metrics.rs @@ -74,19 +74,19 @@ static ATTESTATION_DEADLINE_MISSED_TOTAL: LazyLock = LazyLock::new(| .unwrap() }); -/// Attestations this process declined to sign because it had already signed a -/// conflicting one for that validator in this run. +/// Attestations this process declined to sign because slashing protection +/// judged them unsafe, or could not record them. /// /// Separate from `signing_failures_total`, which counts signatures that were /// attempted and failed. This counts signatures deliberately not attempted, and /// it should normally read zero: a non-zero value means the duty loop tried to -/// sign something the guard judged a double vote, which is worth investigating -/// even though the attestation was correctly suppressed. See -/// `crate::attestation_guard`. +/// sign something slashing protection judged a double or surround vote, which +/// is worth investigating even though the attestation was correctly +/// suppressed. See `crate::slashing_protection`. static ATTESTATIONS_REFUSED_TOTAL: LazyLock = LazyLock::new(|| { register_int_counter!( "ethlambda_validator_attestations_refused_total", - "Attestations refused because this process already signed a conflicting one" + "Attestations refused by slashing protection" ) .unwrap() }); @@ -113,18 +113,18 @@ static BLOCKS_BROADCAST_NOT_IMPORTED_TOTAL: LazyLock = LazyLock::new .unwrap() }); -/// Blocks this process declined to sign because it had already proposed that -/// slot for that validator in this run. +/// Blocks this process declined to sign because slashing protection judged +/// them unsafe, or could not record them. /// /// The proposal-shaped counterpart to `attestations_refused_total`, and it /// should read zero for the same reason: a non-zero value means the duty loop -/// tried to propose a slot the guard judged already proposed, which is worth -/// investigating even though the block was correctly suppressed. See -/// `crate::proposal_guard`. +/// tried to propose a slot slashing protection judged already proposed, which +/// is worth investigating even though the block was correctly suppressed. See +/// `crate::slashing_protection`. static BLOCKS_REFUSED_TOTAL: LazyLock = LazyLock::new(|| { register_int_counter!( "ethlambda_validator_blocks_refused_total", - "Blocks refused because this process already proposed that slot" + "Blocks refused by slashing protection" ) .unwrap() }); @@ -248,6 +248,31 @@ static SIGNING_DURATION_SECONDS: LazyLock = LazyLock::new(|| { .unwrap() }); +/// Time spent durably writing the slashing-protection record: one write per +/// slot's attestation batch and one per block, each flushed to disk before +/// anything is signed. See `crate::slashing_protection`. +static SLASHING_PROTECTION_WRITE_SECONDS: LazyLock = LazyLock::new(|| { + register_histogram!( + "ethlambda_validator_slashing_protection_write_seconds", + "Time spent durably writing the slashing-protection record", + vec![ + 0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5 + ] + ) + .unwrap() +}); + +/// Slashing-protection writes that failed, each of which left its messages +/// unsigned. Should read zero; anything else points at the disk under the +/// validators directory. +static SLASHING_PROTECTION_WRITE_FAILURES_TOTAL: LazyLock = LazyLock::new(|| { + register_int_counter!( + "ethlambda_validator_slashing_protection_write_failures_total", + "Slashing-protection writes that failed, leaving their messages unsigned" + ) + .unwrap() +}); + static PUBLICATION_DELAY_SECONDS: LazyLock = LazyLock::new(|| { register_histogram!( "ethlambda_validator_publication_delay_seconds", @@ -334,6 +359,8 @@ pub fn init() { LazyLock::force(&BEACON_NODE_AVAILABLE); LazyLock::force(&SIGNING_DURATION_SECONDS); LazyLock::force(&PUBLICATION_DELAY_SECONDS); + LazyLock::force(&SLASHING_PROTECTION_WRITE_SECONDS); + LazyLock::force(&SLASHING_PROTECTION_WRITE_FAILURES_TOTAL); } pub fn set_validators_loaded(count: u64) { @@ -450,6 +477,14 @@ pub fn time_signing() -> TimingGuard { TimingGuard::new(&SIGNING_DURATION_SECONDS) } +pub fn time_slashing_protection_write() -> TimingGuard { + TimingGuard::new(&SLASHING_PROTECTION_WRITE_SECONDS) +} + +pub fn inc_slashing_protection_write_failures() { + SLASHING_PROTECTION_WRITE_FAILURES_TOTAL.inc(); +} + /// Record how long after a slot's start its attestations were accepted. pub fn observe_publication_delay(seconds: f64) { PUBLICATION_DELAY_SECONDS.observe(seconds); diff --git a/crates/validator/src/proposal.rs b/crates/validator/src/proposal.rs index 1c76e4b37..34b16ba48 100644 --- a/crates/validator/src/proposal.rs +++ b/crates/validator/src/proposal.rs @@ -9,13 +9,15 @@ //! back from the beacon node in between: //! //! 1. The **RANDAO reveal** for the slot's epoch, signed first because the node -//! cannot build a body without it. It is not slashable and is not guarded. +//! cannot build a body without it. It is not slashable and is not checked +//! against slashing protection. //! 2. The **block**, built by the beacon node around that reveal. This client //! never chooses a block's contents; it asks for one and checks that what //! came back is the one it asked for. -//! 3. The **block signature**, which is slashable and is guarded. +//! 3. The **block signature**, which is slashable, and which +//! [`crate::slashing_protection`] must allow and record first. //! -//! # Why the guard is consulted twice +//! # Why slashing protection is consulted twice //! //! Once before asking for a block, once before signing it. //! @@ -24,10 +26,11 @@ //! most expensive thing this client can ask of it. Discovering only afterwards //! that the slot was already proposed wastes that for nothing. //! -//! The check before signing is the one that matters, and it records. Recording -//! at signing time rather than after publication is what makes the duty loop's -//! deadline safe: a proposal abandoned between signing and publishing cannot be -//! re-signed, because the guard already counts that slot as proposed. +//! The check before signing is the one that matters, and it records, durably, +//! before the signature exists. Recording then rather than after publication is +//! what makes the duty loop's deadline and a crash both safe: a proposal +//! abandoned between signing and publishing cannot be re-signed, because the +//! record already counts that slot as proposed. //! //! # Gloas: the block, then its envelope //! @@ -37,7 +40,7 @@ //! builder domain, and it is published right after the block, in the same //! budget. A bid naming anyone else is a builder's to reveal, and is left alone. //! -//! The envelope is not slashable, so the guard is untouched by it. +//! The envelope is not slashable, so slashing protection is untouched by it. //! //! # Do not retry this call within a slot //! @@ -45,8 +48,8 @@ //! not be: a second call asks the node for a second block, which will differ //! from the first because the node has packed whatever arrived in between, and //! two distinct blocks for one slot from one validator is a slashable proposer -//! offence. The guard refuses it, so a retry is merely useless rather than -//! dangerous, but the caller should not be relying on the guard for that. +//! offence. Slashing protection refuses it, so a retry is merely useless rather +//! than dangerous, but the caller should not be relying on that. use std::sync::Arc; @@ -55,16 +58,16 @@ use ethlambda_types::beacon::fork::ForkName; use ethlambda_types::beacon::primitives::{BlsPubkey, ExecutionAddress, Root, Slot}; use ethlambda_types::beacon::signing::compute_epoch_at_slot; use tokio::sync::RwLock; -use tracing::{info, warn}; +use tracing::{error, info, warn}; use crate::beacon_node::block_contents::{GloasPayload, signed_envelope_ssz}; use crate::beacon_node::dto::{ProposerDutyDto, encode_hex, parse_pubkey}; use crate::beacon_node::{BeaconNodeApi, BlockRequest, Published, validate_produced_block}; use crate::error::{Error, Result}; use crate::keys::ValidatorStore; -use crate::proposal_guard::ProposalGuard; use crate::proposer_settings::ProposerSettings; use crate::signing::SigningContext; +use crate::slashing_protection::SharedProtection; pub struct ProposalService { beacon_node: Arc, @@ -76,14 +79,9 @@ pub struct ProposalService { /// is the one that builds the payload, and the specification is explicit /// that it need not honour the preparation it was sent. settings: Arc, - /// What this process has already proposed, per validator. - /// - /// A `std::sync::Mutex` for the reason [`crate::attestation`]'s is: the - /// critical section is a lookup and an insert with no await inside it, so - /// an async mutex would buy nothing and cost a scheduling point. - /// - /// Not slashing protection. See [`ProposalGuard`]. - guard: std::sync::Mutex, + /// What every validator has signed, shared with the attestation service + /// and the keymanager. See [`crate::slashing_protection`]. + protection: SharedProtection, } impl ProposalService { @@ -91,12 +89,13 @@ impl ProposalService { beacon_node: Arc, context: Arc, settings: Arc, + protection: SharedProtection, ) -> Self { Self { beacon_node, context, settings, - guard: std::sync::Mutex::new(ProposalGuard::new()), + protection, } } @@ -165,18 +164,17 @@ impl ProposalService { // Before paying for a block, not after. See the module doc. // - // A poisoned lock is treated as a refusal rather than unwrapped: the - // lock is poisoned only by a panic while it is held, nothing inside it - // can panic, and a crash in the duty path would be a worse outcome than - // a skipped proposal. - match self.guard.lock() { - Ok(guard) => { - if let Err(refusal) = guard.check(&pubkey, slot) { + // A poisoned lock is treated as a refusal rather than unwrapped: a + // crash in the duty path would be a worse outcome than a skipped + // proposal. + match self.protection.lock() { + Ok(protection) => { + if let Err(refusal) = protection.check_block(&pubkey, slot) { warn!( %slot, validator = duty.validator_index, %refusal, - "Refusing to propose: this process already proposed this slot" + "Refusing to propose: slashing protection judged this block unsafe" ); crate::metrics::inc_blocks_refused(); return Err(Error::ProposalRefused { @@ -186,11 +184,11 @@ impl ProposalService { } } Err(err) => { - warn!(%slot, %err, "Proposal guard is poisoned; refusing to propose"); + warn!(%slot, %err, "Slashing protection is poisoned; refusing to propose"); crate::metrics::inc_blocks_refused(); return Err(Error::ProposalRefused { slot, - reason: "the proposal guard is poisoned".to_string(), + reason: "slashing protection is poisoned".to_string(), }); } } @@ -268,30 +266,38 @@ impl ProposalService { // guard's slot is recorded against and what the signature covers. let block_root = produced.block_root(); - let signature = { - let store = store.read().await; - - // The check that matters, and the one that records. Everything - // after this point may be abandoned without risk, because the - // guard already counts this slot as proposed. - match self.guard.lock() { - Ok(mut guard) => guard.check_and_record(&pubkey, slot).map_err(|refusal| { - crate::metrics::inc_blocks_refused(); - Error::ProposalRefused { - slot, - reason: refusal.to_string(), - } - })?, - Err(err) => { - warn!(%slot, %err, "Proposal guard is poisoned; refusing to sign"); - crate::metrics::inc_blocks_refused(); - return Err(Error::ProposalRefused { - slot, - reason: "the proposal guard is poisoned".to_string(), - }); - } + // The check that matters, and the one that records, durably, before + // the signature exists. Everything after this point may be abandoned + // without risk, because the record already counts this slot as + // proposed. Off the executor, and before the store's read lock below, + // so the disk flush never holds up a keymanager writer. + let protection = self.protection.clone(); + let recorded = tokio::task::spawn_blocking(move || { + let mut protection = protection + .lock() + .map_err(|_| "slashing protection is poisoned".to_string())?; + protection + .check_and_record_block(&pubkey, slot) + .map_err(|err| err.to_string()) + }) + .await + .map_err(|err| err.to_string()) + .and_then(|recorded| recorded); + let refusal = match recorded { + Ok(Ok(())) => None, + Ok(Err(refusal)) => Some(refusal.to_string()), + Err(err) => { + error!(%slot, %err, "Slashing protection could not record this block; refusing to sign"); + Some(err) } + }; + if let Some(reason) = refusal { + crate::metrics::inc_blocks_refused(); + return Err(Error::ProposalRefused { slot, reason }); + } + let signature = { + let store = store.read().await; self.context.sign_block(&store, &pubkey, block_root, slot)? }; @@ -441,6 +447,7 @@ mod tests { use super::*; use crate::beacon_node::block_contents::SignedBlockContents; use crate::beacon_node::mock::MockBeaconNode; + use crate::slashing_protection::SlashingProtection; use ethlambda_types::beacon::config::Config; use ethlambda_types::beacon::primitives::HashTreeRoot as _; use ethlambda_types::beacon::primitives::{BlsPubkey, Bytes32, H160, Root}; @@ -477,7 +484,12 @@ mod tests { fn service(node: Arc) -> ProposalService { let settings = ProposerSettings::new(Bytes32::repeat_byte(0xab), None); - ProposalService::new(node, context(), Arc::new(settings)) + ProposalService::new( + node, + context(), + Arc::new(settings), + SlashingProtection::in_memory(), + ) } fn service_expecting( @@ -485,7 +497,12 @@ mod tests { fee_recipient: ExecutionAddress, ) -> ProposalService { let settings = ProposerSettings::new(Bytes32::repeat_byte(0xab), Some(fee_recipient)); - ProposalService::new(node, context(), Arc::new(settings)) + ProposalService::new( + node, + context(), + Arc::new(settings), + SlashingProtection::in_memory(), + ) } /// A slot inside mainnet's electra era. @@ -582,7 +599,12 @@ mod tests { let (store, pubkey) = store(); let node = Arc::new(MockBeaconNode::new().with_block(slot(), 7)); let settings = Arc::new(ProposerSettings::new(Bytes32::repeat_byte(0xab), None)); - let service = ProposalService::new(node.clone(), context(), settings.clone()); + let service = ProposalService::new( + node.clone(), + context(), + settings.clone(), + SlashingProtection::in_memory(), + ); settings.set_graffiti(&pubkey, Bytes32::repeat_byte(0xcd)); service @@ -654,6 +676,36 @@ mod tests { assert_eq!(node.published_blocks().len(), 1); } + /// The restart shape: a fresh service over the same slashing-protection + /// directory refuses the slot already proposed, before asking for a block. + #[tokio::test] + async fn a_restart_does_not_forget_what_was_proposed() { + let (store, pubkey) = store(); + let dir = tempfile::tempdir().expect("temp dir"); + let settings = Arc::new(ProposerSettings::new(Bytes32::repeat_byte(0xab), None)); + let protection = || { + let opened = + SlashingProtection::open(dir.path(), Root::ZERO, &[pubkey], true).expect("opens"); + Arc::new(std::sync::Mutex::new(opened)) + }; + + let node = Arc::new(MockBeaconNode::new().with_block(slot(), 7)); + ProposalService::new(node.clone(), context(), settings.clone(), protection()) + .propose(slot(), &duty(&pubkey, 7, slot()), &store) + .await + .expect("first"); + assert_eq!(node.published_blocks().len(), 1); + + let node_again = Arc::new(MockBeaconNode::new().with_block(slot(), 7)); + let err = ProposalService::new(node_again.clone(), context(), settings, protection()) + .propose(slot(), &duty(&pubkey, 7, slot()), &store) + .await + .expect_err("the slot was proposed before the restart"); + assert!(matches!(err, Error::ProposalRefused { .. }), "got {err:?}"); + assert!(node_again.block_requests().is_empty()); + assert!(node_again.published_blocks().is_empty()); + } + #[tokio::test] async fn a_node_that_cannot_import_the_block_is_reported_rather_than_hidden() { let (store, pubkey) = store(); @@ -781,6 +833,7 @@ mod gloas_tests { use super::*; use crate::beacon_node::block_contents::SignedExecutionPayloadEnvelopeContents; use crate::beacon_node::mock::MockBeaconNode; + use crate::slashing_protection::SlashingProtection; use ethlambda_types::beacon::config::Config; use ethlambda_types::beacon::containers::gloas; use ethlambda_types::beacon::preset::SLOTS_PER_EPOCH; @@ -827,7 +880,12 @@ mod gloas_tests { ethlambda_types::beacon::primitives::Bytes32::repeat_byte(0xab), None, ); - ProposalService::new(node, context(), Arc::new(settings)) + ProposalService::new( + node, + context(), + Arc::new(settings), + SlashingProtection::in_memory(), + ) } fn verifies(pubkey: &BlsPubkey, signature: &[u8; 96], root: Root) -> bool { @@ -904,10 +962,15 @@ mod gloas_tests { )); let settings = Arc::new(ProposerSettings::new(Bytes32::repeat_byte(0xab), None)); settings.set_graffiti(&pubkey, Bytes32::repeat_byte(0xcd)); - ProposalService::new(node.clone(), context(), settings) - .propose(slot(), &duty(&pubkey), &store) - .await - .expect("proposes"); + ProposalService::new( + node.clone(), + context(), + settings, + SlashingProtection::in_memory(), + ) + .propose(slot(), &duty(&pubkey), &store) + .await + .expect("proposes"); let request = node.block_requests().remove(0); assert_eq!(request.fork, ForkName::Gloas); diff --git a/crates/validator/src/proposal_guard.rs b/crates/validator/src/proposal_guard.rs deleted file mode 100644 index 3d87c03a5..000000000 --- a/crates/validator/src/proposal_guard.rs +++ /dev/null @@ -1,271 +0,0 @@ -//! An in-process guard against signing two blocks for one slot. -//! -//! The proposal-shaped counterpart to [`crate::attestation_guard`], and the -//! same disclaimer applies in full: **this is not slashing protection.** It -//! holds nothing on disk, so it knows nothing about a previous run of this -//! process or about another process holding the same keys. A restart empties -//! it. -//! -//! # Why it is a separate guard rather than a field on the other one -//! -//! The two answer different questions and would answer them badly if merged. -//! An attestation is judged on its source and target *epochs*; a block is -//! judged on its *slot*, and a validator can legitimately propose in an epoch -//! it also attests in. Keying both off one record would either refuse a legal -//! proposal or admit an illegal one, depending on which field won. -//! -//! # The rule -//! -//! EIP-3076's minimal variant for blocks, per validator: remember the highest -//! slot proposed, and refuse anything that does not strictly advance it. -//! -//! One record per validator, like the attestation guard, and for the same -//! reason: the full condition ("never two distinct blocks for one slot") needs -//! the whole history to evaluate, while "the slot must strictly advance" needs -//! one number and is strictly more conservative. An honest proposer's blocks -//! already advance this way, since a validator is assigned at most one slot per -//! epoch and epochs only move forward. -//! -//! # What it actually closes -//! -//! The same two shapes the attestation guard closes, reached the same ways: -//! -//! - **A backward wall-clock step.** The duty loop takes its slot from -//! `SystemTime::now()`, which is not monotonic. An NTP correction can -//! re-enter a slot already proposed, and the block produced the second time -//! will differ from the first, because the beacon node has since seen more -//! attestations and a different head. Two distinct blocks for one slot from -//! one validator is a slashable proposer offence, and unlike a double vote it -//! needs no second validator to be caught: the two signed headers are the -//! whole evidence. -//! - **A proposer schedule replaced mid-epoch.** Proposer duties are only -//! final once the epoch's randao is fixed, so a refresh that lands late can -//! move a validator between slots within the epoch. Acting on both is two -//! blocks from one key. -//! -//! Refusing is always safe here, and cheaper than it is for an attestation: a -//! skipped proposal costs one block reward, and a proposer slashing is the most -//! expensive thing a validator can do. - -use std::collections::HashMap; - -use ethlambda_types::beacon::primitives::{BlsPubkey, Slot}; - -/// Why a proposal was refused. -/// -/// One variant, unlike [`crate::attestation_guard::Refusal`]'s two, because -/// there is only one rule to break. It stays an enum rather than a struct so -/// that the call site's `match`/`Display` shape matches the attestation path's, -/// and so a second rule can be added without changing the signature. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Refusal { - /// The slot is at or below one already proposed for this validator. - /// - /// Equality is refused as well as regression. The same slot proposed twice - /// is a slashable offence unless the two blocks are byte-identical, and - /// this guard deliberately does not keep enough to tell those apart. Nor - /// would it help: a block re-produced for the same slot is almost never - /// identical, since the beacon node packs whatever attestations have - /// arrived since. - SlotNotAdvanced { signed: Slot, proposed: Slot }, -} - -impl std::fmt::Display for Refusal { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::SlotNotAdvanced { signed, proposed } => write!( - f, - "slot {proposed} does not advance past {signed}, already proposed this run" - ), - } - } -} - -/// Per-validator record of the highest slot proposed in this run. -/// -/// Not `Clone`, for the reason [`crate::attestation_guard::AttestationGuard`] -/// is not: one guard per process is the point, and a copy would be a second -/// opinion about what has been signed. -#[derive(Debug, Default)] -pub struct ProposalGuard { - proposed: HashMap, -} - -impl ProposalGuard { - pub fn new() -> Self { - Self::default() - } - - /// Whether a block for `slot` may be signed for `pubkey`, recording - /// nothing. - /// - /// Separate from [`Self::record`] so a caller can decide before paying for - /// a block production round trip, which is far more expensive than the - /// attestation equivalent: it costs the beacon node an execution-layer - /// payload build. - pub fn check(&self, pubkey: &BlsPubkey, slot: Slot) -> Result<(), Refusal> { - let Some(&signed) = self.proposed.get(pubkey) else { - // Nothing proposed for this validator this run. As with the - // attestation guard, that says nothing about previous runs; the - // gap is the crate-level scope decision, not an oversight here. - return Ok(()); - }; - - if slot <= signed { - return Err(Refusal::SlotNotAdvanced { - signed, - proposed: slot, - }); - } - Ok(()) - } - - /// Record that a block for `slot` was signed for `pubkey`. - /// - /// Takes the maximum rather than overwriting, so a caller recording out of - /// order cannot lower the bar. Nothing does that today; making it - /// order-independent is one `max` and removes a class of bug that would - /// only ever appear under a clock step, which is the exact situation the - /// guard exists for. - pub fn record(&mut self, pubkey: BlsPubkey, slot: Slot) { - let entry = self.proposed.entry(pubkey).or_insert(slot); - *entry = (*entry).max(slot); - } - - /// Check and record in one step, for the common call site. - /// - /// Recording only on success is what makes a refusal idempotent: a refused - /// proposal must not move the bar it was measured against. - pub fn check_and_record(&mut self, pubkey: &BlsPubkey, slot: Slot) -> Result<(), Refusal> { - self.check(pubkey, slot)?; - self.record(*pubkey, slot); - Ok(()) - } - - /// How many validators have proposed something this run. For metrics. - pub fn len(&self) -> usize { - self.proposed.len() - } - - pub fn is_empty(&self) -> bool { - self.proposed.is_empty() - } -} - -#[cfg(test)] -mod tests { - use super::*; - - fn pubkey(byte: u8) -> BlsPubkey { - BlsPubkey([byte; 48]) - } - - #[test] - fn a_first_proposal_is_allowed() { - let mut guard = ProposalGuard::new(); - assert!(guard.check_and_record(&pubkey(1), 96).is_ok()); - } - - #[test] - fn advancing_the_slot_is_allowed() { - let mut guard = ProposalGuard::new(); - guard.check_and_record(&pubkey(1), 96).expect("first"); - assert!(guard.check_and_record(&pubkey(1), 128).is_ok()); - } - - /// The backward-clock-step case: the same slot re-entered after an NTP - /// correction. The block produced the second time is a different block, - /// because the beacon node has packed whatever arrived in between, so this - /// is the proposer slashing the guard exists to stop. - #[test] - fn proposing_the_same_slot_twice_is_refused() { - let mut guard = ProposalGuard::new(); - guard.check_and_record(&pubkey(1), 96).expect("first"); - - let err = guard - .check_and_record(&pubkey(1), 96) - .expect_err("a second block for one slot must be refused"); - - assert_eq!( - err, - Refusal::SlotNotAdvanced { - signed: 96, - proposed: 96 - } - ); - } - - #[test] - fn a_regressed_slot_is_refused() { - let mut guard = ProposalGuard::new(); - guard.check_and_record(&pubkey(1), 128).expect("first"); - assert!(guard.check_and_record(&pubkey(1), 96).is_err()); - } - - /// The property that makes this per-validator rather than global: two of - /// this client's validators can be assigned different slots in one epoch, - /// and the second must not be blocked by the first. - #[test] - fn one_validator_does_not_block_another() { - let mut guard = ProposalGuard::new(); - guard - .check_and_record(&pubkey(1), 128) - .expect("first validator"); - - assert!( - guard.check_and_record(&pubkey(2), 96).is_ok(), - "a second validator proposing an earlier slot must be allowed" - ); - } - - #[test] - fn a_refusal_records_nothing() { - let mut guard = ProposalGuard::new(); - guard.check_and_record(&pubkey(1), 128).expect("first"); - guard - .check_and_record(&pubkey(1), 96) - .expect_err("regression refused"); - - // Still measured against 128, not against the refused 96: a proposal - // at 100 must still be refused. - assert!( - guard.check_and_record(&pubkey(1), 100).is_err(), - "the refused attempt must not have lowered the recorded slot" - ); - } - - #[test] - fn check_alone_does_not_record() { - let guard = ProposalGuard::new(); - guard.check(&pubkey(1), 96).expect("allowed"); - guard - .check(&pubkey(1), 96) - .expect("still allowed, because check records nothing"); - } - - #[test] - fn recording_out_of_order_does_not_lower_the_bar() { - let mut guard = ProposalGuard::new(); - guard.record(pubkey(1), 128); - guard.record(pubkey(1), 96); - - assert!( - guard.check(&pubkey(1), 100).is_err(), - "the earlier record must not have displaced the later one" - ); - } - - /// Slot 0 is a real slot, and `HashMap`'s absent-versus-zero distinction is - /// exactly the kind of thing a `unwrap_or_default()` would quietly erase. - /// A validator that proposed slot 0 must not be allowed to propose it - /// again. - #[test] - fn slot_zero_is_recorded_like_any_other() { - let mut guard = ProposalGuard::new(); - guard.check_and_record(&pubkey(1), 0).expect("first"); - assert!( - guard.check_and_record(&pubkey(1), 0).is_err(), - "slot 0 proposed twice must be refused, not treated as never proposed" - ); - } -} diff --git a/crates/validator/src/signing.rs b/crates/validator/src/signing.rs index ced20d40e..9317ab341 100644 --- a/crates/validator/src/signing.rs +++ b/crates/validator/src/signing.rs @@ -367,8 +367,9 @@ impl SigningContext { /// This one *is* slashable, and is the most expensive signature this /// client can get wrong: two distinct blocks for one slot need no second /// validator to be caught, since the two signed headers are the whole - /// evidence. Callers must pass it through - /// [`crate::proposal_guard::ProposalGuard`] first. + /// evidence. Callers must have it allowed and recorded by + /// [`crate::slashing_protection::SlashingProtection::check_and_record_block`] + /// first. pub fn sign_block( &self, store: &ValidatorStore, diff --git a/crates/validator/src/slashing_protection/file.rs b/crates/validator/src/slashing_protection/file.rs new file mode 100644 index 000000000..c75bf057d --- /dev/null +++ b/crates/validator/src/slashing_protection/file.rs @@ -0,0 +1,312 @@ +//! The slashing-protection file: its format, a strict reader, a durable +//! writer, and the lock that keeps a second process away from it. +//! +//! # Format +//! +//! The file is an EIP-3076 version 5 interchange in its minimal form: per +//! validator, at most one `signed_blocks` entry (the highest slot) and at most +//! one `signed_attestations` entry (the highest source and the highest target). +//! Storing the interchange format itself means the record a validator is +//! protected by and the record it would be exported with are the same bytes, +//! with no second encoding to drift from the first. +//! +//! # Reading fails closed +//! +//! Anything other than a well-formed file is an error, never an empty record. +//! An empty record would let every validator sign anything, so a file that is +//! empty, truncated, of another version, or names one validator twice must stop +//! the client rather than be taken as "nothing signed yet". Prysm's minimal +//! store reads an empty file as exactly that. +//! +//! # Writing is durable before it returns +//! +//! Every write goes to a temporary sibling, is flushed to disk, renamed over +//! the real file, and then the directory is flushed so the rename itself +//! survives a power loss. A reader therefore sees the old file or the new one, +//! never a mixture, and a write that returned `Ok` is on disk. + +use std::collections::HashMap; +use std::fs::File; +use std::io::Write as _; +use std::path::{Path, PathBuf}; + +use ethlambda_types::beacon::primitives::{BlsPubkey, Epoch, Root, Slot}; +use ethlambda_types::beacon::serde_helpers::{hex_array, quoted_or_bare}; +use serde::{Deserialize, Serialize}; + +use super::{Record, Signed}; +use crate::error::{Error, Result}; + +/// The record's file name inside the validators directory. +pub const FILE_NAME: &str = "slashing_protection.json"; + +/// The lock's file name, next to the record. +/// +/// A file of its own rather than a lock on the record: the record is replaced +/// by a rename on every write, and a lock held on the old inode would not stop +/// a second process from opening the new one. +pub const LOCK_FILE_NAME: &str = "slashing_protection.lock"; + +/// The only EIP-3076 interchange version this client reads or writes. +pub const INTERCHANGE_VERSION: &str = "5"; + +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct Interchange { + metadata: Metadata, + data: Vec, +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct Metadata { + interchange_format_version: String, + #[serde(with = "hex_array")] + genesis_validators_root: [u8; 32], +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct Entry { + #[serde(with = "hex_array")] + pubkey: [u8; 48], + signed_blocks: Vec, + signed_attestations: Vec, +} + +/// `signing_root` is optional in EIP-3076 and never written here: the minimal +/// rule does not compare roots. It is still accepted on read, so a file another +/// tool wrote in this shape is not rejected for carrying one. +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct SignedBlock { + #[serde(with = "quoted_or_bare")] + slot: Slot, + #[serde(default, skip_serializing)] + #[allow(dead_code)] + signing_root: Option, +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct SignedAttestation { + #[serde(with = "quoted_or_bare")] + source_epoch: Epoch, + #[serde(with = "quoted_or_bare")] + target_epoch: Epoch, + #[serde(default, skip_serializing)] + #[allow(dead_code)] + signing_root: Option, +} + +/// Read and validate the record at `path`. +pub(crate) fn read(path: &Path) -> Result<(Root, HashMap)> { + let invalid = |reason: String| Error::SlashingProtection { + path: path.display().to_string(), + reason, + }; + + let text = std::fs::read_to_string(path).map_err(|source| Error::Io { + path: path.display().to_string(), + source, + })?; + if text.trim().is_empty() { + return Err(invalid( + "the file is empty; refusing to treat it as \"nothing signed\"".to_string(), + )); + } + let interchange: Interchange = + serde_json::from_str(&text).map_err(|err| invalid(format!("malformed: {err}")))?; + if interchange.metadata.interchange_format_version != INTERCHANGE_VERSION { + return Err(invalid(format!( + "interchange format version {:?}, expected {INTERCHANGE_VERSION:?}", + interchange.metadata.interchange_format_version + ))); + } + + let mut records = HashMap::with_capacity(interchange.data.len()); + for entry in interchange.data { + let pubkey = BlsPubkey(entry.pubkey); + let record = record_of(&entry).map_err(|reason| { + invalid(format!( + "validator {}: {reason}", + crate::beacon_node::dto::encode_hex(&pubkey.0) + )) + })?; + if records.insert(pubkey, record).is_some() { + return Err(invalid(format!( + "validator {} appears more than once", + crate::beacon_node::dto::encode_hex(&pubkey.0) + ))); + } + } + Ok(( + Root::from(interchange.metadata.genesis_validators_root), + records, + )) +} + +/// The watermarks one entry stands for: the highest slot, source and target it +/// lists. More than one entry of a kind is reduced rather than refused, since +/// taking the maximum is never less strict than any one of them. +fn record_of(entry: &Entry) -> std::result::Result { + let block_slot = entry.signed_blocks.iter().map(|block| block.slot).max(); + + let mut attestation: Option = None; + for signed in &entry.signed_attestations { + if signed.source_epoch > signed.target_epoch { + return Err(format!( + "attestation source epoch {} is after its target epoch {}", + signed.source_epoch, signed.target_epoch + )); + } + let next = Signed { + source: signed.source_epoch, + target: signed.target_epoch, + }; + attestation = Some(match attestation { + Some(current) => current.max_with(next), + None => next, + }); + } + Ok(Record { + block_slot, + attestation, + }) +} + +/// Durably replace the record at `path` with `records`. +/// +/// Entries are written in pubkey order so the file's bytes depend only on its +/// content, which keeps a diff of two files meaningful. +pub(crate) fn write( + path: &Path, + genesis_validators_root: Root, + records: &HashMap, +) -> Result<()> { + let mut data: Vec = records + .iter() + .map(|(pubkey, record)| Entry { + pubkey: pubkey.0, + signed_blocks: record + .block_slot + .map(|slot| SignedBlock { + slot, + signing_root: None, + }) + .into_iter() + .collect(), + signed_attestations: record + .attestation + .map(|signed| SignedAttestation { + source_epoch: signed.source, + target_epoch: signed.target, + signing_root: None, + }) + .into_iter() + .collect(), + }) + .collect(); + data.sort_by_key(|entry| entry.pubkey); + + let interchange = Interchange { + metadata: Metadata { + interchange_format_version: INTERCHANGE_VERSION.to_string(), + genesis_validators_root: genesis_validators_root.0, + }, + data, + }; + let contents = + serde_json::to_vec_pretty(&interchange).map_err(|err| Error::SlashingProtection { + path: path.display().to_string(), + reason: format!("could not encode the record: {err}"), + })?; + write_durably(path, &contents).map_err(|source| Error::Io { + path: path.display().to_string(), + source, + }) +} + +/// Temporary file, flush, rename, flush the directory. +fn write_durably(path: &Path, contents: &[u8]) -> std::io::Result<()> { + let temporary = temporary_path(path); + let mut file = open_private(&temporary)?; + file.write_all(contents)?; + file.sync_all()?; + drop(file); + std::fs::rename(&temporary, path)?; + sync_directory(path.parent().unwrap_or(Path::new("."))) +} + +fn temporary_path(path: &Path) -> PathBuf { + let mut name = path.file_name().unwrap_or_default().to_os_string(); + name.push(".tmp"); + path.with_file_name(name) +} + +/// Created, or truncated if it exists, at mode `0600` on unix: the record says +/// which keys this client holds. +#[cfg(unix)] +fn open_private(path: &Path) -> std::io::Result { + use std::os::unix::fs::OpenOptionsExt as _; + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path) +} + +#[cfg(not(unix))] +fn open_private(path: &Path) -> std::io::Result { + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .open(path) +} + +/// `fsync` on the directory, so the rename that replaced the record is itself +/// durable. Called through `libc` rather than `File::sync_all`, which on macOS +/// issues `F_FULLFSYNC`, a request some filesystems refuse for a directory. +#[cfg(unix)] +fn sync_directory(directory: &Path) -> std::io::Result<()> { + use std::os::fd::AsRawFd as _; + let directory = File::open(directory)?; + // SAFETY: the descriptor is owned by `directory`, which outlives the call. + if unsafe { libc::fsync(directory.as_raw_fd()) } == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(not(unix))] +fn sync_directory(_directory: &Path) -> std::io::Result<()> { + Ok(()) +} + +/// Take the exclusive lock in `directory`, held until the returned file is +/// dropped. +pub(crate) fn lock(directory: &Path) -> Result { + let path = directory.join(LOCK_FILE_NAME); + let io_error = |source| Error::Io { + path: path.display().to_string(), + source, + }; + let file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&path) + .map_err(io_error)?; + match file.try_lock() { + Ok(()) => Ok(file), + Err(std::fs::TryLockError::WouldBlock) => Err(Error::SlashingProtection { + path: path.display().to_string(), + reason: "in use by another process; two processes must never sign with the same keys" + .to_string(), + }), + Err(std::fs::TryLockError::Error(source)) => Err(io_error(source)), + } +} diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs new file mode 100644 index 000000000..16ec86acc --- /dev/null +++ b/crates/validator/src/slashing_protection/mod.rs @@ -0,0 +1,848 @@ +//! Slashing protection: the durable record of what each validator has signed. +//! +//! # The rule +//! +//! EIP-3076's minimal variant, per validator: +//! +//! - an attestation may be signed only if its target epoch is above the +//! highest one signed and its source epoch is not below the highest one +//! signed; +//! - a block may be signed only if its slot is above the highest one signed. +//! +//! That is stricter than the full slashing conditions, which would need the +//! whole signing history to evaluate: with one record per validator it cannot +//! tell "surrounds an attestation from six epochs ago" from "surrounds +//! nothing", and requiring monotonic progress makes the question unnecessary. +//! An honest validator's messages already advance this way. What the +//! strictness costs is a byte-identical re-sign after a crash, which is +//! refused; that is one missed attestation, against a slashing. +//! +//! # Order of operations +//! +//! Check, record durably, then sign. The record reaches the disk before the +//! signature exists, so a signature that escapes is always one the record +//! already accounts for, whether the process then crashes, the duty is +//! abandoned at its deadline, or the beacon node rejects it. A record that +//! cannot be written means nothing is signed. +//! +//! # What it guards against +//! +//! - **A restart.** The record outlives the process; see [`file`] for how it +//! is written and why reading it fails closed. +//! - **A second process.** [`SlashingProtection::open`] takes an exclusive +//! lock in the validators directory for the life of the process, so a second +//! validator client pointed at the same keys refuses to start. +//! - **Another chain.** The record names the chain's `genesis_validators_root` +//! and is refused for any other. +//! - **A backward clock step or a duty schedule replaced mid-epoch**, the two +//! in-run shapes the in-memory guards this replaces were written for. +//! +//! It cannot see keys running in another client on another machine: no record +//! kept here can. A key moved here from elsewhere must bring its history with +//! it, which is why a key with no record is refused at startup unless the +//! operator says otherwise (see [`SlashingProtection::open`]). + +pub mod file; + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; + +use ethlambda_types::beacon::containers::shared::AttestationData; +use ethlambda_types::beacon::primitives::{BlsPubkey, Epoch, Root, Slot}; +use tracing::warn; + +use crate::beacon_node::dto::encode_hex; +use crate::error::{Error, Result}; + +/// The handle the duty services and the keymanager share. +/// +/// A `std::sync::Mutex`: every critical section is a lookup, an insert and, on +/// a change, a file write, with no `.await` inside. Callers on the async duty +/// path take it inside `spawn_blocking`, so the write does not hold up the +/// executor. +pub type SharedProtection = Arc>; + +/// The highest source and target epoch signed for one validator. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct Signed { + source: Epoch, + target: Epoch, +} + +impl Signed { + /// Each epoch's maximum, so that merging two records never lowers either + /// bar. + fn max_with(self, other: Signed) -> Signed { + Signed { + source: self.source.max(other.source), + target: self.target.max(other.target), + } + } +} + +/// What one validator has signed: its watermarks. +/// +/// `None` is "never signed", kept distinct from zero so that slot 0 and epoch +/// 0 stay signable once and only once. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub(crate) struct Record { + block_slot: Option, + attestation: Option, +} + +impl Record { + fn check_block(&self, slot: Slot) -> std::result::Result<(), Refusal> { + match self.block_slot { + Some(signed) if slot <= signed => Err(Refusal::SlotNotAdvanced { + signed, + proposed: slot, + }), + _ => Ok(()), + } + } + + fn check_attestation(&self, data: &AttestationData) -> std::result::Result<(), Refusal> { + let (source, target) = (data.source.epoch, data.target.epoch); + if source > target { + return Err(Refusal::SourceAfterTarget { source, target }); + } + let Some(signed) = self.attestation else { + return Ok(()); + }; + if target <= signed.target { + return Err(Refusal::TargetNotAdvanced { + signed: signed.target, + proposed: target, + }); + } + if source < signed.source { + return Err(Refusal::SourceRegressed { + signed: signed.source, + proposed: source, + }); + } + Ok(()) + } + + fn with_block(self, slot: Slot) -> Record { + Record { + block_slot: Some(self.block_slot.map_or(slot, |signed| signed.max(slot))), + ..self + } + } + + fn with_attestation(self, data: &AttestationData) -> Record { + let next = Signed { + source: data.source.epoch, + target: data.target.epoch, + }; + Record { + attestation: Some( + self.attestation + .map_or(next, |signed| signed.max_with(next)), + ), + ..self + } + } +} + +/// Why a message was refused. +/// +/// Carried out to the caller so the log says which rule fired: a repeated +/// target usually means the clock moved or a schedule was replaced, while a +/// regressed source or an unregistered key means something stranger. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Refusal { + /// The key has no record, so nothing is known about what it signed. + /// + /// Not reachable through ordinary operation: startup refuses a key with no + /// record, and the keymanager registers a key before loading it. Refused + /// rather than signed if it is ever reached, since signing with no record + /// is exactly what this module exists to prevent. + Unregistered, + /// The attestation's target epoch is at or below one already signed. + /// + /// The double-vote shape. Equality is refused as well as regression: the + /// same target signed twice is a double vote unless the two attestations + /// are byte-identical, and the record deliberately does not keep enough to + /// tell those apart. + TargetNotAdvanced { signed: Epoch, proposed: Epoch }, + /// The attestation's source epoch is below one already signed. + /// + /// The surround shape: a lower source with a higher target surrounds the + /// earlier attestation. + SourceRegressed { signed: Epoch, proposed: Epoch }, + /// The attestation's source epoch is after its own target epoch. Never + /// valid, and never produced by an honest beacon node. + SourceAfterTarget { source: Epoch, target: Epoch }, + /// The block's slot is at or below one already signed. + /// + /// Equality is refused for the reason it is for an attestation's target, + /// and it would not help to allow it: a block re-produced for the same + /// slot is almost never identical, since the beacon node packs whatever + /// attestations have arrived since. + SlotNotAdvanced { signed: Slot, proposed: Slot }, +} + +impl std::fmt::Display for Refusal { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unregistered => write!(f, "the validator has no slashing-protection record"), + Self::TargetNotAdvanced { signed, proposed } => write!( + f, + "target epoch {proposed} does not advance past {signed}, already signed" + ), + Self::SourceRegressed { signed, proposed } => write!( + f, + "source epoch {proposed} is below {signed}, already signed" + ), + Self::SourceAfterTarget { source, target } => { + write!(f, "source epoch {source} is after target epoch {target}") + } + Self::SlotNotAdvanced { signed, proposed } => write!( + f, + "slot {proposed} does not advance past {signed}, already signed" + ), + } + } +} + +/// Where the record lives, and the lock that keeps it to this process. +#[derive(Debug)] +struct Storage { + path: PathBuf, + /// Held, never read: dropping it releases the lock. + _lock: std::fs::File, +} + +/// Every registered validator's watermarks, and where they are kept. +/// +/// Not `Clone`: one per process is the point. A copy would be a second opinion +/// about what has been signed, which is worse than none. +#[derive(Debug)] +pub struct SlashingProtection { + genesis_validators_root: Root, + records: HashMap, + /// `None` only for [`Self::in_memory`]. + storage: Option, +} + +impl SlashingProtection { + /// Open the record in `validators_dir` for the chain named by + /// `genesis_validators_root`, and hold its lock until this is dropped. + /// + /// Every key in `keys` must already have a record. A key without one is + /// refused unless `init` is set, in which case it is registered as never + /// having signed: the operator's statement that this key has not signed + /// anything on this chain that the record should know about. A missing file + /// is created only on the same terms, or when there are no keys at all. + pub fn open( + validators_dir: &Path, + genesis_validators_root: Root, + keys: &[BlsPubkey], + init: bool, + ) -> Result { + let lock = file::lock(validators_dir)?; + let path = validators_dir.join(file::FILE_NAME); + let refused = |reason: String| Error::SlashingProtection { + path: path.display().to_string(), + reason, + }; + + let existed = path.exists(); + let mut records = if existed { + let (stored_root, records) = file::read(&path)?; + if stored_root != genesis_validators_root { + return Err(refused(format!( + "the record is for genesis_validators_root {}, but the beacon node is on {}; \ + these keys' history belongs to another chain", + encode_hex(&stored_root.0), + encode_hex(&genesis_validators_root.0) + ))); + } + records + } else { + HashMap::new() + }; + + let missing: Vec = keys + .iter() + .filter(|key| !records.contains_key(*key)) + .copied() + .collect(); + if !missing.is_empty() && !init { + let first = encode_hex(&missing[0].0); + return Err(refused(format!( + "{} validator key(s) have no slashing-protection record (first: {first}). \ + Import their history from the client that last ran them, or, only if they \ + have not signed anything for at least two epochs, start once with \ + --init-slashing-protection", + missing.len() + ))); + } + for key in &missing { + warn!( + pubkey = %encode_hex(&key.0), + "Registering a validator with no slashing-protection history \ + (--init-slashing-protection)" + ); + records.insert(*key, Record::default()); + } + + if !existed || !missing.is_empty() { + file::write(&path, genesis_validators_root, &records)?; + } + Ok(Self { + genesis_validators_root, + records, + storage: Some(Storage { path, _lock: lock }), + }) + } + + /// A record kept in memory only, with every key treated as registered on + /// first use. + /// + /// For tests, here and in other crates driving the duty services: it + /// protects within one process and nothing more, which is exactly what the + /// in-memory guards this module replaced did. The client itself only ever + /// uses [`Self::open`]. + pub fn in_memory() -> SharedProtection { + Arc::new(Mutex::new(Self { + genesis_validators_root: Root::ZERO, + records: HashMap::new(), + storage: None, + })) + } + + /// The record's path, or `None` for [`Self::in_memory`]. + pub fn path(&self) -> Option<&Path> { + self.storage.as_ref().map(|storage| storage.path.as_path()) + } + + /// How many validators have a record. + pub fn len(&self) -> usize { + self.records.len() + } + + pub fn is_empty(&self) -> bool { + self.records.is_empty() + } + + fn record(&self, pubkey: &BlsPubkey) -> std::result::Result { + match self.records.get(pubkey) { + Some(record) => Ok(*record), + None if self.storage.is_none() => Ok(Record::default()), + None => Err(Refusal::Unregistered), + } + } + + /// Whether a block for `slot` may be signed for `pubkey`, recording + /// nothing. + /// + /// For deciding before paying for a block: production costs the beacon + /// node an execution payload build. Not a substitute for + /// [`Self::check_and_record_block`], which is the check that counts. + pub fn check_block(&self, pubkey: &BlsPubkey, slot: Slot) -> std::result::Result<(), Refusal> { + self.record(pubkey)?.check_block(slot) + } + + /// Check a block for `slot` and, if it may be signed, record it durably. + /// + /// The outer `Err` is a record that could not be written, in which case + /// nothing changed and nothing may be signed. + pub fn check_and_record_block( + &mut self, + pubkey: &BlsPubkey, + slot: Slot, + ) -> Result> { + let record = match self.record(pubkey) { + Ok(record) => record, + Err(refusal) => return Ok(Err(refusal)), + }; + if let Err(refusal) = record.check_block(slot) { + return Ok(Err(refusal)); + } + self.commit(HashMap::from([(*pubkey, record.with_block(slot))]))?; + Ok(Ok(())) + } + + /// Check one slot's attestations, all over `data`, and durably record every + /// one that may be signed, in a single write. + /// + /// The verdicts are in `pubkeys` order. A key listed twice is judged + /// against its first verdict, so it is refused the second time. The outer + /// `Err` is a record that could not be written, in which case nothing + /// changed and none of them may be signed. + pub fn check_and_record_attestations( + &mut self, + pubkeys: &[BlsPubkey], + data: &AttestationData, + ) -> Result>> { + let mut staged: HashMap = HashMap::new(); + let verdicts = pubkeys + .iter() + .map(|pubkey| { + let record = match staged.get(pubkey) { + Some(record) => *record, + None => self.record(pubkey)?, + }; + record.check_attestation(data)?; + staged.insert(*pubkey, record.with_attestation(data)); + Ok(()) + }) + .collect(); + if !staged.is_empty() { + self.commit(staged)?; + } + Ok(verdicts) + } + + /// Register keys with no record as never having signed, and persist. + /// + /// For keys added at runtime through the keymanager API, which loads a key + /// only after this returns. A key that already has a record keeps it: + /// registering never lowers a watermark. Returns how many were new. + pub fn register(&mut self, pubkeys: &[BlsPubkey]) -> Result { + let new: HashMap = pubkeys + .iter() + .filter(|pubkey| !self.records.contains_key(*pubkey)) + .map(|pubkey| (*pubkey, Record::default())) + .collect(); + let count = new.len(); + if count > 0 { + self.commit(new)?; + } + Ok(count) + } + + /// Apply `changes` to the record: written to disk first, and taken into + /// memory only once the write has succeeded, so a failed write leaves both + /// exactly as they were. + fn commit(&mut self, changes: HashMap) -> Result<()> { + let Some(storage) = &self.storage else { + self.records.extend(changes); + return Ok(()); + }; + let mut next = self.records.clone(); + next.extend(changes); + let _timing = crate::metrics::time_slashing_protection_write(); + file::write(&storage.path, self.genesis_validators_root, &next) + .inspect_err(|_| crate::metrics::inc_slashing_protection_write_failures())?; + self.records = next; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use ethlambda_types::beacon::containers::shared::Checkpoint; + use ethlambda_types::beacon::primitives::H256; + + use super::*; + + fn pubkey(byte: u8) -> BlsPubkey { + BlsPubkey([byte; 48]) + } + + fn root(byte: u8) -> Root { + H256([byte; 32]) + } + + /// An attestation at `slot` voting source `source` and target `target`. + /// + /// `beacon_block_root` is a parameter because the double-vote case that + /// matters most is two attestations with one target epoch and *different* + /// roots, which is what a re-fetch after a block arrival produces. + fn data(slot: u64, source: Epoch, target: Epoch, block_root: u8) -> AttestationData { + AttestationData { + slot, + index: 0, + beacon_block_root: H256([block_root; 32]), + source: Checkpoint { + epoch: source, + root: Root::ZERO, + }, + target: Checkpoint { + epoch: target, + root: Root::ZERO, + }, + } + } + + /// One attestation, judged and recorded on its own. + fn attest( + protection: &mut SlashingProtection, + key: u8, + data: &AttestationData, + ) -> std::result::Result<(), Refusal> { + let mut verdicts = protection + .check_and_record_attestations(&[pubkey(key)], data) + .expect("the record is written"); + verdicts.pop().expect("one verdict") + } + + fn propose( + protection: &mut SlashingProtection, + key: u8, + slot: Slot, + ) -> std::result::Result<(), Refusal> { + protection + .check_and_record_block(&pubkey(key), slot) + .expect("the record is written") + } + + fn memory() -> SlashingProtection { + Arc::into_inner(SlashingProtection::in_memory()) + .expect("sole owner") + .into_inner() + .expect("not poisoned") + } + + fn open(dir: &Path, keys: &[u8], init: bool) -> Result { + let keys: Vec = keys.iter().map(|byte| pubkey(*byte)).collect(); + SlashingProtection::open(dir, root(0x42), &keys, init) + } + + // The rule. Carried over from the in-memory guards this replaces. + + #[test] + fn a_first_attestation_is_allowed() { + assert!(attest(&mut memory(), 1, &data(96, 2, 3, 0xaa)).is_ok()); + } + + #[test] + fn advancing_the_target_is_allowed() { + let mut protection = memory(); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("first"); + assert!(attest(&mut protection, 1, &data(128, 3, 4, 0xbb)).is_ok()); + } + + /// The backward-clock-step case: same validator, same target epoch, a + /// different block root because the data was re-fetched after a block + /// arrived. + #[test] + fn re_attesting_one_target_with_different_data_is_refused() { + let mut protection = memory(); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("first"); + assert_eq!( + attest(&mut protection, 1, &data(96, 2, 3, 0xbb)), + Err(Refusal::TargetNotAdvanced { + signed: 3, + proposed: 3 + }) + ); + } + + /// The mid-epoch schedule replacement case: a different slot in the same + /// epoch is still one target epoch and still a double vote. + #[test] + fn a_different_slot_in_the_same_target_epoch_is_refused() { + let mut protection = memory(); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("first"); + assert!(attest(&mut protection, 1, &data(101, 2, 3, 0xbb)).is_err()); + } + + #[test] + fn a_regressed_target_is_refused() { + let mut protection = memory(); + attest(&mut protection, 1, &data(128, 3, 4, 0xaa)).expect("first"); + assert!(attest(&mut protection, 1, &data(96, 2, 3, 0xbb)).is_err()); + } + + /// The surround shape: a later target with an earlier source. + #[test] + fn a_regressed_source_under_an_advancing_target_is_refused() { + let mut protection = memory(); + attest(&mut protection, 1, &data(128, 3, 4, 0xaa)).expect("first"); + assert_eq!( + attest(&mut protection, 1, &data(160, 1, 5, 0xbb)), + Err(Refusal::SourceRegressed { + signed: 3, + proposed: 1 + }) + ); + } + + #[test] + fn a_source_after_its_target_is_refused() { + assert_eq!( + attest(&mut memory(), 1, &data(96, 4, 3, 0xaa)), + Err(Refusal::SourceAfterTarget { + source: 4, + target: 3 + }) + ); + } + + #[test] + fn one_validator_does_not_block_another_in_the_same_epoch() { + let mut protection = memory(); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("first validator"); + assert!(attest(&mut protection, 2, &data(101, 2, 3, 0xaa)).is_ok()); + } + + /// A refusal must not move the bar it was measured against. + #[test] + fn a_refusal_records_nothing() { + let mut protection = memory(); + attest(&mut protection, 1, &data(128, 3, 4, 0xaa)).expect("first"); + attest(&mut protection, 1, &data(160, 1, 5, 0xbb)).expect_err("surround refused"); + assert!( + attest(&mut protection, 1, &data(160, 3, 5, 0xcc)).is_ok(), + "the refused attempt must not have lowered the recorded source" + ); + } + + #[test] + fn a_key_twice_in_one_batch_is_refused_the_second_time() { + let mut protection = memory(); + let verdicts = protection + .check_and_record_attestations( + &[pubkey(1), pubkey(2), pubkey(1)], + &data(96, 2, 3, 0xaa), + ) + .expect("written"); + assert!(verdicts[0].is_ok()); + assert!(verdicts[1].is_ok()); + assert!(verdicts[2].is_err(), "the same vote twice in one batch"); + } + + #[test] + fn proposing_the_same_slot_twice_is_refused() { + let mut protection = memory(); + propose(&mut protection, 1, 96).expect("first"); + assert_eq!( + propose(&mut protection, 1, 96), + Err(Refusal::SlotNotAdvanced { + signed: 96, + proposed: 96 + }) + ); + } + + #[test] + fn a_regressed_slot_is_refused_and_another_validator_is_not() { + let mut protection = memory(); + propose(&mut protection, 1, 128).expect("first"); + assert!(propose(&mut protection, 1, 96).is_err()); + assert!(propose(&mut protection, 2, 96).is_ok()); + } + + #[test] + fn check_block_alone_records_nothing() { + let protection = memory(); + protection.check_block(&pubkey(1), 96).expect("allowed"); + protection + .check_block(&pubkey(1), 96) + .expect("still allowed, because check records nothing"); + } + + /// Slot 0 is a real slot: "never proposed" must not be spelled as zero. + #[test] + fn slot_zero_is_recorded_like_any_other() { + let mut protection = memory(); + propose(&mut protection, 1, 0).expect("first"); + assert!(propose(&mut protection, 1, 0).is_err()); + } + + /// A proposal and an attestation are judged on separate watermarks: a + /// validator can legitimately propose in an epoch it also attests in. + #[test] + fn a_proposal_does_not_block_an_attestation() { + let mut protection = memory(); + propose(&mut protection, 1, 100).expect("block"); + assert!(attest(&mut protection, 1, &data(100, 2, 3, 0xaa)).is_ok()); + } + + // Durability: the record outlives the process. + + #[test] + fn a_reopened_record_remembers_what_was_signed() { + let dir = tempfile::tempdir().expect("temp dir"); + { + let mut protection = open(dir.path(), &[1], true).expect("init"); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("attest"); + propose(&mut protection, 1, 97).expect("propose"); + } + + let mut protection = open(dir.path(), &[1], false).expect("reopen without init"); + assert!( + attest(&mut protection, 1, &data(96, 2, 3, 0xbb)).is_err(), + "the double vote from before the restart must be refused" + ); + assert!(propose(&mut protection, 1, 97).is_err()); + assert!(attest(&mut protection, 1, &data(128, 3, 4, 0xcc)).is_ok()); + } + + #[test] + fn a_key_with_no_record_is_refused_without_init() { + let dir = tempfile::tempdir().expect("temp dir"); + drop(open(dir.path(), &[1], true).expect("init")); + + let err = open(dir.path(), &[1, 2], false).expect_err("key 2 has no record"); + assert!( + err.to_string().contains("--init-slashing-protection"), + "{err}" + ); + + drop(open(dir.path(), &[1, 2], true).expect("init registers key 2")); + open(dir.path(), &[1, 2], false).expect("both now registered"); + } + + #[test] + fn a_missing_file_is_created_only_with_init_or_no_keys() { + let dir = tempfile::tempdir().expect("temp dir"); + open(dir.path(), &[1], false).expect_err("keys but no record"); + assert!(!dir.path().join(file::FILE_NAME).exists()); + + drop(open(dir.path(), &[], false).expect("no keys, nothing to protect yet")); + assert!(dir.path().join(file::FILE_NAME).exists()); + } + + #[test] + fn init_never_lowers_an_existing_record() { + let dir = tempfile::tempdir().expect("temp dir"); + { + let mut protection = open(dir.path(), &[1], true).expect("init"); + propose(&mut protection, 1, 97).expect("propose"); + } + let mut protection = open(dir.path(), &[1], true).expect("init again"); + assert!(propose(&mut protection, 1, 97).is_err()); + } + + #[test] + fn another_chain_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + drop(open(dir.path(), &[1], true).expect("init")); + + let err = SlashingProtection::open(dir.path(), root(0x99), &[pubkey(1)], true) + .expect_err("another genesis_validators_root"); + assert!(err.to_string().contains("another chain"), "{err}"); + } + + #[test] + fn a_second_process_is_refused_while_the_first_holds_the_lock() { + let dir = tempfile::tempdir().expect("temp dir"); + let first = open(dir.path(), &[1], true).expect("first"); + + let err = open(dir.path(), &[1], false).expect_err("locked"); + assert!(err.to_string().contains("another process"), "{err}"); + + drop(first); + open(dir.path(), &[1], false).expect("free once the first is gone"); + } + + /// Every damaged shape is a startup error, never an empty record. + #[test] + fn a_damaged_file_fails_closed() { + let cases = [ + ("empty", String::new()), + ("whitespace", " \n".to_string()), + ("truncated", r#"{"metadata":{"interchange_format_version":"5","#.to_string()), + ("not json", "garbage".to_string()), + ( + "wrong version", + r#"{"metadata":{"interchange_format_version":"4","genesis_validators_root":"0x4242424242424242424242424242424242424242424242424242424242424242"},"data":[]}"#.to_string(), + ), + ]; + for (name, contents) in cases { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write(dir.path().join(file::FILE_NAME), contents).expect("write"); + assert!( + open(dir.path(), &[], true).is_err(), + "{name} must fail closed" + ); + } + } + + #[test] + fn a_validator_listed_twice_fails_closed() { + let dir = tempfile::tempdir().expect("temp dir"); + let key = encode_hex(&pubkey(1).0); + let entry = format!(r#"{{"pubkey":"{key}","signed_blocks":[],"signed_attestations":[]}}"#); + let contents = format!( + r#"{{"metadata":{{"interchange_format_version":"5","genesis_validators_root":"{}"}},"data":[{entry},{entry}]}}"#, + encode_hex(&root(0x42).0) + ); + std::fs::write(dir.path().join(file::FILE_NAME), contents).expect("write"); + + let err = open(dir.path(), &[1], false).expect_err("duplicate"); + assert!(err.to_string().contains("more than once"), "{err}"); + } + + /// A write that fails changes nothing: not the file, and not what the + /// process believes was signed. + #[cfg(unix)] + #[test] + fn a_failed_write_refuses_and_changes_nothing() { + use std::os::unix::fs::PermissionsExt as _; + + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("attest"); + + let read_only = std::fs::Permissions::from_mode(0o500); + std::fs::set_permissions(dir.path(), read_only).expect("chmod"); + let result = protection.check_and_record_attestations(&[pubkey(1)], &data(128, 3, 4, 0xbb)); + let writable = std::fs::Permissions::from_mode(0o700); + std::fs::set_permissions(dir.path(), writable).expect("chmod back"); + + assert!( + result.is_err(), + "the write must fail in a read-only directory" + ); + assert!( + attest(&mut protection, 1, &data(128, 3, 4, 0xbb)).is_ok(), + "the failed write must not have recorded target 4" + ); + } + + #[test] + fn the_file_is_an_eip_3076_minimal_interchange() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("attest"); + propose(&mut protection, 1, 97).expect("propose"); + + let text = std::fs::read_to_string(dir.path().join(file::FILE_NAME)).expect("read"); + let json: serde_json::Value = serde_json::from_str(&text).expect("json"); + assert_eq!(json["metadata"]["interchange_format_version"], "5"); + assert_eq!( + json["metadata"]["genesis_validators_root"], + encode_hex(&root(0x42).0) + ); + let entry = &json["data"][0]; + assert_eq!(entry["pubkey"], encode_hex(&pubkey(1).0)); + assert_eq!(entry["signed_blocks"][0]["slot"], "97"); + assert_eq!(entry["signed_attestations"][0]["source_epoch"], "2"); + assert_eq!(entry["signed_attestations"][0]["target_epoch"], "3"); + } + + #[test] + fn registering_keeps_an_existing_record() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + propose(&mut protection, 1, 97).expect("propose"); + + assert_eq!( + protection + .register(&[pubkey(1), pubkey(2)]) + .expect("register"), + 1 + ); + assert!(propose(&mut protection, 1, 97).is_err()); + assert!(propose(&mut protection, 2, 97).is_ok()); + } + + #[test] + fn an_unregistered_key_is_refused_on_a_persisted_record() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + assert_eq!(propose(&mut protection, 2, 97), Err(Refusal::Unregistered)); + assert_eq!( + attest(&mut protection, 2, &data(96, 2, 3, 0xaa)), + Err(Refusal::Unregistered) + ); + } +} diff --git a/docs/cli.md b/docs/cli.md index 2dcbf6703..4fe979c81 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -262,17 +262,17 @@ Both chains run a chain actor, so `RunningNode` carries a plain `BlockChain`. ## `validator` flags -`ethlambda validator` is not a node. It binds no libp2p port, opens no +`ethlambda validator` is not a node. It binds no libp2p port, opens no chain database, runs no discovery and follows no chain: it holds validator keys and performs duties against a beacon node's standard REST API. It works against any conformant beacon node, this repository's `beacon` subcommand or another implementation's, so none of the [common flags](#common-flags) apply to it. -> **It keeps no slashing-protection record.** Read -> [Spec Deviations](./spec_deviations.md#the-validator-client-keeps-no-slashing-protection-record) -> before running it with keys that hold real stake. In short: do not run these -> keys in any other client while this one runs, and treat a restart with the -> same care as a manual key move. The client repeats this warning at startup. +> **Slashing protection** lives in `--validators-dir/slashing_protection.json`, +> locked to one process. A key with no record is refused at startup unless +> `--init-slashing-protection` is passed. Importing another client's history is +> not supported yet. See +> [Spec Deviations](./spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). | Flag | Default | Meaning | |---|---|---| @@ -285,6 +285,7 @@ implementation's, so none of the [common flags](#common-flags) apply to it. | `--suggested-fee-recipient` | none | Execution address to receive block rewards, `0x`-prefixed, for every validator the keymanager API gives no address of its own. Optional, and startup warns when it is absent: without it the beacon node picks an address, and it will not be yours | | `--graffiti` | empty | Text for the graffiti field of proposed blocks, for every validator the keymanager API gives no graffiti of its own. At most 32 bytes as UTF-8, right-padded with zeros. Refused rather than truncated if longer. An ethlambda beacon node appends both clients' versions to it (see [`rpc.md`](rpc.md#validator-endpoints)) | | `--enable-keymanager` | off | Serve the keymanager API: keystores, plus each validator's own fee recipient, graffiti and gas limit. Off by default because it mutates key material | +| `--init-slashing-protection` | off | Register every loaded key with no slashing-protection record as never having signed, instead of refusing to start. Dangerous: only for keys that have signed nothing anywhere for at least two epochs. Never lowers an existing record; remove it once the keys are registered | ### What it does today diff --git a/docs/metrics.md b/docs/metrics.md index 1ea07953f..1228f5afc 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -718,11 +718,11 @@ and absent is not the same as zero. | `ethlambda_validator_attestations_published_total` | Counter | Attestations a beacon node accepted | | `ethlambda_validator_attestation_failures_total` | Counter | Slots whose attestation duty failed and returned | | `ethlambda_validator_attestation_deadline_missed_total` | Counter | Slots whose duty never returned in time and was abandoned. Points at a slow or hung beacon node rather than a rejected attestation | -| `ethlambda_validator_attestations_refused_total` | Counter | Signatures deliberately not attempted, because this process had already signed a conflicting attestation for that validator. Should normally read zero | +| `ethlambda_validator_attestations_refused_total` | Counter | Signatures deliberately not attempted, because slashing protection judged the attestation unsafe or could not record it. Should normally read zero | | `ethlambda_validator_signing_failures_total` | Counter | Signatures attempted and failed | | `ethlambda_validator_blocks_proposed_total` | Counter | Blocks signed and accepted by a beacon node | | `ethlambda_validator_blocks_broadcast_not_imported_total` | Counter | Blocks a node broadcast but could not import into its own database, which is what a 202 means. A subset of `blocks_proposed_total`, not a failure: the block reached the network. Points at that node's execution layer | -| `ethlambda_validator_blocks_refused_total` | Counter | Blocks deliberately not signed, because this process had already proposed that slot for that validator. Should normally read zero | +| `ethlambda_validator_blocks_refused_total` | Counter | Blocks deliberately not signed, because slashing protection judged the block unsafe or could not record it. Should normally read zero | | `ethlambda_validator_block_proposal_failures_total` | Counter | Proposal duties that did not end in a published block, including ones abandoned for overrunning | | `ethlambda_validator_envelopes_published_total` | Counter | Execution payload envelopes a beacon node accepted for blocks this client proposed (gloas) | | `ethlambda_validator_envelope_failures_total` | Counter | Proposed gloas blocks whose self-built envelope was not published. The block is out and the proposal counts as made, but the slot's payload is withheld, so this is the failure that costs the payload. Should read zero | @@ -739,6 +739,8 @@ and absent is not the same as zero. | `ethlambda_validator_block_publication_delay_seconds` | Histogram | Slot start to block accepted. Its buckets are tighter than the attestation histogram's, because a block is due at the slot boundary rather than a third of the way in | | `ethlambda_validator_beacon_node_available` | Gauge | 1 when a beacon node answered the last duty refresh or attestation | | `ethlambda_validator_signing_duration_seconds` | Histogram | Time to sign one slot's batch, store read lock included | +| `ethlambda_validator_slashing_protection_write_seconds` | Histogram | Time to durably write the slashing-protection record, once per slot's attestation batch and once per block, before signing | +| `ethlambda_validator_slashing_protection_write_failures_total` | Counter | Slashing-protection writes that failed, each leaving its messages unsigned. Should read zero; anything else points at the disk under `--validators-dir` | | `ethlambda_validator_publication_delay_seconds` | Histogram | Slot start to attestations accepted. The headline health number: it should sit near the one-third-slot duty offset | Three of these distinguish failures that look alike on a dashboard but have diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 191be46cd..41fd0d6ad 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -40,58 +40,57 @@ target-slot order as they are scanned. - **Equivalence:** both produce a valid block. ethlambda front-loads the attestations that advance justification and finality, and within those tiers prefers the *newest* target where leanSpec takes the *oldest*; combined with the smaller default budget, an older entry can be outranked by newer ones round after round, so which votes reach peers through blocks differs even though every block stays valid. The smaller budget yields smaller blocks and lower build times. - **Upstream status:** the tiered strategy is proposed upstream as leanSpec [PR #1149](https://github.com/leanEthereum/leanSpec/pull/1149) (open at the time of writing), so this deviation may converge; the recursive-merge collapse follows leanSpec #510. -## The validator client keeps no slashing-protection record +## The validator client keeps EIP-3076's minimal watermarks, not full history -`ethlambda validator` signs attestations and blocks without recording what it -has signed. It cannot detect that a previous run, or another client holding the -same keys, already signed. +`ethlambda validator` records, per validator, only the highest block slot and +the highest attestation source and target epochs it has signed, and refuses +anything that does not advance past them. - **The specification:** [EIP-3076](https://eips.ethereum.org/EIPS/eip-3076) - defines an interchange format and the conditions a validator client must never - violate. For attestations: signing two *distinct* attestations with the same - target epoch (a double vote), and signing one whose source and target surround - or are surrounded by another's. For blocks: signing two distinct ones for the - same slot. Enforcing any of them requires a durable record, written before the - signature leaves the process. -- **ethlambda:** there is no such record and no store to hold one. This is a - deliberate scope decision for the client's first phase, not an oversight. -- **What partially covers it:** two in-memory guards, one per message kind. - `crate::attestation_guard::AttestationGuard` refuses to sign a second - conflicting attestation for a validator *within one run*, using EIP-3076's - minimal rule (the target must advance, the source must not regress). - `crate::proposal_guard::ProposalGuard` does the same for blocks, with the - minimal rule for those (the slot must strictly advance). Between them they - close the shapes reachable through ordinary operation: a backward wall-clock - step, or a duty schedule replaced mid-epoch. Both are held in memory only, so - a restart empties them and neither knows anything about any other process. + forbids signing two *distinct* attestations with the same target epoch (a + double vote), one whose source and target surround or are surrounded by + another's, and two distinct blocks for one slot. It allows enforcing those + either against the full signing history or with the stricter minimal rule + over watermarks. +- **ethlambda:** the minimal rule. An attestation is signed only if its target + is above the recorded target and its source is not below the recorded source; + a block only if its slot is above the recorded slot. The record is + `/slashing_protection.json`, an EIP-3076 version 5 minimal + interchange naming the chain's `genesis_validators_root`. It is written to a + temporary file, flushed, renamed into place and the directory flushed, all + before the signature exists, and a write that fails leaves the message + unsigned. A file that is empty, malformed, of another version or for another + chain stops the client at startup rather than being read as "nothing + signed". An exclusive lock (`slashing_protection.lock`) keeps a second process + away from it. See `crate::slashing_protection`. +- **What the strictness costs:** a byte-identical re-sign is refused. A client + that recorded an attestation, crashed before publishing it, and restarted + into the same slot loses that attestation rather than publishing it; the same + holds for a block. An honest validator's messages otherwise advance this way, + so nothing else is refused. Lighthouse and Prysm keep signing roots to allow + the identical re-sign; Teku keeps the same watermarks as this client. +- **Keys with no record:** refused at startup. `--init-slashing-protection` + registers them as never having signed, which is safe only for keys that have + signed nothing anywhere for at least two epochs. A key imported through the + keymanager API is registered the same way before it is loaded. - **Gloas messages:** a gloas proposer also signs an execution payload envelope, and a payload timeliness committee member signs a payload - attestation. Neither is slashable, so neither has a guard. An envelope is + attestation. Neither is slashable, so neither is recorded. An envelope is published once per proposal, right after its block, and a failure to publish it is logged at error and counted without failing the proposal. A PTC vote has an in-memory dedup of `(validator, slot)` pairs recorded at signing time (`PayloadAttestationService`), so a duty abandoned between signing and - submission is lost rather than re-signed. It is emptied by a restart and says - nothing about another process, so it is not slashing protection either: a - second, different vote for one slot is equivocation that peers penalize in - gossip, not a slashing. -- **Why a proposer slashing is the worse of the two:** a double vote needs a - second validator's attestation to be caught. Two signed block headers for one - slot are the whole of the evidence on their own. -- **What is therefore not covered:** restarting into a state where the chain has - moved, and running these keys in two places at once. Both can produce a - slashable double vote or a double proposal, and nothing in this client will - stop them. -- **Keymanager consequences:** `DELETE /eth/v1/keystores` must return an - EIP-3076 interchange. This client returns a well-formed but empty one whose - `genesis_validators_root` is deliberately all-zero, so a conformant importer - rejects it outright rather than trusting an empty history that was never - recorded. `POST` accepts the `slashing_protection` field and ignores it, with - a server-side warning. A key migrated in or out through this API does not - carry its history. -- **Operationally:** do not run these keys in any other client while this one - runs, and treat a restart as an event that needs the same care a manual key - move would. The client warns about this at startup on every run. + submission is lost rather than re-signed. A second, different vote for one + slot is equivocation that peers penalize in gossip, not a slashing. +- **What is not covered:** the same keys running in another client on another + machine. No local record can see that. +- **Keymanager consequences:** history is not carried in or out yet. + `DELETE /eth/v1/keystores` must return an EIP-3076 interchange; this client + returns a well-formed but empty one whose `genesis_validators_root` is + deliberately all-zero, so a conformant importer rejects it outright rather + than trusting an empty history. `POST` accepts the `slashing_protection` + field and ignores it, with a server-side warning, so an imported key starts + as never having signed. ## Builder bids: gossip and API only (no builder API) From 2808b51d42c12eb0b9f11ff37faeb51db1b15308 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 29 Oct 2026 19:03:45 -0300 Subject: [PATCH 2/3] Address review of the durable slashing protection. Keys registered without history (--init-slashing-protection, keymanager imports) now start at the current slot and epoch instead of as never having signed, closing the double-vote and double-proposal window for a key that signed elsewhere a moment ago; the source stays at zero so a lagging justified checkpoint does not stall attestations. The record's existence check fails closed: only a missing file is absence, and a symlink, directory or unreadable path is refused instead of replaced. Keymanager imports register keys in one write after they are persisted, so a failed import leaves no record behind. Every async access to the record, including the proposal pre-check and keymanager registration, goes through one spawn_blocking helper. The durable write moves to secure_fs and is shared with the validator definitions file, which gains its missing fsyncs; each write no longer clones the record and is compact JSON. A failed attestation write counts every refused attestation, the permission-based tests skip under root, stale guard wording is gone, the docs say the lock is per directory and that --validators-dir must be writable, and the Kurtosis battery passes --init-slashing-protection to its two ethlambda validator clients. --- bin/ethlambda/src/validator.rs | 15 +- crates/validator/src/attestation.rs | 27 ++- crates/validator/src/beacon_node/fallback.rs | 8 +- crates/validator/src/beacon_node/mod.rs | 4 +- crates/validator/src/error.rs | 18 +- crates/validator/src/http_api/keystores.rs | 127 +++++++++--- crates/validator/src/http_api/mod.rs | 3 + crates/validator/src/http_api/proposer.rs | 1 + crates/validator/src/keys/definitions.rs | 21 +- crates/validator/src/lib.rs | 8 +- crates/validator/src/metrics.rs | 4 + crates/validator/src/proposal.rs | 95 ++++----- crates/validator/src/secure_fs.rs | 87 +++++++- .../validator/src/slashing_protection/file.rs | 107 ++++------ .../validator/src/slashing_protection/mod.rs | 189 +++++++++++++++--- docs/cli.md | 5 +- docs/spec_deviations.md | 13 +- .../kurtosis-assertoor/network_params.yaml | 4 + 18 files changed, 489 insertions(+), 247 deletions(-) diff --git a/bin/ethlambda/src/validator.rs b/bin/ethlambda/src/validator.rs index 16aefe31c..2ebcde10c 100644 --- a/bin/ethlambda/src/validator.rs +++ b/bin/ethlambda/src/validator.rs @@ -79,15 +79,16 @@ pub(crate) struct ValidatorOptions { #[arg(long, default_value = "false")] pub(crate) enable_keymanager: bool, - /// Register every loaded key that has no slashing-protection record as - /// never having signed, instead of refusing to start. + /// Register every loaded key that has no slashing-protection record, + /// instead of refusing to start. /// /// DANGEROUS. Use it only for keys that are brand new, or that have signed - /// nothing anywhere for at least two epochs. A key that recently signed in - /// another client must bring that history instead: starting it here as - /// "never signed" can produce a slashable double vote. Keys that already - /// have a record keep it, so the flag never lowers one; remove it once the - /// keys are registered. + /// nothing anywhere for at least two epochs. A key is registered as if it + /// had signed a block and an attestation at the current slot and epoch, + /// which closes the double-vote window for a key that signed a moment ago + /// elsewhere, but not a surround vote against older history. Keys that + /// already have a record keep it, so the flag never lowers one; remove it + /// once the keys are registered. #[arg(long, default_value = "false")] pub(crate) init_slashing_protection: bool, } diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index 50189d6df..4a78f3abf 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -155,25 +155,16 @@ impl AttestationService { // and before the store's read lock below is taken, so the disk flush // never holds up a keymanager writer queued behind it. let pubkeys: Vec = candidates.iter().map(|(_, pubkey)| *pubkey).collect(); - let protection = self.protection.clone(); - let recorded_data = data; - let verdicts = tokio::task::spawn_blocking(move || { - let mut protection = protection - .lock() - .map_err(|_| "slashing protection is poisoned".to_string())?; - protection - .check_and_record_attestations(&pubkeys, &recorded_data) - .map_err(|err| err.to_string()) + let verdicts = crate::slashing_protection::blocking(&self.protection, move |protection| { + protection.check_and_record_attestations(&pubkeys, &data) }) - .await - .map_err(|err| err.to_string()) - .and_then(|verdicts| verdicts); + .await; let verdicts = match verdicts { Ok(verdicts) => verdicts, Err(err) => { // Nothing was recorded, so nothing may be signed. error!(%slot, %err, "Slashing protection could not record this slot's attestations; refusing to sign any"); - crate::metrics::inc_attestations_refused(); + crate::metrics::inc_attestations_refused_by(candidates.len() as u64); return Ok(Attested { published: 0, data: Some(data), @@ -344,7 +335,7 @@ mod tests { ); } - /// The guard engaging in the real signing path, not just in isolation. + /// Slashing protection engaging in the real signing path, not just in isolation. /// /// This is the backward-clock-step shape: the duty loop derives its slot /// from the wall clock, so an NTP correction can re-enter a slot already @@ -383,7 +374,8 @@ mod tests { dir: &std::path::Path, keys: &[ethlambda_types::beacon::primitives::BlsPubkey], ) -> SharedProtection { - let protection = SlashingProtection::open(dir, Root::ZERO, keys, true).expect("opens"); + let protection = + SlashingProtection::open(dir, Root::ZERO, keys, true, None).expect("opens"); Arc::new(std::sync::Mutex::new(protection)) } @@ -430,6 +422,11 @@ mod tests { async fn a_record_that_cannot_be_written_signs_nothing() { use std::os::unix::fs::PermissionsExt as _; + // SAFETY: `geteuid` reads a process attribute and cannot fail. + if unsafe { libc::geteuid() } == 0 { + eprintln!("skipped: root bypasses directory permissions"); + return; + } let mut store = ValidatorStore::new(); let pubkey = store.insert_secret("test", &secret()).expect("inserts"); let store = RwLock::new(store); diff --git a/crates/validator/src/beacon_node/fallback.rs b/crates/validator/src/beacon_node/fallback.rs index 8251f9559..f05107ab0 100644 --- a/crates/validator/src/beacon_node/fallback.rs +++ b/crates/validator/src/beacon_node/fallback.rs @@ -606,8 +606,8 @@ mod tests { /// The proposal-shaped version of the stale-head finding above, and the /// more expensive one to get wrong: a block signed against a stale node's - /// answer is a block for the wrong slot, and it burns the proposal guard's - /// record for that validator on the way out. + /// answer is a block for the wrong slot, and it burns slashing + /// protection's record for that validator on the way out. #[tokio::test] async fn a_node_producing_a_block_for_the_wrong_slot_is_failed_over_from() { let stale = MockBeaconNode::new().with_block(90, 7); @@ -622,8 +622,8 @@ mod tests { } /// A node on a different fork computes a different proposer. Signing its - /// block would produce a signature the network discards, while the guard - /// records the slot as proposed and refuses the real duty. + /// block would produce a signature the network discards, while slashing + /// protection records the slot as proposed and refuses the real duty. #[tokio::test] async fn a_node_naming_the_wrong_proposer_is_failed_over_from() { let wrong = MockBeaconNode::new().with_block(96, 11); diff --git a/crates/validator/src/beacon_node/mod.rs b/crates/validator/src/beacon_node/mod.rs index 9ed3f7bc9..d9f000d7c 100644 --- a/crates/validator/src/beacon_node/mod.rs +++ b/crates/validator/src/beacon_node/mod.rs @@ -156,11 +156,11 @@ pub enum Published { /// stuck on a stale head is never failed over from. /// /// Both fields are checked because both end up signed. The slot selects the -/// signing domain and is the whole of what the proposal guard keys on. The +/// signing domain and is the whole of what slashing protection keys a block on. The /// proposer index is not something this client chooses either: a node on a /// different fork computes a different proposer, and a block naming someone /// else is one this client's key can only sign uselessly, while still burning -/// the guard's record for that slot. +/// the slot in slashing protection's record. pub fn validate_produced_block(request: &BlockRequest, block: &ProducedBlock) -> Result<()> { if block.slot() != request.slot { return Err(crate::error::Error::InconsistentResponse(format!( diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index ee6df5afe..796fb27cd 100644 --- a/crates/validator/src/error.rs +++ b/crates/validator/src/error.rs @@ -72,13 +72,15 @@ pub enum Error { /// abandoned attestation itself is never retried, by design. #[error("attestation duty for slot {slot} ran past the end of its slot")] AttestationDeadline { slot: u64 }, - /// The proposal guard refused to sign a block for this slot. + /// Slashing protection refused to sign a block for this slot, or could not + /// record it. /// - /// Not a beacon-node failure and not retryable: the whole point of a - /// refusal is that trying again produces the same answer. It is an error - /// rather than a quiet `Ok` because a refusal means the duty loop asked for - /// something it should not have, which is worth surfacing even though the - /// block was correctly suppressed. + /// Not a beacon-node failure and not retryable within the slot: a refusal + /// gives the same answer again, and a record that could not be written + /// needs an operator, not a retry. It is an error rather than a quiet `Ok` + /// because either way the duty loop asked for something that was not + /// signed, which is worth surfacing even though the block was correctly + /// suppressed. #[error("refused to propose a block for slot {slot}: {reason}")] ProposalRefused { slot: u64, reason: String }, #[error("beacon node returned an inconsistent response: {0}")] @@ -137,8 +139,8 @@ impl Error { | Self::InconsistentResponse(_) | Self::AttestationDeadline { .. } | Self::Decode(_) => true, - // A refusal is deterministic: the guard will refuse the same slot - // again, so retrying only repeats it. + // A refusal repeats itself, and a record that could not be written + // needs an operator: retrying within the slot helps neither. Self::ProposalRefused { .. } | Self::Keystore { .. } | Self::KeystoreVersion(_) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index b6a9272ce..0508aee63 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -147,7 +147,7 @@ pub async fn import( if request.slashing_protection.is_some() { warn!( "Ignoring slashing_protection on import: importing EIP-3076 history is not \ - supported yet, so every new key starts as never having signed" + supported yet, so every new key starts at the current slot and epoch" ); } @@ -212,6 +212,36 @@ pub async fn import( prepared.push(prepare_import(&context, &mut definitions, json, password)); } + // Every persisted key is registered in the slashing-protection record in + // one write, before any of them is loaded: the duty path must never meet a + // key with no record. After persisting rather than before, so a key whose + // persist failed leaves no record behind to let a hand-copied keystore + // past the startup check later. A crash in between leaves a key on disk + // with no record, which the next start refuses: closed, not open. Still + // under the definitions guard, so a concurrent delete cannot interleave. + let persisted: Vec = prepared + .iter() + .filter_map(|result| result.as_ref().ok().map(|(pubkey, _)| *pubkey)) + .collect(); + let now = context.clock.now(); + let registration = if persisted.is_empty() { + Ok(0) + } else { + crate::slashing_protection::blocking(&context.protection, move |protection| { + protection.register(&persisted, now) + }) + .await + }; + if let Ok(registered) = registration + && registered > 0 + { + warn!( + registered, + "Imported validator keys with no slashing-protection history; each starts as if it \ + had signed at the current slot and epoch" + ); + } + // The definitions file is fully written by this point; nothing below // touches it, so the guard can be released before the store lock is // taken rather than held across it too. @@ -220,9 +250,15 @@ pub async fn import( let mut store = context.store.write().await; let data = prepared .into_iter() - .map(|result| match result { - Ok(secret) => finish_import(&mut store, &secret), - Err(status) => status, + .map(|result| match (result, ®istration) { + (Ok((_, secret)), Ok(_)) => finish_import(&mut store, &secret), + // Persisted but not registered: not loaded now, and refused at the + // next start until it has a record. + (Ok(_), Err(err)) => KeyStatus::error(format!( + "saved, but not loaded: its slashing-protection record could not be written: \ + {err}" + )), + (Err(status), _) => status, }) .collect(); drop(store); @@ -231,13 +267,13 @@ pub async fn import( } /// Decrypt one keystore and persist it to disk: everything an import can do -/// before it needs the store's write lock. +/// before it needs the store's write lock and its slashing-protection record. fn prepare_import( context: &KeymanagerContext, definitions: &mut ValidatorDefinitions, json: &str, password: Option<&str>, -) -> std::result::Result, KeyStatus> { +) -> std::result::Result<(BlsPubkey, Zeroizing<[u8; 32]>), KeyStatus> { let Some(password) = password else { return Err(KeyStatus::error("no password supplied for this keystore")); }; @@ -246,25 +282,6 @@ fn prepare_import( let secret = keystore.decrypt(password).map_err(KeyStatus::error)?; let pubkey = ValidatorStore::derive_pubkey(&secret).map_err(KeyStatus::error)?; - // Registered before the key is written to disk or reachable through the - // store, so the duty path never meets a key it has no record for, and a - // restart finds a record for every key on disk. A key that already has a - // record keeps it. The history the request may carry is not imported yet, - // so a key arriving from another client starts as never having signed: - // see the warning in `import`. - let registered = context - .protection - .lock() - .map_err(|_| KeyStatus::error("slashing protection is poisoned"))? - .register(&[pubkey]) - .map_err(KeyStatus::error)?; - if registered > 0 { - warn!( - pubkey = %encode_hex(&pubkey.0), - "Imported a validator key with no slashing-protection history" - ); - } - // Persisted before the key is reachable through the store: a validator // this process is willing to sign with must already be durable, or a // crash right after "imported" is reported silently loses it, with @@ -272,7 +289,7 @@ fn prepare_import( // vanish on the next restart. persist_import(context, definitions, &pubkey, json, password).map_err(KeyStatus::error)?; - Ok(secret) + Ok((pubkey, secret)) } /// Add an already-decrypted, already-persisted key to the live store. The @@ -587,6 +604,7 @@ mod tests { secrets_dir: dir.to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), protection: crate::slashing_protection::SlashingProtection::in_memory(), + clock: crate::slot_clock::SlotClock::new(0, 12_000, 3_333, 6_667), } } @@ -930,9 +948,14 @@ mod tests { async fn an_imported_key_is_registered_for_slashing_protection() { let dir = tempfile::tempdir().expect("temp dir"); let root = ethlambda_types::beacon::primitives::Root::ZERO; - let protection = - crate::slashing_protection::SlashingProtection::open(dir.path(), root, &[], false) - .expect("opens with no keys"); + let protection = crate::slashing_protection::SlashingProtection::open( + dir.path(), + root, + &[], + false, + None, + ) + .expect("opens with no keys"); let mut context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); context.protection = Arc::new(std::sync::Mutex::new(protection)); @@ -958,10 +981,54 @@ mod tests { // registered. drop(protection); let keys = ValidatorStore::load(dir.path()).expect("loads").pubkeys(); - crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false) + crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false, None) .expect("the imported key has a record"); } + /// A key that fails to persist must leave no slashing-protection record + /// behind: a record would later let the same keystore, copied in by hand, + /// past the startup check as if it had been registered on purpose. + #[cfg(unix)] + #[tokio::test] + async fn a_failed_import_leaves_no_slashing_protection_record() { + let dir = tempfile::tempdir().expect("temp dir"); + // A symlink at the keystore's path makes `persist_import` refuse. + let target = dir.path().join("elsewhere"); + std::fs::write(&target, "untouched").expect("writes target"); + let pubkey = "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07"; + std::os::unix::fs::symlink(&target, dir.path().join(format!("{pubkey}.json"))) + .expect("symlinks"); + + let root = ethlambda_types::beacon::primitives::Root::ZERO; + let protection = crate::slashing_protection::SlashingProtection::open( + dir.path(), + root, + &[], + false, + None, + ) + .expect("opens with no keys"); + let mut context = + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + let protection = context.protection.clone(); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ "keystores": [keystore_json()], "passwords": [PASSWORD] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "error", "got {body}"); + assert!( + protection.lock().expect("not poisoned").is_empty(), + "the failed key must not have been registered" + ); + } + #[tokio::test] async fn deleting_an_unknown_key_reports_not_found() { let (_, body) = send( diff --git a/crates/validator/src/http_api/mod.rs b/crates/validator/src/http_api/mod.rs index 2a7abb54c..812e12f3d 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -61,6 +61,9 @@ pub struct KeymanagerContext { /// The record every key must be registered in before it is loaded, so the /// duty path never meets a key it has no record for. pub protection: crate::slashing_protection::SharedProtection, + /// The current slot, which dates the record of a key imported with no + /// history. See `SlashingProtection::register`. + pub clock: crate::slot_clock::SlotClock, } /// The file the generated token is written to, inside the validators directory. diff --git a/crates/validator/src/http_api/proposer.rs b/crates/validator/src/http_api/proposer.rs index b9f98aed3..196e9eceb 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -368,6 +368,7 @@ mod tests { secrets_dir: dir.path().to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), protection: crate::slashing_protection::SlashingProtection::in_memory(), + clock: crate::slot_clock::SlotClock::new(0, 12_000, 3_333, 6_667), }; Harness { app: router(context, TOKEN.to_string()), diff --git a/crates/validator/src/keys/definitions.rs b/crates/validator/src/keys/definitions.rs index ab48a8db0..1b5495314 100644 --- a/crates/validator/src/keys/definitions.rs +++ b/crates/validator/src/keys/definitions.rs @@ -51,12 +51,11 @@ impl ValidatorDefinitions { /// Write the definitions back, replacing the file. /// - /// Writes to a sibling temporary file and renames it over the target - /// rather than truncating in place. The keymanager API calls this while - /// validators are signing, and a crash partway through an in-place write - /// would leave a truncated file that stops the client booting, losing every - /// validator rather than the one being changed. `rename` within one - /// directory is atomic, so a reader sees the old file or the new one. + /// Through [`secure_fs::write_private_durably`] rather than truncating in + /// place. The keymanager API calls this while validators are signing, and + /// a crash partway through an in-place write would leave a truncated file + /// that stops the client booting, losing every validator rather than the + /// one being changed. pub fn save(&self, validators_dir: &Path) -> Result<()> { let path = validators_dir.join(DEFINITIONS_FILE); let contents = serde_yaml_ng::to_string(self).map_err(|err| Error::Keystore { @@ -64,15 +63,7 @@ impl ValidatorDefinitions { reason: err.to_string(), })?; - // Mode set on the temporary file, not the final path: `rename` - // preserves it, and setting it here means the file is never - // world-readable even for the instant between creation and rename. - let temporary = validators_dir.join(format!("{DEFINITIONS_FILE}.tmp")); - secure_fs::write_private(&temporary, contents).map_err(|source| Error::Io { - path: temporary.display().to_string(), - source, - })?; - std::fs::rename(&temporary, &path).map_err(|source| Error::Io { + secure_fs::write_private_durably(&path, contents).map_err(|source| Error::Io { path: path.display().to_string(), source, }) diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index 1d708bc6d..f54ccb2f0 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -175,15 +175,19 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { "Validator client starting" ); + let clock = SlotClock::from_config(genesis.genesis_time, &spec); + // After the beacon node names the chain, which the record must belong to, // and before any service that signs exists. Every loaded key must already - // have a record, unless the operator said otherwise. + // have a record, unless the operator said otherwise; the clock dates the + // records `--init-slashing-protection` creates. let keys = store.read().await.pubkeys(); let protection = slashing_protection::SlashingProtection::open( &config.validators_dir, genesis.genesis_validators_root, &keys, config.init_slashing_protection, + clock.now(), )?; info!( validators = protection.len(), @@ -192,7 +196,6 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { ); let protection = Arc::new(std::sync::Mutex::new(protection)); - let clock = SlotClock::from_config(genesis.genesis_time, &spec); let context = Arc::new(SigningContext { config: spec, genesis_validators_root: genesis.genesis_validators_root, @@ -220,6 +223,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { secrets_dir: config.secrets_dir.clone(), definitions_lock: Arc::new(Mutex::new(())), protection: protection.clone(), + clock: clock.clone(), }; let router = http_api::router(context, token); let listener = tokio::net::TcpListener::bind(address) diff --git a/crates/validator/src/metrics.rs b/crates/validator/src/metrics.rs index b77f8b2b6..d74377b38 100644 --- a/crates/validator/src/metrics.rs +++ b/crates/validator/src/metrics.rs @@ -391,6 +391,10 @@ pub fn inc_attestations_refused() { ATTESTATIONS_REFUSED_TOTAL.inc(); } +pub fn inc_attestations_refused_by(count: u64) { + ATTESTATIONS_REFUSED_TOTAL.inc_by(count); +} + pub fn inc_attestation_deadline_missed() { ATTESTATION_DEADLINE_MISSED_TOTAL.inc(); } diff --git a/crates/validator/src/proposal.rs b/crates/validator/src/proposal.rs index 34b16ba48..946383257 100644 --- a/crates/validator/src/proposal.rs +++ b/crates/validator/src/proposal.rs @@ -162,35 +162,33 @@ impl ProposalService { let pubkey = parse_pubkey(&duty.pubkey)?; let epoch = compute_epoch_at_slot(slot); - // Before paying for a block, not after. See the module doc. - // - // A poisoned lock is treated as a refusal rather than unwrapped: a - // crash in the duty path would be a worse outcome than a skipped - // proposal. - match self.protection.lock() { - Ok(protection) => { - if let Err(refusal) = protection.check_block(&pubkey, slot) { - warn!( - %slot, - validator = duty.validator_index, - %refusal, - "Refusing to propose: slashing protection judged this block unsafe" - ); - crate::metrics::inc_blocks_refused(); - return Err(Error::ProposalRefused { - slot, - reason: refusal.to_string(), - }); - } + // Before paying for a block, not after. See the module doc. Off the + // executor even though it only reads: the record may be mid-write for + // another caller. A record that cannot be consulted is a refusal, not + // a crash in the duty path. + let checked = crate::slashing_protection::blocking(&self.protection, move |protection| { + Ok(protection.check_block(&pubkey, slot)) + }) + .await; + let refusal = match checked { + Ok(Ok(())) => None, + Ok(Err(refusal)) => { + warn!( + %slot, + validator = duty.validator_index, + %refusal, + "Refusing to propose: slashing protection judged this block unsafe" + ); + Some(refusal.to_string()) } Err(err) => { - warn!(%slot, %err, "Slashing protection is poisoned; refusing to propose"); - crate::metrics::inc_blocks_refused(); - return Err(Error::ProposalRefused { - slot, - reason: "slashing protection is poisoned".to_string(), - }); + warn!(%slot, %err, "Slashing protection could not be consulted; refusing to propose"); + Some(err) } + }; + if let Some(reason) = refusal { + crate::metrics::inc_blocks_refused(); + return Err(Error::ProposalRefused { slot, reason }); } // Scoped away from every await, the way the attestation path's read @@ -223,11 +221,11 @@ impl ProposalService { // implementation that failed to honour its contract can be stopped // before it becomes a signature. // - // It matters more here than it reads. The guard records the *requested* - // slot while the signature covers the *produced* header's slot. If - // those ever diverged and both fell in one epoch, the domain would be - // identical and the result would be a valid, unguarded second block for - // a slot already proposed. + // It matters more here than it reads. Slashing protection records the + // *requested* slot while the signature covers the *produced* header's + // slot. If those ever diverged and both fell in one epoch, the domain + // would be identical and the result would be a valid, unrecorded + // second block for a slot already proposed. validate_produced_block(&request, &produced)?; // The node's fork and this client's must agree, or the signature is @@ -263,7 +261,7 @@ impl ProposalService { // The root is taken from the decoded container, never from anything // reassembled here, and it is taken once: the same value is what the - // guard's slot is recorded against and what the signature covers. + // recorded slot belongs to and what the signature covers. let block_root = produced.block_root(); // The check that matters, and the one that records, durably, before @@ -271,18 +269,10 @@ impl ProposalService { // without risk, because the record already counts this slot as // proposed. Off the executor, and before the store's read lock below, // so the disk flush never holds up a keymanager writer. - let protection = self.protection.clone(); - let recorded = tokio::task::spawn_blocking(move || { - let mut protection = protection - .lock() - .map_err(|_| "slashing protection is poisoned".to_string())?; - protection - .check_and_record_block(&pubkey, slot) - .map_err(|err| err.to_string()) + let recorded = crate::slashing_protection::blocking(&self.protection, move |protection| { + protection.check_and_record_block(&pubkey, slot) }) - .await - .map_err(|err| err.to_string()) - .and_then(|recorded| recorded); + .await; let refusal = match recorded { Ok(Ok(())) => None, Ok(Err(refusal)) => Some(refusal.to_string()), @@ -650,7 +640,7 @@ mod tests { ); } - /// The guard is consulted before the block is asked for, not after, so a + /// Slashing protection is consulted before the block is asked for, not after, so a /// slot already proposed costs the beacon node nothing. #[tokio::test] async fn a_repeated_slot_is_refused_without_asking_for_a_block() { @@ -684,8 +674,8 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); let settings = Arc::new(ProposerSettings::new(Bytes32::repeat_byte(0xab), None)); let protection = || { - let opened = - SlashingProtection::open(dir.path(), Root::ZERO, &[pubkey], true).expect("opens"); + let opened = SlashingProtection::open(dir.path(), Root::ZERO, &[pubkey], true, None) + .expect("opens"); Arc::new(std::sync::Mutex::new(opened)) }; @@ -797,12 +787,12 @@ mod tests { ); } - /// A failed production must leave the guard untouched, so the slot can - /// still be proposed if a later attempt succeeds. Recording on the early - /// check rather than at signing time would have burned it. + /// A failed production must leave slashing protection untouched, so the + /// slot can still be proposed if a later attempt succeeds. Recording on + /// the early check rather than at signing time would have burned it. /// /// The same service and the same node throughout, deliberately: a second - /// service would carry a second, empty guard and the test would pass + /// service would carry a second, empty record and the test would pass /// whether or not the first one recorded. #[tokio::test] async fn a_failed_production_does_not_burn_the_slot() { @@ -1069,8 +1059,9 @@ mod gloas_tests { assert!(node.published_envelopes().is_empty()); } - /// The guard's semantics are unchanged: a second proposal for the slot is - /// refused before asking for anything, and reveals no second envelope. + /// Slashing protection's semantics are unchanged at gloas: a second + /// proposal for the slot is refused before asking for anything, and + /// reveals no second envelope. #[tokio::test] async fn a_repeated_gloas_slot_is_refused_and_reveals_nothing_more() { let (store, pubkey) = store(); diff --git a/crates/validator/src/secure_fs.rs b/crates/validator/src/secure_fs.rs index 574056e9f..f412c8171 100644 --- a/crates/validator/src/secure_fs.rs +++ b/crates/validator/src/secure_fs.rs @@ -1,6 +1,6 @@ //! Helpers for writing files that hold secrets or decide what a validator //! signs: the API token, imported keystores and their passwords, and the -//! validator definitions file. +//! validator definitions file, and the slashing-protection record. //! //! `std::fs::write` and `File::create` land at whatever mode the platform //! default gives them, `0644` under a standard umask on unix, which makes @@ -75,6 +75,72 @@ pub(crate) fn write_private_no_symlink( std::fs::write(path, contents) } +/// Replace the file at `path` with `contents` so that a crash or power loss +/// leaves either the old file or the new one, never a mixture, and so that a +/// return of `Ok` means the new one is on disk. +/// +/// Written to a sibling `.tmp` at mode `0600` (through [`write_private`]'s +/// rules), flushed, renamed over `path`, and then the directory is flushed so +/// the rename itself survives. For files whose loss or truncation would change +/// what a validator signs: the slashing-protection record and the validator +/// definitions file. +pub(crate) fn write_private_durably( + path: &Path, + contents: impl AsRef<[u8]>, +) -> std::io::Result<()> { + use std::io::Write as _; + let mut name = path.file_name().unwrap_or_default().to_os_string(); + name.push(".tmp"); + let temporary = path.with_file_name(name); + + let mut file = open_private(&temporary)?; + file.write_all(contents.as_ref())?; + file.sync_all()?; + drop(file); + std::fs::rename(&temporary, path)?; + sync_directory(path.parent().unwrap_or(Path::new("."))) +} + +#[cfg(unix)] +fn open_private(path: &Path) -> std::io::Result { + use std::os::unix::fs::OpenOptionsExt as _; + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path) +} + +#[cfg(not(unix))] +fn open_private(path: &Path) -> std::io::Result { + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .open(path) +} + +/// `fsync` on the directory, so a rename into it is itself durable. Called +/// through `libc` rather than `File::sync_all`, which on macOS issues +/// `F_FULLFSYNC`, a request some filesystems refuse for a directory. +#[cfg(unix)] +fn sync_directory(directory: &Path) -> std::io::Result<()> { + use std::os::fd::AsRawFd as _; + let directory = std::fs::File::open(directory)?; + // SAFETY: the descriptor is owned by `directory`, which outlives the call. + if unsafe { libc::fsync(directory.as_raw_fd()) } == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(not(unix))] +fn sync_directory(_directory: &Path) -> std::io::Result<()> { + Ok(()) +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -94,6 +160,25 @@ mod tests { assert_eq!(std::fs::read_to_string(&path).expect("reads"), "shh"); } + #[test] + fn write_private_durably_replaces_the_file_at_mode_0600() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("record"); + write_private_durably(&path, "first").expect("writes"); + write_private_durably(&path, "second").expect("replaces"); + + assert_eq!(std::fs::read_to_string(&path).expect("reads"), "second"); + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o600, "got {mode:o}"); + assert!( + !dir.path().join("record.tmp").exists(), + "the temporary file is renamed away" + ); + } + #[test] fn write_private_no_symlink_creates_a_mode_0600_file() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/crates/validator/src/slashing_protection/file.rs b/crates/validator/src/slashing_protection/file.rs index c75bf057d..6bf85fb44 100644 --- a/crates/validator/src/slashing_protection/file.rs +++ b/crates/validator/src/slashing_protection/file.rs @@ -27,8 +27,7 @@ use std::collections::HashMap; use std::fs::File; -use std::io::Write as _; -use std::path::{Path, PathBuf}; +use std::path::Path; use ethlambda_types::beacon::primitives::{BlsPubkey, Epoch, Root, Slot}; use ethlambda_types::beacon::serde_helpers::{hex_array, quoted_or_bare}; @@ -175,17 +174,41 @@ fn record_of(entry: &Entry) -> std::result::Result { }) } +/// Whether the record at `path` exists, failing closed. +/// +/// Only "no such file" is absence. Anything else that is not a regular file +/// (a symlink, perhaps to a volume that is not mounted, or a directory), and +/// any error reading its metadata, is an error: taking any of them for absence +/// would create a fresh record and orphan the real one. +pub(crate) fn exists(path: &Path) -> Result { + match std::fs::symlink_metadata(path) { + Ok(metadata) if metadata.file_type().is_file() => Ok(true), + Ok(_) => Err(Error::SlashingProtection { + path: path.display().to_string(), + reason: "is not a regular file (a symlink or a directory); refusing to read or \ + replace it" + .to_string(), + }), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(source) => Err(Error::Io { + path: path.display().to_string(), + source, + }), + } +} + /// Durably replace the record at `path` with `records`. /// -/// Entries are written in pubkey order so the file's bytes depend only on its -/// content, which keeps a diff of two files meaningful. -pub(crate) fn write( +/// Compact rather than pretty: the file is rewritten once per slot and per +/// block, and its size grows with the number of validators. Entries are +/// written in pubkey order so the bytes depend only on the content, which +/// keeps a diff of two files meaningful. +pub(crate) fn write<'a>( path: &Path, genesis_validators_root: Root, - records: &HashMap, + records: impl Iterator, ) -> Result<()> { let mut data: Vec = records - .iter() .map(|(pubkey, record)| Entry { pubkey: pubkey.0, signed_blocks: record @@ -216,76 +239,16 @@ pub(crate) fn write( }, data, }; - let contents = - serde_json::to_vec_pretty(&interchange).map_err(|err| Error::SlashingProtection { - path: path.display().to_string(), - reason: format!("could not encode the record: {err}"), - })?; - write_durably(path, &contents).map_err(|source| Error::Io { + let contents = serde_json::to_vec(&interchange).map_err(|err| Error::SlashingProtection { + path: path.display().to_string(), + reason: format!("could not encode the record: {err}"), + })?; + crate::secure_fs::write_private_durably(path, contents).map_err(|source| Error::Io { path: path.display().to_string(), source, }) } -/// Temporary file, flush, rename, flush the directory. -fn write_durably(path: &Path, contents: &[u8]) -> std::io::Result<()> { - let temporary = temporary_path(path); - let mut file = open_private(&temporary)?; - file.write_all(contents)?; - file.sync_all()?; - drop(file); - std::fs::rename(&temporary, path)?; - sync_directory(path.parent().unwrap_or(Path::new("."))) -} - -fn temporary_path(path: &Path) -> PathBuf { - let mut name = path.file_name().unwrap_or_default().to_os_string(); - name.push(".tmp"); - path.with_file_name(name) -} - -/// Created, or truncated if it exists, at mode `0600` on unix: the record says -/// which keys this client holds. -#[cfg(unix)] -fn open_private(path: &Path) -> std::io::Result { - use std::os::unix::fs::OpenOptionsExt as _; - std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(path) -} - -#[cfg(not(unix))] -fn open_private(path: &Path) -> std::io::Result { - std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .open(path) -} - -/// `fsync` on the directory, so the rename that replaced the record is itself -/// durable. Called through `libc` rather than `File::sync_all`, which on macOS -/// issues `F_FULLFSYNC`, a request some filesystems refuse for a directory. -#[cfg(unix)] -fn sync_directory(directory: &Path) -> std::io::Result<()> { - use std::os::fd::AsRawFd as _; - let directory = File::open(directory)?; - // SAFETY: the descriptor is owned by `directory`, which outlives the call. - if unsafe { libc::fsync(directory.as_raw_fd()) } == 0 { - Ok(()) - } else { - Err(std::io::Error::last_os_error()) - } -} - -#[cfg(not(unix))] -fn sync_directory(_directory: &Path) -> std::io::Result<()> { - Ok(()) -} - /// Take the exclusive lock in `directory`, held until the returned file is /// dropped. pub(crate) fn lock(directory: &Path) -> Result { diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 16ec86acc..3e03b53d6 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -29,9 +29,11 @@ //! //! - **A restart.** The record outlives the process; see [`file`] for how it //! is written and why reading it fails closed. -//! - **A second process.** [`SlashingProtection::open`] takes an exclusive -//! lock in the validators directory for the life of the process, so a second -//! validator client pointed at the same keys refuses to start. +//! - **A second process on the same directory.** [`SlashingProtection::open`] +//! takes an exclusive lock in the validators directory for the life of the +//! process, so a second validator client pointed at that directory refuses +//! to start. The lock is per directory, not per key: the same keystore +//! copied into a second directory gets a second record and a second lock. //! - **Another chain.** The record names the chain's `genesis_validators_root` //! and is refused for any other. //! - **A backward clock step or a duty schedule replaced mid-epoch**, the two @@ -40,7 +42,9 @@ //! It cannot see keys running in another client on another machine: no record //! kept here can. A key moved here from elsewhere must bring its history with //! it, which is why a key with no record is refused at startup unless the -//! operator says otherwise (see [`SlashingProtection::open`]). +//! operator says otherwise, and why a key registered with no history starts at +//! the current slot and epoch rather than as never having signed (see +//! [`Record::seeded`]). pub mod file; @@ -50,6 +54,7 @@ use std::sync::{Arc, Mutex}; use ethlambda_types::beacon::containers::shared::AttestationData; use ethlambda_types::beacon::primitives::{BlsPubkey, Epoch, Root, Slot}; +use ethlambda_types::beacon::signing::compute_epoch_at_slot; use tracing::warn; use crate::beacon_node::dto::encode_hex; @@ -58,11 +63,36 @@ use crate::error::{Error, Result}; /// The handle the duty services and the keymanager share. /// /// A `std::sync::Mutex`: every critical section is a lookup, an insert and, on -/// a change, a file write, with no `.await` inside. Callers on the async duty -/// path take it inside `spawn_blocking`, so the write does not hold up the -/// executor. +/// a change, a file write, with no `.await` inside. Async callers go through +/// [`blocking`], so neither the write nor waiting for another holder's write +/// holds up the executor. pub type SharedProtection = Arc>; +/// Run `operation` on the record on tokio's blocking pool, and wait for it. +/// +/// Every async caller goes through here, including ones that only read: the +/// mutex can be held by another caller's write, a full rewrite and two +/// flushes, and waiting for it on an executor thread would stall whatever else +/// that thread runs. The error is a reason, for the caller to log. +/// +/// 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 +/// message that is then never signed only costs that message. +pub async fn blocking( + protection: &SharedProtection, + operation: impl FnOnce(&mut SlashingProtection) -> Result + Send + 'static, +) -> std::result::Result { + let protection = protection.clone(); + tokio::task::spawn_blocking(move || { + let mut protection = protection + .lock() + .map_err(|_| "slashing protection is poisoned".to_string())?; + operation(&mut protection).map_err(|err| err.to_string()) + }) + .await + .map_err(|err| err.to_string())? +} + /// The highest source and target epoch signed for one validator. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct Signed { @@ -92,6 +122,29 @@ pub(crate) struct Record { } impl Record { + /// The record a key gets when it is registered with no history. + /// + /// "Never signed" when there is no current slot yet (before genesis, + /// nothing can have been signed). Otherwise the current slot and epoch, as + /// if a block and an attestation had just been signed there: the key may + /// have signed both elsewhere a moment ago, and taking those as signed + /// costs at most the rest of the epoch's duties. The source stays at zero, + /// since the source a beacon node offers is the justified epoch, which lags + /// the current one, and seeding it higher would refuse every attestation + /// until justification caught up. + fn seeded(now: Option) -> Record { + let Some(slot) = now else { + return Record::default(); + }; + Record { + block_slot: Some(slot), + attestation: Some(Signed { + source: 0, + target: compute_epoch_at_slot(slot), + }), + } + } + fn check_block(&self, slot: Slot) -> std::result::Result<(), Refusal> { match self.block_slot { Some(signed) if slot <= signed => Err(Refusal::SlotNotAdvanced { @@ -233,15 +286,17 @@ impl SlashingProtection { /// `genesis_validators_root`, and hold its lock until this is dropped. /// /// Every key in `keys` must already have a record. A key without one is - /// refused unless `init` is set, in which case it is registered as never - /// having signed: the operator's statement that this key has not signed - /// anything on this chain that the record should know about. A missing file - /// is created only on the same terms, or when there are no keys at all. + /// refused unless `init` is set, in which case it is registered as of + /// `now`, the current slot (see [`Record::seeded`]): the operator's + /// statement that this key has signed nothing the record should know about. + /// A missing file is created only on the same terms, or when there are no + /// keys at all. pub fn open( validators_dir: &Path, genesis_validators_root: Root, keys: &[BlsPubkey], init: bool, + now: Option, ) -> Result { let lock = file::lock(validators_dir)?; let path = validators_dir.join(file::FILE_NAME); @@ -250,7 +305,7 @@ impl SlashingProtection { reason, }; - let existed = path.exists(); + let existed = file::exists(&path)?; let mut records = if existed { let (stored_root, records) = file::read(&path)?; if stored_root != genesis_validators_root { @@ -275,9 +330,9 @@ impl SlashingProtection { let first = encode_hex(&missing[0].0); return Err(refused(format!( "{} validator key(s) have no slashing-protection record (first: {first}). \ - Import their history from the client that last ran them, or, only if they \ - have not signed anything for at least two epochs, start once with \ - --init-slashing-protection", + Start once with --init-slashing-protection only once they have signed \ + nothing anywhere for at least two epochs: stop the client that last ran \ + them and wait", missing.len() ))); } @@ -287,11 +342,11 @@ impl SlashingProtection { "Registering a validator with no slashing-protection history \ (--init-slashing-protection)" ); - records.insert(*key, Record::default()); + records.insert(*key, Record::seeded(now)); } if !existed || !missing.is_empty() { - file::write(&path, genesis_validators_root, &records)?; + file::write(&path, genesis_validators_root, records.iter())?; } Ok(Self { genesis_validators_root, @@ -398,16 +453,17 @@ impl SlashingProtection { Ok(verdicts) } - /// Register keys with no record as never having signed, and persist. + /// Register keys with no record as of `now`, the current slot (see + /// [`Record::seeded`]), and persist them in one write. /// /// For keys added at runtime through the keymanager API, which loads a key /// only after this returns. A key that already has a record keeps it: /// registering never lowers a watermark. Returns how many were new. - pub fn register(&mut self, pubkeys: &[BlsPubkey]) -> Result { + pub fn register(&mut self, pubkeys: &[BlsPubkey], now: Option) -> Result { let new: HashMap = pubkeys .iter() .filter(|pubkey| !self.records.contains_key(*pubkey)) - .map(|pubkey| (*pubkey, Record::default())) + .map(|pubkey| (*pubkey, Record::seeded(now))) .collect(); let count = new.len(); if count > 0 { @@ -420,16 +476,19 @@ impl SlashingProtection { /// memory only once the write has succeeded, so a failed write leaves both /// exactly as they were. fn commit(&mut self, changes: HashMap) -> Result<()> { - let Some(storage) = &self.storage else { - self.records.extend(changes); - return Ok(()); - }; - let mut next = self.records.clone(); - next.extend(changes); - let _timing = crate::metrics::time_slashing_protection_write(); - file::write(&storage.path, self.genesis_validators_root, &next) - .inspect_err(|_| crate::metrics::inc_slashing_protection_write_failures())?; - self.records = next; + if let Some(storage) = &self.storage { + // What the record will hold, written without copying it: every + // unchanged entry, then the changed ones. + let next = self + .records + .iter() + .filter(|(pubkey, _)| !changes.contains_key(*pubkey)) + .chain(changes.iter()); + let _timing = crate::metrics::time_slashing_protection_write(); + file::write(&storage.path, self.genesis_validators_root, next) + .inspect_err(|_| crate::metrics::inc_slashing_protection_write_failures())?; + } + self.records.extend(changes); Ok(()) } } @@ -499,9 +558,20 @@ mod tests { .expect("not poisoned") } + /// Opened before genesis, so a key `init` registers starts as never having + /// signed; the seeding is tested on its own below. fn open(dir: &Path, keys: &[u8], init: bool) -> Result { let keys: Vec = keys.iter().map(|byte| pubkey(*byte)).collect(); - SlashingProtection::open(dir, root(0x42), &keys, init) + SlashingProtection::open(dir, root(0x42), &keys, init, None) + } + + /// Whether the permission-based failure tests can run: root bypasses + /// directory permissions, so as root the write they expect to fail + /// succeeds. Docker CI commonly runs as root. + #[cfg(unix)] + fn running_as_root() -> bool { + // SAFETY: `geteuid` reads a process attribute and cannot fail. + unsafe { libc::geteuid() == 0 } } // The rule. Carried over from the in-memory guards this replaces. @@ -716,7 +786,7 @@ mod tests { let dir = tempfile::tempdir().expect("temp dir"); drop(open(dir.path(), &[1], true).expect("init")); - let err = SlashingProtection::open(dir.path(), root(0x99), &[pubkey(1)], true) + let err = SlashingProtection::open(dir.path(), root(0x99), &[pubkey(1)], true, None) .expect_err("another genesis_validators_root"); assert!(err.to_string().contains("another chain"), "{err}"); } @@ -778,6 +848,10 @@ mod tests { fn a_failed_write_refuses_and_changes_nothing() { use std::os::unix::fs::PermissionsExt as _; + if running_as_root() { + eprintln!("skipped: root bypasses directory permissions"); + return; + } let dir = tempfile::tempdir().expect("temp dir"); let mut protection = open(dir.path(), &[1], true).expect("init"); attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("attest"); @@ -827,7 +901,7 @@ mod tests { assert_eq!( protection - .register(&[pubkey(1), pubkey(2)]) + .register(&[pubkey(1), pubkey(2)], None) .expect("register"), 1 ); @@ -835,6 +909,55 @@ mod tests { assert!(propose(&mut protection, 2, 97).is_ok()); } + /// A key registered with no history after genesis is taken as having + /// signed a block and an attestation at the current slot and epoch: it may + /// have done so elsewhere a moment ago. + #[test] + fn a_key_registered_after_genesis_starts_at_the_current_slot() { + let dir = tempfile::tempdir().expect("temp dir"); + // Slot 100 is in epoch 3. + let mut protection = + SlashingProtection::open(dir.path(), root(0x42), &[pubkey(1)], true, Some(100)) + .expect("init"); + protection + .register(&[pubkey(2)], Some(100)) + .expect("register"); + + for key in [1, 2] { + assert!(propose(&mut protection, key, 100).is_err(), "key {key}"); + assert!(propose(&mut protection, key, 101).is_ok(), "key {key}"); + assert!( + attest(&mut protection, key, &data(96, 2, 3, 0xaa)).is_err(), + "key {key}: the current epoch's target may already be signed" + ); + assert!( + attest(&mut protection, key, &data(128, 2, 4, 0xaa)).is_ok(), + "key {key}: the next epoch, with a lagging source, is allowed" + ); + } + } + + /// A symlink in the record's place, perhaps to a volume that is not + /// mounted, must not be taken for "no record" and replaced. + #[cfg(unix)] + #[test] + fn a_record_that_is_not_a_regular_file_fails_closed() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join(file::FILE_NAME); + std::os::unix::fs::symlink(dir.path().join("unmounted/record.json"), &path) + .expect("symlinks"); + + let err = open(dir.path(), &[], true).expect_err("a dangling symlink"); + assert!(err.to_string().contains("not a regular file"), "{err}"); + assert!( + std::fs::symlink_metadata(&path) + .expect("still there") + .file_type() + .is_symlink(), + "the symlink must not have been replaced" + ); + } + #[test] fn an_unregistered_key_is_refused_on_a_persisted_record() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/docs/cli.md b/docs/cli.md index 4fe979c81..4cf0f65ca 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -269,7 +269,8 @@ conformant beacon node, this repository's `beacon` subcommand or another implementation's, so none of the [common flags](#common-flags) apply to it. > **Slashing protection** lives in `--validators-dir/slashing_protection.json`, -> locked to one process. A key with no record is refused at startup unless +> locked to one process, so `--validators-dir` must be writable: a read-only +> mount of the keys (a Kubernetes Secret, a `:ro` volume) fails at startup. A key with no record is refused at startup unless > `--init-slashing-protection` is passed. Importing another client's history is > not supported yet. See > [Spec Deviations](./spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). @@ -285,7 +286,7 @@ implementation's, so none of the [common flags](#common-flags) apply to it. | `--suggested-fee-recipient` | none | Execution address to receive block rewards, `0x`-prefixed, for every validator the keymanager API gives no address of its own. Optional, and startup warns when it is absent: without it the beacon node picks an address, and it will not be yours | | `--graffiti` | empty | Text for the graffiti field of proposed blocks, for every validator the keymanager API gives no graffiti of its own. At most 32 bytes as UTF-8, right-padded with zeros. Refused rather than truncated if longer. An ethlambda beacon node appends both clients' versions to it (see [`rpc.md`](rpc.md#validator-endpoints)) | | `--enable-keymanager` | off | Serve the keymanager API: keystores, plus each validator's own fee recipient, graffiti and gas limit. Off by default because it mutates key material | -| `--init-slashing-protection` | off | Register every loaded key with no slashing-protection record as never having signed, instead of refusing to start. Dangerous: only for keys that have signed nothing anywhere for at least two epochs. Never lowers an existing record; remove it once the keys are registered | +| `--init-slashing-protection` | off | Register every loaded key with no slashing-protection record, as if it had signed a block and an attestation at the current slot and epoch, instead of refusing to start. Dangerous: only for keys that have signed nothing anywhere for at least two epochs. Never lowers an existing record; remove it once the keys are registered | ### What it does today diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 41fd0d6ad..ad224113c 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -70,9 +70,13 @@ anything that does not advance past them. so nothing else is refused. Lighthouse and Prysm keep signing roots to allow the identical re-sign; Teku keeps the same watermarks as this client. - **Keys with no record:** refused at startup. `--init-slashing-protection` - registers them as never having signed, which is safe only for keys that have - signed nothing anywhere for at least two epochs. A key imported through the - keymanager API is registered the same way before it is loaded. + registers them as if each had signed a block and an attestation at the + current slot and epoch, and a key imported through the keymanager API with + no history is registered the same way before it is loaded. That closes the + double-vote window for a key that signed elsewhere a moment ago, at the cost + of the rest of the epoch's duties, but not a surround against older history, + so the flag is still only for keys that have signed nothing anywhere for at + least two epochs. - **Gloas messages:** a gloas proposer also signs an execution payload envelope, and a payload timeliness committee member signs a payload attestation. Neither is slashable, so neither is recorded. An envelope is @@ -90,7 +94,8 @@ anything that does not advance past them. deliberately all-zero, so a conformant importer rejects it outright rather than trusting an empty history. `POST` accepts the `slashing_protection` field and ignores it, with a server-side warning, so an imported key starts - as never having signed. + at the current slot and epoch, like one registered by + `--init-slashing-protection`. ## Builder bids: gossip and API only (no builder API) diff --git a/tooling/kurtosis-assertoor/network_params.yaml b/tooling/kurtosis-assertoor/network_params.yaml index 03aec974b..bf96f4721 100644 --- a/tooling/kurtosis-assertoor/network_params.yaml +++ b/tooling/kurtosis-assertoor/network_params.yaml @@ -25,11 +25,15 @@ participants: cl_type: lighthouse vc_type: ethlambda vc_image: ghcr.io/lambdaclass/ethlambda:unstable + # Genesis keys in a fresh directory: nothing has ever signed with them, so + # registering them without history is safe. + vc_extra_params: ["--init-slashing-protection"] - el_type: geth cl_type: ethlambda cl_image: ghcr.io/lambdaclass/ethlambda:unstable vc_type: ethlambda vc_image: ghcr.io/lambdaclass/ethlambda:unstable + vc_extra_params: ["--init-slashing-protection"] network_params: # ethlambda only supports the mainnet preset: 32-slot epochs of 12 s. preset: mainnet From 696e2a809ea44f51506c8a106eab0115f320d7f4 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 30 Oct 2026 16:45:53 -0300 Subject: [PATCH 3/3] Apply the bot reviews of the durable slashing protection. The durable write creates its temporary file fresh with create_new at mode 0600, removing whatever was at that path first, so a leftover of a crashed write cannot carry a wider mode into the renamed record and a symlink planted there is never written through; two tests pin both. The directory flush after the rename tries File::sync_all first, which is F_FULLFSYNC on macOS like the file's own flush, and falls back to a plain fsync only where a filesystem refuses that for a directory. The blocking helper returns the crate's own Error instead of a string, so a caller can tell a failed write from a record that cannot be reached at all: a lock poisoned by a panic, or the runtime shutting down, is the new SlashingProtectionUnusable, whose message says the client must be restarted. Attested gains a refused count so a slot where slashing protection withheld every signature, or could not record them, is distinguishable from a slot with nothing to publish. --- crates/validator/src/attestation.rs | 23 +++- crates/validator/src/error.rs | 13 +++ crates/validator/src/proposal.rs | 4 +- crates/validator/src/secure_fs.rs | 107 +++++++++++++----- .../validator/src/slashing_protection/mod.rs | 39 +++++-- 5 files changed, 146 insertions(+), 40 deletions(-) diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index 4a78f3abf..18a8f5cba 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -29,6 +29,13 @@ use crate::slashing_protection::SharedProtection; pub struct Attested { /// How many attestations the beacon node accepted. pub published: usize, + /// How many of the slot's duties slashing protection withheld: judged + /// unsafe to sign, or, when the record could not be written, the whole + /// batch, since nothing may be signed then. Kept apart from `published` + /// so that a slot with nothing published can be told from a slot with + /// nothing to publish, which an `Ok` with a count of zero alone cannot. + /// The cause is logged, and counted, where it is decided. + pub refused: usize, /// The data every attestation in this slot was signed over, or `None` when /// there was no duty to fetch it for. /// @@ -98,6 +105,7 @@ impl AttestationService { if duties.is_empty() { return Ok(Attested { published: 0, + refused: 0, data: None, }); } @@ -167,6 +175,7 @@ impl AttestationService { crate::metrics::inc_attestations_refused_by(candidates.len() as u64); return Ok(Attested { published: 0, + refused: candidates.len(), data: Some(data), }); } @@ -180,6 +189,7 @@ impl AttestationService { // EIP-2335 derivation outside it (see `http_api::keystores::import`), // so this is the duty path keeping its half of the same bargain. let mut attestations = Vec::with_capacity(candidates.len()); + let mut refused = 0; { let _timing = crate::metrics::time_signing(); let store = store.read().await; @@ -192,6 +202,7 @@ impl AttestationService { "Refusing to sign: slashing protection judged this attestation unsafe" ); crate::metrics::inc_attestations_refused(); + refused += 1; continue; } @@ -217,6 +228,7 @@ impl AttestationService { if attestations.is_empty() { return Ok(Attested { published: 0, + refused, data: Some(data), }); } @@ -238,6 +250,7 @@ impl AttestationService { crate::metrics::inc_attestations_published(published as u64); Ok(Attested { published, + refused, data: Some(data), }) } @@ -360,8 +373,9 @@ mod tests { .await .expect("the second call is not an error, it simply signs nothing"); - assert_eq!(first.published, 1); + assert_eq!((first.published, first.refused), (1, 0)); assert_eq!(second.published, 0, "the second attempt must be refused"); + assert_eq!(second.refused, 1, "and reported as refused, not as absent"); assert_eq!( node.submitted().len(), 1, @@ -442,7 +456,12 @@ mod tests { std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)) .expect("chmod back"); - assert_eq!(attested.expect("not an error").published, 0); + let attested = attested.expect("not an error"); + assert_eq!(attested.published, 0); + assert_eq!( + attested.refused, 1, + "a record that could not be written refuses the whole batch, visibly" + ); assert!( node.submitted().is_empty(), "nothing may reach the beacon node" diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index 796fb27cd..000580e06 100644 --- a/crates/validator/src/error.rs +++ b/crates/validator/src/error.rs @@ -114,6 +114,18 @@ pub enum Error { /// an operator looks. #[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. + /// + /// 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. + #[error("slashing protection is unusable: {reason}")] + SlashingProtectionUnusable { reason: String }, #[error("{path}: {source}")] Io { @@ -148,6 +160,7 @@ impl Error { | Self::UnknownValidator(_) | Self::Signing { .. } | Self::SlashingProtection { .. } + | Self::SlashingProtectionUnusable { .. } | Self::Io { .. } => false, } } diff --git a/crates/validator/src/proposal.rs b/crates/validator/src/proposal.rs index 946383257..3d24a4dd2 100644 --- a/crates/validator/src/proposal.rs +++ b/crates/validator/src/proposal.rs @@ -183,7 +183,7 @@ impl ProposalService { } Err(err) => { warn!(%slot, %err, "Slashing protection could not be consulted; refusing to propose"); - Some(err) + Some(err.to_string()) } }; if let Some(reason) = refusal { @@ -278,7 +278,7 @@ impl ProposalService { Ok(Err(refusal)) => Some(refusal.to_string()), Err(err) => { error!(%slot, %err, "Slashing protection could not record this block; refusing to sign"); - Some(err) + Some(err.to_string()) } }; if let Some(reason) = refusal { diff --git a/crates/validator/src/secure_fs.rs b/crates/validator/src/secure_fs.rs index f412c8171..7c8b3c560 100644 --- a/crates/validator/src/secure_fs.rs +++ b/crates/validator/src/secure_fs.rs @@ -14,12 +14,12 @@ use std::path::Path; /// Write `contents`, creating the file if it does not exist and truncating it /// if it does, at mode `0600` on unix. /// -/// For files this crate is always the sole, trusted writer of and is willing -/// to overwrite outright: the API token, and the validator definitions file's -/// `.tmp` sibling (`ValidatorDefinitions::save` renames it into place, which -/// preserves the mode set here). Neither is named from attacker-influenced -/// input, so there is no symlink concern for these; see -/// [`write_private_no_symlink`] for the ones that are. +/// For a file this crate is the sole, trusted writer of and is willing to +/// overwrite outright: the API token. It is not named from +/// attacker-influenced input, so there is no symlink concern; see +/// [`write_private_no_symlink`] for the files that are, and +/// [`write_private_durably`] for the ones whose loss would change what a +/// validator signs. #[cfg(unix)] pub(crate) fn write_private(path: &Path, contents: impl AsRef<[u8]>) -> std::io::Result<()> { use std::io::Write as _; @@ -79,11 +79,17 @@ pub(crate) fn write_private_no_symlink( /// leaves either the old file or the new one, never a mixture, and so that a /// return of `Ok` means the new one is on disk. /// -/// Written to a sibling `.tmp` at mode `0600` (through [`write_private`]'s -/// rules), flushed, renamed over `path`, and then the directory is flushed so -/// the rename itself survives. For files whose loss or truncation would change -/// what a validator signs: the slashing-protection record and the validator -/// definitions file. +/// Written to a sibling `.tmp` created fresh at mode `0600`, flushed, renamed +/// over `path`, and then the directory is flushed so the rename itself +/// survives. For files whose loss or truncation would change what a validator +/// signs: the slashing-protection record and the validator definitions file. +/// +/// The temporary is always a new file: whatever is at its path first, a +/// leftover of a write that crashed partway, or something planted there, is +/// removed, never reopened. A mode given at creation applies only at +/// creation, so reopening a leftover would carry its mode, whatever that was, +/// into the renamed record; and `O_EXCL` does not follow a symlink, so the +/// write cannot be redirected through one either. pub(crate) fn write_private_durably( path: &Path, contents: impl AsRef<[u8]>, @@ -93,7 +99,12 @@ pub(crate) fn write_private_durably( name.push(".tmp"); let temporary = path.with_file_name(name); - let mut file = open_private(&temporary)?; + match std::fs::remove_file(&temporary) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err), + } + let mut file = create_private(&temporary)?; file.write_all(contents.as_ref())?; file.sync_all()?; drop(file); @@ -102,38 +113,43 @@ pub(crate) fn write_private_durably( } #[cfg(unix)] -fn open_private(path: &Path) -> std::io::Result { +fn create_private(path: &Path) -> std::io::Result { use std::os::unix::fs::OpenOptionsExt as _; std::fs::OpenOptions::new() .write(true) - .create(true) - .truncate(true) + .create_new(true) .mode(0o600) .open(path) } #[cfg(not(unix))] -fn open_private(path: &Path) -> std::io::Result { +fn create_private(path: &Path) -> std::io::Result { std::fs::OpenOptions::new() .write(true) - .create(true) - .truncate(true) + .create_new(true) .open(path) } -/// `fsync` on the directory, so a rename into it is itself durable. Called -/// through `libc` rather than `File::sync_all`, which on macOS issues -/// `F_FULLFSYNC`, a request some filesystems refuse for a directory. +/// Flush the directory, so a rename into it is itself durable. +/// +/// `File::sync_all` first: `fsync` everywhere but macOS, where it is +/// `F_FULLFSYNC`, the only request there that reaches the disk rather than +/// the drive's cache, and the one the file's own flush above already relies +/// on. Some filesystems refuse that request for a directory, so a plain +/// `fsync` is tried next: it is the best those offer, and the original error +/// is kept if even that fails. #[cfg(unix)] fn sync_directory(directory: &Path) -> std::io::Result<()> { use std::os::fd::AsRawFd as _; let directory = std::fs::File::open(directory)?; - // SAFETY: the descriptor is owned by `directory`, which outlives the call. - if unsafe { libc::fsync(directory.as_raw_fd()) } == 0 { - Ok(()) - } else { - Err(std::io::Error::last_os_error()) - } + directory.sync_all().or_else(|full_sync| { + // SAFETY: the descriptor is owned by `directory`, which outlives the call. + if unsafe { libc::fsync(directory.as_raw_fd()) } == 0 { + Ok(()) + } else { + Err(full_sync) + } + }) } #[cfg(not(unix))] @@ -179,6 +195,43 @@ mod tests { ); } + #[test] + fn write_private_durably_never_inherits_a_leftover_temporary_file_mode() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("record"); + // A write that crashed partway, under a permissive umask. + let leftover = dir.path().join("record.tmp"); + std::fs::write(&leftover, "half").expect("writes leftover"); + std::fs::set_permissions(&leftover, std::fs::Permissions::from_mode(0o644)).expect("chmod"); + + write_private_durably(&path, "whole").expect("writes"); + + assert_eq!(std::fs::read_to_string(&path).expect("reads"), "whole"); + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o600, "got {mode:o}"); + } + + #[test] + fn write_private_durably_does_not_write_through_a_symlink_at_the_temporary_path() { + let dir = tempfile::tempdir().expect("temp dir"); + let target = dir.path().join("target"); + std::fs::write(&target, "untouched").expect("writes target"); + let path = dir.path().join("record"); + std::os::unix::fs::symlink(&target, dir.path().join("record.tmp")).expect("symlinks"); + + write_private_durably(&path, "whole").expect("writes"); + + assert_eq!(std::fs::read_to_string(&path).expect("reads"), "whole"); + assert_eq!( + std::fs::read_to_string(&target).expect("reads"), + "untouched", + "the symlink target must be untouched" + ); + } + #[test] fn write_private_no_symlink_creates_a_mode_0600_file() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 3e03b53d6..b726eb8a6 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -73,7 +73,16 @@ pub type SharedProtection = Arc>; /// Every async caller goes through here, including ones that only read: the /// mutex can be held by another caller's write, a full rewrite and two /// flushes, and waiting for it on an executor thread would stall whatever else -/// that thread runs. The error is a reason, for the caller to log. +/// that thread runs. +/// +/// The operation's own error comes back as it is, so a caller can still tell +/// a write that failed (`Error::Io`, a disk to look at) from a record that +/// 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`. /// /// 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 @@ -81,16 +90,27 @@ pub type SharedProtection = Arc>; pub async fn blocking( protection: &SharedProtection, operation: impl FnOnce(&mut SlashingProtection) -> Result + Send + 'static, -) -> std::result::Result { +) -> 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 || { - let mut protection = protection - .lock() - .map_err(|_| "slashing protection is poisoned".to_string())?; - operation(&mut protection).map_err(|err| err.to_string()) + let mut protection = protection.lock().map_err(|_| poisoned())?; + operation(&mut protection) }) .await - .map_err(|err| err.to_string())? + .map_err(|err| { + if err.is_panic() { + poisoned() + } else { + Error::SlashingProtectionUnusable { + reason: format!("the record could not be consulted: {err}"), + } + } + })? } /// The highest source and target epoch signed for one validator. @@ -425,8 +445,9 @@ impl SlashingProtection { /// Check one slot's attestations, all over `data`, and durably record every /// one that may be signed, in a single write. /// - /// The verdicts are in `pubkeys` order. A key listed twice is judged - /// against its first verdict, so it is refused the second time. The outer + /// The verdicts are in `pubkeys` order. A key listed twice is judged, at + /// its later position, against the verdict of its earlier one, so it is + /// refused the second time: same target as the one just staged. The outer /// `Err` is a record that could not be written, in which case nothing /// changed and none of them may be signed. pub fn check_and_record_attestations(