Skip to content

Commit

Permalink
gov: simplify governance-controlled parameter changes
Browse files Browse the repository at this point in the history 
This commit refactors the governance component to dramatically simplify the way
that governance-controlled parameter changes are performed.  Previously, the
governance component duplicated all of the parameter data of all of the other
components, which had to be kept in sync in a complicated and error-prone way.

Now, parameter changes are specified and performed as key-value edits:

- A parameter change proposal specifies a list of `(component, key, value)` pairs
- The governance component can apply these without knowledge of other components' structures by operating on a `serde_json::Value`
- The application can validate the changed parameters by deserializing a complete `AppParameters` from ProtoJSON.

The logic around applying these changes is also dramatically simplified. The
top-level app is responsible for all parameter changes. Components do not need
to track their own individual "dirty bit" for parameter changes. The governance
component can report to any other component when a parameter change was
scheduled to occur.

In followup work, we should change the `pcli` interface to work with JSON
proposal data and delete the `ProposalToml` entirely.
  • Loading branch information
@hdevalence
hdevalence committed May 4, 2024
1 parent db773d8 commit fc6fe29
Show file tree
Hide file tree
Showing 38 changed files with 832 additions and 676 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 6 additions & 17 deletions crates/bin/pcli/src/command/tx/proposal.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use anyhow::{Context, Result};

use penumbra_app::params::AppParameters;
use penumbra_governance::{proposal::ChangedAppParameters, Proposal, ProposalPayload};
use penumbra_governance::{change::ParameterChange, Proposal, ProposalPayload};
use penumbra_proto::DomainType;
use penumbra_transaction::TransactionPlan;

