Improve governance voting logic

.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))

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,

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.

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() {

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,

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,

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,

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,

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<S>(storage: &mut S, data: VoteProposalData) -> Result<()>
+pub fn vote_proposal<S>(
+    storage: &mut S,
+    data: VoteProposalData,
+    delegation_targets: HashSet<Address>,
+) -> 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(),

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<Address>,
 }
 
 impl TryFrom<DefaultProposal> 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
             }
         }
     }

crates/light_sdk/src/transaction/governance.rs

@@ -106,15 +106,10 @@ impl VoteProposal {
         id: u64,
         vote: ProposalVote,
         voter: Address,
-        delegations: Vec<Address>,
         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,

crates/namada/src/ledger/governance/mod.rs

@@ -13,7 +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_delegations;
 use namada_state::{StateRead, StorageRead};
 use namada_tx::action::{Action, GovAction, Read};
 use namada_tx::Tx;
@@ -303,37 +302,65 @@ 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, &current_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 {
-            return Err(native_vp::Error::new_alloc(format!(
-                "Failed to query delegations for {voter}"
-            ))
-            .into());
-        };
-        if !all_delegations_are_valid {
-            return Err(native_vp::Error::new_alloc(format!(
-                "Not all delegations of {voter} were deemed valid"
-            ))
-            .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

crates/proof_of_stake/src/queries.rs

@@ -37,7 +37,7 @@ where
         return Ok(HashSet::new());
     }
 
-    let mut delegation_targets = HashSet::<Address>::new();
+    let mut delegation_targets = HashSet::new();
 
     for validator in validators.iter(storage)? {
         let (
@@ -56,7 +56,7 @@ where
                     delegation_targets.insert(val);
                 }
             } else {
-                // this is bond is currently held
+                // this bond is currently held
                 delegation_targets.insert(val);
             }
         } else {

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<S>(
+    storage: &S,
+    validator: &Address,
+    epoch: &Epoch,
+) -> namada_storage::Result<Option<ValidatorState>>
+where
+    S: StorageRead,
+{
+    let params = read_pos_params(storage)?;
+    validator_state_handle(validator).get(storage, *epoch, &params)
+}
+
 /// Read PoS validator's delta value.
 pub fn read_validator_deltas_value<S>(
     storage: &S,

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

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, &params)?;
+    let state = namada_proof_of_stake::storage::read_validator_state(
+        ctx.state, &validator, &epoch,
+    )?;
     Ok((state, epoch))
 }
 

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()