Merge branch 'brent/refactor-proposal-loading' into brent/draft+pos

* brent/refactor-proposal-loading:
  changelog: add #2630
  fmt
  rename `grace_epoch` to `activation_epoch`
  refactor loading of gov proposals for execution

.changelog/unreleased/improvements/2630-refactor-proposal-loading.md

@@ -0,0 +1,3 @@
+- Only load governance proposals on a new epoch right before execution.
+  Decoupled the logic from the Shell and implemented in the gov crate.
+  ([\#2630](https://github.com/anoma/namada/pull/2630))

crates/apps/src/lib/bench_utils.rs

@@ -255,7 +255,7 @@ impl Default for BenchShell {
                 r#type: ProposalType::Default,
                 voting_start_epoch,
                 voting_end_epoch: voting_start_epoch + 3_u64,
-                grace_epoch: voting_start_epoch + 9_u64,
+                activation_epoch: voting_start_epoch + 9_u64,
             },
             None,
             Some(vec![content_section]),

crates/apps/src/lib/config/genesis/templates.rs

@@ -450,7 +450,7 @@ pub struct GovernanceParams {
     pub max_proposal_period: u64,
     /// Maximum number of characters in the proposal content
     pub max_proposal_content_size: u64,
-    /// Minimum number of epoch between end and grace epoch
+    /// Minimum number of epochs between the end and activation epochs
     pub min_proposal_grace_epochs: u64,
 }
 

crates/apps/src/lib/node/ledger/mod.rs

@@ -9,7 +9,6 @@ pub mod tendermint_node;
 use std::convert::TryInto;
 use std::net::SocketAddr;
 use std::path::PathBuf;
-use std::str::FromStr;
 use std::thread;
 
 use byte_unit::Byte;
@@ -18,7 +17,6 @@ use futures::future::TryFutureExt;
 use namada::core::storage::{BlockHeight, Key};
 use namada::core::time::DateTimeUtc;
 use namada::eth_bridge::ethers::providers::{Http, Provider};
-use namada::governance::storage::keys as governance_storage;
 use namada::state::DB;
 use namada::storage::DbColFam;
 use namada::tendermint::abci::request::CheckTxKind;
@@ -68,34 +66,6 @@ const ENV_VAR_RAYON_THREADS: &str = "NAMADA_RAYON_THREADS";
 //     }
 //```
 impl Shell {
-    fn load_proposals(&mut self) {
-        let proposals_key = governance_storage::get_commiting_proposals_prefix(
-            self.state.in_mem().last_epoch.0,
-        );
-
-        let (proposal_iter, _) = self.state.db_iter_prefix(&proposals_key);
-        for (key, _, _) in proposal_iter {
-            let key =
-                Key::from_str(key.as_str()).expect("Key should be parsable");
-            if governance_storage::get_commit_proposal_epoch(&key).unwrap()
-                != self.state.in_mem().last_epoch.0
-            {
-                // NOTE: `iter_prefix` iterate over the matching prefix. In this
-                // case  a proposal with grace_epoch 110 will be
-                // matched by prefixes  1, 11 and 110. Thus we
-                // have to skip to the next iteration of
-                //  the cycle for all the prefixes that don't actually match
-                //  the desired epoch.
-                continue;
-            }
-
-            let proposal_id = governance_storage::get_commit_proposal_id(&key);
-            if let Some(id) = proposal_id {
-                self.proposal_data.insert(id);
-            }
-        }
-    }
-
     fn call(&mut self, req: Request) -> Result<Response, Error> {
         match req {
             Request::InitChain(init) => {
@@ -135,7 +105,6 @@ impl Shell {
             }
             Request::FinalizeBlock(finalize) => {
                 tracing::debug!("Request FinalizeBlock");
-                self.load_proposals();
                 self.finalize_block(finalize).map(Response::FinalizeBlock)
             }
             Request::Commit => {

crates/apps/src/lib/node/ledger/shell/finalize_block.rs

@@ -102,7 +102,12 @@ where
         // Sub-system updates:
         // - Governance - applied first in case a proposal changes any of the
         //   other syb-systems
-        governance::finalize_block(self, emit_events, new_epoch)?;
+        governance::finalize_block(
+            self,
+            emit_events,
+            current_epoch,
+            new_epoch,
+        )?;
         // - Token
         token::finalize_block(&mut self.state, emit_events, new_epoch)?;
         // - PoS
@@ -1225,15 +1230,14 @@ mod test_finalize_block {
         // Add a proposal to be executed on next epoch change.
         let mut add_proposal = |proposal_id, vote| {
             let validator = shell.mode.get_validator_address().unwrap().clone();
-            shell.proposal_data.insert(proposal_id);
 
             let proposal = InitProposalData {
                 content: Hash::default(),
                 author: validator.clone(),
                 voting_start_epoch: Epoch::default(),
                 voting_end_epoch: Epoch::default().next(),
-                grace_epoch: Epoch::default().next(),
-                r#type: ProposalType::Default,
+                activation_epoch: Epoch::default().next(),
+                r#type: ProposalType::Default(None),
             };
 
             namada::governance::init_proposal(

crates/apps/src/lib/node/ledger/shell/governance.rs

@@ -5,10 +5,10 @@ use namada::core::storage::Epoch;
 use namada::governance::pgf::storage::keys as pgf_storage;
 use namada::governance::pgf::storage::steward::StewardDetail;
 use namada::governance::pgf::{storage as pgf, ADDRESS};
-use namada::governance::storage::keys as gov_storage;
 use namada::governance::storage::proposal::{
     AddRemove, PGFAction, PGFTarget, ProposalType, StoragePgfFunding,
 };
+use namada::governance::storage::{keys as gov_storage, load_proposals};
 use namada::governance::utils::{
     compute_proposal_result, ProposalVotes, TallyResult, TallyType, VotePower,
 };
@@ -31,14 +31,15 @@ use super::*;
 pub fn finalize_block<D, H>(
     shell: &mut Shell<D, H>,
     events: &mut impl EmitEvents,
+    current_epoch: Epoch,
     is_new_epoch: bool,
 ) -> Result<()>
 where
     D: 'static + DB + for<'iter> DBIter<'iter> + Sync,
     H: 'static + StorageHasher + Sync,
 {
     if is_new_epoch {
-        execute_governance_proposals(shell, events)?;
+        load_and_execute_governance_proposals(shell, events, current_epoch)?;
     }
     Ok(())
 }
@@ -49,17 +50,35 @@ pub struct ProposalsResult {
     rejected: Vec<u64>,
 }
 
+pub fn load_and_execute_governance_proposals<D, H>(
+    shell: &mut Shell<D, H>,
+    events: &mut impl EmitEvents,
+    current_epoch: Epoch,
+) -> Result<ProposalsResult>
+where
+    D: DB + for<'iter> DBIter<'iter> + Sync + 'static,
+    H: StorageHasher + Sync + 'static,
+{
+    let proposal_ids = load_proposals(&shell.state, current_epoch)?;
+
+    let proposals_result =
+        execute_governance_proposals(shell, events, proposal_ids)?;
+
+    Ok(proposals_result)
+}
+
 fn execute_governance_proposals<D, H>(
     shell: &mut Shell<D, H>,
     events: &mut impl EmitEvents,
+    proposal_ids: BTreeSet<u64>,
 ) -> Result<ProposalsResult>
 where
     D: DB + for<'iter> DBIter<'iter> + Sync + 'static,
     H: StorageHasher + Sync + 'static,
 {
     let mut proposals_result = ProposalsResult::default();
 
-    for id in std::mem::take(&mut shell.proposal_data) {
+    for id in proposal_ids {
         let proposal_funds_key = gov_storage::get_funds_key(id);
         let proposal_end_epoch_key = gov_storage::get_voting_end_epoch_key(id);
         let proposal_type_key = gov_storage::get_proposal_type_key(id);

crates/apps/src/lib/node/ledger/shell/mod.rs

@@ -350,8 +350,6 @@ where
     /// limit the how many block heights in the past can the storage be
     /// queried for reading values.
     storage_read_past_height_limit: Option<u64>,
-    /// Proposal execution tracking
-    pub proposal_data: BTreeSet<u64>,
     /// Log of events emitted by `FinalizeBlock` ABCI calls.
     event_log: EventLog,
 }
@@ -527,7 +525,6 @@ where
                 tx_wasm_compilation_cache as usize,
             ),
             storage_read_past_height_limit,
-            proposal_data: BTreeSet::new(),
             // TODO: config event log params
             event_log: EventLog::default(),
         };

crates/benches/native_vps.rs

@@ -133,7 +133,7 @@ fn governance(c: &mut Criterion) {
                         r#type: ProposalType::Default,
                         voting_start_epoch,
                         voting_end_epoch: voting_start_epoch + 3_u64,
-                        grace_epoch: voting_start_epoch + 9_u64,
+                        activation_epoch: voting_start_epoch + 9_u64,
                     },
                     None,
                     Some(vec![content_section]),
@@ -186,7 +186,7 @@ fn governance(c: &mut Criterion) {
                         ),
                         voting_start_epoch,
                         voting_end_epoch: voting_start_epoch + 3_u64,
-                        grace_epoch: voting_start_epoch + 9_u64,
+                        activation_epoch: voting_start_epoch + 9_u64,
                     },
                     None,
                     Some(vec![content_section, wasm_code_section]),
@@ -258,7 +258,7 @@ fn governance(c: &mut Criterion) {
 //              r#type: ProposalType::Default(None),
 //              voting_start_epoch: 12.into(),
 //              voting_end_epoch: 15.into(),
-//              grace_epoch: 18.into(),
+//              activation_epoch: 18.into(),
 //          },
 //          None,
 //          Some(vec![content_section]),

crates/benches/txs.rs

@@ -465,7 +465,7 @@ fn init_proposal(c: &mut Criterion) {
                                     r#type: ProposalType::Default,
                                     voting_start_epoch: 12.into(),
                                     voting_end_epoch: 15.into(),
-                                    grace_epoch: 18.into(),
+                                    activation_epoch: 18.into(),
                                 },
                                 None,
                                 Some(vec![content_section]),
@@ -516,7 +516,7 @@ fn init_proposal(c: &mut Criterion) {
                                     ),
                                     voting_start_epoch: 12.into(),
                                     voting_end_epoch: 15.into(),
-                                    grace_epoch: 18.into(),
+                                    activation_epoch: 18.into(),
                                 },
                                 None,
                                 Some(vec![content_section, wasm_code_section]),

crates/governance/src/cli/onchain.rs

@@ -11,10 +11,10 @@ use namada_migrations::*;
 use serde::{Deserialize, Serialize};
 
 use super::validation::{
-    is_valid_author_balance, is_valid_content, is_valid_default_proposal_data,
-    is_valid_end_epoch, is_valid_grace_epoch, is_valid_pgf_funding_data,
-    is_valid_pgf_stewards_data, is_valid_proposal_period, is_valid_start_epoch,
-    ProposalValidation,
+    is_valid_activation_epoch, is_valid_author_balance, is_valid_content,
+    is_valid_default_proposal_data, is_valid_end_epoch,
+    is_valid_pgf_funding_data, is_valid_pgf_stewards_data,
+    is_valid_proposal_period, is_valid_start_epoch, ProposalValidation,
 };
 use crate::parameters::GovernanceParameters;
 use crate::storage::proposal::PGFTarget;
@@ -34,12 +34,12 @@ pub struct OnChainProposal {
     pub content: BTreeMap<String, String>,
     /// The proposal author address
     pub author: Address,
-    /// The epoch from which voting is allowed
+    /// The epoch in which voting begins
     pub voting_start_epoch: Epoch,
-    /// The epoch from which voting is stopped
+    /// The final epoch in which voting is allowed
     pub voting_end_epoch: Epoch,
-    /// The epoch from which this changes are executed
-    pub grace_epoch: Epoch,
+    /// The epoch in which any changes are executed and become active
+    pub activation_epoch: Epoch,
 }
 
 /// Pgf default proposal
@@ -84,14 +84,14 @@ impl DefaultProposal {
             governance_parameters.min_proposal_voting_period,
             governance_parameters.max_proposal_period,
         )?;
-        is_valid_grace_epoch(
-            self.proposal.grace_epoch,
+        is_valid_activation_epoch(
+            self.proposal.activation_epoch,
             self.proposal.voting_end_epoch,
             governance_parameters.min_proposal_grace_epochs,
         )?;
         is_valid_proposal_period(
             self.proposal.voting_start_epoch,
-            self.proposal.grace_epoch,
+            self.proposal.activation_epoch,
             governance_parameters.max_proposal_period,
         )?;
         is_valid_author_balance(
@@ -162,14 +162,14 @@ impl PgfStewardProposal {
             governance_parameters.min_proposal_voting_period,
             governance_parameters.max_proposal_period,
         )?;
-        is_valid_grace_epoch(
-            self.proposal.grace_epoch,
+        is_valid_activation_epoch(
+            self.proposal.activation_epoch,
             self.proposal.voting_end_epoch,
             governance_parameters.min_proposal_grace_epochs,
         )?;
         is_valid_proposal_period(
             self.proposal.voting_start_epoch,
-            self.proposal.grace_epoch,
+            self.proposal.activation_epoch,
             governance_parameters.max_proposal_period,
         )?;
         is_valid_author_balance(
@@ -235,14 +235,14 @@ impl PgfFundingProposal {
             governance_parameters.min_proposal_voting_period,
             governance_parameters.max_proposal_period,
         )?;
-        is_valid_grace_epoch(
-            self.proposal.grace_epoch,
+        is_valid_activation_epoch(
+            self.proposal.activation_epoch,
             self.proposal.voting_end_epoch,
             governance_parameters.min_proposal_grace_epochs,
         )?;
         is_valid_proposal_period(
             self.proposal.voting_start_epoch,
-            self.proposal.grace_epoch,
+            self.proposal.activation_epoch,
             governance_parameters.max_proposal_period,
         )?;
         is_valid_content(

crates/governance/src/cli/validation.rs

@@ -26,16 +26,16 @@ pub enum ProposalValidation {
          a multiple of {0}"
     )]
     InvalidStartEndDifference(u64, u64),
-    /// The proposal difference between end and grace epoch is invalid
+    /// The proposal difference between end and activation epoch is invalid
     #[error(
-        "Invalid proposal grace epoch: difference between proposal grace and \
-         end epoch must be at least {0}, but found {1}"
+        "Invalid proposal activation epoch: difference between proposal \
+         activation and end epoch must be at least {0}, but found {1}"
     )]
-    InvalidEndGraceDifference(u64, u64),
-    /// The proposal difference between end and grace epoch is invalid
+    InvalidEndActivationDifference(u64, u64),
+    /// The proposal difference between end and activation epoch is invalid
     #[error(
-        "Invalid proposal period: difference between proposal start and grace \
-         epoch must be at most {1}, but found {0}"
+        "Invalid proposal period: difference between proposal start and \
+         activation epoch must be at most {1}, but found {0}"
     )]
     InvalidProposalPeriod(u64, u64),
     /// The proposal author does not have enough balance to pay for proposal
@@ -124,29 +124,29 @@ pub fn is_valid_end_epoch(
     }
 }
 
-pub fn is_valid_grace_epoch(
-    proposal_grace_epoch: Epoch,
+pub fn is_valid_activation_epoch(
+    proposal_activation_epoch: Epoch,
     proposal_end_epoch: Epoch,
-    min_proposal_grace_epoch: u64,
+    min_proposal_grace_epochs: u64,
 ) -> Result<(), ProposalValidation> {
-    let grace_period = proposal_grace_epoch.0 - proposal_end_epoch.0;
+    let grace_period = proposal_activation_epoch.0 - proposal_end_epoch.0;
 
-    if grace_period > 0 && grace_period >= min_proposal_grace_epoch {
+    if grace_period > 0 && grace_period >= min_proposal_grace_epochs {
         Ok(())
     } else {
-        Err(ProposalValidation::InvalidEndGraceDifference(
-            min_proposal_grace_epoch,
+        Err(ProposalValidation::InvalidEndActivationDifference(
+            min_proposal_grace_epochs,
             grace_period,
         ))
     }
 }
 
 pub fn is_valid_proposal_period(
     proposal_start_epoch: Epoch,
-    proposal_grace_epoch: Epoch,
+    proposal_activation_epoch: Epoch,
     max_proposal_period: u64,
 ) -> Result<(), ProposalValidation> {
-    let proposal_period = proposal_grace_epoch.0 - proposal_start_epoch.0;
+    let proposal_period = proposal_activation_epoch.0 - proposal_start_epoch.0;
 
     if proposal_period > 0 && proposal_period <= max_proposal_period {
         Ok(())