From 9b78a0bdac3e709226bd9fce0a53117b8e7e2fb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Mon, 22 Aug 2022 08:55:26 +0200 Subject: [PATCH] pos: update validator raw hash validation --- Cargo.lock | 1 + proof_of_stake/Cargo.toml | 1 + proof_of_stake/src/types.rs | 6 +++ proof_of_stake/src/validation.rs | 75 ++++++++++++++++----------- shared/src/ledger/pos/vp.rs | 11 ---- shared/src/types/key/common.rs | 12 ++++- shared/src/types/key/mod.rs | 6 +++ tests/src/native_vp/pos.rs | 71 ++++++++++++++++++------- wasm/tx_template/Cargo.lock | 1 + wasm/vp_template/Cargo.lock | 1 + wasm/wasm_source/Cargo.lock | 1 + wasm_for_tests/wasm_source/Cargo.lock | 1 + 12 files changed, 125 insertions(+), 62 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f9c14142721..f93cae918e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3994,6 +3994,7 @@ name = "namada_proof_of_stake" version = "0.7.0" dependencies = [ "borsh", + "derivative", "proptest", "thiserror", ] diff --git a/proof_of_stake/Cargo.toml b/proof_of_stake/Cargo.toml index 449df509d06..ca7ecc5ca2e 100644 --- a/proof_of_stake/Cargo.toml +++ b/proof_of_stake/Cargo.toml @@ -18,5 +18,6 @@ borsh = "0.9.1" thiserror = "1.0.30" # A fork with state machine testing proptest = {git = "https://github.com/heliaxdev/proptest", branch = "tomas/sm", optional = true} +derivative = "2.2.0" [dev-dependencies] diff --git a/proof_of_stake/src/types.rs b/proof_of_stake/src/types.rs index 45342ef2773..f7945b45251 100644 --- a/proof_of_stake/src/types.rs +++ b/proof_of_stake/src/types.rs @@ -354,6 +354,12 @@ pub enum SlashType { )] pub struct BasisPoints(u64); +/// Derive Tendermint raw hash from the public key +pub trait PublicKeyTmRawHash { + /// Derive Tendermint raw hash from the public key + fn tm_raw_hash(&self) -> String; +} + impl VotingPower { /// Convert token amount into a voting power. pub fn from_tokens(tokens: impl Into, params: &PosParams) -> Self { diff --git a/proof_of_stake/src/validation.rs b/proof_of_stake/src/validation.rs index 41485bcbb02..c2f5ee60625 100644 --- a/proof_of_stake/src/validation.rs +++ b/proof_of_stake/src/validation.rs @@ -8,21 +8,22 @@ use std::hash::Hash; use std::ops::{Add, AddAssign, Neg, Sub, SubAssign}; use borsh::{BorshDeserialize, BorshSchema, BorshSerialize}; +use derivative::Derivative; use thiserror::Error; use crate::btree_set::BTreeSetShims; use crate::epoched::DynEpochOffset; use crate::parameters::PosParams; use crate::types::{ - BondId, Bonds, Epoch, Slashes, TotalVotingPowers, Unbonds, - ValidatorConsensusKeys, ValidatorSets, ValidatorState, ValidatorStates, - ValidatorTotalDeltas, ValidatorVotingPowers, VotingPower, VotingPowerDelta, - WeightedValidator, + BondId, Bonds, Epoch, PublicKeyTmRawHash, Slashes, TotalVotingPowers, + Unbonds, ValidatorConsensusKeys, ValidatorSets, ValidatorState, + ValidatorStates, ValidatorTotalDeltas, ValidatorVotingPowers, VotingPower, + VotingPowerDelta, WeightedValidator, }; #[allow(missing_docs)] #[derive(Error, Debug)] -pub enum Error +pub enum Error where Address: Display + Debug @@ -36,6 +37,7 @@ where + BorshSchema + BorshDeserialize, TokenChange: Debug + Display, + PublicKey: Debug, { #[error("Unexpectedly missing state value for validator {0}")] ValidatorStateIsRequired(Address), @@ -158,7 +160,7 @@ where #[error("Invalid address raw hash update")] InvalidRawHashUpdate, #[error("Invalid new validator {0}, some fields are missing: {1:?}.")] - InvalidNewValidator(Address, NewValidator), + InvalidNewValidator(Address, NewValidator), #[error("New validator {0} has not been added to the validator set.")] NewValidatorMissingInValidatorSet(Address), #[error("Validator set has not been updated for new validators.")] @@ -241,8 +243,8 @@ where ValidatorAddressRawHash { /// Raw hash value raw_hash: String, - /// The address and raw hash derived from it - data: Data<(Address, String)>, + /// The validator's address + data: Data
, }, } @@ -291,14 +293,16 @@ where /// A new validator account initialized in a transaction, which is used to check /// that all the validator's required fields have been written. -#[derive(Clone, Debug, Default)] -pub struct NewValidator { +#[derive(Clone, Debug, Derivative)] +// https://mcarton.github.io/rust-derivative/latest/Default.html#custom-bound +#[derivative(Default(bound = ""))] +pub struct NewValidator { has_state: bool, - has_consensus_key: bool, + has_consensus_key: Option, has_total_deltas: bool, has_voting_power: bool, has_staking_reward_address: bool, - has_address_raw_hash: bool, + has_address_raw_hash: Option, voting_power: VotingPower, } @@ -309,7 +313,7 @@ pub fn validate( params: &PosParams, changes: Vec>, current_epoch: impl Into, -) -> Vec> +) -> Vec> where Address: Display + Debug @@ -362,7 +366,8 @@ where + BorshDeserialize + BorshSerialize + BorshSchema - + PartialEq, + + PartialEq + + PublicKeyTmRawHash, { let current_epoch = current_epoch.into(); use DataUpdate::*; @@ -408,7 +413,7 @@ where VotingPowerDelta, > = HashMap::default(); - let mut new_validators: HashMap = HashMap::default(); + let mut new_validators: HashMap> = HashMap::default(); for change in changes { match change { @@ -493,16 +498,19 @@ where } // The value must be known at pipeline epoch match post.get(pipeline_epoch) { - Some(_) => {} + Some(consensus_key) => { + let validator = new_validators + .entry(address.clone()) + .or_default(); + validator.has_consensus_key = + Some(consensus_key.clone()); + } _ => errors.push( Error::MissingNewValidatorConsensusKey( pipeline_epoch.into(), ), ), } - let validator = - new_validators.entry(address.clone()).or_default(); - validator.has_consensus_key = true; } (Some(pre), Some(post)) => { if post.last_update() != current_epoch { @@ -1231,16 +1239,10 @@ where }, ValidatorAddressRawHash { raw_hash, data } => { match (data.pre, data.post) { - (None, Some((address, expected_raw_hash))) => { - if raw_hash != expected_raw_hash { - errors.push(Error::InvalidAddressRawHash( - raw_hash, - expected_raw_hash, - )) - } + (None, Some(address)) => { let validator = new_validators.entry(address.clone()).or_default(); - validator.has_address_raw_hash = true; + validator.has_address_raw_hash = Some(raw_hash); } (pre, post) if pre != post => { errors.push(Error::InvalidRawHashUpdate) @@ -1641,17 +1643,30 @@ where } = &new_validator; // The new validator must have set all the required fields if !(*has_state - && *has_consensus_key && *has_total_deltas && *has_voting_power - && *has_staking_reward_address - && *has_address_raw_hash) + && *has_staking_reward_address) { errors.push(Error::InvalidNewValidator( address.clone(), new_validator.clone(), )) } + match (has_address_raw_hash, has_consensus_key) { + (Some(raw_hash), Some(consensus_key)) => { + let expected_raw_hash = consensus_key.tm_raw_hash(); + if raw_hash != &expected_raw_hash { + errors.push(Error::InvalidAddressRawHash( + raw_hash.clone(), + expected_raw_hash, + )) + } + } + _ => errors.push(Error::InvalidNewValidator( + address.clone(), + new_validator.clone(), + )), + } let weighted_validator = WeightedValidator { voting_power: *voting_power, address: address.clone(), diff --git a/shared/src/ledger/pos/vp.rs b/shared/src/ledger/pos/vp.rs index 26be4405367..f09e421e435 100644 --- a/shared/src/ledger/pos/vp.rs +++ b/shared/src/ledger/pos/vp.rs @@ -215,17 +215,6 @@ where .ctx .read_post(key)? .and_then(|bytes| Address::try_from_slice(&bytes[..]).ok()); - // Find the raw hashes of the addresses - let pre = pre.map(|pre| { - let raw_hash = - pre.raw_hash().map(String::from).unwrap_or_default(); - (pre, raw_hash) - }); - let post = post.map(|post| { - let raw_hash = - post.raw_hash().map(String::from).unwrap_or_default(); - (post, raw_hash) - }); changes.push(ValidatorAddressRawHash { raw_hash: raw_hash.to_string(), data: Data { pre, post }, diff --git a/shared/src/types/key/common.rs b/shared/src/types/key/common.rs index 27e7c29b891..d3401258d19 100644 --- a/shared/src/types/key/common.rs +++ b/shared/src/types/key/common.rs @@ -4,13 +4,15 @@ use std::fmt::Display; use std::str::FromStr; use borsh::{BorshDeserialize, BorshSchema, BorshSerialize}; +use namada_proof_of_stake::types::PublicKeyTmRawHash; #[cfg(feature = "rand")] use rand::{CryptoRng, RngCore}; use serde::{Deserialize, Serialize}; use super::{ - ed25519, ParsePublicKeyError, ParseSecretKeyError, ParseSignatureError, - RefTo, SchemeType, SigScheme as SigSchemeTrait, VerifySigError, + ed25519, tm_consensus_key_raw_hash, ParsePublicKeyError, + ParseSecretKeyError, ParseSignatureError, RefTo, SchemeType, + SigScheme as SigSchemeTrait, VerifySigError, }; /// Public key @@ -275,3 +277,9 @@ impl super::SigScheme for SigScheme { } } } + +impl PublicKeyTmRawHash for PublicKey { + fn tm_raw_hash(&self) -> String { + tm_consensus_key_raw_hash(self) + } +} diff --git a/shared/src/types/key/mod.rs b/shared/src/types/key/mod.rs index 59dd6b2cd5a..9b560a4c84c 100644 --- a/shared/src/types/key/mod.rs +++ b/shared/src/types/key/mod.rs @@ -395,6 +395,12 @@ pub mod testing { }) } + /// Generate an arbitrary [`common::SecretKey`]. + pub fn arb_common_keypair() -> impl Strategy { + arb_keypair::() + .prop_map(|keypair| keypair.try_to_sk().unwrap()) + } + /// Generate a new random [`super::SecretKey`]. pub fn gen_keypair() -> S::SecretKey { let mut rng: ThreadRng = thread_rng(); diff --git a/tests/src/native_vp/pos.rs b/tests/src/native_vp/pos.rs index 83878f69653..1779be7e21f 100644 --- a/tests/src/native_vp/pos.rs +++ b/tests/src/native_vp/pos.rs @@ -104,6 +104,7 @@ mod tests { use namada::ledger::pos::namada_proof_of_stake::PosBase; use namada::ledger::pos::PosParams; + use namada::types::key::common::PublicKey; use namada::types::storage::Epoch; use namada::types::token; use namada_vm_env::proof_of_stake::parameters::testing::arb_pos_params; @@ -163,6 +164,7 @@ mod tests { } /// State machine transitions + #[allow(clippy::large_enum_variant)] #[derive(Clone, Debug)] enum Transition { /// Commit all the tx changes already applied in the tx env @@ -379,8 +381,12 @@ mod tests { Transition::CommitTx => true, Transition::NextEpoch => true, Transition::Valid(action) => match action { - ValidPosAction::InitValidator(address) => { + ValidPosAction::InitValidator { + address, + consensus_key, + } => { !state.is_validator(address) + && !state.is_used_key(consensus_key) } ValidPosAction::Bond { amount: _, @@ -457,7 +463,19 @@ mod tests { /// Find if the given address is a validator fn is_validator(&self, addr: &Address) -> bool { self.all_valid_actions().iter().any(|action| match action { - ValidPosAction::InitValidator(validator) => validator == addr, + ValidPosAction::InitValidator { address, .. } => { + address == addr + } + _ => false, + }) + } + + /// Find if the given consensus key is already used by any validators + fn is_used_key(&self, given_consensus_key: &PublicKey) -> bool { + self.all_valid_actions().iter().any(|action| match action { + ValidPosAction::InitValidator { consensus_key, .. } => { + consensus_key == given_consensus_key + } _ => false, }) } @@ -568,7 +586,10 @@ pub mod testing { #[derive(Clone, Debug)] pub enum ValidPosAction { - InitValidator(Address), + InitValidator { + address: Address, + consensus_key: PublicKey, + }, Bond { amount: token::Amount, owner: Address, @@ -666,13 +687,21 @@ pub mod testing { let validators: Vec
= valid_actions .iter() .filter_map(|action| match action { - ValidPosAction::InitValidator(addr) => Some(addr.clone()), + ValidPosAction::InitValidator { address, .. } => { + Some(address.clone()) + } _ => None, }) .collect(); - let init_validator = address::testing::arb_established_address() - .prop_map(|addr| { - ValidPosAction::InitValidator(Address::Established(addr)) + let init_validator = ( + address::testing::arb_established_address(), + key::testing::arb_common_keypair(), + ) + .prop_map(|(addr, consensus_key)| { + ValidPosAction::InitValidator { + address: Address::Established(addr), + consensus_key: consensus_key.ref_to(), + } }); if validators.is_empty() { @@ -816,45 +845,47 @@ pub mod testing { use namada_vm_env::tx_prelude::PosRead; match self { - ValidPosAction::InitValidator(addr) => { + ValidPosAction::InitValidator { + address, + consensus_key, + } => { let offset = DynEpochOffset::PipelineLen; - let consensus_key = key::testing::keypair_1().ref_to(); vec![ PosStorageChange::SpawnAccount { - address: addr.clone(), + address: address.clone(), }, PosStorageChange::ValidatorAddressRawHash { - address: addr.clone(), + address: address.clone(), consensus_key: consensus_key.clone(), }, PosStorageChange::ValidatorSet { - validator: addr.clone(), + validator: address.clone(), token_delta: 0, offset, }, PosStorageChange::ValidatorConsensusKey { - validator: addr.clone(), + validator: address.clone(), pk: consensus_key, }, PosStorageChange::ValidatorStakingRewardsAddress { - validator: addr.clone(), + validator: address.clone(), address: address::testing::established_address_1(), }, PosStorageChange::ValidatorState { - validator: addr.clone(), + validator: address.clone(), state: ValidatorState::Pending, }, PosStorageChange::ValidatorState { - validator: addr.clone(), + validator: address.clone(), state: ValidatorState::Candidate, }, PosStorageChange::ValidatorTotalDeltas { - validator: addr.clone(), + validator: address.clone(), delta: 0, offset, }, PosStorageChange::ValidatorVotingPower { - validator: addr, + validator: address, vp_delta: 0, offset: Either::Left(offset), }, @@ -1577,7 +1608,9 @@ pub mod testing { let validators: Vec
= valid_actions .iter() .filter_map(|action| match action { - ValidPosAction::InitValidator(addr) => Some(addr.clone()), + ValidPosAction::InitValidator { address, .. } => { + Some(address.clone()) + } _ => None, }) .collect(); diff --git a/wasm/tx_template/Cargo.lock b/wasm/tx_template/Cargo.lock index bf9d222920e..dbab33357af 100644 --- a/wasm/tx_template/Cargo.lock +++ b/wasm/tx_template/Cargo.lock @@ -1409,6 +1409,7 @@ name = "namada_proof_of_stake" version = "0.7.0" dependencies = [ "borsh", + "derivative", "proptest", "thiserror", ] diff --git a/wasm/vp_template/Cargo.lock b/wasm/vp_template/Cargo.lock index 98f6c7f71c1..d363d8940bf 100644 --- a/wasm/vp_template/Cargo.lock +++ b/wasm/vp_template/Cargo.lock @@ -1409,6 +1409,7 @@ name = "namada_proof_of_stake" version = "0.7.0" dependencies = [ "borsh", + "derivative", "proptest", "thiserror", ] diff --git a/wasm/wasm_source/Cargo.lock b/wasm/wasm_source/Cargo.lock index b7185cc1100..b57c7ea6815 100644 --- a/wasm/wasm_source/Cargo.lock +++ b/wasm/wasm_source/Cargo.lock @@ -1409,6 +1409,7 @@ name = "namada_proof_of_stake" version = "0.7.0" dependencies = [ "borsh", + "derivative", "proptest", "thiserror", ] diff --git a/wasm_for_tests/wasm_source/Cargo.lock b/wasm_for_tests/wasm_source/Cargo.lock index 39f70c17871..ef4d313e75b 100644 --- a/wasm_for_tests/wasm_source/Cargo.lock +++ b/wasm_for_tests/wasm_source/Cargo.lock @@ -1420,6 +1420,7 @@ name = "namada_proof_of_stake" version = "0.7.0" dependencies = [ "borsh", + "derivative", "proptest", "thiserror", ]