From f76f97a3fd2b2b94a497af6325677f4eac9576c1 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 23 Nov 2019 17:02:39 +1100 Subject: [PATCH] Tidy signing, reduce ForkService duplication --- account_manager/src/cli.rs | 4 +- beacon_node/rest_api/src/config.rs | 2 +- beacon_node/rest_api/tests/test.rs | 5 +- book/src/ci.md | 1 - book/src/installation.md | 4 +- book/src/setup.md | 11 ++- eth2/types/src/beacon_block.rs | 7 ++ lighthouse/src/main.rs | 6 +- validator_client/src/attestation_service.rs | 30 ++----- validator_client/src/block_service.rs | 47 +++------- validator_client/src/duties_service.rs | 6 +- validator_client/src/lib.rs | 53 ++++++------ validator_client/src/validator_store.rs | 96 +++++++++++---------- 13 files changed, 119 insertions(+), 153 deletions(-) delete mode 100644 book/src/ci.md diff --git a/account_manager/src/cli.rs b/account_manager/src/cli.rs index 38b289ff32..01d6376cdb 100644 --- a/account_manager/src/cli.rs +++ b/account_manager/src/cli.rs @@ -12,7 +12,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .about("Create a new Ethereum 2.0 validator.") .subcommand( SubCommand::with_name("insecure") - .about("Uses the insecure deterministic keypairs. Do not store value in these.") + .about("Produce insecure, ephemeral validators. DO NOT USE TO STORE VALUE.") .arg( Arg::with_name("first") .index(1) @@ -32,7 +32,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { ) .subcommand( SubCommand::with_name("random") - .about("Uses the Rust rand crate ThreadRandom to generate keys.") + .about("Produces public keys using entropy from the Rust 'rand' library.") .arg( Arg::with_name("validator_count") .index(1) diff --git a/beacon_node/rest_api/src/config.rs b/beacon_node/rest_api/src/config.rs index d52d3eaed9..1d7f50b6ee 100644 --- a/beacon_node/rest_api/src/config.rs +++ b/beacon_node/rest_api/src/config.rs @@ -2,7 +2,7 @@ use clap::ArgMatches; use serde::{Deserialize, Serialize}; use std::net::Ipv4Addr; -/// Defines the encoding for the API +/// Defines the encoding for the API. #[derive(Clone, Serialize, Deserialize, Copy)] pub enum ApiEncodingFormat { JSON, diff --git a/beacon_node/rest_api/tests/test.rs b/beacon_node/rest_api/tests/test.rs index 7b0add4be2..3d029f0966 100644 --- a/beacon_node/rest_api/tests/test.rs +++ b/beacon_node/rest_api/tests/test.rs @@ -55,10 +55,7 @@ fn sign_block( .block_proposer(block.slot) .expect("should get proposer index"); let keypair = generate_deterministic_keypair(proposer_index); - let epoch = block.slot.epoch(E::slots_per_epoch()); - let message = block.signed_root(); - let domain = spec.get_domain(epoch, Domain::BeaconProposer, &fork); - block.signature = Signature::new(&message, domain, &keypair.sk); + block.sign(&keypair.sk, &fork, spec); } #[test] diff --git a/book/src/ci.md b/book/src/ci.md deleted file mode 100644 index 02289a8d00..0000000000 --- a/book/src/ci.md +++ /dev/null @@ -1 +0,0 @@ -# CI & Testing diff --git a/book/src/installation.md b/book/src/installation.md index 92bfef68fe..547bcac69c 100644 --- a/book/src/installation.md +++ b/book/src/installation.md @@ -12,10 +12,10 @@ If this doesn't work or is not clear enough, see the [Detailed Instructions](#de ## Detailed Instructions 1. Install Rust and Cargo with [rustup](https://rustup.rs/). - - Use the `stable` toolchain (it's the default). + - Use the `stable` toolchain (it's the default). 1. Clone the Lighthouse repository. - Run `$ git clone https://github.com/sigp/lighthouse.git` - - Change into the newly created directory with `$ cd lighthouse` + - Change into the newly created directory with `$ cd lighthouse` 1. Build Lighthouse with `$ make`. 1. Installation was successful if `$ lighthouse --help` displays the command-line documentation. diff --git a/book/src/setup.md b/book/src/setup.md index bd3a8259e3..8573a9a370 100644 --- a/book/src/setup.md +++ b/book/src/setup.md @@ -15,9 +15,8 @@ don't have `ganache-cli` available on your `PATH`. ## Testing -Lighthouse uses `cargo test` for running the test suite. This is the -recommended work-flow for testing during development. For example, test the -`ssz` crate with: +As with most other Rust projects, Lighthouse uses `cargo test` for unit and +integration tests. For example, to test the `ssz` crate run: ```bash cd eth2/utils/ssz @@ -45,6 +44,6 @@ repository contains a large set of tests that verify Lighthouse behaviour against the Ethereum Foundation specifications. These tests are quite large (100's of MB) so they're only downloaded if you run -the `$ make test-ef` command (or anything that calls it). You may want to avoid -these if you're on a slow or metered Internet connection, CI will -require them to pass though. +`$ make test-ef` (or anything that run it). You may want to avoid +downloadingthese tests if you're on a slow or metered Internet connection. CI +will require them to pass, though. diff --git a/eth2/types/src/beacon_block.rs b/eth2/types/src/beacon_block.rs index 4c8fb0110d..1b5757e94f 100644 --- a/eth2/types/src/beacon_block.rs +++ b/eth2/types/src/beacon_block.rs @@ -100,6 +100,13 @@ impl BeaconBlock { ..self.block_header() } } + + /// Signs `self`. + pub fn sign(&mut self, secret_key: &SecretKey, fork: &Fork, spec: &ChainSpec) { + let message = self.signed_root(); + let domain = spec.get_domain(self.epoch(), Domain::BeaconProposer, &fork); + self.signature = Signature::new(&message, domain, &secret_key); + } } #[cfg(test)] diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index 63e466f12f..6a47f854b6 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -137,12 +137,12 @@ fn run( account_manager::run(sub_matches, runtime_context); - // Exit early if the account manager was run. It does not used the tokio executor, so no - // need to wait for it to shutdown. + // Exit early if the account manager was run. It does not use the tokio executor, no need + // to wait for it to shutdown. return Ok(()); } - let beacon_node = if let Some(sub_matches) = matches.subcommand_matches("Beacon Node") { + let beacon_node = if let Some(sub_matches) = matches.subcommand_matches("beacon_node") { let runtime_context = environment.core_context(); let beacon = environment diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 41a67d1db5..1fe5a488c4 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -1,6 +1,4 @@ -use crate::{ - duties_service::DutiesService, fork_service::ForkService, validator_store::ValidatorStore, -}; +use crate::{duties_service::DutiesService, validator_store::ValidatorStore}; use environment::RuntimeContext; use exit_future::Signal; use futures::{Future, Stream}; @@ -11,16 +9,15 @@ use std::collections::HashMap; use std::sync::Arc; use std::time::{Duration, Instant}; use tokio::timer::Interval; -use types::{ChainSpec, CommitteeIndex, EthSpec, Fork, Slot}; +use types::{ChainSpec, CommitteeIndex, EthSpec, Slot}; /// Delay this period of time after the slot starts. This allows the node to process the new slot. const TIME_DELAY_FROM_SLOT: Duration = Duration::from_millis(100); #[derive(Clone)] pub struct AttestationServiceBuilder { - fork_service: Option>, duties_service: Option>, - validator_store: Option>, + validator_store: Option>, slot_clock: Option, beacon_node: Option>, context: Option>, @@ -30,7 +27,6 @@ pub struct AttestationServiceBuilder { impl AttestationServiceBuilder { pub fn new() -> Self { Self { - fork_service: None, duties_service: None, validator_store: None, slot_clock: None, @@ -39,17 +35,12 @@ impl AttestationServiceBuilder } } - pub fn fork_service(mut self, service: ForkService) -> Self { - self.fork_service = Some(service); - self - } - pub fn duties_service(mut self, service: DutiesService) -> Self { self.duties_service = Some(service); self } - pub fn validator_store(mut self, store: ValidatorStore) -> Self { + pub fn validator_store(mut self, store: ValidatorStore) -> Self { self.validator_store = Some(store); self } @@ -72,9 +63,6 @@ impl AttestationServiceBuilder pub fn build(self) -> Result, String> { Ok(AttestationService { inner: Arc::new(Inner { - fork_service: self - .fork_service - .ok_or_else(|| "Cannot build AttestationService without fork_service")?, duties_service: self .duties_service .ok_or_else(|| "Cannot build AttestationService without duties_service")?, @@ -97,8 +85,7 @@ impl AttestationServiceBuilder pub struct Inner { duties_service: DutiesService, - fork_service: ForkService, - validator_store: ValidatorStore, + validator_store: ValidatorStore, slot_clock: T, beacon_node: RemoteBeaconNode, context: RuntimeContext, @@ -178,10 +165,6 @@ impl AttestationService { .slot_clock .now() .ok_or_else(|| "Failed to read slot clock".to_string())?; - let fork = inner - .fork_service - .fork() - .ok_or_else(|| "Failed to get Fork".to_string())?; let mut committee_indices: HashMap> = HashMap::new(); @@ -206,7 +189,6 @@ impl AttestationService { slot, committee_index, validator_duties, - fork.clone(), )); }); @@ -218,7 +200,6 @@ impl AttestationService { slot: Slot, committee_index: CommitteeIndex, validator_duties: Vec, - fork: Fork, ) -> impl Future { let inner_1 = self.inner.clone(); let inner_2 = self.inner.clone(); @@ -250,7 +231,6 @@ impl AttestationService { &duty.validator_pubkey, validator_committee_position, attestation, - &fork, ) .ok_or_else(|| "Unable to sign attestation".to_string()) } else { diff --git a/validator_client/src/block_service.rs b/validator_client/src/block_service.rs index 7a202969f8..5f283fd6d8 100644 --- a/validator_client/src/block_service.rs +++ b/validator_client/src/block_service.rs @@ -1,6 +1,4 @@ -use crate::{ - duties_service::DutiesService, fork_service::ForkService, validator_store::ValidatorStore, -}; +use crate::{duties_service::DutiesService, validator_store::ValidatorStore}; use environment::RuntimeContext; use exit_future::Signal; use futures::{stream, Future, IntoFuture, Stream}; @@ -17,9 +15,8 @@ const TIME_DELAY_FROM_SLOT: Duration = Duration::from_millis(100); #[derive(Clone)] pub struct BlockServiceBuilder { - fork_service: Option>, duties_service: Option>, - validator_store: Option>, + validator_store: Option>, slot_clock: Option>, beacon_node: Option>, context: Option>, @@ -29,7 +26,6 @@ pub struct BlockServiceBuilder { impl BlockServiceBuilder { pub fn new() -> Self { Self { - fork_service: None, duties_service: None, validator_store: None, slot_clock: None, @@ -38,17 +34,12 @@ impl BlockServiceBuilder { } } - pub fn fork_service(mut self, service: ForkService) -> Self { - self.fork_service = Some(service); - self - } - pub fn duties_service(mut self, service: DutiesService) -> Self { self.duties_service = Some(service); self } - pub fn validator_store(mut self, store: ValidatorStore) -> Self { + pub fn validator_store(mut self, store: ValidatorStore) -> Self { self.validator_store = Some(store); self } @@ -70,9 +61,6 @@ impl BlockServiceBuilder { pub fn build(self) -> Result, String> { Ok(BlockService { - fork_service: self - .fork_service - .ok_or_else(|| "Cannot build BlockService without fork_service")?, duties_service: self .duties_service .ok_or_else(|| "Cannot build BlockService without duties_service")?, @@ -95,8 +83,7 @@ impl BlockServiceBuilder { #[derive(Clone)] pub struct BlockService { duties_service: DutiesService, - fork_service: ForkService, - validator_store: ValidatorStore, + validator_store: ValidatorStore, slot_clock: Arc, beacon_node: RemoteBeaconNode, context: RuntimeContext, @@ -167,29 +154,17 @@ impl BlockService { let service_3 = service.clone(); block_producers.next().map(move |validator_pubkey| { - service_2 - .fork_service - .fork() - .ok_or_else(|| "Fork is unknown, unable to sign".to_string()) - .and_then(|fork| { - service_1 - .validator_store - .randao_reveal( - &validator_pubkey, - slot.epoch(E::slots_per_epoch()), - &fork, - ) - .map(|randao_reveal| (fork, randao_reveal)) - .ok_or_else(|| "Unable to produce randao reveal".to_string()) - }) + service_1 + .validator_store + .randao_reveal(&validator_pubkey, slot.epoch(E::slots_per_epoch())) + .ok_or_else(|| "Unable to produce randao reveal".to_string()) .into_future() - .and_then(move |(fork, randao_reveal)| { + .and_then(move |randao_reveal| { service_1 .beacon_node .http .validator() .produce_block(slot, randao_reveal) - .map(|block| (fork, block)) .map_err(|e| { format!( "Error from beacon node when producing block: {:?}", @@ -197,10 +172,10 @@ impl BlockService { ) }) }) - .and_then(move |(fork, block)| { + .and_then(move |block| { service_2 .validator_store - .sign_block(&validator_pubkey, block, &fork) + .sign_block(&validator_pubkey, block) .ok_or_else(|| "Unable to sign block".to_string()) }) .and_then(move |block| { diff --git a/validator_client/src/duties_service.rs b/validator_client/src/duties_service.rs index e236953fd3..12b0d1b17d 100644 --- a/validator_client/src/duties_service.rs +++ b/validator_client/src/duties_service.rs @@ -124,7 +124,7 @@ impl DutiesStore { #[derive(Clone)] pub struct DutiesServiceBuilder { store: Option>, - validator_store: Option>, + validator_store: Option>, slot_clock: Option>, beacon_node: Option>, context: Option>, @@ -142,7 +142,7 @@ impl DutiesServiceBuilder { } } - pub fn validator_store(mut self, store: ValidatorStore) -> Self { + pub fn validator_store(mut self, store: ValidatorStore) -> Self { self.validator_store = Some(store); self } @@ -184,7 +184,7 @@ impl DutiesServiceBuilder { #[derive(Clone)] pub struct DutiesService { store: Arc, - validator_store: ValidatorStore, + validator_store: ValidatorStore, slot_clock: Arc, beacon_node: RemoteBeaconNode, context: RuntimeContext, diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 1ceb527d4a..7c5b787d87 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -120,26 +120,35 @@ impl ProductionValidatorClient { Duration::from_millis(context.eth2_config.spec.milliseconds_per_slot), ); - let validator_store: ValidatorStore = match &config.key_source { - // Load pre-existing validators from the data dir. - // - // Use the `account_manager` to generate these files. - KeySource::Disk => ValidatorStore::load_from_disk( - config.data_dir.clone(), - context.eth2_config.spec.clone(), - log_3.clone(), - )?, - // Generate ephemeral insecure keypairs for testing purposes. - // - // Do not use in production. - KeySource::TestingKeypairRange(range) => { - ValidatorStore::insecure_ephemeral_validators( - range.clone(), + let fork_service = ForkServiceBuilder::new() + .slot_clock(slot_clock.clone()) + .beacon_node(beacon_node.clone()) + .runtime_context(context.service_context("fork")) + .build()?; + + let validator_store: ValidatorStore = + match &config.key_source { + // Load pre-existing validators from the data dir. + // + // Use the `account_manager` to generate these files. + KeySource::Disk => ValidatorStore::load_from_disk( + config.data_dir.clone(), context.eth2_config.spec.clone(), + fork_service.clone(), log_3.clone(), - )? - } - }; + )?, + // Generate ephemeral insecure keypairs for testing purposes. + // + // Do not use in production. + KeySource::TestingKeypairRange(range) => { + ValidatorStore::insecure_ephemeral_validators( + range.clone(), + context.eth2_config.spec.clone(), + fork_service.clone(), + log_3.clone(), + )? + } + }; info!( log_3, @@ -154,15 +163,8 @@ impl ProductionValidatorClient { .runtime_context(context.service_context("duties")) .build()?; - let fork_service = ForkServiceBuilder::new() - .slot_clock(slot_clock.clone()) - .beacon_node(beacon_node.clone()) - .runtime_context(context.service_context("fork")) - .build()?; - let block_service = BlockServiceBuilder::new() .duties_service(duties_service.clone()) - .fork_service(fork_service.clone()) .slot_clock(slot_clock.clone()) .validator_store(validator_store.clone()) .beacon_node(beacon_node.clone()) @@ -171,7 +173,6 @@ impl ProductionValidatorClient { let attestation_service = AttestationServiceBuilder::new() .duties_service(duties_service.clone()) - .fork_service(fork_service.clone()) .slot_clock(slot_clock) .validator_store(validator_store) .beacon_node(beacon_node) diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 2e089b2f71..a730e79081 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -1,7 +1,9 @@ +use crate::fork_service::ForkService; use crate::validator_directory::{ValidatorDirectory, ValidatorDirectoryBuilder}; use parking_lot::RwLock; use rayon::prelude::*; use slog::{error, Logger}; +use slot_clock::SlotClock; use std::collections::HashMap; use std::fs::read_dir; use std::iter::FromIterator; @@ -10,22 +12,28 @@ use std::ops::Range; use std::path::PathBuf; use std::sync::Arc; use tempdir::TempDir; -use tree_hash::{SignedRoot, TreeHash}; +use tree_hash::TreeHash; use types::{ Attestation, BeaconBlock, ChainSpec, Domain, Epoch, EthSpec, Fork, PublicKey, Signature, }; #[derive(Clone)] -pub struct ValidatorStore { +pub struct ValidatorStore { validators: Arc>>, spec: Arc, log: Logger, temp_dir: Option>, + fork_service: ForkService, _phantom: PhantomData, } -impl ValidatorStore { - pub fn load_from_disk(base_dir: PathBuf, spec: ChainSpec, log: Logger) -> Result { +impl ValidatorStore { + pub fn load_from_disk( + base_dir: PathBuf, + spec: ChainSpec, + fork_service: ForkService, + log: Logger, + ) -> Result { let validator_iter = read_dir(&base_dir) .map_err(|e| format!("Failed to read base directory: {:?}", e))? .filter_map(|validator_dir| { @@ -60,6 +68,7 @@ impl ValidatorStore { spec: Arc::new(spec), log, temp_dir: None, + fork_service, _phantom: PhantomData, }) } @@ -67,6 +76,7 @@ impl ValidatorStore { pub fn insecure_ephemeral_validators( range: Range, spec: ChainSpec, + fork_service: ForkService, log: Logger, ) -> Result { let temp_dir = TempDir::new("insecure_validator") @@ -100,6 +110,7 @@ impl ValidatorStore { spec: Arc::new(spec), log, temp_dir: Some(Arc::new(temp_dir)), + fork_service, _phantom: PhantomData, }) } @@ -116,22 +127,27 @@ impl ValidatorStore { self.validators.read().len() } - pub fn randao_reveal( - &self, - validator_pubkey: &PublicKey, - epoch: Epoch, - fork: &Fork, - ) -> Option { + fn fork(&self) -> Option { + if self.fork_service.fork().is_none() { + error!( + self.log, + "Unable to get Fork for signing"; + ); + } + self.fork_service.fork() + } + + pub fn randao_reveal(&self, validator_pubkey: &PublicKey, epoch: Epoch) -> Option { // TODO: check this against the slot clock to make sure it's not an early reveal? self.validators .read() .get(validator_pubkey) .and_then(|validator_dir| { - validator_dir.voting_keypair.as_ref().map(|voting_keypair| { - let message = epoch.tree_hash_root(); - let domain = self.spec.get_domain(epoch, Domain::Randao, &fork); - Signature::new(&message, domain, &voting_keypair.sk) - }) + let voting_keypair = validator_dir.voting_keypair.as_ref()?; + let message = epoch.tree_hash_root(); + let domain = self.spec.get_domain(epoch, Domain::Randao, &self.fork()?); + + Some(Signature::new(&message, domain, &voting_keypair.sk)) }) } @@ -139,20 +155,15 @@ impl ValidatorStore { &self, validator_pubkey: &PublicKey, mut block: BeaconBlock, - fork: &Fork, ) -> Option> { // TODO: check for slashing. self.validators .read() .get(validator_pubkey) .and_then(|validator_dir| { - validator_dir.voting_keypair.as_ref().map(|voting_keypair| { - let epoch = block.slot.epoch(E::slots_per_epoch()); - let message = block.signed_root(); - let domain = self.spec.get_domain(epoch, Domain::BeaconProposer, &fork); - block.signature = Signature::new(&message, domain, &voting_keypair.sk); - block - }) + let voting_keypair = validator_dir.voting_keypair.as_ref()?; + block.sign(&voting_keypair.sk, &self.fork()?, &self.spec); + Some(block) }) } @@ -161,34 +172,31 @@ impl ValidatorStore { validator_pubkey: &PublicKey, validator_committee_position: usize, mut attestation: Attestation, - fork: &Fork, ) -> Option> { // TODO: check for slashing. self.validators .read() .get(validator_pubkey) .and_then(|validator_dir| { - validator_dir - .voting_keypair - .as_ref() - .and_then(|voting_keypair| { - attestation - .sign( - &voting_keypair.sk, - validator_committee_position, - fork, - &self.spec, - ) - .map_err(|e| { - error!( - self.log, - "Error whilst signing attestation"; - "error" => format!("{:?}", e) - ) - }) - .map(|()| attestation) - .ok() + let voting_keypair = validator_dir.voting_keypair.as_ref()?; + + attestation + .sign( + &voting_keypair.sk, + validator_committee_position, + &self.fork()?, + &self.spec, + ) + .map_err(|e| { + error!( + self.log, + "Error whilst signing attestation"; + "error" => format!("{:?}", e) + ) }) + .ok()?; + + Some(attestation) }) } }