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