From ea7c9d7c85bd5efbd11097722a1ed40c91eff468 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Thu, 5 Nov 2026 16:58:46 -0300 Subject: [PATCH] Persist each validator's fee recipient, graffiti and gas limit on its entry in validator_definitions.yml, in Lighthouse's layout (suggested_fee_recipient, graffiti, gas_limit), so a restart runs with what the keymanager API was given instead of putting every validator back on the command line's defaults. The keymanager's settings routes now write the file under the same definitions lock the keystore routes take, and before the in-memory change: a failed write is a 500 that changes nothing. Startup seeds the settings from the enabled entries, refusing a malformed value the way a bad keystore is refused. A re-imported key keeps the overrides its entry held, and a deleted key's entry takes them with it. The fields may also be written by hand, and a Lighthouse file reads as is. --- crates/validator/src/http_api/keystores.rs | 54 +++- crates/validator/src/http_api/proposer.rs | 301 ++++++++++++++++----- crates/validator/src/keys/definitions.rs | 239 +++++++++++++++- crates/validator/src/keys/store.rs | 6 + crates/validator/src/lib.rs | 12 + crates/validator/src/proposer_settings.rs | 138 ++++++++-- docs/cli.md | 11 +- 7 files changed, 669 insertions(+), 92 deletions(-) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 2232679b3..132e0910f 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -314,6 +314,9 @@ fn persist_import( voting_public_key: base.clone(), voting_keystore_path: keystore_path, voting_keystore_password_path: password_path, + suggested_fee_recipient: None, + graffiti: None, + gas_limit: None, }; // How to put `definitions` back if the save below fails. // @@ -341,10 +344,15 @@ fn persist_import( .iter() .position(|existing| existing.voting_public_key == base) { - Some(index) => Undo::Replaced { - index, - previous: std::mem::replace(&mut definitions.0[index], definition), - }, + Some(index) => { + // A re-imported key keeps the fee recipient, graffiti and gas + // limit its entry held: only the keystore and password move. + let definition = definition.with_overrides_of(&definitions.0[index]); + Undo::Replaced { + index, + previous: std::mem::replace(&mut definitions.0[index], definition), + } + } None => { definitions.0.push(definition); Undo::Appended @@ -718,6 +726,9 @@ mod tests { voting_public_key: encode_hex(&pubkey.0), voting_keystore_path: PathBuf::from("old.json"), voting_keystore_password_path: PathBuf::from("old.txt"), + suggested_fee_recipient: None, + graffiti: Some("kept".to_string()), + gas_limit: None, }; let mut definitions = ValidatorDefinitions(vec![existing.clone()]); @@ -865,6 +876,41 @@ mod tests { assert!(store.read().await.is_empty()); } + /// Re-importing a key (a password rotation, or a retry) rebuilds its + /// entry from the new keystore; the fee recipient, graffiti and gas limit + /// the entry held stay on it. + #[tokio::test] + async fn a_reimported_key_keeps_its_persisted_overrides() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let context = context_with_dirs(store.clone(), dir.path()); + let app = router(context.clone(), TOKEN.to_string()); + let import = serde_json::json!({ + "keystores": [keystore_json()], + "passwords": [PASSWORD], + }); + + let (status, body) = send( + app.clone(), + authed("POST", "/eth/v1/keystores", import.clone()), + ) + .await; + assert_eq!(status, StatusCode::OK, "got {body}"); + let pubkey = store.read().await.pubkeys()[0]; + let set = serde_json::json!({ "graffiti": "kept" }); + let path = format!("/eth/v1/validator/{}/graffiti", encode_hex(&pubkey.0)); + let (status, _) = send(app.clone(), authed("POST", &path, set)).await; + assert_eq!(status, StatusCode::ACCEPTED); + + let (status, body) = send(app, authed("POST", "/eth/v1/keystores", import)).await; + assert_eq!(status, StatusCode::OK, "got {body}"); + assert_eq!(body["data"][0]["status"], "imported", "got {body}"); + + let definitions = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(definitions.0.len(), 1, "one entry, not a duplicate"); + assert_eq!(definitions.0[0].graffiti.as_deref(), Some("kept")); + } + /// The regression test for the gap that let the keymanager mutate only /// the in-memory store: a fresh `ValidatorStore::load` from the same /// directory, independent of the one the API mutated, is the only way to diff --git a/crates/validator/src/http_api/proposer.rs b/crates/validator/src/http_api/proposer.rs index beec84d3c..4ef899fe9 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -1,10 +1,10 @@ //! `GET`, `POST` and `DELETE /eth/v1/validator/{pubkey}/feerecipient`, //! `/graffiti` and `/gas_limit`. //! -//! Each overrides one validator's setting in [`ProposerSettings`], which holds -//! them in memory only: a restart puts every validator back on the command -//! line's defaults. See [`crate::proposer_settings`] for why, and for when a -//! change takes effect. +//! Each overrides one validator's setting in [`ProposerSettings`], and first +//! writes it onto that validator's entry in `validator_definitions.yml`, so a +//! restart runs with it. See [`crate::proposer_settings`] for when a change +//! takes effect. //! //! `GET` answers the validator's own value, or the process-wide default when it //! has none, as the specification asks. `DELETE` drops the validator's own @@ -15,24 +15,26 @@ //! the change happen under one read of the validator store, so a keystore //! delete, which takes that store's write lock and then forgets the key's //! settings, cannot land between them and leave an override behind for a key -//! that is gone. +//! that is gone. The file is written under the same `definitions_lock` the +//! keystore routes take, and before memory changes: a failed write is a 500 +//! that changes nothing, so what the operator sees is what the next restart +//! loads. use axum::Json; use axum::extract::rejection::JsonRejection; use axum::extract::{Path, State}; use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; -use ethlambda_types::beacon::primitives::{BlsPubkey, ExecutionAddress, H160}; +use ethlambda_types::beacon::primitives::BlsPubkey; use serde::{Deserialize, Serialize}; use tracing::info; use crate::beacon_node::dto::{encode_hex, parse_pubkey}; use crate::http_api::KeymanagerContext; -use crate::keys::ValidatorStore; -use crate::proposer_settings::{GRAFFITI_BYTES, graffiti_from_text, graffiti_to_text}; - -/// An execution address is exactly this wide. -const ADDRESS_BYTES: usize = 20; +use crate::keys::{ValidatorDefinitions, ValidatorStore}; +use crate::proposer_settings::{ + GRAFFITI_BYTES, SettingChange, fee_recipient_from_text, graffiti_from_text, graffiti_to_text, +}; /// The keymanager API's error body, `{"message": ...}`. #[derive(Debug)] @@ -117,22 +119,53 @@ fn require_held(store: &ValidatorStore, pubkey: &BlsPubkey) -> Result<(), ApiErr } } -/// `0x` and forty hex digits, the specification's `EthAddress` pattern. -fn parse_address(text: &str) -> Result { - let Some(digits) = text.strip_prefix("0x") else { - return Err(ApiError::BadRequest( - "ethaddress must be 0x-prefixed hex".to_string(), - )); - }; - let bytes = hex::decode(digits) - .map_err(|err| ApiError::BadRequest(format!("ethaddress is not hex: {err}")))?; - let bytes: [u8; ADDRESS_BYTES] = bytes.try_into().map_err(|got: Vec| { - ApiError::BadRequest(format!( - "ethaddress is {} bytes, expected {ADDRESS_BYTES}", - got.len() - )) - })?; - Ok(H160(bytes)) +/// Persist `change` on `pubkey`'s definitions entry, then apply it in memory. +/// +/// Locks in the order the keystore routes take them: `definitions_lock` +/// first, which serialises this open-mutate-save cycle against a concurrent +/// import or delete, then `store`'s read lock for the held check, kept until +/// the change is applied so a delete cannot land in between. +/// +/// A held key with no entry in the file can only mean the file was edited +/// under a running client. That is a 500 naming the file rather than a +/// memory-only change the operator would believe persisted. +async fn persist_and_apply( + context: &KeymanagerContext, + pubkey: &BlsPubkey, + change: SettingChange, +) -> Result<(), ApiError> { + let _definitions_guard = context.definitions_lock.lock().await; + let store = context.store.read().await; + require_held(&store, pubkey)?; + + let path = context + .validators_dir + .join(crate::keys::definitions::DEFINITIONS_FILE); + let mut definitions = ValidatorDefinitions::open(&context.validators_dir) + .map_err(|err| ApiError::Internal(format!("could not read {}: {err}", path.display())))?; + let entry = definitions + .0 + .iter_mut() + .find(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == *pubkey) + .unwrap_or(false) + }) + .ok_or_else(|| { + ApiError::Internal(format!( + "validator {} is loaded but has no entry in {}; the file was changed under a \ + running client, so the change was not made", + encode_hex(&pubkey.0), + path.display() + )) + })?; + entry.apply(change); + definitions + .save(&context.validators_dir) + .map_err(|err| ApiError::Internal(format!("could not write {}: {err}", path.display())))?; + + context.settings.apply(pubkey, change); + Ok(()) } /// A decimal `u64` with no sign and no leading zero, the specification's @@ -182,19 +215,15 @@ pub async fn set_fee_recipient( ) -> Result { let pubkey = parse_path_key(&pubkey)?; let Json(request) = body?; - let address = parse_address(&request.ethaddress)?; - // The specification forbids it outright: it would read as an address - // chosen on purpose, and pays nobody. - if address == H160([0; ADDRESS_BYTES]) { - return Err(ApiError::BadRequest( - "the zero address cannot be set as a fee recipient".to_string(), - )); - } + let address = fee_recipient_from_text(&request.ethaddress) + .map_err(|why| ApiError::BadRequest(format!("ethaddress {why}")))?; - let store = context.store.read().await; - require_held(&store, &pubkey)?; - context.settings.set_fee_recipient(&pubkey, address); - drop(store); + persist_and_apply( + &context, + &pubkey, + SettingChange::FeeRecipient(Some(address)), + ) + .await?; info!( pubkey = %encode_hex(&pubkey.0), @@ -209,10 +238,7 @@ pub async fn delete_fee_recipient( Path(pubkey): Path, ) -> Result { let pubkey = parse_path_key(&pubkey)?; - let store = context.store.read().await; - require_held(&store, &pubkey)?; - context.settings.clear_fee_recipient(&pubkey); - drop(store); + persist_and_apply(&context, &pubkey, SettingChange::FeeRecipient(None)).await?; info!(pubkey = %encode_hex(&pubkey.0), "Fee recipient reset to the default"); Ok(StatusCode::NO_CONTENT) @@ -247,10 +273,7 @@ pub async fn set_graffiti( ))); }; - let store = context.store.read().await; - require_held(&store, &pubkey)?; - context.settings.set_graffiti(&pubkey, graffiti); - drop(store); + persist_and_apply(&context, &pubkey, SettingChange::Graffiti(Some(graffiti))).await?; info!(pubkey = %encode_hex(&pubkey.0), graffiti = ?request.graffiti, "Graffiti set"); Ok(StatusCode::ACCEPTED) @@ -261,10 +284,7 @@ pub async fn delete_graffiti( Path(pubkey): Path, ) -> Result { let pubkey = parse_path_key(&pubkey)?; - let store = context.store.read().await; - require_held(&store, &pubkey)?; - context.settings.clear_graffiti(&pubkey); - drop(store); + persist_and_apply(&context, &pubkey, SettingChange::Graffiti(None)).await?; info!(pubkey = %encode_hex(&pubkey.0), "Graffiti reset to the default"); Ok(StatusCode::NO_CONTENT) @@ -294,10 +314,7 @@ pub async fn set_gas_limit( let Json(request) = body?; let gas_limit = parse_uint64(&request.gas_limit)?; - let store = context.store.read().await; - require_held(&store, &pubkey)?; - context.settings.set_gas_limit(&pubkey, gas_limit); - drop(store); + persist_and_apply(&context, &pubkey, SettingChange::GasLimit(Some(gas_limit))).await?; info!(pubkey = %encode_hex(&pubkey.0), gas_limit, "Gas limit set"); Ok(StatusCode::ACCEPTED) @@ -308,10 +325,7 @@ pub async fn delete_gas_limit( Path(pubkey): Path, ) -> Result { let pubkey = parse_path_key(&pubkey)?; - let store = context.store.read().await; - require_held(&store, &pubkey)?; - context.settings.clear_gas_limit(&pubkey); - drop(store); + persist_and_apply(&context, &pubkey, SettingChange::GasLimit(None)).await?; info!(pubkey = %encode_hex(&pubkey.0), "Gas limit reset to the default"); Ok(StatusCode::NO_CONTENT) @@ -323,14 +337,15 @@ mod tests { use axum::body::Body; use axum::http::Request; - use ethlambda_types::beacon::primitives::Bytes32; + use ethlambda_types::beacon::primitives::{Bytes32, ExecutionAddress, H160}; use http_body_util::BodyExt as _; use tokio::sync::RwLock; use tower::ServiceExt as _; use super::*; use crate::http_api::router; - use crate::proposer_settings::{DEFAULT_GAS_LIMIT, ProposerSettings}; + use crate::keys::ValidatorDefinition; + use crate::proposer_settings::{ADDRESS_BYTES, DEFAULT_GAS_LIMIT, ProposerSettings}; const TOKEN: &str = "test-token"; @@ -338,9 +353,21 @@ mod tests { app: axum::Router, settings: Arc, pubkey: BlsPubkey, - /// Kept alive for the test that deletes a keystore, which writes the - /// definitions file. - _dir: tempfile::TempDir, + /// The validators directory, holding the definitions file the routes + /// write through. + dir: tempfile::TempDir, + } + + impl Harness { + /// The key's entry as the file holds it now. + fn entry(&self) -> ValidatorDefinition { + ValidatorDefinitions::open(self.dir.path()) + .expect("opens") + .0 + .into_iter() + .find(|entry| entry.voting_public_key == encode_hex(&self.pubkey.0)) + .expect("the key's entry") + } } fn secret() -> [u8; 32] { @@ -355,8 +382,27 @@ mod tests { } /// A router over a store holding one key, with a default graffiti of - /// `default` and, when `fee_recipient` is `Some`, a default address. + /// `default` and, when `fee_recipient` is `Some`, a default address. The + /// definitions file names the key, as it would for any loaded key. fn harness(fee_recipient: Option) -> Harness { + let harness = harness_without_definitions(fee_recipient); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: encode_hex(&harness.pubkey.0), + voting_keystore_path: harness.dir.path().join("voting-keystore.json"), + voting_keystore_password_path: harness.dir.path().join("password"), + suggested_fee_recipient: None, + graffiti: None, + gas_limit: None, + }]) + .save(harness.dir.path()) + .expect("writes the definitions file"); + harness + } + + /// [`harness`] with no definitions file at all: a key in memory the file + /// does not describe. + fn harness_without_definitions(fee_recipient: Option) -> Harness { let mut store = ValidatorStore::new(); let pubkey = store.insert_secret("test", &secret()).expect("inserts"); let settings = Arc::new(ProposerSettings::new(default_graffiti(), fee_recipient)); @@ -372,7 +418,7 @@ mod tests { app: router(context, TOKEN.to_string()), settings, pubkey, - _dir: dir, + dir, } } @@ -611,6 +657,129 @@ mod tests { assert!(body["message"].is_string(), "got {body}"); } + /// Every change lands on the key's entry in the definitions file before + /// it is answered, and a delete takes the field out again. + #[tokio::test] + async fn a_change_is_written_to_the_definitions_file_and_a_delete_removes_it() { + let harness = harness(Some(default_address())); + let own = format!("0x{}", "bb".repeat(ADDRESS_BYTES)); + + let set = serde_json::json!({ "ethaddress": own }); + let path = uri(&harness.pubkey, "feerecipient"); + let (status, _) = send(&harness.app, request("POST", &path, Some(set))).await; + assert_eq!(status, StatusCode::ACCEPTED); + assert_eq!( + harness.entry().suggested_fee_recipient.as_deref(), + Some(own.as_str()) + ); + + let set = serde_json::json!({ "graffiti": "hello" }); + let path = uri(&harness.pubkey, "graffiti"); + send(&harness.app, request("POST", &path, Some(set))).await; + assert_eq!(harness.entry().graffiti.as_deref(), Some("hello")); + + let set = serde_json::json!({ "gas_limit": "30000000" }); + let path = uri(&harness.pubkey, "gas_limit"); + send(&harness.app, request("POST", &path, Some(set))).await; + assert_eq!(harness.entry().gas_limit, Some(30_000_000)); + + for setting in ["feerecipient", "graffiti", "gas_limit"] { + let request = request("DELETE", &uri(&harness.pubkey, setting), None); + let (status, _) = send(&harness.app, request).await; + assert_eq!(status, StatusCode::NO_CONTENT, "DELETE {setting}"); + } + let entry = harness.entry(); + assert_eq!(entry.suggested_fee_recipient, None); + assert_eq!(entry.graffiti, None); + assert_eq!(entry.gas_limit, None); + } + + /// What a restart does with the file: the overrides a previous run + /// persisted are the ones the next run proposes with. + #[tokio::test] + async fn persisted_overrides_survive_a_restart() { + let harness = harness(Some(default_address())); + let own = format!("0x{}", "bb".repeat(ADDRESS_BYTES)); + let set = serde_json::json!({ "ethaddress": own }); + let path = uri(&harness.pubkey, "feerecipient"); + send(&harness.app, request("POST", &path, Some(set))).await; + let set = serde_json::json!({ "graffiti": "persisted" }); + let path = uri(&harness.pubkey, "graffiti"); + send(&harness.app, request("POST", &path, Some(set))).await; + + // A fresh `ProposerSettings`, as a restart builds, seeded as startup + // seeds it. + let restarted = ProposerSettings::new(default_graffiti(), Some(default_address())); + let seeded = ValidatorDefinitions::open(harness.dir.path()) + .expect("opens") + .seed_overrides(&restarted) + .expect("seeds"); + assert_eq!(seeded, 1); + assert_eq!( + restarted.fee_recipient(&harness.pubkey), + Some(H160([0xbb; ADDRESS_BYTES])) + ); + assert_eq!( + restarted.graffiti(&harness.pubkey), + graffiti_from_text("persisted").expect("fits") + ); + assert_eq!(restarted.gas_limit(&harness.pubkey), DEFAULT_GAS_LIMIT); + } + + /// A loaded key the file does not describe means the file was changed + /// under the client. The change is refused rather than kept in memory + /// alone, where the operator would believe it persisted. + #[tokio::test] + async fn a_key_missing_from_the_definitions_file_is_a_500_and_memory_is_unchanged() { + let harness = harness_without_definitions(None); + let set = serde_json::json!({ "graffiti": "hello" }); + let path = uri(&harness.pubkey, "graffiti"); + let (status, body) = send(&harness.app, request("POST", &path, Some(set))).await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); + assert!( + body["message"] + .as_str() + .is_some_and(|message| message.contains("validator_definitions.yml")), + "got {body}" + ); + assert_eq!( + harness.settings.graffiti(&harness.pubkey), + default_graffiti(), + "a change that was not persisted must not take effect" + ); + let (status, _) = send(&harness.app, request("DELETE", &path, None)).await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); + } + + /// A file that cannot be written is a 500 that changes nothing. + #[cfg(unix)] + #[tokio::test] + async fn a_file_that_cannot_be_written_is_a_500_and_memory_is_unchanged() { + use std::os::unix::fs::PermissionsExt as _; + + let harness = harness(None); + // Saving writes a sibling temporary file, which a read-only directory + // refuses. Root is exempt from permission bits, so the test is moot + // there rather than wrong. + if unsafe { libc::geteuid() } == 0 { + return; + } + std::fs::set_permissions(harness.dir.path(), std::fs::Permissions::from_mode(0o500)) + .expect("chmod"); + + let set = serde_json::json!({ "graffiti": "hello" }); + let path = uri(&harness.pubkey, "graffiti"); + let (status, _) = send(&harness.app, request("POST", &path, Some(set))).await; + std::fs::set_permissions(harness.dir.path(), std::fs::Permissions::from_mode(0o700)) + .expect("chmod back, so the directory can be removed"); + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!( + harness.settings.graffiti(&harness.pubkey), + default_graffiti() + ); + assert_eq!(harness.entry().graffiti, None, "the file is unchanged too"); + } + /// A deleted key's overrides go with it, so the same key imported again /// starts on the defaults. #[tokio::test] diff --git a/crates/validator/src/keys/definitions.rs b/crates/validator/src/keys/definitions.rs index ab48a8db0..d19006fff 100644 --- a/crates/validator/src/keys/definitions.rs +++ b/crates/validator/src/keys/definitions.rs @@ -2,13 +2,19 @@ //! //! One YAML-free, JSON-Lines-free plain list, deliberately separate from the //! keystores themselves. It is what the keymanager API mutates, so a key -//! imported at runtime is still there after a restart. +//! imported at runtime is still there after a restart, and so are the fee +//! recipient, graffiti and gas limit it was given. use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; +use crate::beacon_node::dto::parse_pubkey; use crate::error::{Error, Result}; +use crate::proposer_settings::{ + GRAFFITI_BYTES, Overrides, ProposerSettings, SettingChange, fee_recipient_from_text, + fee_recipient_to_text, graffiti_from_text, graffiti_to_text, +}; use crate::secure_fs; /// The file's name inside the validators directory. @@ -25,6 +31,80 @@ pub struct ValidatorDefinition { pub voting_keystore_path: PathBuf, /// Path to the file holding that keystore's password. pub voting_keystore_password_path: PathBuf, + /// This validator's own fee recipient, `0x`-prefixed hex, in place of + /// `--suggested-fee-recipient`. The three fields below are Lighthouse's, + /// name for name and form for form, so its file reads here and this one + /// reads there. Each is absent from the file, not `null`, when unset. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub suggested_fee_recipient: Option, + /// This validator's own graffiti, as text, in place of `--graffiti`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub graffiti: Option, + /// This validator's own gas limit. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub gas_limit: Option, +} + +impl ValidatorDefinition { + /// The entry's three overrides, parsed. A value the file holds in the + /// wrong form is an error naming the entry and the field: a validator + /// proposing to an address the operator did not mean is worse than a + /// refused start. + pub fn overrides(&self) -> Result { + let reject = |field: &str, reason: String| Error::Keystore { + path: format!("validator_definitions.yml entry {}", self.voting_public_key), + reason: format!("{field} {reason}"), + }; + let fee_recipient = self + .suggested_fee_recipient + .as_deref() + .map(|text| { + fee_recipient_from_text(text).map_err(|why| reject("suggested_fee_recipient", why)) + }) + .transpose()?; + let graffiti = self + .graffiti + .as_deref() + .map(|text| { + graffiti_from_text(text).ok_or_else(|| { + reject( + "graffiti", + format!( + "is {} bytes encoded as UTF-8; the field holds {GRAFFITI_BYTES}", + text.len() + ), + ) + }) + }) + .transpose()?; + Ok(Overrides { + fee_recipient, + graffiti, + gas_limit: self.gas_limit, + }) + } + + /// Write one change into the entry, in the form the file carries. + pub fn apply(&mut self, change: SettingChange) { + match change { + SettingChange::FeeRecipient(address) => { + self.suggested_fee_recipient = address.as_ref().map(fee_recipient_to_text); + } + SettingChange::Graffiti(graffiti) => { + self.graffiti = graffiti.as_ref().map(graffiti_to_text); + } + SettingChange::GasLimit(gas_limit) => self.gas_limit = gas_limit, + } + } + + /// Carry another entry's overrides onto this one, for a re-imported key + /// whose entry is rebuilt from the new keystore. + pub fn with_overrides_of(mut self, other: &ValidatorDefinition) -> Self { + self.suggested_fee_recipient = other.suggested_fee_recipient.clone(); + self.graffiti = other.graffiti.clone(); + self.gas_limit = other.gas_limit; + self + } } #[derive(Debug, Clone, Default, Serialize, Deserialize)] @@ -127,6 +207,9 @@ impl ValidatorDefinitions { voting_keystore_password_path: secrets_dir.join(&pubkey), voting_public_key: pubkey, voting_keystore_path: keystore_path, + suggested_fee_recipient: None, + graffiti: None, + gas_limit: None, }); } // Directory order is not stable across filesystems; sorting makes the @@ -142,6 +225,31 @@ impl ValidatorDefinitions { pub fn enabled(&self) -> impl Iterator { self.0.iter().filter(|definition| definition.enabled) } + + /// Install every enabled entry's overrides into `settings`, returning how + /// many entries carried one. What startup does once the keys are loaded. + /// A disabled entry keeps its fields in the file for when it is enabled + /// again, and is skipped here as its key is skipped by `load`. + pub fn seed_overrides(&self, settings: &ProposerSettings) -> Result { + let mut seeded = 0; + for definition in self.enabled() { + let overrides = definition.overrides()?; + if overrides.is_empty() { + continue; + } + let pubkey = + parse_pubkey(&definition.voting_public_key).map_err(|err| Error::Keystore { + path: format!( + "validator_definitions.yml entry {}", + definition.voting_public_key + ), + reason: format!("the voting_public_key is unreadable: {err}"), + })?; + settings.seed(&pubkey, overrides); + seeded += 1; + } + Ok(seeded) + } } #[cfg(test)] @@ -212,9 +320,138 @@ mod tests { voting_public_key: pubkey.to_string(), voting_keystore_path: PathBuf::from("keystore.json"), voting_keystore_password_path: PathBuf::from("password.txt"), + suggested_fee_recipient: None, + graffiti: None, + gas_limit: None, } } + /// An entry with no overrides is written exactly as it always was: the + /// three fields are absent, not `null`, so a file this client writes + /// stays readable by a reader that predates them. + #[test] + fn an_entry_without_overrides_is_written_without_the_fields() { + let yaml = serde_yaml_ng::to_string(&ValidatorDefinitions(vec![definition("0xaa", true)])) + .expect("serializes"); + assert!(!yaml.contains("suggested_fee_recipient"), "{yaml}"); + assert!(!yaml.contains("graffiti"), "{yaml}"); + assert!(!yaml.contains("gas_limit"), "{yaml}"); + } + + #[test] + fn overrides_round_trip_through_the_file() { + use ethlambda_types::beacon::primitives::H160; + + let dir = tempfile::tempdir().expect("temp dir"); + let mut entry = definition("0xaa", true); + entry.apply(SettingChange::FeeRecipient(Some(H160([0xbb; 20])))); + entry.apply(SettingChange::Graffiti(Some( + graffiti_from_text("hello").expect("fits"), + ))); + entry.apply(SettingChange::GasLimit(Some(30_000_000))); + ValidatorDefinitions(vec![entry.clone(), definition("0xbb", true)]) + .save(dir.path()) + .expect("saves"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(read.0[0], entry); + assert_eq!( + read.0[0].overrides().expect("parses"), + Overrides { + fee_recipient: Some(H160([0xbb; 20])), + graffiti: Some(graffiti_from_text("hello").expect("fits")), + gas_limit: Some(30_000_000), + } + ); + assert_eq!(read.0[1].overrides().expect("parses"), Overrides::default()); + } + + /// Lighthouse's own file, fields it writes that this client does not + /// included, reads here with the three overrides picked up. + #[test] + fn a_lighthouse_written_entry_parses_with_its_overrides() { + use ethlambda_types::beacon::primitives::H160; + + let yaml = format!( + "- enabled: true + voting_public_key: 0x{} + description: \"\" + \ + graffiti: from lighthouse + suggested_fee_recipient: 0x{} + gas_limit: 36000000 + \ + builder_proposals: false + type: local_keystore + \ + voting_keystore_path: /keys/voting-keystore.json + \ + voting_keystore_password_path: /secrets/password +", + "ab".repeat(48), + "cd".repeat(20) + ); + let definitions: ValidatorDefinitions = serde_yaml_ng::from_str(&yaml).expect("parses"); + let overrides = definitions.0[0].overrides().expect("valid"); + assert_eq!(overrides.fee_recipient, Some(H160([0xcd; 20]))); + assert_eq!( + overrides.graffiti, + Some(graffiti_from_text("from lighthouse").expect("fits")) + ); + assert_eq!(overrides.gas_limit, Some(36_000_000)); + } + + #[test] + fn a_malformed_override_is_an_error_naming_the_field() { + let mut entry = definition("0xaa", true); + entry.suggested_fee_recipient = Some("0x1234".to_string()); + let err = entry.overrides().expect_err("two bytes is not an address"); + assert!( + err.to_string().contains("suggested_fee_recipient") && err.to_string().contains("0xaa"), + "got {err}" + ); + + let mut entry = definition("0xaa", true); + entry.graffiti = Some("a".repeat(GRAFFITI_BYTES + 1)); + let err = entry.overrides().expect_err("33 bytes does not fit"); + assert!(err.to_string().contains("graffiti"), "got {err}"); + } + + /// Startup seeding: enabled entries' overrides land in the settings, a + /// disabled entry's do not. + #[test] + fn seeding_installs_enabled_entries_overrides_only() { + use ethlambda_types::beacon::primitives::{BlsPubkey, Bytes32, H160}; + + let enabled_key = format!("0x{}", "11".repeat(48)); + let disabled_key = format!("0x{}", "22".repeat(48)); + let mut enabled = definition(&enabled_key, true); + enabled.apply(SettingChange::FeeRecipient(Some(H160([0xbb; 20])))); + let mut disabled = definition(&disabled_key, false); + disabled.apply(SettingChange::GasLimit(Some(1))); + let definitions = ValidatorDefinitions(vec![enabled, disabled, definition("0xcc", true)]); + + let settings = ProposerSettings::new(Bytes32::ZERO, None); + let seeded = definitions.seed_overrides(&settings).expect("seeds"); + assert_eq!(seeded, 1); + assert_eq!( + settings.fee_recipient(&BlsPubkey([0x11; 48])), + Some(H160([0xbb; 20])) + ); + assert_eq!( + settings.overrides(&BlsPubkey([0x22; 48])), + Overrides::default() + ); + } + + #[test] + fn clearing_an_override_removes_the_field() { + let mut entry = definition("0xaa", true); + entry.apply(SettingChange::GasLimit(Some(1))); + entry.apply(SettingChange::GasLimit(None)); + assert_eq!(entry, definition("0xaa", true)); + } + #[test] fn a_missing_file_is_an_empty_set_not_an_error() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/crates/validator/src/keys/store.rs b/crates/validator/src/keys/store.rs index 216855253..01ad461e3 100644 --- a/crates/validator/src/keys/store.rs +++ b/crates/validator/src/keys/store.rs @@ -280,6 +280,9 @@ mod tests { voting_public_key: "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07".to_string(), voting_keystore_path: PathBuf::from("keystore.json"), voting_keystore_password_path: PathBuf::from("password.txt"), + suggested_fee_recipient: None, + graffiti: None, + gas_limit: None, }]); definitions.save(dir.path()).expect("saves definitions"); @@ -306,6 +309,9 @@ mod tests { voting_public_key: voting_public_key.to_string(), voting_keystore_path: PathBuf::from("keystore.json"), voting_keystore_password_path: PathBuf::from("password.txt"), + suggested_fee_recipient: None, + graffiti: None, + gas_limit: None, }]) .save(dir) .expect("saves definitions"); diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index ebec73b1f..3e0742950 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -150,6 +150,18 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { if store.is_empty() { warn!("No validators loaded; the client will idle"); } + // The per-validator fee recipients, graffiti and gas limits the keymanager + // persisted, or the operator wrote by hand. A malformed one is fatal, as a + // bad keystore is: a validator paying an address the operator did not + // mean is worse than a refused start. + let seeded = + keys::ValidatorDefinitions::open(&config.validators_dir)?.seed_overrides(&settings)?; + if seeded > 0 { + info!( + count = seeded, + "Loaded per-validator proposer settings from the definitions file" + ); + } metrics::set_validators_loaded(store.len() as u64); // The keymanager mutates this at runtime while the duty loop reads it, so // it is shared rather than owned outright from here on. diff --git a/crates/validator/src/proposer_settings.rs b/crates/validator/src/proposer_settings.rs index 01daf3f4d..71b5aa60c 100644 --- a/crates/validator/src/proposer_settings.rs +++ b/crates/validator/src/proposer_settings.rs @@ -4,13 +4,15 @@ //! The keymanager API's `feerecipient`, `graffiti` and `gas_limit` endpoints //! override them per key, and `DELETE` puts a key back on the defaults. //! -//! # In memory only +//! # The definitions file is the record; this is the cache //! -//! Overrides are not written anywhere, so a restart puts every validator back -//! on the defaults. Unlike an imported key, which must survive a restart or a -//! validator silently stops signing, an override that is lost is at worst a -//! block paying the default address or carrying the default text, and an -//! operator driving these endpoints can re-apply them after a restart. +//! Each override is persisted on its validator's entry in +//! `validator_definitions.yml` (`suggested_fee_recipient`, `graffiti`, +//! `gas_limit`, Lighthouse's layout, so a file written by either client reads +//! in the other). The keymanager writes the file before it changes anything +//! here, and startup seeds this from the file, so what an operator set is what +//! the next restart runs with. An operator may also put the fields in the file +//! by hand before starting the client. //! //! # When a change takes effect //! @@ -24,7 +26,7 @@ use std::collections::HashMap; use std::sync::{PoisonError, RwLock}; -use ethlambda_types::beacon::primitives::{BlsPubkey, Bytes32, ExecutionAddress, H256}; +use ethlambda_types::beacon::primitives::{BlsPubkey, Bytes32, ExecutionAddress, H160, H256}; /// A block's graffiti field is exactly this wide. pub const GRAFFITI_BYTES: usize = 32; @@ -64,18 +66,63 @@ pub fn graffiti_to_text(graffiti: &Bytes32) -> String { /// held so the keymanager API can serve it as the specification requires. pub const DEFAULT_GAS_LIMIT: u64 = 60_000_000; +/// `text` as a fee recipient: `0x` and forty hex digits, the keymanager +/// specification's `EthAddress` pattern. The zero address is refused: the +/// specification forbids it outright, since it would read as an address +/// chosen on purpose and pays nobody. +pub fn fee_recipient_from_text(text: &str) -> Result { + let Some(digits) = text.strip_prefix("0x") else { + return Err("must be 0x-prefixed hex".to_string()); + }; + let bytes = hex::decode(digits).map_err(|err| format!("is not hex: {err}"))?; + let bytes: [u8; ADDRESS_BYTES] = bytes + .try_into() + .map_err(|got: Vec| format!("is {} bytes, expected {ADDRESS_BYTES}", got.len()))?; + if bytes == [0; ADDRESS_BYTES] { + return Err("is the zero address, which cannot be a fee recipient".to_string()); + } + Ok(H160(bytes)) +} + +/// A fee recipient as the `0x`-prefixed hex the file and the API carry. +pub fn fee_recipient_to_text(address: &ExecutionAddress) -> String { + format!("0x{}", hex::encode(address.0)) +} + +/// An execution address is exactly this wide. +pub const ADDRESS_BYTES: usize = 20; + /// One validator's overrides; `None` means the default applies. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] -struct Overrides { - fee_recipient: Option, - graffiti: Option, - gas_limit: Option, +pub struct Overrides { + pub fee_recipient: Option, + pub graffiti: Option, + pub gas_limit: Option, } impl Overrides { - fn is_empty(&self) -> bool { + pub fn is_empty(&self) -> bool { *self == Self::default() } + + /// `change` applied on top of these. + pub fn apply(&mut self, change: SettingChange) { + match change { + SettingChange::FeeRecipient(address) => self.fee_recipient = address, + SettingChange::Graffiti(graffiti) => self.graffiti = graffiti, + SettingChange::GasLimit(gas_limit) => self.gas_limit = gas_limit, + } + } +} + +/// One change to one validator's overrides: a new value, or `None` to put +/// that setting back on the default. One type for the three, so the +/// keymanager's persist-then-apply step is written once rather than six times. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SettingChange { + FeeRecipient(Option), + Graffiti(Option), + GasLimit(Option), } #[derive(Debug)] @@ -154,28 +201,45 @@ impl ProposerSettings { }) } + /// Every override `pubkey` holds, defaults left as `None`. + pub fn overrides(&self, pubkey: &BlsPubkey) -> Overrides { + self.read(|inner| inner.overrides.get(pubkey).copied().unwrap_or_default()) + } + + /// Apply one change to `pubkey`'s overrides. The keymanager calls this + /// only after the same change is in the definitions file. + pub fn apply(&self, pubkey: &BlsPubkey, change: SettingChange) { + self.update(pubkey, |entry| entry.apply(change)); + } + + /// Install `pubkey`'s overrides wholesale, replacing any it had. What + /// startup does with each entry read from the definitions file. + pub fn seed(&self, pubkey: &BlsPubkey, overrides: Overrides) { + self.update(pubkey, |entry| *entry = overrides); + } + pub fn set_fee_recipient(&self, pubkey: &BlsPubkey, address: ExecutionAddress) { - self.update(pubkey, |entry| entry.fee_recipient = Some(address)); + self.apply(pubkey, SettingChange::FeeRecipient(Some(address))); } pub fn clear_fee_recipient(&self, pubkey: &BlsPubkey) { - self.update(pubkey, |entry| entry.fee_recipient = None); + self.apply(pubkey, SettingChange::FeeRecipient(None)); } pub fn set_graffiti(&self, pubkey: &BlsPubkey, graffiti: Bytes32) { - self.update(pubkey, |entry| entry.graffiti = Some(graffiti)); + self.apply(pubkey, SettingChange::Graffiti(Some(graffiti))); } pub fn clear_graffiti(&self, pubkey: &BlsPubkey) { - self.update(pubkey, |entry| entry.graffiti = None); + self.apply(pubkey, SettingChange::Graffiti(None)); } pub fn set_gas_limit(&self, pubkey: &BlsPubkey, gas_limit: u64) { - self.update(pubkey, |entry| entry.gas_limit = Some(gas_limit)); + self.apply(pubkey, SettingChange::GasLimit(Some(gas_limit))); } pub fn clear_gas_limit(&self, pubkey: &BlsPubkey) { - self.update(pubkey, |entry| entry.gas_limit = None); + self.apply(pubkey, SettingChange::GasLimit(None)); } /// Drop every override `pubkey` holds, for a key this client no longer @@ -276,6 +340,44 @@ mod tests { assert!(graffiti_from_text(&"ñ".repeat(17)).is_none()); } + #[test] + fn seeding_replaces_a_keys_overrides_wholesale() { + let settings = settings(); + settings.set_gas_limit(&key(1), 30_000_000); + settings.seed( + &key(1), + Overrides { + fee_recipient: Some(H160([0xbb; 20])), + graffiti: None, + gas_limit: None, + }, + ); + assert_eq!(settings.fee_recipient(&key(1)), Some(H160([0xbb; 20]))); + assert_eq!( + settings.gas_limit(&key(1)), + DEFAULT_GAS_LIMIT, + "an override the seed does not carry is gone" + ); + settings.seed(&key(1), Overrides::default()); + assert!(settings.read(|inner| inner.overrides.is_empty())); + } + + #[test] + fn a_fee_recipient_round_trips_through_text_and_the_zero_address_is_refused() { + let text = format!("0x{}", "ab".repeat(ADDRESS_BYTES)); + let address = fee_recipient_from_text(&text).expect("valid"); + assert_eq!(address, H160([0xab; ADDRESS_BYTES])); + assert_eq!(fee_recipient_to_text(&address), text); + for bad in [ + "ab".repeat(ADDRESS_BYTES), + format!("0x{}", "ab".repeat(ADDRESS_BYTES - 1)), + format!("0x{}", "zz".repeat(ADDRESS_BYTES)), + format!("0x{}", "00".repeat(ADDRESS_BYTES)), + ] { + assert!(fee_recipient_from_text(&bad).is_err(), "{bad}"); + } + } + #[test] fn a_key_cleared_of_every_override_leaves_no_entry() { let settings = settings(); diff --git a/docs/cli.md b/docs/cli.md index 2dcbf6703..fb893889a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -415,9 +415,14 @@ give one validator its own value in place of the command line's default: | `graffiti` | `--graffiti` | The next proposal | | `gas_limit` | `DEFAULT_GAS_LIMIT` | Stored and served only: it matters to builder registrations, and there is no builder flow | -**These are held in memory only.** A restart puts every validator back on the -defaults, so an operator driving these endpoints must set them again after one. -Deleting a keystore drops its values too. +**These persist.** Each is written onto the validator's entry in +`validator_definitions.yml` (`suggested_fee_recipient`, `graffiti`, `gas_limit`, +Lighthouse's layout, so a file written by either client reads in the other) +before the request is answered, and loaded again at startup, so a restart runs +with what the operator set. A write that fails is a `500` that changes nothing. +The fields may also be put in the file by hand before the client starts; a +malformed one refuses startup. Deleting a keystore drops its entry and its +values with it. A few answers worth knowing: `GET feerecipient` is a `500` when the validator has no address and there is no `--suggested-fee-recipient` default, as