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 ff41a6a28..99bfcbd1f 100644 --- a/README.md +++ b/README.md @@ -341,13 +341,16 @@ 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; +> even then it rules out a double vote, not a surround of an older vote, which +> only the key's own history can. +> 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..968244963 100644 --- a/bin/ethlambda/src/validator.rs +++ b/bin/ethlambda/src/validator.rs @@ -78,6 +78,19 @@ 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, + /// 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 is registered as never + /// having signed, so nothing here stops it from signing again what it + /// signed elsewhere a moment ago; and two idle epochs rule out a double + /// vote, not a surround of an older vote, which only the key's own history + /// can. 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 +167,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 +188,7 @@ mod tests { graffiti: graffiti.to_string(), suggested_fee_recipient: None, enable_keymanager: false, + init_slashing_protection: false, } } diff --git a/crates/net/rpc/Cargo.toml b/crates/net/rpc/Cargo.toml index d07713af4..547639dbf 100644 --- a/crates/net/rpc/Cargo.toml +++ b/crates/net/rpc/Cargo.toml @@ -33,7 +33,7 @@ jemalloc_pprof.workspace = true futures-util = "0.3" [dev-dependencies] -ethlambda-validator.workspace = true +ethlambda-validator = { workspace = true, features = ["test-utils"] } spawned-concurrency.workspace = true ethlambda-state-transition = { workspace = true, features = ["test-utils"] } ethlambda-types.workspace = true 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..7c498eed1 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 @@ -72,3 +73,7 @@ tokio = { workspace = true, features = ["macros", "rt-multi-thread", "test-util" tower = { version = "0.5", features = ["util"] } http-body-util = "0.1" tempfile = "3" + +[features] +# Exposes `SlashingProtection::in_memory` to other crates' tests. +test-utils = [] diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index a5984e590..5372b8470 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -1,14 +1,14 @@ //! Producing and publishing this slot's attestations. +use std::collections::HashSet; 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 +16,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. /// @@ -29,8 +30,16 @@ use crate::signing::SigningContext; pub struct Attested { /// How many attestations the beacon node accepted. pub published: usize, - /// The data every attestation in this slot was signed over, or `None` when - /// there was no duty to fetch it for. + /// 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 slot's attestation data, which any attestation of it is signed + /// over, or `None` when there was no duty to fetch it for. Not a record of + /// what was signed: `published` and `refused` are. /// /// An `Option` rather than a zeroed default, because a caller handed a /// default would ask for the aggregate of an attestation nobody made. It is @@ -43,25 +52,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 +85,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 @@ -102,6 +107,7 @@ impl AttestationService { if duties.is_empty() { return Ok(Attested { published: 0, + refused: 0, data: None, }); } @@ -142,6 +148,77 @@ 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(); + + // Only keys this client holds. A duty can name one the keymanager + // deleted since the schedule was fetched; recording it would burn its + // watermark for nothing, or, on a persisted record, refuse it as + // unregistered and count a refusal that was never going to be a + // signature. A snapshot, so the read guard is gone before the write; + // a key deleted between it and the signing below fails there as + // unknown, and its watermark stays advanced, which only costs a key + // on its way out. + let held: HashSet = store.read().await.pubkeys().into_iter().collect(); + let candidates: Vec<(&AttesterDutyDto, BlsPubkey)> = candidates + .into_iter() + .filter(|(duty, pubkey)| { + let held = held.contains(pubkey); + if !held { + tracing::debug!( + %slot, + validator = duty.validator_index, + "Duty names a key this client no longer holds; skipped" + ); + } + held + }) + .collect(); + + if candidates.is_empty() { + // Nothing to record or sign; the data still names the aggregate. + return Ok(Attested { + published: 0, + refused: 0, + data: Some(data), + }); + } + + // 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 verdicts = crate::slashing_protection::blocking(&self.protection, move |protection| { + protection.check_and_record_attestations(&pubkeys, &data) + }) + .await; + let verdicts = match verdicts { + Ok(verdicts) => verdicts, + Err(err) => { + // Nothing may be signed. Nothing was recorded either, except + // after a rename whose directory flush failed, where the target + // is on disk and in memory and a retry is refused (see + // `SlashingProtection::check_and_record_attestations`). + error!(%slot, %err, "Slashing protection could not record this slot's attestations; refusing to sign any"); + crate::metrics::inc_attestations_refused_by(candidates.len() as u64); + return Ok(Attested { + published: 0, + refused: candidates.len(), + 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 +226,22 @@ 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 mut refused = 0; { 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(); + refused += 1; + continue; } // One validator failing must not cost the others their attestation. @@ -217,6 +266,7 @@ impl AttestationService { if attestations.is_empty() { return Ok(Attested { published: 0, + refused, data: Some(data), }); } @@ -238,6 +288,7 @@ impl AttestationService { crate::metrics::inc_attestations_published(published as u64); Ok(Attested { published, + refused, data: Some(data), }) } @@ -250,6 +301,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 +320,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 +363,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), @@ -333,7 +386,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 @@ -346,7 +399,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); @@ -357,8 +411,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, @@ -366,6 +421,90 @@ 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 _; + + // 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); + 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"); + + 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" + ); + } + /// 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 +516,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 +527,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 +560,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 +587,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 +609,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 +637,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 +654,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 +669,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 +682,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 +706,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 +750,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 +782,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 +846,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 +906,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/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 c689d2a1d..49c1c54dd 100644 --- a/crates/validator/src/beacon_node/mod.rs +++ b/crates/validator/src/beacon_node/mod.rs @@ -62,6 +62,18 @@ pub fn validate_attestation_data(slot: Slot, data: &AttestationData) -> Result<( data.target.epoch ))); } + // Signing a source at or past its target would record a source the + // chain's justified checkpoint cannot have reached, and the record keeps + // it: every honest attestation afterwards would be refused as a source + // regression until justification caught up, or forever during + // non-finality. + if crate::slashing_protection::is_impossible_source(data.source.epoch, data.target.epoch) { + return Err(crate::error::Error::InconsistentResponse(format!( + "attestation data for slot {slot} carries source epoch {} at or past its target \ + epoch {}", + data.source.epoch, data.target.epoch + ))); + } Ok(()) } @@ -156,11 +168,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!( @@ -322,9 +334,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. @@ -571,3 +584,46 @@ pub fn validate_sync_contribution( } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use ethlambda_types::beacon::containers::shared::Checkpoint; + + fn data(slot: Slot, source: Epoch, target: Epoch) -> AttestationData { + AttestationData { + slot, + index: 0, + beacon_block_root: Root::ZERO, + source: Checkpoint { + epoch: source, + root: Root::ZERO, + }, + target: Checkpoint { + epoch: target, + root: Root::ZERO, + }, + } + } + + /// Slot 100 is in epoch 3 on the mainnet preset. + #[test] + fn attestation_data_with_a_source_at_or_past_its_target_is_refused() { + assert!(validate_attestation_data(100, &data(100, 2, 3)).is_ok()); + assert!(validate_attestation_data(100, &data(100, 0, 3)).is_ok()); + let err = validate_attestation_data(100, &data(100, 3, 3)).expect_err("source == target"); + assert!(err.to_string().contains("source epoch 3"), "{err}"); + assert!(validate_attestation_data(100, &data(100, 4, 3)).is_err()); + } + + /// At genesis both checkpoints are epoch 0, and that is the one valid + /// shape where the source does not precede the target. + #[test] + fn genesis_attestation_data_is_accepted() { + assert!(validate_attestation_data(0, &data(0, 0, 0)).is_ok()); + assert!( + validate_attestation_data(0, &data(0, 1, 0)).is_err(), + "a source past a genesis target is not the genesis shape" + ); + } +} diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index 55b82a0ff..ae65aee92 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}")] @@ -104,12 +106,46 @@ 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 }, + /// 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 { path: String, #[source] source: std::io::Error, }, + + /// A durable write that replaced its file but could not flush the + /// directory, so the new file is what a reader sees now and may not be + /// what survives a power loss (see `secure_fs::DurableWriteError`). + #[error( + "{path}: written, but the directory could not be flushed, so the write may not survive a \ + power loss: {source}" + )] + Unflushed { + path: String, + #[source] + source: std::io::Error, + }, } impl Error { @@ -128,15 +164,18 @@ 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(_) | Self::KeystoreBadPassword | Self::UnknownValidator(_) | Self::Signing { .. } - | Self::Io { .. } => false, + | Self::SlashingProtection { .. } + | Self::SlashingProtectionUnusable { .. } + | Self::Io { .. } + | Self::Unflushed { .. } => false, } } } diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 2232679b3..26f35aebb 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -2,23 +2,25 @@ //! //! # 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 -//! clients must not rely on this client to carry that history. +//! EIP-3076 interchange is returned; `POST` refuses a request that carries the +//! optional `slashing_protection` field, importing none of its keys, since a +//! caller that hands over history must not be told it was taken. A caller +//! migrating keys between clients must not rely on this client to carry that +//! history. //! //! 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. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use axum::{Json, extract::State}; @@ -72,7 +74,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, } @@ -144,8 +146,23 @@ pub async fn import( State(context): State, Json(request): Json, ) -> Json { + // Refused rather than ignored: the caller handed over history, and a + // `200` with `imported` would tell it the history was taken, which is the + // standard way to move an active key and the surest way to double-sign. if request.slashing_protection.is_some() { - warn!("Ignoring slashing_protection on import: this client keeps no signing history"); + warn!( + "Refusing an import that carries slashing_protection; importing history is not supported yet" + ); + let message = "not imported: this client does not import EIP-3076 history yet, so a \ + request carrying slashing_protection imports nothing; import the keys \ + without it only if they have signed nothing anywhere for at least two \ + epochs"; + let data = request + .keystores + .iter() + .map(|_| KeyStatus::error(message)) + .collect(); + return Json(StatusResponse { data }); } if request.keystores.len() > MAX_KEYSTORES_PER_IMPORT { @@ -194,6 +211,21 @@ pub async fn import( } }; + // The entries the file held before this request touches it, by key. A + // failed registration below removes what the request added and puts back + // what it replaced: a disabled entry, which was never loaded and so may + // have no record, must not be left enabled on disk, or the next start + // refuses the whole client for it. + let before: std::collections::HashMap = definitions + .0 + .iter() + .filter_map(|definition| { + parse_pubkey(&definition.voting_public_key) + .ok() + .map(|pubkey| (pubkey, definition.clone())) + }) + .collect(); + // Decrypt (deliberately slow: EIP-2335 key derivation) and persist every // key to disk before ever touching the store's write lock. The duty // loop's `attest` only ever holds a read lock across a short, synchronous @@ -209,32 +241,244 @@ pub async fn import( prepared.push(prepare_import(&context, &mut definitions, json, password)); } - // 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. - drop(_definitions_guard); + // 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. 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 registration = if persisted.is_empty() { + Ok(0) + } else { + let registering = persisted.clone(); + crate::slashing_protection::blocking(&context.protection, move |protection| { + protection.register(®istering) + }) + .await + }; + // The records are on disk and in memory (see `SlashingProtection::commit`), + // only their survival of a power loss is in doubt, and a registration lost + // that way is refused at the next start: closed. So the keys are loaded, + // rather than rolled back to leave records behind for keys never imported. + let registration = match registration { + Err(Error::Unflushed { path, source }) => { + warn!( + %path, + %source, + "Slashing-protection records written, but the directory could not be flushed; \ + the imported keys are registered" + ); + Ok(0) + } + other => other, + }; + if let Ok(registered) = registration + && registered > 0 + { + warn!( + registered, + "Imported validator keys with no slashing-protection history; each starts as never \ + having signed" + ); + } + + // A key on disk with no record is refused at the next start, and + // `SlashingProtection::open` then refuses the whole client, not the one + // key. A crash between persisting and registering leaves exactly that, + // which is the closed side to fail on. Here the write is known to have + // failed, so the keys this request added are taken off disk again, the + // entries it replaced are put back as they were, and the failure costs + // this request only. The keystore and password files go back to what was + // at their paths before, as a pair, or are removed where there was + // nothing. + let rolled_back = match ®istration { + Ok(_) => Ok(()), + Err(_) => { + let mut added = Vec::new(); + let mut replaced = Vec::new(); + for pubkey in &persisted { + match before.get(pubkey) { + Some(previous) => replaced.push((*pubkey, previous.clone())), + None => added.push(*pubkey), + } + } + let files: Vec<(BlsPubkey, &Overwritten)> = prepared + .iter() + .filter_map(|result| { + result + .as_ref() + .ok() + .map(|(pubkey, _, overwritten)| (*pubkey, overwritten)) + }) + .collect(); + undo_import(&context, &mut definitions, &added, &replaced, &files) + } + }; + if let Err(err) = &rolled_back { + tracing::error!( + %err, + "Could not undo a failed import; the next start refuses its keys until they have a \ + slashing-protection record" + ); + } + // The definitions guard stays held across the store lock, in the order + // `delete` takes the two. Released before it, a delete of the same key + // could run in between: it would find the key on disk but not in memory, + // remove it and answer `deleted`, and the insert below would then load a + // key the operator was told is gone. 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), + (Ok((pubkey, _, _)), Err(err)) => { + if before.contains_key(&pubkey) { + // Its keystore was rewritten; its entry is back as it was + // and nothing about it changed in memory, so it stays + // whatever it was, loaded since startup in the ordinary + // case. + let entry = if rolled_back.is_ok() { + "its definitions entry is back as it was" + } else { + "its definitions entry could not be put back" + }; + KeyStatus::error(format!( + "keystore rewritten, but the slashing-protection record could not be \ + written: {err}; {entry}" + )) + } else if rolled_back.is_err() { + // Still on disk, not loaded now, and refused at the next + // start until it has a record. + KeyStatus::error(format!( + "saved, but not loaded: its slashing-protection record could not be \ + written: {err}" + )) + } else { + KeyStatus::error(format!( + "not imported: its slashing-protection record could not be written: {err}" + )) + } + } + (Err(status), _) => status, }) .collect(); drop(store); + drop(_definitions_guard); Json(StatusResponse { data }) } +/// Undo what a failed import did: the definitions entries it added are +/// removed and the ones it replaced put back, in one save, then every +/// keystore and password file it wrote goes back to what was at its path +/// before, as a pair, or is removed where there was nothing. The file goes +/// first, since it is what the next start loads from, so a crash in between +/// leaves orphan files and never a key that would be loaded without a +/// record. A file that is already gone is not an error. +fn undo_import( + context: &KeymanagerContext, + definitions: &mut ValidatorDefinitions, + added: &[BlsPubkey], + replaced: &[(BlsPubkey, ValidatorDefinition)], + files: &[(BlsPubkey, &Overwritten)], +) -> crate::Result<()> { + if added.is_empty() && replaced.is_empty() { + return Ok(()); + } + definitions.0.retain(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| !added.contains(&parsed)) + .unwrap_or(true) + }); + for definition in &mut definitions.0 { + if let Some((_, previous)) = replaced.iter().find(|(pubkey, _)| { + parse_pubkey(&definition.voting_public_key).is_ok_and(|parsed| parsed == *pubkey) + }) { + *definition = previous.clone(); + } + } + save_definitions(context, definitions)?; + // Newest first: a key listed twice in one request snapshotted, the second + // time, what the first time wrote, so the earliest snapshot must be the + // one that lands last. + for (pubkey, overwritten) in files.iter().rev() { + let [keystore, password] = written_paths(context, pubkey)?; + put_back(&keystore, overwritten.keystore.as_deref())?; + put_back( + &password, + overwritten.password.as_deref().map(|bytes| &bytes[..]), + )?; + } + Ok(()) +} + +/// Put `path` back to `previous`: its former bytes, or nothing. +fn put_back(path: &Path, previous: Option<&[u8]>) -> crate::Result<()> { + match previous { + Some(bytes) => { + secure_fs::write_private_no_symlink(path, bytes).map_err(|source| Error::Io { + path: path.display().to_string(), + source, + }) + } + None => remove_if_present(path), + } +} + +/// Save the definitions file, keeping `definitions_unflushed` true from an +/// after-rename flush failure until a later save flushes the directory, which +/// makes every earlier rename in it durable too. +fn save_definitions( + context: &KeymanagerContext, + definitions: &ValidatorDefinitions, +) -> crate::Result<()> { + let saved = definitions.save(&context.validators_dir); + match &saved { + Ok(()) => context + .definitions_unflushed + .store(false, std::sync::atomic::Ordering::Release), + Err(Error::Unflushed { .. }) => context + .definitions_unflushed + .store(true, std::sync::atomic::Ordering::Release), + Err(_) => {} + } + saved +} + +/// The keystore and password paths `persist_import` writes `pubkey` to. +fn written_paths(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Result<[PathBuf; 2]> { + let base = encode_hex(&pubkey.0); + Ok([ + absolute(context.validators_dir.join(format!("{base}.json")))?, + absolute(context.secrets_dir.join(&base))?, + ]) +} + +/// Remove `path`; one that is already gone is not an error. +fn remove_if_present(path: &Path) -> crate::Result<()> { + match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(source) => Err(Error::Io { + path: path.display().to_string(), + source, + }), + } +} + /// 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]>, Overwritten), KeyStatus> { let Some(password) = password else { return Err(KeyStatus::error("no password supplied for this keystore")); }; @@ -248,9 +492,39 @@ fn prepare_import( // crash right after "imported" is reported silently loses it, with // nothing about a live, signing validator hinting that it is about to // vanish on the next restart. - persist_import(context, definitions, &pubkey, json, password).map_err(KeyStatus::error)?; + let overwritten = + persist_import(context, definitions, &pubkey, json, password).map_err(KeyStatus::error)?; + + Ok((pubkey, secret, overwritten)) +} - Ok(secret) +/// What `persist_import` found at the two paths it writes, so an undone +/// import can put it back: the bytes of a regular file that was there, or +/// `None` for nothing. Restored as a pair, so a keystore never ends up next +/// to a password it was not encrypted under. +struct Overwritten { + keystore: Option>, + password: Option>>, +} + +/// The bytes of the regular file at `path`, or `None` when there is none. A +/// symlink or a directory reads as `None` too: the write that follows refuses +/// those, so nothing of theirs is overwritten. +fn regular_file_bytes(path: &Path) -> crate::Result>> { + match std::fs::symlink_metadata(path) { + Ok(metadata) if metadata.is_file() => { + std::fs::read(path).map(Some).map_err(|source| Error::Io { + path: path.display().to_string(), + source, + }) + } + Ok(_) => Ok(None), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(source) => Err(Error::Io { + path: path.display().to_string(), + source, + }), + } } /// Add an already-decrypted, already-persisted key to the live store. The @@ -277,7 +551,7 @@ fn persist_import( pubkey: &BlsPubkey, keystore_json: &str, password: &str, -) -> crate::Result<()> { +) -> crate::Result { // Named after the pubkey, the convention staking tools use, so an // operator can tell which file backs which validator without opening it. // That predictability is exactly why the writes below refuse to follow a @@ -298,22 +572,46 @@ fn persist_import( // file, so re-importing the same key (rotating its password, or simply // retrying) behaves exactly as a plain `std::fs::write` would; it only // refuses to follow a symlink planted at the path ahead of time. - secure_fs::write_private_no_symlink(&keystore_path, keystore_json).map_err(|source| { - Error::Io { + // Taken before the writes, to be put back if this import is undone. + let overwritten = Overwritten { + keystore: regular_file_bytes(&keystore_path)?, + password: regular_file_bytes(&password_path)?.map(Zeroizing::new), + }; + // Both files, or neither: a failure after the first write puts both back + // as they were, so a re-import that fails on its second file does not + // leave the previous keystore destroyed under the previous entry. + let restore = |overwritten: &Overwritten| -> crate::Result<()> { + put_back(&keystore_path, overwritten.keystore.as_deref())?; + put_back( + &password_path, + overwritten.password.as_deref().map(|bytes| &bytes[..]), + ) + }; + let written = secure_fs::write_private_no_symlink(&keystore_path, keystore_json) + .map_err(|source| Error::Io { path: keystore_path.display().to_string(), source, - } - })?; - secure_fs::write_private_no_symlink(&password_path, password).map_err(|source| Error::Io { - path: password_path.display().to_string(), - source, - })?; + }) + .and_then(|()| { + secure_fs::write_private_no_symlink(&password_path, password).map_err(|source| { + Error::Io { + path: password_path.display().to_string(), + source, + } + }) + }); + if let Err(err) = written { + restore(&overwritten).inspect_err( + |undo| tracing::error!(%undo, "Could not put a half-written keystore back"), + )?; + return Err(err); + } let definition = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path, - voting_keystore_password_path: password_path, + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: password_path.clone(), }; // How to put `definitions` back if the save below fails. // @@ -321,9 +619,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 { @@ -351,19 +649,36 @@ fn persist_import( } }; - if let Err(err) = definitions.save(&context.validators_dir) { - // `Appended` pops rather than removing by index because this function - // pushes and saves with nothing in between, so the entry it added is - // still the last one. - match undo { - Undo::Appended => { - definitions.0.pop(); + match save_definitions(context, definitions) { + Ok(()) => Ok(overwritten), + // The entry is on disk and every reader sees it; only its survival + // of a power loss is in doubt, and an entry lost that way only + // removes a key. Counted as persisted, and kept in memory, so what + // this request holds matches the file. + Err(Error::Unflushed { path, source }) => { + warn!( + %path, + %source, + "Validator definitions written, but the directory could not be flushed" + ); + Ok(overwritten) + } + Err(err) => { + // `Appended` pops rather than removing by index because this + // function pushes and saves with nothing in between, so the + // entry it added is still the last one. The files go back too. + match undo { + Undo::Appended => { + definitions.0.pop(); + } + Undo::Replaced { index, previous } => definitions.0[index] = previous, } - Undo::Replaced { index, previous } => definitions.0[index] = previous, + restore(&overwritten).inspect_err(|undo| { + tracing::error!(%undo, "Could not put a keystore whose entry failed to save back") + })?; + Err(err) } - return Err(err); } - Ok(()) } fn absolute(path: PathBuf) -> crate::Result { @@ -435,6 +750,25 @@ fn delete_one( context.settings.forget(pubkey); if !on_disk && !in_memory { + // A removal whose directory flush failed is on disk but may not + // survive a power loss, and a retry lands here. Answering `not_found` + // then would tell the operator the key is gone for good; flush first, + // by saving the file again, and keep the error until that works. + if context + .definitions_unflushed + .load(std::sync::atomic::Ordering::Acquire) + { + let definitions = match ValidatorDefinitions::open(&context.validators_dir) { + Ok(definitions) => definitions, + Err(err) => return KeyStatus::error(err), + }; + if let Err(err) = save_definitions(context, &definitions) { + return KeyStatus::error(format!( + "an earlier removal is on disk but the directory could not be flushed: \ + {err}; the key may come back after a power loss" + )); + } + } return KeyStatus { status: "not_found".to_string(), message: None, @@ -453,8 +787,19 @@ fn delete_one( // a `not_found` that reads as "already gone": acting on that, by importing // the key elsewhere, is what turns a failed delete into two hosts signing // for one validator. - if let Err(err) = persist_delete(context, pubkey) { - return KeyStatus::error(err); + match persist_delete(context, pubkey) { + Ok(()) => {} + // The removal is on disk but may not survive a power loss, and the + // keystore stays, so the key could come back enabled, with its + // record, while the operator runs it elsewhere. An error, so no + // caller takes the key for gone; this process does not sign with it. + Err(Error::Unflushed { path, source }) => { + return KeyStatus::error(format!( + "removed from {path}, but the directory could not be flushed: {source}; the key \ + may come back after a power loss" + )); + } + Err(err) => return KeyStatus::error(err), } info!(pubkey = %encode_hex(&pubkey.0), "Deleted validator key"); @@ -497,7 +842,7 @@ fn persist_delete(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Res .map(|parsed| parsed != *pubkey) .unwrap_or(true) }); - definitions.save(&context.validators_dir) + save_definitions(context, &definitions) } /// A well-formed EIP-3076 interchange holding no history. @@ -564,6 +909,8 @@ mod tests { validators_dir: dir.to_path_buf(), secrets_dir: dir.to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + protection: crate::slashing_protection::SlashingProtection::in_memory(), } } @@ -900,6 +1247,401 @@ 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"); + } + + /// A key whose registration fails is taken off disk again, so the failed + /// write costs this request only and the next start loads nothing it + /// would refuse. + #[tokio::test] + async fn a_failed_registration_rolls_the_import_back() { + 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"); + // A directory in the record's place makes the registration's rename + // fail, whoever runs the test. + let record = protection.path().expect("persisted").to_path_buf(); + std::fs::remove_file(&record).expect("removes the record"); + std::fs::create_dir(&record).expect("a directory in its place"); + let mut context = + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + let store = context.store.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!( + body["data"][0]["message"] + .as_str() + .unwrap_or_default() + .starts_with("not imported"), + "got {body}" + ); + assert!(store.read().await.pubkeys().is_empty(), "not loaded"); + assert!( + ValidatorStore::load(dir.path()) + .expect("loads") + .pubkeys() + .is_empty(), + "the next start finds no key to refuse" + ); + let leftovers: Vec = std::fs::read_dir(dir.path()) + .expect("lists") + .filter_map(|entry| entry.ok()) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| { + !name.starts_with("slashing_protection") + && name != crate::keys::definitions::DEFINITIONS_FILE + }) + .collect(); + assert!( + leftovers.is_empty(), + "keystore and password removed: {leftovers:?}" + ); + } + + /// A failed registration puts back the definitions entry the request + /// replaced: a disabled entry, which was never loaded and has no record, + /// stays disabled rather than enabled on disk, which would refuse the + /// whole client at the next start. + #[tokio::test] + async fn a_failed_registration_restores_a_replaced_definitions_entry() { + 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 record = protection.path().expect("persisted").to_path_buf(); + std::fs::remove_file(&record).expect("removes the record"); + std::fs::create_dir(&record).expect("a directory in its place"); + let keystore: serde_json::Value = + serde_json::from_str(&keystore_json()).expect("the vector is json"); + let previous = ValidatorDefinition { + enabled: false, + voting_public_key: format!("0x{}", keystore["pubkey"].as_str().expect("pubkey")), + voting_keystore_path: dir.path().join("old.json"), + voting_keystore_password_path: dir.path().join("old"), + }; + ValidatorDefinitions(vec![previous.clone()]) + .save(dir.path()) + .expect("saves"); + let mut context = + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + + 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!( + body["data"][0]["message"] + .as_str() + .unwrap_or_default() + .contains("back as it was"), + "got {body}" + ); + let definitions = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(definitions.0, vec![previous], "the disabled entry is back"); + let json_files: Vec = std::fs::read_dir(dir.path()) + .expect("lists") + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.path().is_file()) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.ends_with(".json")) + .collect(); + assert!( + json_files.is_empty(), + "the keystore this request wrote goes back to what was there, nothing: {json_files:?}" + ); + } + + /// A failed registration puts the keystore and password files a re-import + /// overwrote back as they were, as a pair, so the restored entry never + /// points at a keystore encrypted under a password that is gone. + #[tokio::test] + async fn a_failed_registration_restores_the_overwritten_files() { + 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 record = protection.path().expect("persisted").to_path_buf(); + std::fs::remove_file(&record).expect("removes the record"); + std::fs::create_dir(&record).expect("a directory in its place"); + let keystore: serde_json::Value = + serde_json::from_str(&keystore_json()).expect("the vector is json"); + let pubkey = parse_pubkey(keystore["pubkey"].as_str().expect("pubkey")).expect("parses"); + let base = encode_hex(&pubkey.0); + let keystore_path = dir.path().join(format!("{base}.json")); + let password_path = dir.path().join(&base); + std::fs::write(&keystore_path, b"old keystore").expect("writes"); + std::fs::write(&password_path, b"old password").expect("writes"); + let previous = ValidatorDefinition { + enabled: true, + voting_public_key: base.clone(), + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: password_path.clone(), + }; + ValidatorDefinitions(vec![previous.clone()]) + .save(dir.path()) + .expect("saves"); + let mut context = + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + + 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_eq!( + std::fs::read(&keystore_path).expect("still there"), + b"old keystore" + ); + assert_eq!( + std::fs::read(&password_path).expect("still there"), + b"old password" + ); + let definitions = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(definitions.0, vec![previous]); + } + + /// A re-import whose password write fails puts the keystore it had already + /// replaced back, so the previous entry never points at a keystore + /// encrypted under a password that was never written. + #[tokio::test] + async fn a_failed_password_write_puts_the_replaced_keystore_back() { + let dir = tempfile::tempdir().expect("temp dir"); + let keystore: serde_json::Value = + serde_json::from_str(&keystore_json()).expect("the vector is json"); + let pubkey = parse_pubkey(keystore["pubkey"].as_str().expect("pubkey")).expect("parses"); + let base = encode_hex(&pubkey.0); + let keystore_path = dir.path().join(format!("{base}.json")); + let password_path = dir.path().join(&base); + std::fs::write(&keystore_path, b"old keystore").expect("writes"); + // A directory at the password's path makes its write fail, after the + // keystore's succeeded. + std::fs::create_dir(&password_path).expect("a directory in its place"); + let previous = ValidatorDefinition { + enabled: true, + voting_public_key: base.clone(), + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: dir.path().join("old"), + }; + ValidatorDefinitions(vec![previous.clone()]) + .save(dir.path()) + .expect("saves"); + let context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + + 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_eq!( + std::fs::read(&keystore_path).expect("still there"), + b"old keystore" + ); + let definitions = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(definitions.0, vec![previous]); + } + + /// A request that carries history is refused whole, importing nothing. + #[tokio::test] + async fn an_import_carrying_history_is_refused_whole() { + let dir = tempfile::tempdir().expect("temp dir"); + let context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + let store = context.store.clone(); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + "slashing_protection": "{}", + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "error", "got {body}"); + assert!(store.read().await.pubkeys().is_empty()); + assert!( + ValidatorDefinitions::open(dir.path()) + .expect("opens") + .0 + .is_empty() + ); + } + + /// The same keystore twice in one request, with the registration failing: + /// the second copy snapshotted what the first one wrote, and the undo must + /// still end with what was there before the request. + #[tokio::test] + async fn a_failed_registration_restores_the_state_before_a_duplicated_import() { + 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 record = protection.path().expect("persisted").to_path_buf(); + std::fs::remove_file(&record).expect("removes the record"); + std::fs::create_dir(&record).expect("a directory in its place"); + let mut context = + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json(), keystore_json()], + "passwords": [PASSWORD, PASSWORD], + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "error", "got {body}"); + assert_eq!(body["data"][1]["status"], "error", "got {body}"); + assert!( + ValidatorDefinitions::open(dir.path()) + .expect("opens") + .0 + .is_empty() + ); + let leftovers: Vec = std::fs::read_dir(dir.path()) + .expect("lists") + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.path().is_file()) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| { + !name.starts_with("slashing_protection") + && name != crate::keys::definitions::DEFINITIONS_FILE + }) + .collect(); + assert!( + leftovers.is_empty(), + "nothing was there before: {leftovers:?}" + ); + } + + /// 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) + .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 26e250b6e..f16de2850 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -58,6 +58,14 @@ pub struct KeymanagerContext { /// it takes this one first and `store` second, consistently, so the two /// never deadlock against each other. pub definitions_lock: Arc>, + /// Whether the last save of the definitions file replaced it without + /// flushing the directory, so its last change may not survive a power + /// loss. Set and cleared by `keystores::save_definitions`; read where a + /// retry would otherwise report that change as final. + pub definitions_unflushed: 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..91c5ab5e4 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -367,6 +367,8 @@ mod tests { validators_dir: dir.path().to_path_buf(), secrets_dir: dir.path().to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + protection: crate::slashing_protection::SlashingProtection::in_memory(), }; 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..3b091585a 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,17 +63,24 @@ 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 { - path: path.display().to_string(), - source, + // An after-rename failure is `Error::Unflushed`: the file is replaced + // and every reader sees the new one, only its survival of a power loss + // is in doubt. What that means is the caller's to decide. An import + // can count it as written (a lost entry only removes a key), a delete + // cannot (a lost removal brings a key back). + secure_fs::write_private_durably(&path, contents).map_err(|err| { + let path = path.display().to_string(); + if err.after_rename { + Error::Unflushed { + path, + source: err.source, + } + } else { + Error::Io { + path, + source: err.source, + } + } }) } diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index ebec73b1f..faf9f35e7 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 @@ -135,6 +119,11 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { config.suggested_fee_recipient, )); + // The directory's lock first, before any key is read: held from here, + // nothing another process does to the directory can get between what + // this one loads and what it signs with. See `SlashingProtection::lock`. + let record_lock = slashing_protection::SlashingProtection::lock(&config.validators_dir)?; + let discovered = keys::definitions::ValidatorDefinitions::discover_if_absent( &config.validators_dir, &config.secrets_dir, @@ -192,17 +181,42 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { ); 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. + let keys = store.read().await.pubkeys(); + let protection = slashing_protection::SlashingProtection::open_locked( + record_lock, + &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 context = Arc::new(SigningContext { config: spec, genesis_validators_root: genesis.genesis_validators_root, }); 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 +226,8 @@ 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(())), + definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + protection: protection.clone(), }; let router = http_api::router(context, token); let listener = tokio::net::TcpListener::bind(address) @@ -462,7 +478,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 +636,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 +1213,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 +1355,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 +1395,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..ad52aa95b 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, 1.0, 2.5, 5.0 + ] + ) + .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) { @@ -364,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(); } @@ -450,6 +481,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..c16319f07 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::{Refusal, 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, } } @@ -163,37 +162,15 @@ 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: 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) { - warn!( - %slot, - validator = duty.validator_index, - %refusal, - "Refusing to propose: this process already proposed this slot" - ); - crate::metrics::inc_blocks_refused(); - return Err(Error::ProposalRefused { - slot, - reason: refusal.to_string(), - }); - } - } - Err(err) => { - warn!(%slot, %err, "Proposal guard is poisoned; refusing to propose"); - crate::metrics::inc_blocks_refused(); - return Err(Error::ProposalRefused { - slot, - reason: "the proposal guard is poisoned".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; + refuse_unless_safe(slot, duty.validator_index, "consult", checked)?; // Scoped away from every await, the way the attestation path's read // guard is, and for the reason spelled out there: a write-preferring @@ -225,11 +202,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 @@ -265,33 +242,22 @@ 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 + // 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 recorded = crate::slashing_protection::blocking(&self.protection, move |protection| { + protection.check_and_record_block(&pubkey, slot) + }) + .await; + refuse_unless_safe(slot, duty.validator_index, "record", recorded)?; + 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(), - }); - } - } - self.context.sign_block(&store, &pubkey, block_root, slot)? }; @@ -436,11 +402,46 @@ impl ProposalService { } } +/// Slashing protection's answer about a block, as the duty's outcome: a judged +/// refusal, or a record that could not be consulted or written (`what` names +/// which), is a counted [`Error::ProposalRefused`]. +fn refuse_unless_safe( + slot: Slot, + validator: u64, + what: &'static str, + outcome: Result>, +) -> Result<()> { + let reason = match outcome { + Ok(Ok(())) => return Ok(()), + Ok(Err(refusal)) => { + warn!( + %slot, + validator, + %refusal, + "Refusing to propose: slashing protection judged this block unsafe" + ); + refusal.to_string() + } + Err(err) => { + error!( + %slot, + validator, + %err, + "Slashing protection could not {what} this block; refusing to sign" + ); + err.to_string() + } + }; + crate::metrics::inc_blocks_refused(); + Err(Error::ProposalRefused { slot, reason }) +} + #[cfg(test)] 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 +478,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 +491,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 +593,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 @@ -628,7 +644,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() { @@ -654,6 +670,65 @@ 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()); + } + + /// A persisted record holding nothing for the key refuses it before a + /// block is asked for. The in-memory record of the other tests registers + /// a key on first use; a real one does not. + #[tokio::test] + async fn an_unregistered_key_is_refused_before_a_block_is_requested() { + let (store, pubkey) = store(); + let dir = tempfile::tempdir().expect("temp dir"); + let settings = Arc::new(ProposerSettings::new(Bytes32::repeat_byte(0xab), None)); + let opened = SlashingProtection::open(dir.path(), Root::ZERO, &[], false) + .expect("opens with no keys"); + let node = Arc::new(MockBeaconNode::new().with_block(slot(), 7)); + + let err = ProposalService::new( + node.clone(), + context(), + settings, + Arc::new(std::sync::Mutex::new(opened)), + ) + .propose(slot(), &duty(&pubkey, 7, slot()), &store) + .await + .expect_err("the key has no record"); + assert!( + matches!(&err, Error::ProposalRefused { reason, .. } if reason.contains("no slashing-protection record")), + "got {err:?}" + ); + assert!(node.block_requests().is_empty()); + assert!(node.published_blocks().is_empty()); + } + #[tokio::test] async fn a_node_that_cannot_import_the_block_is_reported_rather_than_hidden() { let (store, pubkey) = store(); @@ -745,12 +820,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() { @@ -781,6 +856,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 +903,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 +985,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); @@ -1006,8 +1092,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/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/secure_fs.rs b/crates/validator/src/secure_fs.rs index 574056e9f..77a2b4ad9 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 @@ -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 _; @@ -75,6 +75,154 @@ pub(crate) fn write_private_no_symlink( std::fs::write(path, contents) } +/// Why [`write_private_durably`] failed, and whether the file had already +/// been replaced when it did. +#[derive(Debug)] +pub(crate) struct DurableWriteError { + pub(crate) source: std::io::Error, + /// The rename had happened: the new file is at its path, and only the + /// directory flush that makes the rename survive a power loss failed. + /// Until the next successful flush the file on disk may be either one. + pub(crate) after_rename: bool, +} + +impl std::fmt::Display for DurableWriteError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if self.after_rename { + write!( + f, + "written, but the directory could not be flushed, so the write may not survive \ + a power loss: {}", + self.source + ) + } else { + self.source.fmt(f) + } + } +} + +impl std::error::Error for DurableWriteError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.source) + } +} + +/// 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. An error says whether the +/// file was already replaced when it failed (see [`DurableWriteError`]): +/// before the rename nothing at `path` changed; after it, only the flush that +/// makes the rename durable is missing. Unix only: elsewhere nothing is +/// touched and the write fails up front. +/// +/// 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. A write that fails before +/// the rename removes its temporary again, best effort. +#[cfg(unix)] +pub(crate) fn write_private_durably( + path: &Path, + contents: impl AsRef<[u8]>, +) -> Result<(), DurableWriteError> { + use std::io::Write as _; + let before = |source| DurableWriteError { + source, + after_rename: false, + }; + let mut name = path.file_name().unwrap_or_default().to_os_string(); + name.push(".tmp"); + let temporary = path.with_file_name(name); + + match std::fs::remove_file(&temporary) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(before(err)), + } + let staged = (|| { + let mut file = create_private(&temporary)?; + file.write_all(contents.as_ref())?; + file.sync_all() + })() + .and_then(|()| std::fs::rename(&temporary, path)); + if let Err(source) = staged { + let _ = std::fs::remove_file(&temporary); + return Err(before(source)); + } + // `Path::new("record.json").parent()` is `Some("")`, not `None`, and + // opening "" fails; the current directory is what it means. + let directory = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + sync_directory(directory).map_err(|source| DurableWriteError { + source, + after_rename: true, + }) +} + +/// Off unix the directory flush is not implemented, so the write is refused +/// before anything is touched rather than answer `Ok` for a rename that may +/// not have reached the disk. +#[cfg(not(unix))] +pub(crate) fn write_private_durably( + _path: &Path, + _contents: impl AsRef<[u8]>, +) -> Result<(), DurableWriteError> { + Err(DurableWriteError { + source: std::io::Error::new( + std::io::ErrorKind::Unsupported, + "durable writes are only implemented on unix", + ), + after_rename: false, + }) +} + +#[cfg(unix)] +fn create_private(path: &Path) -> std::io::Result { + use std::os::unix::fs::OpenOptionsExt as _; + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) +} + +/// Flush the directory, so a rename into it is itself durable. +/// +/// `File::sync_all` is `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 macOS filesystems refuse that request for a directory, so there a +/// plain `fsync` is tried next: it is the best those offer, and the original +/// error is kept if even that fails. Only there: on Linux the two are the +/// same call, and a second `fsync` after a failed one can answer success +/// while the kernel has already dropped the writeback error with the data, +/// which would make a flush that did not happen look durable. +#[cfg(unix)] +fn sync_directory(directory: &Path) -> std::io::Result<()> { + let directory = std::fs::File::open(directory)?; + let result = directory.sync_all(); + #[cfg(target_os = "macos")] + let result = result.or_else(|full_sync| { + use std::os::fd::AsRawFd as _; + // 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) + } + }); + result +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -94,6 +242,77 @@ 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_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" + ); + } + + /// A write that fails before the rename takes its temporary with it, and + /// says the file was not replaced. + #[test] + fn a_write_that_fails_before_the_rename_leaves_no_temporary() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("record"); + std::fs::create_dir(&path).expect("a directory in the file's place"); + + let err = write_private_durably(&path, "x").expect_err("cannot rename over a directory"); + + assert!(!err.after_rename, "{err}"); + assert!(!path.with_file_name("record.tmp").exists()); + assert!(path.is_dir(), "the directory is 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/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..84e2e9607 --- /dev/null +++ b/crates/validator/src/slashing_protection/file.rs @@ -0,0 +1,361 @@ +//! 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::path::Path; + +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 only EIP-3076 interchange version this client reads or writes. +pub const INTERCHANGE_VERSION: &str = "5"; + +/// This client's own file, read strictly: a field it did not write is an +/// error, since nothing else should have written here. Not for reading +/// other clients' interchange files, which may carry fields this refuses; +/// an importer needs its own, lenient, types. +#[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 { + // The same shape the record and the attestation-data check refuse. + // Read into the record, it would refuse every honest attestation + // after it. + if super::is_impossible_source(signed.source_epoch, signed.target_epoch) { + return Err(format!( + "attestation source epoch {} is at or 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, + }) +} + +/// 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`. +/// +/// 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: impl Iterator, +) -> Result<()> { + let mut data: Vec = records + .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(&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(|err| { + let path = path.display().to_string(); + if err.after_rename { + Error::Unflushed { + path, + source: err.source, + } + } else { + Error::Io { + path, + source: err.source, + } + } + }) +} + +/// Take the exclusive lock in `directory`, held until the returned file is +/// dropped. +pub(crate) fn lock(directory: &Path) -> Result { + let io_error = |source| Error::Io { + path: directory.display().to_string(), + source, + }; + // The directory itself, not a lock file beside the record: the record is + // replaced by a rename on every write, so a lock on it would be on a + // stale inode; and a lock file can be deleted, after which a second + // process locks a fresh one and both sign. The directory is the one + // inode nothing here replaces, and `flock` takes a directory descriptor. + let file = File::open(directory).map_err(io_error)?; + match file.try_lock() { + Ok(()) => Ok(file), + Err(std::fs::TryLockError::WouldBlock) => Err(Error::SlashingProtection { + path: directory.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)), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A source at its target, outside genesis, is refused on read as it is + /// on write: loaded, it would refuse every honest attestation after it. + #[test] + fn an_entry_with_a_source_at_its_target_is_refused() { + let entry = Entry { + pubkey: [1; 48], + signed_blocks: vec![], + signed_attestations: vec![SignedAttestation { + source_epoch: 5, + target_epoch: 5, + signing_root: None, + }], + }; + let err = record_of(&entry).expect_err("source at target"); + assert!(err.contains("at or after"), "{err}"); + + let genesis = Entry { + pubkey: [1; 48], + signed_blocks: vec![], + signed_attestations: vec![SignedAttestation { + source_epoch: 0, + target_epoch: 0, + signing_root: None, + }], + }; + assert!(record_of(&genesis).is_ok()); + } + + /// Several entries of a kind for one validator reduce to their maxima: + /// what keeps a file holding more than one from lowering a watermark. + #[test] + fn several_entries_of_a_kind_reduce_to_their_maxima() { + let entry = Entry { + pubkey: [1; 48], + signed_blocks: vec![ + SignedBlock { + slot: 7, + signing_root: None, + }, + SignedBlock { + slot: 3, + signing_root: None, + }, + ], + signed_attestations: vec![ + SignedAttestation { + source_epoch: 5, + target_epoch: 9, + signing_root: None, + }, + SignedAttestation { + source_epoch: 2, + target_epoch: 4, + signing_root: None, + }, + ], + }; + + let record = record_of(&entry).expect("valid"); + + assert_eq!( + record, + Record { + block_slot: Some(7), + attestation: Some(Signed { + source: 5, + target: 9 + }), + } + ); + } +} diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs new file mode 100644 index 000000000..55ba67f2e --- /dev/null +++ b/crates/validator/src/slashing_protection/mod.rs @@ -0,0 +1,1094 @@ +//! 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 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 +//! in-run shapes the in-memory guards this replaces were written for. +//! +//! # What it costs +//! +//! Every write rewrites the whole record, one compact JSON file, and flushes +//! it and its directory: the cost of a slot's attestation batch or a block +//! grows with the number of keys, not with the batch, and sits on the signing +//! path, off the executor. Fine for hundreds of keys on a local disk. For +//! thousands, or on a network volume whose flush takes tens of milliseconds +//! or more, `ethlambda_validator_slashing_protection_write_seconds` is the +//! number to watch, and the directory should be a local disk. +//! +//! 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. A key registered with no history starts as never +//! having signed: the migration window, where the key may have signed +//! elsewhere a moment ago, is doppelganger protection's to close, not this +//! record's. + +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::{debug, 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. Async callers go through +/// [`blocking`], so neither the write nor waiting for another holder's write +/// holds up the executor. +pub type SharedProtection = Arc>; + +/// How long [`blocking`] waits for the record before refusing, and how often +/// it looks. A flush takes milliseconds on a healthy disk; a quarter of a +/// 12-second slot is what a slow one gets. +const RECORD_WAIT: std::time::Duration = std::time::Duration::from_secs(3); +const RECORD_POLL: std::time::Duration = std::time::Duration::from_millis(5); + +/// 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 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 +/// message that is then never signed only costs that message. +/// +/// A caller that finds the record held for [`RECORD_WAIT`] is refused, and +/// the duty is not retried within the slot: the holder is a flush still +/// running, and a retry would wait on that same flush. Nothing was recorded +/// for the refused duty, so the next slot's are unaffected; the write itself +/// is not bounded, since a flush cannot be interrupted, only outwaited. +pub async fn blocking( + protection: &SharedProtection, + operation: impl FnOnce(&mut SlashingProtection) -> Result + Send + 'static, +) -> Result { + let poisoned = || Error::SlashingProtectionUnusable { + reason: "an earlier access panicked while holding the record, so nothing is signed \ + until the validator client is restarted" + .to_string(), + }; + let protection = protection.clone(); + tokio::task::spawn_blocking(move || { + // A bounded wait rather than `lock()`: a write normally holds the + // record for milliseconds, so a wait this long means a flush is + // stuck, and refusing then keeps a stuck disk from piling up one + // blocking task per slot until the pool is full. Nothing is signed + // on a refusal. + let deadline = std::time::Instant::now() + RECORD_WAIT; + let mut protection = loop { + match protection.try_lock() { + Ok(guard) => break guard, + Err(std::sync::TryLockError::Poisoned(_)) => return Err(poisoned()), + Err(std::sync::TryLockError::WouldBlock) => { + if std::time::Instant::now() >= deadline { + return Err(Error::SlashingProtectionUnusable { + reason: format!( + "the record has been held by other accesses for {} s: a write \ + may be stuck, or the keymanager may be busy", + RECORD_WAIT.as_secs() + ), + }); + } + std::thread::sleep(RECORD_POLL); + } + } + }; + operation(&mut protection) + }) + .await + .map_err(|err| { + if err.is_panic() { + poisoned() + } else { + Error::SlashingProtectionUnusable { + reason: format!("the record could not be consulted: {err}"), + } + } + })? +} + +/// Whether an attestation's source epoch is at or past its target, which is +/// never valid outside genesis: with unsigned epochs, `(0, 0)` is the only +/// such pair that is. One predicate for the three places that refuse it, +/// the attestation-data check, the record and the file reader, so they +/// cannot drift. +pub(crate) fn is_impossible_source(source: Epoch, target: Epoch) -> bool { + source >= target && source != 0 +} + +/// 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); + // `validate_attestation_data` refuses the shape before it reaches the + // record; this is the record defending itself, since recording such a + // pair would refuse every honest attestation after it. + if is_impossible_source(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 at or 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 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, + ) -> Result { + let lock = Self::lock(validators_dir)?; + Self::open_locked(lock, validators_dir, genesis_validators_root, keys, init) + } + + /// Take the directory's exclusive lock ahead of [`Self::open_locked`], so + /// it can be held from before the keys are loaded: taken only at `open`, + /// a second process could load a key, wait for the first to delete it and + /// exit, then take the lock and sign with a key the file no longer lists. + pub fn lock(validators_dir: &Path) -> Result { + file::lock(validators_dir) + } + + /// [`Self::open`] with the lock already held, from [`Self::lock`]. + pub fn open_locked( + lock: std::fs::File, + validators_dir: &Path, + genesis_validators_root: Root, + keys: &[BlsPubkey], + init: bool, + ) -> Result { + let path = validators_dir.join(file::FILE_NAME); + let refused = |reason: String| Error::SlashingProtection { + path: path.display().to_string(), + reason, + }; + + let existed = file::exists(&path)?; + 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}). \ + 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. That rules out a double vote, not a surround of an older \ + vote; a key with a history should bring it", + missing.len() + ))); + } + for key in &missing { + debug!( + pubkey = %encode_hex(&key.0), + "Registering a validator with no slashing-protection history" + ); + records.insert(*key, Record::default()); + } + if !missing.is_empty() { + warn!( + registered = missing.len(), + "Registered validators with no slashing-protection history \ + (--init-slashing-protection); each starts as never having signed" + ); + } + if init && missing.is_empty() && !keys.is_empty() { + warn!( + "--init-slashing-protection is set but every key already has a record; remove \ + the flag, or a keystore copied into the directory later is registered with no \ + history without anyone noticing" + ); + } + + // Written once whatever its state, as a probe: a filesystem that + // refuses to flush a directory (some FUSE, CIFS and overlay mounts + // answer EINVAL) would otherwise fail every slot's write, with the + // records advancing in memory and nothing ever signed, and nothing at + // startup to say why. Here it is a startup error naming the path. + file::write(&path, genesis_validators_root, records.iter())?; + 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`], and this is behind the `test-utils` feature so no + /// production caller can reach it. + #[cfg(any(test, feature = "test-utils"))] + 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 the in-memory record of the tests. + 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, and nothing may + /// be signed. Nothing changed either, with one exception: on + /// [`Error::Unflushed`] the slot is on disk and in memory (see + /// [`Self::commit`]), so a retry is refused as already proposed. Treat + /// every `Err` as "do not sign"; none of them means the old watermark + /// is still the one in force. + 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, 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, and none of them may be + /// signed. Nothing changed either, with one exception: on + /// [`Error::Unflushed`] the target is on disk and in memory (see + /// [`Self::commit`]), so a retry is refused as already signed. Treat every + /// `Err` as "do not sign"; none of them means the old watermarks are + /// still the ones in force. + 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 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 { + 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 + /// as they were and nothing is signed. One failure is the exception: a + /// write that replaced the file but could not flush the directory + /// ([`Error::Unflushed`]) has the new watermarks on disk, perhaps only + /// until a power loss, and memory takes them too, so that what this + /// process holds is never weaker than what a reader of the file sees. + /// Nothing is signed on that path either. + fn commit(&mut self, changes: HashMap) -> Result<()> { + 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(); + if let Err(err) = file::write(&storage.path, self.genesis_validators_root, next) { + crate::metrics::inc_slashing_protection_write_failures(); + if matches!(err, Error::Unflushed { .. }) { + self.records.extend(changes); + } + return Err(err); + } + } + self.records.extend(changes); + 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) + } + + /// 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. + + #[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 _; + + 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"); + + 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"); + } + + /// The lock is on the directory, so a second opener of the same one is + /// refused, with nothing beside the record an operator could delete to + /// get around it. + #[test] + fn a_second_opener_of_the_same_directory_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + let _first = open(dir.path(), &[1], true).expect("init"); + + let err = open(dir.path(), &[1], false).expect_err("locked"); + assert!( + err.to_string().contains("in use by another process"), + "{err}" + ); + assert!( + !dir.path().join("slashing_protection.lock").exists(), + "no lock file to delete" + ); + } + + #[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()); + } + + /// A key registered with no history, at startup or at runtime, starts as + /// never having signed: slot 0 and epoch 0 are signable once. + #[test] + fn a_key_registered_with_no_history_starts_as_never_signed() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + protection.register(&[pubkey(2)]).expect("register"); + + for key in [1, 2] { + assert!(propose(&mut protection, key, 0).is_ok(), "key {key}"); + assert!(propose(&mut protection, key, 0).is_err(), "key {key}"); + assert!( + attest(&mut protection, key, &data(0, 0, 0, 0xaa)).is_ok(), + "key {key}" + ); + assert!( + attest(&mut protection, key, &data(0, 0, 0, 0xbb)).is_err(), + "key {key}" + ); + } + } + + /// 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"); + 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..4455072c4 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -262,17 +262,23 @@ 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, so `--validators-dir` must be writable: a read-only +> mount of the keys (a Kubernetes Secret, a `:ro` volume) fails at startup, as +> does a record that is a symlink (say, onto another volume) rather than a +> regular file, since a dangling one would read as "no record"; and +> the directory should be a local disk, since every signature waits for the +> record to be flushed to it, and the lock that keeps a second process off it +> is an `flock`, which a network filesystem may not honour. Unix only. A key with no record is refused at startup unless +> `--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 +291,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, and even then it rules out a double vote only, not a surround of an older vote; a key with a history should bring it instead. Never lowers an existing record; remove it once the keys are registered, and startup warns while it is set for nothing | ### What it does today diff --git a/docs/metrics.md b/docs/metrics.md index 7107778d1..9ac92fd4f 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -725,11 +725,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 | @@ -746,6 +746,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. Milliseconds on a local disk; worth an alert above about a second, since the duty waits for it and a write held past 3 s is refused | +| `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..0e96cf6f4 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -40,58 +40,89 @@ 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. -- **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 + 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 `flock` on the validators directory itself, the one + inode nothing replaces or deletes, 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. +- **A wedged record:** the client refuses attestation data whose source is at + or past its target, which is never valid outside genesis, so an impossible + answer from a beacon node is refused rather than recorded. What it cannot + tell is a possible one: a source below the target but ahead of what the + chain has justified passes, the record keeps it, and every honest + attestation after it is refused as `SourceRegressed` until justification + reaches it, which during non-finality may be never. That is trust in the + beacon node, the same trust every attestation already places in it; the + symptom is the `SourceRegressed` refusal in the log and a non-zero + `attestations_refused_total`. There is no tool that lowers a watermark, by + design: stop the client, edit `slashing_protection.json` by hand, and start + it again. Lowering a watermark is only safe for a key that has not signed + past the value written. The same holds for a clock that jumps forward: the + target is pinned to the slot the local clock asked for, so a beacon node + that answers for a slot far ahead lets the record run ahead of the chain. + A bound against the clock would be circular; the guard is the beacon + node's own view of the head, which doppelganger protection will bring. +- **No checksum, and rewritten minimal at every start:** the record is an + EIP-3076 minimal interchange and nothing more, by design: readable by other + tools and editable by the operator, which the hand edit above relies on. A + checksum or a generation counter would make it this client's private format + and refuse that edit, to guard against a flipped digit that still parses, + which the disk's and the filesystem's own integrity checks are there for. + Every start rewrites the file in the minimal shape, so signing roots and + extra entries dropped in by hand collapse to their maxima. +- **Keys with no record:** refused at startup. `--init-slashing-protection` + registers them as never having signed, and a key imported through the + keymanager API with no history is registered the same way before it is + loaded. Nothing here stops such a key from signing again what it signed + elsewhere a moment ago, so the flag is only for keys that have signed nothing + anywhere for at least two epochs; the migration window is doppelganger + protection's to close, which this client does not have yet. +- **Messages outside the rule:** sync committee messages and contributions + are not slashable, and are not recorded. 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 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` refuses a request that carries the + `slashing_protection` field, importing none of its keys, so a migration that + hands over history fails loudly instead of starting the key as never having + signed; a key imported without it starts that way, 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