From f9d60f543648d82b91fd02150cd567fbcd9b4cf9 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 26 Mar 2021 04:53:57 +0000 Subject: [PATCH] VC: accept unknown fields in chain spec (#2277) ## Issue Addressed Closes #2274 ## Proposed Changes * Modify the `YamlConfig` to collect unknown fields into an `extra_fields` map, instead of failing hard. * Log a debug message if there are extra fields returned to the VC from one of its BNs. This restores Lighthouse's compatibility with Teku beacon nodes (and therefore Infura) --- Makefile | 6 +++- account_manager/src/validator/exit.rs | 6 ++-- beacon_node/eth1/src/inner.rs | 2 +- beacon_node/eth2_libp2p/src/behaviour/mod.rs | 4 +-- beacon_node/eth2_libp2p/src/service.rs | 2 +- beacon_node/src/config.rs | 4 +-- beacon_node/store/src/hot_cold_store.rs | 2 +- common/deposit_contract/build.rs | 2 +- common/deposit_contract/src/lib.rs | 2 +- common/lighthouse_version/src/lib.rs | 6 +++- common/remote_signer_consumer/tests/post.rs | 2 +- common/validator_dir/src/validator_dir.rs | 10 +++--- common/validator_dir/tests/tests.rs | 2 +- consensus/serde_utils/src/hex.rs | 4 +-- consensus/ssz/src/decode/impls.rs | 2 +- consensus/ssz_types/src/fixed_vector.rs | 6 ++-- consensus/ssz_types/src/variable_list.rs | 6 ++-- consensus/types/src/chain_spec.rs | 36 +++++++++++++++++-- crypto/bls/src/generic_signature_set.rs | 16 ++++----- testing/ef_tests/tests/tests.rs | 6 ++++ testing/state_transition_vectors/src/main.rs | 6 ++-- validator_client/src/beacon_node_fallback.rs | 10 +++++- .../src/initialized_validators.rs | 14 +++----- 23 files changed, 102 insertions(+), 54 deletions(-) diff --git a/Makefile b/Makefile index 5d21625b6..d843e81cd 100644 --- a/Makefile +++ b/Makefile @@ -121,7 +121,11 @@ test-full: cargo-fmt test-release test-debug test-ef # Lints the code for bad style and potentially unsafe arithmetic using Clippy. # Clippy lints are opt-in per-crate for now. By default, everything is allowed except for performance and correctness lints. lint: - cargo clippy --all --tests -- -D warnings + cargo clippy --all --tests -- \ + -D warnings \ + -A clippy::from-over-into \ + -A clippy::upper-case-acronyms \ + -A clippy::vec-init-then-push # Runs the makefile in the `ef_tests` repo. # diff --git a/account_manager/src/validator/exit.rs b/account_manager/src/validator/exit.rs index 17eafc602..66fb21dd5 100644 --- a/account_manager/src/validator/exit.rs +++ b/account_manager/src/validator/exit.rs @@ -10,7 +10,7 @@ use eth2_keystore::Keystore; use eth2_network_config::Eth2NetworkConfig; use safe_arith::SafeArith; use slot_clock::{SlotClock, SystemTimeSlotClock}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::time::Duration; use tokio::time::sleep; use types::{ChainSpec, Epoch, EthSpec, Fork, VoluntaryExit}; @@ -91,7 +91,7 @@ pub fn cli_run(matches: &ArgMatches, env: Environment) -> Result< /// Gets the keypair and validator_index for every validator and calls `publish_voluntary_exit` on it. async fn publish_voluntary_exit( - keystore_path: &PathBuf, + keystore_path: &Path, password_file_path: Option<&PathBuf>, client: &BeaconNodeHttpClient, spec: &ChainSpec, @@ -310,7 +310,7 @@ fn get_current_epoch(genesis_time: u64, spec: &ChainSpec) -> Option< /// If the `password_file_path` is Some, unlock keystore using password in given file /// otherwise, prompts user for a password to unlock the keystore. fn load_voting_keypair( - voting_keystore_path: &PathBuf, + voting_keystore_path: &Path, password_file_path: Option<&PathBuf>, stdin_inputs: bool, ) -> Result { diff --git a/beacon_node/eth1/src/inner.rs b/beacon_node/eth1/src/inner.rs index 718080a1e..f6c5e85ef 100644 --- a/beacon_node/eth1/src/inner.rs +++ b/beacon_node/eth1/src/inner.rs @@ -53,7 +53,7 @@ impl Inner { pub fn from_bytes(bytes: &[u8], config: Config, spec: ChainSpec) -> Result { let ssz_cache = SszEth1Cache::from_ssz_bytes(bytes) .map_err(|e| format!("Ssz decoding error: {:?}", e))?; - Ok(ssz_cache.to_inner(config, spec)?) + ssz_cache.to_inner(config, spec) } /// Returns a reference to the specification. diff --git a/beacon_node/eth2_libp2p/src/behaviour/mod.rs b/beacon_node/eth2_libp2p/src/behaviour/mod.rs index d998389a8..02468285a 100644 --- a/beacon_node/eth2_libp2p/src/behaviour/mod.rs +++ b/beacon_node/eth2_libp2p/src/behaviour/mod.rs @@ -36,7 +36,7 @@ use ssz::Encode; use std::collections::HashSet; use std::fs::File; use std::io::Write; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::{ collections::VecDeque, marker::PhantomData, @@ -1336,7 +1336,7 @@ impl std::convert::From> for RPCCodedResponse(dir: &PathBuf, metadata: MetaData, log: &slog::Logger) { +pub fn save_metadata_to_disk(dir: &Path, metadata: MetaData, log: &slog::Logger) { let _ = std::fs::create_dir_all(&dir); match File::create(dir.join(METADATA_FILENAME)) .and_then(|mut f| f.write_all(&metadata.as_ssz_bytes())) diff --git a/beacon_node/eth2_libp2p/src/service.rs b/beacon_node/eth2_libp2p/src/service.rs index e49344919..079338b54 100644 --- a/beacon_node/eth2_libp2p/src/service.rs +++ b/beacon_node/eth2_libp2p/src/service.rs @@ -474,7 +474,7 @@ fn strip_peer_id(addr: &mut Multiaddr) { /// Load metadata from persisted file. Return default metadata if loading fails. fn load_or_build_metadata( - network_dir: &std::path::PathBuf, + network_dir: &std::path::Path, log: &slog::Logger, ) -> MetaData { // Default metadata diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index d22811e39..256f7e04f 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -10,7 +10,7 @@ use std::cmp::max; use std::fs; use std::net::{IpAddr, Ipv4Addr, ToSocketAddrs}; use std::net::{TcpListener, UdpSocket}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::str::FromStr; use types::{ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, PublicKeyBytes, GRAFFITI_BYTES_LEN}; @@ -422,7 +422,7 @@ pub fn get_config( pub fn set_network_config( config: &mut NetworkConfig, cli_args: &ArgMatches, - data_dir: &PathBuf, + data_dir: &Path, log: &Logger, use_listening_port_as_enr_port_by_default: bool, ) -> Result<(), String> { diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index ea456acb9..4105c393f 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -660,7 +660,7 @@ impl, Cold: ItemStore> HotColdDB partial_state.load_historical_roots(&self.cold_db, &self.spec)?; partial_state.load_randao_mixes(&self.cold_db, &self.spec)?; - Ok(partial_state.try_into()?) + partial_state.try_into() } /// Load a restore point state by its `restore_point_index`. diff --git a/common/deposit_contract/build.rs b/common/deposit_contract/build.rs index 719a5a05f..ded9e9b78 100644 --- a/common/deposit_contract/build.rs +++ b/common/deposit_contract/build.rs @@ -35,7 +35,7 @@ fn testnet_url() -> String { fn main() { match get_all_contracts() { Ok(()) => (), - Err(e) => panic!(e), + Err(e) => panic!("{}", e), } } diff --git a/common/deposit_contract/src/lib.rs b/common/deposit_contract/src/lib.rs index e57f22dc0..92ccee3be 100644 --- a/common/deposit_contract/src/lib.rs +++ b/common/deposit_contract/src/lib.rs @@ -68,7 +68,7 @@ pub fn decode_eth1_tx_data( ) .map_err(DecodeError::SszDecodeError)? }; - }; + } let root = decode_token!(Hash256, to_fixed_bytes); diff --git a/common/lighthouse_version/src/lib.rs b/common/lighthouse_version/src/lib.rs index 76f9cb411..233305d3c 100644 --- a/common/lighthouse_version/src/lib.rs +++ b/common/lighthouse_version/src/lib.rs @@ -38,6 +38,10 @@ mod test { fn version_formatting() { let re = Regex::new(r"^Lighthouse/v[0-9]+\.[0-9]+\.[0-9]+(-rc.[0-9])?-[[:xdigit:]]{7}\+?$") .unwrap(); - assert!(re.is_match(VERSION), VERSION); + assert!( + re.is_match(VERSION), + "version doesn't match regex: {}", + VERSION + ); } } diff --git a/common/remote_signer_consumer/tests/post.rs b/common/remote_signer_consumer/tests/post.rs index 7448ed130..68a45aac3 100644 --- a/common/remote_signer_consumer/tests/post.rs +++ b/common/remote_signer_consumer/tests/post.rs @@ -149,7 +149,7 @@ mod post { let r = run_testcase(u).unwrap_err(); for msg in msgs.iter() { - assert!(r.contains(msg), format!("{:?} should contain {:?}", r, msg)); + assert!(r.contains(msg), "{:?} should contain {:?}", r, msg); } }; diff --git a/common/validator_dir/src/validator_dir.rs b/common/validator_dir/src/validator_dir.rs index 5706b6630..bfa3e2553 100644 --- a/common/validator_dir/src/validator_dir.rs +++ b/common/validator_dir/src/validator_dir.rs @@ -89,7 +89,7 @@ impl ValidatorDir { } /// Returns the `dir` provided to `Self::open`. - pub fn dir(&self) -> &PathBuf { + pub fn dir(&self) -> &Path { &self.dir } @@ -204,7 +204,7 @@ impl ValidatorDir { /// Attempts to load and decrypt a Keypair given path to the keystore. pub fn unlock_keypair>( - keystore_path: &PathBuf, + keystore_path: &Path, password_dir: P, ) -> Result { let keystore = Keystore::from_json_reader( @@ -229,8 +229,8 @@ pub fn unlock_keypair>( /// Attempts to load and decrypt a Keypair given path to the keystore and the password file. pub fn unlock_keypair_from_password_path( - keystore_path: &PathBuf, - password_path: &PathBuf, + keystore_path: &Path, + password_path: &Path, ) -> Result { let keystore = Keystore::from_json_reader( &mut OpenOptions::new() @@ -242,7 +242,7 @@ pub fn unlock_keypair_from_password_path( .map_err(Error::UnableToReadKeystore)?; let password: PlainText = read(password_path) - .map_err(|_| Error::UnableToReadPassword(password_path.clone()))? + .map_err(|_| Error::UnableToReadPassword(password_path.into()))? .into(); keystore .decrypt_keypair(password.as_bytes()) diff --git a/common/validator_dir/tests/tests.rs b/common/validator_dir/tests/tests.rs index 3a54c22f5..a782d81bb 100644 --- a/common/validator_dir/tests/tests.rs +++ b/common/validator_dir/tests/tests.rs @@ -182,7 +182,7 @@ fn concurrency() { let harness = Harness::new(); let val_dir = harness.create_and_test(&BuildConfig::default()); - let path = val_dir.dir().clone(); + let path = val_dir.dir().to_owned(); // Should not re-open whilst opened after build. ValidatorDir::open(&path).unwrap_err(); diff --git a/consensus/serde_utils/src/hex.rs b/consensus/serde_utils/src/hex.rs index 7ffa347e5..647b0ecfb 100644 --- a/consensus/serde_utils/src/hex.rs +++ b/consensus/serde_utils/src/hex.rs @@ -55,8 +55,8 @@ impl<'de> Visitor<'de> for HexVisitor { where E: de::Error, { - Ok(hex::decode(value.trim_start_matches("0x")) - .map_err(|e| de::Error::custom(format!("invalid hex ({:?})", e)))?) + hex::decode(value.trim_start_matches("0x")) + .map_err(|e| de::Error::custom(format!("invalid hex ({:?})", e))) } } diff --git a/consensus/ssz/src/decode/impls.rs b/consensus/ssz/src/decode/impls.rs index 29c8c1550..f074cd341 100644 --- a/consensus/ssz/src/decode/impls.rs +++ b/consensus/ssz/src/decode/impls.rs @@ -353,7 +353,7 @@ macro_rules! impl_decodable_for_u8_array { Err(DecodeError::InvalidByteLength { len, expected }) } else { let mut array: [u8; $len] = [0; $len]; - array.copy_from_slice(&bytes[..]); + array.copy_from_slice(bytes); Ok(array) } diff --git a/consensus/ssz_types/src/fixed_vector.rs b/consensus/ssz_types/src/fixed_vector.rs index 9ae0d06ba..0d7664c1d 100644 --- a/consensus/ssz_types/src/fixed_vector.rs +++ b/consensus/ssz_types/src/fixed_vector.rs @@ -108,9 +108,9 @@ impl From> for FixedVector { } } -impl Into> for FixedVector { - fn into(self) -> Vec { - self.vec +impl From> for Vec { + fn from(vector: FixedVector) -> Vec { + vector.vec } } diff --git a/consensus/ssz_types/src/variable_list.rs b/consensus/ssz_types/src/variable_list.rs index 4cbef6743..6c58ac690 100644 --- a/consensus/ssz_types/src/variable_list.rs +++ b/consensus/ssz_types/src/variable_list.rs @@ -120,9 +120,9 @@ impl From> for VariableList { } } -impl Into> for VariableList { - fn into(self) -> Vec { - self.vec +impl From> for Vec { + fn from(list: VariableList) -> Vec { + list.vec } } diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 1778d50fb..b6cb5142f 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -1,6 +1,7 @@ use crate::*; use int_to_bytes::int_to_bytes4; use serde_derive::{Deserialize, Serialize}; +use std::collections::HashMap; use std::fs::File; use std::path::Path; use tree_hash::TreeHash; @@ -449,10 +450,8 @@ mod tests { } /// YAML config file as defined by the spec. -/// -/// Spec v0.12.3 #[derive(Serialize, Deserialize, Debug, PartialEq, Clone)] -#[serde(rename_all = "UPPERCASE", deny_unknown_fields)] +#[serde(rename_all = "UPPERCASE")] pub struct YamlConfig { pub config_name: String, // ChainSpec @@ -576,6 +575,10 @@ pub struct YamlConfig { #[serde(with = "serde_utils::quoted_u64")] deposit_network_id: u64, deposit_contract_address: Address, + + // Extra fields (could be from a future hard-fork that we don't yet know). + #[serde(flatten)] + pub extra_fields: HashMap, } impl Default for YamlConfig { @@ -671,6 +674,8 @@ impl YamlConfig { deposit_chain_id: spec.deposit_chain_id, deposit_network_id: spec.deposit_network_id, deposit_contract_address: spec.deposit_contract_address, + + extra_fields: HashMap::new(), } } @@ -849,6 +854,31 @@ mod yaml_tests { assert_eq!(from, yamlconfig); } + #[test] + fn extra_fields_round_trip() { + let tmp_file = NamedTempFile::new().expect("failed to create temp file"); + let writer = OpenOptions::new() + .read(false) + .write(true) + .open(tmp_file.as_ref()) + .expect("error opening file"); + let mainnet_spec = ChainSpec::mainnet(); + let mut yamlconfig = YamlConfig::from_spec::(&mainnet_spec); + let (k1, v1) = ("SAMPLE_HARDFORK_KEY1", "123456789"); + let (k2, v2) = ("SAMPLE_HARDFORK_KEY2", "987654321"); + yamlconfig.extra_fields.insert(k1.into(), v1.into()); + yamlconfig.extra_fields.insert(k2.into(), v2.into()); + serde_yaml::to_writer(writer, &yamlconfig).expect("failed to write or serialize"); + + let reader = OpenOptions::new() + .read(true) + .write(false) + .open(tmp_file.as_ref()) + .expect("error while opening the file"); + let from: YamlConfig = serde_yaml::from_reader(reader).expect("error while deserializing"); + assert_eq!(from, yamlconfig); + } + #[test] fn apply_to_spec() { let mut spec = ChainSpec::minimal(); diff --git a/crypto/bls/src/generic_signature_set.rs b/crypto/bls/src/generic_signature_set.rs index 285667898..16f845fc4 100644 --- a/crypto/bls/src/generic_signature_set.rs +++ b/crypto/bls/src/generic_signature_set.rs @@ -19,35 +19,35 @@ where aggregate: Cow<'a, GenericAggregateSignature>, } -impl<'a, Pub, AggPub, Sig, AggSig> Into> - for &'a GenericSignature +impl<'a, Pub, AggPub, Sig, AggSig> From<&'a GenericSignature> + for WrappedSignature<'a, Pub, AggPub, Sig, AggSig> where Pub: TPublicKey + Clone, AggPub: Clone, Sig: TSignature + Clone, AggSig: TAggregateSignature + Clone, { - fn into(self) -> WrappedSignature<'a, Pub, AggPub, Sig, AggSig> { + fn from(sig: &'a GenericSignature) -> Self { let mut aggregate: GenericAggregateSignature = GenericAggregateSignature::infinity(); - aggregate.add_assign(self); + aggregate.add_assign(sig); WrappedSignature { aggregate: Cow::Owned(aggregate), } } } -impl<'a, Pub, AggPub, Sig, AggSig> Into> - for &'a GenericAggregateSignature +impl<'a, Pub, AggPub, Sig, AggSig> From<&'a GenericAggregateSignature> + for WrappedSignature<'a, Pub, AggPub, Sig, AggSig> where Pub: TPublicKey + Clone, AggPub: Clone, Sig: Clone, AggSig: Clone, { - fn into(self) -> WrappedSignature<'a, Pub, AggPub, Sig, AggSig> { + fn from(aggregate: &'a GenericAggregateSignature) -> Self { WrappedSignature { - aggregate: Cow::Borrowed(self), + aggregate: Cow::Borrowed(aggregate), } } } diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 72bede138..c1a50ffc4 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -1,6 +1,7 @@ #![cfg(feature = "ef_tests")] use ef_tests::*; +use std::collections::HashMap; use std::path::PathBuf; use types::*; @@ -17,6 +18,11 @@ fn config_test() { let yaml_from_spec = YamlConfig::from_spec::(&spec); assert_eq!(yaml_config.apply_to_chain_spec::(&spec), Some(spec)); assert_eq!(yaml_from_spec, yaml_config); + assert_eq!( + yaml_config.extra_fields, + HashMap::new(), + "not all config fields read" + ); } #[test] diff --git a/testing/state_transition_vectors/src/main.rs b/testing/state_transition_vectors/src/main.rs index e259399a7..4f8426bae 100644 --- a/testing/state_transition_vectors/src/main.rs +++ b/testing/state_transition_vectors/src/main.rs @@ -7,7 +7,7 @@ use state_processing::test_utils::BlockProcessingBuilder; use std::env; use std::fs::{self, File}; use std::io::Write; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::exit; use types::MainnetEthSpec; use types::{BeaconState, ChainSpec, EthSpec, SignedBeaconBlock}; @@ -91,12 +91,12 @@ fn write_vectors_to_file(title: &str, vectors: &[TestVector]) -> Result<(), Stri } /// Write some SSZ object to file. -fn write_to_ssz_file(path: &PathBuf, item: &T) -> Result<(), String> { +fn write_to_ssz_file(path: &Path, item: &T) -> Result<(), String> { write_to_file(path, &item.as_ssz_bytes()) } /// Write some bytes to file. -fn write_to_file(path: &PathBuf, item: &[u8]) -> Result<(), String> { +fn write_to_file(path: &Path, item: &[u8]) -> Result<(), String> { File::create(path) .map_err(|e| format!("Unable to create {:?}: {:?}", path, e)) .and_then(|mut file| { diff --git a/validator_client/src/beacon_node_fallback.rs b/validator_client/src/beacon_node_fallback.rs index 65d8ae6d6..78def569e 100644 --- a/validator_client/src/beacon_node_fallback.rs +++ b/validator_client/src/beacon_node_fallback.rs @@ -7,7 +7,7 @@ use crate::http_metrics::metrics::{inc_counter_vec, ENDPOINT_ERRORS, ENDPOINT_RE use environment::RuntimeContext; use eth2::BeaconNodeHttpClient; use futures::future; -use slog::{error, info, warn, Logger}; +use slog::{debug, error, info, warn, Logger}; use slot_clock::SlotClock; use std::fmt; use std::fmt::Debug; @@ -236,6 +236,14 @@ impl CandidateBeaconNode { CandidateError::Incompatible })?; + if !yaml_config.extra_fields.is_empty() { + debug!( + log, + "Beacon spec includes unknown fields"; + "fields" => ?yaml_config.extra_fields + ); + } + if *spec == beacon_node_spec { Ok(()) } else { diff --git a/validator_client/src/initialized_validators.rs b/validator_client/src/initialized_validators.rs index 1475cd995..f89a1096e 100644 --- a/validator_client/src/initialized_validators.rs +++ b/validator_client/src/initialized_validators.rs @@ -19,7 +19,7 @@ use slog::{debug, error, info, warn, Logger}; use std::collections::{HashMap, HashSet}; use std::fs::File; use std::io; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use types::{Graffiti, Keypair, PublicKey, PublicKeyBytes}; use crate::key_cache; @@ -101,20 +101,16 @@ impl InitializedValidator { } } -fn open_keystore(path: &PathBuf) -> Result { +fn open_keystore(path: &Path) -> Result { let keystore_file = File::open(path).map_err(Error::UnableToOpenVotingKeystore)?; Keystore::from_json_reader(keystore_file).map_err(Error::UnableToParseVotingKeystore) } -fn get_lockfile_path(file_path: &PathBuf) -> Option { +fn get_lockfile_path(file_path: &Path) -> Option { file_path .file_name() .and_then(|os_str| os_str.to_str()) - .map(|filename| { - file_path - .clone() - .with_file_name(format!("{}.lock", filename)) - }) + .map(|filename| file_path.with_file_name(format!("{}.lock", filename))) } impl InitializedValidator { @@ -238,7 +234,7 @@ impl InitializedValidator { /// Try to unlock `keystore` at `keystore_path` by prompting the user via `stdin`. fn unlock_keystore_via_stdin_password( keystore: &Keystore, - keystore_path: &PathBuf, + keystore_path: &Path, ) -> Result<(ZeroizeString, Keypair), Error> { eprintln!(); eprintln!(