Expand Down Expand Up @@ -94,22 +94,11 @@ impl ProposalKindCmd {
let payload = match self {
ProposalKindCmd::Signaling => ProposalPayload::Signaling { commit: None },
ProposalKindCmd::Emergency => ProposalPayload::Emergency { halt_chain: false },
ProposalKindCmd::ParameterChange => ProposalPayload::ParameterChange {
old: Box::new(app_params.as_changed_params()),
new: Box::new(ChangedAppParameters {
auction_params: None,
community_pool_params: None,
distributions_params: None,
ibc_params: None,
fee_params: None,
funding_params: None,
governance_params: None,
sct_params: None,
shielded_pool_params: None,
stake_params: None,
dex_params: None,
}),
},
ProposalKindCmd::ParameterChange => {
ProposalPayload::ParameterChange(ParameterChange::encode_parameters(
serde_json::value::to_value(app_params.clone())?,
))
}
ProposalKindCmd::CommunityPoolSpend { transaction_plan } => {
if let Some(file) = transaction_plan {
ProposalPayload::CommunityPoolSpend {
Expand Down
Binary file modified crates/cnidarium/src/gen/proto_descriptor.bin.no_lfs
View file
Binary file not shown.
30 changes: 11 additions & 19 deletions crates/core/app/src/action_handler/actions/submit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ use penumbra_sct::component::tree::SctRead;
use penumbra_shielded_pool::component::AssetRegistry;
use penumbra_transaction::{AuthorizationData, Transaction, TransactionPlan, WitnessData};

use crate::action_handler::AppActionHandler;
use crate::app::StateReadExt;
use crate::community_pool_ext::CommunityPoolStateWriteExt;
use crate::params::AppParameters;
use crate::{action_handler::AppActionHandler, params::change::ParameterChangeExt as _};

// IMPORTANT: these length limits are enforced by consensus! Changing them will change which
// transactions are accepted by the network, and so they *cannot* be changed without a network
Expand Down Expand Up @@ -72,21 +72,7 @@ impl AppActionHandler for ProposalSubmit {
match payload {
Signaling { commit: _ } => { /* all signaling proposals are valid */ }
Emergency { halt_chain: _ } => { /* all emergency proposals are valid */ }
ParameterChange { old, new } => {
// Since the changed app parameters is a differential, we need to construct
// a complete AppParameters:
//
// `old_app_params` should be complete and represent the state of all app parameters
// at the time the proposal was created.
let old_app_params = AppParameters::from_changed_params(old, None)?;
// `new_app_params` should be sparse and only the components whose parameters were changed
// by the proposal should be `Some`.
let new_app_params =
AppParameters::from_changed_params(new, Some(&old_app_params))?;
old_app_params
.check_valid_update(&new_app_params)
.context("invalid change to app parameters")?;
}
ParameterChange(_change) => { /* no stateless checks -- see check-and-execute below */ }
CommunityPoolSpend { transaction_plan } => {
// Check to make sure that the transaction plan contains only valid actions for the
// Community Pool (none of them should require proving to build):
Expand Down Expand Up @@ -172,8 +158,14 @@ impl AppActionHandler for ProposalSubmit {
match &proposal.payload {
ProposalPayload::Signaling { .. } => { /* no stateful checks for signaling */ }
ProposalPayload::Emergency { .. } => { /* no stateful checks for emergency */ }
ProposalPayload::ParameterChange { .. } => {
/* no stateful checks for parameter change (checks are applied when proposal finishes) */
ProposalPayload::ParameterChange(change) => {
// Check that the parameter change is valid and could be applied to the current
// parameters. This doesn't guarantee that it will be valid when/if it passes but
// ensures that clearly malformed proposals are rejected upfront.
let current_parameters = state.get_app_params().await?;
change
.apply_changes(current_parameters)
.context("proposed parameter changes do not apply to current parameters")?;
}
ProposalPayload::CommunityPoolSpend { transaction_plan } => {
// If Community Pool spend proposals aren't enabled, then we can't allow them to be submitted
Expand Down
191 changes: 71 additions & 120 deletions crates/core/app/src/app/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ use penumbra_distributions::component::{Distributions, StateReadExt as _, StateW
use penumbra_fee::component::{Fee, StateReadExt as _, StateWriteExt as _};
use penumbra_funding::component::Funding;
use penumbra_funding::component::{StateReadExt as _, StateWriteExt as _};
use penumbra_governance::component::{Governance, StateReadExt as _};
use penumbra_governance::StateWriteExt as _;
use penumbra_governance::component::{Governance, StateReadExt as _, StateWriteExt as _};
use penumbra_ibc::component::{Ibc, StateWriteExt as _};
use penumbra_ibc::StateReadExt as _;
use penumbra_proto::core::app::v1::TransactionsByHeightResponse;
Expand All @@ -40,6 +39,7 @@ use tracing::Instrument;

use crate::action_handler::AppActionHandler;
use crate::genesis::AppState;
use crate::params::change::ParameterChangeExt as _;
use crate::params::AppParameters;
use crate::{CommunityPoolStateReadExt, PenumbraHost};

Expand Down Expand Up @@ -128,7 +128,7 @@ impl App {
Funding::init_chain(&mut state_tx, Some(&genesis.funding_content)).await;

state_tx
.finish_block(state_tx.app_params_updated())
.finish_block()
.await
.expect("must be able to finish compact block");
}
Expand Down Expand Up @@ -206,49 +206,34 @@ impl App {
pub async fn begin_block(&mut self, begin_block: &request::BeginBlock) -> Vec<abci::Event> {
let mut state_tx = StateDelta::new(self.state.clone());

// If a app parameter change is scheduled for this block, apply it here, before any other
// component has executed. This ensures that app parameter changes are consistently
// applied precisely at the boundary between blocks:
if let Some(app_params) = state_tx
.pending_app_parameters()
// If a app parameter change is scheduled for this block, apply it here,
// before any other component has executed. This ensures that app
// parameter changes are consistently applied precisely at the boundary
// between blocks.
//
// Note that because _nothing_ has executed yet, we need to get the
// current height from the begin_block request, rather than from the
// state (it will be set by the SCT component, which executes first).
if let Some(change) = state_tx
.param_changes_for_height(begin_block.header.height.into())
.await
.expect("app params should always be readable")
.expect("param changes should always be readable, even if unset")
{
tracing::info!(?app_params, "applying pending app parameters");
// The app parameters are sparse so only those which are `Some` need
// updating here
if let Some(community_pool_params) = app_params.new.community_pool_params {
state_tx.put_community_pool_params(community_pool_params);
}
if let Some(distributions_params) = app_params.new.distributions_params {
state_tx.put_distributions_params(distributions_params);
}
if let Some(fee_params) = app_params.new.fee_params {
state_tx.put_fee_params(fee_params);
}
if let Some(funding_params) = app_params.new.funding_params {
state_tx.put_funding_params(funding_params);
}
if let Some(governance_params) = app_params.new.governance_params {
state_tx.put_governance_params(governance_params);
}
if let Some(ibc_params) = app_params.new.ibc_params {
state_tx.put_ibc_params(ibc_params);
}
if let Some(shielded_pool_params) = app_params.new.shielded_pool_params {
state_tx.put_shielded_pool_params(shielded_pool_params);
}
if let Some(sct_params) = app_params.new.sct_params {
state_tx.put_sct_params(sct_params);
}
if let Some(stake_params) = app_params.new.stake_params {
state_tx.put_stake_params(stake_params);
}
if let Some(dex_params) = app_params.new.dex_params {
state_tx.put_dex_params(dex_params);
}
if let Some(auction_params) = app_params.new.auction_params {
state_tx.put_auction_params(auction_params);
let old_params = state_tx
.get_app_params()
.await
.expect("must be able to read app params");
match change.apply_changes(old_params) {
Ok(new_params) => {
tracing::info!(?change, ?new_params, "applied app parameter change");
state_tx.put_app_params(new_params);
}
Err(e) => {
// N.B. this is an "info" rather than "warn" because it does not report
// a problem with _this instance of the application_, but rather is an expected
// behavior.
tracing::info!(?change, ?e, "failed to apply approved app parameter change");
}
}
}

Expand Down Expand Up @@ -404,67 +389,6 @@ impl App {
.expect("components did not retain copies of shared state");
tracing::debug!("finished app components' `end_block` hooks");

// Validate governance proposals here. We must do this here because proposals can affect
// the entirety of application state, and the governance component does not have access to
// the types defined in this crate.
//
// If a proposal was passed in this block, then `schedule_app_param_update` was called
// which will set `next_block_pending_app_parameters`.
//
// If any validation here fails, the `next_block_pending_app_parameters` will be cleared,
// and no change will be enacted during the next block's `begin_block`.
if let Some(params) = self
.state
.next_block_pending_app_parameters()
.await
.expect("should be able to read next block pending app parameters")
{
// If there has been a chain upgrade while the proposal was pending, the stateless
// verification criteria for the parameter change proposal could have changed, so we
// should check them again here, just to be sure:
// `old_app_params` should be complete and represent the state of all app parameters
// at the time the proposal was created.
let old_app_params = AppParameters::from_changed_params(&params.old, None)
.expect("should be able to parse old app params");
// `new_app_params` should be sparse and only the components whose parameters were
// changed by the proposal should be `Some`.
let new_app_params =
AppParameters::from_changed_params(&params.new, Some(&old_app_params))
.expect("should be able to parse new app params");
if old_app_params.check_valid_update(&new_app_params).is_err() {
// An error occurred validating the parameter change, we do not want to enact it.
// Wipe the next block pending app parameters so the change doesn't get applied.
tracing::warn!(
?new_app_params,
"parameter change proposal failed validation, wiping pending parameters"
);
state_tx
.cancel_next_block_pending_app_parameters()
.await
.expect("able to cancel next block pending app parameters");
} else {
// This was a valid change.
//
// Check that the old parameters are an exact match for the current parameters, or
// else abort the update.
let current = self
.state
.get_app_params()
.await
.expect("able to fetch app params");

// The current parameters have to match the old parameters specified in the
// proposal, exactly. This prevents updates from clashing.
if old_app_params != current {
tracing::warn!("current chain parameters do not match the old parameters in the proposal, canceling proposal enactment");
state_tx
.cancel_next_block_pending_app_parameters()
.await
.expect("able to cancel next block pending app parameters");
}
}
}

let current_height = state_tx
.get_block_height()
.await
Expand Down Expand Up @@ -532,7 +456,7 @@ impl App {
.expect("components did not retain copies of shared state");

state_tx
.finish_epoch(state_tx.app_params_updated())
.finish_epoch()
.await
.expect("must be able to finish compact block");

Expand All @@ -556,7 +480,7 @@ impl App {
);

state_tx
.finish_block(state_tx.app_params_updated())
.finish_block()
.await
.expect("must be able to finish compact block");

Expand Down Expand Up @@ -638,19 +562,6 @@ const TOTAL_HALT_COUNT: u64 = 2;

#[async_trait]
pub trait StateReadExt: StateRead {
/// Returns true if the app parameters have been changed in this block.
fn app_params_updated(&self) -> bool {
self.community_pool_params_updated()
|| self.distributions_params_updated()
|| self.ibc_params_updated()
|| self.fee_params_updated()
|| self.funding_params_updated()
|| self.governance_params_updated()
|| self.sct_params_updated()
|| self.shielded_pool_params_updated()
|| self.stake_params_updated()
}

async fn get_chain_id(&self) -> Result<String> {
let raw_chain_id = self
.get_raw(state_key::data::chain_id())
Expand Down Expand Up @@ -785,6 +696,46 @@ pub trait StateWriteExt: StateWrite {
);
Ok(())
}

/// Writes the app parameters to the state.
///
/// Each component stores its own parameters separately, so this method
/// splits up the provided parameters structure and writes it out to each component.
fn put_app_params(&mut self, params: AppParameters) {
// To make sure we don't forget to write any parts, destructure the entire params
let AppParameters {
chain_id,
auction_params,
community_pool_params,
distributions_params,
fee_params,
funding_params,
governance_params,
ibc_params,
sct_params,
shielded_pool_params,
stake_params,
dex_params,
} = params;

// Ignore writes to the chain_id
// TODO(erwan): we are momentarily not supporting chain_id changes
// until the IBC host chain changes land.
// See: https://github.com/penumbra-zone/penumbra/issues/3617#issuecomment-1917708221
std::mem::drop(chain_id);

self.put_auction_params(auction_params);
self.put_community_pool_params(community_pool_params);
self.put_distributions_params(distributions_params);
self.put_fee_params(fee_params);
self.put_funding_params(funding_params);
self.put_governance_params(governance_params);
self.put_ibc_params(ibc_params);
self.put_sct_params(sct_params);
self.put_shielded_pool_params(shielded_pool_params);
self.put_stake_params(stake_params);
self.put_dex_params(dex_params);
}
}

impl<T: StateWrite + ?Sized> StateWriteExt for T {}
Loading

0 comments on commit fc6fe29

Please sign in to comment.