From 1888a0d3c9b9069934b0b3e8298ea9e7a3feb24b Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 29 Oct 2026 18:36:32 -0300 Subject: [PATCH 01/12] Add EIP-3076 interchange import and export to the validator client's slashing protection, so a staker can move keys in and out with their history. SlashingProtection gains import (version 5, complete or minimal, checked against the record's genesis_validators_root, each validator reduced to its highest slot, source and target and merged by maximum in one durable write, so slashable data is absorbed and nothing is lowered) and export (the record as a minimal interchange). A new offline ethlambda slashing-protection import|export command works against --validators-dir under the client's lock. The keymanager API now imports POST /eth/v1/keystores' slashing_protection before any key, refusing every key if it is invalid or for another chain, and DELETE returns the deleted keys' real records, answering not_active for a key that is gone but has one. The shared eth-clients/slashing-protection-interchange-tests v5.3.0 vectors are downloaded by a pinned, checksummed make target that test-node depends on, and all 38 pass under the minimal expectations. --- .gitignore | 4 + Makefile | 31 +- README.md | 5 +- bin/ethlambda/src/command.rs | 38 +- bin/ethlambda/src/main.rs | 6 + bin/ethlambda/src/slashing_protection.rs | 99 +++++ crates/validator/src/http_api/keystores.rs | 374 +++++++++++++----- crates/validator/src/lib.rs | 10 +- .../validator/src/slashing_protection/file.rs | 97 +++-- .../validator/src/slashing_protection/mod.rs | 213 ++++++++++ crates/validator/tests/interchange_vectors.rs | 210 ++++++++++ docs/cli.md | 32 +- docs/spec_deviations.md | 17 +- 13 files changed, 986 insertions(+), 150 deletions(-) create mode 100644 bin/ethlambda/src/slashing_protection.rs create mode 100644 crates/validator/tests/interchange_vectors.rs diff --git a/.gitignore b/.gitignore index b2e487fa2..cfaaffa96 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,10 @@ target # BLS and KZG test vectors, downloaded by `make cryptography-specs` /cryptography-specs +# EIP-3076 interchange test vectors, downloaded by +# `make slashing-protection-interchange-tests` +/slashing-protection-interchange-tests + # Personal Claude Code overrides (not shared with team) CLAUDE.local.md diff --git a/Makefile b/Makefile index 10796aff3..9ede8ba1d 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help fmt lint bench update cooldown-check docker-build shadow-build shadow-docker-build run-devnet test test-consensus test-node test-beacon test-beacon-mainnet test-beacon-minimal consensus-spec-tests cryptography-specs docs docs-deps docs-serve +.PHONY: help fmt lint bench update cooldown-check docker-build shadow-build shadow-docker-build run-devnet test test-consensus test-node test-beacon test-beacon-mainnet test-beacon-minimal consensus-spec-tests cryptography-specs docs docs-deps docs-serve slashing-protection-interchange-tests help: ## 📚 Show help for each of the Makefile recipes @grep -E '^[a-zA-Z0-9_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' @@ -38,7 +38,7 @@ test: test-consensus test-node ## 🧪 Run all tests test-consensus: leanSpec/fixtures ## 🧪 Run the consensus half of the workspace suite $(TEST) $(addprefix -p ,$(CONSENSUS_CRATES)) -test-node: leanSpec/fixtures ## 🧪 Run the node half of the workspace suite +test-node: leanSpec/fixtures slashing-protection-interchange-tests ## 🧪 Run the node half of the workspace suite $(TEST) --workspace $(addprefix --exclude ,$(CONSENSUS_CRATES)) # --lib as well as the spec target: the BLS and KZG modules keep their fixture @@ -211,6 +211,33 @@ $(CRYPTOGRAPHY_SPECS_STAMP): unzip -q "$$tmpdir/tests.zip" -d cryptography-specs || exit 1 @touch $@ +# EIP-3076 slashing-protection interchange test vectors, for the validator +# client's `slashing_protection` module (crates/validator/tests/ +# interchange_vectors.rs). Pinned, and stamped by version like the fixture trees +# above, so a bump re-downloads rather than passing against the old cases. The +# release ships no asset for this tag, so the tag's source archive is fetched +# and checked against a pinned SHA-256 instead. +SLASHING_PROTECTION_TESTS_VERSION ?= v5.3.0 +SLASHING_PROTECTION_TESTS_SHA256 ?= 516d551cfb3e50e4ac2f42db0992f4ceb573a7cb1616d727a725c8161485329f +SLASHING_PROTECTION_TESTS_URL ?= https://github.com/eth-clients/slashing-protection-interchange-tests/archive/refs/tags/$(SLASHING_PROTECTION_TESTS_VERSION).tar.gz +SLASHING_PROTECTION_TESTS_STAMP=slashing-protection-interchange-tests/.version-$(SLASHING_PROTECTION_TESTS_VERSION) + +slashing-protection-interchange-tests: $(SLASHING_PROTECTION_TESTS_STAMP) ## ⬇️ Download the EIP-3076 interchange test vectors + +$(SLASHING_PROTECTION_TESTS_STAMP): + @rm -rf slashing-protection-interchange-tests + @mkdir -p slashing-protection-interchange-tests + @tmpdir=$$(mktemp -d); trap 'rm -rf "$$tmpdir"' EXIT; \ + echo "Downloading EIP-3076 interchange test vectors ($(SLASHING_PROTECTION_TESTS_VERSION))"; \ + curl -L -f -o "$$tmpdir/tests.tar.gz" "$(SLASHING_PROTECTION_TESTS_URL)" || exit 1; \ + actual=$$(sha256sum "$$tmpdir/tests.tar.gz" | awk '{print $$1}'); \ + if [ "$$actual" != "$(SLASHING_PROTECTION_TESTS_SHA256)" ]; then \ + echo "SHA256 mismatch: expected $(SLASHING_PROTECTION_TESTS_SHA256), got $$actual" >&2; \ + exit 1; \ + fi; \ + tar -xzf "$$tmpdir/tests.tar.gz" -C slashing-protection-interchange-tests --strip-components=1 || exit 1 + @touch $@ + # lambdaclass fork of lean-quickstart: genesis keys come from `ethlambda keygen`, and the # partner clients run their devnet-5 images. An existing lean-quickstart/ is never # re-cloned, so delete it to pick up a new pin. diff --git a/README.md b/README.md index 99bfcbd1f..2763cd373 100644 --- a/README.md +++ b/README.md @@ -348,8 +348,9 @@ the standard Beacon API, so it works against `ethlambda beacon` (run with the > 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 +> A key moving from another client brings its history with +> `ethlambda slashing-protection import` (or the keymanager API), and never runs +> in two clients at once. See > [Spec deviations](./docs/spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). ```sh diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs index 7d0b9556a..d5cfe4d54 100644 --- a/bin/ethlambda/src/command.rs +++ b/bin/ethlambda/src/command.rs @@ -30,6 +30,7 @@ const EXPLICIT: &[&str] = &[ BENCHMARK, REPLAY_ERA, VALIDATOR, + SLASHING_PROTECTION, KEYGEN, "help", "-h", @@ -43,6 +44,7 @@ const BEACON: &str = "beacon"; const BENCHMARK: &str = "benchmark"; const REPLAY_ERA: &str = "replay-era"; const VALIDATOR: &str = "validator"; +const SLASHING_PROTECTION: &str = "slashing-protection"; const KEYGEN: &str = "keygen"; #[derive(Debug, clap::Parser)] @@ -84,6 +86,9 @@ pub(crate) enum Command { ReplayEra(ReplayEraOptions), /// Run the beacon-chain validator client against a beacon node's HTTP API. Validator(crate::validator::ValidatorOptions), + /// Import or export the validator client's EIP-3076 slashing-protection + /// history, offline. + SlashingProtection(crate::slashing_protection::SlashingProtectionOptions), /// Generate validator XMSS keys for a genesis. Keygen(KeygenOptions), } @@ -415,7 +420,14 @@ mod tests { /// `ethlambda node keygen ...`, which fails on a stray positional. #[test] fn sub_command_tokens_do_not_get_the_default_inserted() { - for token in [NODE, BEACON, BENCHMARK, VALIDATOR, KEYGEN] { + for token in [ + NODE, + BEACON, + BENCHMARK, + VALIDATOR, + SLASHING_PROTECTION, + KEYGEN, + ] { let args = [OsString::from("ethlambda"), OsString::from(token)]; assert_eq!( default_subcommand(&args), @@ -425,6 +437,30 @@ mod tests { } } + #[test] + fn slashing_protection_parses_as_its_own_sub_command() { + let args = [ + "ethlambda", + SLASHING_PROTECTION, + "export", + "--validators-dir", + "/tmp/validators", + "--pubkeys", + "0xaa,0xbb", + "out.json", + ]; + match try_parse_from(args.iter().map(OsString::from)).expect("parses") { + Command::SlashingProtection(options) => match options.action { + crate::slashing_protection::Action::Export { pubkeys, file, .. } => { + assert_eq!(pubkeys, vec!["0xaa", "0xbb"]); + assert_eq!(file, PathBuf::from("out.json")); + } + other => panic!("expected an export, got {other:?}"), + }, + other => panic!("expected a slashing-protection invocation, got {other:?}"), + } + } + #[test] fn keygen_parses_as_its_own_sub_command() { let args = [ diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index 9110da5de..db3f62368 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -8,6 +8,7 @@ mod era; mod fd_limit; mod keygen; mod network; +mod slashing_protection; mod validator; mod version; @@ -109,6 +110,11 @@ fn main() -> eyre::Result<()> { init_node_logging()?; validator::run(options) } + // Offline file work: no runtime, and stdout left alone. + Command::SlashingProtection(options) => { + init_info_stderr_logging()?; + slashing_protection::run(options) + } // Key generation is synchronous, CPU-bound work whose product is files, // so it runs on this thread too and leaves stdout alone. Command::Keygen(options) => { diff --git a/bin/ethlambda/src/slashing_protection.rs b/bin/ethlambda/src/slashing_protection.rs new file mode 100644 index 000000000..2f3d027a2 --- /dev/null +++ b/bin/ethlambda/src/slashing_protection.rs @@ -0,0 +1,99 @@ +//! `ethlambda slashing-protection`: move a validator client's EIP-3076 history +//! in and out of its `--validators-dir`. +//! +//! Offline on purpose. It takes the same lock the validator client holds while +//! it runs, so it refuses while one is running there: history imported under a +//! running client could arrive after the client had already signed past it. +//! No beacon node is needed either: the record names its own chain, and an +//! import into a directory with no record yet creates one for the chain the +//! interchange names, which the client checks against its beacon node at its +//! next start. + +use std::io::Write as _; +use std::path::PathBuf; + +use ethlambda_validator::beacon_node::dto::parse_pubkey; +use ethlambda_validator::slashing_protection::SlashingProtection; +use tracing::info; + +#[derive(Debug, clap::Args)] +pub(crate) struct SlashingProtectionOptions { + #[command(subcommand)] + pub(crate) action: Action, +} + +#[derive(Debug, clap::Subcommand)] +pub(crate) enum Action { + /// Merge an EIP-3076 interchange file (complete or minimal, version 5) into + /// the record, never lowering what is already there. + Import { + /// The validator client's `--validators-dir`. + #[arg(long)] + validators_dir: PathBuf, + /// The interchange file, as exported by this or another client. + file: PathBuf, + }, + /// Write the record as a minimal EIP-3076 interchange file (version 5). + Export { + /// The validator client's `--validators-dir`. + #[arg(long)] + validators_dir: PathBuf, + /// Only these validators, `0x`-prefixed, comma-separated or repeated. + /// Every validator with a record when omitted. + #[arg(long, value_delimiter = ',')] + pubkeys: Vec, + /// Where to write it. Refused if it exists, rather than overwritten. + file: PathBuf, + }, +} + +pub(crate) fn run(options: SlashingProtectionOptions) -> eyre::Result<()> { + match options.action { + Action::Import { + validators_dir, + file, + } => { + let interchange = std::fs::read_to_string(&file) + .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; + let chain = SlashingProtection::interchange_chain(&interchange)?; + let mut protection = SlashingProtection::open_offline(&validators_dir, Some(chain))?; + let validators = protection.import(&interchange)?; + info!( + validators, + file = %file.display(), + "Imported slashing-protection history" + ); + } + Action::Export { + validators_dir, + pubkeys, + file, + } => { + let protection = SlashingProtection::open_offline(&validators_dir, None)?; + let pubkeys = if pubkeys.is_empty() { + protection.pubkeys() + } else { + pubkeys + .iter() + .map(|text| parse_pubkey(text)) + .collect::, _>>()? + }; + let interchange = protection.export(&pubkeys)?; + let mut output = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&file) + .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; + output + .write_all(interchange.as_bytes()) + .and_then(|()| output.sync_all()) + .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; + info!( + validators = pubkeys.len(), + file = %file.display(), + "Exported slashing-protection history" + ); + } + } + Ok(()) +} diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 26f35aebb..1beee5bdb 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -1,24 +1,24 @@ //! `GET`, `POST` and `DELETE /eth/v1/keystores`. //! -//! # Deviation +//! # Slashing protection //! -//! 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` 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 specification lets these routes carry a validator's EIP-3076 history in +//! and out, and both do: //! -//! 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 (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. +//! - `POST` imports the request's `slashing_protection` before any key, so a +//! key never starts signing ahead of its history. An interchange that does +//! not parse, or is for another chain, fails every key in the request and +//! imports none of them. A key that arrives with no history at all is +//! registered as never having signed, with a warning. +//! - `DELETE` returns the deleted keys' records as a minimal interchange, +//! taken after the keys are out of the store, so it covers every signature +//! this process made with them. A key that is gone but still has a record is +//! reported `not_active`, and its record is kept, so importing it back later +//! stays protected. +//! +//! The history is this client's EIP-3076 minimal watermarks (see +//! [`crate::slashing_protection`]), so an export lists, per validator, its +//! highest slot and its highest source and target rather than every message. use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -37,9 +37,6 @@ use crate::keys::keystore::Keystore; use crate::keys::{ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; use crate::secure_fs; -/// The EIP-3076 interchange version this client reports when it has no history. -const INTERCHANGE_VERSION: &str = "5"; - /// The most keystores accepted in a single import request. /// /// Each one costs a deliberately slow EIP-2335 key derivation (scrypt at the @@ -74,7 +71,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: importing EIP-3076 history is not supported yet. + /// An EIP-3076 interchange, imported before any of the keys. #[serde(default)] pub slashing_protection: Option, } @@ -122,8 +119,8 @@ pub struct DeleteRequest { #[derive(Debug, Serialize)] pub struct DeleteResponse { pub data: Vec, - /// Required by the specification even though this client has no history to - /// report. See the module documentation. + /// The deleted keys' records, as a minimal EIP-3076 interchange. See the + /// module documentation. pub slashing_protection: String, } @@ -146,25 +143,6 @@ 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!( - "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 { let message = format!( "request carries {} keystores, more than the limit of {MAX_KEYSTORES_PER_IMPORT}", @@ -182,6 +160,34 @@ pub async fn import( return Json(StatusResponse { data }); } + // History first, keys second: a key must never be able to sign ahead of + // the history it came with. All or nothing, so a request whose history is + // refused imports no key at all, rather than some keys without it. + if let Some(interchange) = &request.slashing_protection { + let imported = context + .protection + .lock() + .map_err(|_| "slashing protection is poisoned".to_string()) + .and_then(|mut protection| { + protection + .import(interchange) + .map_err(|err| err.to_string()) + }); + match imported { + Ok(validators) => info!(validators, "Imported slashing-protection history"), + Err(err) => { + let message = format!("slashing_protection was not imported: {err}"); + warn!(%err, "Refusing an import whose slashing-protection history was refused"); + let data = request + .keystores + .iter() + .map(|_| KeyStatus::error(&message)) + .collect(); + return Json(StatusResponse { data }); + } + } + } + // Held across this whole function: the entire open-mutate-save cycle // over the definitions file, including the slow decrypt-and-persist loop // below. This is what actually serializes concurrent import/delete @@ -700,29 +706,67 @@ pub async fn delete( let _definitions_guard = context.definitions_lock.lock().await; let mut store = context.store.write().await; - let data = request + let results: Vec<(Option, KeyStatus)> = request .pubkeys .iter() .map(|text| match parse_pubkey(text) { - Ok(pubkey) => delete_one(&context, &mut store, &pubkey), - Err(err) => KeyStatus::error(err), + Ok(pubkey) => (Some(pubkey), delete_one(&context, &mut store, &pubkey)), + Err(err) => (None, KeyStatus::error(err)), }) .collect(); drop(store); - // This interchange cannot be used to migrate the key(s) just deleted to - // another client: see the module doc for why `genesis_validators_root` - // is deliberately wrong rather than merely absent. Logged on every call, - // not just a failure, since a successful response is exactly the one a - // caller might mistake for "safe to import elsewhere". - warn!( - "DELETE /eth/v1/keystores returns an empty EIP-3076 interchange with no real signing \ - history; it cannot be used to migrate these keys to another client without slashing risk" - ); + // Exported only now, with the keys out of the store: a signature with one + // of them either finished before the write lock above was granted, and is + // in the record, or can no longer happen. + let pubkeys: Vec = results.iter().filter_map(|(pubkey, _)| *pubkey).collect(); + let exported = context + .protection + .lock() + .map_err(|_| "slashing protection is poisoned".to_string()) + .and_then(|protection| { + let interchange = protection.export(&pubkeys).map_err(|err| err.to_string())?; + let recorded: Vec = results + .iter() + .map(|(pubkey, _)| pubkey.is_some_and(|pubkey| protection.contains(&pubkey))) + .collect(); + Ok((interchange, recorded)) + }); + let (slashing_protection, recorded) = match exported { + Ok(exported) => exported, + Err(err) => { + // The keys are already deleted, but their history cannot be handed + // over, so no caller may treat any of them as safe to move. + warn!(%err, "Deleted keys whose slashing-protection history could not be exported"); + let message = format!("deleted, but slashing_protection could not be exported: {err}"); + let data = results.iter().map(|_| KeyStatus::error(&message)).collect(); + return Json(DeleteResponse { + data, + slashing_protection: String::new(), + }); + } + }; + + // `not_active`: not held here, but with a record, which the response + // carries. The specification's word for "gone, history attached". + let data = results + .into_iter() + .zip(recorded) + .map(|((_, status), recorded)| { + if status.status == "not_found" && recorded { + KeyStatus { + status: "not_active".to_string(), + message: None, + } + } else { + status + } + }) + .collect(); Json(DeleteResponse { data, - slashing_protection: empty_interchange(), + slashing_protection, }) } @@ -845,23 +889,6 @@ fn persist_delete(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Res save_definitions(context, &definitions) } -/// A well-formed EIP-3076 interchange holding no history. -/// -/// `genesis_validators_root` is deliberately all-zero rather than the -/// chain's real value; see the module doc for why. Do not "fix" this to -/// carry the real root: that would make an empty history look legitimate -/// instead of getting it rejected. -fn empty_interchange() -> String { - serde_json::json!({ - "metadata": { - "interchange_format_version": INTERCHANGE_VERSION, - "genesis_validators_root": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - "data": [] - }) - .to_string() -} - #[cfg(test)] mod tests { use super::*; @@ -1254,9 +1281,14 @@ mod tests { async fn an_imported_key_is_registered_for_slashing_protection() { let dir = tempfile::tempdir().expect("temp dir"); let root = ethlambda_types::beacon::primitives::Root::ZERO; - let protection = - crate::slashing_protection::SlashingProtection::open(dir.path(), root, &[], false) - .expect("opens with no keys"); + let protection = crate::slashing_protection::SlashingProtection::open( + dir.path(), + root, + &[], + false, + None, + ) + .expect("opens with no keys"); let mut context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); context.protection = Arc::new(std::sync::Mutex::new(protection)); @@ -1282,7 +1314,7 @@ mod tests { // registered. drop(protection); let keys = ValidatorStore::load(dir.path()).expect("loads").pubkeys(); - crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false) + crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false, None) .expect("the imported key has a record"); } @@ -1618,9 +1650,14 @@ mod tests { .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 protection = crate::slashing_protection::SlashingProtection::open( + dir.path(), + root, + &[], + false, + None, + ) + .expect("opens with no keys"); let mut context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir.path()); context.protection = Arc::new(std::sync::Mutex::new(protection)); @@ -1642,6 +1679,171 @@ mod tests { ); } + const KEYSTORE_PUBKEY: &str = "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07"; + + /// A context over `dir` whose slashing protection is a real record there, + /// for the chain `0x00..00`, and the handle to inspect it with. + fn context_with_record( + dir: &std::path::Path, + ) -> ( + KeymanagerContext, + crate::slashing_protection::SharedProtection, + ) { + let protection = crate::slashing_protection::SlashingProtection::open( + dir, + ethlambda_types::beacon::primitives::Root::ZERO, + &[], + false, + None, + ) + .expect("opens with no keys"); + let mut context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + let protection = context.protection.clone(); + (context, protection) + } + + /// A minimal interchange for `KEYSTORE_PUBKEY` on the chain `root`. + fn history(root: &str, slot: u64, source: u64, target: u64) -> String { + serde_json::json!({ + "metadata": { "interchange_format_version": "5", "genesis_validators_root": root }, + "data": [{ + "pubkey": KEYSTORE_PUBKEY, + "signed_blocks": [{ "slot": slot.to_string() }], + "signed_attestations": [{ + "source_epoch": source.to_string(), + "target_epoch": target.to_string() + }] + }] + }) + .to_string() + } + + const ZERO_ROOT: &str = "0x0000000000000000000000000000000000000000000000000000000000000000"; + + /// History that arrives with a key is in force before the key can sign. + #[tokio::test] + async fn an_import_brings_its_slashing_protection_history() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, protection) = context_with_record(dir.path()); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + "slashing_protection": history(ZERO_ROOT, 100, 5, 10), + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + + let pubkey = parse_pubkey(KEYSTORE_PUBKEY).expect("pubkey"); + let protection = protection.lock().expect("not poisoned"); + assert!( + protection.check_block(&pubkey, 100).is_err(), + "slot 100 was signed elsewhere" + ); + assert!(protection.check_block(&pubkey, 101).is_ok()); + } + + /// History for another chain is refused, and with it every key in the + /// request: none may start signing without the history it came with. + #[tokio::test] + async fn an_import_whose_history_is_for_another_chain_imports_nothing() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, _) = context_with_record(dir.path()); + let store = context.store.clone(); + let other_chain = "0x0000000000000000000000000000000000000000000000000000000000000001"; + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + "slashing_protection": history(other_chain, 100, 5, 10), + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "error", "got {body}"); + assert!(store.read().await.is_empty(), "the key must not be loaded"); + } + + /// A delete hands over the record the key was protected by, so it can be + /// imported elsewhere, and keeps it here. + #[tokio::test] + async fn a_delete_returns_the_keys_history() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, protection) = context_with_record(dir.path()); + let app = router(context, TOKEN.to_string()); + + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ "keystores": [keystore_json()], "passwords": [PASSWORD] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + let pubkey = parse_pubkey(KEYSTORE_PUBKEY).expect("pubkey"); + protection + .lock() + .expect("not poisoned") + .check_and_record_block(&pubkey, 97) + .expect("written") + .expect("allowed"); + + let (_, body) = send( + app.clone(), + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [KEYSTORE_PUBKEY] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "deleted", "got {body}"); + let interchange: serde_json::Value = + serde_json::from_str(body["slashing_protection"].as_str().expect("a string")) + .expect("json"); + assert_eq!( + interchange["metadata"]["genesis_validators_root"], + ZERO_ROOT + ); + assert_eq!(interchange["data"][0]["pubkey"], KEYSTORE_PUBKEY); + assert_eq!(interchange["data"][0]["signed_blocks"][0]["slot"], "97"); + + // Gone from this client, but its record is still here and still + // returned: the specification's `not_active`. + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [KEYSTORE_PUBKEY] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "not_active", "got {body}"); + assert!( + protection + .lock() + .expect("not poisoned") + .check_block(&pubkey, 97) + .is_err() + ); + } + #[tokio::test] async fn deleting_an_unknown_key_reports_not_found() { let (_, body) = send( @@ -1984,20 +2186,6 @@ mod tests { .to_string() } - /// Finding 6: the interchange's `genesis_validators_root` must stay - /// all-zero. See the module doc: a real root would make an empty history - /// look legitimate, which is worse than the deliberately unusable file - /// this client returns instead. - #[test] - fn the_empty_interchange_root_stays_all_zero() { - let interchange: serde_json::Value = - serde_json::from_str(&empty_interchange()).expect("valid json"); - assert_eq!( - interchange["metadata"]["genesis_validators_root"], - "0x0000000000000000000000000000000000000000000000000000000000000000", - ); - } - /// Finding 3: `require_bearer` must reject a wrong token of a different /// length too, not just one that fails a byte comparison partway /// through. Not a timing assertion (this crate has no timing harness), diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index faf9f35e7..24fc99dfd 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -19,12 +19,10 @@ //! //! [`docs/spec_deviations.md`]: https://github.com/lambdaclass/ethlambda/blob/main/docs/spec_deviations.md //! -//! 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` -//! field on import. +//! History moves in and out as EIP-3076 interchange files: through the +//! keymanager API's `POST` and `DELETE /eth/v1/keystores`, and offline through +//! `ethlambda slashing-protection`. A key imported with no history is +//! registered as never having signed before it is loaded. pub mod aggregation; pub mod aggregation_selection; diff --git a/crates/validator/src/slashing_protection/file.rs b/crates/validator/src/slashing_protection/file.rs index 84e2e9607..865a8237c 100644 --- a/crates/validator/src/slashing_protection/file.rs +++ b/crates/validator/src/slashing_protection/file.rs @@ -95,6 +95,40 @@ struct SignedAttestation { signing_root: Option, } +/// Parse an EIP-3076 version 5 interchange, complete or minimal, into its +/// chain and each entry's watermarks, in file order. +/// +/// A pubkey listed more than once is returned more than once: an import merges +/// them, while the record this client writes must never contain one twice. +/// The error is a reason, for the caller to attach to whatever it was reading. +pub(crate) fn parse(text: &str) -> std::result::Result<(Root, Vec<(BlsPubkey, Record)>), String> { + if text.trim().is_empty() { + return Err("empty; refusing to treat it as \"nothing signed\"".to_string()); + } + let interchange: Interchange = + serde_json::from_str(text).map_err(|err| format!("malformed: {err}"))?; + if interchange.metadata.interchange_format_version != INTERCHANGE_VERSION { + return Err(format!( + "interchange format version {:?}, expected {INTERCHANGE_VERSION:?}", + interchange.metadata.interchange_format_version + )); + } + let mut entries = Vec::with_capacity(interchange.data.len()); + for entry in &interchange.data { + let record = record_of(entry).map_err(|reason| { + format!( + "validator {}: {reason}", + crate::beacon_node::dto::encode_hex(&entry.pubkey) + ) + })?; + entries.push((BlsPubkey(entry.pubkey), record)); + } + Ok(( + Root::from(interchange.metadata.genesis_validators_root), + entries, + )) +} + /// Read and validate the record at `path`. pub(crate) fn read(path: &Path) -> Result<(Root, HashMap)> { let invalid = |reason: String| Error::SlashingProtection { @@ -106,29 +140,10 @@ pub(crate) fn read(path: &Path) -> Result<(Root, HashMap)> { 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 (root, entries) = parse(&text).map_err(invalid)?; - 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) - )) - })?; + let mut records = HashMap::with_capacity(entries.len()); + for (pubkey, record) in entries { if records.insert(pubkey, record).is_some() { return Err(invalid(format!( "validator {} appears more than once", @@ -136,15 +151,17 @@ pub(crate) fn read(path: &Path) -> Result<(Root, HashMap)> { ))); } } - Ok(( - Root::from(interchange.metadata.genesis_validators_root), - records, - )) + Ok((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. +/// +/// An attestation whose source is at or past its target is refused outside +/// genesis, the one shape the importer does not absorb: it is never valid, and +/// read into the record it would refuse every honest attestation after it +/// until the chain justified past that source. fn record_of(entry: &Entry) -> std::result::Result { let block_slot = entry.signed_blocks.iter().map(|block| block.slot).max(); @@ -197,17 +214,17 @@ pub(crate) fn exists(path: &Path) -> Result { } } -/// Durably replace the record at `path` with `records`. +/// `records` as a minimal EIP-3076 interchange for the chain named by +/// `genesis_validators_root`. /// -/// Compact rather than pretty: the file is rewritten once per slot and per +/// Compact rather than pretty: the record 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, +pub(crate) fn render<'a>( genesis_validators_root: Root, records: impl Iterator, -) -> Result<()> { +) -> serde_json::Result { let mut data: Vec = records .map(|(pubkey, record)| Entry { pubkey: pubkey.0, @@ -239,10 +256,20 @@ pub(crate) fn write<'a>( }, 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}"), - })?; + serde_json::to_string(&interchange) +} + +/// Durably replace the record at `path` with `records`. +pub(crate) fn write<'a>( + path: &Path, + genesis_validators_root: Root, + records: impl Iterator, +) -> Result<()> { + let contents = + render(genesis_validators_root, records).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 { diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 55ba67f2e..6af37597f 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -239,6 +239,18 @@ impl Record { } } + /// Both records' maxima: what an import leaves when it meets an existing + /// record. Never less strict than either. + fn merged(self, other: Record) -> Record { + Record { + block_slot: self.block_slot.max(other.block_slot), + attestation: match (self.attestation, other.attestation) { + (Some(ours), Some(theirs)) => Some(ours.max_with(theirs)), + (ours, theirs) => ours.or(theirs), + }, + } + } + fn with_attestation(self, data: &AttestationData) -> Record { let next = Signed { source: data.source.epoch, @@ -446,6 +458,36 @@ impl SlashingProtection { }) } + /// Open the record in `validators_dir` without a beacon node, for the + /// `slashing-protection` command, holding its lock until this is dropped. + /// + /// The chain is whatever the record names. With no record yet, one is + /// created for `create_for` if given, which is how an interchange is + /// imported into a directory before the client first runs there; with + /// neither, this is an error. A record created for the wrong chain is not + /// dangerous: [`Self::open`] refuses it at the client's next start. + pub fn open_offline(validators_dir: &Path, create_for: Option) -> Result { + let lock = file::lock(validators_dir)?; + let path = validators_dir.join(file::FILE_NAME); + let (genesis_validators_root, records) = if file::exists(&path)? { + file::read(&path)? + } else if let Some(root) = create_for { + let records = HashMap::new(); + file::write(&path, root, records.iter())?; + (root, records) + } else { + return Err(Error::SlashingProtection { + path: path.display().to_string(), + reason: "there is no slashing-protection record in this directory".to_string(), + }); + }; + 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. /// @@ -574,6 +616,93 @@ impl SlashingProtection { Ok(count) } + /// The chain an EIP-3076 interchange names, read without importing it: what + /// a record created for it must belong to. + pub fn interchange_chain(interchange: &str) -> Result { + file::parse(interchange) + .map(|(root, _)| root) + .map_err(|reason| Error::SlashingProtection { + path: "interchange".to_string(), + reason, + }) + } + + /// The chain this record belongs to. + pub fn genesis_validators_root(&self) -> Root { + self.genesis_validators_root + } + + /// Whether `pubkey` has a record. + pub fn contains(&self, pubkey: &BlsPubkey) -> bool { + self.records.contains_key(pubkey) + } + + /// Every validator with a record. + pub fn pubkeys(&self) -> Vec { + self.records.keys().copied().collect() + } + + /// Merge an EIP-3076 version 5 interchange, complete or minimal, into the + /// record, in one durable write. + /// + /// Each validator in it is reduced to its highest slot, source and target + /// and merged with any existing record by taking the maximum of each, so an + /// import never lowers a watermark, and slashable data inside it (two + /// blocks for one slot, a surround, a source after its target) is absorbed + /// rather than refused: the result is at least as strict as every + /// message the file lists. A validator it names that has no record yet is + /// registered with the imported watermarks. + /// + /// All or nothing: an interchange for another chain or of another version, + /// or one that does not parse, changes nothing. Returns how many distinct + /// validators it named. + pub fn import(&mut self, interchange: &str) -> Result { + let refused = |reason: String| Error::SlashingProtection { + path: "interchange".to_string(), + reason, + }; + let (root, entries) = file::parse(interchange).map_err(refused)?; + if root != self.genesis_validators_root { + return Err(refused(format!( + "it is for genesis_validators_root {}, but this record is for {}", + encode_hex(&root.0), + encode_hex(&self.genesis_validators_root.0) + ))); + } + + let mut staged: HashMap = HashMap::new(); + for (pubkey, imported) in entries { + let current = staged + .get(&pubkey) + .or_else(|| self.records.get(&pubkey)) + .copied() + .unwrap_or_default(); + staged.insert(pubkey, current.merged(imported)); + } + let count = staged.len(); + if count > 0 { + self.commit(staged)?; + } + Ok(count) + } + + /// The records of `pubkeys` as a minimal EIP-3076 version 5 interchange. + /// + /// A key with no record is left out rather than listed as never having + /// signed: an empty entry would tell the importer something this client + /// does not know. + pub fn export(&self, pubkeys: &[BlsPubkey]) -> Result { + let selected = pubkeys + .iter() + .filter_map(|pubkey| self.records.get_key_value(pubkey)); + file::render(self.genesis_validators_root, selected).map_err(|err| { + Error::SlashingProtection { + path: "interchange".to_string(), + reason: format!("could not encode the export: {err}"), + } + }) + } + /// 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 @@ -1081,6 +1210,90 @@ mod tests { ); } + /// A minimal interchange for `key` on `root`. + fn interchange(root_byte: u8, key: u8, slot: Slot, source: Epoch, target: Epoch) -> String { + format!( + r#"{{"metadata":{{"interchange_format_version":"5","genesis_validators_root":"{}"}},"data":[{{"pubkey":"{}","signed_blocks":[{{"slot":"{slot}"}}],"signed_attestations":[{{"source_epoch":"{source}","target_epoch":"{target}"}}]}}]}}"#, + encode_hex(&root(root_byte).0), + encode_hex(&pubkey(key).0) + ) + } + + #[test] + fn an_import_raises_the_watermarks_and_survives_a_restart() { + let dir = tempfile::tempdir().expect("temp dir"); + { + let mut protection = open(dir.path(), &[], false).expect("opens"); + assert_eq!( + protection + .import(&interchange(0x42, 1, 97, 2, 3)) + .expect("imports"), + 1 + ); + } + let mut protection = open(dir.path(), &[1], false).expect("the imported key has a record"); + assert!(propose(&mut protection, 1, 97).is_err()); + assert!(attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).is_err()); + assert!(attest(&mut protection, 1, &data(128, 3, 4, 0xaa)).is_ok()); + } + + /// An older history imported over a newer record must not lower it. + #[test] + fn an_import_never_lowers_a_watermark() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1], true).expect("init"); + propose(&mut protection, 1, 200).expect("propose"); + attest(&mut protection, 1, &data(320, 9, 10, 0xaa)).expect("attest"); + + protection + .import(&interchange(0x42, 1, 97, 2, 3)) + .expect("imports"); + assert!(propose(&mut protection, 1, 150).is_err()); + assert!(attest(&mut protection, 1, &data(160, 4, 5, 0xaa)).is_err()); + } + + #[test] + fn an_import_for_another_chain_changes_nothing() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[], false).expect("opens"); + let err = protection + .import(&interchange(0x99, 1, 97, 2, 3)) + .expect_err("another chain"); + assert!(err.to_string().contains("genesis_validators_root"), "{err}"); + assert!(protection.is_empty()); + } + + #[test] + fn an_export_lists_only_keys_with_a_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 exported = protection.export(&[pubkey(1), pubkey(2)]).expect("exports"); + let json: serde_json::Value = serde_json::from_str(&exported).expect("json"); + assert_eq!(json["data"].as_array().expect("array").len(), 1); + assert_eq!(json["data"][0]["signed_blocks"][0]["slot"], "97"); + + // And it imports back into a fresh record as the same watermarks. + let other = tempfile::tempdir().expect("temp dir"); + let mut fresh = open(other.path(), &[], false).expect("opens"); + fresh.import(&exported).expect("round trip"); + assert!(propose(&mut fresh, 1, 97).is_err()); + } + + #[test] + fn opening_offline_needs_a_record_or_a_chain_to_create_one_for() { + let dir = tempfile::tempdir().expect("temp dir"); + SlashingProtection::open_offline(dir.path(), None).expect_err("nothing to open"); + let created = + SlashingProtection::open_offline(dir.path(), Some(root(0x42))).expect("creates"); + assert_eq!(created.genesis_validators_root(), root(0x42)); + drop(created); + let reopened = SlashingProtection::open_offline(dir.path(), Some(root(0x99))) + .expect("an existing record keeps its own chain"); + assert_eq!(reopened.genesis_validators_root(), root(0x42)); + } + #[test] fn an_unregistered_key_is_refused_on_a_persisted_record() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs new file mode 100644 index 000000000..bac90070d --- /dev/null +++ b/crates/validator/tests/interchange_vectors.rs @@ -0,0 +1,210 @@ +//! EIP-3076's shared interchange test vectors +//! (`eth-clients/slashing-protection-interchange-tests`), run against +//! `SlashingProtection`. +//! +//! Each case starts from an empty record, then for every step imports the +//! step's interchange and attempts its blocks and attestations, each of which +//! must be allowed or refused as the case says. This client keeps EIP-3076's +//! minimal watermarks, so the expectation read is `should_succeed`, the +//! minimal strategy's, rather than `should_succeed_complete`. +//! +//! The vectors are a download: `make slashing-protection-interchange-tests`, +//! which `make test` runs first. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use ethlambda_types::beacon::containers::shared::{AttestationData, Checkpoint}; +use ethlambda_types::beacon::primitives::{BlsPubkey, H256, Root}; +use ethlambda_validator::beacon_node::dto::parse_pubkey; +use ethlambda_validator::slashing_protection::SlashingProtection; +use serde::Deserialize; + +#[derive(Deserialize)] +struct Case { + name: String, + genesis_validators_root: String, + steps: Vec, +} + +#[derive(Deserialize)] +struct Step { + should_succeed: bool, + contains_slashable_data: bool, + interchange: serde_json::Value, + blocks: Vec, + attestations: Vec, +} + +#[derive(Deserialize)] +struct Block { + pubkey: String, + slot: String, + should_succeed: bool, +} + +#[derive(Deserialize)] +struct Attestation { + pubkey: String, + source_epoch: String, + target_epoch: String, + should_succeed: bool, +} + +fn vectors_dir() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../slashing-protection-interchange-tests/tests/generated") +} + +#[test] +fn eip_3076_interchange_vectors() { + let dir = vectors_dir(); + assert!( + dir.is_dir(), + "{} is missing: run `make slashing-protection-interchange-tests`", + dir.display() + ); + + let mut paths: Vec = std::fs::read_dir(&dir) + .expect("reads the vectors directory") + .map(|entry| entry.expect("reads an entry").path()) + .filter(|path| { + path.extension() + .is_some_and(|extension| extension == "json") + }) + .collect(); + paths.sort(); + assert!(!paths.is_empty(), "no vectors in {}", dir.display()); + + let failures: Vec = paths + .iter() + .filter_map(|path| { + let text = std::fs::read_to_string(path).expect("reads a vector"); + let case: Case = serde_json::from_str(&text).expect("parses a vector"); + run(&case) + .err() + .map(|reason| format!("{}: {reason}", case.name)) + }) + .collect(); + assert!( + failures.is_empty(), + "{} of {} vectors failed:\n{}", + failures.len(), + paths.len(), + failures.join("\n") + ); +} + +fn run(case: &Case) -> Result<(), String> { + let root = parse_root(&case.genesis_validators_root)?; + + // The vectors start from an empty database in which any validator may + // sign, so every key a case signs for is registered as never having signed. + let mut keys: Vec = Vec::new(); + let mut seen = HashSet::new(); + for step in &case.steps { + let texts = step.blocks.iter().map(|block| &block.pubkey).chain( + step.attestations + .iter() + .map(|attestation| &attestation.pubkey), + ); + for text in texts { + let pubkey = parse_pubkey(text).map_err(|err| err.to_string())?; + if seen.insert(pubkey.0) { + keys.push(pubkey); + } + } + } + + let dir = tempfile::tempdir().map_err(|err| err.to_string())?; + let mut protection = + SlashingProtection::open(dir.path(), root, &keys, true).map_err(|err| err.to_string())?; + + for (index, step) in case.steps.iter().enumerate() { + let imported = protection.import(&step.interchange.to_string()); + match (imported.is_ok(), step.should_succeed) { + (true, true) | (false, false) => {} + // The README lets an importer refuse an interchange holding + // slashable data, in which case the rest of the case is skipped. + // This one absorbs it instead, so reaching here is still a + // failure: it would mean an import was refused for some other + // reason. + (false, true) => { + return Err(format!( + "step {index}: import failed but should succeed (slashable data: {}): {}", + step.contains_slashable_data, + imported + .err() + .map(|err| err.to_string()) + .unwrap_or_default() + )); + } + (true, false) => { + return Err(format!("step {index}: import succeeded but should fail")); + } + } + + for (position, block) in step.blocks.iter().enumerate() { + let pubkey = parse_pubkey(&block.pubkey).map_err(|err| err.to_string())?; + let slot = parse_u64(&block.slot)?; + let allowed = protection + .check_and_record_block(&pubkey, slot) + .map_err(|err| err.to_string())? + .is_ok(); + if allowed != block.should_succeed { + return Err(format!( + "step {index}, block {position} at slot {slot}: allowed {allowed}, \ + expected {}", + block.should_succeed + )); + } + } + + for (position, attestation) in step.attestations.iter().enumerate() { + let pubkey = parse_pubkey(&attestation.pubkey).map_err(|err| err.to_string())?; + let source = parse_u64(&attestation.source_epoch)?; + let target = parse_u64(&attestation.target_epoch)?; + let verdicts = protection + .check_and_record_attestations(&[pubkey], &data(source, target)) + .map_err(|err| err.to_string())?; + let allowed = verdicts.first().is_some_and(|verdict| verdict.is_ok()); + if allowed != attestation.should_succeed { + return Err(format!( + "step {index}, attestation {position} ({source} -> {target}): allowed \ + {allowed}, expected {}", + attestation.should_succeed + )); + } + } + } + Ok(()) +} + +/// Only the epochs are judged; the rest of the data is filler. +fn data(source: u64, target: u64) -> AttestationData { + AttestationData { + slot: target * 32, + index: 0, + beacon_block_root: Root::ZERO, + source: Checkpoint { + epoch: source, + root: Root::ZERO, + }, + target: Checkpoint { + epoch: target, + root: Root::ZERO, + }, + } +} + +fn parse_u64(text: &str) -> Result { + text.parse().map_err(|err| format!("{text:?}: {err}")) +} + +fn parse_root(text: &str) -> Result { + let bytes = hex::decode(text.trim_start_matches("0x")).map_err(|err| err.to_string())?; + let bytes: [u8; 32] = bytes + .try_into() + .map_err(|_| format!("{text} is not 32 bytes"))?; + Ok(H256(bytes)) +} diff --git a/docs/cli.md b/docs/cli.md index 4455072c4..7f505e1d6 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -276,8 +276,9 @@ implementation's, so none of the [common flags](#common-flags) apply to it. > 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 +> `--init-slashing-protection` is passed. A key moving here from another client +> brings its history with [`slashing-protection import`](#slashing-protection) +> or the keymanager API. See > [Spec Deviations](./spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). | Flag | Default | Meaning | @@ -433,6 +434,12 @@ specification requires; `POST graffiti` refuses text over 32 bytes rather than truncating it; and every route answers `404` for a key this client does not hold. +The keystore routes carry slashing-protection history. `POST` imports the +request's `slashing_protection` before any key, and refuses every key in the +request if that interchange does not parse or is for another chain. `DELETE` +returns the deleted keys' records as a minimal EIP-3076 interchange, answering +`not_active` for a key that is gone but still has a record. + The specification requires TLS. This binds to `--http-address`, loopback by default; exposing it further means putting a TLS terminator in front. @@ -444,6 +451,27 @@ beacon chain. Every series is registered at startup so it reads zero rather than being absent before its first event, which is what lets an alert fire on "attestations stopped". See [Metrics](./metrics.md#validator-client). +## `slashing-protection` + +Moves the validator client's EIP-3076 history in and out of its +`--validators-dir`, offline. It takes the lock the client holds while running, +so it refuses while a client runs on that directory, and it needs no beacon +node: the record names its own chain. + +```sh +# Bring a key's history from another client before its first start here. +ethlambda slashing-protection import --validators-dir ./validators interchange.json + +# Hand the history over before running the keys elsewhere. +ethlambda slashing-protection export --validators-dir ./validators out.json +ethlambda slashing-protection export --validators-dir ./validators --pubkeys 0xab..,0xcd.. out.json +``` + +| Sub-command | Does | +|---|---| +| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, one is created for the interchange's chain; the client refuses it at its next start if that is not its beacon node's chain | +| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange. Refuses to overwrite an existing file | + ## `benchmark import` flags Neither sub-command follows a chain; see [`benchmarking.md`](./benchmarking.md#import-workload) diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 0e96cf6f4..9b1ab4704 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -114,15 +114,14 @@ anything that does not advance past them. 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`. +- **Interchange:** an import (`ethlambda slashing-protection import`, or the + keymanager API's `POST /eth/v1/keystores`) reduces each validator to its + highest slot, source and target and merges them with the record by maximum, + so slashable data in the file is absorbed rather than refused, as Lighthouse + and Nimbus do. An export (`ethlambda slashing-protection export`, or `DELETE + /eth/v1/keystores`) is the record itself, in the minimal format. Both pass + the shared `eth-clients/slashing-protection-interchange-tests` v5.3.0 vectors + under their `should_succeed` (minimal) expectations. ## Builder bids: gossip and API only (no builder API) From 76d1eabdc9523e0ac17bdca0e9c1b27ea7753c9f Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 29 Oct 2026 19:44:27 -0300 Subject: [PATCH 02/12] Address review of the slashing-protection interchange. The keymanager's POST /eth/v1/keystores now only judges the request's slashing_protection up front (check_import: it parses and is for this record's chain), and writes it after the keys are saved, in the same critical section as their registration and under the definitions lock, so a key that fails to import leaves no history behind and a concurrent delete cannot interleave. When DELETE cannot export the history, only the keys it actually deleted become errors; not_found and malformed pubkeys keep their statuses. The export runs off the executor like every other access to the record. The interchange vectors and the delete test follow the rebase onto the seeded registration. --- crates/validator/src/http_api/keystores.rs | 143 ++++++++++++------ .../validator/src/slashing_protection/mod.rs | 17 +++ crates/validator/tests/interchange_vectors.rs | 7 +- docs/cli.md | 7 +- 4 files changed, 120 insertions(+), 54 deletions(-) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 1beee5bdb..7d85c544d 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -5,11 +5,13 @@ //! The specification lets these routes carry a validator's EIP-3076 history in //! and out, and both do: //! -//! - `POST` imports the request's `slashing_protection` before any key, so a -//! key never starts signing ahead of its history. An interchange that does -//! not parse, or is for another chain, fails every key in the request and -//! imports none of them. A key that arrives with no history at all is -//! registered as never having signed, with a warning. +//! - `POST` judges the request's `slashing_protection` before touching +//! anything: one that does not parse, or is for another chain, fails every +//! key in the request and imports none of them. It is written once the keys +//! are saved, together with their registration and before any is loaded, so +//! a key never starts signing ahead of its history and a key that failed to +//! import leaves none behind. A key with no history at all is registered as +//! of the current slot and epoch, with a warning. //! - `DELETE` returns the deleted keys' records as a minimal interchange, //! taken after the keys are out of the store, so it covers every signature //! this process made with them. A key that is gone but still has a record is @@ -160,31 +162,26 @@ pub async fn import( return Json(StatusResponse { data }); } - // History first, keys second: a key must never be able to sign ahead of - // the history it came with. All or nothing, so a request whose history is - // refused imports no key at all, rather than some keys without it. + // The history is judged before anything is touched, so a request whose + // history would be refused imports no key at all, rather than some keys + // without it. It is written later, together with the keys' registration, + // once the keys it came with are known to be saved: written now, a key + // that then failed to import would leave history behind for nothing. if let Some(interchange) = &request.slashing_protection { - let imported = context - .protection - .lock() - .map_err(|_| "slashing protection is poisoned".to_string()) - .and_then(|mut protection| { - protection - .import(interchange) - .map_err(|err| err.to_string()) - }); - match imported { - Ok(validators) => info!(validators, "Imported slashing-protection history"), - Err(err) => { - let message = format!("slashing_protection was not imported: {err}"); - warn!(%err, "Refusing an import whose slashing-protection history was refused"); - let data = request - .keystores - .iter() - .map(|_| KeyStatus::error(&message)) - .collect(); - return Json(StatusResponse { data }); - } + let interchange = interchange.clone(); + let judged = crate::slashing_protection::blocking(&context.protection, move |protection| { + protection.check_import(&interchange) + }) + .await; + if let Err(err) = judged { + let message = format!("slashing_protection was not imported: {err}"); + warn!(%err, "Refusing an import whose slashing-protection history was refused"); + let data = request + .keystores + .iter() + .map(|_| KeyStatus::error(&message)) + .collect(); + return Json(StatusResponse { data }); } } @@ -257,11 +254,19 @@ pub async fn import( .iter() .filter_map(|result| result.as_ref().ok().map(|(pubkey, _, _)| *pubkey)) .collect(); + // The request's history goes in first, in the same critical section, so + // a key it covers is registered with that history rather than as never + // having signed. + let history = request.slashing_protection.clone(); let registration = if persisted.is_empty() { Ok(0) } else { let registering = persisted.clone(); crate::slashing_protection::blocking(&context.protection, move |protection| { + if let Some(history) = &history { + let validators = protection.import(history)?; + info!(validators, "Imported slashing-protection history"); + } protection.register(®istering) }) .await @@ -719,27 +724,33 @@ pub async fn delete( // Exported only now, with the keys out of the store: a signature with one // of them either finished before the write lock above was granted, and is // in the record, or can no longer happen. - let pubkeys: Vec = results.iter().filter_map(|(pubkey, _)| *pubkey).collect(); - let exported = context - .protection - .lock() - .map_err(|_| "slashing protection is poisoned".to_string()) - .and_then(|protection| { - let interchange = protection.export(&pubkeys).map_err(|err| err.to_string())?; - let recorded: Vec = results - .iter() - .map(|(pubkey, _)| pubkey.is_some_and(|pubkey| protection.contains(&pubkey))) - .collect(); - Ok((interchange, recorded)) - }); + let requested: Vec> = results.iter().map(|(pubkey, _)| *pubkey).collect(); + let exported = crate::slashing_protection::blocking(&context.protection, move |protection| { + let pubkeys: Vec = requested.iter().flatten().copied().collect(); + let interchange = protection.export(&pubkeys)?; + let recorded: Vec = requested + .iter() + .map(|pubkey| pubkey.is_some_and(|pubkey| protection.contains(&pubkey))) + .collect(); + Ok((interchange, recorded)) + }) + .await; let (slashing_protection, recorded) = match exported { Ok(exported) => exported, Err(err) => { - // The keys are already deleted, but their history cannot be handed - // over, so no caller may treat any of them as safe to move. + // A key that was deleted, or is gone with a record, cannot have its + // history handed over, so no caller may treat it as safe to move: + // it becomes an error. Every other status (`not_found`, a malformed + // pubkey) was decided before the export and still stands. warn!(%err, "Deleted keys whose slashing-protection history could not be exported"); let message = format!("deleted, but slashing_protection could not be exported: {err}"); - let data = results.iter().map(|_| KeyStatus::error(&message)).collect(); + let data = results + .into_iter() + .map(|(_, status)| match status.status.as_str() { + "deleted" => KeyStatus::error(&message), + _ => status, + }) + .collect(); return Json(DeleteResponse { data, slashing_protection: String::new(), @@ -1795,11 +1806,14 @@ mod tests { ) .await; assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + // Imported with no history, so the key starts at the current slot; + // a proposal must come after it. + const SLOT: u64 = 1 << 40; let pubkey = parse_pubkey(KEYSTORE_PUBKEY).expect("pubkey"); protection .lock() .expect("not poisoned") - .check_and_record_block(&pubkey, 97) + .check_and_record_block(&pubkey, SLOT) .expect("written") .expect("allowed"); @@ -1821,7 +1835,10 @@ mod tests { ZERO_ROOT ); assert_eq!(interchange["data"][0]["pubkey"], KEYSTORE_PUBKEY); - assert_eq!(interchange["data"][0]["signed_blocks"][0]["slot"], "97"); + assert_eq!( + interchange["data"][0]["signed_blocks"][0]["slot"], + SLOT.to_string() + ); // Gone from this client, but its record is still here and still // returned: the specification's `not_active`. @@ -1839,11 +1856,41 @@ mod tests { protection .lock() .expect("not poisoned") - .check_block(&pubkey, 97) + .check_block(&pubkey, SLOT) .is_err() ); } + /// History that came with a key that then failed to import is not + /// written: nothing it describes is held here. + #[cfg(unix)] + #[tokio::test] + async fn history_whose_key_failed_to_import_is_not_written() { + 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"); + std::os::unix::fs::symlink(&target, dir.path().join(format!("{KEYSTORE_PUBKEY}.json"))) + .expect("symlinks"); + let (context, protection) = context_with_record(dir.path()); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + "slashing_protection": history(ZERO_ROOT, 100, 5, 10), + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "error", "got {body}"); + assert!(protection.lock().expect("not poisoned").is_empty()); + } + #[tokio::test] async fn deleting_an_unknown_key_reports_not_found() { let (_, body) = send( diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 6af37597f..af778cdcb 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -686,6 +686,23 @@ impl SlashingProtection { Ok(count) } + /// Whether [`Self::import`] would accept `interchange`, changing nothing: + /// it parses, is of the right version, and is for this record's chain. + pub fn check_import(&self, interchange: &str) -> Result<()> { + let chain = Self::interchange_chain(interchange)?; + if chain != self.genesis_validators_root { + return Err(Error::SlashingProtection { + path: "interchange".to_string(), + reason: format!( + "it is for genesis_validators_root {}, but this record is for {}", + encode_hex(&chain.0), + encode_hex(&self.genesis_validators_root.0) + ), + }); + } + Ok(()) + } + /// The records of `pubkeys` as a minimal EIP-3076 version 5 interchange. /// /// A key with no record is left out rather than listed as never having diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs index bac90070d..828f08c6c 100644 --- a/crates/validator/tests/interchange_vectors.rs +++ b/crates/validator/tests/interchange_vectors.rs @@ -99,7 +99,8 @@ fn run(case: &Case) -> Result<(), String> { let root = parse_root(&case.genesis_validators_root)?; // The vectors start from an empty database in which any validator may - // sign, so every key a case signs for is registered as never having signed. + // sign, so every key a case signs for is registered as never having signed: + // with no current slot, as before genesis. let mut keys: Vec = Vec::new(); let mut seen = HashSet::new(); for step in &case.steps { @@ -117,8 +118,8 @@ fn run(case: &Case) -> Result<(), String> { } let dir = tempfile::tempdir().map_err(|err| err.to_string())?; - let mut protection = - SlashingProtection::open(dir.path(), root, &keys, true).map_err(|err| err.to_string())?; + let mut protection = SlashingProtection::open(dir.path(), root, &keys, true, None) + .map_err(|err| err.to_string())?; for (index, step) in case.steps.iter().enumerate() { let imported = protection.import(&step.interchange.to_string()); diff --git a/docs/cli.md b/docs/cli.md index 7f505e1d6..71c59946d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -434,9 +434,10 @@ specification requires; `POST graffiti` refuses text over 32 bytes rather than truncating it; and every route answers `404` for a key this client does not hold. -The keystore routes carry slashing-protection history. `POST` imports the -request's `slashing_protection` before any key, and refuses every key in the -request if that interchange does not parse or is for another chain. `DELETE` +The keystore routes carry slashing-protection history. `POST` refuses every +key in the request if its `slashing_protection` does not parse or is for +another chain, and otherwise writes that history once the keys are saved and +before any is loaded. `DELETE` returns the deleted keys' records as a minimal EIP-3076 interchange, answering `not_active` for a key that is gone but still has a record. From 6e1d01102e93ffff8ff0c1a322cf052d9d605f1e Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 30 Oct 2026 14:57:56 -0300 Subject: [PATCH 03/12] Apply the bot reviews of the slashing-protection interchange. The export command writes its file readable by its owner only and removes it if the write fails partway, so a retry is not refused for a file this run never finished; it warns for every requested validator that has no record and logs how many it actually wrote, and a malformed --pubkeys value is reported with the value. open_offline no longer writes an empty record at open: the first change writes it, so an import that fails, or imports nothing, leaves the directory as it found it. Two tests pin that an import is idempotent and that an empty interchange is refused. The docs say that a POST's history may name validators without a keystore in the request, and the README and CLI reference say the keymanager carries history in the same request as the key. --- README.md | 4 +- bin/ethlambda/src/slashing_protection.rs | 55 +++++++++++++++---- crates/validator/src/http_api/keystores.rs | 4 +- .../validator/src/slashing_protection/mod.rs | 55 ++++++++++++++++--- docs/cli.md | 11 ++-- 5 files changed, 100 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 2763cd373..dc913cf4d 100644 --- a/README.md +++ b/README.md @@ -349,8 +349,8 @@ the standard Beacon API, so it works against `ethlambda beacon` (run with the > even then it rules out a double vote, not a surround of an older vote, which > only the key's own history can. > A key moving from another client brings its history with -> `ethlambda slashing-protection import` (or the keymanager API), and never runs -> in two clients at once. See +> `ethlambda slashing-protection import`, or in the same keymanager API request +> that imports the key, and never runs in two clients at once. See > [Spec deviations](./docs/spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). ```sh diff --git a/bin/ethlambda/src/slashing_protection.rs b/bin/ethlambda/src/slashing_protection.rs index 2f3d027a2..d855193d4 100644 --- a/bin/ethlambda/src/slashing_protection.rs +++ b/bin/ethlambda/src/slashing_protection.rs @@ -12,9 +12,9 @@ use std::io::Write as _; use std::path::PathBuf; -use ethlambda_validator::beacon_node::dto::parse_pubkey; +use ethlambda_validator::beacon_node::dto::{encode_hex, parse_pubkey}; use ethlambda_validator::slashing_protection::SlashingProtection; -use tracing::info; +use tracing::{info, warn}; #[derive(Debug, clap::Args)] pub(crate) struct SlashingProtectionOptions { @@ -75,21 +75,30 @@ pub(crate) fn run(options: SlashingProtectionOptions) -> eyre::Result<()> { } else { pubkeys .iter() - .map(|text| parse_pubkey(text)) + .map(|text| { + parse_pubkey(text).map_err(|err| eyre::eyre!("--pubkeys {text}: {err}")) + }) .collect::, _>>()? }; + // A requested key with no record is left out of the file rather + // than listed as never having signed, so say so: an operator + // moving that key would otherwise find out at the other end. + let mut exported = 0; + for pubkey in &pubkeys { + if protection.contains(pubkey) { + exported += 1; + } else { + warn!( + pubkey = %encode_hex(&pubkey.0), + "No slashing-protection record for this validator; it is not in the export" + ); + } + } let interchange = protection.export(&pubkeys)?; - let mut output = std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&file) - .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; - output - .write_all(interchange.as_bytes()) - .and_then(|()| output.sync_all()) + write_export(&file, &interchange) .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; info!( - validators = pubkeys.len(), + validators = exported, file = %file.display(), "Exported slashing-protection history" ); @@ -97,3 +106,25 @@ pub(crate) fn run(options: SlashingProtectionOptions) -> eyre::Result<()> { } Ok(()) } + +/// Write `interchange` to a new file at `path`, refusing an existing one. +/// +/// Private to the owner, like the record it was taken from. A write that +/// fails partway removes what it left, so the retry is not refused for a file +/// this run created and never finished. +fn write_export(path: &std::path::Path, interchange: &str) -> std::io::Result<()> { + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + let mut output = options.open(path)?; + output + .write_all(interchange.as_bytes()) + .and_then(|()| output.sync_all()) + .inspect_err(|_| { + let _ = std::fs::remove_file(path); + }) +} diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 7d85c544d..d6620dffa 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -11,7 +11,9 @@ //! are saved, together with their registration and before any is loaded, so //! a key never starts signing ahead of its history and a key that failed to //! import leaves none behind. A key with no history at all is registered as -//! of the current slot and epoch, with a warning. +//! of the current slot and epoch, with a warning. The history may also name +//! validators that are not in the request: they get records too, and a +//! later `DELETE` of one answers `not_active`. //! - `DELETE` returns the deleted keys' records as a minimal interchange, //! taken after the keys are out of the store, so it covers every signature //! this process made with them. A key that is gone but still has a record is diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index af778cdcb..d588e5527 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -352,7 +352,8 @@ pub struct SlashingProtection { impl SlashingProtection { /// Open the record in `validators_dir` for the chain named by - /// `genesis_validators_root`, and hold its lock until this is dropped. + /// `genesis_validators_root`, and hold its lock until the returned record 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 @@ -459,22 +460,23 @@ impl SlashingProtection { } /// Open the record in `validators_dir` without a beacon node, for the - /// `slashing-protection` command, holding its lock until this is dropped. + /// `slashing-protection` command, holding its lock until the returned record + /// is dropped. /// /// The chain is whatever the record names. With no record yet, one is - /// created for `create_for` if given, which is how an interchange is + /// started for `create_for` if given, which is how an interchange is /// imported into a directory before the client first runs there; with - /// neither, this is an error. A record created for the wrong chain is not - /// dangerous: [`Self::open`] refuses it at the client's next start. + /// neither, this is an error. The file itself is only written by the + /// first change, so an import that fails, or imports nothing, leaves the + /// directory as it found it. A record created for the wrong chain is not + /// dangerous either: [`Self::open`] refuses it at the client's next start. pub fn open_offline(validators_dir: &Path, create_for: Option) -> Result { let lock = file::lock(validators_dir)?; let path = validators_dir.join(file::FILE_NAME); let (genesis_validators_root, records) = if file::exists(&path)? { file::read(&path)? } else if let Some(root) = create_for { - let records = HashMap::new(); - file::write(&path, root, records.iter())?; - (root, records) + (root, HashMap::new()) } else { return Err(Error::SlashingProtection { path: path.display().to_string(), @@ -1301,16 +1303,51 @@ mod tests { #[test] fn opening_offline_needs_a_record_or_a_chain_to_create_one_for() { let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join(file::FILE_NAME); SlashingProtection::open_offline(dir.path(), None).expect_err("nothing to open"); - let created = + let mut created = SlashingProtection::open_offline(dir.path(), Some(root(0x42))).expect("creates"); assert_eq!(created.genesis_validators_root(), root(0x42)); + // Nothing is on disk until something is recorded, so an import that + // fails leaves no record behind for a chain nothing then runs on. + assert!(!path.exists(), "the record is not written at open"); + created + .import(&interchange(0x42, 1, 97, 2, 3)) + .expect("imports"); + assert!(path.exists(), "the first change writes the record"); drop(created); let reopened = SlashingProtection::open_offline(dir.path(), Some(root(0x99))) .expect("an existing record keeps its own chain"); assert_eq!(reopened.genesis_validators_root(), root(0x42)); } + /// Importing a history twice is the same as importing it once: the merge + /// takes maxima, so the second pass changes nothing. + #[test] + fn an_import_is_idempotent() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[], false).expect("opens"); + let history = interchange(0x42, 1, 97, 2, 3); + protection.import(&history).expect("imports"); + let once = protection.export(&[pubkey(1)]).expect("exports"); + assert_eq!(protection.import(&history).expect("imports again"), 1); + let twice = protection.export(&[pubkey(1)]).expect("exports"); + assert_eq!(once, twice); + assert!(propose(&mut protection, 1, 97).is_err()); + assert!(propose(&mut protection, 1, 98).is_ok()); + } + + #[test] + fn an_empty_interchange_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[], false).expect("opens"); + for text in ["", "{}", "[]"] { + protection.import(text).expect_err("not an interchange"); + SlashingProtection::interchange_chain(text).expect_err("not an interchange"); + } + assert!(protection.is_empty()); + } + #[test] fn an_unregistered_key_is_refused_on_a_persisted_record() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/docs/cli.md b/docs/cli.md index 71c59946d..4650a5aba 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -277,8 +277,8 @@ implementation's, so none of the [common flags](#common-flags) apply to it. > 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. A key moving here from another client -> brings its history with [`slashing-protection import`](#slashing-protection) -> or the keymanager API. See +> brings its history with [`slashing-protection import`](#slashing-protection), +> or in the same keymanager API request that imports the key. See > [Spec Deviations](./spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history). | Flag | Default | Meaning | @@ -437,7 +437,8 @@ hold. The keystore routes carry slashing-protection history. `POST` refuses every key in the request if its `slashing_protection` does not parse or is for another chain, and otherwise writes that history once the keys are saved and -before any is loaded. `DELETE` +before any is loaded; a validator the history names without a keystore in the +request gets a record too. `DELETE` returns the deleted keys' records as a minimal EIP-3076 interchange, answering `not_active` for a key that is gone but still has a record. @@ -470,8 +471,8 @@ ethlambda slashing-protection export --validators-dir ./validators --pubkeys 0xa | Sub-command | Does | |---|---| -| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, one is created for the interchange's chain; the client refuses it at its next start if that is not its beacon node's chain | -| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange. Refuses to overwrite an existing file | +| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing; the client refuses the record at its next start if that is not its beacon node's chain | +| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record is left out, with a warning. Refuses to overwrite an existing file | ## `benchmark import` flags From 95366f424c257d959ab4672ca07d424bfa2f9e1e Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 30 Oct 2026 15:08:05 -0300 Subject: [PATCH 04/12] Name the slashing-protection sub-command where the others are listed, in the CLI reference's chain-less sub-command paragraph and the README's pointer to it, and say on the keymanager's slashing_protection field that the history is written once the keys are saved and before any is loaded, which is what the module documentation and the handler already say. --- README.md | 2 +- crates/validator/src/http_api/keystores.rs | 3 ++- docs/cli.md | 6 ++++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index dc913cf4d..bbe23f7cc 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,7 @@ cargo build --release ``` [`docs/cli.md`](./docs/cli.md) is the full flag reference for every sub-command, -`benchmark` and `replay-era` included. The +`benchmark`, `replay-era` and `slashing-protection` included. The [quickstart](#beacon-chain-follower) runs a release binary with the same arguments. #### `ethlambda node` — the Lean consensus chain diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index d6620dffa..cd7a20f27 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -75,7 +75,8 @@ 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>, - /// An EIP-3076 interchange, imported before any of the keys. + /// An EIP-3076 interchange, written once the keys are saved and before + /// any of them is loaded; see the module documentation. #[serde(default)] pub slashing_protection: Option, } diff --git a/docs/cli.md b/docs/cli.md index 4650a5aba..c0aa37566 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -8,7 +8,7 @@ | `ethlambda beacon ` | The Ethereum Beacon Chain, as a gossip follower | | `ethlambda ` | `node`: the subcommand is injected | -Three subcommands follow no chain. `ethlambda replay-era` replays real chain +Four subcommands follow no chain. `ethlambda replay-era` replays real chain history from `.era` files and checks it against the states stored in them; see [`era_replay.md`](./era_replay.md) for its flags and checks. `ethlambda benchmark` is the offline benchmarking harness, covering block building (`benchmark synthetic`, run @@ -18,7 +18,9 @@ what each workload measures; `benchmark import`'s flags are in [their own section](#benchmark-import-flags) below. `ethlambda validator` is the validator client: a separate process that holds keys and performs duties against a beacon node over the standard REST API, documented in -[its own section](#validator-flags) below. +[its own section](#validator-flags) below. `ethlambda slashing-protection` +moves that client's EIP-3076 history in and out of its directory, offline; see +[its section](#slashing-protection) below. ## `node` is the default From 98c8c43599a26f94af1ea0ae7fa917bb07967f96 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 16:39:36 -0300 Subject: [PATCH 05/12] Follow the base branch in registering a key with no history as never having signed: the keymanager import no longer dates its registrations, and the module doc, the import warning and the spec deviations say so instead of describing the dropped seeding at the current slot and epoch. The interchange vector test and the keymanager tests open the record without a slot, matching the base branch's signature. --- crates/validator/src/http_api/keystores.rs | 27 ++++++------------- crates/validator/tests/interchange_vectors.rs | 4 +-- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index cd7a20f27..699294ebc 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -11,7 +11,7 @@ //! are saved, together with their registration and before any is loaded, so //! a key never starts signing ahead of its history and a key that failed to //! import leaves none behind. A key with no history at all is registered as -//! of the current slot and epoch, with a warning. The history may also name +//! never having signed, with a warning. The history may also name //! validators that are not in the request: they get records too, and a //! later `DELETE` of one answers `not_active`. //! - `DELETE` returns the deleted keys' records as a minimal interchange, @@ -1295,14 +1295,9 @@ mod tests { async fn an_imported_key_is_registered_for_slashing_protection() { let dir = tempfile::tempdir().expect("temp dir"); let root = ethlambda_types::beacon::primitives::Root::ZERO; - let protection = crate::slashing_protection::SlashingProtection::open( - dir.path(), - root, - &[], - false, - None, - ) - .expect("opens with no keys"); + 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)); @@ -1328,7 +1323,7 @@ mod tests { // registered. drop(protection); let keys = ValidatorStore::load(dir.path()).expect("loads").pubkeys(); - crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false, None) + crate::slashing_protection::SlashingProtection::open(dir.path(), root, &keys, false) .expect("the imported key has a record"); } @@ -1664,14 +1659,9 @@ mod tests { .expect("symlinks"); let root = ethlambda_types::beacon::primitives::Root::ZERO; - let protection = crate::slashing_protection::SlashingProtection::open( - dir.path(), - root, - &[], - false, - None, - ) - .expect("opens with no keys"); + let 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)); @@ -1708,7 +1698,6 @@ mod tests { ethlambda_types::beacon::primitives::Root::ZERO, &[], false, - None, ) .expect("opens with no keys"); let mut context = context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), dir); diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs index 828f08c6c..f335068b9 100644 --- a/crates/validator/tests/interchange_vectors.rs +++ b/crates/validator/tests/interchange_vectors.rs @@ -118,8 +118,8 @@ fn run(case: &Case) -> Result<(), String> { } let dir = tempfile::tempdir().map_err(|err| err.to_string())?; - let mut protection = SlashingProtection::open(dir.path(), root, &keys, true, None) - .map_err(|err| err.to_string())?; + let mut protection = + SlashingProtection::open(dir.path(), root, &keys, true).map_err(|err| err.to_string())?; for (index, step) in case.steps.iter().enumerate() { let imported = protection.import(&step.interchange.to_string()); From ef20a83d5b0ea92719ced66e6e7942058b5d079b Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 16:58:32 -0300 Subject: [PATCH 06/12] Follow the base branch's record_of test into this branch's infallible record_of: an entry whose source is after its target is absorbed here rather than refused, so the test reads the record directly instead of unwrapping a result. --- crates/validator/src/slashing_protection/file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/validator/src/slashing_protection/file.rs b/crates/validator/src/slashing_protection/file.rs index 865a8237c..04b11ccbf 100644 --- a/crates/validator/src/slashing_protection/file.rs +++ b/crates/validator/src/slashing_protection/file.rs @@ -372,7 +372,7 @@ mod tests { ], }; - let record = record_of(&entry).expect("valid"); + let record = record_of(&entry); assert_eq!( record, From 625611f6cd161cd6d78fa9ebb8b80a5cfe0d45a7 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 17:32:22 -0300 Subject: [PATCH 07/12] Address the second review of the slashing-protection interchange. The keymanager import merges the request's history and registers its keys in one durable write (SlashingProtection::absorb, over an interchange parsed once off every lock as a StagedHistory), so a failed write holds neither and a key the history covers can no longer load after a restart that the response said would refuse it; a request that saved no key still writes nothing. The keymanager router allows 64 MiB bodies, since a complete-format interchange for a batch of validators runs past axum's 2 MB default. DELETE hands history over only for keys that are deleted or not_active, as the specification requires and Teku and Prysm do, and when the export fails every key that parsed is an error, as Prysm answers, rather than a not_found that reads as nothing to carry. The offline export fails on a requested validator with no record instead of writing a file without it. The interchange vectors are a test = false target that make test-node and make lint run by name, so a plain cargo test needs no download; the vector test reuses the public parse_root. Docs: the kept record covers what this client signed, a failed import leaves the lock file, and export needs a writable directory. --- Makefile | 3 +- bin/ethlambda/src/slashing_protection.rs | 32 +-- crates/validator/Cargo.toml | 8 + crates/validator/src/http_api/keystores.rs | 197 ++++++++++++------ crates/validator/src/http_api/mod.rs | 8 + .../validator/src/slashing_protection/mod.rs | 104 +++++---- crates/validator/tests/interchange_vectors.rs | 6 +- docs/cli.md | 4 +- 8 files changed, 234 insertions(+), 128 deletions(-) diff --git a/Makefile b/Makefile index 9ede8ba1d..5fead8535 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ lint: ## 🔍 Run clippy on all workspace crates cargo clippy --locked --workspace --all-targets -- -D warnings # The spectests are `test = false` (stale leanSpec fixtures), which # `--all-targets` skips: lint them by name so they keep compiling - cargo clippy --locked --workspace --test forkchoice_spectests --test signature_spectests --test stf_spectests --test ssz_spectests -- -D warnings + cargo clippy --locked --workspace --test forkchoice_spectests --test signature_spectests --test stf_spectests --test ssz_spectests --test interchange_vectors -- -D warnings # release-fast: release-grade opt-level to avoid stack overflows during # signature verification/aggregation, without paying for LTO on every rebuild @@ -40,6 +40,7 @@ test-consensus: leanSpec/fixtures ## 🧪 Run the consensus half of the workspac test-node: leanSpec/fixtures slashing-protection-interchange-tests ## 🧪 Run the node half of the workspace suite $(TEST) --workspace $(addprefix --exclude ,$(CONSENSUS_CRATES)) + $(TEST) -p ethlambda-validator --test interchange_vectors # --lib as well as the spec target: the BLS and KZG modules keep their fixture # vectors as unit tests, which are `ignore`d unless this feature is on. diff --git a/bin/ethlambda/src/slashing_protection.rs b/bin/ethlambda/src/slashing_protection.rs index d855193d4..501c30d7c 100644 --- a/bin/ethlambda/src/slashing_protection.rs +++ b/bin/ethlambda/src/slashing_protection.rs @@ -14,7 +14,7 @@ use std::path::PathBuf; use ethlambda_validator::beacon_node::dto::{encode_hex, parse_pubkey}; use ethlambda_validator::slashing_protection::SlashingProtection; -use tracing::{info, warn}; +use tracing::info; #[derive(Debug, clap::Args)] pub(crate) struct SlashingProtectionOptions { @@ -80,20 +80,24 @@ pub(crate) fn run(options: SlashingProtectionOptions) -> eyre::Result<()> { }) .collect::, _>>()? }; - // A requested key with no record is left out of the file rather - // than listed as never having signed, so say so: an operator - // moving that key would otherwise find out at the other end. - let mut exported = 0; - for pubkey in &pubkeys { - if protection.contains(pubkey) { - exported += 1; - } else { - warn!( - pubkey = %encode_hex(&pubkey.0), - "No slashing-protection record for this validator; it is not in the export" - ); - } + // A requested key with no record fails the export, as Lighthouse + // does, rather than being left out of a file that then reads as + // complete: a typo, or the wrong directory, would otherwise be + // found out at the other end, after the keys moved. + let missing: Vec = pubkeys + .iter() + .filter(|pubkey| !protection.contains(pubkey)) + .map(|pubkey| encode_hex(&pubkey.0)) + .collect(); + if !missing.is_empty() { + eyre::bail!( + "no slashing-protection record for {} requested validator(s) (first: {}); \ + nothing was written", + missing.len(), + missing[0] + ); } + let exported = pubkeys.len(); let interchange = protection.export(&pubkeys)?; write_export(&file, &interchange) .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; diff --git a/crates/validator/Cargo.toml b/crates/validator/Cargo.toml index 7c498eed1..b34e72488 100644 --- a/crates/validator/Cargo.toml +++ b/crates/validator/Cargo.toml @@ -77,3 +77,11 @@ tempfile = "3" [features] # Exposes `SlashingProtection::in_memory` to other crates' tests. test-utils = [] + +# Skipped by a plain `cargo test`: it reads the EIP-3076 interchange vectors, +# which `make slashing-protection-interchange-tests` downloads, and fails +# without them. `make test-node` runs it by name. +[[test]] +name = "interchange_vectors" +path = "tests/interchange_vectors.rs" +test = false diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 699294ebc..03ff1dcaa 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -7,18 +7,23 @@ //! //! - `POST` judges the request's `slashing_protection` before touching //! anything: one that does not parse, or is for another chain, fails every -//! key in the request and imports none of them. It is written once the keys -//! are saved, together with their registration and before any is loaded, so -//! a key never starts signing ahead of its history and a key that failed to -//! import leaves none behind. A key with no history at all is registered as -//! never having signed, with a warning. The history may also name -//! validators that are not in the request: they get records too, and a -//! later `DELETE` of one answers `not_active`. +//! key in the request and imports none of them. It is written once at least +//! one key is saved, in the same durable write as the keys' registration +//! and before any is loaded, so a key never starts signing ahead of its +//! history, and a write that fails, or a request that saved no key, leaves +//! nothing behind. It is written whole, entries for keys of the request +//! that failed included: real history is never the dangerous direction, and +//! a later import of such a key merges with it. A key with no history at +//! all is registered as never having signed, with a warning. The history +//! may also name validators that are not in the request: they get records +//! too, and a later `DELETE` of one answers `not_active`. //! - `DELETE` returns the deleted keys' records as a minimal interchange, //! taken after the keys are out of the store, so it covers every signature //! this process made with them. A key that is gone but still has a record is //! reported `not_active`, and its record is kept, so importing it back later -//! stays protected. +//! starts from what this client signed with it (not from what the key may +//! have signed elsewhere in between: see `docs/spec_deviations.md` on +//! doppelganger protection). //! //! The history is this client's EIP-3076 minimal watermarks (see //! [`crate::slashing_protection`]), so an export lists, per validator, its @@ -165,28 +170,42 @@ pub async fn import( return Json(StatusResponse { data }); } - // The history is judged before anything is touched, so a request whose - // history would be refused imports no key at all, rather than some keys - // without it. It is written later, together with the keys' registration, - // once the keys it came with are known to be saved: written now, a key - // that then failed to import would leave history behind for nothing. - if let Some(interchange) = &request.slashing_protection { - let interchange = interchange.clone(); - let judged = crate::slashing_protection::blocking(&context.protection, move |protection| { - protection.check_import(&interchange) - }) - .await; - if let Err(err) = judged { - let message = format!("slashing_protection was not imported: {err}"); - warn!(%err, "Refusing an import whose slashing-protection history was refused"); - let data = request - .keystores - .iter() - .map(|_| KeyStatus::error(&message)) - .collect(); - return Json(StatusResponse { data }); + // The history is parsed once, off every lock, and judged against the + // record's chain before anything is touched, so a request whose history + // would be refused imports no key at all, rather than some keys without + // it. It is written later, in the same write as the keys' registration, + // once the keys it came with are known to be saved. + let history = match &request.slashing_protection { + None => None, + Some(interchange) => { + let staged = crate::slashing_protection::SlashingProtection::stage(interchange); + let judged = match staged { + Ok(staged) => { + let staged = Arc::new(staged); + let checked = staged.clone(); + crate::slashing_protection::blocking(&context.protection, move |protection| { + protection.check_staged(&checked) + }) + .await + .map(|()| staged) + } + Err(err) => Err(err), + }; + match judged { + Ok(staged) => Some(staged), + Err(err) => { + let message = format!("slashing_protection was not imported: {err}"); + warn!(%err, "Refusing an import whose slashing-protection history was refused"); + let data = request + .keystores + .iter() + .map(|_| KeyStatus::error(&message)) + .collect(); + return Json(StatusResponse { data }); + } + } } - } + }; // Held across this whole function: the entire open-mutate-save cycle // over the definitions file, including the slow decrypt-and-persist loop @@ -247,30 +266,26 @@ pub async fn import( prepared.push(prepare_import(&context, &mut definitions, json, password)); } - // Every persisted key is registered in the slashing-protection record in - // one write, before any of them is loaded: the duty path must never meet a - // key with no record. After persisting rather than before, so a key whose - // persist failed leaves no record behind to let a hand-copied keystore - // past the startup check later. Still under the definitions guard, so a - // concurrent delete cannot interleave. + // The request's history is merged and every persisted key registered in + // one durable write, before any of them is loaded: the duty path must + // never meet a key with no record, and a key the history covers starts + // with that history rather than as never having signed. 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(); - // The request's history goes in first, in the same critical section, so - // a key it covers is registered with that history rather than as never - // having signed. - let history = request.slashing_protection.clone(); + // Nothing is written for a request that saved no key, its history + // included: such a request leaves nothing behind. let registration = if persisted.is_empty() { - Ok(0) + Ok((0, 0)) } else { let registering = persisted.clone(); + let history = history.clone(); crate::slashing_protection::blocking(&context.protection, move |protection| { - if let Some(history) = &history { - let validators = protection.import(history)?; - info!(validators, "Imported slashing-protection history"); - } - protection.register(®istering) + protection.absorb(history.as_deref(), ®istering) }) .await }; @@ -286,18 +301,21 @@ pub async fn import( "Slashing-protection records written, but the directory could not be flushed; \ the imported keys are registered" ); - Ok(0) + Ok((0, 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" - ); + if let Ok((named, registered)) = registration { + if named > 0 { + info!(validators = named, "Imported slashing-protection history"); + } + if 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 @@ -726,8 +744,17 @@ pub async fn delete( // Exported only now, with the keys out of the store: a signature with one // of them either finished before the write lock above was granted, and is - // in the record, or can no longer happen. - let requested: Vec> = results.iter().map(|(pubkey, _)| *pubkey).collect(); + // in the record, or can no longer happen. Only for the keys that are + // `deleted` or may become `not_active`, as the specification requires: a + // key whose delete failed is still on disk and comes back on restart, so + // its history is not handed over. Teku and Prysm filter the same way. + let requested: Vec> = results + .iter() + .map(|(pubkey, status)| match status.status.as_str() { + "deleted" | "not_found" => *pubkey, + _ => None, + }) + .collect(); let exported = crate::slashing_protection::blocking(&context.protection, move |protection| { let pubkeys: Vec = requested.iter().flatten().copied().collect(); let interchange = protection.export(&pubkeys)?; @@ -741,17 +768,18 @@ pub async fn delete( let (slashing_protection, recorded) = match exported { Ok(exported) => exported, Err(err) => { - // A key that was deleted, or is gone with a record, cannot have its - // history handed over, so no caller may treat it as safe to move: - // it becomes an error. Every other status (`not_found`, a malformed - // pubkey) was decided before the export and still stands. + // With the record unreadable, nothing here can tell a key with no + // history from one whose history could not be read, so every key + // that parsed becomes an error, as Prysm answers: a `not_found` + // here would tell a migrator there is nothing to carry. A + // malformed pubkey keeps its own error. warn!(%err, "Deleted keys whose slashing-protection history could not be exported"); - let message = format!("deleted, but slashing_protection could not be exported: {err}"); + let message = format!("slashing_protection could not be exported: {err}"); let data = results .into_iter() - .map(|(_, status)| match status.status.as_str() { - "deleted" => KeyStatus::error(&message), - _ => status, + .map(|(pubkey, status)| match pubkey { + Some(_) => KeyStatus::error(&message), + None => status, }) .collect(); return Json(DeleteResponse { @@ -1883,6 +1911,47 @@ mod tests { assert!(protection.lock().expect("not poisoned").is_empty()); } + /// The history and the registration are one write: when it fails, the + /// history is not held either, so the keys it named do not load and sign + /// after a restart as if the import had half succeeded. + #[tokio::test] + async fn a_failed_write_holds_neither_history_nor_registration() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, protection) = context_with_record(dir.path()); + // A directory in the record's place makes the write's rename fail. + let record = protection + .lock() + .expect("not poisoned") + .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 (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + "slashing_protection": history(ZERO_ROOT, 100, 5, 10), + }), + ), + ) + .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!(protection.lock().expect("not poisoned").is_empty()); + } + #[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 f16de2850..189e5ac7a 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -163,8 +163,16 @@ pub fn router(context: KeymanagerContext, token: String) -> Router { .with_state(context) .layer(middleware::from_fn(require_bearer)) .layer(Extension(ApiToken(token))) + // `POST /eth/v1/keystores` carries a whole EIP-3076 interchange, which + // in the complete format for a batch of validators runs past axum's + // 2 MB default. Nimbus allows 16 MiB and Lodestar 20 MB; Lighthouse + // and Prysm set no limit. + .layer(axum::extract::DefaultBodyLimit::max(KEYMANAGER_BODY_LIMIT)) } +/// The largest request body the keymanager API reads. +const KEYMANAGER_BODY_LIMIT: usize = 64 * 1024 * 1024; + #[derive(Clone)] struct ApiToken(String); diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index d588e5527..326706738 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -185,6 +185,15 @@ impl Signed { } } +/// An EIP-3076 interchange parsed once, for [`SlashingProtection::absorb`]: +/// the chain it names and each validator's watermarks, reduced as +/// [`SlashingProtection::import`] describes. +#[derive(Debug, Clone)] +pub struct StagedHistory { + root: Root, + entries: Vec<(BlsPubkey, Record)>, +} + /// What one validator has signed: its watermarks. /// /// `None` is "never signed", kept distinct from zero so that slot 0 and epoch @@ -606,16 +615,7 @@ impl SlashingProtection { /// 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) + self.absorb(None, pubkeys).map(|(_, registered)| registered) } /// The chain an EIP-3076 interchange names, read without importing it: what @@ -659,45 +659,30 @@ impl SlashingProtection { /// or one that does not parse, changes nothing. Returns how many distinct /// validators it named. pub fn import(&mut self, interchange: &str) -> Result { - let refused = |reason: String| Error::SlashingProtection { - path: "interchange".to_string(), - reason, - }; - let (root, entries) = file::parse(interchange).map_err(refused)?; - if root != self.genesis_validators_root { - return Err(refused(format!( - "it is for genesis_validators_root {}, but this record is for {}", - encode_hex(&root.0), - encode_hex(&self.genesis_validators_root.0) - ))); - } + let staged = Self::stage(interchange)?; + self.absorb(Some(&staged), &[]).map(|(named, _)| named) + } - let mut staged: HashMap = HashMap::new(); - for (pubkey, imported) in entries { - let current = staged - .get(&pubkey) - .or_else(|| self.records.get(&pubkey)) - .copied() - .unwrap_or_default(); - staged.insert(pubkey, current.merged(imported)); - } - let count = staged.len(); - if count > 0 { - self.commit(staged)?; - } - Ok(count) + /// Parse `interchange` into what [`Self::absorb`] takes, off any lock: + /// its version and shape are judged here, its chain by + /// [`Self::check_staged`] or by `absorb` itself. + pub fn stage(interchange: &str) -> Result { + let (root, entries) = + file::parse(interchange).map_err(|reason| Error::SlashingProtection { + path: "interchange".to_string(), + reason, + })?; + Ok(StagedHistory { root, entries }) } - /// Whether [`Self::import`] would accept `interchange`, changing nothing: - /// it parses, is of the right version, and is for this record's chain. - pub fn check_import(&self, interchange: &str) -> Result<()> { - let chain = Self::interchange_chain(interchange)?; - if chain != self.genesis_validators_root { + /// Whether `staged` is for this record's chain, changing nothing. + pub fn check_staged(&self, staged: &StagedHistory) -> Result<()> { + if staged.root != self.genesis_validators_root { return Err(Error::SlashingProtection { path: "interchange".to_string(), reason: format!( "it is for genesis_validators_root {}, but this record is for {}", - encode_hex(&chain.0), + encode_hex(&staged.root.0), encode_hex(&self.genesis_validators_root.0) ), }); @@ -705,6 +690,41 @@ impl SlashingProtection { Ok(()) } + /// Merge `history`, if any, as [`Self::import`] describes, and register + /// every key in `register` that then still has no record, in one durable + /// write: either all of it is on disk or none of it is. Returns how many + /// validators the history named and how many keys were newly registered. + pub fn absorb( + &mut self, + history: Option<&StagedHistory>, + register: &[BlsPubkey], + ) -> Result<(usize, usize)> { + let mut staged: HashMap = HashMap::new(); + if let Some(history) = history { + self.check_staged(history)?; + for (pubkey, imported) in &history.entries { + let current = staged + .get(pubkey) + .or_else(|| self.records.get(pubkey)) + .copied() + .unwrap_or_default(); + staged.insert(*pubkey, current.merged(*imported)); + } + } + let named = staged.len(); + let mut registered = 0; + for pubkey in register { + if !staged.contains_key(pubkey) && !self.records.contains_key(pubkey) { + staged.insert(*pubkey, Record::default()); + registered += 1; + } + } + if !staged.is_empty() { + self.commit(staged)?; + } + Ok((named, registered)) + } + /// The records of `pubkeys` as a minimal EIP-3076 version 5 interchange. /// /// A key with no record is left out rather than listed as never having diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs index f335068b9..c67ed4af8 100644 --- a/crates/validator/tests/interchange_vectors.rs +++ b/crates/validator/tests/interchange_vectors.rs @@ -203,9 +203,5 @@ fn parse_u64(text: &str) -> Result { } fn parse_root(text: &str) -> Result { - let bytes = hex::decode(text.trim_start_matches("0x")).map_err(|err| err.to_string())?; - let bytes: [u8; 32] = bytes - .try_into() - .map_err(|_| format!("{text} is not 32 bytes"))?; - Ok(H256(bytes)) + ethlambda_validator::beacon_node::dto::parse_root(text).map_err(|err| err.to_string()) } diff --git a/docs/cli.md b/docs/cli.md index c0aa37566..87eb3cf97 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -473,8 +473,8 @@ ethlambda slashing-protection export --validators-dir ./validators --pubkeys 0xa | Sub-command | Does | |---|---| -| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing; the client refuses the record at its next start if that is not its beacon node's chain | -| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record is left out, with a warning. Refuses to overwrite an existing file | +| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing but the lock file; the client refuses the record at its next start if that is not its beacon node's chain | +| `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record fails the export, so a typo or the wrong directory cannot produce a file that reads as complete. Takes the record's lock, so the directory must be writable. Refuses to overwrite an existing file | ## `benchmark import` flags From 21ef15e3e7ae9f664062154dfb01e130af64540a Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 18:33:52 -0300 Subject: [PATCH 08/12] Apply the bot reviews of the slashing-protection interchange's second round. An export lists a key once however many times it is requested: a DELETE naming one key twice answered deleted then not_active and handed over an interchange naming it twice, which this client's own record reader refuses, and a repeated --pubkeys did the same. The CLI import parses its file once, staging it before the record is opened for the chain it names, so interchange_chain is gone and StagedHistory exposes its root. The keymanager import parses the interchange on the blocking pool, since a complete-format file runs to tens of megabytes and the handler's thread also drives the duty services. absorb warns when a registered key already has a record and brought no history, since that record is what this client last signed with the key and says nothing about what it signed elsewhere since. The DELETE comments say why the export may run after the keys leave the store (the duty services record before taking the store's read lock) and that a failed export leaves the deletions in place with their records kept for a retry. The interchange_vectors target drops an unused import that failed make lint, and the workflow's by-name clippy step now names the target as the Makefile does, which is why CI had not caught it. Four tests: an import raises an existing watermark and the raise survives a restart, an export lists a key requested twice once, a DELETE listing a key twice exports it once, and history naming a key outside the request gives it a record that a later DELETE answers not_active with. The CLI reference no longer says a failed import leaves a lock file, since the lock is now on the directory. --- .github/workflows/ci.yml | 2 +- bin/ethlambda/src/slashing_protection.rs | 9 +- crates/validator/src/http_api/keystores.rs | 132 ++++++++++++++++-- .../validator/src/slashing_protection/mod.rs | 91 +++++++++--- crates/validator/tests/interchange_vectors.rs | 2 +- docs/cli.md | 2 +- 6 files changed, 205 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bb50e5dc8..232c865ae 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,7 +63,7 @@ jobs: # lag leanVM, and `--all-targets` leaves such targets out. Name them so # they keep compiling until they run again. - name: Clippy (skipped spectests) - run: cargo clippy --locked --workspace --test forkchoice_spectests --test signature_spectests --test stf_spectests --test ssz_spectests -- -D warnings + run: cargo clippy --locked --workspace --test forkchoice_spectests --test signature_spectests --test stf_spectests --test ssz_spectests --test interchange_vectors -- -D warnings # Two features gate code that no step in `lint` reaches, so both would rot # silently between merges. diff --git a/bin/ethlambda/src/slashing_protection.rs b/bin/ethlambda/src/slashing_protection.rs index 501c30d7c..1c080b497 100644 --- a/bin/ethlambda/src/slashing_protection.rs +++ b/bin/ethlambda/src/slashing_protection.rs @@ -55,9 +55,12 @@ pub(crate) fn run(options: SlashingProtectionOptions) -> eyre::Result<()> { } => { let interchange = std::fs::read_to_string(&file) .map_err(|err| eyre::eyre!("{}: {err}", file.display()))?; - let chain = SlashingProtection::interchange_chain(&interchange)?; - let mut protection = SlashingProtection::open_offline(&validators_dir, Some(chain))?; - let validators = protection.import(&interchange)?; + // Parsed once: the file can be tens of megabytes in the complete + // format, and its chain is needed before the record is opened. + let staged = SlashingProtection::stage(&interchange)?; + let mut protection = + SlashingProtection::open_offline(&validators_dir, Some(staged.root()))?; + let (validators, _) = protection.absorb(Some(&staged), &[])?; info!( validators, file = %file.display(), diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 03ff1dcaa..137da5a9a 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -151,7 +151,7 @@ pub async fn list(State(context): State) -> Json, - Json(request): Json, + Json(mut request): Json, ) -> Json { if request.keystores.len() > MAX_KEYSTORES_PER_IMPORT { let message = format!( @@ -174,11 +174,23 @@ pub async fn import( // record's chain before anything is touched, so a request whose history // would be refused imports no key at all, rather than some keys without // it. It is written later, in the same write as the keys' registration, - // once the keys it came with are known to be saved. - let history = match &request.slashing_protection { + // once the keys it came with are known to be saved. Parsed on the + // blocking pool: a complete-format interchange runs to tens of megabytes + // (see `KEYMANAGER_BODY_LIMIT`), and the executor thread this handler + // runs on also drives the duty services. + let history = match request.slashing_protection.take() { None => None, Some(interchange) => { - let staged = crate::slashing_protection::SlashingProtection::stage(interchange); + let staged = tokio::task::spawn_blocking(move || { + crate::slashing_protection::SlashingProtection::stage(&interchange) + }) + .await + .unwrap_or_else(|err| { + Err(Error::SlashingProtection { + path: "interchange".to_string(), + reason: format!("could not be parsed: {err}"), + }) + }); let judged = match staged { Ok(staged) => { let staged = Arc::new(staged); @@ -744,10 +756,14 @@ pub async fn delete( // Exported only now, with the keys out of the store: a signature with one // of them either finished before the write lock above was granted, and is - // in the record, or can no longer happen. Only for the keys that are - // `deleted` or may become `not_active`, as the specification requires: a - // key whose delete failed is still on disk and comes back on restart, so - // its history is not handed over. Teku and Prysm filter the same way. + // in the record, or can no longer happen. That holds because the duty + // services record before they take the store's read lock to sign (see + // `attestation.rs` and `proposal.rs`), so no signature exists without its + // record already on disk. Only for the keys that are `deleted` or may + // become `not_active`, as the specification requires: a key whose delete + // failed is still on disk and comes back on restart, so its history is + // not handed over. Teku and Prysm filter the same way. A key listed twice + // is `deleted` once and `not_found` once; `export` lists it once. let requested: Vec> = results .iter() .map(|(pubkey, status)| match status.status.as_str() { @@ -772,7 +788,11 @@ pub async fn delete( // history from one whose history could not be read, so every key // that parsed becomes an error, as Prysm answers: a `not_found` // here would tell a migrator there is nothing to carry. A - // malformed pubkey keeps its own error. + // malformed pubkey keeps its own error. The deletions themselves + // already happened and are not undone: the keys' records are + // kept, so a retry answers `not_active` with their history. The + // field is required, so it carries an empty string rather than + // an interchange that would read as "nothing signed". warn!(%err, "Deleted keys whose slashing-protection history could not be exported"); let message = format!("slashing_protection could not be exported: {err}"); let data = results @@ -1736,10 +1756,14 @@ mod tests { /// A minimal interchange for `KEYSTORE_PUBKEY` on the chain `root`. fn history(root: &str, slot: u64, source: u64, target: u64) -> String { + history_for(KEYSTORE_PUBKEY, root, slot, source, target) + } + + fn history_for(pubkey: &str, root: &str, slot: u64, source: u64, target: u64) -> String { serde_json::json!({ "metadata": { "interchange_format_version": "5", "genesis_validators_root": root }, "data": [{ - "pubkey": KEYSTORE_PUBKEY, + "pubkey": pubkey, "signed_blocks": [{ "slot": slot.to_string() }], "signed_attestations": [{ "source_epoch": source.to_string(), @@ -1881,6 +1905,94 @@ mod tests { ); } + /// The history may name a validator that is not in the request: it gets a + /// record too, so a later delete of it answers `not_active` with that + /// history rather than `not_found`. + #[tokio::test] + async fn history_for_a_key_outside_the_request_gets_a_record() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, protection) = context_with_record(dir.path()); + let app = router(context, TOKEN.to_string()); + let other = format!("0x{}", "ab".repeat(48)); + + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + "slashing_protection": history_for(&other, ZERO_ROOT, 100, 5, 10), + }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + + let other_pubkey = parse_pubkey(&other).expect("pubkey"); + { + let protection = protection.lock().expect("not poisoned"); + assert!(protection.contains(&other_pubkey)); + assert!(protection.check_block(&other_pubkey, 100).is_err()); + assert!(protection.contains(&parse_pubkey(KEYSTORE_PUBKEY).expect("pubkey"))); + } + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [other] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "not_active", "got {body}"); + let interchange: serde_json::Value = + serde_json::from_str(body["slashing_protection"].as_str().expect("a string")) + .expect("json"); + assert_eq!(interchange["data"][0]["pubkey"], other); + assert_eq!(interchange["data"][0]["signed_blocks"][0]["slot"], "100"); + } + + /// A key listed twice in one delete is `deleted` once and `not_active` + /// once, and the returned interchange names it once: one that named a + /// validator twice would be refused on import. + #[tokio::test] + async fn a_delete_listing_a_key_twice_exports_it_once() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, _) = context_with_record(dir.path()); + let app = router(context, TOKEN.to_string()); + + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ "keystores": [keystore_json()], "passwords": [PASSWORD] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [KEYSTORE_PUBKEY, KEYSTORE_PUBKEY] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "deleted", "got {body}"); + assert_eq!(body["data"][1]["status"], "not_active", "got {body}"); + let interchange: serde_json::Value = + serde_json::from_str(body["slashing_protection"].as_str().expect("a string")) + .expect("json"); + assert_eq!(interchange["data"].as_array().expect("array").len(), 1); + assert_eq!(interchange["data"][0]["pubkey"], KEYSTORE_PUBKEY); + } + /// History that came with a key that then failed to import is not /// written: nothing it describes is held here. #[cfg(unix)] diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 326706738..bcd366ba1 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -59,7 +59,7 @@ pub mod file; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; @@ -194,6 +194,14 @@ pub struct StagedHistory { entries: Vec<(BlsPubkey, Record)>, } +impl StagedHistory { + /// The chain the interchange names: what a record created for it must + /// belong to. + pub fn root(&self) -> Root { + self.root + } +} + /// What one validator has signed: its watermarks. /// /// `None` is "never signed", kept distinct from zero so that slot 0 and epoch @@ -618,17 +626,6 @@ impl SlashingProtection { self.absorb(None, pubkeys).map(|(_, registered)| registered) } - /// The chain an EIP-3076 interchange names, read without importing it: what - /// a record created for it must belong to. - pub fn interchange_chain(interchange: &str) -> Result { - file::parse(interchange) - .map(|(root, _)| root) - .map_err(|reason| Error::SlashingProtection { - path: "interchange".to_string(), - reason, - }) - } - /// The chain this record belongs to. pub fn genesis_validators_root(&self) -> Root { self.genesis_validators_root @@ -694,6 +691,11 @@ impl SlashingProtection { /// every key in `register` that then still has no record, in one durable /// write: either all of it is on disk or none of it is. Returns how many /// validators the history named and how many keys were newly registered. + /// + /// A key in `register` that already has a record and is not in the + /// history keeps that record, with a warning: it is what this client last + /// signed with the key, which says nothing about what the key signed + /// elsewhere since. pub fn absorb( &mut self, history: Option<&StagedHistory>, @@ -714,10 +716,19 @@ impl SlashingProtection { let named = staged.len(); let mut registered = 0; for pubkey in register { - if !staged.contains_key(pubkey) && !self.records.contains_key(pubkey) { - staged.insert(*pubkey, Record::default()); - registered += 1; + if staged.contains_key(pubkey) { + continue; } + if self.records.contains_key(pubkey) { + warn!( + pubkey = %encode_hex(&pubkey.0), + "Registering a validator that already has a slashing-protection record and \ + brought no history; it continues from what this client last signed with it" + ); + continue; + } + staged.insert(*pubkey, Record::default()); + registered += 1; } if !staged.is_empty() { self.commit(staged)?; @@ -729,10 +740,14 @@ impl SlashingProtection { /// /// A key with no record is left out rather than listed as never having /// signed: an empty entry would tell the importer something this client - /// does not know. + /// does not know. A key listed twice is exported once: an interchange that + /// names a validator twice is refused by stricter importers, this client's + /// own record reader among them. pub fn export(&self, pubkeys: &[BlsPubkey]) -> Result { + let mut seen = HashSet::with_capacity(pubkeys.len()); let selected = pubkeys .iter() + .filter(|pubkey| seen.insert(**pubkey)) .filter_map(|pubkey| self.records.get_key_value(pubkey)); file::render(self.genesis_validators_root, selected).map_err(|err| { Error::SlashingProtection { @@ -1276,6 +1291,30 @@ mod tests { assert!(attest(&mut protection, 1, &data(128, 3, 4, 0xaa)).is_ok()); } + /// A newer history imported over an older record raises it, and the raised + /// watermarks survive a restart: the merge updates an existing entry, not + /// only inserts a missing one. + #[test] + fn an_import_raises_an_existing_watermark() { + let dir = tempfile::tempdir().expect("temp dir"); + { + let mut protection = open(dir.path(), &[1], true).expect("init"); + propose(&mut protection, 1, 10).expect("propose"); + attest(&mut protection, 1, &data(96, 2, 3, 0xaa)).expect("attest"); + + protection + .import(&interchange(0x42, 1, 50, 4, 6)) + .expect("imports"); + assert!(propose(&mut protection, 1, 40).is_err()); + assert!(attest(&mut protection, 1, &data(160, 3, 5, 0xaa)).is_err()); + } + let mut protection = open(dir.path(), &[1], false).expect("reopens"); + assert!(propose(&mut protection, 1, 50).is_err()); + assert!(propose(&mut protection, 1, 51).is_ok()); + assert!(attest(&mut protection, 1, &data(192, 4, 6, 0xaa)).is_err()); + assert!(attest(&mut protection, 1, &data(224, 4, 7, 0xaa)).is_ok()); + } + /// An older history imported over a newer record must not lower it. #[test] fn an_import_never_lowers_a_watermark() { @@ -1320,6 +1359,24 @@ mod tests { assert!(propose(&mut fresh, 1, 97).is_err()); } + /// A key requested twice is exported once: this client's own record + /// reader refuses a file naming a validator twice, and so may others'. + #[test] + fn an_export_lists_a_key_requested_twice_once() { + 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 exported = protection.export(&[pubkey(1), pubkey(1)]).expect("exports"); + let json: serde_json::Value = serde_json::from_str(&exported).expect("json"); + assert_eq!(json["data"].as_array().expect("array").len(), 1); + + let other = tempfile::tempdir().expect("temp dir"); + let mut fresh = open(other.path(), &[], false).expect("opens"); + fresh.import(&exported).expect("round trip"); + assert!(propose(&mut fresh, 1, 97).is_err()); + } + #[test] fn opening_offline_needs_a_record_or_a_chain_to_create_one_for() { let dir = tempfile::tempdir().expect("temp dir"); @@ -1363,7 +1420,7 @@ mod tests { let mut protection = open(dir.path(), &[], false).expect("opens"); for text in ["", "{}", "[]"] { protection.import(text).expect_err("not an interchange"); - SlashingProtection::interchange_chain(text).expect_err("not an interchange"); + SlashingProtection::stage(text).expect_err("not an interchange"); } assert!(protection.is_empty()); } diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs index c67ed4af8..475013c38 100644 --- a/crates/validator/tests/interchange_vectors.rs +++ b/crates/validator/tests/interchange_vectors.rs @@ -15,7 +15,7 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; use ethlambda_types::beacon::containers::shared::{AttestationData, Checkpoint}; -use ethlambda_types::beacon::primitives::{BlsPubkey, H256, Root}; +use ethlambda_types::beacon::primitives::{BlsPubkey, Root}; use ethlambda_validator::beacon_node::dto::parse_pubkey; use ethlambda_validator::slashing_protection::SlashingProtection; use serde::Deserialize; diff --git a/docs/cli.md b/docs/cli.md index 87eb3cf97..e9e28a389 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -473,7 +473,7 @@ ethlambda slashing-protection export --validators-dir ./validators --pubkeys 0xa | Sub-command | Does | |---|---| -| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing but the lock file; the client refuses the record at its next start if that is not its beacon node's chain | +| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing; the client refuses the record at its next start if that is not its beacon node's chain | | `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record fails the export, so a typo or the wrong directory cannot produce a file that reads as complete. Takes the record's lock, so the directory must be writable. Refuses to overwrite an existing file | ## `benchmark import` flags From 1962a95623762afcaf1aa956c4e0dec1b63b1ec0 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 18:46:02 -0300 Subject: [PATCH 09/12] Write the keymanager import's history before saving any key, rather than in the same write as the keys' registration. A key deleted here earlier keeps its record, so with the keys saved first a crash before the history landed would load such a key at its next start on the watermarks it had before it left, with whatever it signed elsewhere since unknown: the migration case the history exists for, and the one the startup check cannot see because the key has a record. The history only ever raises watermarks, so writing it for keys that then fail to save leaves nothing dangerous behind; the keys are still registered after they are saved, in one write, with the existing rollback. The post-persist absorb takes the history along again so that a key it covers is not warned about as having brought none, and absorb now drops entries whose watermarks would not change before committing, so that second call writes nothing unless a key is new and re-importing the same history costs one write. check_staged is private, with no caller outside absorb. history_is_written_before_its_key_is_saved pins the ordering on a key with an older record whose save fails, and a_failed_history_write_saves_no_key replaces the one-write test, asserting that no keystore reaches disk when the history cannot be written. The module doc, the field doc and the CLI reference say the history is written first and why. --- crates/validator/src/http_api/keystores.rs | 138 +++++++++++------- .../validator/src/slashing_protection/mod.rs | 9 +- docs/cli.md | 6 +- 3 files changed, 92 insertions(+), 61 deletions(-) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 137da5a9a..f266ea9f0 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -5,18 +5,22 @@ //! The specification lets these routes carry a validator's EIP-3076 history in //! and out, and both do: //! -//! - `POST` judges the request's `slashing_protection` before touching -//! anything: one that does not parse, or is for another chain, fails every -//! key in the request and imports none of them. It is written once at least -//! one key is saved, in the same durable write as the keys' registration -//! and before any is loaded, so a key never starts signing ahead of its -//! history, and a write that fails, or a request that saved no key, leaves -//! nothing behind. It is written whole, entries for keys of the request -//! that failed included: real history is never the dangerous direction, and -//! a later import of such a key merges with it. A key with no history at -//! all is registered as never having signed, with a warning. The history -//! may also name validators that are not in the request: they get records -//! too, and a later `DELETE` of one answers `not_active`. +//! - `POST` judges and writes the request's `slashing_protection` before +//! touching any key: one that does not parse, or is for another chain, +//! fails every key in the request and imports none of them. Written first, +//! rather than with the keys' registration, because a key that was deleted +//! here earlier keeps its record: were the keys saved first, a crash before +//! the history landed would load such a key at its next start on the +//! watermarks it had before it left, with whatever it signed elsewhere +//! unknown. The history only ever raises watermarks, so writing it for keys +//! that then fail to save leaves nothing dangerous behind, and it is +//! written whole, entries for keys of the request that failed included: a +//! later import of such a key merges with it. The keys are registered after +//! they are saved, in one write, before any is loaded, and a registration +//! that fails takes the keys off disk again. A key with no history at all is +//! registered as never having signed, with a warning. The history may also +//! name validators that are not in the request: they get records too, and +//! a later `DELETE` of one answers `not_active`. //! - `DELETE` returns the deleted keys' records as a minimal interchange, //! taken after the keys are out of the store, so it covers every signature //! this process made with them. A key that is gone but still has a record is @@ -80,8 +84,8 @@ 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>, - /// An EIP-3076 interchange, written once the keys are saved and before - /// any of them is loaded; see the module documentation. + /// An EIP-3076 interchange, written before any key is saved; see the + /// module documentation. #[serde(default)] pub slashing_protection: Option, } @@ -170,11 +174,10 @@ pub async fn import( return Json(StatusResponse { data }); } - // The history is parsed once, off every lock, and judged against the - // record's chain before anything is touched, so a request whose history - // would be refused imports no key at all, rather than some keys without - // it. It is written later, in the same write as the keys' registration, - // once the keys it came with are known to be saved. Parsed on the + // The history is parsed once, off every lock, and written before any key + // is touched, so a request whose history is refused imports no key at + // all, and no key can be saved ahead of the history it came with (see the + // module documentation for the crash that ordering closes). Parsed on the // blocking pool: a complete-format interchange runs to tens of megabytes // (see `KEYMANAGER_BODY_LIMIT`), and the executor thread this handler // runs on also drives the duty services. @@ -191,20 +194,23 @@ pub async fn import( reason: format!("could not be parsed: {err}"), }) }); - let judged = match staged { + let written = match staged { Ok(staged) => { let staged = Arc::new(staged); - let checked = staged.clone(); + let absorbed = staged.clone(); crate::slashing_protection::blocking(&context.protection, move |protection| { - protection.check_staged(&checked) + protection.absorb(Some(&absorbed), &[]) }) .await - .map(|()| staged) + .map(|(named, _)| (staged, named)) } Err(err) => Err(err), }; - match judged { - Ok(staged) => Some(staged), + match written { + Ok((staged, named)) => { + info!(validators = named, "Imported slashing-protection history"); + Some(staged) + } Err(err) => { let message = format!("slashing_protection was not imported: {err}"); warn!(%err, "Refusing an import whose slashing-protection history was refused"); @@ -278,19 +284,19 @@ pub async fn import( prepared.push(prepare_import(&context, &mut definitions, json, password)); } - // The request's history is merged and every persisted key registered in - // one durable write, before any of them is loaded: the duty path must - // never meet a key with no record, and a key the history covers starts - // with that history rather than as never having signed. 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. + // Every persisted key is registered in one durable 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. The history goes along again so that a key it covers is + // left on it rather than warned about as having brought none; it was + // written above, so this costs nothing unless a key is new. 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(); - // Nothing is written for a request that saved no key, its history - // included: such a request leaves nothing behind. + // Nothing to register for a request that saved no key. let registration = if persisted.is_empty() { Ok((0, 0)) } else { @@ -317,17 +323,14 @@ pub async fn import( } other => other, }; - if let Ok((named, registered)) = registration { - if named > 0 { - info!(validators = named, "Imported slashing-protection history"); - } - if registered > 0 { - warn!( - registered, - "Imported validator keys with no slashing-protection history; each starts as \ - never having signed" - ); - } + 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 @@ -1993,11 +1996,14 @@ mod tests { assert_eq!(interchange["data"][0]["pubkey"], KEYSTORE_PUBKEY); } - /// History that came with a key that then failed to import is not - /// written: nothing it describes is held here. + /// The history is written before its key is saved, so it is held even + /// when the key then fails to save. The key here was deleted earlier and + /// kept an older record: had the key been saved first, a crash before the + /// history landed would have loaded it on that older record at the next + /// start, which is the case the ordering exists for. #[cfg(unix)] #[tokio::test] - async fn history_whose_key_failed_to_import_is_not_written() { + async fn history_is_written_before_its_key_is_saved() { let dir = tempfile::tempdir().expect("temp dir"); // A symlink at the keystore's path makes `persist_import` refuse. let target = dir.path().join("elsewhere"); @@ -2005,6 +2011,16 @@ mod tests { std::os::unix::fs::symlink(&target, dir.path().join(format!("{KEYSTORE_PUBKEY}.json"))) .expect("symlinks"); let (context, protection) = context_with_record(dir.path()); + let pubkey = parse_pubkey(KEYSTORE_PUBKEY).expect("pubkey"); + { + let mut protection = protection.lock().expect("not poisoned"); + protection.register(&[pubkey]).expect("registers"); + protection + .check_and_record_block(&pubkey, 10) + .expect("written") + .expect("allowed"); + } + let store = context.store.clone(); let (_, body) = send( router(context, TOKEN.to_string()), @@ -2020,14 +2036,24 @@ mod tests { ) .await; assert_eq!(body["data"][0]["status"], "error", "got {body}"); - assert!(protection.lock().expect("not poisoned").is_empty()); + assert!(store.read().await.is_empty(), "the key did not load"); + assert!( + !dir.path().join("validator_definitions.yml").exists(), + "nothing was saved" + ); + let protection = protection.lock().expect("not poisoned"); + assert!( + protection.check_block(&pubkey, 100).is_err(), + "the history raised the key's record" + ); + assert!(protection.check_block(&pubkey, 101).is_ok()); } - /// The history and the registration are one write: when it fails, the - /// history is not held either, so the keys it named do not load and sign - /// after a restart as if the import had half succeeded. + /// The history is written before any key is touched: when that write + /// fails, no key is saved or loaded, so nothing of the request is on disk + /// to load and sign after a restart as if the import had half succeeded. #[tokio::test] - async fn a_failed_write_holds_neither_history_nor_registration() { + async fn a_failed_history_write_saves_no_key() { let dir = tempfile::tempdir().expect("temp dir"); let (context, protection) = context_with_record(dir.path()); // A directory in the record's place makes the write's rename fail. @@ -2058,10 +2084,14 @@ mod tests { body["data"][0]["message"] .as_str() .unwrap_or_default() - .starts_with("not imported"), + .starts_with("slashing_protection was not imported"), "got {body}" ); assert!(protection.lock().expect("not poisoned").is_empty()); + assert!( + !dir.path().join(format!("{KEYSTORE_PUBKEY}.json")).exists(), + "no keystore was saved" + ); } #[tokio::test] diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index bcd366ba1..4fdc80cb2 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -661,8 +661,7 @@ impl SlashingProtection { } /// Parse `interchange` into what [`Self::absorb`] takes, off any lock: - /// its version and shape are judged here, its chain by - /// [`Self::check_staged`] or by `absorb` itself. + /// its version and shape are judged here, its chain by `absorb`. pub fn stage(interchange: &str) -> Result { let (root, entries) = file::parse(interchange).map_err(|reason| Error::SlashingProtection { @@ -673,7 +672,7 @@ impl SlashingProtection { } /// Whether `staged` is for this record's chain, changing nothing. - pub fn check_staged(&self, staged: &StagedHistory) -> Result<()> { + fn check_staged(&self, staged: &StagedHistory) -> Result<()> { if staged.root != self.genesis_validators_root { return Err(Error::SlashingProtection { path: "interchange".to_string(), @@ -695,7 +694,8 @@ impl SlashingProtection { /// A key in `register` that already has a record and is not in the /// history keeps that record, with a warning: it is what this client last /// signed with the key, which says nothing about what the key signed - /// elsewhere since. + /// elsewhere since. Nothing is written when no watermark would change, + /// so absorbing a history twice costs one write, not two. pub fn absorb( &mut self, history: Option<&StagedHistory>, @@ -730,6 +730,7 @@ impl SlashingProtection { staged.insert(*pubkey, Record::default()); registered += 1; } + staged.retain(|pubkey, record| self.records.get(pubkey) != Some(record)); if !staged.is_empty() { self.commit(staged)?; } diff --git a/docs/cli.md b/docs/cli.md index e9e28a389..c732b1501 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -438,9 +438,9 @@ hold. The keystore routes carry slashing-protection history. `POST` refuses every key in the request if its `slashing_protection` does not parse or is for -another chain, and otherwise writes that history once the keys are saved and -before any is loaded; a validator the history names without a keystore in the -request gets a record too. `DELETE` +another chain, and otherwise writes that history before any key is saved, so a +key never loads ahead of it; a validator the history names without a keystore +in the request gets a record too. `DELETE` returns the deleted keys' records as a minimal EIP-3076 interchange, answering `not_active` for a key that is gone but still has a record. From de3baead44923a5f72961f75b966af70e2336e34 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 19:05:45 -0300 Subject: [PATCH 10/12] Warn when a keymanager import's history raises the slashing-protection watermarks of a validator this client is running. The history may name validators outside the request, and one of them may be loaded here; its watermarks are raised like any other, since a history saying it signed elsewhere is exactly what the record must know, but the operator should learn that the key ran in two places. absorb now returns an Absorbed report (how many validators the history named, how many keys were newly registered, and which existing records it raised) in place of the pair, every caller reads the field it needs, and the import warns for each raised key the store holds. absorb_reports_the_records_it_raised pins that a raised record is reported while an unchanged one and a newly created one are not. --- bin/ethlambda/src/slashing_protection.rs | 2 +- crates/validator/src/http_api/keystores.rs | 36 +++++++--- .../validator/src/slashing_protection/mod.rs | 69 +++++++++++++++++-- 3 files changed, 91 insertions(+), 16 deletions(-) diff --git a/bin/ethlambda/src/slashing_protection.rs b/bin/ethlambda/src/slashing_protection.rs index 1c080b497..044e51250 100644 --- a/bin/ethlambda/src/slashing_protection.rs +++ b/bin/ethlambda/src/slashing_protection.rs @@ -60,7 +60,7 @@ pub(crate) fn run(options: SlashingProtectionOptions) -> eyre::Result<()> { let staged = SlashingProtection::stage(&interchange)?; let mut protection = SlashingProtection::open_offline(&validators_dir, Some(staged.root()))?; - let (validators, _) = protection.absorb(Some(&staged), &[])?; + let validators = protection.absorb(Some(&staged), &[])?.named; info!( validators, file = %file.display(), diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index f266ea9f0..941f18330 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -20,7 +20,11 @@ //! that fails takes the keys off disk again. A key with no history at all is //! registered as never having signed, with a warning. The history may also //! name validators that are not in the request: they get records too, and -//! a later `DELETE` of one answers `not_active`. +//! a later `DELETE` of one answers `not_active`. One of them may be a key +//! this client is running now; its watermarks are raised like any other, +//! since a history saying it signed elsewhere is exactly what the record +//! must know, and the raise is logged so the operator finds out that the +//! key ran in two places. //! - `DELETE` returns the deleted keys' records as a minimal interchange, //! taken after the keys are out of the store, so it covers every signature //! this process made with them. A key that is gone but still has a record is @@ -202,13 +206,27 @@ pub async fn import( protection.absorb(Some(&absorbed), &[]) }) .await - .map(|(named, _)| (staged, named)) + .map(|absorbed| (staged, absorbed)) } Err(err) => Err(err), }; match written { - Ok((staged, named)) => { - info!(validators = named, "Imported slashing-protection history"); + Ok((staged, absorbed)) => { + info!( + validators = absorbed.named, + "Imported slashing-protection history" + ); + // A key running here whose watermarks this history raised + // signed elsewhere while it ran here: worth knowing. + let store = context.store.read().await; + for pubkey in absorbed.raised.iter().filter(|key| store.contains(key)) { + warn!( + pubkey = %encode_hex(&pubkey.0), + "Imported history raised the slashing-protection watermarks of a \ + validator this client is running; it has signed elsewhere too" + ); + } + drop(store); Some(staged) } Err(err) => { @@ -298,7 +316,7 @@ pub async fn import( .collect(); // Nothing to register for a request that saved no key. let registration = if persisted.is_empty() { - Ok((0, 0)) + Ok(crate::slashing_protection::Absorbed::default()) } else { let registering = persisted.clone(); let history = history.clone(); @@ -319,15 +337,15 @@ pub async fn import( "Slashing-protection records written, but the directory could not be flushed; \ the imported keys are registered" ); - Ok((0, 0)) + Ok(crate::slashing_protection::Absorbed::default()) } other => other, }; - if let Ok((_, registered)) = registration - && registered > 0 + if let Ok(absorbed) = ®istration + && absorbed.registered > 0 { warn!( - registered, + registered = absorbed.registered, "Imported validator keys with no slashing-protection history; each starts as \ never having signed" ); diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 4fdc80cb2..4c05f843f 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -202,6 +202,18 @@ impl StagedHistory { } } +/// What [`SlashingProtection::absorb`] did. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct Absorbed { + /// How many distinct validators the history named. + pub named: usize, + /// How many keys were newly registered as never having signed. + pub registered: usize, + /// The validators that already had a record and whose watermarks the + /// history raised: a caller running some of them can say so. + pub raised: Vec, +} + /// What one validator has signed: its watermarks. /// /// `None` is "never signed", kept distinct from zero so that slot 0 and epoch @@ -623,7 +635,8 @@ impl SlashingProtection { /// 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 { - self.absorb(None, pubkeys).map(|(_, registered)| registered) + self.absorb(None, pubkeys) + .map(|absorbed| absorbed.registered) } /// The chain this record belongs to. @@ -657,7 +670,8 @@ impl SlashingProtection { /// validators it named. pub fn import(&mut self, interchange: &str) -> Result { let staged = Self::stage(interchange)?; - self.absorb(Some(&staged), &[]).map(|(named, _)| named) + self.absorb(Some(&staged), &[]) + .map(|absorbed| absorbed.named) } /// Parse `interchange` into what [`Self::absorb`] takes, off any lock: @@ -688,8 +702,9 @@ impl SlashingProtection { /// Merge `history`, if any, as [`Self::import`] describes, and register /// every key in `register` that then still has no record, in one durable - /// write: either all of it is on disk or none of it is. Returns how many - /// validators the history named and how many keys were newly registered. + /// write: either all of it is on disk or none of it is. Reports how many + /// validators the history named, how many keys were newly registered, + /// and which existing records the history raised. /// /// A key in `register` that already has a record and is not in the /// history keeps that record, with a warning: it is what this client last @@ -700,7 +715,7 @@ impl SlashingProtection { &mut self, history: Option<&StagedHistory>, register: &[BlsPubkey], - ) -> Result<(usize, usize)> { + ) -> Result { let mut staged: HashMap = HashMap::new(); if let Some(history) = history { self.check_staged(history)?; @@ -731,10 +746,19 @@ impl SlashingProtection { registered += 1; } staged.retain(|pubkey, record| self.records.get(pubkey) != Some(record)); + let raised = staged + .keys() + .filter(|pubkey| self.records.contains_key(*pubkey)) + .copied() + .collect(); if !staged.is_empty() { self.commit(staged)?; } - Ok((named, registered)) + Ok(Absorbed { + named, + registered, + raised, + }) } /// The records of `pubkeys` as a minimal EIP-3076 version 5 interchange. @@ -1316,6 +1340,39 @@ mod tests { assert!(attest(&mut protection, 1, &data(224, 4, 7, 0xaa)).is_ok()); } + /// `absorb` says which existing records it raised, and only those: a + /// record the history creates, or leaves as it was, is not one. + #[test] + fn absorb_reports_the_records_it_raised() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[1, 2], true).expect("init"); + propose(&mut protection, 1, 10).expect("propose"); + propose(&mut protection, 2, 200).expect("propose"); + + let staged = SlashingProtection::stage(&interchange(0x42, 1, 50, 4, 6)).expect("stages"); + let absorbed = protection.absorb(Some(&staged), &[]).expect("absorbs"); + assert_eq!(absorbed.raised, vec![pubkey(1)]); + assert_eq!(absorbed.named, 1); + + // Already at these watermarks: nothing raised, nothing registered. + let again = protection + .absorb(Some(&staged), &[pubkey(1)]) + .expect("absorbs"); + assert_eq!( + again, + Absorbed { + named: 1, + ..Absorbed::default() + } + ); + + // A record the history creates is not a raised one. + let fresh = SlashingProtection::stage(&interchange(0x42, 3, 50, 4, 6)).expect("stages"); + let created = protection.absorb(Some(&fresh), &[]).expect("absorbs"); + assert!(created.raised.is_empty()); + assert!(protection.contains(&pubkey(3))); + } + /// An older history imported over a newer record must not lower it. #[test] fn an_import_never_lowers_a_watermark() { From 1b4c66e1eef2e9fcdc17a781712030ff2d964dc6 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 19:23:33 -0300 Subject: [PATCH 11/12] Follow the base branch's lock and unflushed-write changes into the interchange. open_offline takes the directory lock through SlashingProtection::lock rather than file::lock, like open does. Two comments settle what Error::Unflushed means on the interchange paths, where the base now accepts it at the keys' registration: the keymanager import's history write still refuses it, since the history is on disk and in memory but were it lost to a power loss a key saved on the strength of it would load at the next start on the record it had before, the case writing it first closes; and a delete whose removal reached the disk but not a flush answers error, so its history stays here like that of any other key not confirmed gone. --- crates/validator/src/http_api/keystores.rs | 11 +++++++++-- crates/validator/src/slashing_protection/mod.rs | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 941f18330..c8356decb 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -229,6 +229,11 @@ pub async fn import( drop(store); Some(staged) } + // `Error::Unflushed` is refused here too, unlike at the keys' + // registration below: the history is on disk and in memory, + // but were it lost to a power loss, a key saved on the + // strength of it would load at the next start on the record + // it had before, which is the case writing it first closes. Err(err) => { let message = format!("slashing_protection was not imported: {err}"); warn!(%err, "Refusing an import whose slashing-protection history was refused"); @@ -783,8 +788,10 @@ pub async fn delete( // record already on disk. Only for the keys that are `deleted` or may // become `not_active`, as the specification requires: a key whose delete // failed is still on disk and comes back on restart, so its history is - // not handed over. Teku and Prysm filter the same way. A key listed twice - // is `deleted` once and `not_found` once; `export` lists it once. + // not handed over, and neither is one whose removal reached the disk but + // not a flush (`Error::Unflushed`, an `error` too), since it may come + // back. Teku and Prysm filter the same way. A key listed twice is + // `deleted` once and `not_found` once; `export` lists it once. let requested: Vec> = results .iter() .map(|(pubkey, status)| match status.status.as_str() { diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 4c05f843f..1de8e06b3 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -500,7 +500,7 @@ impl SlashingProtection { /// directory as it found it. A record created for the wrong chain is not /// dangerous either: [`Self::open`] refuses it at the client's next start. pub fn open_offline(validators_dir: &Path, create_for: Option) -> Result { - let lock = file::lock(validators_dir)?; + let lock = Self::lock(validators_dir)?; let path = validators_dir.join(file::FILE_NAME); let (genesis_validators_root, records) = if file::exists(&path)? { file::read(&path)? From 135c9e2103817cc530cab197631562d3359392fc Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 19:33:03 -0300 Subject: [PATCH 12/12] Refuse an interchange attestation whose source is at or past its target outside genesis, following the base branch's record_of into the import rather than absorbing it: such an entry is never valid, and read into the record it would refuse every honest attestation until the chain justified past that source, so the whole import fails and names the validator. The vector runner applies the README's rule that an importer may refuse a step holding slashable data and skip the rest of the case, as Lighthouse does, which is where the four source_greater_than_target cases now end; the two genesis cases still import (0, 0). an_import_with_a_source_past_its_target_is_refused_whole pins both sides. The import doc, the CLI reference and the spec deviations state the one exception to absorbing slashable data, and the record fixture test unwraps the now fallible record_of. --- .../validator/src/slashing_protection/file.rs | 2 +- .../validator/src/slashing_protection/mod.rs | 32 ++++++++++++++++--- crates/validator/tests/interchange_vectors.rs | 7 ++-- docs/cli.md | 2 +- docs/spec_deviations.md | 5 ++- 5 files changed, 38 insertions(+), 10 deletions(-) diff --git a/crates/validator/src/slashing_protection/file.rs b/crates/validator/src/slashing_protection/file.rs index 04b11ccbf..865a8237c 100644 --- a/crates/validator/src/slashing_protection/file.rs +++ b/crates/validator/src/slashing_protection/file.rs @@ -372,7 +372,7 @@ mod tests { ], }; - let record = record_of(&entry); + let record = record_of(&entry).expect("valid"); assert_eq!( record, diff --git a/crates/validator/src/slashing_protection/mod.rs b/crates/validator/src/slashing_protection/mod.rs index 1de8e06b3..04f7c861d 100644 --- a/crates/validator/src/slashing_protection/mod.rs +++ b/crates/validator/src/slashing_protection/mod.rs @@ -660,10 +660,13 @@ impl SlashingProtection { /// Each validator in it is reduced to its highest slot, source and target /// and merged with any existing record by taking the maximum of each, so an /// import never lowers a watermark, and slashable data inside it (two - /// blocks for one slot, a surround, a source after its target) is absorbed - /// rather than refused: the result is at least as strict as every - /// message the file lists. A validator it names that has no record yet is - /// registered with the imported watermarks. + /// blocks for one slot, a surround) is absorbed rather than refused: the + /// result is at least as strict as every message the file lists. The one + /// shape refused is an attestation whose source is at or past its target + /// outside genesis: never valid, and read into the record it would refuse + /// every honest attestation until the chain justified past that source. + /// A validator it names that has no record yet is registered with the + /// imported watermarks. /// /// All or nothing: an interchange for another chain or of another version, /// or one that does not parse, changes nothing. Returns how many distinct @@ -1373,6 +1376,27 @@ mod tests { assert!(protection.contains(&pubkey(3))); } + /// A source at or past its target is refused whole outside genesis, where + /// `(0, 0)` is what every validator's first attestation looks like. + #[test] + fn an_import_with_a_source_past_its_target_is_refused_whole() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut protection = open(dir.path(), &[], false).expect("opens"); + let err = protection + .import(&interchange(0x42, 1, 97, 5, 2)) + .expect_err("source past target"); + assert!(err.to_string().contains("at or after"), "{err}"); + protection + .import(&interchange(0x42, 1, 97, 5, 5)) + .expect_err("source at target"); + assert!(protection.is_empty(), "nothing of a refused import is held"); + + protection + .import(&interchange(0x42, 1, 0, 0, 0)) + .expect("the genesis attestation imports"); + assert!(attest(&mut protection, 1, &data(32, 0, 1, 0xaa)).is_ok()); + } + /// An older history imported over a newer record must not lower it. #[test] fn an_import_never_lowers_a_watermark() { diff --git a/crates/validator/tests/interchange_vectors.rs b/crates/validator/tests/interchange_vectors.rs index 475013c38..a338d8b73 100644 --- a/crates/validator/tests/interchange_vectors.rs +++ b/crates/validator/tests/interchange_vectors.rs @@ -127,9 +127,10 @@ fn run(case: &Case) -> Result<(), String> { (true, true) | (false, false) => {} // The README lets an importer refuse an interchange holding // slashable data, in which case the rest of the case is skipped. - // This one absorbs it instead, so reaching here is still a - // failure: it would mean an import was refused for some other - // reason. + // This one absorbs all of it but an attestation whose source is at + // or past its target, so a refusal on such a step ends the case, + // as it does in Lighthouse; on any other step it is a failure. + (false, true) if step.contains_slashable_data => return Ok(()), (false, true) => { return Err(format!( "step {index}: import failed but should succeed (slashable data: {}): {}", diff --git a/docs/cli.md b/docs/cli.md index c732b1501..e77aa426a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -473,7 +473,7 @@ ethlambda slashing-protection export --validators-dir ./validators --pubkeys 0xa | Sub-command | Does | |---|---| -| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing; the client refuses the record at its next start if that is not its beacon node's chain | +| `import ` | Merges a version 5 interchange, complete or minimal, into the record. Each validator is reduced to its highest slot, source and target and merged by maximum, so nothing already recorded is lowered and slashable data in the file is absorbed rather than refused, except an attestation whose source is at or past its target outside genesis, which is refused. All or nothing. With no record yet, the import creates one for the interchange's chain, and a failed import creates nothing; the client refuses the record at its next start if that is not its beacon node's chain | | `export [--pubkeys ...] ` | Writes the record, or the named validators' part of it, as a minimal version 5 interchange, readable by its owner only. A named validator with no record fails the export, so a typo or the wrong directory cannot produce a file that reads as complete. Takes the record's lock, so the directory must be writable. Refuses to overwrite an existing file | ## `benchmark import` flags diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 9b1ab4704..49ee15c03 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -118,7 +118,10 @@ anything that does not advance past them. keymanager API's `POST /eth/v1/keystores`) reduces each validator to its highest slot, source and target and merges them with the record by maximum, so slashable data in the file is absorbed rather than refused, as Lighthouse - and Nimbus do. An export (`ethlambda slashing-protection export`, or `DELETE + and Nimbus do. The one exception is an attestation whose source is at or + past its target outside genesis, which fails the whole import: it is never + valid, and read into the record it would refuse every honest attestation + until the chain justified past that source. An export (`ethlambda slashing-protection export`, or `DELETE /eth/v1/keystores`) is the record itself, in the minimal format. Both pass the shared `eth-clients/slashing-protection-interchange-tests` v5.3.0 vectors under their `should_succeed` (minimal) expectations.