From ac07ad1afe36a2d8f4814adaabb74e763db8c1a8 Mon Sep 17 00:00:00 2001 From: Gianmarco Fraccaroli Date: Thu, 25 Apr 2024 11:31:29 +0200 Subject: [PATCH 1/9] added changelog --- .../imrpovements/3120-improve-governance-voting-logic.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/imrpovements/3120-improve-governance-voting-logic.md diff --git a/.changelog/unreleased/imrpovements/3120-improve-governance-voting-logic.md b/.changelog/unreleased/imrpovements/3120-improve-governance-voting-logic.md new file mode 100644 index 0000000000..87950e23ed --- /dev/null +++ b/.changelog/unreleased/imrpovements/3120-improve-governance-voting-logic.md @@ -0,0 +1,2 @@ +- Improve vote proposal logic transaction. + ([\#3120](https://github.com/anoma/namada/pull/3120)) \ No newline at end of file From fa4237f77d4160c99fce69231fef8393811e8a4c Mon Sep 17 00:00:00 2001 From: Gianmarco Fraccaroli Date: Tue, 23 Apr 2024 16:19:08 +0200 Subject: [PATCH 2/9] wip --- .../lib/node/ledger/shell/finalize_block.rs | 8 ++- crates/benches/native_vps.rs | 4 -- crates/benches/txs.rs | 2 - crates/benches/vps.rs | 4 -- crates/governance/src/storage/mod.rs | 9 +++- crates/governance/src/storage/proposal.rs | 6 +-- .../light_sdk/src/transaction/governance.rs | 9 +--- crates/namada/src/ledger/governance/mod.rs | 53 ++++++++++--------- crates/proof_of_stake/src/queries.rs | 39 +++++++++++--- crates/proof_of_stake/src/tests/test_pos.rs | 48 ++++++++--------- crates/sdk/src/queries/vp/pos.rs | 13 +++-- crates/sdk/src/signing.rs | 7 --- crates/sdk/src/tx.rs | 30 ++++------- crates/tx_prelude/src/proof_of_stake.rs | 1 + wasm/tx_vote_proposal/src/lib.rs | 29 +++++++++- 15 files changed, 147 insertions(+), 115 deletions(-) diff --git a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs index 4c6b6c419a..b6a8cac9f9 100644 --- a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -1274,11 +1274,15 @@ mod test_finalize_block { id: proposal_id, vote, voter: validator, - delegation_validators: vec![], }; // Vote to accept the proposal (there's only one validator, so its // vote decides) - namada::governance::vote_proposal(&mut shell.state, vote).unwrap(); + namada::governance::vote_proposal( + &mut shell.state, + vote, + HashSet::new(), + ) + .unwrap(); }; // Add a proposal to be accepted and one to be rejected. diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index dfa2863c93..c9de686b37 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -89,9 +89,6 @@ fn governance(c: &mut Criterion) { id: 0, vote: ProposalVote::Yay, voter: defaults::albert_address(), - delegation_validators: vec![ - defaults::validator_address(), - ], }, None, None, @@ -107,7 +104,6 @@ fn governance(c: &mut Criterion) { id: 0, vote: ProposalVote::Nay, voter: defaults::validator_address(), - delegation_validators: vec![], }, None, None, diff --git a/crates/benches/txs.rs b/crates/benches/txs.rs index 881aa16010..c504378c3b 100644 --- a/crates/benches/txs.rs +++ b/crates/benches/txs.rs @@ -547,7 +547,6 @@ fn vote_proposal(c: &mut Criterion) { id: 0, vote: ProposalVote::Yay, voter: defaults::albert_address(), - delegation_validators: vec![defaults::validator_address()], }, None, None, @@ -560,7 +559,6 @@ fn vote_proposal(c: &mut Criterion) { id: 0, vote: ProposalVote::Nay, voter: defaults::validator_address(), - delegation_validators: vec![], }, None, None, diff --git a/crates/benches/vps.rs b/crates/benches/vps.rs index 8d12d0393b..e4520dbb25 100644 --- a/crates/benches/vps.rs +++ b/crates/benches/vps.rs @@ -94,9 +94,6 @@ fn vp_implicit(c: &mut Criterion) { id: 0, vote: ProposalVote::Yay, voter: Address::from(&implicit_account.to_public()), - delegation_validators: vec![], /* NOTE: no need to bond tokens - * because the - * implicit vp doesn't check that */ }, None, None, @@ -252,7 +249,6 @@ fn vp_user(c: &mut Criterion) { id: 0, vote: ProposalVote::Yay, voter: defaults::validator_address(), - delegation_validators: vec![], }, None, None, diff --git a/crates/governance/src/storage/mod.rs b/crates/governance/src/storage/mod.rs index b1e6cf9b7a..68ea10842f 100644 --- a/crates/governance/src/storage/mod.rs +++ b/crates/governance/src/storage/mod.rs @@ -11,6 +11,7 @@ use std::collections::{BTreeMap, BTreeSet}; use namada_core::address::Address; use namada_core::borsh::BorshDeserialize; +use namada_core::collections::HashSet; use namada_core::storage::Epoch; use namada_storage::{iter_prefix, Error, Result, StorageRead, StorageWrite}; use namada_trans_token as token; @@ -100,11 +101,15 @@ where } /// A proposal vote transaction. -pub fn vote_proposal(storage: &mut S, data: VoteProposalData) -> Result<()> +pub fn vote_proposal( + storage: &mut S, + data: VoteProposalData, + delegation_targets: HashSet
, +) -> Result<()> where S: StorageRead + StorageWrite, { - for validator in data.delegation_validators { + for validator in delegation_targets { let vote_key = governance_keys::get_vote_proposal_key( data.id, data.voter.clone(), diff --git a/crates/governance/src/storage/proposal.rs b/crates/governance/src/storage/proposal.rs index 8e5d8c0e9d..44ada20abc 100644 --- a/crates/governance/src/storage/proposal.rs +++ b/crates/governance/src/storage/proposal.rs @@ -82,8 +82,6 @@ pub struct VoteProposalData { pub vote: ProposalVote, /// The proposal voter address pub voter: Address, - /// Validators to who the voter has delegations to - pub delegation_validators: Vec
, } impl TryFrom for InitProposalData { @@ -771,13 +769,11 @@ pub mod testing { id: u64, vote in arb_proposal_vote(), voter in arb_non_internal_address(), - delegation_validators in collection::vec(arb_non_internal_address(), 0..10), ) -> VoteProposalData { VoteProposalData { id, vote, - voter, - delegation_validators, + voter } } } diff --git a/crates/light_sdk/src/transaction/governance.rs b/crates/light_sdk/src/transaction/governance.rs index ae97e0d4c9..2f6e6fed42 100644 --- a/crates/light_sdk/src/transaction/governance.rs +++ b/crates/light_sdk/src/transaction/governance.rs @@ -106,15 +106,10 @@ impl VoteProposal { id: u64, vote: ProposalVote, voter: Address, - delegations: Vec
, args: GlobalArgs, ) -> Self { - let vote_proposal = namada_sdk::governance::VoteProposalData { - id, - vote, - voter, - delegation_validators: delegations, - }; + let vote_proposal = + namada_sdk::governance::VoteProposalData { id, vote, voter }; Self(transaction::build_tx( args, diff --git a/crates/namada/src/ledger/governance/mod.rs b/crates/namada/src/ledger/governance/mod.rs index 4bacadac4d..d9b55d5ed5 100644 --- a/crates/namada/src/ledger/governance/mod.rs +++ b/crates/namada/src/ledger/governance/mod.rs @@ -13,7 +13,8 @@ use namada_governance::storage::{is_proposal_accepted, keys as gov_storage}; use namada_governance::utils::is_valid_validator_voting_period; use namada_governance::ProposalVote; use namada_proof_of_stake::is_validator; -use namada_proof_of_stake::queries::find_delegations; +use namada_proof_of_stake::queries::find_delegation_validators; +use namada_proof_of_stake::types::ValidatorState; use namada_state::{StateRead, StorageRead}; use namada_tx::action::{Action, GovAction, Read}; use namada_tx::Tx; @@ -303,34 +304,36 @@ where .into()); } - // TODO: We should refactor this by modifying the vote proposal tx - let all_delegations_are_valid = if let Ok(delegations) = - find_delegations(&self.ctx.pre(), voter, ¤t_epoch) - { - if delegations.is_empty() { - return Err(native_vp::Error::new_alloc(format!( - "No delegations found for {voter}" - )) - .into()); - } else { - delegations.iter().all(|(val_address, _)| { - let vote_key = gov_storage::get_vote_proposal_key( - proposal_id, - voter.clone(), - val_address.clone(), - ); - self.ctx.post().has_key(&vote_key).unwrap_or(false) - }) - } - } else { + let delegations_targets = find_delegation_validators( + &self.ctx.pre(), + voter, + &pre_voting_start_epoch, + ) + .unwrap_or_default(); + + if delegations_targets.is_empty() { return Err(native_vp::Error::new_alloc(format!( - "Failed to query delegations for {voter}" + "No delegations found for {voter}" )) .into()); - }; - if !all_delegations_are_valid { + } + + if !delegations_targets.contains_key(validator) { + return Err(native_vp::Error::new_alloc(format!( + "The vote key is not valid due to {voter} not having \ + delegations towards {validator}" + )) + .into()); + } + + // this is safe as we check above that validator is part of the hashmap + if !matches!( + delegations_targets.get(validator).unwrap(), + ValidatorState::Consensus + ) { return Err(native_vp::Error::new_alloc(format!( - "Not all delegations of {voter} were deemed valid" + "The vote key is not valid due to {validator} being not in \ + the active set" )) .into()); } diff --git a/crates/proof_of_stake/src/queries.rs b/crates/proof_of_stake/src/queries.rs index 5d643d2f48..8d4a193207 100644 --- a/crates/proof_of_stake/src/queries.rs +++ b/crates/proof_of_stake/src/queries.rs @@ -5,7 +5,7 @@ use std::collections::BTreeMap; use borsh::BorshDeserialize; use namada_core::address::Address; -use namada_core::collections::{HashMap, HashSet}; +use namada_core::collections::HashMap; use namada_core::dec::Dec; use namada_core::storage::Epoch; use namada_core::token; @@ -15,10 +15,11 @@ use namada_storage::StorageRead; use crate::slashing::{find_validator_slashes, get_slashed_amount}; use crate::storage::{ bond_handle, delegation_targets_handle, read_pos_params, unbond_handle, + validator_state_handle, }; use crate::types::{ BondDetails, BondId, BondsAndUnbondsDetail, BondsAndUnbondsDetails, - DelegationEpochs, Slash, UnbondDetails, + DelegationEpochs, Slash, UnbondDetails, ValidatorState, }; use crate::{raw_bond_amount, storage_key, PosParams}; @@ -28,16 +29,16 @@ pub fn find_delegation_validators( storage: &S, owner: &Address, epoch: &Epoch, -) -> namada_storage::Result> +) -> namada_storage::Result> where S: StorageRead, { let validators = delegation_targets_handle(owner); if validators.is_empty(storage)? { - return Ok(HashSet::new()); + return Ok(HashMap::new()); } - let mut delegation_targets = HashSet::
::new(); + let mut delegation_targets = HashMap::new(); for validator in validators.iter(storage)? { let ( @@ -53,18 +54,27 @@ where // the `last_range` will tell us if there was a bond if let Some(end) = last_end { if *epoch < end { - delegation_targets.insert(val); + let validator_state = + validator_state(storage, &val, epoch)? + .expect("Delegation target must be a validator."); + delegation_targets.insert(val, validator_state); } } else { // this is bond is currently held - delegation_targets.insert(val); + let validator_state = validator_state(storage, &val, epoch)? + .expect("Delegation target must be a validator."); + delegation_targets.insert(val, validator_state); } } else { // need to search through the `prev_ranges` now for (start, end) in prev_ranges.iter().rev() { if *epoch >= *start { if *epoch < *end { - delegation_targets.insert(val); + let validator_state = validator_state( + storage, &val, epoch, + )? + .expect("Delegation target must be a validator."); + delegation_targets.insert(val, validator_state); } break; } @@ -75,6 +85,19 @@ where Ok(delegation_targets) } +/// Get the validator state +pub fn validator_state( + storage: &S, + validator: &Address, + epoch: &Epoch, +) -> namada_storage::Result> +where + S: StorageRead, +{ + let params = read_pos_params(storage)?; + validator_state_handle(validator).get(storage, *epoch, ¶ms) +} + /// Find all validators to which a given bond `owner` (or source) has a /// delegation with the amount pub fn find_delegations( diff --git a/crates/proof_of_stake/src/tests/test_pos.rs b/crates/proof_of_stake/src/tests/test_pos.rs index 0150cba4d5..398474459c 100644 --- a/crates/proof_of_stake/src/tests/test_pos.rs +++ b/crates/proof_of_stake/src/tests/test_pos.rs @@ -385,7 +385,7 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( details.bonds.len(), 2, - "Contains genesis and newly added self-bond" + "contains_key genesis and newly added self-bond" ); // dbg!(&details.bonds); assert_eq!( @@ -527,7 +527,7 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( self_bond_details.bonds.len(), 2, - "Contains genesis and newly added self-bond" + "contains_key genesis and newly added self-bond" ); assert_eq!( self_bond_details.bonds[0], @@ -645,7 +645,7 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( self_bond_details.bonds.len(), 1, - "Contains only part of the genesis bond now" + "contains_key only part of the genesis bond now" ); assert_eq!( self_bond_details.bonds[0], @@ -667,8 +667,8 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( self_bond_details.unbonds.len(), if unbonded_genesis_self_bond { 2 } else { 1 }, - "Contains a full unbond of the last self-bond and an unbond from \ - the genesis bond" + "contains_key a full unbond of the last self-bond and an unbond \ + from the genesis bond" ); if unbonded_genesis_self_bond { assert_eq!( @@ -1809,8 +1809,8 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &validator2, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains(&validator1)); - assert!(delegatees2.contains(&validator2)); + assert!(delegatees1.contains_key(&validator1)); + assert!(delegatees2.contains_key(&validator2)); } // Advance to epoch 1 and check if the delegation targets are properly @@ -1826,8 +1826,8 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &validator2, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains(&validator1)); - assert!(delegatees2.contains(&validator2)); + assert!(delegatees1.contains_key(&validator1)); + assert!(delegatees2.contains_key(&validator2)); } // Bond from a delegator to validator1 in epoch 1 @@ -1863,15 +1863,15 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &validator2, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains(&validator1)); - assert!(delegatees2.contains(&validator2)); + assert!(delegatees1.contains_key(&validator1)); + assert!(delegatees2.contains_key(&validator2)); } let delegatees1 = find_delegation_validators(&storage, &validator1, &pipeline_epoch) .unwrap(); assert_eq!(delegatees1.len(), 1); - assert!(delegatees1.contains(&validator1)); + assert!(delegatees1.contains_key(&validator1)); let delegatees2 = find_delegation_validators(&storage, &validator2, &pipeline_epoch) @@ -1882,7 +1882,7 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &delegator, &pipeline_epoch) .unwrap(); assert_eq!(del_delegatees.len(), 1); - assert!(del_delegatees.contains(&validator1)); + assert!(del_delegatees.contains_key(&validator1)); // Advance to epoch 3 advance_epoch(&mut storage, ¶ms); @@ -1925,8 +1925,8 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &delegator, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains(&validator1)); - assert!(delegatees2.contains(&validator2)); + assert!(delegatees1.contains_key(&validator1)); + assert!(delegatees2.contains_key(&validator2)); assert!(del_delegatees.is_empty()); } @@ -1943,8 +1943,8 @@ fn test_delegation_targets() { assert_eq!(delegatees1.len(), 1); assert!(delegatees2.is_empty()); assert_eq!(del_delegatees.len(), 1); - assert!(delegatees1.contains(&validator1)); - assert!(del_delegatees.contains(&validator1)); + assert!(delegatees1.contains_key(&validator1)); + assert!(del_delegatees.contains_key(&validator1)); } // Epoch 5 (pipeline) @@ -1960,9 +1960,9 @@ fn test_delegation_targets() { assert_eq!(delegatees1.len(), 1); assert!(delegatees2.is_empty()); assert_eq!(del_delegatees.len(), 2); - assert!(delegatees1.contains(&validator1)); - assert!(del_delegatees.contains(&validator1)); - assert!(del_delegatees.contains(&validator2)); + assert!(delegatees1.contains_key(&validator1)); + assert!(del_delegatees.contains_key(&validator1)); + assert!(del_delegatees.contains_key(&validator2)); // Advance to epoch 4 and self-bond from validator2 again current_epoch = advance_epoch(&mut storage, ¶ms); @@ -1991,10 +1991,10 @@ fn test_delegation_targets() { assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); assert_eq!(del_delegatees.len(), 2); - assert!(delegatees1.contains(&validator1)); - assert!(delegatees2.contains(&validator2)); - assert!(del_delegatees.contains(&validator1)); - assert!(del_delegatees.contains(&validator2)); + assert!(delegatees1.contains_key(&validator1)); + assert!(delegatees2.contains_key(&validator2)); + assert!(del_delegatees.contains_key(&validator1)); + assert!(del_delegatees.contains_key(&validator2)); // Check everything again including the raw bond amount this time diff --git a/crates/sdk/src/queries/vp/pos.rs b/crates/sdk/src/queries/vp/pos.rs index 3947e9b01c..92b1e88b42 100644 --- a/crates/sdk/src/queries/vp/pos.rs +++ b/crates/sdk/src/queries/vp/pos.rs @@ -24,7 +24,7 @@ use namada_proof_of_stake::storage::{ read_validator_last_slash_epoch, read_validator_max_commission_rate_change, read_validator_stake, read_validator_website, unbond_handle, validator_commission_rate_handle, validator_incoming_redelegations_handle, - validator_slashes_handle, validator_state_handle, + validator_slashes_handle, }; pub use namada_proof_of_stake::types::ValidatorStateInfo; use namada_proof_of_stake::types::{ @@ -306,9 +306,9 @@ where H: 'static + StorageHasher + Sync, { let epoch = epoch.unwrap_or(ctx.state.in_mem().last_epoch); - let params = read_pos_params(ctx.state)?; - let state = - validator_state_handle(&validator).get(ctx.state, epoch, ¶ms)?; + let state = namada_proof_of_stake::queries::validator_state( + ctx.state, &validator, &epoch, + )?; Ok((state, epoch)) } @@ -573,7 +573,10 @@ where H: 'static + StorageHasher + Sync, { let epoch = epoch.unwrap_or(ctx.state.in_mem().last_epoch); - find_delegation_validators(ctx.state, &owner, &epoch) + Ok(find_delegation_validators(ctx.state, &owner, &epoch)? + .keys() + .cloned() + .collect()) } /// Find all the validator addresses to whom the given `owner` address has diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index 1e138a3a6a..189bf074ea 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -1260,19 +1260,12 @@ pub async fn to_ledger_vector( format!("Vote : {}", LedgerProposalVote(&vote_proposal.vote)), format!("Voter : {}", vote_proposal.voter), ]); - for delegation in &vote_proposal.delegation_validators { - tv.output.push(format!("Delegation : {}", delegation)); - } tv.output_expert.extend(vec![ format!("ID : {}", vote_proposal.id), format!("Vote : {}", LedgerProposalVote(&vote_proposal.vote)), format!("Voter : {}", vote_proposal.voter), ]); - for delegation in vote_proposal.delegation_validators { - tv.output_expert - .push(format!("Delegation : {}", delegation)); - } } else if code_sec.tag == Some(TX_REVEAL_PK.to_string()) { let public_key = common::PublicKey::try_from_slice( &tx.data() diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 66514ca6cb..1adafec031 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2179,7 +2179,7 @@ pub async fn build_vote_proposal( } } - let delegations = if is_validator { + if is_validator { let stake = get_validator_stake(context.client(), current_epoch, voter_address) .await?; @@ -2187,9 +2187,8 @@ pub async fn build_vote_proposal( if stake.is_zero() { edisplay_line!( context.io(), - "Voter address {} is a validator but has no stake, so it has \ - no votes.", - voter_address + "Voter address {voter_address} is a validator but has no \ + stake, so it has no votes.", ); if !tx.force { return Err(Error::Other( @@ -2206,18 +2205,15 @@ pub async fn build_vote_proposal( .0 .expect("Expected to find the state of the validator"); - if !matches!( + if matches!( val_state, ValidatorState::Jailed | ValidatorState::Inactive ) { - vec![voter_address.clone()] - } else { edisplay_line!( context.io(), - "Voter address {} is a validator that is either jailed or \ - inactive, and so it may not vote in governance at this \ - moment.", - voter_address + "Voter address {voter_address} is a validator that is either \ + jailed or inactive, and so it may not vote in governance at \ + this moment.", ); if !tx.force { return Err(Error::from( @@ -2227,12 +2223,11 @@ pub async fn build_vote_proposal( ), )); } - vec![] } } else { // Get active valid validators with whom the voter has delegations // (bonds) - let delegation_vals = rpc::get_delegations_of_delegator_at( + let delegation_vals = rpc::get_delegation_validators( context.client(), voter_address, proposal.voting_start_epoch, @@ -2240,10 +2235,10 @@ pub async fn build_vote_proposal( .await?; let mut delegation_validators = Vec::
::new(); - for validator in delegation_vals.keys() { + for validator in delegation_vals { let val_state = rpc::get_validator_state( context.client(), - validator, + &validator, Some(current_epoch), ) .await? @@ -2263,8 +2258,7 @@ pub async fn build_vote_proposal( if delegation_validators.is_empty() { edisplay_line!( context.io(), - "Voter address {} does not have any delegations.", - voter_address + "Voter address {voter_address} does not have any delegations.", ); if !tx.force { return Err(Error::from(TxSubmitError::NoDelegationsFound( @@ -2273,14 +2267,12 @@ pub async fn build_vote_proposal( ))); } } - delegation_validators }; let data = VoteProposalData { id: *proposal_id, vote: proposal_vote, voter: voter_address.clone(), - delegation_validators: delegations, }; build( diff --git a/crates/tx_prelude/src/proof_of_stake.rs b/crates/tx_prelude/src/proof_of_stake.rs index e15d0e82da..1815855e3e 100644 --- a/crates/tx_prelude/src/proof_of_stake.rs +++ b/crates/tx_prelude/src/proof_of_stake.rs @@ -3,6 +3,7 @@ use namada_core::dec::Dec; use namada_core::{key, token}; pub use namada_proof_of_stake::parameters::PosParams; +pub use namada_proof_of_stake::queries::find_delegation_validators; use namada_proof_of_stake::storage::read_pos_params; use namada_proof_of_stake::types::{ResultSlashing, ValidatorMetaData}; use namada_proof_of_stake::{ diff --git a/wasm/tx_vote_proposal/src/lib.rs b/wasm/tx_vote_proposal/src/lib.rs index 0f5e92803c..9f93120ab0 100644 --- a/wasm/tx_vote_proposal/src/lib.rs +++ b/wasm/tx_vote_proposal/src/lib.rs @@ -1,6 +1,9 @@ //! A tx to vote on a proposal use namada_tx_prelude::action::{Action, GovAction, Write}; +use namada_tx_prelude::gov_storage::keys::get_voting_start_epoch_key; +use namada_tx_prelude::proof_of_stake::find_delegation_validators; +use namada_tx_prelude::proof_of_stake::types::ValidatorState; use namada_tx_prelude::*; #[transaction] @@ -21,8 +24,32 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { voter: tx_data.voter.clone(), }))?; + let voting_start_epoch_key = get_voting_start_epoch_key(tx_data.id); + let proposal_start_epoch = ctx.read(&voting_start_epoch_key)?; + let proposal_start_epoch = if let Some(epoch) = proposal_start_epoch { + epoch + } else { + return Err(Error::new_alloc(format!( + "Proposal id {} doesn't have a start epoch", + tx_data.id + ))); + }; + debug_log!("apply_tx called to vote a governance proposal"); - governance::vote_proposal(ctx, tx_data) + let delegations_targets = + find_delegation_validators(ctx, &tx_data.voter, &proposal_start_epoch)?; + let active_delegations_targets = delegations_targets + .into_iter() + .filter_map(|(address, state)| { + if matches!(state, ValidatorState::Consensus) { + Some(address) + } else { + None + } + }) + .collect(); + + governance::vote_proposal(ctx, tx_data, active_delegations_targets) .wrap_err("Failed to vote on governance proposal") } From 9ae214aa83051a8db2498ca7a6a0991e17b1435a Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 29 Apr 2024 19:43:03 -0700 Subject: [PATCH 3/9] should gov VP have any delegations logic? turn off --- crates/namada/src/ledger/governance/mod.rs | 94 ++++++++++++++-------- 1 file changed, 59 insertions(+), 35 deletions(-) diff --git a/crates/namada/src/ledger/governance/mod.rs b/crates/namada/src/ledger/governance/mod.rs index d9b55d5ed5..37d1fdac76 100644 --- a/crates/namada/src/ledger/governance/mod.rs +++ b/crates/namada/src/ledger/governance/mod.rs @@ -13,8 +13,6 @@ use namada_governance::storage::{is_proposal_accepted, keys as gov_storage}; use namada_governance::utils::is_valid_validator_voting_period; use namada_governance::ProposalVote; use namada_proof_of_stake::is_validator; -use namada_proof_of_stake::queries::find_delegation_validators; -use namada_proof_of_stake::types::ValidatorState; use namada_state::{StateRead, StorageRead}; use namada_tx::action::{Action, GovAction, Read}; use namada_tx::Tx; @@ -304,39 +302,65 @@ where .into()); } - let delegations_targets = find_delegation_validators( - &self.ctx.pre(), - voter, - &pre_voting_start_epoch, - ) - .unwrap_or_default(); - - if delegations_targets.is_empty() { - return Err(native_vp::Error::new_alloc(format!( - "No delegations found for {voter}" - )) - .into()); - } - - if !delegations_targets.contains_key(validator) { - return Err(native_vp::Error::new_alloc(format!( - "The vote key is not valid due to {voter} not having \ - delegations towards {validator}" - )) - .into()); - } - - // this is safe as we check above that validator is part of the hashmap - if !matches!( - delegations_targets.get(validator).unwrap(), - ValidatorState::Consensus - ) { - return Err(native_vp::Error::new_alloc(format!( - "The vote key is not valid due to {validator} being not in \ - the active set" - )) - .into()); - } + // TODO: should any checks happen in the VP for the target validators? + // Not sure, since ultimately whether the vote counts or not is + // determined by the validator state at the end epoch, which likely has + // not occurred yet during VP execution + + // // Check the target validator of the bond used to vote + // let delegations_targets = find_delegation_validators( + // &self.ctx.pre(), + // voter, + // &pre_voting_end_epoch, + // ) + // .unwrap_or_default(); + + // if delegations_targets.is_empty() { + // return Err(native_vp::Error::new_alloc(format!( + // "No delegations found for {voter}" + // )) + // .into()); + // } + + // if !delegations_targets.contains(validator) { + // return Err(native_vp::Error::new_alloc(format!( + // "The vote key is not valid due to {voter} not having \ + // delegations to {validator}" + // )) + // .into()); + // } + + // let validator_state = read_validator_state(&self.ctx.pre(), + // validator, &pre_voting_end_epoch)?; if !matches! + // (validator_state, ValidatorState::Consensus | + // ValidatorState::BelowCapacity | ValidatorState::BelowThreshold) { + // if validator_state.is_none() { + // return Err(native_vp::Error::new_alloc(format!( + // "The vote key is invalid because the validator + // {validator} is not in \ the active set (jailed + // or inactive)" )) + // .into()); + // } else { + // return Err(native_vp::Error::new_alloc(format!( + // "The vote key is invalid because the validator + // {validator} is not in \ the active set (jailed + // or inactive)" )) + // .into()); + // } + + // } + + // // this is safe as we check above that validator is part of the + // hashmap if !matches!( + // delegations_targets.get(validator).unwrap(), + // ValidatorState::Consensus + // ) { + // return Err(native_vp::Error::new_alloc(format!( + // "The vote key is not valid due to {validator} being not in \ + // the active set" + // )) + // .into()); + // } // Voted outside of voting window. We dont check for validator because // if the proposal type is validator, we need to let From 989627ff8b4e62aad9ed32a742fee10fe89f01d0 Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 29 Apr 2024 19:44:54 -0700 Subject: [PATCH 4/9] revert find_delegation_validators -> no state --- crates/proof_of_stake/src/queries.rs | 41 ++++-------------- crates/proof_of_stake/src/storage.rs | 13 ++++++ crates/proof_of_stake/src/tests/test_pos.rs | 48 ++++++++++----------- crates/sdk/src/queries/vp/pos.rs | 7 +-- wasm/tx_vote_proposal/src/lib.rs | 11 ----- 5 files changed, 48 insertions(+), 72 deletions(-) diff --git a/crates/proof_of_stake/src/queries.rs b/crates/proof_of_stake/src/queries.rs index 8d4a193207..2d94bc687a 100644 --- a/crates/proof_of_stake/src/queries.rs +++ b/crates/proof_of_stake/src/queries.rs @@ -5,7 +5,7 @@ use std::collections::BTreeMap; use borsh::BorshDeserialize; use namada_core::address::Address; -use namada_core::collections::HashMap; +use namada_core::collections::{HashMap, HashSet}; use namada_core::dec::Dec; use namada_core::storage::Epoch; use namada_core::token; @@ -15,11 +15,10 @@ use namada_storage::StorageRead; use crate::slashing::{find_validator_slashes, get_slashed_amount}; use crate::storage::{ bond_handle, delegation_targets_handle, read_pos_params, unbond_handle, - validator_state_handle, }; use crate::types::{ BondDetails, BondId, BondsAndUnbondsDetail, BondsAndUnbondsDetails, - DelegationEpochs, Slash, UnbondDetails, ValidatorState, + DelegationEpochs, Slash, UnbondDetails, }; use crate::{raw_bond_amount, storage_key, PosParams}; @@ -29,16 +28,16 @@ pub fn find_delegation_validators( storage: &S, owner: &Address, epoch: &Epoch, -) -> namada_storage::Result> +) -> namada_storage::Result> where S: StorageRead, { let validators = delegation_targets_handle(owner); if validators.is_empty(storage)? { - return Ok(HashMap::new()); + return Ok(HashSet::new()); } - let mut delegation_targets = HashMap::new(); + let mut delegation_targets = HashSet::new(); for validator in validators.iter(storage)? { let ( @@ -54,27 +53,18 @@ where // the `last_range` will tell us if there was a bond if let Some(end) = last_end { if *epoch < end { - let validator_state = - validator_state(storage, &val, epoch)? - .expect("Delegation target must be a validator."); - delegation_targets.insert(val, validator_state); + delegation_targets.insert(val); } } else { - // this is bond is currently held - let validator_state = validator_state(storage, &val, epoch)? - .expect("Delegation target must be a validator."); - delegation_targets.insert(val, validator_state); + // this bond is currently held + delegation_targets.insert(val); } } else { // need to search through the `prev_ranges` now for (start, end) in prev_ranges.iter().rev() { if *epoch >= *start { if *epoch < *end { - let validator_state = validator_state( - storage, &val, epoch, - )? - .expect("Delegation target must be a validator."); - delegation_targets.insert(val, validator_state); + delegation_targets.insert(val); } break; } @@ -85,19 +75,6 @@ where Ok(delegation_targets) } -/// Get the validator state -pub fn validator_state( - storage: &S, - validator: &Address, - epoch: &Epoch, -) -> namada_storage::Result> -where - S: StorageRead, -{ - let params = read_pos_params(storage)?; - validator_state_handle(validator).get(storage, *epoch, ¶ms) -} - /// Find all validators to which a given bond `owner` (or source) has a /// delegation with the amount pub fn find_delegations( diff --git a/crates/proof_of_stake/src/storage.rs b/crates/proof_of_stake/src/storage.rs index d431d9a7e4..42c4e743b7 100644 --- a/crates/proof_of_stake/src/storage.rs +++ b/crates/proof_of_stake/src/storage.rs @@ -446,6 +446,19 @@ where storage.write(&key, inflation) } +/// Read the validator state +pub fn read_validator_state( + storage: &S, + validator: &Address, + epoch: &Epoch, +) -> namada_storage::Result> +where + S: StorageRead, +{ + let params = read_pos_params(storage)?; + validator_state_handle(validator).get(storage, *epoch, ¶ms) +} + /// Read PoS validator's delta value. pub fn read_validator_deltas_value( storage: &S, diff --git a/crates/proof_of_stake/src/tests/test_pos.rs b/crates/proof_of_stake/src/tests/test_pos.rs index 398474459c..0150cba4d5 100644 --- a/crates/proof_of_stake/src/tests/test_pos.rs +++ b/crates/proof_of_stake/src/tests/test_pos.rs @@ -385,7 +385,7 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( details.bonds.len(), 2, - "contains_key genesis and newly added self-bond" + "Contains genesis and newly added self-bond" ); // dbg!(&details.bonds); assert_eq!( @@ -527,7 +527,7 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( self_bond_details.bonds.len(), 2, - "contains_key genesis and newly added self-bond" + "Contains genesis and newly added self-bond" ); assert_eq!( self_bond_details.bonds[0], @@ -645,7 +645,7 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( self_bond_details.bonds.len(), 1, - "contains_key only part of the genesis bond now" + "Contains only part of the genesis bond now" ); assert_eq!( self_bond_details.bonds[0], @@ -667,8 +667,8 @@ fn test_bonds_aux(params: OwnedPosParams, validators: Vec) { assert_eq!( self_bond_details.unbonds.len(), if unbonded_genesis_self_bond { 2 } else { 1 }, - "contains_key a full unbond of the last self-bond and an unbond \ - from the genesis bond" + "Contains a full unbond of the last self-bond and an unbond from \ + the genesis bond" ); if unbonded_genesis_self_bond { assert_eq!( @@ -1809,8 +1809,8 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &validator2, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains_key(&validator1)); - assert!(delegatees2.contains_key(&validator2)); + assert!(delegatees1.contains(&validator1)); + assert!(delegatees2.contains(&validator2)); } // Advance to epoch 1 and check if the delegation targets are properly @@ -1826,8 +1826,8 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &validator2, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains_key(&validator1)); - assert!(delegatees2.contains_key(&validator2)); + assert!(delegatees1.contains(&validator1)); + assert!(delegatees2.contains(&validator2)); } // Bond from a delegator to validator1 in epoch 1 @@ -1863,15 +1863,15 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &validator2, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains_key(&validator1)); - assert!(delegatees2.contains_key(&validator2)); + assert!(delegatees1.contains(&validator1)); + assert!(delegatees2.contains(&validator2)); } let delegatees1 = find_delegation_validators(&storage, &validator1, &pipeline_epoch) .unwrap(); assert_eq!(delegatees1.len(), 1); - assert!(delegatees1.contains_key(&validator1)); + assert!(delegatees1.contains(&validator1)); let delegatees2 = find_delegation_validators(&storage, &validator2, &pipeline_epoch) @@ -1882,7 +1882,7 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &delegator, &pipeline_epoch) .unwrap(); assert_eq!(del_delegatees.len(), 1); - assert!(del_delegatees.contains_key(&validator1)); + assert!(del_delegatees.contains(&validator1)); // Advance to epoch 3 advance_epoch(&mut storage, ¶ms); @@ -1925,8 +1925,8 @@ fn test_delegation_targets() { find_delegation_validators(&storage, &delegator, &epoch).unwrap(); assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); - assert!(delegatees1.contains_key(&validator1)); - assert!(delegatees2.contains_key(&validator2)); + assert!(delegatees1.contains(&validator1)); + assert!(delegatees2.contains(&validator2)); assert!(del_delegatees.is_empty()); } @@ -1943,8 +1943,8 @@ fn test_delegation_targets() { assert_eq!(delegatees1.len(), 1); assert!(delegatees2.is_empty()); assert_eq!(del_delegatees.len(), 1); - assert!(delegatees1.contains_key(&validator1)); - assert!(del_delegatees.contains_key(&validator1)); + assert!(delegatees1.contains(&validator1)); + assert!(del_delegatees.contains(&validator1)); } // Epoch 5 (pipeline) @@ -1960,9 +1960,9 @@ fn test_delegation_targets() { assert_eq!(delegatees1.len(), 1); assert!(delegatees2.is_empty()); assert_eq!(del_delegatees.len(), 2); - assert!(delegatees1.contains_key(&validator1)); - assert!(del_delegatees.contains_key(&validator1)); - assert!(del_delegatees.contains_key(&validator2)); + assert!(delegatees1.contains(&validator1)); + assert!(del_delegatees.contains(&validator1)); + assert!(del_delegatees.contains(&validator2)); // Advance to epoch 4 and self-bond from validator2 again current_epoch = advance_epoch(&mut storage, ¶ms); @@ -1991,10 +1991,10 @@ fn test_delegation_targets() { assert_eq!(delegatees1.len(), 1); assert_eq!(delegatees2.len(), 1); assert_eq!(del_delegatees.len(), 2); - assert!(delegatees1.contains_key(&validator1)); - assert!(delegatees2.contains_key(&validator2)); - assert!(del_delegatees.contains_key(&validator1)); - assert!(del_delegatees.contains_key(&validator2)); + assert!(delegatees1.contains(&validator1)); + assert!(delegatees2.contains(&validator2)); + assert!(del_delegatees.contains(&validator1)); + assert!(del_delegatees.contains(&validator2)); // Check everything again including the raw bond amount this time diff --git a/crates/sdk/src/queries/vp/pos.rs b/crates/sdk/src/queries/vp/pos.rs index 92b1e88b42..10cc677634 100644 --- a/crates/sdk/src/queries/vp/pos.rs +++ b/crates/sdk/src/queries/vp/pos.rs @@ -306,7 +306,7 @@ where H: 'static + StorageHasher + Sync, { let epoch = epoch.unwrap_or(ctx.state.in_mem().last_epoch); - let state = namada_proof_of_stake::queries::validator_state( + let state = namada_proof_of_stake::storage::read_validator_state( ctx.state, &validator, &epoch, )?; Ok((state, epoch)) @@ -573,10 +573,7 @@ where H: 'static + StorageHasher + Sync, { let epoch = epoch.unwrap_or(ctx.state.in_mem().last_epoch); - Ok(find_delegation_validators(ctx.state, &owner, &epoch)? - .keys() - .cloned() - .collect()) + find_delegation_validators(ctx.state, &owner, &epoch) } /// Find all the validator addresses to whom the given `owner` address has diff --git a/wasm/tx_vote_proposal/src/lib.rs b/wasm/tx_vote_proposal/src/lib.rs index 9f93120ab0..4175de8003 100644 --- a/wasm/tx_vote_proposal/src/lib.rs +++ b/wasm/tx_vote_proposal/src/lib.rs @@ -3,7 +3,6 @@ use namada_tx_prelude::action::{Action, GovAction, Write}; use namada_tx_prelude::gov_storage::keys::get_voting_start_epoch_key; use namada_tx_prelude::proof_of_stake::find_delegation_validators; -use namada_tx_prelude::proof_of_stake::types::ValidatorState; use namada_tx_prelude::*; #[transaction] @@ -39,16 +38,6 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { let delegations_targets = find_delegation_validators(ctx, &tx_data.voter, &proposal_start_epoch)?; - let active_delegations_targets = delegations_targets - .into_iter() - .filter_map(|(address, state)| { - if matches!(state, ValidatorState::Consensus) { - Some(address) - } else { - None - } - }) - .collect(); governance::vote_proposal(ctx, tx_data, active_delegations_targets) .wrap_err("Failed to vote on governance proposal") From 2d279bb338c325313a998233f3a15673da0de85c Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 29 Apr 2024 19:45:45 -0700 Subject: [PATCH 5/9] bug fix: ensure votes only to explicit active validators are counted --- crates/apps/src/lib/node/ledger/shell/governance.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/apps/src/lib/node/ledger/shell/governance.rs b/crates/apps/src/lib/node/ledger/shell/governance.rs index 3381cd4f8c..89a24f7f69 100644 --- a/crates/apps/src/lib/node/ledger/shell/governance.rs +++ b/crates/apps/src/lib/node/ledger/shell/governance.rs @@ -269,12 +269,22 @@ where let validator = vote.validator.clone(); let validator_state = validator_state_handle(&validator).get(storage, epoch, params)?; + if matches!( validator_state, Some(ValidatorState::Jailed) | Some(ValidatorState::Inactive) ) { continue; } + if validator_state.is_none() { + tracing::error!( + "While computing votes for proposal id {proposal_id} in epoch \ + {epoch}, encountered validator {validator} that has no \ + stored state. Please report this as a bug. Skipping this \ + vote." + ); + continue; + } // Tally the votes involving active validators if vote.is_validator() { From 5994a452947a8ede98511fe1802a8f551ff0f5dc Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 29 Apr 2024 19:47:02 -0700 Subject: [PATCH 6/9] clean up client code --- crates/sdk/src/tx.rs | 108 ++++++++++++------------------------------- 1 file changed, 29 insertions(+), 79 deletions(-) diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 1adafec031..5d6fcc3b8a 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2126,36 +2126,6 @@ pub async fn build_vote_proposal( let is_validator = rpc::is_validator(context.client(), voter_address).await?; - // Prevent jailed or inactive validators from voting - if is_validator { - let state = rpc::get_validator_state( - context.client(), - voter_address, - Some(current_epoch), - ) - .await? - .0 - .expect("Expected to find the state of the validator"); - - if matches!(state, ValidatorState::Jailed | ValidatorState::Inactive) { - edisplay_line!( - context.io(), - "The voter {} is a validator who is currently jailed or \ - inactive. Thus, this address is prohibited from voting in \ - governance right now.", - voter_address - ); - if !tx.force { - return Err(Error::from( - TxSubmitError::CannotVoteInGovernance( - voter_address.clone(), - current_epoch, - ), - )); - } - } - } - // Check if the voting period is still valid for the voter if !proposal.can_be_voted(current_epoch, is_validator) { edisplay_line!( @@ -2180,23 +2150,9 @@ pub async fn build_vote_proposal( } if is_validator { - let stake = - get_validator_stake(context.client(), current_epoch, voter_address) - .await?; - - if stake.is_zero() { - edisplay_line!( - context.io(), - "Voter address {voter_address} is a validator but has no \ - stake, so it has no votes.", - ); - if !tx.force { - return Err(Error::Other( - "Voter address must have delegations".to_string(), - )); - } - } - let val_state = rpc::get_validator_state( + // Prevent a validator voter from voting if they are jailed or inactive + // right now + let state = rpc::get_validator_state( context.client(), voter_address, Some(current_epoch), @@ -2205,15 +2161,14 @@ pub async fn build_vote_proposal( .0 .expect("Expected to find the state of the validator"); - if matches!( - val_state, - ValidatorState::Jailed | ValidatorState::Inactive - ) { + if matches!(state, ValidatorState::Jailed | ValidatorState::Inactive) { edisplay_line!( context.io(), - "Voter address {voter_address} is a validator that is either \ - jailed or inactive, and so it may not vote in governance at \ - this moment.", + "The voter {} is a validator who is currently jailed or \ + inactive. Thus, this address is prohibited from voting in \ + governance right now. Please try again when not jailed or \ + inactive.", + voter_address ); if !tx.force { return Err(Error::from( @@ -2224,37 +2179,32 @@ pub async fn build_vote_proposal( )); } } + + let stake = + get_validator_stake(context.client(), current_epoch, voter_address) + .await?; + + if stake.is_zero() { + edisplay_line!( + context.io(), + "Voter address {voter_address} is a validator but has no \ + stake, so it has no votes.", + ); + if !tx.force { + return Err(Error::Other( + "Voter address must have delegations".to_string(), + )); + } + } } else { - // Get active valid validators with whom the voter has delegations - // (bonds) - let delegation_vals = rpc::get_delegation_validators( + // Check that there are delegations to vote with + let delegation_validators = rpc::get_delegation_validators( context.client(), voter_address, - proposal.voting_start_epoch, + current_epoch, ) .await?; - let mut delegation_validators = Vec::
::new(); - for validator in delegation_vals { - let val_state = rpc::get_validator_state( - context.client(), - &validator, - Some(current_epoch), - ) - .await? - .0 - .expect("Expected to find the state of the validator"); - - if matches!( - val_state, - ValidatorState::Jailed | ValidatorState::Inactive - ) { - continue; - } - delegation_validators.push(validator.clone()); - } - - // Check that there are delegations to vote with if delegation_validators.is_empty() { edisplay_line!( context.io(), From 6d7cde77dc0d31f946590c54b7a8f72897138392 Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 29 Apr 2024 19:47:56 -0700 Subject: [PATCH 7/9] wasm tx: pass current delegations instead of those at proposal start --- wasm/tx_vote_proposal/src/lib.rs | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/wasm/tx_vote_proposal/src/lib.rs b/wasm/tx_vote_proposal/src/lib.rs index 4175de8003..bdf23a4e0f 100644 --- a/wasm/tx_vote_proposal/src/lib.rs +++ b/wasm/tx_vote_proposal/src/lib.rs @@ -1,7 +1,6 @@ //! A tx to vote on a proposal use namada_tx_prelude::action::{Action, GovAction, Write}; -use namada_tx_prelude::gov_storage::keys::get_voting_start_epoch_key; use namada_tx_prelude::proof_of_stake::find_delegation_validators; use namada_tx_prelude::*; @@ -23,22 +22,15 @@ fn apply_tx(ctx: &mut Ctx, tx_data: Tx) -> TxResult { voter: tx_data.voter.clone(), }))?; - let voting_start_epoch_key = get_voting_start_epoch_key(tx_data.id); - let proposal_start_epoch = ctx.read(&voting_start_epoch_key)?; - let proposal_start_epoch = if let Some(epoch) = proposal_start_epoch { - epoch - } else { - return Err(Error::new_alloc(format!( - "Proposal id {} doesn't have a start epoch", - tx_data.id - ))); - }; - debug_log!("apply_tx called to vote a governance proposal"); - let delegations_targets = - find_delegation_validators(ctx, &tx_data.voter, &proposal_start_epoch)?; + // Pass in all target validators to the proposal vote. Whether or not the + // vote will be counted based on the validator state will be determined + // when tallying the votes and executing the proposal. + let current_epoch = ctx.get_block_epoch()?; + let delegation_targets = + find_delegation_validators(ctx, &tx_data.voter, ¤t_epoch)?; - governance::vote_proposal(ctx, tx_data, active_delegations_targets) + governance::vote_proposal(ctx, tx_data, delegation_targets) .wrap_err("Failed to vote on governance proposal") } From 5295bb9ef1477502d5639c3e03d2545a1178b679 Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 29 Apr 2024 20:18:36 -0700 Subject: [PATCH 8/9] keep validator state longer in storage --- crates/proof_of_stake/src/types/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/proof_of_stake/src/types/mod.rs b/crates/proof_of_stake/src/types/mod.rs index 1d47e22176..76790e3665 100644 --- a/crates/proof_of_stake/src/types/mod.rs +++ b/crates/proof_of_stake/src/types/mod.rs @@ -65,7 +65,7 @@ pub type ValidatorEthColdKeys = crate::epoched::Epoched< pub type ValidatorStates = crate::epoched::Epoched< ValidatorState, crate::epoched::OffsetPipelineLen, - crate::epoched::OffsetDefaultNumPastEpochs, + crate::epoched::OffsetMaxProposalPeriodPlus, >; /// A map from a position to an address in a Validator Set From 48aba22219a1a553679e6fc2b6b9c739349099a5 Mon Sep 17 00:00:00 2001 From: brentstone Date: Wed, 1 May 2024 18:47:53 -0700 Subject: [PATCH 9/9] better documentation of proposal periods --- crates/apps/src/lib/config/genesis/templates.rs | 5 +++-- crates/governance/src/parameters.rs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/apps/src/lib/config/genesis/templates.rs b/crates/apps/src/lib/config/genesis/templates.rs index 38d2c6dc05..f4def8214a 100644 --- a/crates/apps/src/lib/config/genesis/templates.rs +++ b/crates/apps/src/lib/config/genesis/templates.rs @@ -444,9 +444,10 @@ pub struct GovernanceParams { pub min_proposal_fund: u64, /// Maximum size of proposal in kibibytes (KiB) pub max_proposal_code_size: u64, - /// Minimum proposal period length in epochs + /// Minimum number of epochs between the proposal end epoch and start epoch pub min_proposal_voting_period: u64, - /// Maximum proposal period length in epochs + /// Maximum number of epochs between the proposal activation epoch and + /// start epoch pub max_proposal_period: u64, /// Maximum number of characters in the proposal content pub max_proposal_content_size: u64, diff --git a/crates/governance/src/parameters.rs b/crates/governance/src/parameters.rs index eb4873c8af..f015c715f3 100644 --- a/crates/governance/src/parameters.rs +++ b/crates/governance/src/parameters.rs @@ -25,9 +25,10 @@ pub struct GovernanceParameters { pub min_proposal_fund: token::Amount, /// Maximum kibibyte length for proposal code pub max_proposal_code_size: u64, - /// Minimum proposal voting period in epochs + /// Minimum number of epochs between the proposal end epoch and start epoch pub min_proposal_voting_period: u64, - /// Maximum proposal voting period in epochs + /// Maximum number of epochs between the proposal activation epoch and + /// start epoch pub max_proposal_period: u64, /// Maximum number of characters for proposal content pub max_proposal_content_size: u64,