diff --git a/CLAUDE.md b/CLAUDE.md index d08ee404f..c2a845f45 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -849,7 +849,27 @@ transitions are in `ethlambda-types`, per the section above. Nothing above keeps EIP-3076 minimal-watermark slashing protection in `--validators-dir/slashing_protection.json` (`crate::slashing_protection`), durable before every block and attestation signature; PTC votes have an - in-memory `(validator, slot)` dedup only, and envelopes are unguarded. + in-memory `(validator, slot)` dedup only, and envelopes are unguarded. A key can live + in a Web3Signer instead of the process (`SigningMethod::Web3Signer`, + `keys/web3signer.rs`): every `SigningContext::sign_*` is async, resolves the + signer under a read guard scoped to the lookup and signs after releasing it, + and sends the remote signer the message in full so its own slashing + protection applies on top of this client's record, which a remote key is + registered in and checked against like a local one; the signature the + signer returns is verified against the pubkey and root before it is used, + on the blocking pool (`SigningContext::verified`), never inline on the duty + task. Each duty signs its batch at once through + `signing::sign_all`, every signature bounded by `SIGNING_BUDGET`, so one + hung signer costs only its own keys; one signer is sent at most + `MAX_CONCURRENT_REQUESTS` at a time. The epoch's subnet subscriptions, whose + selection proofs are round trips for remote keys, are sent from a spawned + task after the slot's proposal, not from `refresh_epoch`. Remote keys are + `type: web3signer` + entries in `validator_definitions.yml` or `POST /eth/v1/remotekeys` (default signer + `--web3signer-url`); `/keystores` and `/remotekeys` partition the store, and + both deletes go through `keystores::delete_keys` (one read and one save of + the definitions file per request, the store's write lock held only for the + in-memory removals). `get_head_node` is one bottom-up walk for every fork, with the current slot's fork selecting the payload rules and the boost gate, so a follower on any network that schedules gloas (Sepolia does) pays work diff --git a/Cargo.lock b/Cargo.lock index 0bec6c9e0..61e826be2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2272,6 +2272,7 @@ dependencies = [ "ctr", "ethlambda-metrics", "ethlambda-types", + "futures", "hex", "hmac", "http-body-util", diff --git a/bin/ethlambda/src/validator.rs b/bin/ethlambda/src/validator.rs index 968244963..d1b3051c4 100644 --- a/bin/ethlambda/src/validator.rs +++ b/bin/ethlambda/src/validator.rs @@ -8,6 +8,7 @@ use std::path::PathBuf; use ethlambda_types::beacon::primitives::{Bytes32, ExecutionAddress, H160}; use ethlambda_validator::ValidatorConfig; use ethlambda_validator::proposer_settings::{GRAFFITI_BYTES, graffiti_from_text}; +use eyre::WrapErr as _; /// An execution address is exactly this wide. const ADDRESS_BYTES: usize = 20; @@ -79,6 +80,14 @@ pub(crate) struct ValidatorOptions { #[arg(long, default_value = "false")] pub(crate) enable_keymanager: bool, + /// Base URL of a Web3Signer remote signer (e.g. https://signer:9000), + /// the default for remote keys imported through the keymanager API's + /// `POST /eth/v1/remotekeys` without a `url` of their own. + /// + /// Optional: a remote key may name its own signer in that request or in + /// `validator_definitions.yml`, and local keystores never need one. + #[arg(long)] + pub(crate) web3signer_url: Option, /// Register every loaded key that has no slashing-protection record, /// instead of refusing to start. /// @@ -155,6 +164,13 @@ impl ValidatorOptions { #[tokio::main] pub(crate) async fn run(options: ValidatorOptions) -> eyre::Result<()> { options.validate_ports()?; + // The same raise `run_node` makes, and for a like reason: with remote + // keys every signing request in flight is a socket (capped per signer, + // see `Web3Signer`), next to the beacon nodes', the metrics server's and + // the keymanager's. On a default soft limit `connect` starts failing + // with `EMFILE` once several signers are busy at once, and every + // signature those requests were for fails with it. + crate::fd_limit::raise_fd_limit().wrap_err("failed to raise RLIMIT_NOFILE")?; let graffiti = options.graffiti()?; let suggested_fee_recipient = options.suggested_fee_recipient()?; ethlambda_validator::run(ValidatorConfig { @@ -167,6 +183,7 @@ pub(crate) async fn run(options: ValidatorOptions) -> eyre::Result<()> { keymanager: options .enable_keymanager .then(|| std::net::SocketAddr::new(options.http_address, options.keymanager_port)), + web3signer_url: options.web3signer_url, init_slashing_protection: options.init_slashing_protection, }) .await @@ -188,6 +205,7 @@ mod tests { graffiti: graffiti.to_string(), suggested_fee_recipient: None, enable_keymanager: false, + web3signer_url: None, init_slashing_protection: false, } } diff --git a/crates/net/rpc/src/beacon/validator_client_tests.rs b/crates/net/rpc/src/beacon/validator_client_tests.rs index 107b1ccea..50d237275 100644 --- a/crates/net/rpc/src/beacon/validator_client_tests.rs +++ b/crates/net/rpc/src/beacon/validator_client_tests.rs @@ -238,7 +238,7 @@ async fn the_validator_client_can_exit_a_validator_through_this_node() { store .insert_secret("test", &secret_key_for(0).to_bytes()) .expect("valid key"); - store + RwLock::new(store) }; let options = ExitOptions { no_confirmation: true, @@ -286,7 +286,7 @@ async fn the_validator_client_can_exit_a_validator_through_this_node() { store .insert_secret("test", &secret_key_for(1).to_bytes()) .expect("valid key"); - store + RwLock::new(store) }; let options = ExitOptions { epoch: Some(0), @@ -329,7 +329,7 @@ async fn the_validator_client_follows_this_nodes_shard_committee_period() { store .insert_secret("test", &secret_key_for(0).to_bytes()) .expect("valid key"); - store + RwLock::new(store) }; let options = ExitOptions { no_confirmation: true, @@ -1033,10 +1033,7 @@ async fn serve_fulu_pinned_under( .spec() .await .expect("the client reads this node's spec"); - let context = Arc::new(SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }); + let context = Arc::new(SigningContext::new(spec, genesis.genesis_validators_root)); (Arc::new(client), state, network, context) } @@ -1333,10 +1330,7 @@ mod gloas { .spec() .await .expect("the client reads this node's spec"); - let context = Arc::new(SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }); + let context = Arc::new(SigningContext::new(spec, genesis.genesis_validators_root)); Served { client, state, diff --git a/crates/validator/Cargo.toml b/crates/validator/Cargo.toml index b34e72488..e129410b6 100644 --- a/crates/validator/Cargo.toml +++ b/crates/validator/Cargo.toml @@ -32,6 +32,9 @@ tracing.workspace = true # so this crate still builds if it is ever compiled on its own. tokio = { workspace = true, features = ["time", "macros", "rt"] } reqwest = { workspace = true, features = ["json"] } +# `join_all`, so a slot's attestations are signed concurrently: each one is a +# round trip when a remote signer holds the key. +futures = "0.3" # BLS12-381. Same version and backend the state transition verifies with, so a # signature produced here and one checked there come from one implementation. diff --git a/crates/validator/src/aggregation.rs b/crates/validator/src/aggregation.rs index 3a0a32dcd..0f605b143 100644 --- a/crates/validator/src/aggregation.rs +++ b/crates/validator/src/aggregation.rs @@ -39,16 +39,16 @@ use ethlambda_types::beacon::containers::electra; use ethlambda_types::beacon::containers::gloas; use ethlambda_types::beacon::containers::shared::AttestationData; use ethlambda_types::beacon::fork::ForkName; -use ethlambda_types::beacon::primitives::{HashTreeRoot as _, Slot}; +use ethlambda_types::beacon::primitives::{BlsPubkey, BlsSignature, HashTreeRoot as _, Slot}; use tokio::sync::RwLock; -use tracing::{info, warn}; +use tracing::{debug, info, warn}; use crate::aggregation_selection::selection_for; use crate::beacon_node::dto::{AttesterDutyDto, parse_pubkey}; use crate::beacon_node::{AggregateKind, BeaconNodeApi, SignedAggregates}; use crate::error::{Error, Result}; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{AggregateAndProofRef, SigningContext, sign_all}; /// An `AggregateAndProof` of either fork, held between building it and /// signing it. @@ -91,30 +91,51 @@ impl AggregationService { } // Which of this client's validators aggregates, and with what proof. - // - // The read guard covers the whole selection pass and no await, the way - // the attestation path's does: `selection_for` signs, and the lock is - // write-preferring, so holding this across the fetches below would put - // every later duty behind any keymanager writer that queued meanwhile. - let selected: Vec<(AttesterDutyDto, _)> = { - let store = store.read().await; - duties + // Asked for all at once, each within the signing budget: with only + // the last third of the slot left, a round trip per duty in series + // could run the whole duty out. Usually no request is made at all, + // since the proof was already produced for this epoch's subscription + // and `sign_selection_proof` remembers it. + let mut keyed = Vec::with_capacity(duties.len()); + for duty in duties { + match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => keyed.push((duty, pubkey)), + Err(err) => { + warn!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey") + } + } + } + let selections = sign_all( + keyed .iter() - .filter_map(|duty| match selection_for(&self.context, &store, duty) { - Ok(Some(proof)) => Some((duty.clone(), proof)), - Ok(None) => None, - Err(err) => { - warn!( - %slot, - validator = duty.validator_index, - %err, - "Could not compute aggregator selection; skipping this duty" - ); - None - } - }) - .collect() - }; + .map(|(duty, pubkey)| (*pubkey, selection_for(&self.context, store, duty))), + ) + .await; + let mut selected: Vec<(&AttesterDutyDto, BlsPubkey, BlsSignature)> = Vec::new(); + for ((duty, pubkey), selection) in keyed.into_iter().zip(selections) { + match selection { + Ok(Some(proof)) => selected.push((duty, pubkey, proof)), + Ok(None) => {} + // A key the keymanager deleted since the schedule was + // fetched. Its duty is skipped, as its attestation was, and + // is not a signature that failed: counting it would raise a + // signing alert for a delete the operator asked for. + Err(Error::UnknownValidator(_)) => debug!( + %slot, + validator = duty.validator_index, + "Duty names a key this client no longer holds; skipped" + ), + Err(err) => { + warn!( + %slot, + validator = duty.validator_index, + %err, + "Could not compute aggregator selection; skipping this duty" + ); + crate::metrics::inc_signing_failures(); + } + } + } if selected.is_empty() { return Ok(0); @@ -126,7 +147,7 @@ impl AggregationService { // twice for it would be the same answer at twice the cost. let data_root = data.hash_tree_root(); let mut aggregates = BTreeMap::new(); - for (duty, _) in &selected { + for (duty, _, _) in &selected { if aggregates.contains_key(&duty.committee_index) { continue; } @@ -161,54 +182,69 @@ impl AggregationService { // fork boundary sits. let fork = self.batch_fork(&aggregates, slot)?; + // Every wrapper is built first and all of them are signed at once, + // each within the signing budget, for the reason the selections are. + let mut wrappers = Vec::with_capacity(selected.len()); + for (duty, pubkey, selection_proof) in selected { + let Some(aggregate) = aggregates.get(&duty.committee_index) else { + continue; + }; + // The wrapper is the fork's own container, and the signing path + // takes its root from it: gloas's `Attestation` hashes + // differently from electra's, so a root computed over the wrong + // one would sign a message no node can verify. + let wrapper = match &aggregate.attestation { + AggregateKind::Electra(attestation) => { + Wrapper::Electra(electra::AggregateAndProof { + aggregator_index: duty.validator_index, + aggregate: attestation.clone(), + selection_proof, + }) + } + AggregateKind::Gloas(attestation) => Wrapper::Gloas(gloas::AggregateAndProof { + aggregator_index: duty.validator_index, + aggregate: attestation.clone(), + selection_proof, + }), + }; + wrappers.push((duty, pubkey, wrapper)); + } + + // Signed over the whole wrapper, not over the aggregate: that is what + // binds this validator's index and its selection proof to the votes + // it is republishing. + let signatures = sign_all(wrappers.iter().map(|(_, pubkey, wrapper)| { + let message = match wrapper { + Wrapper::Electra(message) => AggregateAndProofRef::Electra(message), + Wrapper::Gloas(message) => AggregateAndProofRef::Gloas(message), + }; + ( + *pubkey, + self.context + .sign_aggregate_and_proof(store, pubkey, message), + ) + })) + .await; + let signed = { - let store = store.read().await; let mut electra_signed = Vec::new(); let mut gloas_signed = Vec::new(); - for (duty, selection_proof) in &selected { - let Some(aggregate) = aggregates.get(&duty.committee_index) else { - continue; - }; - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - warn!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); + for ((duty, _, wrapper), signature) in wrappers.into_iter().zip(signatures) { + let signature = match signature { + Ok(signature) => signature, + // Deleted between the selection above and here; not a + // signing failure, for the reason given there. + Err(Error::UnknownValidator(_)) => { + debug!( + %slot, + validator = duty.validator_index, + "Duty names a key this client no longer holds; skipped" + ); continue; } - }; - - // The wrapper is the fork's own container, and its root is taken - // from it: gloas's `Attestation` hashes differently from - // electra's, so a root computed over the wrong one would sign a - // message no node can verify. - let (root, wrapper) = match &aggregate.attestation { - AggregateKind::Electra(attestation) => { - let message = electra::AggregateAndProof { - aggregator_index: duty.validator_index, - aggregate: attestation.clone(), - selection_proof: *selection_proof, - }; - (message.hash_tree_root(), Wrapper::Electra(message)) - } - AggregateKind::Gloas(attestation) => { - let message = gloas::AggregateAndProof { - aggregator_index: duty.validator_index, - aggregate: attestation.clone(), - selection_proof: *selection_proof, - }; - (message.hash_tree_root(), Wrapper::Gloas(message)) - } - }; - // Signed over the whole wrapper, not over the aggregate: that - // is what binds this validator's index and its selection proof - // to the votes it is republishing. - let signature = match self - .context - .sign_aggregate_and_proof(&store, &pubkey, root, slot) - { - Ok(signature) => signature, Err(err) => { warn!(%slot, validator = duty.validator_index, %err, "Failed to sign aggregate"); + crate::metrics::inc_signing_failures(); continue; } }; @@ -285,10 +321,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } fn store() -> (RwLock, BlsPubkey) { @@ -333,13 +366,14 @@ mod tests { /// A slot this key is selected to aggregate at, and one it is not. Which /// slots those are is a property of SHA-256, so they are found rather than /// written down. - fn slots(pubkey: &BlsPubkey, store: &ValidatorStore) -> (Slot, Slot) { + async fn slots(pubkey: &BlsPubkey, store: &RwLock) -> (Slot, Slot) { let context = context(); let mut selected = None; let mut rejected = None; for slot in 3_200..3_400 { let proof = context .sign_selection_proof(store, pubkey, slot) + .await .expect("signs"); if is_aggregator(128, &proof) { selected.get_or_insert(slot); @@ -358,10 +392,7 @@ mod tests { async fn fixture() -> (RwLock, BlsPubkey, Slot, Slot) { let (store, pubkey) = store(); - let (selected, rejected) = { - let guard = store.read().await; - slots(&pubkey, &guard) - }; + let (selected, rejected) = slots(&pubkey, &store).await; (store, pubkey, selected, rejected) } @@ -489,9 +520,9 @@ mod tests { let (_, list) = node.published_aggregates().remove(0); - let guard = store.read().await; let expected = context() - .sign_selection_proof(&guard, &pubkey, slot) + .sign_selection_proof(&store, &pubkey, slot) + .await .expect("signs"); assert_eq!(list[0].message.selection_proof, expected); assert!( @@ -531,24 +562,26 @@ mod tests { } fn gloas_context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet().with_fork_epoch(ForkName::Gloas, 100), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ForkName::Gloas, 100), + Root::ZERO, + )) } /// A slot in the gloas era at which this key is selected under the gloas /// fork version, found rather than written down for the reason `slots` is. - fn gloas_selected_slot(pubkey: &BlsPubkey, store: &ValidatorStore) -> Slot { + async fn gloas_selected_slot(pubkey: &BlsPubkey, store: &RwLock) -> Slot { let context = gloas_context(); - (3_200..3_600) - .find(|slot| { - let proof = context - .sign_selection_proof(store, pubkey, *slot) - .expect("signs"); - is_aggregator(128, &proof) - }) - .expect("some slot selects this key") + for slot in 3_200..3_600 { + let proof = context + .sign_selection_proof(store, pubkey, slot) + .await + .expect("signs"); + if is_aggregator(128, &proof) { + return slot; + } + } + panic!("some slot selects this key") } /// At a gloas slot the aggregate is gloas's container, and the wrapper is @@ -560,10 +593,7 @@ mod tests { use blst::min_pk::{PublicKey, Signature}; let (store, pubkey) = store(); - let slot = { - let guard = store.read().await; - gloas_selected_slot(&pubkey, &guard) - }; + let slot = gloas_selected_slot(&pubkey, &store).await; let node = Arc::new(MockBeaconNode::new().with_gloas_aggregate(data(slot))); let published = AggregationService::new(node.clone(), gloas_context()) diff --git a/crates/validator/src/aggregation_selection.rs b/crates/validator/src/aggregation_selection.rs index d29aa159d..4aec8323b 100644 --- a/crates/validator/src/aggregation_selection.rs +++ b/crates/validator/src/aggregation_selection.rs @@ -34,6 +34,7 @@ use ethlambda_types::beacon::primitives::BlsSignature; use sha2::{Digest, Sha256}; +use tokio::sync::RwLock; use crate::beacon_node::dto::{AttesterDutyDto, parse_pubkey}; use crate::error::Result; @@ -88,13 +89,15 @@ pub fn is_aggregator(committee_length: u64, selection_proof: &BlsSignature) -> b /// silent `None`. Both mean the duty cannot be served at all, and reporting /// them as "not an aggregator" would hide a misconfiguration behind a role /// this validator was never going to fill anyway. -pub fn selection_for( +pub async fn selection_for( context: &SigningContext, - store: &ValidatorStore, + store: &RwLock, duty: &AttesterDutyDto, ) -> Result> { let pubkey = parse_pubkey(&duty.pubkey)?; - let proof = context.sign_selection_proof(store, &pubkey, duty.slot)?; + let proof = context + .sign_selection_proof(store, &pubkey, duty.slot) + .await?; Ok(is_aggregator(duty.committee_length, &proof).then_some(proof)) } diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index 5372b8470..564434f0b 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -15,7 +15,7 @@ use crate::beacon_node::dto::{ use crate::beacon_node::{BeaconNodeApi, validate_attestation_data}; use crate::error::Result; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; use crate::slashing_protection::SharedProtection; /// What one slot's attestation duty produced. @@ -195,8 +195,8 @@ impl AttestationService { // Check and durably record the whole batch before any signature exists: // a signature that exists is a signature that can escape, whatever the // code after it intended. One write for the slot, off the executor, - // and before the store's read lock below is taken, so the disk flush - // never holds up a keymanager writer queued behind it. + // and with no guard on the store held, so the disk flush never holds + // up a keymanager writer queued behind it. let pubkeys: Vec = candidates.iter().map(|(_, pubkey)| *pubkey).collect(); let verdicts = crate::slashing_protection::blocking(&self.protection, move |protection| { protection.check_and_record_attestations(&pubkeys, &data) @@ -219,48 +219,64 @@ impl AttestationService { } }; - // The read guard is scoped to this block alone and must never be widened to cover an - // await. `tokio::sync::RwLock` is write-preferring, so a keymanager - // import or delete queuing for the write lock blocks every read behind - // it; keeping this guard narrow bounds how long that wait can be. The - // import holds its own write lock for the inserts alone, doing the slow - // EIP-2335 derivation outside it (see `http_api::keystores::import`), - // so this is the duty path keeping its half of the same bargain. - let mut attestations = Vec::with_capacity(candidates.len()); + // Every duty the record let through is signed together below. + let mut to_sign = Vec::with_capacity(candidates.len()); let mut refused = 0; - { - let _timing = crate::metrics::time_signing(); - let store = store.read().await; - for ((duty, pubkey), verdict) in candidates.into_iter().zip(verdicts) { - if let Err(refusal) = verdict { - warn!( - %slot, - validator = duty.validator_index, - %refusal, - "Refusing to sign: slashing protection judged this attestation unsafe" - ); - crate::metrics::inc_attestations_refused(); - refused += 1; + for ((duty, pubkey), verdict) in candidates.into_iter().zip(verdicts) { + if let Err(refusal) = verdict { + warn!( + %slot, + validator = duty.validator_index, + %refusal, + "Refusing to sign: slashing protection judged this attestation unsafe" + ); + crate::metrics::inc_attestations_refused(); + refused += 1; + continue; + } + to_sign.push((duty, pubkey)); + } + + // Signed concurrently rather than one after another. A local key + // signs in microseconds either way; a remote signer costs a round + // trip per signature, and a hundred validators behind one signer + // must not pay a hundred of them in series inside one slot. Each + // signature takes the store's read guard for its own lookup only + // (see `SigningContext::sign`), so nothing here holds it across an + // await: `tokio::sync::RwLock` is write-preferring, and a keymanager + // import queuing for the write lock would otherwise stall every read + // behind it for as long as the guard was held. + // + // Each signature gets `SIGNING_BUDGET` and no more, so a hung signer + // costs only its own keys: the batch below goes out with every + // signature that arrived, rather than the slot's deadline dropping + // the whole batch, local keys included, while it waits. + let signatures = + { + let _timing = crate::metrics::time_signing(); + sign_all(to_sign.iter().map(|(_, pubkey)| { + (*pubkey, self.context.sign_attestation(store, pubkey, &data)) + })) + .await + }; + + let mut attestations = Vec::with_capacity(to_sign.len()); + for ((duty, _), signature) in to_sign.iter().zip(signatures) { + // One validator failing must not cost the others their attestation. + let signature = match signature { + Ok(signature) => signature, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Failed to sign attestation"); + crate::metrics::inc_signing_failures(); continue; } - - // One validator failing must not cost the others their attestation. - let signature = match self.context.sign_attestation(&store, &pubkey, &data) { - Ok(signature) => signature, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign attestation"); - crate::metrics::inc_signing_failures(); - continue; - } - }; - - attestations.push(SingleAttestationDto { - committee_index: duty.committee_index, - attester_index: duty.validator_index, - data: data_dto.clone(), - signature: encode_hex(&signature.0), - }); - } + }; + attestations.push(SingleAttestationDto { + committee_index: duty.committee_index, + attester_index: duty.validator_index, + data: data_dto.clone(), + signature: encode_hex(&signature.0), + }); } if attestations.is_empty() { @@ -331,10 +347,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } fn duty( @@ -820,11 +833,10 @@ mod tests { } fn gloas_context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet() - .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, 10), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, 10), + Root::ZERO, + )) } /// From gloas `data.index` is the payload signal, so a node answering 1 diff --git a/crates/validator/src/beacon_node/block_contents.rs b/crates/validator/src/beacon_node/block_contents.rs index c4d2806ca..77a837cd3 100644 --- a/crates/validator/src/beacon_node/block_contents.rs +++ b/crates/validator/src/beacon_node/block_contents.rs @@ -390,6 +390,33 @@ impl ProducedBlock { } } + /// The block's header: the block with its body replaced by the body's + /// root. It merkleizes to [`Self::block_root`], which is what lets the + /// signing path take the header (a remote signer reads it) and sign the + /// block's root all the same. + pub fn header(&self) -> ethlambda_types::beacon::containers::shared::BeaconBlockHeader { + use ethlambda_types::beacon::containers::shared::BeaconBlockHeader; + use ethlambda_types::beacon::primitives::HashTreeRoot as _; + match &self.contents { + Contents::WithBlobProofs { block, .. } | Contents::WithCellProofs { block, .. } => { + BeaconBlockHeader { + slot: block.slot, + proposer_index: block.proposer_index, + parent_root: block.parent_root, + state_root: block.state_root, + body_root: block.body.hash_tree_root(), + } + } + Contents::Gloas { block, .. } => BeaconBlockHeader { + slot: block.slot, + proposer_index: block.proposer_index, + parent_root: block.parent_root, + state_root: block.state_root, + body_root: block.body.hash_tree_root(), + }, + } + } + /// Where the block pays its execution-layer rewards: the payload's fee /// recipient before gloas, the bid's from it on. pub fn fee_recipient(&self) -> ethlambda_types::beacon::primitives::ExecutionAddress { diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index 5cf4db23b..be4f199da 100644 --- a/crates/validator/src/error.rs +++ b/crates/validator/src/error.rs @@ -101,12 +101,14 @@ pub enum Error { #[error("no signing key for validator {0:?}")] UnknownValidator(BlsPubkey), - /// Not constructed today: the only `SigningMethod` is `LocalKeystore`, - /// whose signing call cannot fail once a key has been resolved. Kept for - /// the remote-signer variant `keys::store`'s module doc already commits - /// to supporting, whose call to an external signer can fail in ways a - /// local scalar multiplication cannot; remove this only if that plan is - /// dropped. + /// A remote signer did not produce the signature: it refused the message + /// (its slashing protection), does not hold the key, answered badly, or + /// could not be reached. A local key cannot raise this; its signing call + /// cannot fail once the key has been resolved. + /// + /// Not retryable: it costs one signature, the duty that needed it logs + /// and counts it, and none of its causes is something trying again + /// within the slot would change. #[error("signing failed for {pubkey:?}: {reason}")] Signing { pubkey: BlsPubkey, reason: String }, diff --git a/crates/validator/src/exit.rs b/crates/validator/src/exit.rs index 6f71a2a15..d6b310c45 100644 --- a/crates/validator/src/exit.rs +++ b/crates/validator/src/exit.rs @@ -29,6 +29,7 @@ use ethlambda_types::beacon::constants::FAR_FUTURE_EPOCH; use ethlambda_types::beacon::containers::shared::{SignedVoluntaryExit, VoluntaryExit}; use ethlambda_types::beacon::preset::SLOTS_PER_EPOCH; use ethlambda_types::beacon::primitives::{BlsPubkey, Epoch}; +use tokio::sync::RwLock; use tracing::info; use crate::beacon_node::dto::encode_hex; @@ -95,6 +96,9 @@ pub enum Outcome { pub async fn run(config: ExitConfig) -> Result { let (store, pubkey) = load_keystore(&config.keystore, &config.password_file)?; let node = HttpBeaconNode::new(&config.beacon_node)?; + // Behind the lock every signing method takes its store through. Nothing + // else holds this one: the command signs once and exits. + let store = RwLock::new(store); let stdin = std::io::stdin(); let mut input = stdin.lock(); run_with(&node, &store, pubkey, &config.options, &mut input).await @@ -116,7 +120,7 @@ pub fn load_keystore( /// phrase, which is what the tests drive. pub async fn run_with( node: &B, - store: &ValidatorStore, + store: &RwLock, pubkey: BlsPubkey, options: &ExitOptions, input: &mut dyn BufRead, @@ -124,10 +128,7 @@ pub async fn run_with( let genesis = node.genesis().await?; let spec = node.spec().await?; let clock = SlotClock::from_config(genesis.genesis_time, &spec); - let signing = SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }; + let signing = SigningContext::new(spec, genesis.genesis_validators_root); // A syncing node's registry is behind the chain's, so its answer about // the validator's status and index is not one to sign over. @@ -152,7 +153,9 @@ pub async fn run_with( epoch, validator_index: details.index, }; - let signature = signing.sign_voluntary_exit(store, &pubkey, &exit, current_epoch)?; + let signature = signing + .sign_voluntary_exit(store, &pubkey, &exit, current_epoch) + .await?; let signed = SignedVoluntaryExit { message: exit, signature, @@ -382,10 +385,10 @@ mod tests { .expect("32 bytes") } - fn store_with_key() -> (ValidatorStore, BlsPubkey) { + fn store_with_key() -> (RwLock, BlsPubkey) { let mut store = ValidatorStore::new(); let pubkey = store.insert_secret("test", &secret()).expect("inserts"); - (store, pubkey) + (RwLock::new(store), pubkey) } /// Genesis far enough back that the current epoch clears mainnet's @@ -577,10 +580,7 @@ mod tests { "presign must not publish" ); - let signing = SigningContext { - config: Config::mainnet(), - genesis_validators_root: genesis().genesis_validators_root, - }; + let signing = SigningContext::new(Config::mainnet(), genesis().genesis_validators_root); let root = signing.voluntary_exit_signing_root(&signed.message, now()); let pk = PublicKey::from_bytes(&pubkey.0).expect("valid pubkey"); let sig = Signature::from_bytes(&signed.signature.0).expect("valid signature"); diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 60221587f..319a32182 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -37,6 +37,7 @@ //! [`crate::slashing_protection`]), so an export lists, per validator, its //! highest slot and its highest source and target rather than every message. +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -51,7 +52,7 @@ use crate::Error; use crate::beacon_node::dto::{encode_hex, parse_pubkey}; use crate::http_api::KeymanagerContext; use crate::keys::keystore::Keystore; -use crate::keys::{ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; +use crate::keys::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; use crate::secure_fs; /// The most keystores accepted in a single import request. @@ -121,12 +122,28 @@ pub struct KeyStatus { } impl KeyStatus { - fn error(message: impl std::fmt::Display) -> Self { + pub(super) fn error(message: impl std::fmt::Display) -> Self { Self { status: "error".to_string(), message: Some(message.to_string()), } } + + /// The key is already known to this client, through either route. + pub(super) fn duplicate() -> Self { + Self { + status: "duplicate".to_string(), + message: None, + } + } + + /// The route asked has no such key. + fn not_found() -> Self { + Self { + status: "not_found".to_string(), + message: None, + } + } } #[derive(Debug, Deserialize)] @@ -142,11 +159,13 @@ pub struct DeleteResponse { pub slashing_protection: String, } +/// The keys this process holds itself. Remote ones are `/eth/v1/remotekeys`'s +/// to list; the two routes partition the store. pub async fn list(State(context): State) -> Json { let store = context.store.read().await; Json(ListResponse { data: store - .pubkeys() + .local_pubkeys() .into_iter() .map(|pubkey| ListedKeystore { validating_pubkey: encode_hex(&pubkey.0), @@ -282,14 +301,9 @@ pub async fn import( // what it replaced: a disabled entry, which was never loaded and so may // have no record, must not be left enabled on disk, or the next start // refuses the whole client for it. - let before: std::collections::HashMap = definitions - .0 - .iter() - .filter_map(|definition| { - parse_pubkey(&definition.voting_public_key) - .ok() - .map(|pubkey| (pubkey, definition.clone())) - }) + let before: HashMap = definitions + .keyed() + .map(|(pubkey, definition)| (pubkey, definition.clone())) .collect(); // Decrypt (deliberately slow: EIP-2335 key derivation) and persist every @@ -301,10 +315,38 @@ pub async fn import( // due for as long as the whole batch takes, rather than just for the // instant the inserts below actually need. `definitions_lock` above has // no such reader/writer to starve: only import and delete ever take it. + // + // The keys a remote signer holds, by the store or by the file, gathered + // once for the whole request rather than looked up again for each of its + // keys. The store's are read under a guard released at once. + // `definitions_lock` is held until the inserts below, and every + // keymanager writer takes it first, so neither the store nor the file + // can change between this read and those inserts; nothing this request + // does adds a remote entry either, so the set stays true to its end. + let mut held_remotely: HashSet = context + .store + .read() + .await + .remote_entries() + .into_iter() + .map(|(pubkey, _)| pubkey) + .collect(); + held_remotely.extend( + definitions + .keyed() + .filter(|(_, definition)| definition.signing.is_remote()) + .map(|(pubkey, _)| pubkey), + ); let mut prepared = Vec::with_capacity(request.keystores.len()); for (position, json) in request.keystores.iter().enumerate() { let password = request.passwords.get(position).map(String::as_str); - prepared.push(prepare_import(&context, &mut definitions, json, password)); + prepared.push(prepare_import( + &context, + &held_remotely, + &mut definitions, + json, + password, + )); } // Every persisted key is registered in one durable write, before any of @@ -460,15 +502,12 @@ fn undo_import( if added.is_empty() && replaced.is_empty() { return Ok(()); } - definitions.0.retain(|definition| { - parse_pubkey(&definition.voting_public_key) - .map(|parsed| !added.contains(&parsed)) - .unwrap_or(true) - }); + definitions.remove_where(|pubkey, _| added.contains(pubkey)); for definition in &mut definitions.0 { - if let Some((_, previous)) = replaced.iter().find(|(pubkey, _)| { - parse_pubkey(&definition.voting_public_key).is_ok_and(|parsed| parsed == *pubkey) - }) { + let Ok(named) = parse_pubkey(&definition.voting_public_key) else { + continue; + }; + if let Some((_, previous)) = replaced.iter().find(|(pubkey, _)| *pubkey == named) { *definition = previous.clone(); } } @@ -503,7 +542,7 @@ fn put_back(path: &Path, previous: Option<&[u8]>) -> crate::Result<()> { /// Save the definitions file, keeping `definitions_unflushed` true from an /// after-rename flush failure until a later save flushes the directory, which /// makes every earlier rename in it durable too. -fn save_definitions( +pub(super) fn save_definitions( context: &KeymanagerContext, definitions: &ValidatorDefinitions, ) -> crate::Result<()> { @@ -545,6 +584,7 @@ fn remove_if_present(path: &Path) -> crate::Result<()> { /// before it needs the store's write lock and its slashing-protection record. fn prepare_import( context: &KeymanagerContext, + held_remotely: &HashSet, definitions: &mut ValidatorDefinitions, json: &str, password: Option<&str>, @@ -557,6 +597,16 @@ fn prepare_import( let secret = keystore.decrypt(password).map_err(KeyStatus::error)?; let pubkey = ValidatorStore::derive_pubkey(&secret).map_err(KeyStatus::error)?; + // A key a remote signer holds, by the file or by the store, is + // `duplicate` here, the mirror of what `/eth/v1/remotekeys` answers for a + // local one: neither route silently replaces the other's entry. Judged + // before anything is written, so a `duplicate` never leaves files behind. + // Re-importing a key this route already holds is still `imported`, since + // that is how a password is rotated. + if held_remotely.contains(&pubkey) { + return Err(KeyStatus::duplicate()); + } + // Persisted before the key is reachable through the store: a validator // this process is willing to sign with must already be durable, or a // crash right after "imported" is reported silently loses it, with @@ -599,7 +649,9 @@ fn regular_file_bytes(path: &Path) -> crate::Result>> { /// Add an already-decrypted, already-persisted key to the live store. The /// only step of an import that needs the write lock: a cheap, synchronous -/// scalar validation and hashmap insert, never an await. +/// scalar validation and hashmap insert, never an await. Nothing is judged +/// again here: `prepare_import` judged the file and the store under +/// `definitions_lock`, which is still held. fn finish_import(store: &mut ValidatorStore, secret: &[u8; 32]) -> KeyStatus { match store.insert_secret("keymanager import", secret) { Ok(pubkey) => { @@ -680,8 +732,10 @@ fn persist_import( let definition = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path.clone(), - voting_keystore_password_path: password_path.clone(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: password_path.clone(), + }, }; // How to put `definitions` back if the save below fails. // @@ -762,28 +816,23 @@ pub async fn delete( State(context): State, Json(request): Json, ) -> Json { - // Taken before `store`'s write lock below, the same order `import` uses, - // so the two handlers never deadlock against each other. See - // `KeymanagerContext::definitions_lock`: this is what makes `persist_delete` - // below and a concurrent import's own open-mutate-save cycle mutually - // exclusive, rather than racing to overwrite the definitions file. + // Taken before `store`'s write lock, the same order `import` uses, so the + // two handlers never deadlock against each other. See + // `KeymanagerContext::definitions_lock`: this is what makes the read and + // save of the definitions file in `delete_keys` and a concurrent import's + // own open-mutate-save cycle mutually exclusive, rather than racing to + // overwrite the file. let _definitions_guard = context.definitions_lock.lock().await; - let mut store = context.store.write().await; - let results: Vec<(Option, KeyStatus)> = request - .pubkeys - .iter() - .map(|text| match parse_pubkey(text) { - Ok(pubkey) => (Some(pubkey), delete_one(&context, &mut store, &pubkey)), - Err(err) => (None, KeyStatus::error(err)), - }) - .collect(); - drop(store); + // A key of the other route comes back without its pubkey, so nothing + // below exports its record or reports it `not_active` while a remote + // signer still signs with it. + let results = delete_keys(&context, &request.pubkeys, Origin::Local, judge_delete).await; // 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. That holds because the duty - // services record before they take the store's read lock to sign (see + // of them was either asked for before the write lock above was granted, + // and is in the record, or can no longer happen. That holds because the + // duty services record before they look a key up to sign with it (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 @@ -860,132 +909,305 @@ pub async fn delete( }) } -fn delete_one( +/// How this route judges one key of a delete; see [`Judge`]. +fn judge_delete( context: &KeymanagerContext, store: &mut ValidatorStore, pubkey: &BlsPubkey, -) -> KeyStatus { - // Whether this key is on disk, read before the in-memory removal makes the - // store an unreliable witness. `not_found` has to mean "this client has no - // definitions entry for it", not "it is not in memory right now": a key - // removed from memory by an earlier failed attempt is still very much - // present on disk, and reporting `not_found` for it tells an operator the - // key is gone when a restart will bring it back signing. - let on_disk = match definitions_contain(context, pubkey) { - Ok(present) => present, - // Cannot tell. Report the failure rather than guessing either way: a - // wrong `not_found` here is the dangerous direction. - Err(err) => return KeyStatus::error(err), - }; + entry: Option, +) -> Judged { + // A remote key is the other route's whatever its state: one the store + // holds, or one only the file lists, which the next start would load. It + // is `/eth/v1/remotekeys`'s to delete, and this route must not reach into + // the other's half of the store. + if store.remote_url(pubkey).is_some() + || (entry == Some(Origin::Remote) && !store.contains(pubkey)) + { + return Judged::OtherRoutes; + } let in_memory = store.remove(pubkey); - // Whatever becomes of the definitions file below, this process no longer - // signs with the key, so its overrides have nothing left to apply to. A key + // Whatever becomes of the definitions file, this process no longer signs + // with the key, so its overrides have nothing left to apply to. A key // imported again starts on the defaults. context.settings.forget(pubkey); - if !on_disk && !in_memory { - // A removal whose directory flush failed is on disk but may not - // survive a power loss, and a retry lands here. Answering `not_found` - // then would tell the operator the key is gone for good; flush first, - // by saving the file again, and keep the error until that works. - if context - .definitions_unflushed - .load(std::sync::atomic::Ordering::Acquire) - { - let definitions = match ValidatorDefinitions::open(&context.validators_dir) { - Ok(definitions) => definitions, - Err(err) => return KeyStatus::error(err), - }; - if let Err(err) = save_definitions(context, &definitions) { - return KeyStatus::error(format!( - "an earlier removal is on disk but the directory could not be flushed: \ - {err}; the key may come back after a power loss" - )); - } + if entry != Some(Origin::Local) && !in_memory { + return Judged::Absent; + } + // Stranded when the store held the key as a local one while the file + // lists it under a remote signer, which only a hand edit of the file + // under a running client produces. This route leaves that entry alone, + // so answering `deleted`, with the key's history attached, would tell an + // operator the key is free to run elsewhere while the next start loads + // it again and signs through a signer that never heard of what it did + // since. + Judged::Removed { + stranded: entry == Some(Origin::Remote), + } +} + +/// What a route decided about one key of a delete, with the store's write +/// lock held and before the definitions file is saved. +pub(super) enum Judged { + /// The other route's key: `not_found` here, with nothing more said or + /// exported about it. + OtherRoutes, + /// This route holds nothing for the key, in the file or in the store. + Absent, + /// The key is out of the store, and its entry comes off the file in the + /// request's one save. + Removed { + /// The file also lists the key under the other route's kind, an + /// entry this route leaves where it is and the next start would load. + /// The delete is then answered with an error rather than `deleted`. + stranded: bool, + }, +} + +/// How a route judges one key of a delete: against the store, which it may +/// take the key out of, and the kind of entry the definitions file holds for +/// it (`None` for no entry). Called with the store's write lock held, so it +/// must not touch the disk. +pub(super) type Judge = + fn(&KeymanagerContext, &mut ValidatorStore, &BlsPubkey, Option) -> Judged; + +/// Delete `pubkeys` through `origin`'s route: each one's status, beside the +/// key it named when that was a readable key of this route's. +/// +/// The caller holds `definitions_lock`. The definitions file is read once and +/// saved once for the whole request, and the store's write lock is held only +/// while keys are taken out of memory, never across either. +/// `tokio::sync::RwLock` is write-preferring, so a write lock held across a +/// read and a durable save of the file per key would stall every signature's +/// lookup, local keys' included, for as long as the request has keys. +/// +/// The file is read before the in-memory removal makes the store an +/// unreliable witness, and it is the file that decides `not_found`: that has +/// to mean "this client has no definitions entry for it", not "it is not in +/// memory right now". A key removed from memory by an earlier failed attempt +/// is still very much present on disk, and reporting `not_found` for it tells +/// an operator the key is gone when a restart will bring it back signing. A +/// disabled entry counts as present for the same reason: a delete is still +/// what removes it. An entry whose `voting_public_key` does not parse names +/// no key (see `ValidatorDefinitions::keyed`), so it is neither found nor +/// removed. +/// +/// The keystore and password files are deliberately left on disk: `deleted` +/// means this client has stopped using the key, not that it destroys operator +/// key material on an HTTP call. A definitions file with no entry for them is +/// enough to keep them from being loaded again. +pub(super) async fn delete_keys( + context: &KeymanagerContext, + pubkeys: &[String], + origin: Origin, + judge: Judge, +) -> Vec<(Option, KeyStatus)> { + let parsed: Vec> = pubkeys + .iter() + .map(|text| parse_pubkey(text).map_err(KeyStatus::error)) + .collect(); + + let mut definitions = match ValidatorDefinitions::open(&context.validators_dir) { + Ok(definitions) => definitions, + // Cannot tell what is on disk. Reported rather than guessed either + // way, with nothing removed: a wrong `not_found` here is the + // dangerous direction. + Err(err) => { + return parsed + .into_iter() + .map(|key| match key { + Ok(pubkey) => (Some(pubkey), KeyStatus::error(&err)), + Err(status) => (None, status), + }) + .collect(); } - return KeyStatus { - status: "not_found".to_string(), - message: None, - }; + }; + // The kind of entry the file holds for each key, every entry decoded + // once for the request. The first entry naming a key speaks for it. + let mut on_file: HashMap = HashMap::new(); + for (pubkey, definition) in definitions.keyed() { + on_file + .entry(pubkey) + .or_insert(Origin::of(&definition.signing)); } - // The in-memory store has already stopped signing with this key, which is - // the safety-relevant effect and happens first, the opposite order from - // import. If persisting the removal below fails, this process will not - // sign with the key again this run, but the definitions file still lists - // it, so a restart would reactivate it. + // The in-memory removals, and nothing else, under the write lock. They + // come first, the opposite order from import: that this process stops + // signing with a key is the safety-relevant effect, and it holds whatever + // becomes of the file below. + let judged: Vec> = { + let mut store = context.store.write().await; + parsed + .into_iter() + .map(|key| { + let pubkey = key?; + let entry = on_file.get(&pubkey).copied(); + Ok((pubkey, judge(context, &mut store, &pubkey, entry))) + }) + .collect() + }; + + // One save for the request. Only entries of this route's kind come off: + // the routes partition the store, and a delete through one must not erase + // the other's entry for the same key when memory and the file disagree + // about which kind it is. Saved even when no entry does (a key that was + // in memory only), since a save is also what flushes an earlier removal + // whose directory flush failed (see `save_definitions`). // - // That is why the error is returned rather than swallowed, and why - // `not_found` above is gated on the file rather than on memory. An - // operator who retries after this error must get the same error again, not - // a `not_found` that reads as "already gone": acting on that, by importing + // If it fails, this process will not sign with the keys again this run, + // but the file still lists them, so a restart would reactivate them. That + // is why the failure is every removed key's answer rather than swallowed: + // an operator who retries after it must get an error again, not a + // `not_found` that reads as "already gone". Acting on that, by importing // the key elsewhere, is what turns a failed delete into two hosts signing // for one validator. - match persist_delete(context, pubkey) { - Ok(()) => {} - // The removal is on disk but may not survive a power loss, and the - // keystore stays, so the key could come back enabled, with its - // record, while the operator runs it elsewhere. An error, so no - // caller takes the key for gone; this process does not sign with it. - Err(Error::Unflushed { path, source }) => { - return KeyStatus::error(format!( + let removing: HashSet = judged + .iter() + .filter_map(|key| match key { + Ok((pubkey, Judged::Removed { .. })) => Some(*pubkey), + _ => None, + }) + .collect(); + let saved: std::result::Result<(), String> = if removing.is_empty() { + Ok(()) + } else { + definitions + .remove_where(|pubkey, signing| removing.contains(pubkey) && origin.owns(signing)); + match save_definitions(context, &definitions) { + Ok(()) => Ok(()), + // The removal is on disk but may not survive a power loss, and + // the keystore stays, so the key could come back enabled, with + // its record, while the operator runs it elsewhere. An error, so + // no caller takes the key for gone; this process does not sign + // with it. + Err(Error::Unflushed { path, source }) => Err(format!( "removed from {path}, but the directory could not be flushed: {source}; the key \ may come back after a power loss" - )); + )), + Err(err) => Err(err.to_string()), } - Err(err) => return KeyStatus::error(err), - } + }; - info!(pubkey = %encode_hex(&pubkey.0), "Deleted validator key"); - KeyStatus { - status: "deleted".to_string(), - message: None, - } + // `not_found`, once an earlier removal is known to be durable; asked at + // most once for the request, and only if some key needs the answer. + let mut earlier: Option> = None; + let mut absent = || match earlier.get_or_insert_with(|| flush_earlier_removal(context)) { + Ok(()) => KeyStatus::not_found(), + Err(message) => KeyStatus::error(message), + }; + let mut answered: HashSet = HashSet::new(); + judged + .into_iter() + .map(|key| { + let (pubkey, judged) = match key { + Ok(judged) => judged, + Err(status) => return (None, status), + }; + let status = match judged { + Judged::OtherRoutes => return (None, KeyStatus::not_found()), + Judged::Absent => absent(), + Judged::Removed { stranded } => { + if let Err(message) = &saved { + KeyStatus::error(message) + } else if !answered.insert(pubkey) { + // Named earlier in this request, and removed there. + absent() + } else if stranded { + KeyStatus::error(origin.stranded()) + } else { + let pubkey = encode_hex(&pubkey.0); + match origin { + Origin::Local => info!(%pubkey, "Deleted validator key"), + Origin::Remote => info!(%pubkey, "Deleted remote validator key"), + } + KeyStatus { + status: "deleted".to_string(), + message: None, + } + } + } + }; + (Some(pubkey), status) + }) + .collect() } -/// Whether the definitions file currently holds an entry for `pubkey`. -/// -/// Read from disk rather than from the in-memory store, because the two can -/// disagree and it is the file that decides what the next restart loads. A -/// disabled entry counts as present: it is still there, `delete` is still what -/// removes it, and reporting `not_found` for one would leave the operator -/// believing a key is gone while its entry waits to be re-enabled. +/// Make an earlier removal durable before a key is reported `not_found`. /// -/// An unparseable `voting_public_key` cannot match, matching the filter in -/// [`persist_delete`]: if that entry will not be removed, this must not claim -/// it was found, or a delete would report success having removed nothing. -fn definitions_contain(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Result { - let definitions = ValidatorDefinitions::open(&context.validators_dir)?; - Ok(definitions.0.iter().any(|definition| { - parse_pubkey(&definition.voting_public_key) - .map(|parsed| parsed == *pubkey) - .unwrap_or(false) - })) +/// A removal whose directory flush failed is on disk but may not survive a +/// power loss, and a retry of that delete finds nothing left to remove. +/// Answering `not_found` then would tell the operator the key is gone for +/// good; this flushes first, by saving the file again, and `Err` is the +/// message to answer with until that works. +fn flush_earlier_removal(context: &KeymanagerContext) -> std::result::Result<(), String> { + if !context + .definitions_unflushed + .load(std::sync::atomic::Ordering::Acquire) + { + return Ok(()); + } + let definitions = + ValidatorDefinitions::open(&context.validators_dir).map_err(|err| err.to_string())?; + save_definitions(context, &definitions).map_err(|err| { + format!( + "an earlier removal is on disk but the directory could not be flushed: {err}; the \ + key may come back after a power loss" + ) + }) } -/// Drop a deleted key's entry from the definitions file. -/// -/// Deliberately leaves the keystore and password files on disk: `deleted` -/// means this client has stopped using the key, not that it destroys operator -/// key material on an HTTP call. A definitions file with no entry for them is -/// enough to keep them from being loaded again. -fn persist_delete(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Result<()> { - let mut definitions = ValidatorDefinitions::open(&context.validators_dir)?; - definitions.0.retain(|definition| { - parse_pubkey(&definition.voting_public_key) - .map(|parsed| parsed != *pubkey) - .unwrap_or(true) - }); - save_definitions(context, &definitions) +/// Which route a definitions entry belongs to: the keystores route's local +/// keys, or the remotekeys route's remote ones. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Origin { + Local, + Remote, +} + +impl Origin { + fn of(signing: &SigningDefinition) -> Self { + if signing.is_remote() { + Self::Remote + } else { + Self::Local + } + } + + fn owns(self, signing: &SigningDefinition) -> bool { + Self::of(signing) == self + } + + /// What a delete through this route answers when the key is out of the + /// store but the file keeps an entry of the other route's kind for it; + /// see [`Judged::Removed`]. + fn stranded(self) -> &'static str { + match self { + Self::Local => { + "no longer signing with this key, but the definitions file lists it under a \ + remote signer, an entry this route does not remove, so it would sign again \ + after a restart; delete it through /eth/v1/remotekeys" + } + Self::Remote => { + "no longer signing with this key, but the definitions file lists it as a local \ + keystore, an entry this route does not remove, so it would sign again after a \ + restart; delete it through /eth/v1/keystores" + } + } + } } #[cfg(test)] mod tests { use super::*; + use crate::http_api::router; + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use http_body_util::BodyExt as _; + use tower::ServiceExt as _; const TOKEN: &str = "test-token"; @@ -1032,6 +1254,7 @@ mod tests { clock, definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), protection: crate::slashing_protection::SlashingProtection::in_memory(), + default_remote_signer: None, } } @@ -1184,8 +1407,10 @@ mod tests { let existing = ValidatorDefinition { enabled: false, voting_public_key: encode_hex(&pubkey.0), - voting_keystore_path: PathBuf::from("old.json"), - voting_keystore_password_path: PathBuf::from("old.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("old.json"), + voting_keystore_password_path: PathBuf::from("old.txt"), + }, }; let mut definitions = ValidatorDefinitions(vec![existing.clone()]); @@ -1368,6 +1593,224 @@ mod tests { ); } + /// The two routes must not replace each other's entries. A keystore for + /// a key a remote signer already holds is `duplicate`, and the remote + /// entry survives in memory and on disk; and a remote import of a key + /// the file already lists (here, a disabled local entry) is `duplicate` + /// too, rather than turning the disabled key into a signing one. + #[tokio::test] + async fn neither_route_replaces_the_others_entry() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path()), + TOKEN.to_string(), + ); + let pubkey = "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07"; + let signer = "https://signer.example:9000"; + + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey, "url": signer }] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported"); + + 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"], "duplicate"); + let parsed = parse_pubkey(pubkey).expect("pubkey"); + assert_eq!( + store.read().await.remote_url(&parsed).as_deref(), + Some(signer), + "the remote entry must survive in memory" + ); + let reloaded = ValidatorStore::load(dir.path()).expect("loads"); + assert_eq!(reloaded.remote_url(&parsed).as_deref(), Some(signer)); + + // The other direction: a disabled local entry on disk, nothing in + // memory, and a remote import for the same key. + let other = tempfile::tempdir().expect("temp dir"); + let mut definitions = ValidatorDefinitions::default(); + definitions.0.push(ValidatorDefinition { + enabled: false, + voting_public_key: pubkey.to_string(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, + }); + definitions.save(other.path()).expect("saves"); + let app = router( + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), other.path()), + TOKEN.to_string(), + ); + let (_, body) = send( + app, + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey, "url": signer }] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "duplicate"); + let on_disk = ValidatorDefinitions::open(other.path()).expect("opens"); + assert!( + !on_disk.0[0].enabled && !on_disk.0[0].signing.is_remote(), + "the disabled local entry must be untouched" + ); + } + + /// Memory holding a local key while the file holds a remote entry for + /// it, which only a hand edit under a running client produces. A delete + /// through `/keystores` stops the local key signing and leaves the other + /// route's file entry alone, and because that entry would bring the key + /// back at the next start, the answer is an error with no history + /// attached, never `deleted`. + #[tokio::test] + async fn a_delete_never_erases_the_other_routes_file_entry() { + let dir = tempfile::tempdir().expect("temp dir"); + let (context, protection) = context_with_record(dir.path()); + let secret: [u8; 32] = + hex::decode("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f") + .expect("valid hex") + .try_into() + .expect("32 bytes"); + let pubkey = context + .store + .write() + .await + .insert_secret("test", &secret) + .expect("inserts"); + // A record the key signed under, so there is history a `deleted` + // would hand over. + { + let mut protection = protection.lock().expect("not poisoned"); + protection.absorb(None, &[pubkey]).expect("registers"); + protection + .check_and_record_block(&pubkey, 1 << 40) + .expect("written") + .expect("allowed"); + } + let hex = encode_hex(&pubkey.0); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: hex.clone(), + signing: SigningDefinition::Web3Signer { + url: "https://signer.example:9000".to_string(), + }, + }]) + .save(dir.path()) + .expect("saves"); + let store = context.store.clone(); + let app = router(context, TOKEN.to_string()); + let delete = || { + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [hex] }), + ) + }; + let exported = |body: &serde_json::Value| -> serde_json::Value { + serde_json::from_str(body["slashing_protection"].as_str().expect("a string")) + .expect("json") + }; + + let (_, body) = send(app.clone(), delete()).await; + assert_eq!(body["data"][0]["status"], "error", "got {body}"); + let message = body["data"][0]["message"].as_str().expect("a message"); + assert!(message.contains("/eth/v1/remotekeys"), "{message}"); + assert!(store.read().await.is_empty(), "the local key stops signing"); + let on_disk = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(on_disk.0.len(), 1, "the remote entry is not this route's"); + assert!(on_disk.0[0].signing.is_remote()); + assert_eq!( + exported(&body)["data"], + serde_json::json!([]), + "no history for a key the next start would load again" + ); + + // Asked again, the key is the other route's and nothing else: not + // `not_active`, which would hand its history over after all. + let (_, body) = send(app, delete()).await; + assert_eq!(body["data"][0]["status"], "not_found", "got {body}"); + assert_eq!(exported(&body)["data"], serde_json::json!([])); + } + + /// One request, many keys: every key is judged against a single read of + /// the file and comes off it in a single save, with a key named twice + /// `deleted` the first time and gone the second, and a key this client + /// never had `not_found`. + #[tokio::test] + async fn a_delete_of_several_keys_judges_each_and_saves_once() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut store = ValidatorStore::new(); + let mut definitions = ValidatorDefinitions::default(); + let mut held = Vec::new(); + for last in 1..=3u8 { + let mut secret = [0u8; 32]; + secret[31] = last; + let pubkey = store.insert_secret("test", &secret).expect("inserts"); + let hex = encode_hex(&pubkey.0); + definitions.0.push(ValidatorDefinition { + enabled: true, + voting_public_key: hex.clone(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from(format!("{hex}.json")), + voting_keystore_password_path: PathBuf::from(&hex), + }, + }); + held.push(hex); + } + definitions.save(dir.path()).expect("saves"); + let store = Arc::new(RwLock::new(store)); + let app = router( + context_with_dirs(store.clone(), dir.path()), + TOKEN.to_string(), + ); + let stranger = encode_hex(&[0xaa; 48]); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ + "pubkeys": [held[0], stranger, held[1], "0xnothex", held[0]] + }), + ), + ) + .await; + let statuses: Vec<&str> = body["data"] + .as_array() + .expect("an array") + .iter() + .map(|entry| entry["status"].as_str().expect("a status")) + .collect(); + assert_eq!( + statuses, + ["deleted", "not_found", "deleted", "error", "not_found"], + "got {body}" + ); + + assert_eq!(store.read().await.len(), 1, "the third key still signs"); + let on_disk = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(on_disk.0.len(), 1); + assert_eq!(on_disk.0[0].voting_public_key, held[2]); + } + /// An imported key is registered in the slashing-protection record before /// it is loaded, so the next start finds a record for it instead of /// refusing it. @@ -1489,8 +1932,10 @@ mod tests { let previous = ValidatorDefinition { enabled: false, voting_public_key: format!("0x{}", keystore["pubkey"].as_str().expect("pubkey")), - voting_keystore_path: dir.path().join("old.json"), - voting_keystore_password_path: dir.path().join("old"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: dir.path().join("old.json"), + voting_keystore_password_path: dir.path().join("old"), + }, }; ValidatorDefinitions(vec![previous.clone()]) .save(dir.path()) @@ -1558,8 +2003,10 @@ mod tests { let previous = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path.clone(), - voting_keystore_password_path: password_path.clone(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: password_path.clone(), + }, }; ValidatorDefinitions(vec![previous.clone()]) .save(dir.path()) @@ -1612,8 +2059,10 @@ mod tests { let previous = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path.clone(), - voting_keystore_password_path: dir.path().join("old"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: dir.path().join("old"), + }, }; ValidatorDefinitions(vec![previous.clone()]) .save(dir.path()) diff --git a/crates/validator/src/http_api/mod.rs b/crates/validator/src/http_api/mod.rs index a00d3fdbc..94be9521f 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -27,6 +27,7 @@ use crate::slot_clock::SlotClock; pub mod keystores; pub mod proposer; +pub mod remotekeys; pub mod voluntary_exit; pub use keystores::SharedStore; @@ -56,12 +57,22 @@ pub struct KeymanagerContext { /// each handler across its entire read-modify-write cycle (not just /// `store`), is what actually orders those cycles. /// - /// Guards the file only, never `store`: holding it across the slow - /// EIP-2335 key derivation an import performs must not stall the duty - /// loop, which only ever waits on `store`. Where a handler needs both, - /// it takes this one first and `store` second, consistently, so the two - /// never deadlock against each other. + /// Every keymanager handler that changes the store or the file holds it + /// until both changes are done, so the two are always judged and changed + /// together: a handler that released it between writing the file and + /// inserting into the store would let a concurrent delete land in the + /// gap and leave memory and disk disagreeing. + /// + /// The duty loop never takes it, so holding it across the slow EIP-2335 + /// key derivation an import performs stalls only other keymanager + /// requests. Where a handler needs both, it takes this one first and + /// `store` second, consistently, so the two never deadlock against each + /// other, and it holds `store` only for in-memory work, never across file + /// I/O or the derivation. pub definitions_lock: Arc>, + /// `--web3signer-url`: the signer a remote key imported without a `url` + /// of its own is assigned to. `None` makes such an import an error. + pub default_remote_signer: Option, /// Where the `voluntary_exit` route resolves a key's validator index. pub beacon_node: Arc, /// What the `voluntary_exit` route signs with. @@ -153,6 +164,12 @@ pub fn router(context: KeymanagerContext, token: String) -> Router { .post(keystores::import) .delete(keystores::delete), ) + .route( + "/eth/v1/remotekeys", + get(remotekeys::list) + .post(remotekeys::import) + .delete(remotekeys::delete), + ) .route( "/eth/v1/validator/{pubkey}/feerecipient", get(proposer::get_fee_recipient) @@ -252,10 +269,7 @@ pub(crate) mod tests { let config = Config::mainnet(); let clock = SlotClock::from_config(0, &config); - let signing = Arc::new(SigningContext { - config, - genesis_validators_root: Root::ZERO, - }); + let signing = Arc::new(SigningContext::new(config, Root::ZERO)); let node = Arc::new(crate::beacon_node::mock::MockBeaconNode::new()); (node, signing, clock) } diff --git a/crates/validator/src/http_api/proposer.rs b/crates/validator/src/http_api/proposer.rs index b5d1a787f..45aeb00d3 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -370,6 +370,7 @@ mod tests { validators_dir: dir.path().to_path_buf(), secrets_dir: dir.path().to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + default_remote_signer: None, beacon_node, signing, clock, diff --git a/crates/validator/src/http_api/remotekeys.rs b/crates/validator/src/http_api/remotekeys.rs new file mode 100644 index 000000000..37886d36a --- /dev/null +++ b/crates/validator/src/http_api/remotekeys.rs @@ -0,0 +1,1020 @@ +//! `GET`, `POST` and `DELETE /eth/v1/remotekeys`: the validators a Web3Signer +//! signs for. +//! +//! The counterpart of `/eth/v1/keystores`, and shaped like it, with the key +//! material replaced by a URL. The two routes partition the store: this one +//! lists and deletes remote keys only, that one local keys only, and a key +//! asked of the wrong route is `not_found`. An import here of a key the +//! other route holds is `duplicate`, since the specification's word for "the +//! pubkey is already known to the validator client" covers both. +//! +//! Nothing is asked of the signer at import. The signer may start after this +//! client, or hold the key only later, and a key it turns out not to have +//! fails at its first signature, where it is logged and counted, the same way +//! an unreachable signer is. +//! +//! `readonly` is always `false`: this client has no keys it cannot delete. +//! +//! # Slashing protection +//! +//! A remote key is registered in this client's slashing-protection record +//! before it is loaded, as a local one is, and is checked against it before +//! every block and attestation: the signer keeps a record of its own, and +//! this one is kept as well, so the key is covered whichever of the two a +//! message meets first. Unlike `/eth/v1/keystores`, these routes carry no +//! history in or out, since the specification gives them no +//! `slashing_protection` field. A key imported with no record here starts as +//! never having signed, with a warning, and `DELETE` leaves the record where +//! it is, so a key imported again continues from it. + +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; + +use axum::{Json, extract::State}; +use ethlambda_types::beacon::primitives::BlsPubkey; +use serde::{Deserialize, Serialize}; +use tracing::{info, warn}; + +use crate::Error; +use crate::beacon_node::dto::{encode_hex, parse_pubkey}; +use crate::http_api::KeymanagerContext; +use crate::http_api::keystores::{ + DeleteRequest, Judged, KeyStatus, Origin, StatusResponse, delete_keys, save_definitions, +}; +use crate::keys::{ + SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore, Web3Signer, +}; + +#[derive(Debug, Serialize)] +pub struct ListResponse { + pub data: Vec, +} + +#[derive(Debug, Serialize)] +pub struct RemoteKey { + pub pubkey: String, + pub url: String, + pub readonly: bool, +} + +#[derive(Debug, Deserialize)] +pub struct ImportRequest { + pub remote_keys: Vec, +} + +#[derive(Debug, Deserialize)] +pub struct ImportRemoteKey { + pub pubkey: String, + /// The signer's base URL. Absent means `--web3signer-url`, and an import + /// with neither is an error for that key. + #[serde(default)] + pub url: Option, +} + +pub async fn list(State(context): State) -> Json { + let store = context.store.read().await; + let mut data: Vec = store + .remote_entries() + .into_iter() + .map(|(pubkey, url)| RemoteKey { + pubkey: encode_hex(&pubkey.0), + url, + readonly: false, + }) + .collect(); + // The store is a hash map; a stable order makes the answer diffable. + data.sort_by(|a, b| a.pubkey.cmp(&b.pubkey)); + Json(ListResponse { data }) +} + +/// The most keys one `POST /eth/v1/remotekeys` may carry. Larger than the +/// keystores route's cap, since a remote import does no key derivation, but +/// still bounded: the whole batch is judged and persisted while +/// `definitions_lock` is held, and a migration of thousands of keys is a few +/// requests rather than one unbounded one. +pub const MAX_REMOTE_KEYS_PER_IMPORT: usize = 1000; + +/// One key of an import, judged: the key and its signer's client to insert, +/// `None` for a duplicate, or the status to answer with instead. +type Prepared = std::result::Result)>, KeyStatus>; + +pub async fn import( + State(context): State, + Json(request): Json, +) -> Json { + let fail_all = |message: &str| { + let data = request + .remote_keys + .iter() + .map(|_| KeyStatus::error(message)) + .collect(); + Json(StatusResponse { data }) + }; + + if request.remote_keys.len() > MAX_REMOTE_KEYS_PER_IMPORT { + warn!( + count = request.remote_keys.len(), + "Rejecting oversized remote key import request" + ); + return fail_all(&format!( + "request carries {} remote keys, more than the limit of \ + {MAX_REMOTE_KEYS_PER_IMPORT}", + request.remote_keys.len() + )); + } + + // Held until the keys are in the store, not just until they are on disk. + // Every keymanager writer takes this first, so while it is held nothing + // else can change the file or the store: what is judged below against + // the two is still true at the insert, and no re-check is needed there. + // See `KeymanagerContext::definitions_lock` for the lock order. + let _definitions_guard = context.definitions_lock.lock().await; + + let mut definitions = match ValidatorDefinitions::open(&context.validators_dir) { + Ok(definitions) => definitions, + Err(err) => return fail_all(&err.to_string()), + }; + + // What the store holds, read under a guard released at once: nothing + // below holds the store across file I/O or client construction, so a + // duty's per-signature read never waits on this request. + let (mut known, mut signers) = { + let store = context.store.read().await; + let known: HashSet = store.pubkeys().into_iter().collect(); + (known, store.remote_signers()) + }; + // And what the file names, each entry decoded once for the request + // rather than once for every key in it. A disabled local entry, or one + // left behind by a failed delete, is still "already known": overwriting + // it would turn a key the operator disabled into a signing one. + known.extend(definitions.keyed().map(|(pubkey, _)| pubkey)); + + let mut prepared: Vec = request + .remote_keys + .iter() + .map(|key| prepare_import(&context, &mut known, &mut signers, &mut definitions, key)) + .collect(); + + // One save for the whole batch rather than one per key. A failure fails + // every key that would have been added, and none is put in the store: + // a key this process signs with must already be durable. + let staged: Vec = prepared + .iter() + .filter_map(|result| match result { + Ok(Some((pubkey, _))) => Some(*pubkey), + _ => None, + }) + .collect(); + let mut failure = None; + if !staged.is_empty() { + match save_definitions(&context, &definitions) { + Ok(()) => {} + // The entries are on disk and every reader sees them; only their + // survival of a power loss is in doubt, and an entry lost that way + // only removes a key. Counted as persisted, as a local import is. + Err(Error::Unflushed { path, source }) => warn!( + %path, + %source, + "Validator definitions written, but the directory could not be flushed" + ), + Err(err) => failure = Some(err.to_string()), + } + } + + // Every key is registered in the slashing-protection record, in one + // durable write, before any of them is loaded: the duty path must never + // meet a key with no record. After the save rather than before, for the + // reason `keystores::import` gives: a key whose save failed leaves no + // record behind to let a hand-written entry past the startup check later. + if failure.is_none() && !staged.is_empty() { + let registering = staged.clone(); + let registration = + crate::slashing_protection::blocking(&context.protection, move |protection| { + protection.absorb(None, ®istering) + }) + .await; + match registration { + Ok(absorbed) => { + if absorbed.registered > 0 { + warn!( + registered = absorbed.registered, + "Imported remote validator keys with no slashing-protection record \ + here; each starts as never having signed" + ); + } + } + // On disk and in memory, with only a power loss in doubt, and a + // registration lost that way is refused at the next start. So the + // keys are loaded, as `keystores::import` loads its own. + Err(Error::Unflushed { path, source }) => warn!( + %path, + %source, + "Slashing-protection records written, but the directory could not be flushed; \ + the imported keys are registered" + ), + // The write is known to have failed, so the entries this request + // added come off the file again: left there, the next start would + // refuse the whole client for a key with no record. + Err(err) => { + let added: HashSet = staged.iter().copied().collect(); + definitions.remove_where(|pubkey, _| added.contains(pubkey)); + failure = Some(match save_definitions(&context, &definitions) { + Ok(()) | Err(Error::Unflushed { .. }) => format!( + "not imported: its slashing-protection record could not be written: {err}" + ), + Err(undo) => { + tracing::error!( + %undo, + "Could not undo a failed remote import; the next start refuses its \ + keys until they have a slashing-protection record" + ); + format!( + "saved, but not loaded: its slashing-protection record could not be \ + written: {err}" + ) + } + }); + } + } + } + if let Some(message) = &failure { + for result in &mut prepared { + if matches!(result, Ok(Some(_))) { + *result = Err(KeyStatus::error(message)); + } + } + } + + // The write lock covers map inserts and nothing else: every client was + // built above. + let mut store = context.store.write().await; + let data = prepared + .into_iter() + .map(|result| match result { + Ok(Some((pubkey, signer))) => { + info!(pubkey = %encode_hex(&pubkey.0), url = signer.url(), "Imported remote validator key"); + store.insert_remote_signer(pubkey, signer); + KeyStatus { + status: "imported".to_string(), + message: None, + } + } + Ok(None) => KeyStatus::duplicate(), + Err(status) => status, + }) + .collect(); + drop(store); + + Json(StatusResponse { data }) +} + +/// Judge one key of an import and stage its definition: resolve the URL, +/// refuse a key already known, build (or reuse) the signer's client, and +/// append the entry to `definitions`, which the caller saves once for the +/// whole batch. +/// +/// `Ok(None)` is a duplicate. `known` is every key the store holds or the +/// file names, and gains each key staged here, so a key named twice in one +/// request is a duplicate the second time. +fn prepare_import( + context: &KeymanagerContext, + known: &mut HashSet, + signers: &mut HashMap>, + definitions: &mut ValidatorDefinitions, + key: &ImportRemoteKey, +) -> Prepared { + let pubkey = parse_pubkey(&key.pubkey).map_err(KeyStatus::error)?; + let url = match key + .url + .as_deref() + .or(context.default_remote_signer.as_deref()) + { + Some(url) => url.trim_end_matches('/').to_string(), + None => { + return Err(KeyStatus::error( + "no url supplied for this key and no --web3signer-url to default to", + )); + } + }; + + // Known to the store, or to the file. Judged before anything is built or + // staged. + if known.contains(&pubkey) { + return Ok(None); + } + + // Built here, outside the store's lock, and once per URL: the client is + // the one the store will keep, so the URL's shape is checked by building + // the thing that will use it, and an unusable URL is refused before it + // is persisted and refuses the next startup. + let signer = match signers.get(&url) { + Some(signer) => signer.clone(), + None => { + let signer = Arc::new(Web3Signer::new(&url).map_err(KeyStatus::error)?); + signers.insert(url.clone(), signer.clone()); + signer + } + }; + + definitions.0.push(ValidatorDefinition { + enabled: true, + voting_public_key: encode_hex(&pubkey.0), + signing: SigningDefinition::Web3Signer { url }, + }); + known.insert(pubkey); + Ok(Some((pubkey, signer))) +} + +pub async fn delete( + State(context): State, + Json(request): Json, +) -> Json { + let _definitions_guard = context.definitions_lock.lock().await; + + // Uncapped, and a migration off a signer deletes hundreds of keys in one + // request. `delete_keys` reads the file once, holds the store's write + // lock only to take the keys out of memory, and saves once, so a request + // that size does not stand between the duties and their signatures. + let data = delete_keys(&context, &request.pubkeys, Origin::Remote, judge_delete) + .await + .into_iter() + .map(|(_, status)| status) + .collect(); + + Json(StatusResponse { data }) +} + +/// The keystores route's judgement, gated the other way: only a remote key is +/// this route's to remove, and a local one is `not_found` here whatever its +/// state. The file decides `not_found`, for the reason spelled out on +/// `keystores::delete_keys`. +fn judge_delete( + context: &KeymanagerContext, + store: &mut ValidatorStore, + pubkey: &BlsPubkey, + entry: Option, +) -> Judged { + let on_disk = entry == Some(Origin::Remote); + let in_memory = store.remote_url(pubkey).is_some(); + if !on_disk && !in_memory { + return Judged::Absent; + } + + if in_memory { + store.remove(pubkey); + } + // The key's overrides go with it, unless the store still holds it. That + // is the file listing a remote entry for a key this process runs as a + // local one, which only a hand edit under a running client produces: the + // entry comes off the file, but the local key is the keystores route's + // and keeps proposing, so its fee recipient, graffiti and gas limit stay + // or its next blocks would pay the default address. + if !store.contains(pubkey) { + context.settings.forget(pubkey); + } + // The mirror of the keystores route's case: the store held the key + // remotely while the file lists it as a local keystore, an entry this + // route leaves alone and the next start would load. + Judged::Removed { + stranded: in_memory && entry == Some(Origin::Local), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::http_api::{SharedStore, router}; + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use http_body_util::BodyExt as _; + use std::sync::Arc; + use tokio::sync::RwLock; + use tower::ServiceExt as _; + + const TOKEN: &str = "test-token"; + const SIGNER: &str = "https://signer.example:9000"; + + fn pubkey_hex(byte: u8) -> String { + format!("0x{}", hex::encode([byte; 48])) + } + + fn context_with_dirs( + store: SharedStore, + dir: &std::path::Path, + default_remote_signer: Option<&str>, + ) -> KeymanagerContext { + let (beacon_node, signing, clock) = crate::http_api::tests::exit_route_context(); + KeymanagerContext { + store, + settings: Arc::new(crate::proposer_settings::ProposerSettings::new( + ethlambda_types::beacon::primitives::Bytes32::ZERO, + None, + )), + validators_dir: dir.to_path_buf(), + secrets_dir: dir.to_path_buf(), + definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + protection: crate::slashing_protection::SlashingProtection::in_memory(), + default_remote_signer: default_remote_signer.map(str::to_string), + beacon_node, + signing, + clock, + } + } + + async fn send(app: axum::Router, request: Request) -> (StatusCode, serde_json::Value) { + let response = app.oneshot(request).await.expect("responds"); + let status = response.status(); + let bytes = response + .into_body() + .collect() + .await + .expect("body") + .to_bytes(); + let json = serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null); + (status, json) + } + + fn authed(method: &str, uri: &str, body: serde_json::Value) -> Request { + Request::builder() + .method(method) + .uri(uri) + .header("authorization", format!("Bearer {TOKEN}")) + .header("content-type", "application/json") + .body(Body::from(body.to_string())) + .expect("request") + } + + fn statuses(body: &serde_json::Value) -> Vec { + body["data"] + .as_array() + .expect("data array") + .iter() + .map(|entry| entry["status"].as_str().expect("status").to_string()) + .collect() + } + + #[tokio::test] + async fn the_route_is_behind_the_bearer_token() { + let dir = tempfile::tempdir().expect("temp dir"); + let app = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + dir.path(), + None, + ), + TOKEN.to_string(), + ); + let request = Request::builder() + .uri("/eth/v1/remotekeys") + .body(Body::empty()) + .expect("request"); + let (status, _) = send(app, request).await; + assert_eq!(status, StatusCode::UNAUTHORIZED); + } + + #[tokio::test] + async fn import_list_delete_round_trip() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + + let (status, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": pubkey_hex(1), "url": SIGNER }, + { "pubkey": pubkey_hex(2), "url": format!("{SIGNER}/") }, + ] }), + ), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(statuses(&body), ["imported", "imported"]); + + let (status, body) = send( + app.clone(), + authed("GET", "/eth/v1/remotekeys", serde_json::json!({})), + ) + .await; + assert_eq!(status, StatusCode::OK); + let listed = body["data"].as_array().expect("data"); + assert_eq!(listed.len(), 2); + assert_eq!(listed[0]["pubkey"], pubkey_hex(1)); + assert_eq!(listed[0]["url"], SIGNER, "the trailing slash is normalized"); + assert_eq!(listed[1]["url"], SIGNER); + assert_eq!(listed[0]["readonly"], false); + + // Persisted: a fresh load from the directory holds both as remote. + let reloaded = ValidatorStore::load(dir.path()).expect("loads"); + assert_eq!(reloaded.remote_entries().len(), 2); + assert!(reloaded.local_pubkeys().is_empty()); + + // The keystores route does not list them. + let (_, body) = send( + app.clone(), + authed("GET", "/eth/v1/keystores", serde_json::json!({})), + ) + .await; + assert!(body["data"].as_array().expect("data").is_empty()); + + let (status, body) = send( + app.clone(), + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1), pubkey_hex(3)] }), + ), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(statuses(&body), ["deleted", "not_found"]); + assert!( + body.get("slashing_protection").is_none(), + "the remote route has no interchange to report" + ); + + assert_eq!(store.read().await.len(), 1); + let reloaded = ValidatorStore::load(dir.path()).expect("loads"); + assert_eq!(reloaded.len(), 1, "the delete is persisted"); + } + + #[tokio::test] + async fn a_missing_url_falls_back_to_the_configured_signer_or_errors() { + let dir = tempfile::tempdir().expect("temp dir"); + let with_default = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + dir.path(), + Some(SIGNER), + ), + TOKEN.to_string(), + ); + let (_, body) = send( + with_default.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1) }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + let (_, body) = send( + with_default, + authed("GET", "/eth/v1/remotekeys", serde_json::json!({})), + ) + .await; + assert_eq!(body["data"][0]["url"], SIGNER); + + let other = tempfile::tempdir().expect("temp dir"); + let without_default = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + other.path(), + None, + ), + TOKEN.to_string(), + ); + let (_, body) = send( + without_default, + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1) }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error"]); + assert!( + body["data"][0]["message"] + .as_str() + .expect("message") + .contains("--web3signer-url") + ); + assert!( + ValidatorStore::load(other.path()) + .expect("loads") + .is_empty(), + "an errored import persists nothing" + ); + } + + #[tokio::test] + async fn an_already_held_key_is_a_duplicate_whichever_route_holds_it() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut store = ValidatorStore::new(); + let local = store + .insert_secret( + "test", + &hex::decode("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f") + .expect("hex") + .try_into() + .expect("32 bytes"), + ) + .expect("inserts"); + let store = Arc::new(RwLock::new(store)); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + + let import = |pubkey: String| { + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey, "url": SIGNER }] }), + ) + }; + let (_, body) = send(app.clone(), import(pubkey_hex(1))).await; + assert_eq!(statuses(&body), ["imported"]); + let (_, body) = send(app.clone(), import(pubkey_hex(1))).await; + assert_eq!(statuses(&body), ["duplicate"]); + let (_, body) = send(app.clone(), import(encode_hex(&local.0))).await; + assert_eq!(statuses(&body), ["duplicate"]); + + assert!( + store.read().await.remote_url(&local).is_none(), + "a duplicate must not replace the local key with a remote one" + ); + } + + #[tokio::test] + async fn a_bad_pubkey_or_url_is_an_error_for_that_key_alone() { + let dir = tempfile::tempdir().expect("temp dir"); + let app = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + dir.path(), + None, + ), + TOKEN.to_string(), + ); + let (_, body) = send( + app, + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": "0xnope", "url": SIGNER }, + { "pubkey": pubkey_hex(1), "url": "ftp://signer" }, + { "pubkey": pubkey_hex(2), "url": SIGNER }, + ] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error", "error", "imported"]); + } + + /// The two routes partition the store: a key each one holds is + /// `not_found` to the other's delete, and only its own route removes it. + #[tokio::test] + async fn each_route_deletes_only_its_own_keys() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1), "url": SIGNER }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + + let (_, body) = send( + app.clone(), + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + // `not_found`, not `not_active`: the key has a slashing-protection + // record, but it is held, by the other route, and still signing. Its + // record is not handed over as though the key were gone. + assert_eq!(statuses(&body), ["not_found"]); + let exported: serde_json::Value = + serde_json::from_str(body["slashing_protection"].as_str().expect("a string")) + .expect("an interchange"); + assert_eq!(exported["data"], serde_json::json!([]), "got {exported}"); + assert_eq!( + store.read().await.len(), + 1, + "the keystores route left it alone" + ); + assert_eq!(ValidatorStore::load(dir.path()).expect("loads").len(), 1); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["deleted"]); + assert!(store.read().await.is_empty()); + } + + /// One request, many keys: each is judged against a single read of the + /// file and comes off it in a single save. A key named twice is `deleted` + /// the first time and gone the second, and a key this client never had + /// is `not_found`. + #[tokio::test] + async fn a_delete_of_several_keys_judges_each_and_saves_once() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), Some(SIGNER)), + TOKEN.to_string(), + ); + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": pubkey_hex(1) }, + { "pubkey": pubkey_hex(2) }, + { "pubkey": pubkey_hex(3) }, + ] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported", "imported", "imported"]); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [ + pubkey_hex(1), pubkey_hex(9), pubkey_hex(2), "0xnothex", pubkey_hex(1), + ] }), + ), + ) + .await; + assert_eq!( + statuses(&body), + ["deleted", "not_found", "deleted", "error", "not_found"], + "got {body}" + ); + + let kept = parse_pubkey(&pubkey_hex(3)).expect("pubkey"); + assert_eq!(store.read().await.pubkeys(), vec![kept]); + let on_disk = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(on_disk.0.len(), 1); + assert_eq!(on_disk.0[0].voting_public_key, pubkey_hex(3)); + } + + /// The file listing a remote entry for a key this process runs as a local + /// one, which only a hand edit under a running client produces. The + /// entry is this route's to remove, but the local key is not: it keeps + /// signing, and it keeps the settings it proposes with. + #[tokio::test] + async fn a_delete_leaves_a_local_key_under_the_same_pubkey_its_settings() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut store = ValidatorStore::new(); + let mut secret = [0u8; 32]; + secret[31] = 1; + let pubkey = store.insert_secret("test", &secret).expect("inserts"); + let hex = encode_hex(&pubkey.0); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: hex.clone(), + signing: SigningDefinition::Web3Signer { + url: SIGNER.to_string(), + }, + }]) + .save(dir.path()) + .expect("saves"); + let store = Arc::new(RwLock::new(store)); + let context = context_with_dirs(store.clone(), dir.path(), None); + let settings = context.settings.clone(); + let fee_recipient = ethlambda_types::beacon::primitives::H160([0xcd; 20]); + settings.set_fee_recipient(&pubkey, fee_recipient); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [hex] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["deleted"], "got {body}"); + + let on_disk = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert!(on_disk.0.is_empty(), "the remote entry is off the file"); + assert!( + store.read().await.contains(&pubkey), + "the local key is the other route's and still signs" + ); + assert_eq!( + settings.fee_recipient(&pubkey), + Some(fee_recipient), + "a key that keeps proposing keeps its fee recipient" + ); + } + + /// The mirror of the keystores route's case: the store holds the key + /// through a signer while the file lists it as a local keystore. The + /// delete stops the key signing and leaves the other route's entry + /// alone, and since that entry would bring the key back at the next + /// start, the answer is an error rather than `deleted`. + #[tokio::test] + async fn a_delete_does_not_report_gone_a_key_the_file_lists_as_local() { + let dir = tempfile::tempdir().expect("temp dir"); + let pubkey = parse_pubkey(&pubkey_hex(1)).expect("pubkey"); + let mut store = ValidatorStore::new(); + store.insert_remote(pubkey, SIGNER).expect("inserts"); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: pubkey_hex(1), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: "keystore.json".into(), + voting_keystore_password_path: "password.txt".into(), + }, + }]) + .save(dir.path()) + .expect("saves"); + let store = Arc::new(RwLock::new(store)); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error"], "got {body}"); + let message = body["data"][0]["message"].as_str().expect("a message"); + assert!(message.contains("/eth/v1/keystores"), "{message}"); + assert!( + store.read().await.is_empty(), + "the remote key stops signing" + ); + let on_disk = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(on_disk.0.len(), 1, "the local entry is not this route's"); + assert!(!on_disk.0[0].signing.is_remote()); + } + + /// 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( + store: SharedStore, + dir: &std::path::Path, + ) -> ( + KeymanagerContext, + crate::slashing_protection::SharedProtection, + ) { + let protection = crate::slashing_protection::SlashingProtection::open( + dir, + ethlambda_types::beacon::primitives::Root::ZERO, + &[], + false, + ) + .expect("opens with no keys"); + let mut context = context_with_dirs(store, dir, None); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + let protection = context.protection.clone(); + (context, protection) + } + + /// A remote key is registered in this client's slashing-protection record + /// before it is loaded, as a local one is: the duty path checks every key + /// against the record, and the next start refuses one that has none. + #[tokio::test] + async fn an_imported_remote_key_is_registered_for_slashing_protection() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let (context, protection) = context_with_record(store.clone(), dir.path()); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1), "url": SIGNER }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + + let pubkey = parse_pubkey(&pubkey_hex(1)).expect("valid"); + assert!(store.read().await.contains(&pubkey), "loaded"); + assert!( + protection.lock().expect("lock").contains(&pubkey), + "and registered" + ); + } + + /// A registration that fails costs the request only: the entries it added + /// come off the file again and no key is loaded. Left on the file, a key + /// with no record would have the next start refuse the whole client. + #[tokio::test] + async fn a_failed_registration_rolls_the_remote_import_back() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let (context, protection) = context_with_record(store.clone(), dir.path()); + // A directory in the record's place makes the registration's rename + // fail, whoever runs the test. + let record = protection + .lock() + .expect("lock") + .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/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": pubkey_hex(1), "url": SIGNER }, + { "pubkey": pubkey_hex(2), "url": SIGNER }, + ] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error", "error"], "got {body}"); + assert!( + body["data"][0]["message"] + .as_str() + .unwrap_or_default() + .starts_with("not imported"), + "got {body}" + ); + assert!(store.read().await.is_empty(), "not loaded"); + assert!( + ValidatorDefinitions::open(dir.path()) + .expect("opens") + .0 + .is_empty(), + "the next start finds no key to refuse" + ); + } + + /// A remote entry still on disk after a failed delete must not be + /// `not_found`, for the reason the keystores route's is not: an operator + /// who reads `not_found` moves the key elsewhere, and the next restart + /// brings it back here too. + #[tokio::test] + async fn a_remote_key_still_on_disk_is_not_reported_not_found() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1), "url": SIGNER }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + + // Gone from memory, still on disk. + let pubkey = parse_pubkey(&pubkey_hex(1)).expect("pubkey"); + assert!(store.write().await.remove(&pubkey)); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["deleted"]); + assert!(ValidatorStore::load(dir.path()).expect("loads").is_empty()); + } +} diff --git a/crates/validator/src/http_api/voluntary_exit.rs b/crates/validator/src/http_api/voluntary_exit.rs index 3dd146754..68ef7370d 100644 --- a/crates/validator/src/http_api/voluntary_exit.rs +++ b/crates/validator/src/http_api/voluntary_exit.rs @@ -84,13 +84,15 @@ pub async fn create_signed_voluntary_exit( epoch, validator_index: entry.index, }; - // The read lock is not held across the round trip to the beacon node, so - // a keystore delete can land between the check above and here. Signing - // then finds no key, which is the same 404 the check gives. - let store = context.store.read().await; + // No guard on the store is held here or across the round trip to the + // beacon node above: the signing path takes its own for the lookup alone, + // since for a key in a remote signer the signature is a round trip too. + // So a delete can land between the check above and here. Signing then + // finds no key, which is the same 404 the check gives. let signature = context .signing - .sign_voluntary_exit(&store, &pubkey, &exit, current_epoch) + .sign_voluntary_exit(&context.store, &pubkey, &exit, current_epoch) + .await .map_err(|err| match err { crate::Error::UnknownValidator(_) => ApiError::NotFound(format!( "no validator with pubkey {} is held by this client", @@ -98,7 +100,6 @@ pub async fn create_signed_voluntary_exit( )), other => ApiError::Internal(format!("signing failed: {other}")), })?; - drop(store); info!( pubkey = %encode_hex(&pubkey.0), @@ -182,10 +183,7 @@ mod tests { .as_secs() - epochs * 32 * 12; let clock = SlotClock::from_config(genesis_time, &config); - let signing = Arc::new(SigningContext { - config, - genesis_validators_root: Root::repeat_byte(3), - }); + let signing = Arc::new(SigningContext::new(config, Root::repeat_byte(3))); let dir = tempfile::tempdir().expect("temp dir"); let context = KeymanagerContext { store: Arc::new(RwLock::new(store)), @@ -195,6 +193,7 @@ mod tests { definitions_lock: Arc::new(tokio::sync::Mutex::new(())), definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), protection: crate::slashing_protection::SlashingProtection::in_memory(), + default_remote_signer: None, beacon_node: Arc::new(node), signing: signing.clone(), clock: clock.clone(), diff --git a/crates/validator/src/keys/definitions.rs b/crates/validator/src/keys/definitions.rs index 3b091585a..7d62f1b3e 100644 --- a/crates/validator/src/keys/definitions.rs +++ b/crates/validator/src/keys/definitions.rs @@ -4,10 +4,14 @@ //! keystores themselves. It is what the keymanager API mutates, so a key //! imported at runtime is still there after a restart. +use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use ethlambda_types::beacon::primitives::BlsPubkey; +use serde::de::IgnoredAny; use serde::{Deserialize, Serialize}; +use crate::beacon_node::dto::parse_pubkey; use crate::error::{Error, Result}; use crate::secure_fs; @@ -15,16 +19,190 @@ use crate::secure_fs; pub const DEFINITIONS_FILE: &str = "validator_definitions.yml"; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "RawDefinition", into = "RawDefinition")] pub struct ValidatorDefinition { /// Whether this validator should perform duties. A disabled entry keeps its /// keystore on disk and is simply not loaded. pub enabled: bool, /// The validator's BLS public key, `0x`-prefixed hex. pub voting_public_key: String, - /// Path to the EIP-2335 keystore. - pub voting_keystore_path: PathBuf, - /// Path to the file holding that keystore's password. - pub voting_keystore_password_path: PathBuf, + /// Where the key is and how it signs. Kept at the top level of the entry + /// in the file, beside a `type` naming it; see [`RawDefinition`]. + pub signing: SigningDefinition, +} + +/// How one definition's key signs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SigningDefinition { + /// An EIP-2335 keystore on disk, decrypted into this process at startup. + LocalKeystore { + /// Path to the EIP-2335 keystore. + voting_keystore_path: PathBuf, + /// Path to the file holding that keystore's password. + voting_keystore_password_path: PathBuf, + }, + /// A Web3Signer holding the key; `url` is the signer's base URL. + Web3Signer { url: String }, +} + +/// The `type` of an entry, under the names Lighthouse's definitions file uses. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +enum SigningKind { + LocalKeystore, + Web3signer, +} + +/// One entry exactly as the file holds it. +/// +/// Every entry is written with a `type`, and one that has a `type` must carry +/// that method's fields and no other's. An entry without one, which is every +/// entry written before remote signers existed, is read by its fields, and is +/// refused when it names both a keystore and a `url`: loading that as either +/// would silently ignore half of what the operator wrote, and loading it as +/// local would bypass the remote signer and its slashing protection. +/// +/// A field this client does not read is refused on a web3signer entry and +/// skipped on a local one. On a remote entry it is most likely one of +/// Lighthouse's signer settings (`root_certificate_path`, say), and an +/// operator who set it expects it to take effect: skipped, the key would sign +/// through a connection configured differently from the one they wrote down. +/// A local entry has no such field, so a file Lighthouse wrote, with its own +/// per-validator settings on each entry, still loads. +#[derive(Serialize, Deserialize)] +struct RawDefinition { + enabled: bool, + voting_public_key: String, + #[serde(rename = "type", default, skip_serializing_if = "Option::is_none")] + kind: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + voting_keystore_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + voting_keystore_password_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + url: Option, + /// Every other field the entry carries, by name: what a web3signer entry + /// is refused for. Read only, so a rewritten entry holds the fields above + /// and nothing else. + #[serde(flatten, skip_serializing)] + unread: BTreeMap, +} + +/// The one unread field a web3signer entry may carry. Lighthouse writes +/// `description` on every entry, an empty one included, so refusing it would +/// refuse every remote entry Lighthouse wrote; it is a label, and changes +/// nothing about how the key signs. +const DESCRIPTION_FIELD: &str = "description"; + +impl TryFrom for ValidatorDefinition { + type Error = String; + + fn try_from(raw: RawDefinition) -> std::result::Result { + let names_keystore = + raw.voting_keystore_path.is_some() || raw.voting_keystore_password_path.is_some(); + let names_url = raw.url.is_some(); + let kind = match raw.kind { + Some(SigningKind::LocalKeystore) if names_url => { + return Err(format!( + "{}: a local_keystore entry must not carry a url", + raw.voting_public_key + )); + } + Some(SigningKind::Web3signer) if names_keystore => { + return Err(format!( + "{}: a web3signer entry must not carry keystore paths", + raw.voting_public_key + )); + } + Some(kind) => kind, + None if names_keystore && names_url => { + return Err(format!( + "{}: the entry names both a keystore and a url; remove one, or set \ + `type: local_keystore` or `type: web3signer`", + raw.voting_public_key + )); + } + None if names_url => SigningKind::Web3signer, + None if names_keystore => SigningKind::LocalKeystore, + None => { + return Err(format!( + "{}: the entry names no way to sign: it needs keystore paths or a url", + raw.voting_public_key + )); + } + }; + if kind == SigningKind::Web3signer { + let unread: Vec<&str> = raw + .unread + .keys() + .map(String::as_str) + .filter(|field| *field != DESCRIPTION_FIELD) + .collect(); + if !unread.is_empty() { + return Err(format!( + "{}: a web3signer entry carries fields this client does not implement: {}", + raw.voting_public_key, + unread.join(", ") + )); + } + } + let missing = |field: &str| format!("{}: missing field `{field}`", raw.voting_public_key); + let signing = match kind { + SigningKind::LocalKeystore => SigningDefinition::LocalKeystore { + voting_keystore_path: raw + .voting_keystore_path + .clone() + .ok_or_else(|| missing("voting_keystore_path"))?, + voting_keystore_password_path: raw + .voting_keystore_password_path + .clone() + .ok_or_else(|| missing("voting_keystore_password_path"))?, + }, + SigningKind::Web3signer => SigningDefinition::Web3Signer { + url: raw.url.clone().ok_or_else(|| missing("url"))?, + }, + }; + Ok(Self { + enabled: raw.enabled, + voting_public_key: raw.voting_public_key, + signing, + }) + } +} + +impl From for RawDefinition { + fn from(definition: ValidatorDefinition) -> Self { + let (kind, voting_keystore_path, voting_keystore_password_path, url) = + match definition.signing { + SigningDefinition::LocalKeystore { + voting_keystore_path, + voting_keystore_password_path, + } => ( + SigningKind::LocalKeystore, + Some(voting_keystore_path), + Some(voting_keystore_password_path), + None, + ), + SigningDefinition::Web3Signer { url } => { + (SigningKind::Web3signer, None, None, Some(url)) + } + }; + Self { + enabled: definition.enabled, + voting_public_key: definition.voting_public_key, + kind: Some(kind), + voting_keystore_path, + voting_keystore_password_path, + url, + unread: BTreeMap::new(), + } + } +} + +impl SigningDefinition { + pub fn is_remote(&self) -> bool { + matches!(self, Self::Web3Signer { .. }) + } } #[derive(Debug, Clone, Default, Serialize, Deserialize)] @@ -130,9 +308,11 @@ impl ValidatorDefinitions { let pubkey = format!("0x{}", pubkey.trim_start_matches("0x")); definitions.push(ValidatorDefinition { enabled: true, - voting_keystore_password_path: secrets_dir.join(&pubkey), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path, + voting_keystore_password_path: secrets_dir.join(&pubkey), + }, voting_public_key: pubkey, - voting_keystore_path: keystore_path, }); } // Directory order is not stable across filesystems; sorting makes the @@ -148,6 +328,32 @@ impl ValidatorDefinitions { pub fn enabled(&self) -> impl Iterator { self.0.iter().filter(|definition| definition.enabled) } + + /// Every entry beside the key it names, each `voting_public_key` decoded + /// once. + /// + /// What a caller asking about several keys builds its lookup from, rather + /// than decoding every entry again for each key it asks about. An entry + /// whose `voting_public_key` does not parse names no key and is left + /// out: nothing can ask for it by key, and [`Self::remove_where`] keeps + /// it, so no caller may count it as found. + pub fn keyed(&self) -> impl Iterator { + self.0.iter().filter_map(|definition| { + let pubkey = parse_pubkey(&definition.voting_public_key).ok()?; + Some((pubkey, definition)) + }) + } + + /// Drop every entry `drop` names, judged by its key and by how it signs. + /// An entry whose key does not parse is kept; see [`Self::keyed`]. + pub fn remove_where(&mut self, drop: impl Fn(&BlsPubkey, &SigningDefinition) -> bool) { + self.0.retain( + |definition| match parse_pubkey(&definition.voting_public_key) { + Ok(pubkey) => !drop(&pubkey, &definition.signing), + Err(_) => true, + }, + ); + } } #[cfg(test)] @@ -186,8 +392,11 @@ mod tests { let definitions = ValidatorDefinitions::open(validators.path()).unwrap(); assert_eq!(definitions.0[0].voting_public_key, "0xaa"); assert_eq!( - definitions.0[0].voting_keystore_password_path, - secrets.path().join("0xaa") + definitions.0[0].signing, + SigningDefinition::LocalKeystore { + voting_keystore_path: validators.path().join("0xaa").join("voting-keystore.json"), + voting_keystore_password_path: secrets.path().join("0xaa"), + } ); assert!(definitions.0.iter().all(|definition| definition.enabled)); } @@ -216,11 +425,213 @@ mod tests { ValidatorDefinition { enabled, voting_public_key: pubkey.to_string(), - voting_keystore_path: PathBuf::from("keystore.json"), - voting_keystore_password_path: PathBuf::from("password.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, + } + } + + fn remote_definition(pubkey: &str, url: &str) -> ValidatorDefinition { + ValidatorDefinition { + enabled: true, + voting_public_key: pubkey.to_string(), + signing: SigningDefinition::Web3Signer { + url: url.to_string(), + }, } } + /// A file written before remote signers existed has no `url` and no tag, + /// and must read back as the local entry it always was. + #[test] + fn a_pre_remote_signer_file_still_parses() { + let dir = tempfile::tempdir().expect("temp dir"); + let legacy = "- enabled: true\n voting_public_key: '0xaa'\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n"; + std::fs::write(dir.path().join(DEFINITIONS_FILE), legacy).expect("writes"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(read.0, vec![definition("0xaa", true)]); + } + + #[test] + fn remote_and_local_entries_round_trip_together() { + let dir = tempfile::tempdir().expect("temp dir"); + let written = ValidatorDefinitions(vec![ + definition("0xaa", true), + remote_definition("0xbb", "https://signer:9000"), + ]); + written.save(dir.path()).expect("saves"); + + let text = std::fs::read_to_string(dir.path().join(DEFINITIONS_FILE)).expect("reads"); + assert!(text.contains("url: https://signer:9000"), "{text}"); + assert!(text.contains("type: web3signer"), "{text}"); + assert!(text.contains("type: local_keystore"), "{text}"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(read.0, written.0); + assert!(read.0[1].signing.is_remote()); + assert!(!read.0[0].signing.is_remote()); + } + + /// An entry with neither a keystore nor a URL describes no way to sign, + /// and must be refused rather than read as one of the variants. + #[test] + fn an_entry_with_no_signing_method_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write( + dir.path().join(DEFINITIONS_FILE), + "- enabled: true\n voting_public_key: '0xaa'\n", + ) + .expect("writes"); + assert!(ValidatorDefinitions::open(dir.path()).is_err()); + } + + /// Open a definitions file holding `yaml`, rendering a refusal as text. + fn open_text(yaml: &str) -> std::result::Result { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write(dir.path().join(DEFINITIONS_FILE), yaml).expect("writes"); + ValidatorDefinitions::open(dir.path()).map_err(|err| err.to_string()) + } + + /// An entry naming both a keystore and a url must not load as either: + /// read as local, it would sign in this process and bypass the remote + /// signer's slashing protection without a word. + #[test] + fn an_untyped_entry_with_two_signing_methods_is_refused() { + let err = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n \ + url: https://signer:9000\n", + ) + .expect_err("ambiguous entry"); + assert!(err.contains("both a keystore and a url"), "{err}"); + } + + #[test] + fn a_typed_entry_must_carry_only_its_own_fields() { + let local_with_url = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: local_keystore\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n \ + url: https://signer:9000\n", + ); + assert!(local_with_url.is_err()); + let remote_with_keystore = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n voting_keystore_path: keystore.json\n", + ); + assert!(remote_with_keystore.is_err()); + let remote = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n", + ) + .expect("a well-formed remote entry"); + assert_eq!( + remote.0, + vec![remote_definition("0xaa", "https://signer:9000")] + ); + } + + /// The refusal names what is missing, rather than serde's generic "did + /// not match any variant". + #[test] + fn a_local_entry_missing_its_password_path_names_the_field() { + let err = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + voting_keystore_path: keystore.json\n", + ) + .expect_err("incomplete entry"); + assert!(err.contains("voting_keystore_password_path"), "{err}"); + } + + /// A field this client does not implement is reported on a remote entry, + /// not ignored: an operator who set `root_certificate_path` expects it to + /// take effect. Every such field is named, whether the entry says its + /// `type` or is read as remote by its `url`. + #[test] + fn an_unimplemented_field_on_a_remote_entry_is_refused() { + let typed = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n root_certificate_path: ca.pem\n \ + request_timeout_ms: 500\n", + ) + .expect_err("unimplemented fields"); + assert!( + typed.contains("request_timeout_ms, root_certificate_path"), + "{typed}" + ); + + let untyped = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + url: https://signer:9000\n suggested_fee_recipient: '0x00'\n", + ) + .expect_err("unimplemented field"); + assert!(untyped.contains("suggested_fee_recipient"), "{untyped}"); + } + + /// Lighthouse writes `description` on every entry, so a remote entry it + /// wrote, with no signer settings of its own, has to load. + #[test] + fn a_remote_entry_may_carry_a_description() { + let read = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n description: ''\n \ + type: web3signer\n url: https://signer:9000\n", + ) + .expect("a remote entry as Lighthouse writes it"); + assert_eq!( + read.0, + vec![remote_definition("0xaa", "https://signer:9000")] + ); + } + + /// A local entry is read past the fields this client does not have, typed + /// or not, so a file Lighthouse wrote loads as it did before entries had + /// a `type`. + #[test] + fn a_field_this_client_does_not_read_is_skipped_on_a_local_entry() { + let read = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + graffiti: mine\n builder_proposals: true\n description: ''\n \ + type: local_keystore\n voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n\ + - enabled: false\n voting_public_key: '0xbb'\n gas_limit: 30000000\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n", + ) + .expect("local entries with Lighthouse's own fields"); + assert_eq!( + read.0, + vec![definition("0xaa", true), definition("0xbb", false)] + ); + } + + /// The fields read past on the way in are not written back: an entry is + /// saved as the fields this client has, as it always was. + #[test] + fn a_skipped_field_is_not_written_back() { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write( + dir.path().join(DEFINITIONS_FILE), + "- enabled: true\n voting_public_key: '0xaa'\n graffiti: mine\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n", + ) + .expect("writes"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + read.save(dir.path()).expect("saves"); + let text = std::fs::read_to_string(dir.path().join(DEFINITIONS_FILE)).expect("reads"); + assert!(!text.contains("graffiti"), "{text}"); + assert_eq!( + ValidatorDefinitions::open(dir.path()).expect("opens").0, + read.0 + ); + } + #[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/mod.rs b/crates/validator/src/keys/mod.rs index ac89a776a..b4947d41c 100644 --- a/crates/validator/src/keys/mod.rs +++ b/crates/validator/src/keys/mod.rs @@ -3,6 +3,8 @@ pub mod definitions; pub mod keystore; pub mod store; +pub mod web3signer; -pub use definitions::{ValidatorDefinition, ValidatorDefinitions}; +pub use definitions::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions}; pub use store::{SigningMethod, ValidatorStore}; +pub use web3signer::Web3Signer; diff --git a/crates/validator/src/keys/store.rs b/crates/validator/src/keys/store.rs index db45bb861..8c1a2e19b 100644 --- a/crates/validator/src/keys/store.rs +++ b/crates/validator/src/keys/store.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::path::Path; +use std::sync::{Arc, Weak}; use blst::min_pk::SecretKey; use ethlambda_types::beacon::primitives::{BLS_PUBKEY_SIZE, BlsPubkey}; @@ -10,13 +11,15 @@ use zeroize::Zeroizing; use crate::beacon_node::dto::{encode_hex, parse_pubkey}; use crate::error::{Error, Result}; -use crate::keys::definitions::ValidatorDefinitions; +use crate::keys::definitions::{DEFINITIONS_FILE, SigningDefinition, ValidatorDefinitions}; use crate::keys::keystore::Keystore; +use crate::keys::web3signer::Web3Signer; /// How one validator produces a signature. /// -/// One variant today. It is an enum from the start so that adding a remote -/// signer later is a new arm rather than a reshaping of every call site. +/// Cheap to clone: both arms are an `Arc`, so a signing path can take a copy +/// out of the store under a short read guard and sign after releasing it, +/// which matters for the remote arm, whose signature is an HTTP round trip. /// /// **Never derive `Debug` on this type, or on anything holding it.** /// `blst::min_pk::SecretKey` zeroizes on drop but still derives a plain @@ -24,20 +27,39 @@ use crate::keys::keystore::Keystore; /// chain would put a validator's signing key into a log line. `Zeroizing` /// is no protection either: it forwards `Debug` to the type it wraps, so it /// controls the key's lifetime in memory, not whether it can be printed. +#[derive(Clone)] pub enum SigningMethod { /// The secret key is held in this process, decrypted from a local keystore. - LocalKeystore { secret_key: Box }, + LocalKeystore { secret_key: Arc }, + /// The key is held by a Web3Signer, which signs on this client's behalf. + Web3Signer { signer: Arc }, +} + +impl SigningMethod { + pub fn is_remote(&self) -> bool { + matches!(self, Self::Web3Signer { .. }) + } } /// Every validator this client signs for, keyed by public key. pub struct ValidatorStore { validators: HashMap, + /// One client per remote signer URL, shared by every validator on it, so + /// a hundred keys behind one signer are one connection pool rather than a + /// hundred. + /// + /// Weak: the validators using a client hold it, and so does any signature + /// in flight through it. A client stops being reachable here once the + /// last of those lets go, with no pruning pass having to guess whether an + /// in-flight request still counts as a user. + signers: HashMap>, } impl ValidatorStore { pub fn new() -> Self { Self { validators: HashMap::new(), + signers: HashMap::new(), } } @@ -46,14 +68,45 @@ impl ValidatorStore { /// A keystore that fails to decrypt aborts startup rather than being /// skipped: a validator that silently does not attest looks exactly like a /// healthy one from inside this process, and the operator finds out from - /// missed-attestation penalties days later. + /// missed-attestation penalties days later. A remote entry is registered + /// without contacting its signer: the signer may well start after this + /// process does, and a key it turns out not to hold fails at its first + /// signature, where it is logged and counted, rather than at startup. pub fn load(validators_dir: &Path) -> Result { let definitions = ValidatorDefinitions::open(validators_dir)?; let mut store = Self::new(); - for definition in definitions.enabled() { - let keystore_path = resolve(validators_dir, &definition.voting_keystore_path); - let password_path = resolve(validators_dir, &definition.voting_keystore_password_path); + let entries = definitions.0.iter().enumerate(); + for (position, definition) in entries.filter(|(_, definition)| definition.enabled) { + let (voting_keystore_path, voting_keystore_password_path) = match &definition.signing { + SigningDefinition::Web3Signer { url } => { + // Named by its place in the file, never by its `url`. The + // URL here is as the operator wrote it: `Web3Signer::new` + // has not seen it yet, so it may still carry the + // credentials that would be refused there, and this + // error is logged as startup aborts. The position also + // tells entries apart, which a URL shared by every key + // on one signer does not. + let pubkey = parse_pubkey(&definition.voting_public_key).map_err(|err| { + Error::Keystore { + path: validators_dir.join(DEFINITIONS_FILE).display().to_string(), + reason: format!( + "entry {} (a web3signer entry): its voting_public_key is \ + unreadable: {err}", + position + 1 + ), + } + })?; + store.insert_remote(pubkey, url)?; + continue; + } + SigningDefinition::LocalKeystore { + voting_keystore_path, + voting_keystore_password_path, + } => (voting_keystore_path, voting_keystore_password_path), + }; + let keystore_path = resolve(validators_dir, voting_keystore_path); + let password_path = resolve(validators_dir, voting_keystore_password_path); let derived = store.insert_keystore_file(&keystore_path, &password_path)?; @@ -66,7 +119,7 @@ impl ValidatorStore { // The keymanager's delete is where that becomes dangerous. It // removes from the store by the derived key but removes from the // definitions file by the *declared* one - // (`http_api::keystores::persist_delete`), so a mismatch makes the + // (`http_api::keystores::delete_keys`), so a mismatch makes the // file retain the entry while the response reports `deleted`. The // next restart loads the keystore again and the validator signs // again, which is exactly the shape that produces a double vote @@ -97,7 +150,11 @@ impl ValidatorStore { } } - info!(count = store.len(), "Loaded validator keys"); + info!( + count = store.len(), + remote = store.remote_entries().len(), + "Loaded validator keys" + ); Ok(store) } @@ -160,12 +217,65 @@ impl ValidatorStore { self.validators.insert( pubkey, SigningMethod::LocalKeystore { - secret_key: Box::new(secret_key), + secret_key: Arc::new(secret_key), }, ); Ok(pubkey) } + /// Add a validator whose key a Web3Signer at `url` holds, replacing any + /// method the store had for it, building the signer's client if no other + /// validator already shares one. + /// + /// Nothing is asked of the signer here; see [`Self::load`] for why. The + /// URL is checked for shape, which is the one failure worth refusing up + /// front: an entry nobody could ever reach. + /// + /// For startup only. Building a client is not cheap, so a caller holding + /// the store behind a lock uses [`Self::signer_for_url`] and + /// [`Self::insert_remote_signer`] instead, and builds any missing client + /// before taking the write lock. + pub fn insert_remote(&mut self, pubkey: BlsPubkey, url: &str) -> Result<()> { + let signer = match self.signer_for_url(url) { + Some(signer) => signer, + None => Arc::new(Web3Signer::new(url)?), + }; + self.insert_remote_signer(pubkey, signer); + Ok(()) + } + + /// The client already serving `url`, if any validator still uses one. + /// Keyed by the URL as the client normalizes it, so `.../` and `...` name + /// one signer rather than two. + pub fn signer_for_url(&self, url: &str) -> Option> { + self.signers + .get(url.trim_end_matches('/')) + .and_then(Weak::upgrade) + } + + /// Every client some validator still uses, by URL. Lets a caller that + /// builds clients outside the store's lock reuse these instead. + pub fn remote_signers(&self) -> HashMap> { + self.signers + .iter() + .filter_map(|(url, signer)| Some((url.clone(), signer.upgrade()?))) + .collect() + } + + /// Add a validator signing through an already-built `signer`, replacing + /// any method the store had for it. A map insert and nothing more, so it + /// is cheap enough to run under the store's write lock. + pub fn insert_remote_signer(&mut self, pubkey: BlsPubkey, signer: Arc) { + // Entries whose client nobody holds any more are dropped here rather + // than on every mutation: they are a URL and a dead pointer, and a + // lookup already treats one as absent. + self.signers.retain(|_, signer| signer.strong_count() > 0); + self.signers + .insert(signer.url().to_string(), Arc::downgrade(&signer)); + self.validators + .insert(pubkey, SigningMethod::Web3Signer { signer }); + } + /// Derive a validator's public key from its raw secret, without adding it /// to the store. /// @@ -187,14 +297,46 @@ impl ValidatorStore { self.validators.contains_key(pubkey) } - pub fn get(&self, pubkey: &BlsPubkey) -> Option<&SigningMethod> { - self.validators.get(pubkey) + /// How `pubkey` signs, as a copy the caller can hold after releasing + /// whatever lock guards this store. `None` for a validator it does not + /// hold. + pub fn signer(&self, pubkey: &BlsPubkey) -> Option { + self.validators.get(pubkey).cloned() + } + + /// The remote signer's URL for `pubkey`, or `None` when the key is local + /// or unknown. + pub fn remote_url(&self, pubkey: &BlsPubkey) -> Option { + match self.validators.get(pubkey)? { + SigningMethod::Web3Signer { signer } => Some(signer.url().to_string()), + SigningMethod::LocalKeystore { .. } => None, + } } pub fn pubkeys(&self) -> Vec { self.validators.keys().copied().collect() } + /// The validators whose key is in this process. + pub fn local_pubkeys(&self) -> Vec { + self.validators + .iter() + .filter(|(_, method)| !method.is_remote()) + .map(|(pubkey, _)| *pubkey) + .collect() + } + + /// The validators a remote signer signs for, each with its signer's URL. + pub fn remote_entries(&self) -> Vec<(BlsPubkey, String)> { + self.validators + .iter() + .filter_map(|(pubkey, method)| match method { + SigningMethod::Web3Signer { signer } => Some((*pubkey, signer.url().to_string())), + SigningMethod::LocalKeystore { .. } => None, + }) + .collect() + } + pub fn len(&self) -> usize { self.validators.len() } @@ -238,7 +380,7 @@ mod tests { use std::path::PathBuf; use super::*; - use crate::keys::definitions::{ValidatorDefinition, ValidatorDefinitions}; + use crate::keys::definitions::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions}; /// The secret from the EIP-2335 test vectors. fn test_secret() -> [u8; 32] { @@ -296,8 +438,10 @@ mod tests { let definitions = ValidatorDefinitions(vec![ValidatorDefinition { enabled: true, voting_public_key: "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07".to_string(), - voting_keystore_path: PathBuf::from("keystore.json"), - voting_keystore_password_path: PathBuf::from("password.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, }]); definitions.save(dir.path()).expect("saves definitions"); @@ -322,13 +466,150 @@ mod tests { ValidatorDefinitions(vec![ValidatorDefinition { enabled: true, voting_public_key: voting_public_key.to_string(), - voting_keystore_path: PathBuf::from("keystore.json"), - voting_keystore_password_path: PathBuf::from("password.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, }]) .save(dir) .expect("saves definitions"); } + #[test] + fn a_remote_definition_loads_without_contacting_its_signer() { + let dir = tempfile::tempdir().expect("temp dir"); + // A URL nothing listens on: loading must not care. + let url = "http://127.0.0.1:9"; + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: VECTOR_PUBKEY.to_string(), + signing: SigningDefinition::Web3Signer { + url: url.to_string(), + }, + }]) + .save(dir.path()) + .expect("saves definitions"); + + let store = ValidatorStore::load(dir.path()).expect("loads"); + let pubkey = parse_pubkey(VECTOR_PUBKEY).expect("pubkey"); + assert_eq!(store.len(), 1); + assert_eq!(store.remote_url(&pubkey).as_deref(), Some(url)); + assert!(store.signer(&pubkey).expect("held").is_remote()); + assert!(store.local_pubkeys().is_empty()); + assert_eq!(store.remote_entries(), vec![(pubkey, url.to_string())]); + } + + #[test] + fn a_remote_definition_with_a_bad_url_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: VECTOR_PUBKEY.to_string(), + signing: SigningDefinition::Web3Signer { + url: "not a url".to_string(), + }, + }]) + .save(dir.path()) + .expect("saves definitions"); + + assert!(ValidatorStore::load(dir.path()).is_err()); + } + + /// A remote entry's `url` is as the operator wrote it until + /// `Web3Signer::new` has judged it, so an error raised before that names + /// the entry by its place in the file and never echoes the URL, which + /// may carry credentials. + #[test] + fn an_unreadable_pubkey_on_a_remote_entry_does_not_echo_its_url() { + let dir = tempfile::tempdir().expect("temp dir"); + let remote = |voting_public_key: &str, url: &str| ValidatorDefinition { + enabled: true, + voting_public_key: voting_public_key.to_string(), + signing: SigningDefinition::Web3Signer { + url: url.to_string(), + }, + }; + ValidatorDefinitions(vec![ + remote(VECTOR_PUBKEY, "http://127.0.0.1:9"), + remote("0xnothex", "https://admin:s3cret@signer:9000"), + ]) + .save(dir.path()) + .expect("saves definitions"); + + let Err(err) = ValidatorStore::load(dir.path()) else { + panic!("an entry with an unreadable pubkey must refuse the load"); + }; + let message = err.to_string(); + assert!(message.contains("entry 2"), "{message}"); + assert!( + !message.contains("s3cret") && !message.contains("admin"), + "the error must not carry the URL's credentials: {message}" + ); + } + + /// How many validators' worth of clients the store can still hand out. + fn live_signers(store: &ValidatorStore) -> usize { + store + .signers + .values() + .filter(|signer| signer.strong_count() > 0) + .count() + } + + #[test] + fn remote_validators_on_one_url_share_a_client_and_release_it_together() { + let mut store = ValidatorStore::new(); + let first = BlsPubkey([1; BLS_PUBKEY_SIZE]); + let second = BlsPubkey([2; BLS_PUBKEY_SIZE]); + store + .insert_remote(first, "http://signer:9000") + .expect("inserts"); + store + .insert_remote(second, "http://signer:9000/") + .expect("inserts"); + assert_eq!(live_signers(&store), 1, "one URL, one client"); + + assert!(store.remove(&first)); + assert_eq!(live_signers(&store), 1, "still in use by the second key"); + assert!(store.remove(&second)); + assert_eq!(live_signers(&store), 0, "nobody uses it any more"); + assert!(store.signer_for_url("http://signer:9000").is_none()); + } + + /// A signature in flight holds the client, and must not keep it alive in + /// the store after it finishes: removal and the request's end can come in + /// either order. + #[test] + fn a_client_held_by_an_in_flight_signature_is_released_when_it_ends() { + let mut store = ValidatorStore::new(); + let pubkey = BlsPubkey([1; BLS_PUBKEY_SIZE]); + store + .insert_remote(pubkey, "http://signer:9000") + .expect("inserts"); + let in_flight = store.signer(&pubkey).expect("held"); + + assert!(store.remove(&pubkey)); + assert_eq!(live_signers(&store), 1, "the request still holds it"); + drop(in_flight); + assert_eq!(live_signers(&store), 0, "released with the request"); + } + + #[test] + fn inserting_a_local_key_over_a_remote_one_replaces_it() { + let mut store = ValidatorStore::new(); + let pubkey = parse_pubkey(VECTOR_PUBKEY).expect("pubkey"); + store + .insert_remote(pubkey, "http://signer:9000") + .expect("inserts"); + let derived = store + .insert_secret("test", &test_secret()) + .expect("inserts"); + assert_eq!(derived, pubkey); + assert_eq!(store.len(), 1); + assert!(store.remote_url(&pubkey).is_none()); + assert_eq!(live_signers(&store), 0); + } + /// A definitions entry declaring a key its keystore does not hold must not /// load. /// @@ -406,6 +687,7 @@ mod tests { #[test] fn an_unknown_pubkey_has_no_signing_method() { let store = ValidatorStore::new(); - assert!(store.get(&BlsPubkey([7; BLS_PUBKEY_SIZE])).is_none()); + assert!(store.signer(&BlsPubkey([7; BLS_PUBKEY_SIZE])).is_none()); + assert!(store.remote_url(&BlsPubkey([7; BLS_PUBKEY_SIZE])).is_none()); } } diff --git a/crates/validator/src/keys/web3signer.rs b/crates/validator/src/keys/web3signer.rs new file mode 100644 index 000000000..7c3ad5efc --- /dev/null +++ b/crates/validator/src/keys/web3signer.rs @@ -0,0 +1,729 @@ +//! Signing through a Web3Signer remote signer. +//! +//! The key never enters this process: each signature is a `POST +//! /api/v1/eth2/sign/{pubkey}` carrying the message, the fork it is signed +//! under and the signing root, and the signer answers with the signature. The +//! message travels in full rather than as its root alone because the signer +//! keeps a slashing-protection record of its own, beside this client's (see +//! [`crate::slashing_protection`]), and it can only refuse a double vote it +//! can read. The root is sent as well so the signer can check the two agree. +//! +//! Request shapes follow Web3Signer's own OpenAPI +//! (`core/src/main/resources/openapi-specs/eth2/signing/schemas.yaml`): +//! quoted integers, `0x`-prefixed hex, and a fork-named `version` on the +//! containers that changed shape across forks. + +use std::time::Duration; + +use ethlambda_types::beacon::containers::altair::{ + ContributionAndProof, SyncAggregatorSelectionData, +}; +use ethlambda_types::beacon::containers::gloas::{ + ExecutionPayloadEnvelope, PayloadAttestationData, +}; +use ethlambda_types::beacon::containers::shared::{ + AttestationData, BeaconBlockHeader, Fork, VoluntaryExit, +}; +use ethlambda_types::beacon::containers::{electra, gloas}; +use ethlambda_types::beacon::primitives::{BLS_SIGNATURE_SIZE, BlsPubkey, BlsSignature, Root}; +use serde::Serialize; +use tokio::sync::Semaphore; + +use crate::beacon_node::dto::{encode_hex, parse_signature}; +use crate::error::{Error, Result}; + +/// How long one signing request may take. The same bound the beacon node +/// client uses; every duty is cut off by its own slot deadline anyway, and a +/// signer slower than this is one whose signatures arrive too late to count. +const REQUEST_TIMEOUT: Duration = Duration::from_secs(8); + +/// Web3Signer's answer when its slashing protection refused the message. +const SLASHING_PROTECTION_REFUSED: u16 = 412; + +/// The most requests one signer is sent at once; the rest of a batch waits +/// its turn. +/// +/// The client speaks HTTP/1.1, so every request in flight is a connection of +/// its own. A duty asks for all its signatures together, and uncapped that is +/// a socket per key: a thousand remote keys at an epoch boundary would open a +/// thousand, past the usual 1024 descriptor limit, at which point `connect` +/// fails and every remote signature with it. Capped, a signer costs this many +/// descriptors however many keys it holds, and its connections are reused +/// from one capful to the next. A healthy signer answers in milliseconds, so +/// a thousand signatures still fit many times over in the signing budget. +pub const MAX_CONCURRENT_REQUESTS: usize = 64; + +/// One remote signer, shared by every validator whose key it holds. +pub struct Web3Signer { + base_url: String, + client: reqwest::Client, + /// One permit per request in flight; see [`MAX_CONCURRENT_REQUESTS`]. + /// Per signer, so one that is slow holds up only its own keys. + in_flight: Semaphore, +} + +impl Web3Signer { + /// Build a client for the signer at `url`, which is the server's base + /// (`https://signer:9000`), not the signing path. + /// + /// Building one sets up a TLS configuration and a connection pool, so it + /// is not free: callers that hold a lock build it before taking the lock + /// (see [`crate::keys::ValidatorStore::insert_remote_signer`]). + pub fn new(url: &str) -> Result { + Self::with_cap(url, MAX_CONCURRENT_REQUESTS) + } + + /// [`Self::new`], sent at most `cap` requests at once. + pub(crate) fn with_cap(url: &str, cap: usize) -> Result { + let base_url = url.trim_end_matches('/').to_string(); + // Every error below names the URL as `redact` renders it, never as + // given: these errors are logged, at startup and on import, and a URL + // refused for its shape may still carry a `user:password@` that the + // credentials check further down never got to reject. + let invalid = |reason: &str| Error::Keystore { + path: redact(&base_url), + reason: reason.to_string(), + }; + let parsed = reqwest::Url::parse(&base_url) + .map_err(|err| invalid(&format!("not a valid remote signer URL: {err}")))?; + if parsed.scheme() != "http" && parsed.scheme() != "https" { + return Err(invalid("a remote signer URL must be http or https")); + } + // The URL is named in every error this client raises about the + // signer, and those are logged. Credentials embedded in it would be + // logged with them, so they are refused rather than redacted: the + // signer's own authentication is TLS, not basic auth. + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(invalid("a remote signer URL must not carry credentials")); + } + // The signing path is appended to the base, so a query or fragment + // would swallow it (`https://signer?token=abc` would request `/`), and + // a token in the query would be logged with every signing error. + if parsed.query().is_some() || parsed.fragment().is_some() { + return Err(invalid( + "a remote signer URL must not carry a query string or fragment", + )); + } + let client = reqwest::Client::builder() + .timeout(REQUEST_TIMEOUT) + .build() + .map_err(|err| invalid(&format!("could not build an HTTP client: {err}")))?; + Ok(Self { + base_url, + client, + in_flight: Semaphore::new(cap), + }) + } + + /// The signer's base URL, with any trailing slash already stripped. + pub fn url(&self) -> &str { + &self.base_url + } + + /// Ask the signer for `pubkey`'s signature over `request`. + /// + /// Every failure is an [`Error::Signing`]: a refusal by the signer's + /// slashing protection, a key the signer does not hold, and a transport + /// failure all cost this one signature and nothing else, and none of them + /// is something retrying within the slot would change. + pub async fn sign( + &self, + pubkey: &BlsPubkey, + request: &SigningRequest<'_>, + ) -> Result { + let pubkey_hex = encode_hex(&pubkey.0); + let url = format!("{}/api/v1/eth2/sign/{pubkey_hex}", self.base_url); + let failed = |reason: String| Error::Signing { + pubkey: *pubkey, + reason, + }; + + // Held until the answer is read. Waiting for one is part of what the + // caller's deadline bounds, and a request dropped at that deadline + // gives its permit back at once. + let _permit = self.in_flight.acquire().await.map_err(|_| { + failed(format!( + "remote signer {} is no longer in use", + self.base_url + )) + })?; + // The round trip alone, from here to the answer or the failure: the + // wait for a permit above is this client's own queue, not the signer. + let _timing = crate::metrics::time_remote_signer_request(); + + let response = self + .client + .post(&url) + .header(reqwest::header::ACCEPT, "application/json") + .json(request) + .send() + .await + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url)))?; + + let status = response.status(); + let body = response + .text() + .await + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url)))?; + + if status.as_u16() == SLASHING_PROTECTION_REFUSED { + return Err(failed(format!( + "remote signer {} refused to sign: its slashing protection rejected the message", + self.base_url + ))); + } + if status == reqwest::StatusCode::NOT_FOUND { + return Err(failed(format!( + "remote signer {} does not hold this key", + self.base_url + ))); + } + if !status.is_success() { + return Err(failed(format!( + "remote signer {} answered {status}: {}", + self.base_url, + truncate(body.trim()) + ))); + } + + parse_signature_body(&body) + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url))) + } +} + +/// The most of a failed response's body that goes into an error, and so into +/// a log line: enough to say why, not a whole error page. +const MAX_LOGGED_BODY: usize = 200; + +fn truncate(body: &str) -> String { + match body.char_indices().nth(MAX_LOGGED_BODY) { + Some((cut, _)) => format!("{}...", &body[..cut]), + None => body.to_string(), + } +} + +/// `url` with anything that could be a credential cut out, for an error +/// message: the userinfo before an `@` in the authority, and any query string +/// or fragment. Works on text rather than a parsed URL, since it is what +/// reports a URL that does not parse. +fn redact(url: &str) -> String { + let (scheme, rest) = match url.split_once("://") { + Some((scheme, rest)) => (Some(scheme), rest), + None => (None, url), + }; + let rest = rest.split(['?', '#']).next().unwrap_or_default(); + let (authority, path) = match rest.find('/') { + Some(cut) => rest.split_at(cut), + None => (rest, ""), + }; + let host = match authority.rsplit_once('@') { + Some((_, host)) => host, + None => authority, + }; + match scheme { + Some(scheme) => format!("{scheme}://{host}{path}"), + None => format!("{host}{path}"), + } +} + +/// The signature out of a `200` body: `{"signature": "0x..."}` when the +/// signer honoured `Accept: application/json`, or the bare hex string an +/// older signer answers with regardless. +fn parse_signature_body(body: &str) -> Result { + #[derive(serde::Deserialize)] + struct SignatureResponse { + signature: String, + } + let hex = match serde_json::from_str::(body) { + Ok(response) => response.signature, + Err(_) => body.trim().trim_matches('"').to_string(), + }; + let signature = parse_signature(&hex).map_err(|err| { + Error::Decode(format!( + "the signature in the response is unreadable: {err}" + )) + })?; + debug_assert_eq!(signature.0.len(), BLS_SIGNATURE_SIZE); + Ok(signature) +} + +/// The fork a message is signed under, as the signer wants it told. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct ForkInfo { + pub fork: Fork, + pub genesis_validators_root: Root, +} + +/// One signing request: what kind of message, under which fork, over which +/// root, and the message itself. +#[derive(Serialize)] +pub struct SigningRequest<'a> { + #[serde(rename = "type")] + kind: &'static str, + pub fork_info: ForkInfo, + #[serde(rename = "signingRoot")] + pub signing_root: Root, + #[serde(flatten)] + pub body: SigningBody<'a>, +} + +impl<'a> SigningRequest<'a> { + pub fn new(fork_info: ForkInfo, signing_root: Root, body: SigningBody<'a>) -> Self { + Self { + kind: body.kind(), + fork_info, + signing_root, + body, + } + } +} + +/// The message half of a request, one field per Web3Signer request type. +/// +/// Untagged on purpose: the `type` discriminator is a sibling of these fields +/// in the request, not a tag around them, so the variant contributes its one +/// field and [`SigningRequest`] writes the `type` beside it. +#[derive(Serialize)] +#[serde(untagged)] +pub enum SigningBody<'a> { + Attestation { + attestation: &'a AttestationData, + }, + RandaoReveal { + randao_reveal: RandaoReveal, + }, + Block { + beacon_block: VersionedHeader, + }, + AggregationSlot { + aggregation_slot: AggregationSlot, + }, + AggregateAndProof { + aggregate_and_proof: Versioned>, + }, + SyncCommitteeMessage { + sync_committee_message: SyncCommitteeMessage, + }, + SyncCommitteeSelectionProof { + sync_aggregator_selection_data: SyncAggregatorSelectionData, + }, + SyncCommitteeContributionAndProof { + contribution_and_proof: &'a ContributionAndProof, + }, + ExecutionPayloadEnvelope { + execution_payload_envelope: Versioned<&'a ExecutionPayloadEnvelope>, + }, + PayloadAttestationMessage { + payload_attestation_message: Versioned<&'a PayloadAttestationData>, + }, + VoluntaryExit { + voluntary_exit: &'a VoluntaryExit, + }, +} + +impl SigningBody<'_> { + /// Web3Signer's `type` for this message. + fn kind(&self) -> &'static str { + match self { + Self::Attestation { .. } => "ATTESTATION", + Self::RandaoReveal { .. } => "RANDAO_REVEAL", + Self::Block { .. } => "BLOCK_V2", + Self::AggregationSlot { .. } => "AGGREGATION_SLOT", + Self::AggregateAndProof { .. } => "AGGREGATE_AND_PROOF_V2", + Self::SyncCommitteeMessage { .. } => "SYNC_COMMITTEE_MESSAGE", + Self::SyncCommitteeSelectionProof { .. } => "SYNC_COMMITTEE_SELECTION_PROOF", + Self::SyncCommitteeContributionAndProof { .. } => { + "SYNC_COMMITTEE_CONTRIBUTION_AND_PROOF" + } + Self::ExecutionPayloadEnvelope { .. } => "EXECUTION_PAYLOAD_ENVELOPE", + Self::PayloadAttestationMessage { .. } => "PAYLOAD_ATTESTATION_MESSAGE", + Self::VoluntaryExit { .. } => "VOLUNTARY_EXIT", + } + } +} + +#[derive(Serialize)] +pub struct RandaoReveal { + #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")] + pub epoch: u64, +} + +#[derive(Serialize)] +pub struct AggregationSlot { + #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")] + pub slot: u64, +} + +#[derive(Serialize)] +pub struct SyncCommitteeMessage { + pub beacon_block_root: Root, + #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")] + pub slot: u64, +} + +/// A block, as the signer wants it: the fork's name and the header alone. +/// The body never travels; its root in the header is what the signature +/// covers. +#[derive(Serialize)] +pub struct VersionedHeader { + pub version: String, + pub block_header: BeaconBlockHeader, +} + +/// A container whose JSON shape the signer selects by fork name. +#[derive(Serialize)] +pub struct Versioned { + pub version: String, + pub data: T, +} + +/// An `AggregateAndProof` of either fork this client publishes, by reference. +#[derive(Clone, Copy, Serialize)] +#[serde(untagged)] +pub enum AggregateAndProofRef<'a> { + Electra(&'a electra::AggregateAndProof), + Gloas(&'a gloas::AggregateAndProof), +} + +impl AggregateAndProofRef<'_> { + /// The slot of the votes being republished, which is the slot the + /// wrapper's signature is domained on. + pub fn slot(&self) -> u64 { + match self { + Self::Electra(message) => message.aggregate.data.slot, + Self::Gloas(message) => message.aggregate.data.slot, + } + } + + pub fn hash_tree_root(&self) -> Root { + use ethlambda_types::beacon::primitives::HashTreeRoot as _; + match self { + Self::Electra(message) => message.hash_tree_root(), + Self::Gloas(message) => message.hash_tree_root(), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use ethlambda_types::beacon::containers::shared::Checkpoint; + use serde_json::json; + + fn fork_info() -> ForkInfo { + ForkInfo { + fork: Fork { + previous_version: [0, 0, 0, 0], + current_version: [1, 0, 0, 0], + epoch: 74_240, + }, + genesis_validators_root: Root::repeat_byte(0xaa), + } + } + + fn attestation_data() -> AttestationData { + AttestationData { + slot: 3200, + index: 2, + beacon_block_root: Root::repeat_byte(1), + source: Checkpoint { + epoch: 99, + root: Root::repeat_byte(2), + }, + target: Checkpoint { + epoch: 100, + root: Root::repeat_byte(3), + }, + } + } + + fn encode(request: &SigningRequest<'_>) -> serde_json::Value { + serde_json::to_value(request).expect("serializes") + } + + /// The exact wire shape, pinned against Web3Signer's `AttestationSigning` + /// schema: `type` beside `fork_info`, `signingRoot` camel-cased, every + /// integer quoted, every hash `0x`-prefixed. + #[test] + fn an_attestation_request_has_web3signers_shape() { + let data = attestation_data(); + let request = SigningRequest::new( + fork_info(), + Root::repeat_byte(9), + SigningBody::Attestation { attestation: &data }, + ); + assert_eq!( + encode(&request), + json!({ + "type": "ATTESTATION", + "fork_info": { + "fork": { + "previous_version": "0x00000000", + "current_version": "0x01000000", + "epoch": "74240" + }, + "genesis_validators_root": format!("0x{}", "aa".repeat(32)) + }, + "signingRoot": format!("0x{}", "09".repeat(32)), + "attestation": { + "slot": "3200", + "index": "2", + "beacon_block_root": format!("0x{}", "01".repeat(32)), + "source": { "epoch": "99", "root": format!("0x{}", "02".repeat(32)) }, + "target": { "epoch": "100", "root": format!("0x{}", "03".repeat(32)) } + } + }) + ); + } + + #[test] + fn every_body_names_its_type_and_its_one_field() { + let data = attestation_data(); + let header = BeaconBlockHeader { + slot: 3200, + proposer_index: 7, + parent_root: Root::repeat_byte(1), + state_root: Root::repeat_byte(2), + body_root: Root::repeat_byte(3), + }; + let exit = VoluntaryExit { + epoch: 100, + validator_index: 7, + }; + let cases: Vec<(SigningBody<'_>, &str, &str)> = vec![ + ( + SigningBody::Attestation { attestation: &data }, + "ATTESTATION", + "attestation", + ), + ( + SigningBody::VoluntaryExit { + voluntary_exit: &exit, + }, + "VOLUNTARY_EXIT", + "voluntary_exit", + ), + ( + SigningBody::RandaoReveal { + randao_reveal: RandaoReveal { epoch: 100 }, + }, + "RANDAO_REVEAL", + "randao_reveal", + ), + ( + SigningBody::Block { + beacon_block: VersionedHeader { + version: "ELECTRA".to_string(), + block_header: header, + }, + }, + "BLOCK_V2", + "beacon_block", + ), + ( + SigningBody::AggregationSlot { + aggregation_slot: AggregationSlot { slot: 3200 }, + }, + "AGGREGATION_SLOT", + "aggregation_slot", + ), + ( + SigningBody::SyncCommitteeMessage { + sync_committee_message: SyncCommitteeMessage { + beacon_block_root: Root::ZERO, + slot: 3200, + }, + }, + "SYNC_COMMITTEE_MESSAGE", + "sync_committee_message", + ), + ( + SigningBody::SyncCommitteeSelectionProof { + sync_aggregator_selection_data: SyncAggregatorSelectionData { + slot: 3200, + subcommittee_index: 1, + }, + }, + "SYNC_COMMITTEE_SELECTION_PROOF", + "sync_aggregator_selection_data", + ), + ]; + for (body, kind, field) in cases { + let encoded = encode(&SigningRequest::new(fork_info(), Root::ZERO, body)); + assert_eq!(encoded["type"], kind); + assert!( + encoded.get(field).is_some(), + "{kind} must carry its message under `{field}`: {encoded}" + ); + assert_eq!( + encoded.as_object().expect("object").len(), + 4, + "{kind}: type, fork_info, signingRoot and the message, nothing else" + ); + } + } + + /// Web3Signer's `VoluntaryExit` schema: both fields quoted integers. + #[test] + fn a_voluntary_exit_request_has_web3signers_shape() { + let exit = VoluntaryExit { + epoch: 269_568, + validator_index: 1234, + }; + let encoded = encode(&SigningRequest::new( + fork_info(), + Root::repeat_byte(0x11), + SigningBody::VoluntaryExit { + voluntary_exit: &exit, + }, + )); + assert_eq!(encoded["type"], "VOLUNTARY_EXIT"); + assert_eq!( + encoded["voluntary_exit"], + serde_json::json!({ "epoch": "269568", "validator_index": "1234" }) + ); + assert_eq!( + encoded["signingRoot"], + format!("0x{}", "11".repeat(32)), + "the root travels, so the signer can refuse one it does not derive itself" + ); + } + + #[test] + fn a_block_request_carries_the_fork_name_and_the_header_only() { + let header = BeaconBlockHeader { + slot: 3200, + proposer_index: 7, + parent_root: Root::repeat_byte(1), + state_root: Root::repeat_byte(2), + body_root: Root::repeat_byte(3), + }; + let request = SigningRequest::new( + fork_info(), + Root::ZERO, + SigningBody::Block { + beacon_block: VersionedHeader { + version: "FULU".to_string(), + block_header: header, + }, + }, + ); + assert_eq!( + encode(&request)["beacon_block"], + json!({ + "version": "FULU", + "block_header": { + "slot": "3200", + "proposer_index": "7", + "parent_root": format!("0x{}", "01".repeat(32)), + "state_root": format!("0x{}", "02".repeat(32)), + "body_root": format!("0x{}", "03".repeat(32)) + } + }) + ); + } + + #[test] + fn a_payload_attestation_request_is_versioned_gloas() { + let data = PayloadAttestationData { + beacon_block_root: Root::repeat_byte(5), + slot: 3200, + payload_present: true, + blob_data_available: false, + }; + let request = SigningRequest::new( + fork_info(), + Root::ZERO, + SigningBody::PayloadAttestationMessage { + payload_attestation_message: Versioned { + version: "GLOAS".to_string(), + data: &data, + }, + }, + ); + assert_eq!( + encode(&request)["payload_attestation_message"], + json!({ + "version": "GLOAS", + "data": { + "beacon_block_root": format!("0x{}", "05".repeat(32)), + "slot": "3200", + "payload_present": true, + "blob_data_available": false + } + }) + ); + } + + #[test] + fn a_failed_response_body_is_cut_short_before_it_is_reported() { + let long = "x".repeat(1_000); + assert_eq!(truncate(&long).len(), MAX_LOGGED_BODY + 3); + assert_eq!(truncate("short"), "short"); + } + + #[test] + fn the_signature_is_read_from_json_or_from_a_bare_string() { + let hex = format!("0x{}", "ab".repeat(BLS_SIGNATURE_SIZE)); + let from_json = + parse_signature_body(&format!("{{\"signature\":\"{hex}\"}}")).expect("json"); + let from_text = parse_signature_body(&format!("{hex}\n")).expect("text"); + assert_eq!(from_json, from_text); + assert_eq!(from_json.0, [0xab; BLS_SIGNATURE_SIZE]); + assert!(parse_signature_body("0x1234").is_err()); + } + + #[test] + fn the_url_must_be_http_or_https() { + assert!(Web3Signer::new("https://signer:9000/").is_ok()); + assert_eq!( + Web3Signer::new("https://signer:9000/") + .expect("valid") + .url(), + "https://signer:9000" + ); + assert!(Web3Signer::new("ftp://signer:9000").is_err()); + let Err(err) = Web3Signer::new("https://user:secret@signer:9000") else { + panic!("a URL carrying credentials must be refused"); + }; + assert!(!err.to_string().contains("secret"), "{err}"); + assert!(Web3Signer::new("signer:9000").is_err()); + assert!(Web3Signer::new("not a url").is_err()); + } + + /// A URL refused for its shape must not put its credentials into the + /// error either: the scheme and parse checks run before the credentials + /// one, and every one of these errors is logged. + #[test] + fn no_refusal_echoes_the_urls_credentials() { + for url in [ + "ftp://admin:s3cret@signer:9000", + "https://admin:s3cret@signer:notaport", + "https://admin:s3cret@signer:9000", + "https://signer:9000?token=s3cret", + "https://signer:9000/#s3cret", + ] { + let Err(err) = Web3Signer::new(url) else { + panic!("{url} must be refused"); + }; + assert!(!err.to_string().contains("s3cret"), "{url}: {err}"); + } + } + + #[test] + fn a_url_with_a_query_or_fragment_is_refused() { + assert!(Web3Signer::new("https://signer:9000?token=abc").is_err()); + assert!(Web3Signer::new("https://signer:9000/#frag").is_err()); + assert!(Web3Signer::new("https://signer:9000/prefix").is_ok()); + } + + #[test] + fn redaction_keeps_the_host_and_path_only() { + assert_eq!( + redact("https://user:pw@signer:9000/base?x=1#y"), + "https://signer:9000/base" + ); + assert_eq!(redact("signer:9000"), "signer:9000"); + assert_eq!(redact("a@b"), "b"); + } +} diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index 1f24ba749..b032b0d42 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -90,6 +90,10 @@ pub struct ValidatorConfig { /// Bind address for the keymanager API. `None` when `--enable-keymanager` /// was not passed, in which case it is never spawned. pub keymanager: Option, + /// `--web3signer-url`: the remote signer a key imported through + /// `POST /eth/v1/remotekeys` without a `url` of its own is assigned to. + /// Keys the definitions file already names a signer for need no default. + pub web3signer_url: Option, /// Register keys that have no slashing-protection record as never having /// signed, instead of refusing to start. See /// [`slashing_protection::SlashingProtection::open`]. @@ -134,6 +138,12 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { directory" ); } + // Checked for shape at startup rather than at the first import that + // leans on it, which could be days later. + if let Some(url) = &config.web3signer_url { + keys::Web3Signer::new(url)?; + } + let store = ValidatorStore::load(&config.validators_dir)?; if store.is_empty() { warn!("No validators loaded; the client will idle"); @@ -183,7 +193,9 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { // After the beacon node names the chain, which the record must belong to, // and before any service that signs exists. Every loaded key must already - // have a record, unless the operator said otherwise. + // have a record, unless the operator said otherwise. That includes a key + // a remote signer holds: the signer keeps its own record, and this one is + // kept as well, so a key is covered whichever of the two it meets first. let keys = store.read().await.pubkeys(); let protection = slashing_protection::SlashingProtection::open_locked( record_lock, @@ -199,10 +211,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { ); let protection = Arc::new(std::sync::Mutex::new(protection)); - let context = Arc::new(SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }); + let context = Arc::new(SigningContext::new(spec, genesis.genesis_validators_root)); let mut duties = DutiesService::new(beacon_node.clone(), Vec::new()); let attestation = @@ -225,6 +234,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { validators_dir: config.validators_dir.clone(), secrets_dir: config.secrets_dir.clone(), definitions_lock: Arc::new(Mutex::new(())), + default_remote_signer: config.web3signer_url.clone(), beacon_node: beacon_node.clone(), signing: context.clone(), clock: clock.clone(), @@ -271,6 +281,10 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { let epoch = clock.epoch_of(slot); + // Whether the refresh below moved the attester schedule, which owes + // the beacon node its subnet subscriptions once the slot's block is + // out. + let mut resubscribe = false; if last_epoch != Some(epoch) { // Snapshot the pubkeys and let the guard drop right here, before // `refresh_epoch`'s awaits (a sync check, two duties fetches, a @@ -290,11 +304,13 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { &pubkeys, epoch, &settings, - &store, &context, ); match refreshed.await { - Ok(()) => last_epoch = Some(epoch), + Ok(moved) => { + last_epoch = Some(epoch); + resubscribe = moved; + } // A beacon node that is down, syncing or answering badly will // likely answer next epoch, so keep the schedule already held // and try again. Anything else will not fix itself, and looping @@ -326,6 +342,34 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { propose(&proposal, &clock, slot, &duty, &store).await; } + // The subscriptions the refresh above left owing, sent from a task of + // their own and only now, after the block. + // + // Each one needs a selection proof, and for a remote key that is a + // round trip to its signer: awaited here, or inside the refresh as + // they once were, a slow signer would stand between the slot boundary + // and this slot's block, a block from a local key included, for as + // long as the signing budget lets it. Started after the proposal + // rather than beside it so the proofs do not queue ahead of the + // block's own signatures at the same signer either (requests to one + // signer are capped, see `Web3Signer`). + // + // Nothing waits on them. The schedule covers this epoch and the + // next, so the duties that are near were subscribed an epoch ago, and + // the proofs are remembered for the aggregation whenever they land. + if resubscribe { + let (beacon_node, store, context) = + (beacon_node.clone(), store.clone(), context.clone()); + let owed = duties.all(); + tokio::spawn(async move { + let subscribed = subscriptions::subscribe(&beacon_node, &owed, &store, &context); + if let Err(err) = subscribed.await { + warn!(%epoch, %err, "Failed to subscribe to attestation subnets"); + metrics::set_beacon_node_available(false); + } + }); + } + // The rest of the way to the first of the slot's remaining duties: // the attester offset, or the sync committee offset when the network // puts it earlier. Zero if the refresh or the proposal above already @@ -676,22 +720,26 @@ async fn propose( } } -/// Resolve any newly activated validators, refresh the duty schedule, and -/// re-send subnet subscriptions if it moved. +/// Resolve any newly activated validators and refresh the duty schedule. +/// +/// Returns whether the attester schedule moved. When it did, the beacon node +/// is owed this client's subnet subscriptions, which the caller sends once +/// the slot's block is out rather than having them sent from here: each +/// needs a selection proof, a round trip per remote key, and this runs ahead +/// of the proposal. See the duty loop in [`run`]. async fn refresh_epoch( beacon_node: &Arc, duties: &mut DutiesService, pubkeys: &[BlsPubkey], epoch: u64, settings: &ProposerSettings, - store: &RwLock, context: &SigningContext, -) -> Result<()> { +) -> Result { // Nothing to resolve, nothing to schedule, nothing to register. Returning // here also keeps an empty pubkey list away from `validator_indices`, whose // endpoint reads one as "every validator on the chain". if pubkeys.is_empty() { - return Ok(()); + return Ok(false); } if beacon_node.is_optimistic_or_syncing().await? { @@ -758,11 +806,8 @@ async fn refresh_epoch( refresh_ptc(duties, epoch, context).await; refresh_sync(beacon_node, duties, epoch, context).await; metrics::set_duties_held(duties.all().len() as u64); - if changed { - subscriptions::subscribe(beacon_node, &duties.all(), store, context).await?; - } metrics::set_beacon_node_available(true); - Ok(()) + Ok(changed) } /// Fetch the payload timeliness committee schedule for this epoch and the next, @@ -883,10 +928,10 @@ mod tests { } fn context() -> SigningContext { - SigningContext { - config: ethlambda_types::beacon::config::Config::mainnet(), - genesis_validators_root: Root::ZERO, - } + SigningContext::new( + ethlambda_types::beacon::config::Config::mainnet(), + Root::ZERO, + ) } fn empty_store() -> RwLock { @@ -904,17 +949,9 @@ mod tests { async fn refresh_with(node: Arc, settings: &ProposerSettings) { let mut duties = DutiesService::new(node.clone(), Vec::new()); let keys = [pubkey(1), pubkey(2)]; - refresh_epoch( - &node, - &mut duties, - &keys, - 3, - settings, - &empty_store(), - &context(), - ) - .await - .expect("refreshes"); + refresh_epoch(&node, &mut duties, &keys, 3, settings, &context()) + .await + .expect("refreshes"); } /// One preparation per resolved validator, every epoch. The node keeps a @@ -946,6 +983,31 @@ mod tests { assert_eq!(node.preparations().len(), 4); } + /// The refresh says when the attester schedule moved, which is what owes + /// the node its subnet subscriptions, and sends none itself: they wait + /// for the slot's block, since each costs a remote key a round trip to + /// its signer. + #[tokio::test] + async fn a_refresh_reports_a_moved_schedule_and_leaves_the_subscriptions_to_the_caller() { + let node = Arc::new(node()); + let mut duties = DutiesService::new(node.clone(), Vec::new()); + let keys = [pubkey(1), pubkey(2)]; + let settings = settings(None); + let context = context(); + + let moved = refresh_epoch(&node, &mut duties, &keys, 3, &settings, &context) + .await + .expect("refreshes"); + assert!(moved, "a schedule fetched for the first time is a new one"); + + let moved = refresh_epoch(&node, &mut duties, &keys, 3, &settings, &context) + .await + .expect("refreshes"); + assert!(!moved, "the same dependent roots are the same schedule"); + + assert!(node.subscriptions().is_empty()); + } + #[tokio::test] async fn no_configured_address_sends_no_registration() { let node = Arc::new(node()); @@ -1008,7 +1070,6 @@ mod tests { &keys, 3, &settings(Some(H160([0xab; 20]))), - &empty_store(), &context(), ) .await @@ -1034,7 +1095,6 @@ mod tests { &[], 3, &settings(Some(H160([0xab; 20]))), - &empty_store(), &context(), ) .await @@ -1080,11 +1140,11 @@ mod tests { // ---- gloas ---------------------------------------------------------- fn gloas_context(epoch: u64) -> SigningContext { - SigningContext { - config: ethlambda_types::beacon::config::Config::mainnet() + SigningContext::new( + ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, epoch), - genesis_validators_root: Root::ZERO, - } + Root::ZERO, + ) } fn ptc_duty(index: u64, slot: u64) -> crate::beacon_node::dto::PtcDutyDto { @@ -1116,7 +1176,6 @@ mod tests { &keys, 3, &settings(None), - &empty_store(), &gloas_context(4), ) .await @@ -1131,17 +1190,9 @@ mod tests { // A chain with no gloas scheduled is never asked at all. let node = Arc::new(node_without_ptc()); let mut duties = DutiesService::new(node.clone(), Vec::new()); - refresh_epoch( - &node, - &mut duties, - &keys, - 3, - &settings(None), - &empty_store(), - &context(), - ) - .await - .expect("refreshes"); + refresh_epoch(&node, &mut duties, &keys, 3, &settings(None), &context()) + .await + .expect("refreshes"); assert_eq!(node.ptc_call_count(), 0); } @@ -1161,7 +1212,6 @@ mod tests { &keys, 3, &settings(None), - &empty_store(), &gloas_context(0), ) .await @@ -1180,10 +1230,7 @@ mod tests { let mut config = ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, 0); config.slot_duration_ms = 1_000; - let context = Arc::new(SigningContext { - config: config.clone(), - genesis_validators_root: Root::ZERO, - }); + let context = Arc::new(SigningContext::new(config.clone(), Root::ZERO)); let now_secs = SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .expect("after the epoch") @@ -1237,11 +1284,11 @@ mod tests { } fn altair_context() -> SigningContext { - SigningContext { - config: ethlambda_types::beacon::config::Config::mainnet() + SigningContext::new( + ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Altair, 0), - genesis_validators_root: Root::ZERO, - } + Root::ZERO, + ) } fn sync_duty(index: u64, seats: Vec) -> crate::beacon_node::dto::SyncDutyDto { @@ -1272,7 +1319,6 @@ mod tests { &keys, 3, &settings(None), - &empty_store(), &altair_context(), ) .await @@ -1313,7 +1359,6 @@ mod tests { &keys, 3, &settings(None), - &empty_store(), &altair_context(), ) .await @@ -1329,10 +1374,7 @@ mod tests { let mut config = ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Altair, 0); config.slot_duration_ms = 1_000; - let context = Arc::new(SigningContext { - config: config.clone(), - genesis_validators_root: Root::ZERO, - }); + let context = Arc::new(SigningContext::new(config.clone(), Root::ZERO)); let now_secs = SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .expect("after the epoch") @@ -1388,10 +1430,7 @@ mod tests { use crate::beacon_node::mock::MockBeaconNode; let config = ethlambda_types::beacon::config::Config::mainnet(); - let context = Arc::new(SigningContext { - config: config.clone(), - genesis_validators_root: Root::ZERO, - }); + let context = Arc::new(SigningContext::new(config.clone(), Root::ZERO)); let clock = SlotClock::from_config(0, &config); let node = Arc::new(MockBeaconNode::new()); let store = empty_store(); diff --git a/crates/validator/src/metrics.rs b/crates/validator/src/metrics.rs index ad52aa95b..622d5bc0b 100644 --- a/crates/validator/src/metrics.rs +++ b/crates/validator/src/metrics.rs @@ -237,12 +237,35 @@ static BEACON_NODE_AVAILABLE: LazyLock = LazyLock::new(|| { .unwrap() }); +/// How long a duty waited for its batch of signatures. +/// +/// For local keys that is the signing itself, well under a millisecond a key. +/// With remote keys it is the batch's slowest answer, round trip and +/// verification included, and at most the signing budget +/// (`signing::SIGNING_BUDGET`): a sample at the top bucket means a signer +/// that did not answer in time, not slow arithmetic. What one signer request +/// takes is `remote_signer_request_duration_seconds`. static SIGNING_DURATION_SECONDS: LazyLock = LazyLock::new(|| { register_histogram!( "ethlambda_validator_signing_duration_seconds", - "Time spent signing one slot's batch of attestations, read lock included", + "Time a duty waited for its batch of signatures, remote signer round trips included", vec![ - 0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5 + 0.0005, 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.5 + ] + ) + .unwrap() +}); + +/// One request to a remote signer, from sending it to its answer or failure. +/// +/// The top bucket is the client's own request timeout; a request the signing +/// budget cut off before that is recorded at the moment it was dropped. +static REMOTE_SIGNER_REQUEST_DURATION_SECONDS: LazyLock = LazyLock::new(|| { + register_histogram!( + "ethlambda_validator_remote_signer_request_duration_seconds", + "Time one signing request to a remote signer took, whatever its outcome", + vec![ + 0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1.0, 2.5, 8.0 ] ) .unwrap() @@ -358,6 +381,7 @@ pub fn init() { LazyLock::force(&FEE_RECIPIENT_MISMATCHES_TOTAL); LazyLock::force(&BEACON_NODE_AVAILABLE); LazyLock::force(&SIGNING_DURATION_SECONDS); + LazyLock::force(&REMOTE_SIGNER_REQUEST_DURATION_SECONDS); LazyLock::force(&PUBLICATION_DELAY_SECONDS); LazyLock::force(&SLASHING_PROTECTION_WRITE_SECONDS); LazyLock::force(&SLASHING_PROTECTION_WRITE_FAILURES_TOTAL); @@ -472,15 +496,21 @@ pub fn set_beacon_node_available(available: bool) { BEACON_NODE_AVAILABLE.set(i64::from(available)); } -/// Time one slot's signing loop. The returned guard records to -/// `ethlambda_validator_signing_duration_seconds` when dropped; hold it only -/// across the synchronous signing loop, never across an await, or the sample -/// stops meaning "how long the signing took" and starts meaning "how long -/// the signing took plus whatever else shared the guard's scope". +/// Time one duty's batch of signatures. The returned guard records to +/// `ethlambda_validator_signing_duration_seconds` when dropped; hold it +/// across the `signing::sign_all` call and nothing else, or the sample stops +/// meaning "how long the duty waited for its signatures" and starts meaning +/// that plus whatever else shared the guard's scope. pub fn time_signing() -> TimingGuard { TimingGuard::new(&SIGNING_DURATION_SECONDS) } +/// Time one request to a remote signer. The returned guard records to +/// `ethlambda_validator_remote_signer_request_duration_seconds` when dropped. +pub fn time_remote_signer_request() -> TimingGuard { + TimingGuard::new(&REMOTE_SIGNER_REQUEST_DURATION_SECONDS) +} + pub fn time_slashing_protection_write() -> TimingGuard { TimingGuard::new(&SLASHING_PROTECTION_WRITE_SECONDS) } diff --git a/crates/validator/src/payload_attestation.rs b/crates/validator/src/payload_attestation.rs index 0870c038a..fbfa39d67 100644 --- a/crates/validator/src/payload_attestation.rs +++ b/crates/validator/src/payload_attestation.rs @@ -43,7 +43,7 @@ use crate::beacon_node::BeaconNodeApi; use crate::beacon_node::dto::{PtcDutyDto, parse_pubkey}; use crate::error::Result; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; /// How many slots of dedup history are kept. Far more than the one slot a duty /// is ever owed for; the margin only absorbs a clock that steps back a little. @@ -117,38 +117,43 @@ impl PayloadAttestationService { ))); } - let mut messages = Vec::with_capacity(pending.len()); - { - let _timing = crate::metrics::time_signing(); - let store = store.read().await; - for duty in pending { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - - // Recorded before signing, so a signature that exists is one - // the record already knows about. - if !self.record(duty.validator_index, slot) { + // Recorded one by one before anything is signed, so a signature that + // exists is one the record already knows about; then signed all at + // once, each within the signing budget. + let mut to_sign = Vec::with_capacity(pending.len()); + for duty in pending { + let pubkey = match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => pubkey, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); continue; } - - match self - .context - .sign_payload_attestation(&store, &pubkey, &data) - { - Ok(signature) => messages.push(PayloadAttestationMessage { - validator_index: duty.validator_index, - data, - signature, - }), - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign payload attestation"); - crate::metrics::inc_signing_failures(); - } + }; + if self.record(duty.validator_index, slot) { + to_sign.push((duty.validator_index, pubkey)); + } + } + let signatures = { + let _timing = crate::metrics::time_signing(); + sign_all(to_sign.iter().map(|(_, pubkey)| { + ( + *pubkey, + self.context.sign_payload_attestation(store, pubkey, &data), + ) + })) + .await + }; + let mut messages = Vec::with_capacity(to_sign.len()); + for ((validator_index, _), signature) in to_sign.into_iter().zip(signatures) { + match signature { + Ok(signature) => messages.push(PayloadAttestationMessage { + validator_index, + data, + signature, + }), + Err(err) => { + error!(%slot, validator = validator_index, %err, "Failed to sign payload attestation"); + crate::metrics::inc_signing_failures(); } } } @@ -225,10 +230,10 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), + Root::ZERO, + )) } fn store() -> (RwLock, BlsPubkey) { diff --git a/crates/validator/src/proposal.rs b/crates/validator/src/proposal.rs index c16319f07..8b84d5dcc 100644 --- a/crates/validator/src/proposal.rs +++ b/crates/validator/src/proposal.rs @@ -55,7 +55,9 @@ use std::sync::Arc; use ethlambda_types::beacon::constants::BUILDER_INDEX_SELF_BUILD; use ethlambda_types::beacon::fork::ForkName; -use ethlambda_types::beacon::primitives::{BlsPubkey, ExecutionAddress, Root, Slot}; +use ethlambda_types::beacon::primitives::{ + BlsPubkey, ExecutionAddress, HashTreeRoot as _, Root, Slot, +}; use ethlambda_types::beacon::signing::compute_epoch_at_slot; use tokio::sync::RwLock; use tracing::{error, info, warn}; @@ -172,15 +174,7 @@ impl ProposalService { .await; refuse_unless_safe(slot, duty.validator_index, "consult", checked)?; - // Scoped away from every await, the way the attestation path's read - // guard is, and for the reason spelled out there: a write-preferring - // lock lets one queued keymanager writer block every reader behind it, - // so a guard held across the block production round trip below would - // put the whole duty path behind it. - let randao_reveal = { - let store = store.read().await; - self.context.sign_randao(&store, &pubkey, epoch)? - }; + let randao_reveal = self.context.sign_randao(store, &pubkey, epoch).await?; // Both read once, before the block is asked for, so the check below // compares against the address in force when the request went out. @@ -240,26 +234,29 @@ impl ProposalService { "Block produced; signing" ); - // The root is taken from the decoded container, never from anything - // reassembled here, and it is taken once: the same value is what the + // The header is taken from the decoded container, never from anything + // reassembled here, and it is taken once: its root is what the // recorded slot belongs to and what the signature covers. - let block_root = produced.block_root(); + // + // The block's root is the header's, so it is taken from the header: + // that rehashes five fields, where `produced.block_root()` would hash + // the whole body, transactions included, a second time inside the + // proposal's budget. + let header = produced.header(); + let block_root = header.hash_tree_root(); // The check that matters, and the one that records, durably, before // the signature exists. Everything after this point may be abandoned // without risk, because the record already counts this slot as - // proposed. Off the executor, and before the store's read lock below, - // so the disk flush never holds up a keymanager writer. + // proposed. Off the executor, and with no guard on the store held, so + // the disk flush never holds up a keymanager writer. let recorded = crate::slashing_protection::blocking(&self.protection, move |protection| { protection.check_and_record_block(&pubkey, slot) }) .await; refuse_unless_safe(slot, duty.validator_index, "record", recorded)?; - let signature = { - let store = store.read().await; - self.context.sign_block(&store, &pubkey, block_root, slot)? - }; + let signature = self.context.sign_block(store, &pubkey, &header).await?; // Taken out before the block is signed into its body: the envelope is // published on its own, after the block, and the bare gloas block body @@ -350,11 +347,10 @@ impl ProposalService { ))); } - let signature = { - let store = store.read().await; - self.context - .sign_execution_payload_envelope(&store, pubkey, &envelope, slot)? - }; + let signature = self + .context + .sign_execution_payload_envelope(store, pubkey, &envelope, slot) + .await?; let body = match payload { Some(payload) => payload.into_signed_contents_ssz(signature), None => signed_envelope_ssz(envelope, signature), @@ -443,7 +439,6 @@ mod tests { use crate::beacon_node::mock::MockBeaconNode; use crate::slashing_protection::SlashingProtection; use ethlambda_types::beacon::config::Config; - use ethlambda_types::beacon::primitives::HashTreeRoot as _; use ethlambda_types::beacon::primitives::{BlsPubkey, Bytes32, H160, Root}; use libssz::SszDecode as _; @@ -455,10 +450,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } /// A store holding one key, and the pubkey it resolves to. @@ -860,7 +852,7 @@ mod gloas_tests { use ethlambda_types::beacon::config::Config; use ethlambda_types::beacon::containers::gloas; use ethlambda_types::beacon::preset::SLOTS_PER_EPOCH; - use ethlambda_types::beacon::primitives::{BlsPubkey, HashTreeRoot as _}; + use ethlambda_types::beacon::primitives::BlsPubkey; use libssz::SszDecode as _; const GLOAS_EPOCH: u64 = 10; @@ -874,10 +866,10 @@ mod gloas_tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), + Root::ZERO, + )) } fn store() -> (RwLock, BlsPubkey) { diff --git a/crates/validator/src/signing.rs b/crates/validator/src/signing.rs index 2c673521c..5f23cb298 100644 --- a/crates/validator/src/signing.rs +++ b/crates/validator/src/signing.rs @@ -19,11 +19,13 @@ use ethlambda_types::beacon::containers::altair::{ use ethlambda_types::beacon::containers::gloas::{ ExecutionPayloadEnvelope, PayloadAttestationData, }; -use ethlambda_types::beacon::containers::shared::{AttestationData, VoluntaryExit}; +use ethlambda_types::beacon::containers::shared::{ + AttestationData, BeaconBlockHeader, Fork, VoluntaryExit, +}; use ethlambda_types::beacon::fork::ForkName; use ethlambda_types::beacon::primitives::{ BLS_SIGNATURE_SIZE, BlsPubkey, BlsSignature, Domain, DomainType, Epoch, HashTreeRoot as _, - Root, Slot, + Root, Slot, Version, }; // `AttestationData`, `Checkpoint`, `Fork` and `SigningData` all live in // `containers::shared`, verified against `containers/shared.rs:158,120,97,220`. @@ -31,14 +33,43 @@ use ethlambda_types::beacon::signing::{ compute_domain, compute_epoch_at_slot, compute_signing_root, }; +use std::collections::HashMap; +use std::num::NonZeroUsize; +use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; +use std::time::Duration; + +use ethlambda_types::beacon::preset::SLOTS_PER_EPOCH; +use tokio::sync::{RwLock, Semaphore}; + use crate::error::{Error, Result}; +use crate::keys::web3signer::{ + AggregationSlot, ForkInfo, RandaoReveal, SigningBody, SigningRequest, SyncCommitteeMessage, + Versioned, VersionedHeader, +}; use crate::keys::{SigningMethod, ValidatorStore}; +pub use crate::keys::web3signer::AggregateAndProofRef; + /// The ciphersuite the consensus layer pins BLS signatures to. It must match /// `ethlambda_state_transition::beacon::bls`'s own constant, or nothing this /// client signs will ever verify. const DST: &[u8] = b"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"; +/// Whether `signature` is `pubkey`'s signature over `root`. A pubkey or a +/// signature that is not a point on the curve verifies nothing. +fn verifies(pubkey: &BlsPubkey, root: Root, signature: &BlsSignature) -> bool { + use blst::min_pk::{PublicKey, Signature}; + + let (Ok(public_key), Ok(signature)) = ( + PublicKey::from_bytes(&pubkey.0), + Signature::from_bytes(&signature.0), + ) else { + return false; + }; + signature.verify(true, root.as_slice(), DST, &[], &public_key, true) + == blst::BLST_ERROR::BLST_SUCCESS +} + /// The chain-wide half of a signing domain, fixed for the life of the process. /// /// The fork version deliberately does not live here. It is a function of the @@ -52,9 +83,96 @@ pub struct SigningContext { /// signature from one chain never verifies on another running the same /// fork schedule. pub genesis_validators_root: Root, + /// Selection proofs already produced; see [`Self::sign_selection_proof`]. + selection_proofs: Mutex>, + /// One permit per remote signature being verified; see [`Self::verified`]. + verifications: Arc, +} + +/// How long a selection proof is remembered past its slot: the duty +/// schedule holds the current epoch and the next, so a proof computed for a +/// subscription at the start of an epoch is still wanted by the aggregation +/// up to two epochs later. +const SELECTION_PROOF_RETENTION_SLOTS: Slot = 3 * SLOTS_PER_EPOCH; + +/// How long one duty's batch of signatures may take, from when it is asked +/// for. +/// +/// Well inside every duty's own budget (the rest of its slot, at least a +/// third of one), so a signer that hangs costs the signatures it owes and +/// nothing else: the batch is published with whatever arrived in time, local +/// keys and other signers' keys included. A healthy Web3Signer answers in +/// milliseconds; one slower than this is one whose signatures would land too +/// late to be worth much anyway. +pub const SIGNING_BUDGET: Duration = Duration::from_secs(2); + +/// Ask for every signature in `requests` at once, and give each until +/// [`SIGNING_BUDGET`] from now. +/// +/// One result per request, in order. A request still outstanding at the +/// deadline is dropped and reported as an [`Error::Signing`] for its key, so +/// one unresponsive signer delays the batch by the budget at most and never +/// takes the signatures that did arrive down with it. +pub async fn sign_all(requests: impl IntoIterator) -> Vec> +where + F: Future>, +{ + let deadline = tokio::time::Instant::now() + SIGNING_BUDGET; + futures::future::join_all(requests.into_iter().map(|(pubkey, request)| async move { + tokio::time::timeout_at(deadline, request) + .await + .unwrap_or_else(|_| { + Err(Error::Signing { + pubkey, + reason: format!( + "no signature within the {}s signing budget", + SIGNING_BUDGET.as_secs() + ), + }) + }) + })) + .await } impl SigningContext { + pub fn new(config: Config, genesis_validators_root: Root) -> Self { + // One verification per core: each is a pairing, pure CPU, so more at + // once would only take turns on the same cores. + let cores = std::thread::available_parallelism().map_or(1, NonZeroUsize::get); + Self { + config, + genesis_validators_root, + selection_proofs: Mutex::new(HashMap::new()), + verifications: Arc::new(Semaphore::new(cores)), + } + } + + /// Whether `signature` is `pubkey`'s signature over `root`, checked on + /// the blocking pool rather than on the task that asked. + /// + /// A verification is a pairing, about a millisecond of CPU with no await + /// in it. Run inline, a batch's answers would be verified one after + /// another on the duty task, where [`sign_all`]'s deadline cannot cut in: + /// a thousand remote keys would hold the duty loop for a second or more + /// that the signing budget does not bound, ahead of whatever the slot + /// owes next. Here they run a core each, and waiting for a core is an + /// await the budget covers. + async fn verified(&self, pubkey: BlsPubkey, root: Root, signature: BlsSignature) -> bool { + let Ok(permit) = self.verifications.clone().acquire_owned().await else { + return false; + }; + // The permit goes into the blocking task and is released when the + // pairing ends, not when this future is dropped: a batch cut off at + // its deadline leaves its verifications running, and they must still + // count against the bound. + let verification = tokio::task::spawn_blocking(move || { + let _permit = permit; + verifies(&pubkey, root, &signature) + }); + // A verification that did not run to its end vouches for nothing. + verification.await.unwrap_or(false) + } + /// The domain for `domain_type` as of `epoch`. pub fn domain(&self, domain_type: DomainType, epoch: Epoch) -> Domain { let fork = self.config.fork_at_epoch(epoch); @@ -195,6 +313,34 @@ impl SigningContext { compute_signing_root(message.hash_tree_root(), domain) } + /// The fork a message domained at `epoch` is signed under, told the way a + /// remote signer wants it: the fork active at that epoch as `current`, + /// the one before it as `previous`, and the epoch the current one took + /// effect. Phase0 has no predecessor and names itself twice, as the + /// genesis state's own `Fork` does. + pub fn fork_info(&self, epoch: Epoch) -> ForkInfo { + let fork = self.config.fork_at_epoch(epoch); + let current_version = self.config.fork_version(fork); + let previous_version = fork + .previous() + .map(|previous| self.config.fork_version(previous)) + .unwrap_or(current_version); + ForkInfo { + fork: Fork { + previous_version, + current_version, + epoch: self.config.fork_epoch(fork), + }, + genesis_validators_root: self.genesis_validators_root, + } + } + + /// Web3Signer's name for the fork at `epoch`: the specification's + /// lowercase name in capitals (`ELECTRA`, `GLOAS`). + fn fork_label(&self, epoch: Epoch) -> String { + self.config.fork_at_epoch(epoch).as_str().to_uppercase() + } + /// The root a voluntary exit's signature is computed over. /// /// Every other signature here follows the fork at its message's own epoch. @@ -219,54 +365,161 @@ impl SigningContext { /// pre-deneb epoch under that epoch's version, which no node on a chain /// past deneb accepts. pub fn voluntary_exit_signing_root(&self, exit: &VoluntaryExit, current_epoch: Epoch) -> Root { + let domain = compute_domain( + DOMAIN_VOLUNTARY_EXIT, + self.voluntary_exit_fork_version(exit, current_epoch), + self.genesis_validators_root, + ); + compute_signing_root(exit.hash_tree_root(), domain) + } + + /// The fork version `exit`'s domain is built from; the rule is spelled + /// out on [`Self::voluntary_exit_signing_root`]. + fn voluntary_exit_fork_version(&self, exit: &VoluntaryExit, current_epoch: Epoch) -> Version { let fork = self.config.fork_at_epoch(current_epoch.max(exit.epoch)); - let fork_version = if fork >= ForkName::Deneb { + if fork >= ForkName::Deneb { self.config.capella_fork_version } else if exit.epoch < self.config.fork_epoch(fork) { self.config.fork_version(fork.previous().unwrap_or(fork)) } else { self.config.fork_version(fork) - }; - let domain = compute_domain( - DOMAIN_VOLUNTARY_EXIT, - fork_version, - self.genesis_validators_root, - ); - compute_signing_root(exit.hash_tree_root(), domain) + } + } + + /// The fork a remote signer is told `exit` is signed under: the one + /// version its domain is built from, as both `previous` and `current`. + /// + /// Not [`Self::fork_info`] at the exit's epoch, which names the fork the + /// schedule has there and so, from deneb on, a version the exit is not + /// signed under. A signer derives the root itself from what it is told + /// and refuses a request whose `signingRoot` disagrees, and Web3Signer + /// has derived an exit's two ways: older releases pick the rule by the + /// exit's epoch, current ones by this fork's epoch, and either then + /// answers the capella version from its own configuration (deneb on) or + /// picks `previous` or `current` by comparing the two epochs (before). + /// With both versions the same, every one of those paths lands on the + /// version this client signs under, whichever epoch the exit names. + /// Lighthouse tells its signer the same thing for the same reason. + fn voluntary_exit_fork_info(&self, exit: &VoluntaryExit, current_epoch: Epoch) -> ForkInfo { + let version = self.voluntary_exit_fork_version(exit, current_epoch); + ForkInfo { + fork: Fork { + previous_version: version, + current_version: version, + epoch: exit.epoch, + }, + genesis_validators_root: self.genesis_validators_root, + } } /// Sign an already-computed signing root on behalf of `pubkey`. /// /// Every public signing method funnels through here, so there is one place - /// a signature is actually produced and one place the remote-signer - /// variant will need to be added. Deliberately private: a caller that can - /// hand in an arbitrary root can make this client sign anything at all, - /// and the domain separation that keeps an attestation from being read as - /// a block lives in the callers above. - fn sign_root( + /// a signature is actually produced. Deliberately private: a caller that + /// can hand in an arbitrary root can make this client sign anything at + /// all, and the domain separation that keeps an attestation from being + /// read as a block lives in the callers above. + /// + /// `epoch` is the epoch the root's domain was taken at, and `body` the + /// message itself; a local key needs neither, a remote signer needs both. + async fn sign( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, root: Root, + epoch: Epoch, + body: SigningBody<'_>, ) -> Result { - let method = store.get(pubkey).ok_or(Error::UnknownValidator(*pubkey))?; + let fork_info = self.fork_info(epoch); + self.sign_under(store, pubkey, root, epoch, fork_info, body) + .await + } + + /// [`Self::sign`], with the fork a remote signer is told given outright + /// rather than read off the schedule at `epoch`. Only a voluntary exit + /// needs that: every other message is domained by the fork at its own + /// epoch. + /// + /// The store's read guard is held for the lookup alone and released before + /// anything is signed. `tokio::sync::RwLock` is write-preferring, so a + /// guard held across the remote signer's round trip would let one queued + /// keymanager writer stall every other duty behind it for that long. + async fn sign_under( + &self, + store: &RwLock, + pubkey: &BlsPubkey, + root: Root, + epoch: Epoch, + fork_info: ForkInfo, + body: SigningBody<'_>, + ) -> Result { + let method = store + .read() + .await + .signer(pubkey) + .ok_or(Error::UnknownValidator(*pubkey))?; match method { SigningMethod::LocalKeystore { secret_key } => { let signature = secret_key.sign(root.as_slice(), DST, &[]); let bytes: [u8; BLS_SIGNATURE_SIZE] = signature.to_bytes(); Ok(BlsSignature(bytes)) } + SigningMethod::Web3Signer { signer } => { + // Web3Signer's `BLOCK_V2` takes the full block before + // bellatrix and the header only from bellatrix on, and this + // client only ever sends the header. Refused here rather than + // sent in a shape the signer answers with a 400; the proposal + // path refuses those forks long before this anyway. + if matches!(body, SigningBody::Block { .. }) + && self.config.fork_at_epoch(epoch) < ForkName::Bellatrix + { + return Err(Error::Signing { + pubkey: *pubkey, + reason: "a remote signer is only asked for blocks from bellatrix on" + .to_string(), + }); + } + let request = SigningRequest::new(fork_info, root, body); + let signature = signer.sign(pubkey, &request).await?; + // Checked before it leaves this function. The signer is + // another process on the far side of a network: one holding a + // different key under this pubkey, computing another root + // for a fork it does not know, or answering through a proxy + // that rewrote the body would otherwise have this client + // publish a signature no peer accepts, and decide + // `is_aggregator` on bytes that are not the selection proof. + // A local key needs no such check: its signature is computed + // here, over this root. + if !self.verified(*pubkey, root, signature).await { + return Err(Error::Signing { + pubkey: *pubkey, + reason: format!( + "remote signer {} answered with a signature that does not verify \ + for this key and message", + signer.url() + ), + }); + } + Ok(signature) + } } } /// Sign `data` on behalf of `pubkey`. - pub fn sign_attestation( + pub async fn sign_attestation( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, data: &AttestationData, ) -> Result { - self.sign_root(store, pubkey, self.attestation_signing_root(data)) + self.sign( + store, + pubkey, + self.attestation_signing_root(data), + data.target.epoch, + SigningBody::Attestation { attestation: data }, + ) + .await } /// Sign the RANDAO reveal for `epoch` on behalf of `pubkey`. @@ -275,13 +528,22 @@ impl SigningContext { /// it commits to an epoch, not to a chain position, and producing one for /// an epoch a validator is not proposing in reveals nothing and risks /// nothing. It is therefore not guarded, and does not need to be. - pub fn sign_randao( + pub async fn sign_randao( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, epoch: Epoch, ) -> Result { - self.sign_root(store, pubkey, self.randao_signing_root(epoch)) + self.sign( + store, + pubkey, + self.randao_signing_root(epoch), + epoch, + SigningBody::RandaoReveal { + randao_reveal: RandaoReveal { epoch }, + }, + ) + .await } /// Sign the selection proof for `slot` on behalf of `pubkey`. @@ -290,17 +552,58 @@ impl SigningContext { /// to a chain position, so producing one twice is producing the same bytes /// twice: BLS signatures are deterministic, which is the property the /// selection rule rests on. - pub fn sign_selection_proof( + /// + /// That determinism is also why the answer is remembered. The same proof + /// is wanted once when the epoch's subscriptions are sent and again by the + /// aggregation tick, and for a remote key each ask is a round trip; a + /// remembered proof is the very signature the signer would return. Only + /// a proof that was produced is remembered, so a failure is asked again. + /// + /// The store is asked before the memory is. A proof outlives its key + /// there, so answering from it first would hand a key the keymanager + /// deleted mid-epoch its selection: the aggregation would go on to fetch + /// an aggregate it then cannot sign. A key this client no longer holds + /// is [`Error::UnknownValidator`] here, as it is on every other path. + pub async fn sign_selection_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, slot: Slot, ) -> Result { - self.sign_root(store, pubkey, self.selection_proof_signing_root(slot)) + if !store.read().await.contains(pubkey) { + return Err(Error::UnknownValidator(*pubkey)); + } + let remembered = self.selection_proofs().get(&(*pubkey, slot)).copied(); + if let Some(proof) = remembered { + return Ok(proof); + } + let proof = self + .sign( + store, + pubkey, + self.selection_proof_signing_root(slot), + compute_epoch_at_slot(slot), + SigningBody::AggregationSlot { + aggregation_slot: AggregationSlot { slot }, + }, + ) + .await?; + let mut proofs = self.selection_proofs(); + proofs.retain(|(_, held), _| held + SELECTION_PROOF_RETENTION_SLOTS >= slot); + proofs.insert((*pubkey, slot), proof); + Ok(proof) + } + + /// The remembered selection proofs. A poisoned map is still a map of + /// valid proofs, since an entry is inserted whole or not at all. + fn selection_proofs(&self) -> MutexGuard<'_, HashMap<(BlsPubkey, Slot), BlsSignature>> { + self.selection_proofs + .lock() + .unwrap_or_else(PoisonError::into_inner) } - /// Sign the aggregate whose `AggregateAndProof` root is `root`, for `slot`, - /// on behalf of `pubkey`. + /// Sign `message`, an aggregator's `AggregateAndProof`, on behalf of + /// `pubkey`. /// /// Not slashable either, which is worth stating because it is the only /// signature here that covers an attestation and is not. The slashing @@ -308,18 +611,34 @@ impl SigningContext { /// republishing other validators' votes with a wrapper saying who /// collected them, and publishing two different aggregates for one slot is /// wasteful rather than punishable. - pub fn sign_aggregate_and_proof( + /// + /// Takes the whole wrapper rather than its root: a remote signer wants to + /// read what it signs, and the wrapper's own fork decides its JSON shape. + pub async fn sign_aggregate_and_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, - root: Root, - slot: Slot, + message: AggregateAndProofRef<'_>, ) -> Result { - self.sign_root( + let slot = message.slot(); + let epoch = compute_epoch_at_slot(slot); + let version = match message { + AggregateAndProofRef::Gloas(_) => ForkName::Gloas.as_str().to_uppercase(), + AggregateAndProofRef::Electra(_) => self.fork_label(epoch), + }; + self.sign( store, pubkey, - self.aggregate_and_proof_signing_root(root, slot), + self.aggregate_and_proof_signing_root(message.hash_tree_root(), slot), + epoch, + SigningBody::AggregateAndProof { + aggregate_and_proof: Versioned { + version, + data: message, + }, + }, ) + .await } /// Sign the envelope revealing the payload of the block proposed for `slot` @@ -330,14 +649,26 @@ impl SigningContext { /// condition to an envelope: signing two for one slot is at worst a /// payload the network ignores one of. The block it belongs to is the /// guarded signature. - pub fn sign_execution_payload_envelope( + pub async fn sign_execution_payload_envelope( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, envelope: &ExecutionPayloadEnvelope, slot: Slot, ) -> Result { - self.sign_root(store, pubkey, self.envelope_signing_root(envelope, slot)) + self.sign( + store, + pubkey, + self.envelope_signing_root(envelope, slot), + compute_epoch_at_slot(slot), + SigningBody::ExecutionPayloadEnvelope { + execution_payload_envelope: Versioned { + version: ForkName::Gloas.as_str().to_uppercase(), + data: envelope, + }, + }, + ) + .await } /// Sign a payload timeliness committee vote on behalf of `pubkey`. @@ -345,13 +676,25 @@ impl SigningContext { /// Not slashable by the specification, and deduplicated per validator and /// slot by [`crate::payload_attestation`] only to avoid publishing the same /// vote twice. - pub fn sign_payload_attestation( + pub async fn sign_payload_attestation( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, data: &PayloadAttestationData, ) -> Result { - self.sign_root(store, pubkey, self.payload_attestation_signing_root(data)) + self.sign( + store, + pubkey, + self.payload_attestation_signing_root(data), + compute_epoch_at_slot(data.slot), + SigningBody::PayloadAttestationMessage { + payload_attestation_message: Versioned { + version: ForkName::Gloas.as_str().to_uppercase(), + data, + }, + }, + ) + .await } /// Sign a sync committee message for `slot` over `beacon_block_root`. @@ -359,49 +702,70 @@ impl SigningContext { /// Not slashable, and deduplicated per validator and slot by /// [`crate::sync_committee`] only to avoid publishing the same message /// twice. - pub fn sign_sync_committee_message( + pub async fn sign_sync_committee_message( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, slot: Slot, beacon_block_root: Root, ) -> Result { - self.sign_root( + self.sign( store, pubkey, self.sync_committee_message_signing_root(slot, beacon_block_root), + compute_epoch_at_slot(slot), + SigningBody::SyncCommitteeMessage { + sync_committee_message: SyncCommitteeMessage { + beacon_block_root, + slot, + }, + }, ) + .await } /// Sign the sync committee selection proof for `slot` and one /// subcommittee. Deterministic, so not guarded, for the reason /// [`Self::sign_selection_proof`] is not. - pub fn sign_sync_selection_proof( + pub async fn sign_sync_selection_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, slot: Slot, subcommittee_index: u64, ) -> Result { - self.sign_root( + self.sign( store, pubkey, self.sync_selection_proof_signing_root(slot, subcommittee_index), + compute_epoch_at_slot(slot), + SigningBody::SyncCommitteeSelectionProof { + sync_aggregator_selection_data: SyncAggregatorSelectionData { + slot, + subcommittee_index, + }, + }, ) + .await } /// Sign a `ContributionAndProof` on behalf of its aggregator `pubkey`. - pub fn sign_contribution_and_proof( + pub async fn sign_contribution_and_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, message: &ContributionAndProof, ) -> Result { - self.sign_root( + self.sign( store, pubkey, self.contribution_and_proof_signing_root(message), + compute_epoch_at_slot(message.contribution.slot), + SigningBody::SyncCommitteeContributionAndProof { + contribution_and_proof: message, + }, ) + .await } /// Sign a voluntary exit on behalf of `pubkey`. @@ -414,20 +778,35 @@ impl SigningContext { /// signed message to its caller, which is what then decides to publish. /// /// `current_epoch` is the chain's, by the caller's clock; see - /// [`Self::voluntary_exit_signing_root`] for why the domain needs it. - pub fn sign_voluntary_exit( + /// [`Self::voluntary_exit_signing_root`] for why the domain needs it, and + /// [`Self::voluntary_exit_fork_info`] for what a remote signer is told. + pub async fn sign_voluntary_exit( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, exit: &VoluntaryExit, current_epoch: Epoch, ) -> Result { - let root = self.voluntary_exit_signing_root(exit, current_epoch); - self.sign_root(store, pubkey, root) + self.sign_under( + store, + pubkey, + self.voluntary_exit_signing_root(exit, current_epoch), + exit.epoch, + self.voluntary_exit_fork_info(exit, current_epoch), + SigningBody::VoluntaryExit { + voluntary_exit: exit, + }, + ) + .await } - /// Sign the block whose root is `block_root`, proposed for `slot`, on - /// behalf of `pubkey`. + /// Sign the block whose header is `header` on behalf of `pubkey`. + /// + /// The root signed is the header's, which is the block's: a header is the + /// block with its body replaced by that body's root, and the two + /// merkleize to the same value. The header rather than the root travels + /// because a remote signer reads it to keep its slashing-protection + /// record. /// /// This one *is* slashable, and is the most expensive signature this /// client can get wrong: two distinct blocks for one slot need no second @@ -435,14 +814,26 @@ impl SigningContext { /// evidence. Callers must have it allowed and recorded by /// [`crate::slashing_protection::SlashingProtection::check_and_record_block`] /// first. - pub fn sign_block( + pub async fn sign_block( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, - block_root: Root, - slot: Slot, + header: &BeaconBlockHeader, ) -> Result { - self.sign_root(store, pubkey, self.block_signing_root(block_root, slot)) + let epoch = compute_epoch_at_slot(header.slot); + self.sign( + store, + pubkey, + self.block_signing_root(header.hash_tree_root(), header.slot), + epoch, + SigningBody::Block { + beacon_block: VersionedHeader { + version: self.fork_label(epoch), + block_header: header.clone(), + }, + }, + ) + .await } } @@ -460,10 +851,7 @@ mod tests { } fn context() -> SigningContext { - SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - } + SigningContext::new(Config::mainnet(), Root::ZERO) } fn attestation_data(target_epoch: Epoch) -> AttestationData { @@ -482,17 +870,17 @@ mod tests { } } - #[test] - fn a_signature_verifies_under_the_signing_root() { + #[tokio::test] + async fn a_signature_verifies_under_the_signing_root() { use blst::min_pk::{PublicKey, Signature}; - let mut store = ValidatorStore::new(); - let pubkey = store.insert_secret("test", &secret()).expect("inserts"); + let (store, pubkey) = store_with_key(); let context = context(); let data = attestation_data(100); let signature = context .sign_attestation(&store, &pubkey, &data) + .await .expect("signs"); let root = context.attestation_signing_root(&data); @@ -504,6 +892,50 @@ mod tests { ); } + /// One request that never answers costs its own signature, after the + /// budget, and nothing else: the others come back as they were. + #[tokio::test(start_paused = true)] + async fn a_signature_past_the_budget_does_not_hold_back_the_rest() { + use futures::FutureExt as _; + + let started = tokio::time::Instant::now(); + let answered = BlsPubkey([1; 48]); + let hung = BlsPubkey([2; 48]); + let results = sign_all([ + (answered, async { Ok(7u8) }.boxed()), + (hung, std::future::pending().boxed()), + ]) + .await; + + assert_eq!(started.elapsed(), SIGNING_BUDGET); + assert!(matches!(results[0], Ok(7))); + assert!( + matches!(&results[1], Err(Error::Signing { pubkey, .. }) if *pubkey == hung), + "the late one is a signing failure for its own key" + ); + } + + /// A remembered proof is for a validator this client serves. Once the + /// keymanager has deleted the key its proofs answer nothing, so the + /// aggregation stops at the selection rather than fetching an aggregate + /// it then cannot sign. + #[tokio::test] + async fn a_remembered_selection_proof_is_not_handed_out_for_a_deleted_key() { + let (store, pubkey) = store_with_key(); + let context = context(); + context + .sign_selection_proof(&store, &pubkey, 3200) + .await + .expect("signs"); + + assert!(store.write().await.remove(&pubkey)); + let err = context + .sign_selection_proof(&store, &pubkey, 3200) + .await + .expect_err("the key is gone"); + assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); + } + #[test] fn the_domain_changes_with_the_epoch() { let context = context(); @@ -545,11 +977,12 @@ mod tests { assert_eq!(context.attestation_signing_root(&data), from_target); } - #[test] - fn signing_for_an_unknown_validator_is_an_error() { - let store = ValidatorStore::new(); + #[tokio::test] + async fn signing_for_an_unknown_validator_is_an_error() { + let store = RwLock::new(ValidatorStore::new()); let err = context() .sign_attestation(&store, &BlsPubkey::default(), &attestation_data(1)) + .await .expect_err("must fail"); assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); } @@ -566,18 +999,31 @@ mod tests { sig.verify(true, root.as_slice(), DST, &[], &pk, true) == blst::BLST_ERROR::BLST_SUCCESS } - fn store_with_key() -> (ValidatorStore, BlsPubkey) { + fn store_with_key() -> (RwLock, BlsPubkey) { let mut store = ValidatorStore::new(); let pubkey = store.insert_secret("test", &secret()).expect("inserts"); - (store, pubkey) + (RwLock::new(store), pubkey) } - #[test] - fn a_randao_reveal_verifies_under_the_randao_signing_root() { + fn header(slot: Slot) -> BeaconBlockHeader { + BeaconBlockHeader { + slot, + proposer_index: 7, + parent_root: Root::repeat_byte(1), + state_root: Root::repeat_byte(2), + body_root: Root::repeat_byte(3), + } + } + + #[tokio::test] + async fn a_randao_reveal_verifies_under_the_randao_signing_root() { let (store, pubkey) = store_with_key(); let context = context(); - let signature = context.sign_randao(&store, &pubkey, 100).expect("signs"); + let signature = context + .sign_randao(&store, &pubkey, 100) + .await + .expect("signs"); assert!(verify( &pubkey, &signature, @@ -585,19 +1031,20 @@ mod tests { )); } - #[test] - fn a_block_signature_verifies_under_the_block_signing_root() { + #[tokio::test] + async fn a_block_signature_verifies_under_the_block_signing_root() { let (store, pubkey) = store_with_key(); let context = context(); - let block_root = Root::repeat_byte(7); + let header = header(3200); let signature = context - .sign_block(&store, &pubkey, block_root, 3200) + .sign_block(&store, &pubkey, &header) + .await .expect("signs"); assert!(verify( &pubkey, &signature, - context.block_signing_root(block_root, 3200) + context.block_signing_root(header.hash_tree_root(), 3200) )); } @@ -700,22 +1147,24 @@ mod tests { ); } - #[test] - fn signing_a_block_for_an_unknown_validator_is_an_error() { - let store = ValidatorStore::new(); + #[tokio::test] + async fn signing_a_block_for_an_unknown_validator_is_an_error() { + let store = RwLock::new(ValidatorStore::new()); let err = context() - .sign_block(&store, &BlsPubkey::default(), Root::ZERO, 1) + .sign_block(&store, &BlsPubkey::default(), &header(1)) + .await .expect_err("must fail"); assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); } - #[test] - fn a_selection_proof_verifies_under_its_own_root() { + #[tokio::test] + async fn a_selection_proof_verifies_under_its_own_root() { let (store, pubkey) = store_with_key(); let context = context(); let signature = context .sign_selection_proof(&store, &pubkey, 3200) + .await .expect("signs"); assert!(verify( &pubkey, @@ -724,30 +1173,43 @@ mod tests { )); } - #[test] - fn an_aggregate_signature_verifies_under_its_own_root() { + #[tokio::test] + async fn an_aggregate_signature_verifies_under_its_own_root() { + use ethlambda_types::beacon::containers::electra; + let (store, pubkey) = store_with_key(); let context = context(); - let root = Root::repeat_byte(4); + let message = electra::AggregateAndProof { + aggregator_index: 9, + aggregate: electra::Attestation { + aggregation_bits: Default::default(), + data: attestation_data(100), + signature: BlsSignature([2; 96]), + committee_bits: Default::default(), + }, + selection_proof: BlsSignature([3; 96]), + }; let signature = context - .sign_aggregate_and_proof(&store, &pubkey, root, 3200) + .sign_aggregate_and_proof(&store, &pubkey, AggregateAndProofRef::Electra(&message)) + .await .expect("signs"); assert!(verify( &pubkey, &signature, - context.aggregate_and_proof_signing_root(root, 3200) + context.aggregate_and_proof_signing_root(message.hash_tree_root(), 3200) )); } - #[test] - fn a_sync_committee_message_verifies_under_its_own_root() { + #[tokio::test] + async fn a_sync_committee_message_verifies_under_its_own_root() { let (store, pubkey) = store_with_key(); let context = context(); let root = Root::repeat_byte(5); let signature = context .sign_sync_committee_message(&store, &pubkey, 3200, root) + .await .expect("signs"); assert!(verify( &pubkey, @@ -772,13 +1234,14 @@ mod tests { ); } - #[test] - fn a_sync_selection_proof_verifies_and_names_the_subcommittee() { + #[tokio::test] + async fn a_sync_selection_proof_verifies_and_names_the_subcommittee() { let (store, pubkey) = store_with_key(); let context = context(); let signature = context .sign_sync_selection_proof(&store, &pubkey, 3200, 2) + .await .expect("signs"); assert!(verify( &pubkey, @@ -797,8 +1260,8 @@ mod tests { ); } - #[test] - fn a_contribution_and_proof_verifies_under_its_own_root() { + #[tokio::test] + async fn a_contribution_and_proof_verifies_under_its_own_root() { use ethlambda_types::beacon::containers::altair::SyncCommitteeContribution; let (store, pubkey) = store_with_key(); @@ -817,6 +1280,7 @@ mod tests { let signature = context .sign_contribution_and_proof(&store, &pubkey, &message) + .await .expect("signs"); assert!(verify( &pubkey, @@ -835,8 +1299,8 @@ mod tests { compute_signing_root(exit.hash_tree_root(), domain) } - #[test] - fn a_voluntary_exit_verifies_under_its_own_root() { + #[tokio::test] + async fn a_voluntary_exit_verifies_under_its_own_root() { let (store, pubkey) = store_with_key(); let context = context(); let exit = VoluntaryExit { @@ -846,6 +1310,7 @@ mod tests { let signature = context .sign_voluntary_exit(&store, &pubkey, &exit, exit.epoch) + .await .expect("signs"); assert!(verify( &pubkey, @@ -917,6 +1382,74 @@ mod tests { ); } + /// A remote signer derives an exit's root itself, from the fork it is + /// told, and refuses a request whose `signingRoot` is another. So what it + /// derives has to be the root this client signs, for every pairing of + /// the exit's epoch and the chain's. + /// + /// `derived` is Teku's rule, which Web3Signer signs through: the capella + /// version from deneb on, `get_domain` over the fork it was told before. + /// Older releases pick between the two by the exit's epoch and current + /// ones by the told fork's epoch; the fork told names the exit's epoch, + /// so here the two readings are one. + #[test] + fn a_remote_signer_derives_the_exits_root_from_the_fork_it_is_told() { + let context = context(); + let config = &context.config; + let derived = |exit: &VoluntaryExit, told: &ForkInfo| { + let version = if config.fork_at_epoch(told.fork.epoch) >= ForkName::Deneb { + config.capella_fork_version + } else if exit.epoch < told.fork.epoch { + told.fork.previous_version + } else { + told.fork.current_version + }; + exit_root_under(&context, exit, version) + }; + + let past_deneb = config.electra_fork_epoch + 1_000; + let in_bellatrix = config.bellatrix_fork_epoch + 5; + for (exit_epoch, current_epoch) in [ + // On a chain past deneb: the current epoch, epoch 0, and the last + // epoch before deneb. + (past_deneb, past_deneb), + (0, past_deneb), + (config.deneb_fork_epoch - 1, past_deneb), + // Pre-signed for a deneb epoch while the chain is still before it. + (config.deneb_fork_epoch, config.altair_fork_epoch), + // On a chain before deneb: this fork, one fork back, two back. + (in_bellatrix, in_bellatrix), + (config.altair_fork_epoch + 1, in_bellatrix), + (config.altair_fork_epoch - 1, in_bellatrix), + ] { + let exit = VoluntaryExit { + epoch: exit_epoch, + validator_index: 1, + }; + let told = context.voluntary_exit_fork_info(&exit, current_epoch); + assert_eq!(told.fork.epoch, exit.epoch); + assert_eq!( + derived(&exit, &told), + context.voluntary_exit_signing_root(&exit, current_epoch), + "exit epoch {exit_epoch} on a chain at epoch {current_epoch}" + ); + } + + // What the schedule says at the exit's epoch is not it: from deneb on + // that is a version no exit is signed under. + let exit = VoluntaryExit { + epoch: past_deneb, + validator_index: 1, + }; + assert_ne!( + context.fork_info(exit.epoch).fork.current_version, + context + .voluntary_exit_fork_info(&exit, past_deneb) + .fork + .current_version + ); + } + /// Before deneb the ordinary rule held: the version of the fork at the /// exit's epoch, for an exit processed in that fork. #[test] @@ -965,12 +1498,453 @@ mod tests { ); } - #[test] - fn signing_a_randao_reveal_for_an_unknown_validator_is_an_error() { - let store = ValidatorStore::new(); + #[tokio::test] + async fn signing_a_randao_reveal_for_an_unknown_validator_is_an_error() { + let store = RwLock::new(ValidatorStore::new()); let err = context() .sign_randao(&store, &BlsPubkey::default(), 1) + .await .expect_err("must fail"); assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); } + + /// `fork_info` must describe the fork at the epoch the domain was taken + /// at, since the remote signer recomputes the domain from it. Mainnet's + /// altair boundary pins all three fields on either side. + #[test] + fn fork_info_names_the_fork_at_the_epoch_and_its_predecessor() { + let context = context(); + let altair = context.config.altair_fork_epoch; + + let before = context.fork_info(altair - 1); + assert_eq!( + before.fork.current_version, + context.config.genesis_fork_version + ); + assert_eq!( + before.fork.previous_version, before.fork.current_version, + "phase0 has no predecessor and names itself twice" + ); + assert_eq!(before.fork.epoch, 0); + + let after = context.fork_info(altair); + assert_eq!( + after.fork.current_version, + context.config.altair_fork_version + ); + assert_eq!( + after.fork.previous_version, + context.config.genesis_fork_version + ); + assert_eq!(after.fork.epoch, altair); + assert_eq!( + after.genesis_validators_root, + context.genesis_validators_root + ); + } + + /// A remote signer, stood in for by a local HTTP server: the request it + /// receives is the one Web3Signer documents, and the signature it returns + /// is what the caller gets back. + mod remote { + use super::*; + use axum::Router; + use axum::extract::{Path, State}; + use axum::http::StatusCode; + use axum::routing::post; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{Arc, Mutex}; + + /// What the fake signer does: records every request and answers with + /// a signature, or with a fixed status. + #[derive(Clone)] + struct Fake { + requests: Arc>>, + answer: Arc>, + /// How many requests are being answered right now, and the most + /// that ever were at once. + in_flight: Arc, + peak: Arc, + } + + #[derive(Clone, Copy)] + enum Answer { + /// The test key's signature over the request's `signingRoot`, as + /// a signer holding that key would give. + Signature, + /// The same, after a pause long enough for requests to overlap. + SlowSignature, + /// A well-formed signature over the request's `signingRoot`, by a + /// key other than the one asked for. + AnotherKeysSignature, + /// The test key's signature, over a root other than the request's. + AnotherRootsSignature, + /// 96 bytes that are not a signature at all. + Garbage, + Status(u16), + } + + /// `secret`'s signature over `root`, as hex. + fn signature_hex(secret: &[u8; 32], root: &[u8]) -> String { + let secret_key = blst::min_pk::SecretKey::from_bytes(secret).expect("a valid scalar"); + hex::encode(secret_key.sign(root, DST, &[]).to_bytes()) + } + + /// A second key, for a signer that answers with the wrong one. + fn other_secret() -> [u8; 32] { + let mut bytes = secret(); + bytes[31] ^= 0x01; + bytes + } + + async fn handle( + State(fake): State, + Path(pubkey): Path, + body: String, + ) -> (StatusCode, String) { + let json: serde_json::Value = serde_json::from_str(&body).expect("json body"); + let root = hex::decode( + json["signingRoot"] + .as_str() + .expect("a signing root") + .trim_start_matches("0x"), + ) + .expect("hex"); + fake.requests.lock().expect("lock").push((pubkey, json)); + let signed = |signature: String| { + ( + StatusCode::OK, + format!("{{\"signature\":\"0x{signature}\"}}"), + ) + }; + let answer = *fake.answer.lock().expect("lock"); + if matches!(answer, Answer::SlowSignature) { + let now = fake.in_flight.fetch_add(1, Ordering::SeqCst) + 1; + fake.peak.fetch_max(now, Ordering::SeqCst); + tokio::time::sleep(Duration::from_millis(50)).await; + fake.in_flight.fetch_sub(1, Ordering::SeqCst); + } + match answer { + Answer::Signature | Answer::SlowSignature => { + signed(signature_hex(&secret(), &root)) + } + Answer::AnotherKeysSignature => signed(signature_hex(&other_secret(), &root)), + Answer::AnotherRootsSignature => signed(signature_hex(&secret(), &[0x11; 32])), + Answer::Garbage => signed("ab".repeat(96)), + Answer::Status(code) => ( + StatusCode::from_u16(code).expect("status"), + "refused".to_string(), + ), + } + } + + /// Start the fake on an ephemeral port and return it with its URL. + async fn serve(answer: Answer) -> (Fake, String) { + let fake = Fake { + requests: Arc::new(Mutex::new(Vec::new())), + answer: Arc::new(Mutex::new(answer)), + in_flight: Arc::new(AtomicUsize::new(0)), + peak: Arc::new(AtomicUsize::new(0)), + }; + let app = Router::new() + .route("/api/v1/eth2/sign/{pubkey}", post(handle)) + .with_state(fake.clone()); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("binds"); + let address = listener.local_addr().expect("address"); + tokio::spawn(async move { + axum::serve(listener, app).await.expect("serves"); + }); + (fake, format!("http://{address}")) + } + + /// A store whose one key, the test key's, is held by the signer at + /// `url` and not by this process. + fn remote_store(url: &str) -> (RwLock, BlsPubkey) { + let pubkey = ValidatorStore::derive_pubkey(&secret()).expect("a valid key"); + let mut store = ValidatorStore::new(); + store.insert_remote(pubkey, url).expect("inserts"); + (RwLock::new(store), pubkey) + } + + #[tokio::test] + async fn an_attestation_is_signed_by_the_remote_signer() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let data = attestation_data(context.config.altair_fork_epoch + 1); + + let signature = context + .sign_attestation(&store, &pubkey, &data) + .await + .expect("signs"); + assert!( + verify(&pubkey, &signature, context.attestation_signing_root(&data)), + "the signer's signature is the one returned" + ); + + let requests = fake.requests.lock().expect("lock"); + assert_eq!(requests.len(), 1); + let (path_pubkey, body) = &requests[0]; + assert_eq!(path_pubkey, &format!("0x{}", hex::encode(pubkey.0))); + assert_eq!(body["type"], "ATTESTATION"); + assert_eq!( + body["signingRoot"], + serde_json::to_value(context.attestation_signing_root(&data)).expect("root") + ); + assert_eq!( + body["attestation"]["target"]["epoch"], + data.target.epoch.to_string() + ); + assert_eq!( + body["fork_info"], + serde_json::to_value(context.fork_info(data.target.epoch)).expect("fork info"), + "the fork is the target epoch's, where the domain came from" + ); + } + + #[tokio::test] + async fn a_block_request_names_the_fork_and_carries_the_header() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let slot = context.config.electra_fork_epoch * preset::SLOTS_PER_EPOCH; + let header = header(slot); + + context + .sign_block(&store, &pubkey, &header) + .await + .expect("signs"); + + let requests = fake.requests.lock().expect("lock"); + let body = &requests[0].1; + assert_eq!(body["type"], "BLOCK_V2"); + assert_eq!(body["beacon_block"]["version"], "ELECTRA"); + assert_eq!( + body["beacon_block"]["block_header"]["slot"], + slot.to_string() + ); + assert_eq!( + body["signingRoot"], + serde_json::to_value(context.block_signing_root(header.hash_tree_root(), slot)) + .expect("root") + ); + } + + /// An exit for a key in a remote signer: the message travels in + /// full, under the root this client derived, and the fork told is + /// the capella version twice over (EIP-7044) rather than the fork + /// the schedule has at the exit's epoch. + #[tokio::test] + async fn a_voluntary_exit_is_signed_by_the_remote_signer() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let now = context.config.electra_fork_epoch + 1_000; + // Naming an epoch from before deneb, on a chain well past it. + let exit = VoluntaryExit { + epoch: context.config.altair_fork_epoch, + validator_index: 42, + }; + + let signature = context + .sign_voluntary_exit(&store, &pubkey, &exit, now) + .await + .expect("signs"); + let root = context.voluntary_exit_signing_root(&exit, now); + assert!(verify(&pubkey, &signature, root)); + + let requests = fake.requests.lock().expect("lock"); + assert_eq!(requests.len(), 1); + let body = &requests[0].1; + assert_eq!(body["type"], "VOLUNTARY_EXIT"); + assert_eq!(body["voluntary_exit"]["epoch"], exit.epoch.to_string()); + assert_eq!(body["voluntary_exit"]["validator_index"], "42"); + assert_eq!( + body["signingRoot"], + serde_json::to_value(root).expect("root") + ); + let capella = serde_json::to_value(Fork { + previous_version: context.config.capella_fork_version, + current_version: context.config.capella_fork_version, + epoch: exit.epoch, + }) + .expect("fork"); + assert_eq!(body["fork_info"]["fork"], capella); + } + + /// Before bellatrix the signer wants the full block, which this + /// client never sends, so the request is refused rather than made. + #[tokio::test] + async fn a_pre_bellatrix_block_is_not_sent_to_the_signer() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let slot = context.config.altair_fork_epoch * preset::SLOTS_PER_EPOCH; + + let err = context + .sign_block(&store, &pubkey, &header(slot)) + .await + .expect_err("refused"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(fake.requests.lock().expect("lock").is_empty()); + } + + /// What the signer answers is checked before it is used: a signature + /// by another key, over another root, or that is no signature at all + /// is a signing failure for that key, never something to publish. + #[tokio::test] + async fn a_signature_that_does_not_verify_is_a_signing_error() { + for answer in [ + Answer::AnotherKeysSignature, + Answer::AnotherRootsSignature, + Answer::Garbage, + ] { + let (fake, url) = serve(answer).await; + let (store, pubkey) = remote_store(&url); + + let err = context() + .sign_attestation(&store, &pubkey, &attestation_data(100)) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(err.to_string().contains("does not verify"), "{err}"); + assert!(!err.is_retryable()); + assert_eq!(fake.requests.lock().expect("lock").len(), 1, "asked once"); + } + } + + /// The signer is asked for a selection proof once: the same key and + /// slot again is answered from memory, another slot is another ask. + #[tokio::test] + async fn a_selection_proof_is_remembered_once_produced() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + + let first = context + .sign_selection_proof(&store, &pubkey, 3200) + .await + .expect("signs"); + let again = context + .sign_selection_proof(&store, &pubkey, 3200) + .await + .expect("remembered"); + assert_eq!(first, again); + assert_eq!(fake.requests.lock().expect("lock").len(), 1, "asked once"); + + context + .sign_selection_proof(&store, &pubkey, 3201) + .await + .expect("signs"); + assert_eq!( + fake.requests.lock().expect("lock").len(), + 2, + "another slot is another proof" + ); + } + + /// A batch larger than the signer's cap is sent a capful at a time, + /// and every signature in it still arrives. Without the cap each + /// request in flight is a socket of its own, and a thousand keys at + /// an epoch boundary are a thousand sockets. + #[tokio::test] + async fn a_signer_is_never_sent_more_than_its_cap_at_once() { + use crate::keys::Web3Signer; + + const CAP: usize = 4; + const ASKED: usize = 4 * CAP; + + let (fake, url) = serve(Answer::SlowSignature).await; + let pubkey = ValidatorStore::derive_pubkey(&secret()).expect("a valid key"); + let signer = Arc::new(Web3Signer::with_cap(&url, CAP).expect("a usable url")); + let mut store = ValidatorStore::new(); + store.insert_remote_signer(pubkey, signer); + let store = RwLock::new(store); + let context = context(); + + let proofs = sign_all((0..ASKED as Slot).map(|slot| { + ( + pubkey, + context.sign_selection_proof(&store, &pubkey, 3200 + slot), + ) + })) + .await; + + assert!(proofs.iter().all(Result::is_ok), "every proof arrives"); + assert_eq!(fake.requests.lock().expect("lock").len(), ASKED); + assert_eq!( + fake.peak.load(Ordering::SeqCst), + CAP, + "the batch fills the cap and never passes it" + ); + } + + /// A selection proof that does not verify is not remembered: the + /// next ask goes to the signer again rather than being answered from + /// bytes that were never the proof. + #[tokio::test] + async fn an_unverified_selection_proof_is_not_remembered() { + let (fake, url) = serve(Answer::AnotherKeysSignature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + + context + .sign_selection_proof(&store, &pubkey, 100) + .await + .expect_err("must fail"); + *fake.answer.lock().expect("lock") = Answer::Signature; + let proof = context + .sign_selection_proof(&store, &pubkey, 100) + .await + .expect("signs"); + assert!(verify( + &pubkey, + &proof, + context.selection_proof_signing_root(100) + )); + assert_eq!(fake.requests.lock().expect("lock").len(), 2); + } + + #[tokio::test] + async fn a_slashing_protection_refusal_is_a_signing_error() { + let (_fake, url) = serve(Answer::Status(412)).await; + let (store, pubkey) = remote_store(&url); + + let err = context() + .sign_attestation(&store, &pubkey, &attestation_data(100)) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(err.to_string().contains("slashing protection"), "{err}"); + assert!(!err.is_retryable()); + } + + #[tokio::test] + async fn a_key_the_signer_does_not_hold_is_a_signing_error() { + let (_fake, url) = serve(Answer::Status(404)).await; + let (store, pubkey) = remote_store(&url); + + let err = context() + .sign_randao(&store, &pubkey, 1) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(err.to_string().contains("does not hold"), "{err}"); + } + + #[tokio::test] + async fn an_unreachable_signer_is_a_signing_error() { + // Nothing listens on port 9 (discard), and a connection refused + // comes back at once rather than after the request timeout. + let (store, pubkey) = remote_store("http://127.0.0.1:9"); + + let err = context() + .sign_randao(&store, &pubkey, 1) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + } + } } diff --git a/crates/validator/src/subscriptions.rs b/crates/validator/src/subscriptions.rs index 5de975781..c1ab00503 100644 --- a/crates/validator/src/subscriptions.rs +++ b/crates/validator/src/subscriptions.rs @@ -9,7 +9,7 @@ use std::sync::Arc; use tokio::sync::RwLock; -use tracing::{info, warn}; +use tracing::{debug, info, warn}; use crate::aggregation_selection::selection_for; use crate::beacon_node::BeaconNodeApi; @@ -17,10 +17,11 @@ use ethlambda_types::beacon::primitives::Epoch; use crate::beacon_node::dto::{ AttesterDutyDto, CommitteeSubscriptionDto, SyncCommitteeSubscriptionDto, SyncDutyDto, + parse_pubkey, }; -use crate::error::Result; +use crate::error::{Error, Result}; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; /// Send one subscription per held duty, claiming the aggregator role for the /// duties this client was selected for. @@ -47,37 +48,68 @@ pub async fn subscribe( return Ok(()); } - // The read guard is scoped to this block and never held across the await - // below, for the reason spelled out in `crate::attestation`: the lock is - // write-preferring, so one queued keymanager writer blocks every reader - // behind it. - let subscriptions: Vec = { - let store = store.read().await; - duties + // Every selection proof is asked for at once, each within the signing + // budget: one request per duty in series would cost a slow or + // unreachable signer as many round trips as there are duties before any + // subscription went out. Bounded this way it costs `SIGNING_BUDGET` at + // most, and the proofs that did arrive are remembered for the + // aggregation tick (see `SigningContext::sign_selection_proof`). The + // duty loop runs this from a task of its own, after the slot's block, so + // even that budget is never spent ahead of a proposal. + let mut is_aggregator = vec![false; duties.len()]; + let mut keyed = Vec::with_capacity(duties.len()); + for (position, duty) in duties.iter().enumerate() { + match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => keyed.push((position, duty, pubkey)), + Err(err) => warn!( + slot = duty.slot, + validator = duty.validator_index, + %err, + "Duty carried an unreadable pubkey; subscribing without the role" + ), + } + } + let selections = sign_all( + keyed .iter() - .map(|duty| { - let is_aggregator = match selection_for(context, &store, duty) { - Ok(selection) => selection.is_some(), - Err(err) => { - warn!( - slot = duty.slot, - validator = duty.validator_index, - %err, - "Could not compute aggregator selection; subscribing without the role" - ); - false - } - }; - CommitteeSubscriptionDto { - validator_index: duty.validator_index, - committee_index: duty.committee_index, - committees_at_slot: duty.committees_at_slot, - slot: duty.slot, - is_aggregator, - } - }) - .collect() - }; + .map(|(_, duty, pubkey)| (*pubkey, selection_for(context, store, duty))), + ) + .await; + for ((position, duty, _), selection) in keyed.into_iter().zip(selections) { + match selection { + Ok(selection) => is_aggregator[position] = selection.is_some(), + // A key the keymanager deleted since the schedule was fetched: + // nothing failed to sign, so nothing is counted. Its subscription + // still goes out, without the role, like any other it cannot + // claim one for. + Err(Error::UnknownValidator(_)) => debug!( + slot = duty.slot, + validator = duty.validator_index, + "Duty names a key this client no longer holds; subscribing without the role" + ), + Err(err) => { + warn!( + slot = duty.slot, + validator = duty.validator_index, + %err, + "Could not compute aggregator selection; subscribing without the role" + ); + crate::metrics::inc_signing_failures(); + } + } + } + + let subscriptions: Vec = duties + .iter() + .zip(is_aggregator) + .map(|(duty, is_aggregator)| CommitteeSubscriptionDto { + validator_index: duty.validator_index, + committee_index: duty.committee_index, + committees_at_slot: duty.committees_at_slot, + slot: duty.slot, + is_aggregator, + }) + .collect(); let aggregating = subscriptions .iter() @@ -134,10 +166,7 @@ mod tests { } fn context() -> SigningContext { - SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - } + SigningContext::new(Config::mainnet(), Root::ZERO) } fn store() -> (RwLock, BlsPubkey) { @@ -236,10 +265,10 @@ mod tests { .await .expect("subscribes"); - let guard = store.read().await; for (duty, sent) in duties.iter().zip(node.subscriptions()) { let proof = context - .sign_selection_proof(&guard, &pubkey, duty.slot) + .sign_selection_proof(&store, &pubkey, duty.slot) + .await .expect("signs"); assert_eq!( sent.is_aggregator, diff --git a/crates/validator/src/sync_committee.rs b/crates/validator/src/sync_committee.rs index 7501895e6..d898c92a9 100644 --- a/crates/validator/src/sync_committee.rs +++ b/crates/validator/src/sync_committee.rs @@ -46,7 +46,7 @@ use crate::beacon_node::dto::{SyncDutyDto, parse_pubkey}; use crate::beacon_node::{BeaconNodeApi, validate_sync_contribution}; use crate::error::{Error, Result}; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; /// How many slots of dedup history are kept. const HISTORY_SLOTS: u64 = 64; @@ -129,35 +129,45 @@ impl SyncCommitteeService { Err(err) => return Err(err), }; - let mut messages = Vec::with_capacity(pending.len()); - { - let _timing = crate::metrics::time_signing(); - let store = store.read().await; - for duty in pending { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - if !self.record(duty.validator_index, slot) { + // Recorded one by one, then signed all at once, each within the + // signing budget: a hung remote signer costs only its own keys' + // messages, not the whole batch's. + let mut to_sign = Vec::with_capacity(pending.len()); + for duty in pending { + let pubkey = match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => pubkey, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); continue; } - match self - .context - .sign_sync_committee_message(&store, &pubkey, slot, root) - { - Ok(signature) => messages.push(SyncCommitteeMessage { - slot, - beacon_block_root: root, - validator_index: duty.validator_index, - signature, - }), - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign sync committee message"); - crate::metrics::inc_signing_failures(); - } + }; + if self.record(duty.validator_index, slot) { + to_sign.push((duty.validator_index, pubkey)); + } + } + let signatures = { + let _timing = crate::metrics::time_signing(); + sign_all(to_sign.iter().map(|(_, pubkey)| { + ( + *pubkey, + self.context + .sign_sync_committee_message(store, pubkey, slot, root), + ) + })) + .await + }; + let mut messages = Vec::with_capacity(to_sign.len()); + for ((validator_index, _), signature) in to_sign.into_iter().zip(signatures) { + match signature { + Ok(signature) => messages.push(SyncCommitteeMessage { + slot, + beacon_block_root: root, + validator_index, + signature, + }), + Err(err) => { + error!(%slot, validator = validator_index, %err, "Failed to sign sync committee message"); + crate::metrics::inc_signing_failures(); } } } @@ -191,32 +201,40 @@ impl SyncCommitteeService { duties: &[SyncDutyDto], store: &RwLock, ) -> Result { + // Every (validator, subnet) pair's selection proof, asked for at once + // and each within the signing budget. + let mut seats = Vec::new(); + for duty in duties { + let pubkey = match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => pubkey, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); + continue; + } + }; + for subnet in subnets_of(duty) { + seats.push((duty.validator_index, pubkey, subnet)); + } + } + let proofs = sign_all(seats.iter().map(|(_, pubkey, subnet)| { + ( + *pubkey, + self.context + .sign_sync_selection_proof(store, pubkey, slot, *subnet), + ) + })) + .await; // (validator, pubkey, subnet, selection proof) for each selected pair. let mut selected: Vec<(ValidatorIndex, BlsPubkey, u64, BlsSignature)> = Vec::new(); - { - let store = store.read().await; - for duty in duties { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - for subnet in subnets_of(duty) { - match self - .context - .sign_sync_selection_proof(&store, &pubkey, slot, subnet) - { - Ok(proof) if is_sync_committee_aggregator(&proof) => { - selected.push((duty.validator_index, pubkey, subnet, proof)); - } - Ok(_) => {} - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign sync selection proof"); - crate::metrics::inc_signing_failures(); - } - } + for ((validator, pubkey, subnet), proof) in seats.into_iter().zip(proofs) { + match proof { + Ok(proof) if is_sync_committee_aggregator(&proof) => { + selected.push((validator, pubkey, subnet, proof)); + } + Ok(_) => {} + Err(err) => { + error!(%slot, validator, %err, "Failed to sign sync selection proof"); + crate::metrics::inc_signing_failures(); } } } @@ -258,27 +276,33 @@ impl SyncCommitteeService { fetched.insert(*subnet, answer); } - let mut signed = Vec::new(); - { - let store = store.read().await; - for (validator, pubkey, subnet, proof) in selected { - let Some(Some(contribution)) = fetched.get(&subnet) else { - continue; - }; - let message = ContributionAndProof { - aggregator_index: validator, - contribution: contribution.clone(), - selection_proof: proof, - }; - match self - .context - .sign_contribution_and_proof(&store, &pubkey, &message) - { - Ok(signature) => signed.push(SignedContributionAndProof { message, signature }), - Err(err) => { - error!(%slot, validator, %err, "Failed to sign contribution and proof"); - crate::metrics::inc_signing_failures(); - } + let mut to_sign = Vec::new(); + for (validator, pubkey, subnet, proof) in selected { + let Some(Some(contribution)) = fetched.get(&subnet) else { + continue; + }; + let message = ContributionAndProof { + aggregator_index: validator, + contribution: contribution.clone(), + selection_proof: proof, + }; + to_sign.push((pubkey, message)); + } + let signatures = sign_all(to_sign.iter().map(|(pubkey, message)| { + ( + *pubkey, + self.context + .sign_contribution_and_proof(store, pubkey, message), + ) + })) + .await; + let mut signed = Vec::with_capacity(to_sign.len()); + for ((_, message), signature) in to_sign.into_iter().zip(signatures) { + match signature { + Ok(signature) => signed.push(SignedContributionAndProof { message, signature }), + Err(err) => { + error!(%slot, validator = message.aggregator_index, %err, "Failed to sign contribution and proof"); + crate::metrics::inc_signing_failures(); } } } @@ -327,10 +351,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } fn store() -> (RwLock, BlsPubkey) { @@ -452,16 +473,13 @@ mod tests { /// Duty with a seat in every subnet, on the first slot where the selection /// is a mix, so "only selected pairs fetch" is not satisfied by all or none. - fn mixed_slot(context: &SigningContext, store: &ValidatorStore, pubkey: &BlsPubkey) -> Slot { + async fn mixed_slot( + context: &SigningContext, + store: &RwLock, + pubkey: &BlsPubkey, + ) -> Slot { for slot in 3200..4200 { - let picks = (0..4) - .filter(|subnet| { - let proof = context - .sign_sync_selection_proof(store, pubkey, slot, *subnet) - .expect("signs"); - is_sync_committee_aggregator(&proof) - }) - .count(); + let picks = picked_subnets(context, store, pubkey, slot).await.len(); if picks > 0 && picks < 4 { return slot; } @@ -469,6 +487,26 @@ mod tests { panic!("no mixed slot found"); } + /// The subnets whose selection proof selects this key at `slot`. + async fn picked_subnets( + context: &SigningContext, + store: &RwLock, + pubkey: &BlsPubkey, + slot: Slot, + ) -> Vec { + let mut picked = Vec::new(); + for subnet in 0..4 { + let proof = context + .sign_sync_selection_proof(store, pubkey, slot, subnet) + .await + .expect("signs"); + if is_sync_committee_aggregator(&proof) { + picked.push(subnet); + } + } + picked + } + fn all_subnet_duty(pubkey: &BlsPubkey) -> SyncDutyDto { let size = SYNC_SUBCOMMITTEE_SIZE as u64; duty(pubkey, 9, vec![0, size, 2 * size, 3 * size]) @@ -478,10 +516,7 @@ mod tests { async fn only_selected_pairs_fetch_and_a_published_contribution_verifies() { let (store, pubkey) = store(); let context = context(); - let slot = { - let guard = store.read().await; - mixed_slot(&context, &guard, &pubkey) - }; + let slot = mixed_slot(&context, &store, &pubkey).await; let root = Root::repeat_byte(4); let mut node = MockBeaconNode::new(); for subnet in 0..4 { @@ -495,15 +530,7 @@ mod tests { .await .expect("aggregates"); - let guard = store.read().await; - let expected: Vec = (0..4) - .filter(|subnet| { - let proof = context - .sign_sync_selection_proof(&guard, &pubkey, slot, *subnet) - .expect("signs"); - is_sync_committee_aggregator(&proof) - }) - .collect(); + let expected = picked_subnets(&context, &store, &pubkey, slot).await; let requested: Vec = node .contribution_requests() .iter() @@ -535,10 +562,7 @@ mod tests { async fn a_mismatched_contribution_is_not_published() { let (store, pubkey) = store(); let context = context(); - let slot = { - let guard = store.read().await; - mixed_slot(&context, &guard, &pubkey) - }; + let slot = mixed_slot(&context, &store, &pubkey).await; let root = Root::repeat_byte(4); let mut node = MockBeaconNode::new(); for subnet in 0..4 { @@ -561,22 +585,15 @@ mod tests { let (store, pubkey) = store(); let context = context(); // A slot where at least two subnets are selected. - let (slot, picked) = { - let guard = store.read().await; - (3200..6000) - .find_map(|slot| { - let picked: Vec = (0..4) - .filter(|subnet| { - let proof = context - .sign_sync_selection_proof(&guard, &pubkey, slot, *subnet) - .expect("signs"); - is_sync_committee_aggregator(&proof) - }) - .collect(); - (picked.len() >= 2).then_some((slot, picked)) - }) - .expect("a slot selecting two subnets") - }; + let mut found = None; + for slot in 3200..6000 { + let picked = picked_subnets(&context, &store, &pubkey, slot).await; + if picked.len() >= 2 { + found = Some((slot, picked)); + break; + } + } + let (slot, picked) = found.expect("a slot selecting two subnets"); let root = Root::repeat_byte(4); // Only the last picked subnet has a contribution; the first answers 404. let kept = *picked.last().expect("nonempty"); diff --git a/docs/cli.md b/docs/cli.md index bbb53e2e2..74a8ac2bd 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -288,14 +288,15 @@ implementation's, so none of the [common flags](#common-flags) apply to it. | Flag | Default | Meaning | |---|---|---| | `--beacon-nodes` | required | Base URLs of the beacon nodes to use, comma-separated or repeated. Tried in list order; the first that answers serves the request, so the order is a preference, not load balancing | -| `--validators-dir` | required | Directory holding the EIP-2335 keystores and `validator_definitions.yml`. Without that file, keystores in the Lighthouse layout (`<0xpubkey>/voting-keystore.json`, password in `--secrets-dir/<0xpubkey>`) are discovered and the file is written | +| `--validators-dir` | required | Directory holding the EIP-2335 keystores and `validator_definitions.yml`, which also lists the keys a remote signer holds (see [Remote signing](#remote-signing-web3signer)). Without that file, keystores in the Lighthouse layout (`<0xpubkey>/voting-keystore.json`, password in `--secrets-dir/<0xpubkey>`) are discovered and the file is written | | `--secrets-dir` | required | Directory holding one password file per keystore, named after the validator's public key | | `--http-address` | `127.0.0.1` | Bind address for the metrics and keymanager servers | | `--metrics-port` | `5064` | Prometheus metrics port | | `--keymanager-port` | `5062` | Keymanager API port. Only bound with `--enable-keymanager` | | `--suggested-fee-recipient` | none | Execution address to receive block rewards, `0x`-prefixed, for every validator the keymanager API gives no address of its own. Optional, and startup warns when it is absent: without it the beacon node picks an address, and it will not be yours | | `--graffiti` | empty | Text for the graffiti field of proposed blocks, for every validator the keymanager API gives no graffiti of its own. At most 32 bytes as UTF-8, right-padded with zeros. Refused rather than truncated if longer. An ethlambda beacon node appends both clients' versions to it (see [`rpc.md`](rpc.md#validator-endpoints)) | -| `--enable-keymanager` | off | Serve the keymanager API: keystores, plus each validator's own fee recipient, graffiti and gas limit. Off by default because it mutates key material | +| `--enable-keymanager` | off | Serve the keymanager API: keystores, remote keys, plus each validator's own fee recipient, graffiti and gas limit. Off by default because it mutates key material | +| `--web3signer-url` | none | Base URL of a Web3Signer (`https://signer:9000`): the signer a remote key imported through `POST /eth/v1/remotekeys` without a `url` of its own is assigned to. Checked for shape at startup. Keys whose definitions entry names a signer need no default | | `--init-slashing-protection` | off | Register every loaded key with no slashing-protection record as never having signed, instead of refusing to start. Dangerous: only for keys that have signed nothing anywhere for at least two epochs, and even then it rules out a double vote only, not a surround of an older vote; a key with a history should bring it instead. Never lowers an existing record; remove it once the keys are registered, and startup warns while it is set for nothing | ### What it does today @@ -380,9 +381,9 @@ assigned to slot `S` sign for `S - 1`. Both halves are bounded by the end of the slot. Not implemented: the builder flow and blinded blocks (the client asks for an -unblinded block and refuses a blinded one), doppelganger protection and remote -signing. Voluntary exits are signed by the keymanager API's `voluntary_exit` -route and by [`ethlambda voluntary-exit`](#voluntary-exit). +unblinded block and refuses a blinded one) and doppelganger protection. +Voluntary exits are signed by the keymanager API's `voluntary_exit` route and +by [`ethlambda voluntary-exit`](#voluntary-exit). ### Duty offsets come from the network @@ -410,6 +411,91 @@ attester duties untouched can still move a proposer between slots. This client fetches proposer duties once per epoch, at the boundary, so a reorg inside the epoch is not picked up until the next one. +### Remote signing (Web3Signer) + +A validator's key can stay in a [Web3Signer](https://docs.web3signer.consensys.io/) +instead of this process. Each signature is then one `POST +/api/v1/eth2/sign/{pubkey}` carrying the message in full (the attestation +data, the block header, the aggregate, and so on), the fork it is signed under +and the signing root, and the signer answers with the signature. Every duty +this client performs signs this way when the key is remote, the gloas envelope +and payload attestation included; a slot's attestations are requested +concurrently, so a hundred keys behind one signer cost one round trip rather +than a hundred in series. + +Because the message travels in full, the signer's own slashing protection +applies to it, on top of this client's: a remote key has a record in +[`slashing_protection.json`](#slashing-protection) like a local one, and a +block or attestation is checked against it and recorded before the signer is +asked. So a remote key written into the definitions file by hand is refused at +startup until it has a record, the same as a local keystore; one imported +through the keymanager API is registered as it is imported. The two records +are separate (see +[Spec Deviations](./spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history)): +a key moved to or from a signer carries its history through the signer's own +tools. A signer refusal (`412`) is logged, counted as a +signing failure and never retried; so is a key the signer does not hold, a +signer that cannot be reached, and an answer that does not verify: every +signature a signer returns is checked against the validator's public key and +the signing root before it is used, so a signer holding another key under that +pubkey, or computing another root, costs the signature rather than publishing +one no peer accepts. That check runs off the duty loop, a core per signature, +so a large batch of answers does not hold the loop up. Each duty asks for its +signatures all at once +and gives each one 2 seconds: a signer that has not answered by then costs the +signatures it owes and nothing else, so the duty publishes whatever arrived, +local keys and other signers' keys included. One signer is sent at most 64 +requests at a time and the rest of a batch waits its turn inside the same 2 +seconds, so a signer costs this client 64 connections however many keys it +holds; the sub-command also raises its open-file limit at startup, as `node` +and `beacon` do. Aggregator selection proofs are +remembered once produced, so the one the epoch's subnet subscription asked for +is not asked for again by the aggregation. Those subscriptions are sent from a +task of their own, started once the epoch's first slot has had its block, so +the round trips for their selection proofs never stand between a slot boundary +and a proposal. Nothing is asked of the signer at startup or at +import: a signer may start after this client does. + +A voluntary exit for a remote key is signed the same way (`VOLUNTARY_EXIT`), +through the keymanager API's `voluntary_exit` route. The signer derives an +exit's root itself and refuses a request naming another, so it is told the one +fork version the exit is signed under, as both `previous_version` and +`current_version`: from deneb on that is the capella version (EIP-7044), not +the fork the schedule has at the exit's epoch. [`ethlambda +voluntary-exit`](#voluntary-exit) decrypts a keystore file, so it signs for +local keys only. + +A remote key is declared in `validator_definitions.yml` with `type: +web3signer` and a `url` where a local entry has `type: local_keystore` and its +two keystore paths: + +```yaml +- enabled: true + voting_public_key: "0x9612…0d07" + type: web3signer + url: https://signer:9000 +``` + +or imported at runtime through `POST /eth/v1/remotekeys` (below). A `url` +left out of that request means `--web3signer-url`. An entry written without a +`type` (every entry from before remote signing) is read by its fields; one +naming both keystore paths and a `url` is refused at startup rather than read +as either. So is a `web3signer` entry carrying a field this client does not +implement, such as Lighthouse's `root_certificate_path`: skipping it would sign +through a connection the operator did not describe. `description` is the one +exception, since Lighthouse writes it on every entry. A local entry is read +past such fields, so a file Lighthouse wrote for local keystores loads as it +is; the fields are not kept when the file is next rewritten. The `url` must not +carry credentials, a query string or a fragment. + +The signer is reached over HTTP or HTTPS. HTTPS is verified against the Mozilla +root store bundled into the binary (`webpki-roots`), **not** the operating +system's trust store, and no other CA can be added: a signer behind an internal +or self-signed CA fails every TLS handshake, and every signature for its keys +is then counted as a signing failure. Client certificates are not supported +either. Put such a signer behind a certificate from a public CA, or reach it +over plain HTTP on a private network. + ### The keymanager API Off unless `--enable-keymanager` is passed. It serves `GET`, `POST` and @@ -418,10 +504,34 @@ Off unless `--enable-keymanager` is passed. It serves `GET`, `POST` and if absent. The token file is written `0600` and a file too short to be a real token is refused rather than accepted. +It serves `GET`, `POST` and `DELETE /eth/v1/remotekeys` for the keys a +Web3Signer holds. The two routes partition the store: `/keystores` lists and +deletes local keys only, `/remotekeys` remote keys only, and a key asked of the +wrong route is `not_found`. An import of a key either route already holds is +`duplicate`. `POST /remotekeys` persists each key to the definitions file and +registers it in the slashing-protection record before it starts signing, the +same order the keystores import uses, and `readonly` is always `false`: this +client has no key it cannot delete. These routes carry no slashing-protection +history, since the specification gives them no field for it: a remote key +imported with no record starts as never having signed, and `DELETE +/remotekeys` leaves the record in place. `DELETE /keystores` exports nothing +for a key a remote signer holds. + +A delete through either route reads the definitions file once and saves it +once for the whole request, and takes the keys out of memory in between, so a +request naming hundreds of keys does not hold up the duties' signatures. The +two routes can only disagree with the file about a key's kind after the file +was edited by hand under a running client. A delete that then finds the key in +memory as its own kind, and in the file as the other route's, stops signing +with it and answers `error` naming the other route, not `deleted`: the entry +it leaves would load the key again at the next start. `DELETE /remotekeys` of +a remote entry whose key this process runs from a local keystore removes the +entry and leaves the local key, and its per-validator settings, as they are. + It also serves `POST /eth/v1/validator/{pubkey}/voluntary_exit`, which signs a -voluntary exit for a validator this client holds and answers with the -`SignedVoluntaryExit`, for the exit's `epoch` query parameter or the current -epoch when none is given. **It does not publish the exit.** As the +voluntary exit for a validator this client holds, in a local keystore or in a +remote signer, and answers with the `SignedVoluntaryExit`, for the exit's +`epoch` query parameter or the current epoch when none is given. **It does not publish the exit.** As the specification has it, the caller hands the message to a beacon node's `POST /eth/v1/beacon/pool/voluntary_exits`, or holds on to it. The validator index comes from the beacon node on each call, so a key whose deposit the chain diff --git a/docs/metrics.md b/docs/metrics.md index 9ac92fd4f..c6f3b89bc 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -745,7 +745,8 @@ and absent is not the same as zero. | `ethlambda_validator_fee_recipient_mismatches_total` | Counter | Blocks paying execution rewards to an address this client did not request. Should read zero forever; a non-zero value means every proposal is paying somewhere else | | `ethlambda_validator_block_publication_delay_seconds` | Histogram | Slot start to block accepted. Its buckets are tighter than the attestation histogram's, because a block is due at the slot boundary rather than a third of the way in | | `ethlambda_validator_beacon_node_available` | Gauge | 1 when a beacon node answered the last duty refresh or attestation | -| `ethlambda_validator_signing_duration_seconds` | Histogram | Time to sign one slot's batch, store read lock included | +| `ethlambda_validator_signing_duration_seconds` | Histogram | Time a duty waited for its batch of signatures. With local keys that is the signing itself. With keys in a remote signer it is the batch's slowest answer, round trip and verification included, and at most the 2 s signing budget, so a sample in the top bucket means a signer that did not answer in time | +| `ethlambda_validator_remote_signer_request_duration_seconds` | Histogram | Time one signing request to a remote signer took, from sending it to its answer or failure, not counting the wait for one of the signer's 64 request slots. The number to watch for a slow signer; empty when every key is local | | `ethlambda_validator_slashing_protection_write_seconds` | Histogram | Time to durably write the slashing-protection record, once per slot's attestation batch and once per block, before signing. Milliseconds on a local disk; worth an alert above about a second, since the duty waits for it and a write held past 3 s is refused | | `ethlambda_validator_slashing_protection_write_failures_total` | Counter | Slashing-protection writes that failed, each leaving its messages unsigned. Should read zero; anything else points at the disk under `--validators-dir` | | `ethlambda_validator_publication_delay_seconds` | Histogram | Slot start to attestations accepted. The headline health number: it should sit near the one-third-slot duty offset | diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 49ee15c03..263e0c7ed 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -114,6 +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. +- **A key held by a remote signer:** a key in a Web3Signer (see + [Remote signing](./cli.md#remote-signing-web3signer)) is registered in this + record and checked against it like any other, and is then signed by the + signer, which receives every message in full and keeps a record of its own; + a message it refuses is dropped here and never retried. The two records are + separate: the signer's is neither imported into this one nor exported from + it, so a key moved to or from a signer carries its history through the + signer's own tools. - **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,