From 65cbf601aef3f69cb728364d14573acb910bb591 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 25 Nov 2019 15:11:27 +1100 Subject: [PATCH] Address Michael's comments --- beacon_node/beacon_chain/src/beacon_chain.rs | 5 ++-- beacon_node/rest_api/Cargo.toml | 24 +++++++++---------- beacon_node/rest_api/src/beacon.rs | 20 ++++++++-------- beacon_node/rest_api/src/metrics.rs | 2 +- beacon_node/rest_api/src/spec.rs | 6 ++--- beacon_node/rest_api/src/validator.rs | 14 +++++------ beacon_node/rest_api/tests/test.rs | 5 +--- book/src/setup.md | 2 +- .../slot_clock/src/testing_slot_clock.rs | 2 +- validator_client/src/duties_service.rs | 12 ++-------- validator_client/src/validator_directory.rs | 4 ++-- 11 files changed, 42 insertions(+), 54 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3c3fa99ff6..a35aa85dec 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -525,7 +525,7 @@ impl BeaconChain { } } - /// Produce an `Attestation` that is valid for the given `slot` `shard`. + /// Produce an `Attestation` that is valid for the given `slot` and `index`. /// /// Always attests to the canonical chain. pub fn produce_attestation( @@ -870,8 +870,7 @@ impl BeaconChain { result }; - if block.slot > 0 - && (block.slot <= finalized_epoch.start_slot(T::EthSpec::slots_per_epoch())) + if block.slot > 0 && block.slot <= finalized_epoch.start_slot(T::EthSpec::slots_per_epoch()) { // Ignore any attestation where the slot of `data.beacon_block_root` is equal to or // prior to the finalized epoch. diff --git a/beacon_node/rest_api/Cargo.toml b/beacon_node/rest_api/Cargo.toml index 77c032051b..f9370e335d 100644 --- a/beacon_node/rest_api/Cargo.toml +++ b/beacon_node/rest_api/Cargo.toml @@ -13,28 +13,28 @@ eth2-libp2p = { path = "../eth2-libp2p" } store = { path = "../store" } version = { path = "../version" } serde = { version = "1.0", features = ["derive"] } -serde_json = "^1.0" +serde_json = "1.0" serde_yaml = "0.8" -slog = "^2.2.3" -slog-term = "^2.4.0" -slog-async = "^2.3.0" +slog = "2.5" +slog-term = "2.4" +slog-async = "2.3" eth2_ssz = { path = "../../eth2/utils/ssz" } eth2_ssz_derive = { path = "../../eth2/utils/ssz_derive" } state_processing = { path = "../../eth2/state_processing" } types = { path = "../../eth2/types" } -clap = "2.32.0" -http = "^0.1.17" -hyper = "0.12.35" -exit-future = "0.1.3" -tokio = "0.1.17" -url = "2.0" +clap = "2.33" +http = "0.1" +hyper = "0.12" +exit-future = "0.1.4" +tokio = "0.1.22" +url = "2.1" lazy_static = "1.3.0" eth2_config = { path = "../../eth2/utils/eth2_config" } lighthouse_metrics = { path = "../../eth2/utils/lighthouse_metrics" } slot_clock = { path = "../../eth2/utils/slot_clock" } -hex = "0.3.2" +hex = "0.3" parking_lot = "0.9" -futures = "0.1.25" +futures = "0.1.29" [dev-dependencies] remote_beacon_node = { path = "../../eth2/utils/remote_beacon_node" } diff --git a/beacon_node/rest_api/src/beacon.rs b/beacon_node/rest_api/src/beacon.rs index 2a95eba5de..9aa19cd457 100644 --- a/beacon_node/rest_api/src/beacon.rs +++ b/beacon_node/rest_api/src/beacon.rs @@ -23,7 +23,7 @@ pub struct HeadResponse { } /// HTTP handler to return a `BeaconBlock` at a given `root` or `slot`. -pub fn get_head( +pub fn get_head( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -64,7 +64,7 @@ pub struct BlockResponse { } /// HTTP handler to return a `BeaconBlock` at a given `root` or `slot`. -pub fn get_block( +pub fn get_block( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -102,7 +102,7 @@ pub fn get_block( } /// HTTP handler to return a `BeaconBlock` root at a given `slot`. -pub fn get_block_root( +pub fn get_block_root( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -117,7 +117,7 @@ pub fn get_block_root( } /// HTTP handler to return the `Fork` of the current head. -pub fn get_fork( +pub fn get_fork( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -128,7 +128,7 @@ pub fn get_fork( /// /// The `Epoch` parameter can be any epoch number. If it is not specified, /// the current epoch is assumed. -pub fn get_validators( +pub fn get_validators( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -168,7 +168,7 @@ pub struct StateResponse { /// /// Will not return a state if the request slot is in the future. Will return states higher than /// the current head by skipping slots. -pub fn get_state( +pub fn get_state( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -216,7 +216,7 @@ pub fn get_state( /// /// Will not return a state if the request slot is in the future. Will return states higher than /// the current head by skipping slots. -pub fn get_state_root( +pub fn get_state_root( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -229,7 +229,7 @@ pub fn get_state_root( } /// HTTP handler to return the highest finalized slot. -pub fn get_current_finalized_checkpoint( +pub fn get_current_finalized_checkpoint( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -241,7 +241,7 @@ pub fn get_current_finalized_checkpoint( } /// HTTP handler to return a `BeaconState` at the genesis block. -pub fn get_genesis_state( +pub fn get_genesis_state( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -251,7 +251,7 @@ pub fn get_genesis_state( } /// Read the genesis time from the current beacon chain state. -pub fn get_genesis_time( +pub fn get_genesis_time( req: Request, beacon_chain: Arc>, ) -> ApiResult { diff --git a/beacon_node/rest_api/src/metrics.rs b/beacon_node/rest_api/src/metrics.rs index fbf36f116c..3b4466da12 100644 --- a/beacon_node/rest_api/src/metrics.rs +++ b/beacon_node/rest_api/src/metrics.rs @@ -28,7 +28,7 @@ lazy_static! { /// # Note /// /// This is a HTTP handler method. -pub fn get_prometheus( +pub fn get_prometheus( req: Request, beacon_chain: Arc>, db_path: PathBuf, diff --git a/beacon_node/rest_api/src/spec.rs b/beacon_node/rest_api/src/spec.rs index 529ed830fe..6fd2575f5f 100644 --- a/beacon_node/rest_api/src/spec.rs +++ b/beacon_node/rest_api/src/spec.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use types::EthSpec; /// HTTP handler to return the full spec object. -pub fn get_spec( +pub fn get_spec( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -15,7 +15,7 @@ pub fn get_spec( } /// HTTP handler to return the full Eth2Config object. -pub fn get_eth2_config( +pub fn get_eth2_config( req: Request, eth2_config: Arc, ) -> ApiResult { @@ -23,6 +23,6 @@ pub fn get_eth2_config( } /// HTTP handler to return the full spec object. -pub fn get_slots_per_epoch(req: Request) -> ApiResult { +pub fn get_slots_per_epoch(req: Request) -> ApiResult { ResponseBuilder::new(&req)?.body(&T::EthSpec::slots_per_epoch()) } diff --git a/beacon_node/rest_api/src/validator.rs b/beacon_node/rest_api/src/validator.rs index c7029e5a57..cb0a42b51d 100644 --- a/beacon_node/rest_api/src/validator.rs +++ b/beacon_node/rest_api/src/validator.rs @@ -41,7 +41,7 @@ pub struct BulkValidatorDutiesRequest { /// HTTP Handler to retrieve a the duties for a set of validators during a particular epoch. This /// method allows for collecting bulk sets of validator duties without risking exceeding the max /// URL length with query pairs. -pub fn post_validator_duties( +pub fn post_validator_duties( req: Request, beacon_chain: Arc>, ) -> BoxFut { @@ -70,7 +70,7 @@ pub fn post_validator_duties( /// HTTP Handler to retrieve a the duties for a set of validators during a particular epoch /// /// The given `epoch` must be within one epoch of the current epoch. -pub fn get_validator_duties( +pub fn get_validator_duties( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -88,7 +88,7 @@ pub fn get_validator_duties( ResponseBuilder::new(&req)?.body_no_ssz(&duties) } -fn return_validator_duties( +fn return_validator_duties( beacon_chain: Arc>, epoch: Epoch, validator_pubkeys: Vec, @@ -174,7 +174,7 @@ fn return_validator_duties( } /// HTTP Handler to produce a new BeaconBlock from the current state, ready to be signed by a validator. -pub fn get_new_beacon_block( +pub fn get_new_beacon_block( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -196,7 +196,7 @@ pub fn get_new_beacon_block( } /// HTTP Handler to publish a BeaconBlock, which has been signed by a validator. -pub fn publish_beacon_block( +pub fn publish_beacon_block( req: Request, beacon_chain: Arc>, network_chan: NetworkChannel, @@ -241,7 +241,7 @@ pub fn publish_beacon_block( } /// HTTP Handler to produce a new Attestation from the current state, ready to be signed by a validator. -pub fn get_new_attestation( +pub fn get_new_attestation( req: Request, beacon_chain: Arc>, ) -> ApiResult { @@ -258,7 +258,7 @@ pub fn get_new_attestation( } /// HTTP Handler to publish an Attestation, which has been signed by a validator. -pub fn publish_attestation( +pub fn publish_attestation( req: Request, beacon_chain: Arc>, network_chan: NetworkChannel, diff --git a/beacon_node/rest_api/tests/test.rs b/beacon_node/rest_api/tests/test.rs index 3d029f0966..4163da328a 100644 --- a/beacon_node/rest_api/tests/test.rs +++ b/beacon_node/rest_api/tests/test.rs @@ -7,7 +7,7 @@ use node_test_rig::{ }; use remote_beacon_node::{PublishStatus, ValidatorDuty}; use std::sync::Arc; -use tree_hash::{SignedRoot, TreeHash}; +use tree_hash::TreeHash; use types::{ test_utils::generate_deterministic_keypair, BeaconBlock, ChainSpec, Domain, Epoch, EthSpec, MinimalEthSpec, PublicKey, RelativeEpoch, Signature, Slot, @@ -79,8 +79,6 @@ fn validator_produce_attestation() { .expect("should have attestation duties cache") .expect("should have attestation duties"); - dbg!(&duties); - let mut attestation = env .runtime() .block_on( @@ -150,7 +148,6 @@ fn validator_produce_attestation() { .publish_attestation(attestation.clone()), ) .expect("should publish attestation"); - dbg!(publish_status.clone()); assert!( publish_status.is_valid(), "the signed published attestation should be valid" diff --git a/book/src/setup.md b/book/src/setup.md index 8573a9a370..0edb0ac0e2 100644 --- a/book/src/setup.md +++ b/book/src/setup.md @@ -45,5 +45,5 @@ against the Ethereum Foundation specifications. These tests are quite large (100's of MB) so they're only downloaded if you run `$ 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 +downloading these tests if you're on a slow or metered Internet connection. CI will require them to pass, though. diff --git a/eth2/utils/slot_clock/src/testing_slot_clock.rs b/eth2/utils/slot_clock/src/testing_slot_clock.rs index 88eb2eec28..7eaee4a1b9 100644 --- a/eth2/utils/slot_clock/src/testing_slot_clock.rs +++ b/eth2/utils/slot_clock/src/testing_slot_clock.rs @@ -39,7 +39,7 @@ impl SlotClock for TestingSlotClock { /// Always returns a duration of `1 * slots_per_epoch` second. fn duration_to_next_epoch(&self, slots_per_epoch: u64) -> Option { - Some(Duration::from_secs(1 + slots_per_epoch)) + Some(Duration::from_secs(slots_per_epoch)) } /// Always returns a slot duration of 0 seconds. diff --git a/validator_client/src/duties_service.rs b/validator_client/src/duties_service.rs index 9480de66e8..de29180cd0 100644 --- a/validator_client/src/duties_service.rs +++ b/validator_client/src/duties_service.rs @@ -90,16 +90,8 @@ impl DutiesStore { return InsertOutcome::Invalid; } - if store.contains_key(&duties.validator_pubkey) { - let validator_map = store.get_mut(&duties.validator_pubkey).expect( - "Store is exclusively locked and this path is guarded to ensure the key exists.", - ); - - if validator_map.contains_key(&epoch) { - let known_duties = validator_map.get_mut(&epoch).expect( - "Validator map is exclusively mutable and this path is guarded to ensure the key exists.", - ); - + if let Some(validator_map) = store.get_mut(&duties.validator_pubkey) { + if let Some(known_duties) = validator_map.get_mut(&epoch) { if *known_duties == duties { InsertOutcome::Identical } else { diff --git a/validator_client/src/validator_directory.rs b/validator_client/src/validator_directory.rs index c2f3742dea..15d2edd9a2 100644 --- a/validator_client/src/validator_directory.rs +++ b/validator_client/src/validator_directory.rs @@ -202,11 +202,11 @@ impl ValidatorDirectoryBuilder { let voting_keypair = self .voting_keypair .clone() - .ok_or_else(|| "build requires a voting_keypair")?; + .ok_or_else(|| "write_keypair_files requires a voting_keypair")?; let withdrawal_keypair = self .withdrawal_keypair .clone() - .ok_or_else(|| "build requires a withdrawal_keypair")?; + .ok_or_else(|| "write_keypair_files requires a withdrawal_keypair")?; self.save_keypair(voting_keypair, VOTING_KEY_PREFIX)?; self.save_keypair(withdrawal_keypair, WITHDRAWAL_KEY_PREFIX)?;