diff --git a/proof_of_stake/src/storage.rs b/proof_of_stake/src/storage.rs index da10c156547..2782934e0dd 100644 --- a/proof_of_stake/src/storage.rs +++ b/proof_of_stake/src/storage.rs @@ -191,19 +191,28 @@ pub fn validator_commission_rate_key(validator: &Address) -> Key { .expect("Cannot obtain a storage key") } -/// Is storage key for validator's commissionr ate? -pub fn is_validator_commission_rate_key(key: &Key) -> Option<&Address> { +/// Is storage key for validator's commission rate? +pub fn is_validator_commission_rate_key( + key: &Key, +) -> Option<(&Address, Epoch)> { match &key.segments[..] { [ DbKeySeg::AddressSeg(addr), DbKeySeg::StringSeg(prefix), DbKeySeg::AddressSeg(validator), DbKeySeg::StringSeg(key), + DbKeySeg::StringSeg(lazy_map), + DbKeySeg::StringSeg(data), + DbKeySeg::StringSeg(epoch), ] if addr == &ADDRESS && prefix == VALIDATOR_STORAGE_PREFIX - && key == VALIDATOR_COMMISSION_RATE_STORAGE_KEY => + && key == VALIDATOR_COMMISSION_RATE_STORAGE_KEY + && lazy_map == LAZY_MAP_SUB_KEY + && data == lazy_map::DATA_SUBKEY => { - Some(validator) + let epoch = Epoch::parse(epoch.clone()) + .expect("Should be able to parse the epoch"); + Some((validator, epoch)) } _ => None, } @@ -236,6 +245,27 @@ pub fn is_validator_max_commission_rate_change_key( } } +/// Is storage key for some piece of validator metadata? +pub fn is_validator_metadata_key(key: &Key) -> Option<&Address> { + match &key.segments[..] { + [ + DbKeySeg::AddressSeg(addr), + DbKeySeg::StringSeg(prefix), + DbKeySeg::AddressSeg(validator), + DbKeySeg::StringSeg(metadata), + ] if addr == &ADDRESS + && prefix == VALIDATOR_STORAGE_PREFIX + && matches!( + metadata.as_str(), + "email" | "description" | "website" | "discord_handle" + ) => + { + Some(validator) + } + _ => None, + } +} + /// Storage key for validator's rewards products. pub fn validator_rewards_product_key(validator: &Address) -> Key { validator_prefix(validator) diff --git a/tests/src/e2e/ledger_tests.rs b/tests/src/e2e/ledger_tests.rs index 19a6d3ab1a9..9f584e2b246 100644 --- a/tests/src/e2e/ledger_tests.rs +++ b/tests/src/e2e/ledger_tests.rs @@ -3457,3 +3457,211 @@ fn change_validator_metadata() -> Result<()> { Ok(()) } + +#[test] +fn test_invalid_validator_txs() -> Result<()> { + let pipeline_len = 2; + let unbonding_len = 4; + let test = setup::network( + |mut genesis, base_dir: &_| { + genesis.parameters.pos_params.pipeline_len = pipeline_len; + genesis.parameters.pos_params.unbonding_len = unbonding_len; + // genesis.parameters.parameters.min_num_of_blocks = 6; + // genesis.parameters.parameters.max_expected_time_per_block = 1; + // genesis.parameters.parameters.epochs_per_year = 31_536_000; + let mut genesis = setup::set_validators( + 2, + genesis, + base_dir, + default_port_offset, + ); + let bonds = genesis.transactions.bond.unwrap(); + genesis.transactions.bond = Some( + bonds + .into_iter() + .filter(|bond| { + (&bond.data.validator).as_ref() != "validator-1" + }) + .collect(), + ); + genesis + }, + None, + )?; + + set_ethereum_bridge_mode( + &test, + &test.net.chain_id, + &Who::Validator(0), + ethereum_bridge::ledger::Mode::Off, + None, + ); + + // 1. Run the ledger node + let _bg_validator_0 = + start_namada_ledger_node_wait_wasm(&test, Some(0), Some(40))? + .background(); + + let _bg_validator_1 = + start_namada_ledger_node_wait_wasm(&test, Some(1), Some(40))? + .background(); + + let validator_0_rpc = get_actor_rpc(&test, &Who::Validator(0)); + let validator_1_rpc = get_actor_rpc(&test, &Who::Validator(1)); + + // put money in the validator-1 account from its balance account so that it + // can deactivate and reactivate + let tx_args = vec![ + "transfer", + "--source", + "validator-1-balance-key", + "--target", + "validator-1-validator-key", + "--amount", + "100.0", + "--token", + "NAM", + "--node", + &validator_1_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(1), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is valid.")?; + client.assert_success(); + + // put money in the validator-0 account from its balance account so that it + // can deactivate and reactivate + let tx_args = vec![ + "transfer", + "--source", + "validator-0-balance-key", + "--target", + "validator-0-validator-key", + "--amount", + "100.0", + "--token", + "NAM", + "--node", + &validator_0_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is valid.")?; + client.assert_success(); + + // Try to change validator-1 commission rate as validator-0 + let tx_args = vec![ + "change-commission-rate", + "--validator", + "validator-1", + "--commission-rate", + "0.06", + "--signing-keys", + "validator-0-validator-key", + "--node", + &validator_0_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is invalid.")?; + client.assert_success(); + + // Try to deactivate validator-1 as validator-0 + let tx_args = vec![ + "deactivate-validator", + "--validator", + "validator-1", + "--signing-keys", + "validator-0-validator-key", + "--node", + &validator_0_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is invalid.")?; + client.assert_success(); + + // Try to change the validator-1 website as validator-0 + let tx_args = vec![ + "change-metadata", + "--validator", + "validator-1", + "--website", + "theworstvalidator@namada.net", + "--signing-keys", + "validator-0-validator-key", + "--node", + &validator_0_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is invalid.")?; + client.assert_success(); + + // Deactivate validator-1 + let tx_args = vec![ + "deactivate-validator", + "--validator", + "validator-1", + "--signing-keys", + "validator-1-validator-key", + "--node", + &validator_1_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(1), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is valid.")?; + client.assert_success(); + + let deactivate_epoch = get_epoch(&test, &validator_1_rpc)?; + let start = Instant::now(); + let loop_timeout = Duration::new(120, 0); + loop { + if Instant::now().duration_since(start) > loop_timeout { + panic!( + "Timed out waiting for epoch: {}", + deactivate_epoch + pipeline_len + ); + } + let epoch = epoch_sleep(&test, &validator_1_rpc, 40)?; + if epoch >= deactivate_epoch + pipeline_len { + break; + } + } + + // Check the state of validator-1 + let tx_args = vec![ + "validator-state", + "--validator", + "validator-1", + "--node", + &validator_1_rpc, + ]; + let mut client = run!(test, Bin::Client, tx_args, Some(40))?; + client.exp_regex(r"Validator [a-z0-9]+ is inactive")?; + client.assert_success(); + + // Try to reactivate validator-1 as validator-0 + let tx_args = vec![ + "reactivate-validator", + "--validator", + "validator-1", + "--signing-keys", + "validator-0-validator-key", + "--node", + &validator_0_rpc, + ]; + let mut client = + run_as!(test, Who::Validator(0), Bin::Client, tx_args, Some(40))?; + client.exp_string("Transaction applied with result:")?; + client.exp_string("Transaction is invalid.")?; + client.assert_success(); + + Ok(()) +} diff --git a/vp_prelude/src/lib.rs b/vp_prelude/src/lib.rs index 3c2d4f7b880..caa7551e605 100644 --- a/vp_prelude/src/lib.rs +++ b/vp_prelude/src/lib.rs @@ -14,7 +14,6 @@ use std::convert::TryFrom; use std::marker::PhantomData; pub use borsh::{BorshDeserialize, BorshSerialize}; -pub use borsh_ext; use borsh_ext::BorshSerializeExt; pub use namada_core::ledger::governance::storage as gov_storage; pub use namada_core::ledger::parameters; @@ -34,10 +33,10 @@ use namada_core::types::storage::{ }; pub use namada_core::types::*; pub use namada_macros::validity_predicate; -pub use namada_proof_of_stake::storage as proof_of_stake; use namada_vm_env::vp::*; use namada_vm_env::{read_from_buffer, read_key_val_bytes_from_buffer}; pub use sha2::{Digest, Sha256, Sha384, Sha512}; +pub use {borsh_ext, namada_proof_of_stake as proof_of_stake}; pub fn sha256(bytes: &[u8]) -> Hash { let digest = Sha256::digest(bytes); diff --git a/wasm/wasm_source/src/vp_implicit.rs b/wasm/wasm_source/src/vp_implicit.rs index db29995c2ab..abeb08d3746 100644 --- a/wasm/wasm_source/src/vp_implicit.rs +++ b/wasm/wasm_source/src/vp_implicit.rs @@ -32,7 +32,7 @@ impl<'a> From<&'a storage::Key> for KeyType<'a> { Self::Pk(address) } else if let Some([_, owner]) = token::is_any_token_balance_key(key) { Self::Token { owner } - } else if proof_of_stake::is_pos_key(key) { + } else if proof_of_stake::storage::is_pos_key(key) { Self::PoS } else if gov_storage::keys::is_vote_key(key) { let voter_address = gov_storage::keys::get_voter_address(key); @@ -132,10 +132,10 @@ fn validate_tx( } KeyType::PoS => { // Allow the account to be used in PoS - let bond_id = proof_of_stake::is_bond_key(key) + let bond_id = proof_of_stake::storage::is_bond_key(key) .map(|(bond_id, _)| bond_id) .or_else(|| { - proof_of_stake::is_unbond_key(key) + proof_of_stake::storage::is_unbond_key(key) .map(|(bond_id, _, _)| bond_id) }); let valid = match bond_id { diff --git a/wasm/wasm_source/src/vp_user.rs b/wasm/wasm_source/src/vp_user.rs index 6e81bcdac19..064e0f9076e 100644 --- a/wasm/wasm_source/src/vp_user.rs +++ b/wasm/wasm_source/src/vp_user.rs @@ -27,7 +27,7 @@ impl<'a> From<&'a storage::Key> for KeyType<'a> { fn from(key: &'a storage::Key) -> KeyType<'a> { if let Some([_, owner]) = token::is_any_token_balance_key(key) { Self::Token { owner } - } else if proof_of_stake::is_pos_key(key) { + } else if proof_of_stake::storage::is_pos_key(key) { Self::PoS } else if gov_storage::keys::is_vote_key(key) { let voter_address = gov_storage::keys::get_voter_address(key); @@ -107,10 +107,10 @@ fn validate_tx( } KeyType::PoS => { // Allow the account to be used in PoS - let bond_id = proof_of_stake::is_bond_key(key) + let bond_id = proof_of_stake::storage::is_bond_key(key) .map(|(bond_id, _)| bond_id) .or_else(|| { - proof_of_stake::is_unbond_key(key) + proof_of_stake::storage::is_unbond_key(key) .map(|(bond_id, _, _)| bond_id) }); let valid = match bond_id { diff --git a/wasm/wasm_source/src/vp_validator.rs b/wasm/wasm_source/src/vp_validator.rs index 7714205e53b..fccd1691d4b 100644 --- a/wasm/wasm_source/src/vp_validator.rs +++ b/wasm/wasm_source/src/vp_validator.rs @@ -15,6 +15,7 @@ use namada_vp_prelude::storage::KeySeg; use namada_vp_prelude::*; use once_cell::unsync::Lazy; +use proof_of_stake::types::ValidatorState; enum KeyType<'a> { Token { owner: &'a Address }, @@ -29,7 +30,7 @@ impl<'a> From<&'a storage::Key> for KeyType<'a> { fn from(key: &'a storage::Key) -> KeyType<'a> { if let Some([_, owner]) = token::is_any_token_balance_key(key) { Self::Token { owner } - } else if proof_of_stake::is_pos_key(key) { + } else if proof_of_stake::storage::is_pos_key(key) { Self::PoS } else if gov_storage::keys::is_vote_key(key) { let voter_address = gov_storage::keys::get_voter_address(key); @@ -57,7 +58,8 @@ fn validate_tx( verifiers: BTreeSet
, ) -> VpResult { debug_log!( - "vp_user called with user addr: {}, key_changed: {:?}, verifiers: {:?}", + "vp_validator called with user addr: {}, key_changed: {:?}, \ + verifiers: {:?}", addr, keys_changed, verifiers @@ -105,11 +107,11 @@ fn validate_tx( } } KeyType::PoS => { - // Allow the account to be used in PoS - let bond_id = proof_of_stake::is_bond_key(key) + // Bond or unbond + let bond_id = proof_of_stake::storage::is_bond_key(key) .map(|(bond_id, _)| bond_id) .or_else(|| { - proof_of_stake::is_unbond_key(key) + proof_of_stake::storage::is_unbond_key(key) .map(|(bond_id, _, _)| bond_id) }); let valid_bond_or_unbond_change = match bond_id { @@ -123,15 +125,93 @@ fn validate_tx( true } }; + // Commission rate changes must be signed by the validator let comm = - proof_of_stake::is_validator_commission_rate_key(key); - // Validator's commission rate change must be signed + proof_of_stake::storage::is_validator_commission_rate_key( + key, + ); let valid_commission_rate_change = match comm { - Some(source) => *source != addr || *valid_sig, + Some((validator, _epoch)) => { + *valid_sig && *validator == addr + } + None => true, + }; + ); + // Metadata changes must be signed by the validator whose + // metadata is manipulated + let metadata = + proof_of_stake::storage::is_validator_metadata_key(key); + let valid_metadata_change = match metadata { + Some(address) => *address == addr && *valid_sig, None => true, }; - let valid = - valid_bond_or_unbond_change && valid_commission_rate_change; + + // Changes due to unjailing, deactivating, and reactivating are + // marked by changes in validator state + let state_change = + proof_of_stake::storage::is_validator_state_key(key); + let ( + valid_unjail_change, + valid_deactivation_change, + valid_reactionation_change, + ) = match state_change { + Some((address, epoch)) => { + let params_pre = + proof_of_stake::read_pos_params(&ctx.pre())?; + let state_pre = + proof_of_stake::validator_state_handle(address) + .get(&ctx.pre(), epoch, ¶ms_pre)?; + + let params_post = + proof_of_stake::read_pos_params(&ctx.post())?; + let state_post = + proof_of_stake::validator_state_handle(address) + .get(&ctx.post(), epoch, ¶ms_post)?; + + match (state_pre, state_post) { + (Some(pre), Some(post)) => { + // Deactivation case + if matches!( + pre, + ValidatorState::Consensus + | ValidatorState::BelowCapacity + | ValidatorState::BelowThreshold + ) && post == ValidatorState::Inactive + { + (true, *address == addr && *valid_sig, true) + } + // Reactivation case + else if pre == ValidatorState::Inactive + && post != ValidatorState::Inactive + { + (true, true, *address == addr && *valid_sig) + } + // Unjail case + else if pre == ValidatorState::Jailed + && matches!( + post, + ValidatorState::Consensus + | ValidatorState::BelowCapacity + | ValidatorState::BelowThreshold + ) + { + (*address == addr && *valid_sig, true, true) + } else { + (true, true, true) + } + } + _ => (true, true, true), + } + } + None => (true, true, true), + }; + + let valid = valid_bond_or_unbond_change + && valid_commission_rate_change + && valid_deactivation_change + && valid_reactionation_change + && valid_unjail_change + && valid_metadata_change; debug_log!( "PoS key {} {}", key, @@ -562,12 +642,13 @@ mod tests { tx::ctx() .unbond_tokens(Some(&vp_owner), &validator, unbond_amount) .unwrap(); - tx::ctx() - .change_validator_commission_rate( - &validator, - &Dec::new(6, 2).unwrap(), - ) - .unwrap(); + // tx::ctx() + // .change_validator_commission_rate( + // &validator, + // &Dec::new(6, 2).unwrap(), + // ) + // .unwrap(); + // tx::ctx().deactivate_validator(&validator).unwrap(); }); let pks_map = AccountPublicKeysMap::from_iter(vec![public_key]);