From 15ffacc6485ab997f080df9e9f9f7a79e24b9e55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 10:22:09 +0200 Subject: [PATCH 01/10] parameters: env agnostic VP --- Cargo.lock | 2 +- crates/benches/native_vps.rs | 25 ++++++---- crates/node/src/protocol.rs | 15 +++--- crates/node/src/shell/finalize_block.rs | 15 +++--- crates/parameters/Cargo.toml | 2 +- crates/parameters/src/vp.rs | 57 ++++++--------------- crates/sdk/src/validation.rs | 11 ++--- crates/vp_env/src/lib.rs | 66 ++++++++----------------- wasm/Cargo.lock | 2 +- wasm_for_tests/Cargo.lock | 2 +- 10 files changed, 76 insertions(+), 121 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 171f8e4c26..7fbd5780e1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5233,7 +5233,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "smooth-operator", "thiserror", ] diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index 4f4ab800c3..f91b5ada11 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -62,9 +62,14 @@ use namada_node::bench_utils::{ TX_BRIDGE_POOL_WASM, TX_IBC_WASM, TX_INIT_PROPOSAL_WASM, TX_RESIGN_STEWARD, TX_TRANSFER_WASM, TX_UPDATE_STEWARD_COMMISSION, TX_VOTE_PROPOSAL_WASM, }; -use namada_vp::native_vp::{Ctx, NativeVp}; +use namada_vm::wasm::run::VpEvalWasm; +use namada_vm::wasm::VpCache; +use namada_vp::native_vp::{self, NativeVp}; use rand_core::OsRng; +type Ctx<'ctx, S, D, H, CA> = + native_vp::Ctx<'ctx, S, VpCache, VpEvalWasm>; + fn governance(c: &mut Criterion) { let mut group = c.benchmark_group("vp_governance"); @@ -1557,7 +1562,7 @@ fn parameters(c: &mut Criterion) { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let parameters = ParametersVp::new(Ctx::new( + let ctx = Ctx::new( &vp_address, &shell.state, &signed_tx.tx, @@ -1567,18 +1572,18 @@ fn parameters(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); group.bench_function(bench_name, |b| { b.iter(|| { assert!( - parameters - .validate_tx( - &signed_tx.to_ref(), - parameters.ctx.keys_changed, - parameters.ctx.verifiers, - ) - .is_ok() + ParametersVp::validate_tx( + &ctx, + &signed_tx.to_ref(), + ctx.keys_changed, + ctx.verifiers, + ) + .is_ok() ) }) }); diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index f9bcde8fa8..1ace8b45bf 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1238,14 +1238,13 @@ where .map_err(Error::NativeVpError) } InternalAddress::Parameters => { - let parameters = ParametersVp::new(ctx); - parameters - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) + ParametersVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError) } InternalAddress::PosSlashPool => { Err(Error::AccessForbidden( diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 04c816470e..8acc54e240 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -1339,8 +1339,8 @@ mod test_finalize_block { use namada_sdk::validation::ParametersVp; use namada_test_utils::tx_data::TxWriteData; use namada_test_utils::TestWasms; + use namada_vm::wasm::run::VpEvalWasm; use namada_vote_ext::ethereum_events; - use namada_vp::native_vp::NativeVp; use proof_of_stake::{bond_tokens, PosParams}; use test_log::test; @@ -5650,7 +5650,7 @@ mod test_finalize_block { let keys_changed = BTreeSet::from([min_confirmations_key()]); let verifiers = BTreeSet::default(); let batched_tx = tx.batch_ref_first_tx().unwrap(); - let ctx = namada_vp::native_vp::Ctx::new( + let ctx = namada_vp::native_vp::Ctx::<_, _, VpEvalWasm<_, _, _>>::new( shell.mode.get_validator_address().expect("Test failed"), shell.state.read_only(), batched_tx.tx, @@ -5661,11 +5661,14 @@ mod test_finalize_block { &verifiers, shell.vp_wasm_cache.clone(), ); - let parameters = ParametersVp::new(ctx); assert!( - parameters - .validate_tx(&batched_tx, &keys_changed, &verifiers) - .is_ok() + ParametersVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ) + .is_ok() ); // we advance forward to the next epoch diff --git a/crates/parameters/Cargo.toml b/crates/parameters/Cargo.toml index de2ad221d6..214a5fab6f 100644 --- a/crates/parameters/Cargo.toml +++ b/crates/parameters/Cargo.toml @@ -25,7 +25,7 @@ namada_macros = { path = "../macros" } namada_state = { path = "../state" } namada_systems = { path = "../systems" } namada_tx = { path = "../tx" } -namada_vp = { path = "../vp" } +namada_vp_env = { path = "../vp_env" } smooth-operator.workspace = true thiserror.workspace = true diff --git a/crates/parameters/src/vp.rs b/crates/parameters/src/vp.rs index 3d34467a30..50f3b462b9 100644 --- a/crates/parameters/src/vp.rs +++ b/crates/parameters/src/vp.rs @@ -1,40 +1,30 @@ -//! Native VP for protocol parameters +//! VP for protocol parameters use std::collections::BTreeSet; use std::marker::PhantomData; use namada_core::address::Address; use namada_core::booleans::BoolResultUnitExt; -use namada_state::{Key, StateRead}; use namada_systems::governance; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{ - Ctx, CtxPreStorageRead, Error, NativeVp, Result, VpEvaluator, -}; +use namada_vp_env::{Error, Key, Result, VpEnv}; use crate::storage; /// Parameters VP -pub struct ParametersVp<'ctx, S, CA, EVAL, Gov> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, - /// Generic types for DI - pub _marker: PhantomData, +pub struct ParametersVp<'ctx, CTX, Gov> { + /// Generic types for VP context and DI + pub _marker: PhantomData<(&'ctx CTX, Gov)>, } -impl<'view, 'ctx: 'view, S, CA, EVAL, Gov> NativeVp<'view> - for ParametersVp<'ctx, S, CA, EVAL, Gov> +impl<'ctx, CTX, Gov> ParametersVp<'ctx, CTX, Gov> where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Gov: governance::Read>, + CTX: VpEnv<'ctx>, + Gov: governance::Read<>::Pre>, { - fn validate_tx( - &'view self, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, keys_changed: &BTreeSet, _verifiers: &BTreeSet
, @@ -45,20 +35,21 @@ where data } else { return Err(Error::new_const( - "Token parameter changes require tx data to be present", + "Parameter changes require tx data to be present", )); }; match key_type { KeyType::PARAMETER | KeyType::UNKNOWN_PARAMETER => { - Gov::is_proposal_accepted(&self.ctx.pre(), &data)? - .ok_or_else(|| { + Gov::is_proposal_accepted(&ctx.pre(), &data)?.ok_or_else( + || { Error::new_alloc(format!( "Attempted to change a protocol parameter \ from outside of a governance proposal, or \ from a non-accepted governance proposal: \ {key}", )) - }) + }, + ) } KeyType::UNKNOWN => Ok(()), } @@ -66,22 +57,6 @@ where } } -impl<'ctx, S, CA, EVAL, Gov> ParametersVp<'ctx, S, CA, EVAL, Gov> -where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Gov: governance::Read>, -{ - /// Instantiate parameters VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, - } - } -} - #[allow(clippy::upper_case_acronyms)] enum KeyType { #[allow(clippy::upper_case_acronyms)] diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index c1c8d8ab13..eb82ce7e51 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -4,6 +4,7 @@ use namada_vm::wasm::run::VpEvalWasm; use namada_vm::wasm::VpCache; use namada_vp::native_vp::{self, CtxPostStorageRead, CtxPreStorageRead}; +use namada_vp::VpEnv; use crate::state::StateRead; use crate::{eth_bridge, governance, ibc, parameters, proof_of_stake, token}; @@ -51,12 +52,10 @@ pub type IbcVpContext<'view, 'a, S, CA, EVAL> = >; /// Native parameters VP -pub type ParametersVp<'a, S, CA> = parameters::vp::ParametersVp< - 'a, - S, - VpCache, - Eval, - GovPreStore<'a, S, CA>, +pub type ParametersVp<'ctx, CTX> = parameters::vp::ParametersVp< + 'ctx, + CTX, + governance::Store<>::Pre>, >; /// Native governance VP diff --git a/crates/vp_env/src/lib.rs b/crates/vp_env/src/lib.rs index 7b1dab928c..7f3a1abbec 100644 --- a/crates/vp_env/src/lib.rs +++ b/crates/vp_env/src/lib.rs @@ -25,10 +25,9 @@ use namada_core::borsh::BorshDeserialize; use namada_core::chain::ChainId; pub use namada_core::chain::{BlockHeader, BlockHeight, Epoch, Epochs}; use namada_core::hash::Hash; -pub use namada_core::storage::{Key, TxIndex}; use namada_events::{Event, EventType}; use namada_gas::Gas; -pub use namada_storage::StorageRead; +pub use namada_storage::{Error, Key, Result, StorageRead, TxIndex}; use namada_tx::BatchedTxRef; /// Validity predicate's environment is available for native VPs and WASM VPs @@ -56,56 +55,47 @@ where /// Storage read temporary state Borsh encoded value (after tx execution). /// It will try to read from only the write log and then decode it if /// found. - fn read_temp( - &self, - key: &Key, - ) -> Result, namada_storage::Error>; + fn read_temp(&self, key: &Key) -> Result>; /// Storage read temporary state raw bytes (after tx execution). It will try /// to read from only the write log. - fn read_bytes_temp( - &self, - key: &Key, - ) -> Result>, namada_storage::Error>; + fn read_bytes_temp(&self, key: &Key) -> Result>>; /// Getting the chain ID. - fn get_chain_id(&self) -> Result; + fn get_chain_id(&self) -> Result; /// Getting the block height. The height is that of the block to which the /// current transaction is being applied. - fn get_block_height(&self) -> Result; + fn get_block_height(&self) -> Result; /// Getting the block header. fn get_block_header( &self, height: BlockHeight, - ) -> Result, namada_storage::Error>; + ) -> Result>; /// Getting the block epoch. The epoch is that of the block to which the /// current transaction is being applied. - fn get_block_epoch(&self) -> Result; + fn get_block_epoch(&self) -> Result; /// Get the shielded transaction index. - fn get_tx_index(&self) -> Result; + fn get_tx_index(&self) -> Result; /// Get the address of the native token. - fn get_native_token(&self) -> Result; + fn get_native_token(&self) -> Result
; /// Given the information about predecessor block epochs fn get_pred_epochs(&self) -> namada_storage::Result; /// Get the events emitted by the current tx. - fn get_events( - &self, - event_type: &EventType, - ) -> Result, namada_storage::Error>; + fn get_events(&self, event_type: &EventType) -> Result>; /// Storage prefix iterator, ordered by storage keys. It will try to get an /// iterator from the storage. fn iter_prefix<'iter>( &'iter self, prefix: &Key, - ) -> Result, namada_storage::Error>; + ) -> Result>; /// Evaluate a validity predicate with given data. The address, changed /// storage keys and verifiers will have the same values as the input to @@ -113,17 +103,13 @@ where /// /// If the execution fails for whatever reason, this will return `false`. /// Otherwise returns the result of evaluation. - fn eval( - &self, - vp_code: Hash, - input_data: BatchedTxRef<'_>, - ) -> Result<(), namada_storage::Error>; + fn eval(&self, vp_code: Hash, input_data: BatchedTxRef<'_>) -> Result<()>; /// Get a tx hash - fn get_tx_code_hash(&self) -> Result, namada_storage::Error>; + fn get_tx_code_hash(&self) -> Result>; /// Charge the provided gas for the current vp - fn charge_gas(&self, used_gas: Gas) -> Result<(), namada_storage::Error>; + fn charge_gas(&self, used_gas: Gas) -> Result<()>; // ---- Methods below have default implementation via `pre/post` ---- @@ -132,16 +118,13 @@ where fn read_pre( &'view self, key: &Key, - ) -> Result, namada_storage::Error> { + ) -> Result> { self.pre().read(key) } /// Storage read prior state raw bytes (before tx execution). It /// will try to read from the storage. - fn read_bytes_pre( - &'view self, - key: &Key, - ) -> Result>, namada_storage::Error> { + fn read_bytes_pre(&'view self, key: &Key) -> Result>> { self.pre().read_bytes(key) } @@ -151,35 +134,26 @@ where fn read_post( &'view self, key: &Key, - ) -> Result, namada_storage::Error> { + ) -> Result> { self.post().read(key) } /// Storage read posterior state raw bytes (after tx execution). It will try /// to read from the write log first and if no entry found then from the /// storage. - fn read_bytes_post( - &'view self, - key: &Key, - ) -> Result>, namada_storage::Error> { + fn read_bytes_post(&'view self, key: &Key) -> Result>> { self.post().read_bytes(key) } /// Storage `has_key` in prior state (before tx execution). It will try to /// read from the storage. - fn has_key_pre( - &'view self, - key: &Key, - ) -> Result { + fn has_key_pre(&'view self, key: &Key) -> Result { self.pre().has_key(key) } /// Storage `has_key` in posterior state (after tx execution). It will try /// to check the write log first and if no entry found then the storage. - fn has_key_post( - &'view self, - key: &Key, - ) -> Result { + fn has_key_post(&'view self, key: &Key) -> Result { self.post().has_key(key) } } diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index a9c4964664..34c76b18dd 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3849,7 +3849,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "smooth-operator", "thiserror", ] diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index 9ecc644451..468a222e42 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -2122,7 +2122,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "smooth-operator", "thiserror", ] From 06c3b34d01773c6bf8797ede362a464d4e4061c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 11:37:33 +0200 Subject: [PATCH 02/10] gov: env agnostic VP --- Cargo.lock | 1 + crates/benches/native_vps.rs | 18 +- crates/governance/Cargo.toml | 1 + crates/governance/src/vp/mod.rs | 484 ++++++++++++++++++-------------- crates/node/src/protocol.rs | 15 +- crates/sdk/src/validation.rs | 10 +- wasm/Cargo.lock | 1 + wasm_for_tests/Cargo.lock | 1 + 8 files changed, 302 insertions(+), 229 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7fbd5780e1..02ab58bff2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5042,6 +5042,7 @@ dependencies = [ "namada_tx", "namada_vm", "namada_vp", + "namada_vp_env", "proptest", "serde", "serde_json", diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index f91b5ada11..a821a68f18 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -220,7 +220,7 @@ fn governance(c: &mut Criterion) { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let governance = GovernanceVp::new(Ctx::new( + let ctx = Ctx::new( &Address::Internal(InternalAddress::Governance), &shell.state, &signed_tx.tx, @@ -230,18 +230,18 @@ fn governance(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); group.bench_function(bench_name, |b| { b.iter(|| { assert!( - governance - .validate_tx( - &signed_tx.to_ref(), - governance.ctx.keys_changed, - governance.ctx.verifiers, - ) - .is_ok() + GovernanceVp::validate_tx( + &ctx, + &signed_tx.to_ref(), + ctx.keys_changed, + ctx.verifiers, + ) + .is_ok() ) }) }); diff --git a/crates/governance/Cargo.toml b/crates/governance/Cargo.toml index caba406454..618379a5ad 100644 --- a/crates/governance/Cargo.toml +++ b/crates/governance/Cargo.toml @@ -30,6 +30,7 @@ namada_state = { path = "../state" } namada_systems = { path = "../systems" } namada_tx = { path = "../tx" } namada_vp = { path = "../vp" } +namada_vp_env = { path = "../vp_env" } arbitrary = { workspace = true, optional = true } borsh.workspace = true diff --git a/crates/governance/src/vp/mod.rs b/crates/governance/src/vp/mod.rs index 4b4f18197c..45e13a12e1 100644 --- a/crates/governance/src/vp/mod.rs +++ b/crates/governance/src/vp/mod.rs @@ -11,14 +11,10 @@ use namada_core::arith::checked; use namada_core::booleans::{BoolResultUnitExt, ResultBoolExt}; use namada_core::chain::Epoch; use namada_core::storage; -use namada_state::{StateRead, StorageRead}; use namada_systems::{proof_of_stake, trans_token as token}; -use namada_tx::action::{Action, GovAction, Read}; +use namada_tx::action::{Action, GovAction}; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{ - Ctx, CtxPreStorageRead, Error, NativeVp, Result, VpEvaluator, -}; -use namada_vp::VpEnv; +use namada_vp_env::{Error, Result, StorageRead, VpEnv}; use thiserror::Error; use self::utils::ReadType; @@ -50,33 +46,26 @@ impl From for Error { } /// Governance VP -pub struct GovernanceVp<'ctx, S, CA, EVAL, PoS, TokenKeys> -where - S: StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct GovernanceVp<'ctx, CTX, PoS, TokenKeys> { /// Generic types for DI - pub _marker: PhantomData<(PoS, TokenKeys)>, + pub _marker: PhantomData<(&'ctx CTX, PoS, TokenKeys)>, } -impl<'view, 'ctx: 'view, S, CA, EVAL, PoS, TokenKeys> NativeVp<'view> - for GovernanceVp<'ctx, S, CA, EVAL, PoS, TokenKeys> +impl<'ctx, CTX, PoS, TokenKeys> GovernanceVp<'ctx, CTX, PoS, TokenKeys> where - S: StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - PoS: proof_of_stake::Read>, + CTX: VpEnv<'ctx> + namada_tx::action::Read, + PoS: proof_of_stake::Read<>::Pre>, TokenKeys: token::Keys, { - fn validate_tx( - &'view self, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, tx_data: &BatchedTxRef<'_>, keys_changed: &BTreeSet, verifiers: &BTreeSet
, ) -> Result<()> { let (is_valid_keys_set, set_count) = - self.is_valid_init_proposal_key_set(keys_changed)?; + Self::is_valid_init_proposal_key_set(ctx, keys_changed)?; if !is_valid_keys_set { tracing::info!("Invalid changed governance key set"); return Err(Error::new_const("Invalid changed governance key set")); @@ -84,16 +73,16 @@ where // Is VP triggered by a governance proposal? if is_proposal_accepted( - &self.ctx.pre(), + &ctx.pre(), tx_data.tx.data(tx_data.cmt).unwrap_or_default().as_ref(), )? { return Ok(()); } - let native_token = self.ctx.pre().get_native_token()?; + let native_token = ctx.pre().get_native_token()?; // Find the actions applied in the tx - let actions = self.ctx.read_actions()?; + let actions = ctx.read_actions()?; // There must be at least one action if any of the keys belong to gov if actions.is_empty() @@ -143,45 +132,48 @@ where } } - // keys_changed.iter().try_for_each(|key| { for key in keys_changed.iter() { let proposal_id = gov_storage::get_proposal_id(key); let key_type = KeyType::from_key::(key, &native_token); let result = match (key_type, proposal_id) { (KeyType::VOTE, Some(proposal_id)) => { - self.is_valid_vote_key(proposal_id, key, verifiers) + Self::is_valid_vote_key(ctx, proposal_id, key, verifiers) } (KeyType::CONTENT, Some(proposal_id)) => { - self.is_valid_content_key(proposal_id) + Self::is_valid_content_key(ctx, proposal_id) } (KeyType::TYPE, Some(proposal_id)) => { - self.is_valid_proposal_type(proposal_id) + Self::is_valid_proposal_type(ctx, proposal_id) } (KeyType::PROPOSAL_CODE, Some(proposal_id)) => { - self.is_valid_proposal_code(proposal_id) + Self::is_valid_proposal_code(ctx, proposal_id) } (KeyType::ACTIVATION_EPOCH, Some(proposal_id)) => { - self.is_valid_activation_epoch(proposal_id) + Self::is_valid_activation_epoch(ctx, proposal_id) } (KeyType::START_EPOCH, Some(proposal_id)) => { - self.is_valid_start_epoch(proposal_id) + Self::is_valid_start_epoch(ctx, proposal_id) } (KeyType::END_EPOCH, Some(proposal_id)) => { - self.is_valid_end_epoch(proposal_id) + Self::is_valid_end_epoch(ctx, proposal_id) } (KeyType::FUNDS, Some(proposal_id)) => { - self.is_valid_funds(proposal_id, &native_token) + Self::is_valid_funds(ctx, proposal_id, &native_token) } (KeyType::AUTHOR, Some(proposal_id)) => { - self.is_valid_author(proposal_id, verifiers) + Self::is_valid_author(ctx, proposal_id, verifiers) } - (KeyType::COUNTER, _) => self.is_valid_counter(set_count), + (KeyType::COUNTER, _) => Self::is_valid_counter(ctx, set_count), (KeyType::PROPOSAL_COMMIT, _) => { - self.is_valid_proposal_commit() + Self::is_valid_proposal_commit(ctx) + } + (KeyType::PARAMETER, _) => { + Self::is_valid_parameter(ctx, tx_data) + } + (KeyType::BALANCE, _) => { + Self::is_valid_balance(ctx, &native_token) } - (KeyType::PARAMETER, _) => self.is_valid_parameter(tx_data), - (KeyType::BALANCE, _) => self.is_valid_balance(&native_token), (KeyType::UNKNOWN_GOVERNANCE, _) => Err(Error::new_alloc( format!("Unkown governance key change: {key}"), )), @@ -199,33 +191,16 @@ where } Ok(()) } -} - -impl<'view, 'ctx: 'view, S, CA, EVAL, PoS, TokenKeys> - GovernanceVp<'ctx, S, CA, EVAL, PoS, TokenKeys> -where - S: StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - PoS: proof_of_stake::Read>, - TokenKeys: token::Keys, -{ - /// Instantiate a Governance VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, - } - } fn is_valid_init_proposal_key_set( - &self, + ctx: &'ctx CTX, keys: &BTreeSet, ) -> Result<(bool, u64)> { let counter_key = gov_storage::get_counter_key(); - let pre_counter: u64 = self.force_read(&counter_key, ReadType::Pre)?; + let pre_counter: u64 = + Self::force_read(ctx, &counter_key, ReadType::Pre)?; let post_counter: u64 = - self.force_read(&counter_key, ReadType::Post)?; + Self::force_read(ctx, &counter_key, ReadType::Post)?; if post_counter < pre_counter { return Ok((false, 0)); @@ -256,7 +231,7 @@ where } fn is_valid_vote_key( - &'view self, + ctx: &'ctx CTX, proposal_id: u64, key: &storage::Key, verifiers: &BTreeSet
, @@ -267,13 +242,14 @@ where let voting_end_epoch_key = gov_storage::get_voting_end_epoch_key(proposal_id); - let current_epoch = self.ctx.get_block_epoch()?; + let current_epoch = ctx.get_block_epoch()?; - let pre_counter: u64 = self.force_read(&counter_key, ReadType::Pre)?; + let pre_counter: u64 = + Self::force_read(ctx, &counter_key, ReadType::Pre)?; let pre_voting_start_epoch: Epoch = - self.force_read(&voting_start_epoch_key, ReadType::Pre)?; + Self::force_read(ctx, &voting_start_epoch_key, ReadType::Pre)?; let pre_voting_end_epoch: Epoch = - self.force_read(&voting_end_epoch_key, ReadType::Pre)?; + Self::force_read(ctx, &voting_end_epoch_key, ReadType::Pre)?; let voter = gov_storage::get_voter_address(key).ok_or(Error::new_alloc( @@ -301,8 +277,7 @@ where validator.clone(), ); - if self - .force_read::(&vote_key, ReadType::Post) + if Self::force_read::(ctx, &vote_key, ReadType::Post) .is_err() { return Err(Error::new_alloc(format!( @@ -317,7 +292,7 @@ where // Voted outside of voting window. We dont check for validator because // if the proposal type is validator, we need to let // them vote for the entire voting window. - if !self.is_valid_voting_window( + if !Self::is_valid_voting_window( current_epoch, pre_voting_start_epoch, pre_voting_end_epoch, @@ -332,7 +307,8 @@ where } // first check if validator, then check if delegator - let is_validator = self.is_validator(verifiers, voter, validator)?; + let is_validator = + Self::is_validator(ctx, verifiers, voter, validator)?; if is_validator { return is_valid_validator_voting_period( @@ -350,7 +326,8 @@ where }); } - let is_delegator = self.is_delegator( + let is_delegator = Self::is_delegator( + ctx, pre_voting_start_epoch, verifiers, voter, @@ -368,13 +345,16 @@ where } /// Validate a content key - pub fn is_valid_content_key(&self, proposal_id: u64) -> Result<()> { + pub fn is_valid_content_key( + ctx: &'ctx CTX, + proposal_id: u64, + ) -> Result<()> { let content_key: storage::Key = gov_storage::get_content_key(proposal_id); let max_content_length_parameter_key = gov_storage::get_max_proposal_content_key(); - let has_pre_content: bool = self.ctx.has_key_pre(&content_key)?; + let has_pre_content: bool = ctx.has_key_pre(&content_key)?; if has_pre_content { return Err(Error::new_alloc(format!( "Proposal with id {proposal_id} already had content written \ @@ -382,11 +362,14 @@ where ))); } - let max_content_length: usize = - self.force_read(&max_content_length_parameter_key, ReadType::Pre)?; + let max_content_length: usize = Self::force_read( + ctx, + &max_content_length_parameter_key, + ReadType::Pre, + )?; // Check the byte length let post_content_bytes = - self.ctx.read_bytes_post(&content_key)?.unwrap_or_default(); + ctx.read_bytes_post(&content_key)?.unwrap_or_default(); let is_valid = post_content_bytes.len() <= max_content_length; if !is_valid { @@ -401,10 +384,13 @@ where } /// Validate the proposal type - pub fn is_valid_proposal_type(&self, proposal_id: u64) -> Result<()> { + pub fn is_valid_proposal_type( + ctx: &'ctx CTX, + proposal_id: u64, + ) -> Result<()> { let proposal_type_key = gov_storage::get_proposal_type_key(proposal_id); let proposal_type: ProposalType = - self.force_read(&proposal_type_key, ReadType::Post)?; + Self::force_read(ctx, &proposal_type_key, ReadType::Post)?; match proposal_type { ProposalType::PGFSteward(stewards) => { @@ -446,8 +432,11 @@ where }; } else if let Some(address) = stewards_added.first() { let author_key = gov_storage::get_author_key(proposal_id); - let author = self - .force_read::
(&author_key, ReadType::Post)?; + let author = Self::force_read::
( + ctx, + &author_key, + ReadType::Post, + )?; let is_valid_author = address.eq(&author); if !is_valid_author { @@ -558,10 +547,13 @@ where } /// Validate a proposal code - pub fn is_valid_proposal_code(&self, proposal_id: u64) -> Result<()> { + pub fn is_valid_proposal_code( + ctx: &'ctx CTX, + proposal_id: u64, + ) -> Result<()> { let proposal_type_key = gov_storage::get_proposal_type_key(proposal_id); let proposal_type: ProposalType = - self.force_read(&proposal_type_key, ReadType::Post)?; + Self::force_read(ctx, &proposal_type_key, ReadType::Post)?; if !proposal_type.is_default_with_wasm() { return Err(Error::new_alloc(format!( @@ -574,7 +566,7 @@ where let max_code_size_parameter_key = gov_storage::get_max_proposal_code_size_key(); - let has_pre_code: bool = self.ctx.has_key_pre(&code_key)?; + let has_pre_code: bool = ctx.has_key_pre(&code_key)?; if has_pre_code { return Err(Error::new_alloc(format!( "Proposal with id {proposal_id} already had wasm code written \ @@ -583,9 +575,8 @@ where } let max_proposal_length: usize = - self.force_read(&max_code_size_parameter_key, ReadType::Pre)?; - let post_code: Vec = - self.ctx.read_post(&code_key)?.unwrap_or_default(); + Self::force_read(ctx, &max_code_size_parameter_key, ReadType::Pre)?; + let post_code: Vec = ctx.read_post(&code_key)?.unwrap_or_default(); let wasm_code_below_max_len = post_code.len() <= max_proposal_length; @@ -602,7 +593,10 @@ where } /// Validate an activation_epoch key - pub fn is_valid_activation_epoch(&self, proposal_id: u64) -> Result<()> { + pub fn is_valid_activation_epoch( + ctx: &'ctx CTX, + proposal_id: u64, + ) -> Result<()> { let start_epoch_key = gov_storage::get_voting_start_epoch_key(proposal_id); let end_epoch_key = gov_storage::get_voting_end_epoch_key(proposal_id); @@ -613,7 +607,7 @@ where gov_storage::get_min_proposal_grace_epochs_key(); let has_pre_activation_epoch = - self.ctx.has_key_pre(&activation_epoch_key)?; + ctx.has_key_pre(&activation_epoch_key)?; if has_pre_activation_epoch { return Err(Error::new_alloc(format!( "Proposal with id {proposal_id} already had a grace epoch \ @@ -622,22 +616,22 @@ where } let start_epoch: Epoch = - self.force_read(&start_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &start_epoch_key, ReadType::Post)?; let end_epoch: Epoch = - self.force_read(&end_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &end_epoch_key, ReadType::Post)?; let activation_epoch: Epoch = - self.force_read(&activation_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &activation_epoch_key, ReadType::Post)?; let min_grace_epochs: u64 = - self.force_read(&min_grace_epochs_key, ReadType::Pre)?; + Self::force_read(ctx, &min_grace_epochs_key, ReadType::Pre)?; let max_proposal_period: u64 = - self.force_read(&max_proposal_period, ReadType::Pre)?; + Self::force_read(ctx, &max_proposal_period, ReadType::Pre)?; let committing_epoch_key = gov_storage::get_committing_proposals_key( proposal_id, activation_epoch.into(), ); let has_post_committing_epoch = - self.ctx.has_key_post(&committing_epoch_key)?; + ctx.has_key_post(&committing_epoch_key)?; if !has_post_committing_epoch { let error = Error::new_const("Committing proposal key is missing present"); @@ -673,7 +667,10 @@ where } /// Validate a start_epoch key - pub fn is_valid_start_epoch(&self, proposal_id: u64) -> Result<()> { + pub fn is_valid_start_epoch( + ctx: &'ctx CTX, + proposal_id: u64, + ) -> Result<()> { let start_epoch_key = gov_storage::get_voting_start_epoch_key(proposal_id); let end_epoch_key = gov_storage::get_voting_end_epoch_key(proposal_id); @@ -682,9 +679,9 @@ where let max_latency_paramater_key = gov_storage::get_max_proposal_latency_key(); - let current_epoch = self.ctx.get_block_epoch()?; + let current_epoch = ctx.get_block_epoch()?; - let has_pre_start_epoch = self.ctx.has_key_pre(&start_epoch_key)?; + let has_pre_start_epoch = ctx.has_key_pre(&start_epoch_key)?; if has_pre_start_epoch { let error = Error::new_alloc(format!( "Failed to validate start epoch. Proposal with id \ @@ -695,7 +692,7 @@ where return Err(error); } - let has_pre_end_epoch = self.ctx.has_key_pre(&end_epoch_key)?; + let has_pre_end_epoch = ctx.has_key_pre(&end_epoch_key)?; if has_pre_end_epoch { let error = Error::new_alloc(format!( "Failed to validate start epoch. Proposal with id \ @@ -707,11 +704,11 @@ where } let start_epoch: Epoch = - self.force_read(&start_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &start_epoch_key, ReadType::Post)?; let end_epoch: Epoch = - self.force_read(&end_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &end_epoch_key, ReadType::Post)?; let min_period: u64 = - self.force_read(&min_period_parameter_key, ReadType::Pre)?; + Self::force_read(ctx, &min_period_parameter_key, ReadType::Pre)?; if end_epoch <= start_epoch { return Err(Error::new_alloc(format!( @@ -730,7 +727,7 @@ where } let latency: u64 = - self.force_read(&max_latency_paramater_key, ReadType::Pre)?; + Self::force_read(ctx, &max_latency_paramater_key, ReadType::Pre)?; if checked!(start_epoch.0 - current_epoch.0)? > latency { return Err(Error::new_alloc(format!( "Starting epoch {start_epoch} of the proposal with id \ @@ -753,7 +750,7 @@ where } /// Validate a end_epoch key - fn is_valid_end_epoch(&self, proposal_id: u64) -> Result<()> { + fn is_valid_end_epoch(ctx: &'ctx CTX, proposal_id: u64) -> Result<()> { let start_epoch_key = gov_storage::get_voting_start_epoch_key(proposal_id); let end_epoch_key = gov_storage::get_voting_end_epoch_key(proposal_id); @@ -762,9 +759,9 @@ where let max_period_parameter_key = gov_storage::get_max_proposal_period_key(); - let current_epoch = self.ctx.get_block_epoch()?; + let current_epoch = ctx.get_block_epoch()?; - let has_pre_start_epoch = self.ctx.has_key_pre(&start_epoch_key)?; + let has_pre_start_epoch = ctx.has_key_pre(&start_epoch_key)?; if has_pre_start_epoch { let error = Error::new_alloc(format!( "Failed to validate end epoch. Proposal with id {proposal_id} \ @@ -774,7 +771,7 @@ where return Err(error); } - let has_pre_end_epoch = self.ctx.has_key_pre(&end_epoch_key)?; + let has_pre_end_epoch = ctx.has_key_pre(&end_epoch_key)?; if has_pre_end_epoch { let error = Error::new_alloc(format!( "Failed to validate end epoch. Proposal with id {proposal_id} \ @@ -785,13 +782,13 @@ where } let start_epoch: Epoch = - self.force_read(&start_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &start_epoch_key, ReadType::Post)?; let end_epoch: Epoch = - self.force_read(&end_epoch_key, ReadType::Post)?; + Self::force_read(ctx, &end_epoch_key, ReadType::Post)?; let min_period: u64 = - self.force_read(&min_period_parameter_key, ReadType::Pre)?; + Self::force_read(ctx, &min_period_parameter_key, ReadType::Pre)?; let max_period: u64 = - self.force_read(&max_period_parameter_key, ReadType::Pre)?; + Self::force_read(ctx, &max_period_parameter_key, ReadType::Pre)?; if end_epoch <= start_epoch || start_epoch <= current_epoch { let error = Error::new_alloc(format!( @@ -818,23 +815,23 @@ where /// Validate a funds key pub fn is_valid_funds( - &self, + ctx: &'ctx CTX, proposal_id: u64, native_token_address: &Address, ) -> Result<()> { let funds_key = gov_storage::get_funds_key(proposal_id); let balance_key = - TokenKeys::balance_key(native_token_address, self.ctx.address); + TokenKeys::balance_key(native_token_address, &ADDRESS); let min_funds_parameter_key = gov_storage::get_min_proposal_fund_key(); let min_funds_parameter: token::Amount = - self.force_read(&min_funds_parameter_key, ReadType::Pre)?; + Self::force_read(ctx, &min_funds_parameter_key, ReadType::Pre)?; let pre_balance: Option = - self.ctx.pre().read(&balance_key)?; + ctx.pre().read(&balance_key)?; let post_balance: token::Amount = - self.force_read(&balance_key, ReadType::Post)?; + Self::force_read(ctx, &balance_key, ReadType::Post)?; let post_funds: token::Amount = - self.force_read(&funds_key, ReadType::Post)?; + Self::force_read(ctx, &funds_key, ReadType::Post)?; pre_balance.map_or_else( // null pre balance @@ -883,18 +880,21 @@ where } /// Validate a balance key - fn is_valid_balance(&self, native_token_address: &Address) -> Result<()> { + fn is_valid_balance( + ctx: &'ctx CTX, + native_token_address: &Address, + ) -> Result<()> { let balance_key = - TokenKeys::balance_key(native_token_address, self.ctx.address); + TokenKeys::balance_key(native_token_address, &ADDRESS); let min_funds_parameter_key = gov_storage::get_min_proposal_fund_key(); let pre_balance: Option = - self.ctx.pre().read(&balance_key)?; + ctx.pre().read(&balance_key)?; let min_funds_parameter: token::Amount = - self.force_read(&min_funds_parameter_key, ReadType::Pre)?; + Self::force_read(ctx, &min_funds_parameter_key, ReadType::Pre)?; let post_balance: token::Amount = - self.force_read(&balance_key, ReadType::Post)?; + Self::force_read(ctx, &balance_key, ReadType::Post)?; let balance_is_valid = if let Some(pre_balance) = pre_balance { post_balance > pre_balance @@ -913,13 +913,13 @@ where /// Validate a author key pub fn is_valid_author( - &self, + ctx: &'ctx CTX, proposal_id: u64, verifiers: &BTreeSet
, ) -> Result<()> { let author_key = gov_storage::get_author_key(proposal_id); - let has_pre_author = self.ctx.has_key_pre(&author_key)?; + let has_pre_author = ctx.has_key_pre(&author_key)?; if has_pre_author { return Err(Error::new_alloc(format!( "Proposal with id {proposal_id} already had an author written \ @@ -927,15 +927,13 @@ where ))); } - let author = self.force_read(&author_key, ReadType::Post)?; - namada_account::exists(&self.ctx.pre(), &author).true_or_else( - || { - Error::new_alloc(format!( - "No author account {author} could be found for the \ - proposal with id {proposal_id}" - )) - }, - )?; + let author = Self::force_read(ctx, &author_key, ReadType::Post)?; + namada_account::exists(&ctx.pre(), &author).true_or_else(|| { + Error::new_alloc(format!( + "No author account {author} could be found for the proposal \ + with id {proposal_id}" + )) + })?; verifiers.contains(&author).ok_or_else(|| { Error::new_alloc(format!( @@ -946,11 +944,12 @@ where } /// Validate a counter key - pub fn is_valid_counter(&self, set_count: u64) -> Result<()> { + pub fn is_valid_counter(ctx: &'ctx CTX, set_count: u64) -> Result<()> { let counter_key = gov_storage::get_counter_key(); - let pre_counter: u64 = self.force_read(&counter_key, ReadType::Pre)?; + let pre_counter: u64 = + Self::force_read(ctx, &counter_key, ReadType::Pre)?; let post_counter: u64 = - self.force_read(&counter_key, ReadType::Post)?; + Self::force_read(ctx, &counter_key, ReadType::Post)?; let expected_counter = checked!(pre_counter + set_count)?; let valid_counter = expected_counter == post_counter; @@ -964,11 +963,12 @@ where } /// Validate a commit key - pub fn is_valid_proposal_commit(&self) -> Result<()> { + pub fn is_valid_proposal_commit(ctx: &'ctx CTX) -> Result<()> { let counter_key = gov_storage::get_counter_key(); - let pre_counter: u64 = self.force_read(&counter_key, ReadType::Pre)?; + let pre_counter: u64 = + Self::force_read(ctx, &counter_key, ReadType::Pre)?; let post_counter: u64 = - self.force_read(&counter_key, ReadType::Post)?; + Self::force_read(ctx, &counter_key, ReadType::Post)?; // NOTE: can't do pre_counter + set_count == post_counter here // because someone may update an empty proposal that just @@ -985,7 +985,7 @@ where /// Validate a governance parameter pub fn is_valid_parameter( - &self, + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, ) -> Result<()> { let BatchedTxRef { tx, cmt } = batched_tx; @@ -997,21 +997,22 @@ where )) }, |data| { - is_proposal_accepted(&self.ctx.pre(), data.as_ref())? - .ok_or_else(|| { + is_proposal_accepted(&ctx.pre(), data.as_ref())?.ok_or_else( + || { Error::new_const( "Governance parameter changes can only be \ performed by a governance proposal that has been \ accepted", ) - }) + }, + ) }, ) } /// Check if a vote is from a validator pub fn is_validator( - &'view self, + ctx: &'ctx CTX, verifiers: &BTreeSet
, voter: &Address, validator: &Address, @@ -1020,14 +1021,14 @@ where return Ok(false); } - let is_validator = PoS::is_validator(&self.ctx.pre(), voter)?; + let is_validator = PoS::is_validator(&ctx.pre(), voter)?; Ok(is_validator && verifiers.contains(voter)) } /// Private method to read from storage data that are 100% in storage. fn force_read( - &self, + ctx: &'ctx CTX, key: &storage::Key, read_type: ReadType, ) -> Result @@ -1035,8 +1036,8 @@ where T: BorshDeserialize, { let res = match read_type { - ReadType::Pre => self.ctx.pre().read::(key), - ReadType::Post => self.ctx.post().read::(key), + ReadType::Pre => ctx.pre().read::(key), + ReadType::Post => ctx.post().read::(key), }?; if let Some(data) = res { @@ -1049,7 +1050,6 @@ where } fn is_valid_voting_window( - &self, current_epoch: Epoch, start_epoch: Epoch, end_epoch: Epoch, @@ -1068,7 +1068,7 @@ where /// Check if a vote is from a delegator pub fn is_delegator( - &'view self, + ctx: &'ctx CTX, epoch: Epoch, verifiers: &BTreeSet
, address: &Address, @@ -1076,7 +1076,7 @@ where ) -> Result { Ok(address != delegation_address && verifiers.contains(address) - && PoS::is_delegator(&self.ctx.pre(), address, Some(epoch))?) + && PoS::is_delegator(&ctx.pre(), address, Some(epoch))?) } } @@ -1188,7 +1188,7 @@ mod test { use namada_vm::wasm::run::VpEvalWasm; use namada_vm::wasm::VpCache; use namada_vm::{wasm, WasmCacheRwAccess}; - use namada_vp::native_vp::{Ctx, CtxPreStorageRead, NativeVp}; + use namada_vp::native_vp::{self, CtxPreStorageRead}; use crate::storage::keys::{ get_activation_epoch_key, get_author_key, get_committing_proposals_key, @@ -1200,11 +1200,10 @@ mod test { type CA = WasmCacheRwAccess; type Eval = VpEvalWasm<::D, ::H, CA>; + type Ctx<'ctx, S> = native_vp::Ctx<'ctx, S, VpCache, Eval>; type GovernanceVp<'ctx, S> = super::GovernanceVp< 'ctx, - S, - VpCache, - Eval, + Ctx<'ctx, S>, namada_proof_of_stake::Store< CtxPreStorageRead<'ctx, 'ctx, S, VpCache, Eval>, >, @@ -1279,10 +1278,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); } @@ -1538,10 +1541,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -1634,9 +1641,12 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - let result = - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers); // this should fail + let result = GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers, + ); // this should fail assert_matches!(&result, Err(_)); if result.is_err() { @@ -1732,9 +1742,12 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - let result = - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers); + let result = GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers, + ); assert_matches!(&result, Ok(_)); if result.is_err() { @@ -1830,10 +1843,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -1908,10 +1925,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -1986,10 +2007,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -2082,10 +2107,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -2178,10 +2207,14 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -2256,10 +2289,14 @@ mod test { vp_wasm_cache.clone(), ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -2307,10 +2344,13 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); } @@ -2385,10 +2425,14 @@ mod test { vp_wasm_cache.clone(), ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -2436,10 +2480,13 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -2514,10 +2561,14 @@ mod test { vp_wasm_cache.clone(), ); - let governance_vp = GovernanceVp::new(ctx); // this should return true because state has been stored assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -2565,10 +2616,13 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -2643,10 +2697,13 @@ mod test { vp_wasm_cache.clone(), ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -2711,10 +2768,13 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); } @@ -2789,10 +2849,13 @@ mod test { vp_wasm_cache.clone(), ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -2857,10 +2920,13 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } @@ -2935,10 +3001,13 @@ mod test { vp_wasm_cache.clone(), ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Ok(_) ); @@ -3003,10 +3072,13 @@ mod test { vp_wasm_cache, ); - let governance_vp = GovernanceVp::new(ctx); - assert_matches!( - governance_vp.validate_tx(&batched_tx, &keys_changed, &verifiers), + GovernanceVp::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers + ), Err(_) ); } diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 1ace8b45bf..80d1087bc5 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1252,14 +1252,13 @@ where )) } InternalAddress::Governance => { - let governance = GovernanceVp::new(ctx); - governance - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) + GovernanceVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError) } InternalAddress::Pgf => { let pgf_vp = PgfVp::new(ctx); diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index eb82ce7e51..c86651cfae 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -59,12 +59,10 @@ pub type ParametersVp<'ctx, CTX> = parameters::vp::ParametersVp< >; /// Native governance VP -pub type GovernanceVp<'a, S, CA> = governance::vp::GovernanceVp< - 'a, - S, - VpCache, - Eval, - PosPreStore<'a, S, CA>, +pub type GovernanceVp<'ctx, CTX> = governance::vp::GovernanceVp< + 'ctx, + CTX, + proof_of_stake::Store<>::Pre>, TokenKeys, >; diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index 34c76b18dd..d109e20518 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3764,6 +3764,7 @@ dependencies = [ "namada_systems", "namada_tx", "namada_vp", + "namada_vp_env", "proptest", "serde", "serde_json", diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index 468a222e42..82335078af 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -2052,6 +2052,7 @@ dependencies = [ "namada_systems", "namada_tx", "namada_vp", + "namada_vp_env", "serde", "serde_json", "smooth-operator", From 5db5b2d36b2329aebb57d3c68539eaeb7721cf06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 12:28:38 +0200 Subject: [PATCH 03/10] pgf: env agnostic VP --- crates/benches/native_vps.rs | 11 +-- crates/governance/Cargo.toml | 2 +- crates/governance/src/vp/pgf.rs | 141 ++++++++++++++------------------ crates/node/src/protocol.rs | 17 ++-- crates/sdk/src/validation.rs | 3 +- wasm/Cargo.lock | 1 - wasm_for_tests/Cargo.lock | 1 - 7 files changed, 76 insertions(+), 100 deletions(-) diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index a821a68f18..fca02b818b 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -1246,7 +1246,7 @@ fn pgf(c: &mut Criterion) { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let pgf = PgfVp::new(Ctx::new( + let ctx = Ctx::new( &Address::Internal(InternalAddress::Pgf), &shell.state, &signed_tx.tx, @@ -1256,15 +1256,16 @@ fn pgf(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); group.bench_function(bench_name, |b| { b.iter(|| { assert!( - pgf.validate_tx( + PgfVp::validate_tx( + &ctx, &signed_tx.to_ref(), - pgf.ctx.keys_changed, - pgf.ctx.verifiers, + ctx.keys_changed, + ctx.verifiers, ) .is_ok() ) diff --git a/crates/governance/Cargo.toml b/crates/governance/Cargo.toml index 618379a5ad..075ad806fb 100644 --- a/crates/governance/Cargo.toml +++ b/crates/governance/Cargo.toml @@ -29,7 +29,6 @@ namada_migrations = { path= "../migrations", optional = true } namada_state = { path = "../state" } namada_systems = { path = "../systems" } namada_tx = { path = "../tx" } -namada_vp = { path = "../vp" } namada_vp_env = { path = "../vp_env" } arbitrary = { workspace = true, optional = true } @@ -54,6 +53,7 @@ namada_state = { path = "../state", features = ["testing"] } namada_token = { path = "../token", features = ["testing"] } namada_tx = { path = "../tx", features = ["testing"] } namada_vm = { path = "../vm", features = ["testing"] } +namada_vp = { path = "../vp" } assert_matches.workspace = true proptest.workspace = true diff --git a/crates/governance/src/vp/pgf.rs b/crates/governance/src/vp/pgf.rs index 7f9f168177..6f2c656652 100644 --- a/crates/governance/src/vp/pgf.rs +++ b/crates/governance/src/vp/pgf.rs @@ -1,13 +1,13 @@ //! Pgf VP use std::collections::BTreeSet; +use std::marker::PhantomData; use namada_core::booleans::BoolResultUnitExt; use namada_core::storage::Key; -use namada_state::StateRead; -use namada_tx::action::{Action, PgfAction, Read}; +use namada_tx::action::{Action, PgfAction}; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{self, Ctx, Error, NativeVp, Result, VpEvaluator}; +use namada_vp_env::{Error, Result, VpEnv}; use thiserror::Error; use crate::address::{Address, InternalAddress}; @@ -33,32 +33,28 @@ impl From for Error { } /// Pgf VP -pub struct PgfVp<'ctx, S, CA, EVAL> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct PgfVp<'ctx, CTX> { + /// Generic types for DI + pub _marker: PhantomData<&'ctx CTX>, } -impl<'view, 'ctx, S, CA, EVAL> NativeVp<'view> for PgfVp<'ctx, S, CA, EVAL> +impl<'ctx, CTX> PgfVp<'ctx, CTX> where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, + CTX: VpEnv<'ctx> + namada_tx::action::Read, { - fn validate_tx( - &'view self, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, keys_changed: &BTreeSet, verifiers: &BTreeSet
, ) -> Result<()> { // Find the actions applied in the tx - let actions = self.ctx.read_actions()?; + let actions = ctx.read_actions()?; // Is VP triggered by a governance proposal? if is_proposal_accepted( - &self.ctx.pre(), + &ctx.pre(), batched_tx .tx .data(batched_tx.cmt) @@ -75,7 +71,7 @@ where tracing::info!( "Rejecting tx without any action written to temp storage" ); - return Err(native_vp::Error::new_const( + return Err(Error::new_const( "Rejecting tx without any action written to temp storage", )); } @@ -126,108 +122,93 @@ where // TODO(namada#3238): we should check errors here, which // could be out-of-gas related let total_stewards_pre = pgf_storage::stewards_handle() - .len(&self.ctx.pre()) + .len(&ctx.pre()) .unwrap_or_default(); let total_stewards_post = pgf_storage::stewards_handle() - .len(&self.ctx.post()) + .len(&ctx.post()) .unwrap_or_default(); total_stewards_pre < total_stewards_post }; if stewards_have_increased { - return Err(native_vp::Error::new_const( + return Err(Error::new_const( "Stewards can only be added via governance \ proposals", )); } - pgf::storage::get_steward( - &self.ctx.post(), - steward_address, - )? - .map_or_else( - // if a steward resigns, check their signature - || { - verifiers.contains(steward_address).ok_or_else( - || { - native_vp::Error::new_alloc(format!( + pgf::storage::get_steward(&ctx.post(), steward_address)? + .map_or_else( + // if a steward resigns, check their signature + || { + verifiers.contains(steward_address).ok_or_else( + || { + Error::new_alloc(format!( + "The VP of the steward \ + {steward_address} should have \ + been triggered to check their \ + signature" + )) + }, + ) + }, + // if a steward updates the reward distribution (so + // total_stewards_pre == total_stewards_post) check + // their signature and if commissions are valid + |steward| { + if !verifiers.contains(steward_address) { + return Err(Error::new_alloc(format!( "The VP of the steward \ {steward_address} should have been \ triggered to check their signature" - )) - }, - ) - }, - // if a steward updates the reward distribution (so - // total_stewards_pre == total_stewards_post) check - // their signature and if commissions are valid - |steward| { - if !verifiers.contains(steward_address) { - return Err(native_vp::Error::new_alloc( - format!( - "The VP of the steward \ - {steward_address} should have been \ - triggered to check their signature" - ), - )); - } - steward.is_valid_reward_distribution().ok_or_else( - || { - native_vp::Error::new_const( - "Steward commissions are invalid", - ) - }, - ) - }, - ) + ))); + } + steward + .is_valid_reward_distribution() + .ok_or_else(|| { + Error::new_const( + "Steward commissions are invalid", + ) + }) + }, + ) } - KeyType::Fundings => Err(native_vp::Error::new_alloc(format!( + KeyType::Fundings => Err(Error::new_alloc(format!( "Cannot update PGF fundings key: {key}" ))), KeyType::PgfInflationRate | KeyType::StewardInflationRate => { - self.is_valid_parameter_change(batched_tx) + Self::is_valid_parameter_change(ctx, batched_tx) } - KeyType::UnknownPgf => Err(native_vp::Error::new_alloc( - format!("Unknown PGF state update on key: {key}"), - )), + KeyType::UnknownPgf => Err(Error::new_alloc(format!( + "Unknown PGF state update on key: {key}" + ))), KeyType::Unknown => Ok(()), } }) } -} - -impl<'view, 'ctx, S, CA, EVAL> PgfVp<'ctx, S, CA, EVAL> -where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, -{ - /// Instantiate PGF VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { ctx } - } /// Validate a governance parameter pub fn is_valid_parameter_change( - &'view self, + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, ) -> Result<()> { batched_tx.tx.data(batched_tx.cmt).map_or_else( || { - Err(native_vp::Error::new_const( + Err(Error::new_const( "PGF parameter changes require tx data to be present", )) }, |data| { - is_proposal_accepted(&self.ctx.pre(), data.as_ref())? - .ok_or_else(|| { - native_vp::Error::new_const( + is_proposal_accepted(&ctx.pre(), data.as_ref())?.ok_or_else( + || { + Error::new_const( "PGF parameter changes can only be performed by a \ governance proposal that has been accepted", ) - }) + }, + ) }, ) } diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 80d1087bc5..a287456ff1 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1260,16 +1260,13 @@ where ) .map_err(Error::NativeVpError) } - InternalAddress::Pgf => { - let pgf_vp = PgfVp::new(ctx); - pgf_vp - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) - } + InternalAddress::Pgf => PgfVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError), InternalAddress::Multitoken => { let multitoken = MultitokenVp::new(ctx); multitoken diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index c86651cfae..16e3145703 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -67,8 +67,7 @@ pub type GovernanceVp<'ctx, CTX> = governance::vp::GovernanceVp< >; /// Native PGF VP -pub type PgfVp<'a, S, CA> = - governance::vp::pgf::PgfVp<'a, S, VpCache, Eval>; +pub type PgfVp<'ctx, CTX> = governance::vp::pgf::PgfVp<'ctx, CTX>; /// Native multitoken VP pub type MultitokenVp<'a, S, CA> = token::vp::MultitokenVp< diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index d109e20518..bbd144b83b 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3763,7 +3763,6 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", "namada_vp_env", "proptest", "serde", diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index 82335078af..8eae5e410f 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -2051,7 +2051,6 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", "namada_vp_env", "serde", "serde_json", From 624b6b679513b0c4de165f091f153076ae4deb7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 13:06:09 +0200 Subject: [PATCH 04/10] eth/bridge_pool: env agnostic VP --- Cargo.lock | 1 + crates/benches/native_vps.rs | 18 +- crates/ethereum_bridge/Cargo.toml | 1 + .../ethereum_bridge/src/vp/bridge_pool_vp.rs | 432 ++++++++---------- crates/node/src/protocol.rs | 15 +- crates/sdk/src/validation.rs | 4 +- crates/tests/src/native_vp/eth_bridge_pool.rs | 10 +- crates/tests/src/native_vp/mod.rs | 18 + wasm/Cargo.lock | 1 + 9 files changed, 228 insertions(+), 272 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 02ab58bff2..42f7edfe6c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4941,6 +4941,7 @@ dependencies = [ "namada_vm", "namada_vote_ext", "namada_vp", + "namada_vp_env", "proptest", "rand 0.8.5", "serde", diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index fca02b818b..e537e2357e 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -1490,7 +1490,7 @@ fn eth_bridge_pool(c: &mut Criterion) { let vp_address = Address::Internal(InternalAddress::EthBridgePool); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter(&TxGasMeter::new(u64::MAX))); - let bridge_pool = EthBridgePoolVp::new(Ctx::new( + let ctx = Ctx::new( &vp_address, &shell.state, &signed_tx.tx, @@ -1500,18 +1500,18 @@ fn eth_bridge_pool(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); c.bench_function("vp_eth_bridge_pool", |b| { b.iter(|| { assert!( - bridge_pool - .validate_tx( - &signed_tx.to_ref(), - bridge_pool.ctx.keys_changed, - bridge_pool.ctx.verifiers, - ) - .is_ok() + EthBridgePoolVp::validate_tx( + &ctx, + &signed_tx.to_ref(), + ctx.keys_changed, + ctx.verifiers, + ) + .is_ok() ) }) }); diff --git a/crates/ethereum_bridge/Cargo.toml b/crates/ethereum_bridge/Cargo.toml index 9cf79413b0..f6611d940f 100644 --- a/crates/ethereum_bridge/Cargo.toml +++ b/crates/ethereum_bridge/Cargo.toml @@ -42,6 +42,7 @@ namada_trans_token = {path = "../trans_token"} namada_tx = {path = "../tx"} namada_vote_ext = {path = "../vote_ext"} namada_vp = {path = "../vp"} +namada_vp_env = {path = "../vp_env"} borsh.workspace = true ethers.workspace = true diff --git a/crates/ethereum_bridge/src/vp/bridge_pool_vp.rs b/crates/ethereum_bridge/src/vp/bridge_pool_vp.rs index 983f397d19..6935c63cb7 100644 --- a/crates/ethereum_bridge/src/vp/bridge_pool_vp.rs +++ b/crates/ethereum_bridge/src/vp/bridge_pool_vp.rs @@ -26,10 +26,10 @@ use namada_core::ethereum_events::EthAddress; use namada_core::hints; use namada_core::storage::Key; use namada_core::uint::I320; -use namada_state::{ResultExt, StateRead}; +use namada_state::ResultExt; use namada_systems::trans_token::{self as token, Amount}; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{Ctx, Error, NativeVp, StorageReader, VpEvaluator}; +use namada_vp_env::{Error, Result, StorageRead, VpEnv}; use crate::storage::bridge_pool::{ get_pending_key, is_bridge_pool_key, BRIDGE_POOL_ADDRESS, @@ -51,47 +51,164 @@ struct AmountDelta { impl AmountDelta { /// Resolve the updated amount by applying the delta value. #[inline] - fn resolve(self) -> Result { + fn resolve(self) -> Result { checked!(self.delta + I320::from(self.base)).map_err(Into::into) } } /// Validity predicate for the Ethereum bridge -pub struct BridgePool<'ctx, S, CA, EVAL, TokenKeys> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct BridgePool<'ctx, CTX, TokenKeys> { /// Generic types for DI - pub _marker: PhantomData, + pub _marker: PhantomData<(&'ctx CTX, TokenKeys)>, } -impl<'ctx, S, CA, EVAL, TokenKeys> BridgePool<'ctx, S, CA, EVAL, TokenKeys> +impl<'ctx, CTX, TokenKeys> BridgePool<'ctx, CTX, TokenKeys> where - S: 'static + StateRead, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - CA: 'static + Clone, + CTX: VpEnv<'ctx> + namada_tx::action::Read, TokenKeys: token::Keys, { - /// Instantiate bridge pool VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, + batched_tx: &BatchedTxRef<'_>, + keys_changed: &BTreeSet, + _verifiers: &BTreeSet
, + ) -> Result<()> { + tracing::debug!( + keys_changed_len = keys_changed.len(), + verifiers_len = _verifiers.len(), + "Ethereum Bridge Pool VP triggered", + ); + if !is_bridge_active_at(&ctx.pre(), ctx.get_block_epoch()?)? { + tracing::debug!( + "Rejecting transaction, since the Ethereum bridge is disabled." + ); + return Err(Error::new_const( + "Rejecting transaction, since the Ethereum bridge is disabled.", + )); + } + let Some(tx_data) = batched_tx.tx.data(batched_tx.cmt) else { + return Err(Error::new_const("No transaction data found")); + }; + let transfer: PendingTransfer = + BorshDeserialize::try_from_slice(&tx_data[..]) + .into_storage_result()?; + + let pending_key = get_pending_key(&transfer); + // check that transfer is not already in the pool + match ctx.pre().read::(&pending_key) { + Ok(Some(_)) => { + let error = Error::new_const( + "Rejecting transaction as the transfer is already in the \ + Ethereum bridge pool.", + ); + tracing::debug!("{error}"); + return Err(error); + } + // NOTE: make sure we don't erase storage errors returned by the + // ctx, as these may contain gas errors! + Err(e) => return Err(e), + _ => {} + } + for key in keys_changed.iter().filter(|k| is_bridge_pool_key(k)) { + if *key != pending_key { + let error = Error::new_alloc(format!( + "Rejecting transaction as it is attempting to change an \ + incorrect key in the Ethereum bridge pool: {key}.\n \ + Expected key: {pending_key}", + )); + tracing::debug!("{error}"); + return Err(error); + } + } + let pending: PendingTransfer = + ctx.post().read(&pending_key)?.ok_or_else(|| { + Error::new_const( + "Rejecting transaction as the transfer wasn't added to \ + the pool of pending transfers", + ) + })?; + if pending != transfer { + let error = Error::new_alloc(format!( + "An incorrect transfer was added to the Ethereum bridge pool: \ + {transfer:?}.\n Expected: {pending:?}", + )); + tracing::debug!("{error}"); + return Err(error); + } + // The deltas in the escrowed amounts we must check. + let wnam_address = read_native_erc20_address(&ctx.pre())?; + let escrow_checks = + Self::determine_escrow_checks(ctx, &wnam_address, &transfer)?; + if !escrow_checks.validate::(keys_changed) { + let error = Error::new_const( + // TODO(namada#3247): specify which storage changes are missing + // or which ones are invalid + "Invalid storage modifications in the Bridge pool", + ); + tracing::debug!("{error}"); + return Err(error); + } + // check that gas was correctly escrowed. + if !Self::check_gas_escrow( ctx, - _marker: PhantomData, + &wnam_address, + &transfer, + escrow_checks.gas_check, + )? { + return Err(Error::new_const( + "Gas was not correctly escrowed into the Bridge pool storage", + )); + } + // check the escrowed assets + if transfer.transfer.asset == wnam_address { + Self::check_wnam_escrow( + ctx, + &wnam_address, + &transfer, + escrow_checks.token_check, + )? + .ok_or_else(|| { + Error::new_const( + "The wrapped NAM tokens were not escrowed properly", + ) + }) + } else { + Self::check_escrowed_toks(ctx, escrow_checks.token_check)? + .ok_or_else(|| { + Error::new_alloc(format!( + "The {} tokens were not escrowed properly", + transfer.transfer.asset + )) + }) } + .inspect(|_| { + tracing::info!( + "The Ethereum bridge pool VP accepted the transfer {:?}.", + transfer + ); + }) + .inspect_err(|err| { + tracing::debug!( + ?transfer, + reason = ?err, + "The assets of the transfer were not properly escrowed \ + into the Ethereum bridge pool." + ); + }) } /// Get the change in the balance of an account /// associated with an address fn account_balance_delta( - &self, + ctx: &'ctx CTX, token: &Address, address: &Address, - ) -> Result, Error> { + ) -> Result> { let account_key = TokenKeys::balance_key(token, address); - let before: Amount = (&self.ctx) - .read_pre_value(&account_key) + let before: Amount = ctx + .pre() + .read(&account_key) .map_err(|error| { tracing::warn!(?error, %account_key, "reading pre value"); error @@ -101,7 +218,7 @@ where // being credited, such as when we escrow gas under // the Bridge pool .unwrap_or_default(); - let after: Amount = match (&self.ctx).read_post_value(&account_key)? { + let after: Amount = match ctx.post().read(&account_key)? { Some(after) => after, None => { tracing::warn!(%account_key, "no post value"); @@ -123,10 +240,10 @@ where /// from the correct account into escrow. #[inline] fn check_escrowed_toks( - &self, + ctx: &'ctx CTX, delta: EscrowDelta<'_, K>, - ) -> Result { - self.check_escrowed_toks_balance(delta) + ) -> Result { + Self::check_escrowed_toks_balance(ctx, delta) .map(|balance| balance.is_some()) } @@ -134,9 +251,9 @@ where /// from the correct account into escrow, and return /// the updated escrow balance. fn check_escrowed_toks_balance( - &self, + ctx: &'ctx CTX, delta: EscrowDelta<'_, K>, - ) -> Result, Error> { + ) -> Result> { let EscrowDelta { token, payer_account, @@ -145,8 +262,8 @@ where expected_credit, .. } = delta; - let debit = self.account_balance_delta(&token, payer_account)?; - let credit = self.account_balance_delta(&token, escrow_account)?; + let debit = Self::account_balance_delta(ctx, &token, payer_account)?; + let credit = Self::account_balance_delta(ctx, &token, escrow_account)?; match (debit, credit) { // success case @@ -186,11 +303,11 @@ where /// Check that the gas was correctly escrowed. fn check_gas_escrow( - &self, + ctx: &'ctx CTX, wnam_address: &EthAddress, transfer: &PendingTransfer, gas_check: EscrowDelta<'_, GasCheck>, - ) -> Result { + ) -> Result { if hints::unlikely( *gas_check.token == erc20_token_address(wnam_address), ) { @@ -212,7 +329,7 @@ where ); return Ok(false); } - if !self.check_escrowed_toks(gas_check)? { + if !Self::check_escrowed_toks(ctx, gas_check)? { tracing::debug!( ?transfer, "The gas fees of the transfer were not properly escrowed into \ @@ -225,11 +342,11 @@ where /// Validate a wrapped NAM transfer to Ethereum. fn check_wnam_escrow( - &self, + ctx: &'ctx CTX, &wnam_address: &EthAddress, transfer: &PendingTransfer, token_check: EscrowDelta<'_, TokenCheck>, - ) -> Result { + ) -> Result { if hints::unlikely(matches!( &transfer.transfer.kind, TransferToEthereumKind::Nut @@ -251,7 +368,7 @@ where suffix: whitelist::KeyType::Whitelisted, } .into(); - (&self.ctx).read_pre_value(&key)?.unwrap_or(false) + ctx.pre().read(&key)?.unwrap_or(false) }; if !wnam_whitelisted { tracing::debug!( @@ -265,7 +382,7 @@ where // amount of Nam must be escrowed in the Ethereum bridge VP's // storage. let escrowed_balance = - match self.check_escrowed_toks_balance(token_check)? { + match Self::check_escrowed_toks_balance(ctx, token_check)? { Some(balance) => balance.resolve()?, None => return Ok(false), }; @@ -276,7 +393,7 @@ where suffix: whitelist::KeyType::Cap, } .into(); - (&self.ctx).read_pre_value(&key)?.unwrap_or_default() + ctx.pre().read(&key)?.unwrap_or_default() }; if escrowed_balance > I320::from(wnam_cap) { tracing::debug!( @@ -293,11 +410,11 @@ where } /// Determine the debit and credit amounts that should be checked. - fn determine_escrow_checks<'trans, 'this: 'trans>( - &'this self, + fn determine_escrow_checks<'trans>( + ctx: &'ctx CTX, wnam_address: &EthAddress, transfer: &'trans PendingTransfer, - ) -> Result, Error> { + ) -> Result> { let tok_is_native_asset = &transfer.transfer.asset == wnam_address; // NB: this comparison is not enough to check @@ -315,7 +432,7 @@ where let same_sender_and_fee_payer = transfer.gas_fee.payer == transfer.transfer.sender; let gas_is_native_asset = - transfer.gas_fee.token == self.ctx.state.in_mem().native_token; + transfer.gas_fee.token == ctx.get_native_token()?; let gas_and_token_is_native_asset = gas_is_native_asset && tok_is_native_asset; let same_token_and_gas_asset = @@ -347,7 +464,7 @@ where { // when minting wrapped NAM on Ethereum, escrow to the Ethereum // bridge address, and draw from NAM token accounts - let token = Cow::Borrowed(&self.ctx.state.in_mem().native_token); + let token = Cow::Owned(ctx.get_native_token()?); let escrow_account = &BRIDGE_ADDRESS; (token, escrow_account) } else { @@ -494,9 +611,7 @@ enum TokenCheck {} /// Sum gas and token amounts on a pending transfer, checking for overflows. #[inline] -fn sum_gas_and_token_amounts( - transfer: &PendingTransfer, -) -> Result { +fn sum_gas_and_token_amounts(transfer: &PendingTransfer) -> Result { transfer .gas_fee .amount @@ -508,146 +623,6 @@ fn sum_gas_and_token_amounts( }) } -impl<'view, 'ctx: 'view, S, CA, EVAL, TokenKeys> NativeVp<'view> - for BridgePool<'ctx, S, CA, EVAL, TokenKeys> -where - S: 'static + StateRead, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - CA: 'static + Clone, - TokenKeys: token::Keys, -{ - fn validate_tx( - &'view self, - batched_tx: &BatchedTxRef<'_>, - keys_changed: &BTreeSet, - _verifiers: &BTreeSet
, - ) -> Result<(), Error> { - tracing::debug!( - keys_changed_len = keys_changed.len(), - verifiers_len = _verifiers.len(), - "Ethereum Bridge Pool VP triggered", - ); - if !is_bridge_active_at( - &self.ctx.pre(), - self.ctx.state.in_mem().get_current_epoch().0, - )? { - tracing::debug!( - "Rejecting transaction, since the Ethereum bridge is disabled." - ); - return Err(Error::new_const( - "Rejecting transaction, since the Ethereum bridge is disabled.", - )); - } - let Some(tx_data) = batched_tx.tx.data(batched_tx.cmt) else { - return Err(Error::new_const("No transaction data found")); - }; - let transfer: PendingTransfer = - BorshDeserialize::try_from_slice(&tx_data[..]) - .into_storage_result()?; - - let pending_key = get_pending_key(&transfer); - // check that transfer is not already in the pool - match (&self.ctx).read_pre_value::(&pending_key) { - Ok(Some(_)) => { - let error = Error::new_const( - "Rejecting transaction as the transfer is already in the \ - Ethereum bridge pool.", - ); - tracing::debug!("{error}"); - return Err(error); - } - // NOTE: make sure we don't erase storage errors returned by the - // ctx, as these may contain gas errors! - Err(e) => return Err(e), - _ => {} - } - for key in keys_changed.iter().filter(|k| is_bridge_pool_key(k)) { - if *key != pending_key { - let error = Error::new_alloc(format!( - "Rejecting transaction as it is attempting to change an \ - incorrect key in the Ethereum bridge pool: {key}.\n \ - Expected key: {pending_key}", - )); - tracing::debug!("{error}"); - return Err(error); - } - } - let pending: PendingTransfer = - (&self.ctx).read_post_value(&pending_key)?.ok_or_else(|| { - Error::new_const( - "Rejecting transaction as the transfer wasn't added to \ - the pool of pending transfers", - ) - })?; - if pending != transfer { - let error = Error::new_alloc(format!( - "An incorrect transfer was added to the Ethereum bridge pool: \ - {transfer:?}.\n Expected: {pending:?}", - )); - tracing::debug!("{error}"); - return Err(error); - } - // The deltas in the escrowed amounts we must check. - let wnam_address = read_native_erc20_address(&self.ctx.pre())?; - let escrow_checks = - self.determine_escrow_checks(&wnam_address, &transfer)?; - if !escrow_checks.validate::(keys_changed) { - let error = Error::new_const( - // TODO(namada#3247): specify which storage changes are missing - // or which ones are invalid - "Invalid storage modifications in the Bridge pool", - ); - tracing::debug!("{error}"); - return Err(error); - } - // check that gas was correctly escrowed. - if !self.check_gas_escrow( - &wnam_address, - &transfer, - escrow_checks.gas_check, - )? { - return Err(Error::new_const( - "Gas was not correctly escrowed into the Bridge pool storage", - )); - } - // check the escrowed assets - if transfer.transfer.asset == wnam_address { - self.check_wnam_escrow( - &wnam_address, - &transfer, - escrow_checks.token_check, - )? - .ok_or_else(|| { - Error::new_const( - "The wrapped NAM tokens were not escrowed properly", - ) - }) - } else { - self.check_escrowed_toks(escrow_checks.token_check)? - .ok_or_else(|| { - Error::new_alloc(format!( - "The {} tokens were not escrowed properly", - transfer.transfer.asset - )) - }) - } - .inspect(|_| { - tracing::info!( - "The Ethereum bridge pool VP accepted the transfer {:?}.", - transfer - ); - }) - .inspect_err(|err| { - tracing::debug!( - ?transfer, - reason = ?err, - "The assets of the transfer were not properly escrowed \ - into the Ethereum bridge pool." - ); - }) - } -} - #[allow(clippy::arithmetic_side_effects)] #[cfg(test)] mod test_bridge_pool_vp { @@ -661,13 +636,14 @@ mod test_bridge_pool_vp { use namada_gas::{TxGasMeter, VpGasMeter}; use namada_state::testing::TestState; use namada_state::write_log::WriteLog; - use namada_state::{StorageWrite, TxIndex}; + use namada_state::{StateRead, StorageWrite, TxIndex}; use namada_trans_token::storage_key::balance_key; use namada_tx::data::TxType; use namada_tx::Tx; use namada_vm::wasm::run::VpEvalWasm; use namada_vm::wasm::VpCache; use namada_vm::WasmCacheRwAccess; + use namada_vp::native_vp; use super::*; use crate::storage::bridge_pool::get_signed_root_key; @@ -677,14 +653,10 @@ mod test_bridge_pool_vp { use crate::storage::wrapped_erc20s; type CA = WasmCacheRwAccess; - type Eval = VpEvalWasm< - ::D, - ::H, - CA, - >; + type Eval = VpEvalWasm<::D, ::H, CA>; + type Ctx<'ctx, S> = native_vp::Ctx<'ctx, S, VpCache, Eval>; type TokenKeys = namada_token::Store<()>; - type BridgePool<'a, S> = - super::BridgePool<'a, S, VpCache, Eval, TokenKeys>; + type BridgePool<'ctx, S> = super::BridgePool<'ctx, Ctx<'ctx, S>, TokenKeys>; /// The amount of NAM Bertha has const ASSET: EthAddress = EthAddress([0; 20]); @@ -941,7 +913,7 @@ mod test_bridge_pool_vp { gas_meter: &'ctx RefCell, keys_changed: &'ctx BTreeSet, verifiers: &'ctx BTreeSet
, - ) -> Ctx<'ctx, TestState, VpCache, Eval> { + ) -> Ctx<'ctx, TestState> { let batched_tx = tx.batch_ref_first_tx().unwrap(); Ctx::new( &BRIDGE_POOL_ADDRESS, @@ -1031,19 +1003,13 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); match (expect, res) { (Expect::Accepted, Ok(())) => (), (Expect::Accepted, Err(err)) => { @@ -1393,19 +1359,13 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); assert!(res.is_err()); } @@ -1457,19 +1417,13 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); assert!(res.is_err()); } @@ -1542,19 +1496,13 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); assert!(res.is_ok()); } @@ -1622,19 +1570,13 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); assert!(res.is_err()); } @@ -1719,19 +1661,13 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::new(state.in_mem().chain_id.clone(), None); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); assert!(res.is_err()); } @@ -1802,20 +1738,14 @@ mod test_bridge_pool_vp { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let vp = BridgePool::new(setup_ctx( - &tx, - &state, - &gas_meter, - &keys_changed, - &verifiers, - )); + let ctx = setup_ctx(&tx, &state, &gas_meter, &keys_changed, &verifiers); let mut tx = Tx::from_type(TxType::Raw); tx.push_default_inner_tx(); tx.add_data(transfer); let tx = tx.batch_ref_first_tx().unwrap(); - let res = vp.validate_tx(&tx, &keys_changed, &verifiers); + let res = BridgePool::validate_tx(&ctx, &tx, &keys_changed, &verifiers); match (expect, res) { (Expect::Accepted, Ok(())) => (), (Expect::Accepted, Err(err)) => { diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index a287456ff1..a056d93d06 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1297,14 +1297,13 @@ where .map_err(Error::NativeVpError) } InternalAddress::EthBridgePool => { - let bridge_pool = EthBridgePoolVp::new(ctx); - bridge_pool - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) + EthBridgePoolVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError) } InternalAddress::Nut(_) => { let non_usable_tokens = diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index 16e3145703..28b4f585d4 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -97,8 +97,8 @@ pub type EthBridgeVp<'a, S, CA> = eth_bridge::vp::EthBridge<'a, S, VpCache, Eval, TokenKeys>; /// Native ETH bridge pool VP -pub type EthBridgePoolVp<'a, S, CA> = - eth_bridge::vp::BridgePool<'a, S, VpCache, Eval, TokenKeys>; +pub type EthBridgePoolVp<'ctx, CTX> = + eth_bridge::vp::BridgePool<'ctx, CTX, TokenKeys>; /// Native ETH bridge NUT VP pub type EthBridgeNutVp<'a, S, CA> = diff --git a/crates/tests/src/native_vp/eth_bridge_pool.rs b/crates/tests/src/native_vp/eth_bridge_pool.rs index 686fadc0c6..d2f556b688 100644 --- a/crates/tests/src/native_vp/eth_bridge_pool.rs +++ b/crates/tests/src/native_vp/eth_bridge_pool.rs @@ -118,8 +118,14 @@ mod test_bridge_pool_vp { )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, BRIDGE_POOL_ADDRESS); - let vp = vp_env.init_vp(&gas_meter, EthBridgePoolVp::new); - vp_env.validate_tx(&vp).is_ok() + let ctx = vp_env.ctx(&gas_meter); + EthBridgePoolVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ) + .is_ok() } fn validate_tx(tx: BatchedTx) { diff --git a/crates/tests/src/native_vp/mod.rs b/crates/tests/src/native_vp/mod.rs index 677c3827ae..c124448ef1 100644 --- a/crates/tests/src/native_vp/mod.rs +++ b/crates/tests/src/native_vp/mod.rs @@ -75,6 +75,24 @@ impl TestNativeVpEnv { init_native_vp(ctx) } + /// Instantiate a native VP ctx to run a VP + pub fn ctx<'ctx>( + &'ctx self, + gas_meter: &'ctx RefCell, + ) -> NativeVpCtx<'ctx> { + NativeVpCtx::new( + &self.address, + &self.tx_env.state, + &self.tx_env.batched_tx.tx, + &self.tx_env.batched_tx.cmt, + &self.tx_env.tx_index, + gas_meter, + &self.keys_changed, + &self.verifiers, + self.tx_env.vp_wasm_cache.clone(), + ) + } + /// Run some transaction code `apply_tx` and validate it with a native VP pub fn validate_tx<'view, 'ctx: 'view, T>( &'ctx self, diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index bbd144b83b..9df36fbbe9 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3718,6 +3718,7 @@ dependencies = [ "namada_tx", "namada_vote_ext", "namada_vp", + "namada_vp_env", "serde", "smooth-operator", "thiserror", From 1fd64101600e1cad3b387f2cf9da5260b4aa1695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 13:20:17 +0200 Subject: [PATCH 05/10] eth_bridge: env agnostic VP --- crates/benches/native_vps.rs | 18 +- .../ethereum_bridge/src/vp/eth_bridge_vp.rs | 168 ++++++++---------- crates/node/src/protocol.rs | 15 +- crates/sdk/src/validation.rs | 4 +- 4 files changed, 95 insertions(+), 110 deletions(-) diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index e537e2357e..ee7964a81e 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -1393,7 +1393,7 @@ fn eth_bridge(c: &mut Criterion) { let vp_address = Address::Internal(InternalAddress::EthBridge); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter(&TxGasMeter::new(u64::MAX))); - let eth_bridge = EthBridgeVp::new(Ctx::new( + let ctx = Ctx::new( &vp_address, &shell.state, &signed_tx.tx, @@ -1403,18 +1403,18 @@ fn eth_bridge(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); c.bench_function("vp_eth_bridge", |b| { b.iter(|| { assert!( - eth_bridge - .validate_tx( - &signed_tx.to_ref(), - eth_bridge.ctx.keys_changed, - eth_bridge.ctx.verifiers, - ) - .is_ok() + EthBridgeVp::validate_tx( + &ctx, + &signed_tx.to_ref(), + ctx.keys_changed, + ctx.verifiers, + ) + .is_ok() ) }) }); diff --git a/crates/ethereum_bridge/src/vp/eth_bridge_vp.rs b/crates/ethereum_bridge/src/vp/eth_bridge_vp.rs index ff7b1a4873..40caee6985 100644 --- a/crates/ethereum_bridge/src/vp/eth_bridge_vp.rs +++ b/crates/ethereum_bridge/src/vp/eth_bridge_vp.rs @@ -7,55 +7,68 @@ use namada_core::address::Address; use namada_core::booleans::BoolResultUnitExt; use namada_core::collections::HashSet; use namada_core::storage::Key; -use namada_state::StateRead; use namada_systems::trans_token::{self as token, Amount}; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{ - self, Ctx, NativeVp, Result, StorageReader, VpEvaluator, -}; +use namada_vp_env::{Error, Result, StorageRead, VpEnv}; use crate::storage; use crate::storage::escrow_key; /// Validity predicate for the Ethereum bridge -pub struct EthBridge<'ctx, S, CA, EVAL, TokenKeys> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct EthBridge<'ctx, CTX, TokenKeys> { /// Generic types for DI - pub _marker: PhantomData, + pub _marker: PhantomData<(&'ctx CTX, TokenKeys)>, } -impl<'ctx, S, CA, EVAL, TokenKeys> EthBridge<'ctx, S, CA, EVAL, TokenKeys> +impl<'ctx, CTX, TokenKeys> EthBridge<'ctx, CTX, TokenKeys> where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, + CTX: VpEnv<'ctx> + namada_tx::action::Read, TokenKeys: token::Keys, { - /// Instantiate eth bridge VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, - } + /// Validate that a wasm transaction is permitted to change keys under this + /// account. + /// + /// We only permit increasing the escrowed balance of NAM under the Ethereum + /// bridge address, when writing to storage from wasm transactions. + /// + /// Some other changes to the storage subspace of this account are expected + /// to happen natively i.e. bypassing this validity predicate. For example, + /// changes to the `eth_msgs/...` keys. For those cases, we reject here as + /// no wasm transactions should be able to modify those keys. + pub fn validate_tx( + ctx: &'ctx CTX, + _: &BatchedTxRef<'_>, + keys_changed: &BTreeSet, + verifiers: &BTreeSet
, + ) -> Result<()> { + tracing::debug!( + keys_changed_len = keys_changed.len(), + verifiers_len = verifiers.len(), + "Ethereum Bridge VP triggered", + ); + + let native_token = ctx.get_native_token()?; + validate_changed_keys::(&native_token, keys_changed)?; + + Self::check_escrow(ctx, &native_token, verifiers) } /// If the Ethereum bridge's escrow key was written to, we check /// that the NAM balance increased and that the Bridge pool VP has /// been triggered. - fn check_escrow(&self, verifiers: &BTreeSet
) -> Result<()> { - let escrow_key = TokenKeys::balance_key( - &self.ctx.state.in_mem().native_token, - &crate::ADDRESS, - ); + fn check_escrow( + ctx: &'ctx CTX, + native_token: &Address, + verifiers: &BTreeSet
, + ) -> Result<()> { + let escrow_key = TokenKeys::balance_key(native_token, &crate::ADDRESS); let escrow_pre: Amount = - (&self.ctx).read_pre_value(&escrow_key)?.unwrap_or_default(); - let escrow_post: Amount = - (&self.ctx).must_read_post_value(&escrow_key)?; + ctx.pre().read(&escrow_key)?.unwrap_or_default(); + let escrow_post: Amount = ctx + .post() + .read(&escrow_key)? + .ok_or_else(|| Error::new_const("Escrow must be present"))?; // The amount escrowed should increase. if escrow_pre < escrow_post { @@ -65,13 +78,13 @@ where verifiers.contains(&storage::bridge_pool::BRIDGE_POOL_ADDRESS); bridge_pool_is_verifier.ok_or_else(|| { - native_vp::Error::new_const( + Error::new_const( "Bridge pool VP was not marked as a verifier of the \ transaction", ) }) } else { - Err(native_vp::Error::new_const( + Err(Error::new_const( "User tx attempted to decrease the amount of native tokens \ escrowed in the Ethereum Bridge's account", )) @@ -79,45 +92,6 @@ where } } -impl<'view, 'ctx: 'view, S, CA, EVAL, TokenKeys> NativeVp<'view> - for EthBridge<'ctx, S, CA, EVAL, TokenKeys> -where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - TokenKeys: token::Keys, -{ - /// Validate that a wasm transaction is permitted to change keys under this - /// account. - /// - /// We only permit increasing the escrowed balance of NAM under the Ethereum - /// bridge address, when writing to storage from wasm transactions. - /// - /// Some other changes to the storage subspace of this account are expected - /// to happen natively i.e. bypassing this validity predicate. For example, - /// changes to the `eth_msgs/...` keys. For those cases, we reject here as - /// no wasm transactions should be able to modify those keys. - fn validate_tx( - &'view self, - _: &BatchedTxRef<'_>, - keys_changed: &BTreeSet, - verifiers: &BTreeSet
, - ) -> Result<()> { - tracing::debug!( - keys_changed_len = keys_changed.len(), - verifiers_len = verifiers.len(), - "Ethereum Bridge VP triggered", - ); - - validate_changed_keys::( - &self.ctx.state.in_mem().native_token, - keys_changed, - )?; - - self.check_escrow(verifiers) - } -} - /// Checks if `keys_changed` represents a valid set of changed keys. /// /// This implies checking if two distinct keys were changed: @@ -143,7 +117,7 @@ fn validate_changed_keys( }) .collect(); if keys_changed.is_empty() { - return Err(native_vp::Error::SimpleMessage( + return Err(Error::SimpleMessage( "No keys changed under our account so this validity predicate \ shouldn't have been triggered", )); @@ -154,7 +128,7 @@ fn validate_changed_keys( ); let nam_escrow_addr_modified = keys_changed.contains(&escrow_key(nam_addr)); if !nam_escrow_addr_modified { - let error = native_vp::Error::new_const( + let error = Error::new_const( "The native token's escrow balance should have been modified", ); tracing::debug!("{error}"); @@ -164,7 +138,7 @@ fn validate_changed_keys( .iter() .all(|key| TokenKeys::is_balance_key(nam_addr, key).is_some()); if !all_keys_are_nam_balance { - let error = native_vp::Error::new_const( + let error = Error::new_const( "Some modified keys were not a native token's balance key", ); tracing::debug!("{error}"); @@ -184,13 +158,14 @@ mod tests { use namada_core::ethereum_events::EthAddress; use namada_gas::{TxGasMeter, VpGasMeter}; use namada_state::testing::TestState; - use namada_state::{StorageWrite, TxIndex}; + use namada_state::{StateRead, StorageWrite, TxIndex}; use namada_trans_token::storage_key::{balance_key, minted_balance_key}; use namada_tx::data::TxType; use namada_tx::{Tx, TxCommitments}; use namada_vm::wasm::run::VpEvalWasm; use namada_vm::wasm::VpCache; use namada_vm::WasmCacheRwAccess; + use namada_vp::native_vp; use rand::Rng; use super::*; @@ -207,14 +182,10 @@ mod tests { const BRIDGE_POOL_ESCROW_INITIAL_BALANCE: u64 = 0; type CA = WasmCacheRwAccess; - type Eval = VpEvalWasm< - ::D, - ::H, - CA, - >; + type Eval = VpEvalWasm<::D, ::H, CA>; + type Ctx<'ctx, S> = native_vp::Ctx<'ctx, S, VpCache, Eval>; type TokenKeys = namada_token::Store<()>; - type EthBridge<'a, S> = - super::EthBridge<'a, S, VpCache, Eval, TokenKeys>; + type EthBridge<'ctx, S> = super::EthBridge<'ctx, Ctx<'ctx, S>, TokenKeys>; /// Return some arbitrary random key belonging to this account fn arbitrary_key() -> Key { @@ -267,7 +238,7 @@ mod tests { gas_meter: &'ctx RefCell, keys_changed: &'ctx BTreeSet, verifiers: &'ctx BTreeSet
, - ) -> Ctx<'ctx, TestState, VpCache, Eval> { + ) -> Ctx<'ctx, TestState> { Ctx::new( &crate::ADDRESS, state, @@ -412,16 +383,21 @@ mod tests { &TxGasMeter::new(u64::MAX), )); let batched_tx = tx.batch_ref_first_tx().unwrap(); - let vp = EthBridge::new(setup_ctx( + let ctx = setup_ctx( batched_tx.tx, batched_tx.cmt, &state, &gas_meter, &keys_changed, &verifiers, - )); + ); - let res = vp.validate_tx(&batched_tx, &keys_changed, &verifiers); + let res = EthBridge::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers, + ); assert!(res.is_ok()); } @@ -464,16 +440,21 @@ mod tests { &TxGasMeter::new(u64::MAX), )); let batched_tx = tx.batch_ref_first_tx().unwrap(); - let vp = EthBridge::new(setup_ctx( + let ctx = setup_ctx( batched_tx.tx, batched_tx.cmt, &state, &gas_meter, &keys_changed, &verifiers, - )); + ); - let res = vp.validate_tx(&batched_tx, &keys_changed, &verifiers); + let res = EthBridge::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers, + ); assert!(res.is_err()); } @@ -519,16 +500,21 @@ mod tests { &TxGasMeter::new(u64::MAX), )); let batched_tx = tx.batch_ref_first_tx().unwrap(); - let vp = EthBridge::new(setup_ctx( + let ctx = setup_ctx( batched_tx.tx, batched_tx.cmt, &state, &gas_meter, &keys_changed, &verifiers, - )); + ); - let res = vp.validate_tx(&batched_tx, &keys_changed, &verifiers); + let res = EthBridge::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers, + ); assert!(res.is_err()); } } diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index a056d93d06..c90627bd95 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1287,14 +1287,13 @@ where .map_err(Error::NativeVpError) } InternalAddress::EthBridge => { - let bridge = EthBridgeVp::new(ctx); - bridge - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) + EthBridgeVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError) } InternalAddress::EthBridgePool => { EthBridgePoolVp::validate_tx( diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index 28b4f585d4..13fb1e5590 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -93,8 +93,8 @@ pub type MaspVp<'a, S, CA> = token::vp::MaspVp< >; /// Native ETH bridge VP -pub type EthBridgeVp<'a, S, CA> = - eth_bridge::vp::EthBridge<'a, S, VpCache, Eval, TokenKeys>; +pub type EthBridgeVp<'ctx, CTX> = + eth_bridge::vp::EthBridge<'ctx, CTX, TokenKeys>; /// Native ETH bridge pool VP pub type EthBridgePoolVp<'ctx, CTX> = From 2c2c32c3afab554daa51e2edea96874e835da4a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 13:51:58 +0200 Subject: [PATCH 06/10] eth/nut_vp: env agnostic VP --- crates/benches/native_vps.rs | 11 +-- crates/ethereum_bridge/Cargo.toml | 2 +- crates/ethereum_bridge/src/vp/nut_vp.rs | 91 ++++++++----------------- crates/node/src/protocol.rs | 16 ++--- crates/sdk/src/validation.rs | 4 +- wasm/Cargo.lock | 1 - 6 files changed, 46 insertions(+), 79 deletions(-) diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index ee7964a81e..fbcd125165 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -1322,7 +1322,7 @@ fn eth_bridge_nut(c: &mut Criterion) { Address::Internal(InternalAddress::Nut(native_erc20_addres)); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter(&TxGasMeter::new(u64::MAX))); - let nut = EthBridgeNutVp::new(Ctx::new( + let ctx = Ctx::new( &vp_address, &shell.state, &signed_tx.tx, @@ -1332,15 +1332,16 @@ fn eth_bridge_nut(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); c.bench_function("vp_eth_bridge_nut", |b| { b.iter(|| { assert!( - nut.validate_tx( + EthBridgeNutVp::validate_tx( + &ctx, &signed_tx.to_ref(), - nut.ctx.keys_changed, - nut.ctx.verifiers, + ctx.keys_changed, + ctx.verifiers, ) .is_ok() ) diff --git a/crates/ethereum_bridge/Cargo.toml b/crates/ethereum_bridge/Cargo.toml index f6611d940f..18425c0f54 100644 --- a/crates/ethereum_bridge/Cargo.toml +++ b/crates/ethereum_bridge/Cargo.toml @@ -41,7 +41,6 @@ namada_systems = { path = "../systems" } namada_trans_token = {path = "../trans_token"} namada_tx = {path = "../tx"} namada_vote_ext = {path = "../vote_ext"} -namada_vp = {path = "../vp"} namada_vp_env = {path = "../vp_env"} borsh.workspace = true @@ -65,6 +64,7 @@ namada_state = { path = "../state", features = ["testing"] } namada_token = {path = "../token", features = ["testing"]} namada_tx = {path = "../tx", features = ["testing"]} namada_vm = {path = "../vm", features = ["testing"]} +namada_vp = {path = "../vp"} assert_matches.workspace = true data-encoding.workspace = true diff --git a/crates/ethereum_bridge/src/vp/nut_vp.rs b/crates/ethereum_bridge/src/vp/nut_vp.rs index fe0356afd3..07934440ae 100644 --- a/crates/ethereum_bridge/src/vp/nut_vp.rs +++ b/crates/ethereum_bridge/src/vp/nut_vp.rs @@ -6,36 +6,27 @@ use std::marker::PhantomData; use namada_core::address::{Address, InternalAddress}; use namada_core::booleans::BoolResultUnitExt; use namada_core::storage::Key; -use namada_state::StateRead; use namada_systems::trans_token::{self as token, Amount}; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{self, Ctx, Error, NativeVp, Result, VpEvaluator}; -use namada_vp::VpEnv; +use namada_vp_env::{Error, Result, VpEnv}; /// Validity predicate for non-usable tokens. /// /// All this VP does is reject NUT transfers whose destination /// address is not the Bridge pool escrow address. -pub struct NonUsableTokens<'ctx, S, CA, EVAL, TokenKeys> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct NonUsableTokens<'ctx, CTX, TokenKeys> { /// Generic types for DI - pub _marker: PhantomData, + pub _marker: PhantomData<(&'ctx CTX, TokenKeys)>, } -impl<'view, 'ctx: 'view, S, CA, EVAL, TokenKeys> NativeVp<'view> - for NonUsableTokens<'ctx, S, CA, EVAL, TokenKeys> +impl<'ctx, CTX, TokenKeys> NonUsableTokens<'ctx, CTX, TokenKeys> where - S: 'static + StateRead, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - CA: 'static + Clone, + CTX: VpEnv<'ctx> + namada_tx::action::Read, TokenKeys: token::Keys, { - fn validate_tx( - &'view self, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, _: &BatchedTxRef<'_>, keys_changed: &BTreeSet, verifiers: &BTreeSet
, @@ -65,10 +56,8 @@ where }); for (changed_key, token_owner) in nut_owners { - let pre: Amount = - self.ctx.read_pre(changed_key)?.unwrap_or_default(); - let post: Amount = - self.ctx.read_post(changed_key)?.unwrap_or_default(); + let pre: Amount = ctx.read_pre(changed_key)?.unwrap_or_default(); + let post: Amount = ctx.read_post(changed_key)?.unwrap_or_default(); match token_owner { // the NUT balance of the bridge pool should increase @@ -80,7 +69,7 @@ where post_amount = ?post, "Bridge pool balance should have increased" ); - return Err(native_vp::Error::new_alloc(format!( + return Err(Error::new_alloc(format!( "Bridge pool balance should have increased. The \ previous balance was {pre:?}, the post balance \ is {post:?}.", @@ -96,7 +85,7 @@ where post_amount = ?post, "Balance should have decreased" ); - return Err(native_vp::Error::new_alloc(format!( + return Err(Error::new_alloc(format!( "Balance should have decreased. The previous \ balance was {pre:?}, the post balance is \ {post:?}." @@ -110,22 +99,6 @@ where } } -impl<'ctx, S, CA, EVAL, TokenKeys> NonUsableTokens<'ctx, S, CA, EVAL, TokenKeys> -where - S: 'static + StateRead, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - CA: 'static + Clone, - TokenKeys: token::Keys, -{ - /// Instantiate NUT VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, - } - } -} - #[cfg(test)] mod test_nuts { use std::cell::RefCell; @@ -137,27 +110,25 @@ mod test_nuts { use namada_core::storage::TxIndex; use namada_gas::{TxGasMeter, VpGasMeter}; use namada_state::testing::TestState; - use namada_state::StorageWrite; + use namada_state::{StateRead, StorageWrite}; use namada_trans_token::storage_key::balance_key; use namada_tx::data::TxType; use namada_tx::Tx; use namada_vm::wasm::run::VpEvalWasm; use namada_vm::wasm::VpCache; use namada_vm::WasmCacheRwAccess; + use namada_vp::native_vp; use proptest::prelude::*; use super::*; use crate::storage::wrapped_erc20s; type CA = WasmCacheRwAccess; - type Eval = VpEvalWasm< - ::D, - ::H, - CA, - >; + type Eval = VpEvalWasm<::D, ::H, CA>; + type Ctx<'ctx, S> = native_vp::Ctx<'ctx, S, VpCache, Eval>; type TokenKeys = namada_token::Store<()>; - type NonUsableTokens<'a, S> = - super::NonUsableTokens<'a, S, VpCache, Eval, TokenKeys>; + type NonUsableTokens<'ctx, S> = + super::NonUsableTokens<'ctx, Ctx<'ctx, S>, TokenKeys>; /// Run a VP check on a NUT transfer between the two provided addresses. fn check_nut_transfer(src: Address, dst: Address) -> bool { @@ -215,7 +186,7 @@ mod test_nuts { &TxGasMeter::new(u64::MAX), )); let batched_tx = tx.batch_ref_first_tx().unwrap(); - let ctx = Ctx::<_, VpCache, Eval>::new( + let ctx = Ctx::new( &Address::Internal(InternalAddress::Nut(DAI_ERC20_ETH_ADDRESS)), &state, batched_tx.tx, @@ -226,25 +197,23 @@ mod test_nuts { &verifiers, VpCache::new(temp_dir(), 100usize), ); - let vp = NonUsableTokens::new(ctx); // print debug info in case we run into failures for key in &keys_changed { - let pre: Amount = vp - .ctx - .read_pre(key) - .expect("Test failed") - .unwrap_or_default(); - let post: Amount = vp - .ctx - .read_post(key) - .expect("Test failed") - .unwrap_or_default(); + let pre: Amount = + ctx.read_pre(key).expect("Test failed").unwrap_or_default(); + let post: Amount = + ctx.read_post(key).expect("Test failed").unwrap_or_default(); println!("{key}: PRE={pre:?} POST={post:?}"); } - vp.validate_tx(&batched_tx, &keys_changed, &verifiers) - .map_or_else(|_| false, |()| true) + NonUsableTokens::validate_tx( + &ctx, + &batched_tx, + &keys_changed, + &verifiers, + ) + .map_or_else(|_| false, |()| true) } proptest! { diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index c90627bd95..39f6956e9b 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1305,15 +1305,13 @@ where .map_err(Error::NativeVpError) } InternalAddress::Nut(_) => { - let non_usable_tokens = - EthBridgeNutVp::new(ctx); - non_usable_tokens - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) + EthBridgeNutVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError) } internal_addr @ (InternalAddress::IbcToken(_) | InternalAddress::Erc20(_)) => { diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index 13fb1e5590..3f2bd91e5c 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -101,8 +101,8 @@ pub type EthBridgePoolVp<'ctx, CTX> = eth_bridge::vp::BridgePool<'ctx, CTX, TokenKeys>; /// Native ETH bridge NUT VP -pub type EthBridgeNutVp<'a, S, CA> = - eth_bridge::vp::NonUsableTokens<'a, S, VpCache, Eval, TokenKeys>; +pub type EthBridgeNutVp<'ctx, CTX> = + eth_bridge::vp::NonUsableTokens<'ctx, CTX, TokenKeys>; /// Governance store implementation over the native prior context pub type GovPreStore<'a, S, CA> = diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index 9df36fbbe9..0a030e6f63 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3717,7 +3717,6 @@ dependencies = [ "namada_trans_token", "namada_tx", "namada_vote_ext", - "namada_vp", "namada_vp_env", "serde", "smooth-operator", From bc62e43e8baa0cbaec429ae4a7f6b411f2e93bd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 16:20:04 +0200 Subject: [PATCH 07/10] pos: env agnostic VP --- Cargo.lock | 2 +- crates/benches/native_vps.rs | 11 ++-- crates/node/src/protocol.rs | 16 +++--- crates/proof_of_stake/Cargo.toml | 2 +- crates/proof_of_stake/src/vp.rs | 55 +++++-------------- crates/sdk/src/validation.rs | 10 ++-- crates/tests/src/native_vp/pos.rs | 9 ++- wasm/Cargo.lock | 2 +- wasm/tx_bond/src/lib.rs | 9 ++- .../tx_change_validator_commission/src/lib.rs | 9 ++- wasm/tx_redelegate/src/lib.rs | 9 ++- wasm/tx_unbond/src/lib.rs | 9 ++- wasm/tx_withdraw/src/lib.rs | 9 ++- wasm_for_tests/Cargo.lock | 2 +- 14 files changed, 78 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 42f7edfe6c..e324392e6a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5262,7 +5262,7 @@ dependencies = [ "namada_systems", "namada_trans_token", "namada_tx", - "namada_vp", + "namada_vp_env", "once_cell", "pretty_assertions", "proptest", diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index fbcd125165..4afe4ca92b 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -1640,7 +1640,7 @@ fn pos(c: &mut Criterion) { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let pos = PosVp::new(Ctx::new( + let ctx = Ctx::new( &vp_address, &shell.state, &signed_tx.tx, @@ -1650,15 +1650,16 @@ fn pos(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); group.bench_function(bench_name, |b| { b.iter(|| { assert!( - pos.validate_tx( + PosVp::validate_tx( + &ctx, &signed_tx.to_ref(), - pos.ctx.keys_changed, - pos.ctx.verifiers, + ctx.keys_changed, + ctx.verifiers, ) .is_ok() ) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 39f6956e9b..4dbc5f9b44 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1219,15 +1219,13 @@ where ); match internal_addr { - InternalAddress::PoS => { - let pos = PosVp::new(ctx); - pos.validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) - } + InternalAddress::PoS => PosVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError), InternalAddress::Ibc => { let ibc = IbcVp::new(ctx); ibc.validate_tx( diff --git a/crates/proof_of_stake/Cargo.toml b/crates/proof_of_stake/Cargo.toml index 09eadcbc56..6e7da54a42 100644 --- a/crates/proof_of_stake/Cargo.toml +++ b/crates/proof_of_stake/Cargo.toml @@ -31,7 +31,7 @@ namada_migrations = { path = "../migrations", optional = true } namada_state = { path = "../state" } namada_systems = { path = "../systems" } namada_tx = { path = "../tx" } -namada_vp = { path = "../vp" } +namada_vp_env = { path = "../vp_env" } borsh.workspace = true konst.workspace = true diff --git a/crates/proof_of_stake/src/vp.rs b/crates/proof_of_stake/src/vp.rs index fccffb796f..dcb2d989f1 100644 --- a/crates/proof_of_stake/src/vp.rs +++ b/crates/proof_of_stake/src/vp.rs @@ -6,15 +6,12 @@ use std::marker::PhantomData; use namada_core::address::Address; use namada_core::booleans::BoolResultUnitExt; use namada_core::storage::Key; -use namada_state::StateRead; use namada_systems::governance; use namada_tx::action::{ - Action, Bond, ClaimRewards, PosAction, Read, Redelegation, Unbond, Withdraw, + Action, Bond, ClaimRewards, PosAction, Redelegation, Unbond, Withdraw, }; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{ - Ctx, CtxPreStorageRead, Error, NativeVp, Result, VpEvaluator, -}; +use namada_vp_env::{Error, Result, VpEnv}; use thiserror::Error; use crate::storage::read_owned_pos_params; @@ -38,26 +35,19 @@ impl From for Error { } /// Proof-of-Stake validity predicate -pub struct PosVp<'ctx, S, CA, EVAL, Gov> -where - S: StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct PosVp<'ctx, CTX, Gov> { /// Generic types for DI - pub _marker: PhantomData, + pub _marker: PhantomData<(&'ctx CTX, Gov)>, } -impl<'view, 'ctx: 'view, S, CA, EVAL, Gov> NativeVp<'view> - for PosVp<'ctx, S, CA, EVAL, Gov> +impl<'ctx, CTX, Gov> PosVp<'ctx, CTX, Gov> where - S: StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Gov: governance::Read>, + CTX: VpEnv<'ctx> + namada_tx::action::Read, + Gov: governance::Read<>::Pre>, { - fn validate_tx( - &'view self, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, keys_changed: &BTreeSet, verifiers: &BTreeSet
, @@ -68,7 +58,7 @@ where if batched_tx .tx .data(batched_tx.cmt) - .map(|tx_data| Gov::is_proposal_accepted(&self.ctx.pre(), &tx_data)) + .map(|tx_data| Gov::is_proposal_accepted(&ctx.pre(), &tx_data)) .transpose()? .unwrap_or(false) { @@ -76,7 +66,7 @@ where if is_params_key(key) { // If governance changes PoS params, the params have to be // valid - self.is_valid_parameter_change()?; + Self::is_valid_parameter_change(ctx)?; } // Any other change from governance is allowed without further // checks @@ -85,7 +75,7 @@ where } // Find the actions applied in the tx - let actions = self.ctx.read_actions()?; + let actions = ctx.read_actions()?; // There must be at least one action if actions.is_empty() @@ -320,26 +310,11 @@ where } Ok(()) } -} - -impl<'view, 'ctx: 'view, S, CA, EVAL, Gov> PosVp<'ctx, S, CA, EVAL, Gov> -where - S: StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, -{ - /// Instantiate a `PosVP`. - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, - } - } /// Return `Ok` if the changed parameters are valid - fn is_valid_parameter_change(&self) -> Result<()> { + fn is_valid_parameter_change(ctx: &'ctx CTX) -> Result<()> { let validation_errors: Vec = - read_owned_pos_params(&self.ctx.post())?.validate(); + read_owned_pos_params(&ctx.post())?.validate(); validation_errors.is_empty().ok_or_else(|| { let validation_errors_str = itertools::join(validation_errors, ", "); diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index 3f2bd91e5c..4004eb9c77 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -17,12 +17,10 @@ pub type NativeVpCtx<'a, S, CA> = type Eval = VpEvalWasm<::D, ::H, CA>; /// Native PoS VP -pub type PosVp<'a, S, CA> = proof_of_stake::vp::PosVp< - 'a, - S, - VpCache, - Eval, - GovPreStore<'a, S, CA>, +pub type PosVp<'ctx, CTX> = proof_of_stake::vp::PosVp< + 'ctx, + CTX, + governance::Store<>::Pre>, >; /// Native IBC VP diff --git a/crates/tests/src/native_vp/pos.rs b/crates/tests/src/native_vp/pos.rs index cba0a0fd87..2c03c43b29 100644 --- a/crates/tests/src/native_vp/pos.rs +++ b/crates/tests/src/native_vp/pos.rs @@ -449,8 +449,13 @@ mod tests { &tx_env.gas_meter.borrow(), )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, address::POS); - let vp = vp_env.init_vp(&gas_meter, PosVp::new); - let result = vp_env.validate_tx(&vp); + let ctx = vp_env.ctx(&gas_meter); + let result = PosVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ); // Put the tx_env back before checking the result tx_host_env::set(vp_env.tx_env); diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index 0a030e6f63..ab53eefae6 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3869,7 +3869,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "once_cell", "proptest", "serde", diff --git a/wasm/tx_bond/src/lib.rs b/wasm/tx_bond/src/lib.rs index b8a1f60536..e894e28c38 100644 --- a/wasm/tx_bond/src/lib.rs +++ b/wasm/tx_bond/src/lib.rs @@ -329,8 +329,13 @@ mod tests { &tx_env.gas_meter.borrow(), )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, address::POS); - let vp = vp_env.init_vp(&gas_meter, PosVp::new); - let result = vp_env.validate_tx(&vp); + let ctx = vp_env.ctx(&gas_meter); + let result = PosVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ); assert!( result.is_ok(), "PoS Validity predicate must accept this transaction" diff --git a/wasm/tx_change_validator_commission/src/lib.rs b/wasm/tx_change_validator_commission/src/lib.rs index 2218f5db61..bc30b89ebd 100644 --- a/wasm/tx_change_validator_commission/src/lib.rs +++ b/wasm/tx_change_validator_commission/src/lib.rs @@ -154,8 +154,13 @@ mod tests { &tx_env.gas_meter.borrow(), )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, address::POS); - let vp = vp_env.init_vp(&gas_meter, PosVp::new); - let result = vp_env.validate_tx(&vp); + let ctx = vp_env.ctx(&gas_meter); + let result = PosVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ); assert!( result.is_ok(), "PoS Validity predicate must accept this transaction" diff --git a/wasm/tx_redelegate/src/lib.rs b/wasm/tx_redelegate/src/lib.rs index 3ccbd7520e..66c03c1285 100644 --- a/wasm/tx_redelegate/src/lib.rs +++ b/wasm/tx_redelegate/src/lib.rs @@ -365,8 +365,13 @@ mod tests { &tx_env.gas_meter.borrow(), )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, address::POS); - let vp = vp_env.init_vp(&gas_meter, PosVp::new); - let result = vp_env.validate_tx(&vp); + let ctx = vp_env.ctx(&gas_meter); + let result = PosVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ); assert!( result.is_ok(), "PoS Validity predicate must accept this transaction" diff --git a/wasm/tx_unbond/src/lib.rs b/wasm/tx_unbond/src/lib.rs index 3188c16673..bc8078327b 100644 --- a/wasm/tx_unbond/src/lib.rs +++ b/wasm/tx_unbond/src/lib.rs @@ -341,8 +341,13 @@ mod tests { &tx_env.gas_meter.borrow(), )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, address::POS); - let vp = vp_env.init_vp(&gas_meter, PosVp::new); - let result = vp_env.validate_tx(&vp); + let ctx = vp_env.ctx(&gas_meter); + let result = PosVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ); assert!( result.is_ok(), "PoS Validity predicate must accept this transaction" diff --git a/wasm/tx_withdraw/src/lib.rs b/wasm/tx_withdraw/src/lib.rs index 7a0dc5591d..283f0eae9d 100644 --- a/wasm/tx_withdraw/src/lib.rs +++ b/wasm/tx_withdraw/src/lib.rs @@ -227,8 +227,13 @@ mod tests { &tx_env.gas_meter.borrow(), )); let vp_env = TestNativeVpEnv::from_tx_env(tx_env, address::POS); - let vp = vp_env.init_vp(&gas_meter, PosVp::new); - let result = vp_env.validate_tx(&vp); + let ctx = vp_env.ctx(&gas_meter); + let result = PosVp::validate_tx( + &ctx, + &vp_env.tx_env.batched_tx.to_ref(), + &vp_env.keys_changed, + &vp_env.verifiers, + ); assert!( result.is_ok(), "PoS Validity predicate must accept this transaction" diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index 8eae5e410f..6d2ab03883 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -2142,7 +2142,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "once_cell", "serde", "smooth-operator", From a75ad6621d134276cefc390de51a8ad79de87306 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 16:55:05 +0200 Subject: [PATCH 08/10] trans_token: env agnostic VP --- Cargo.lock | 1 + crates/benches/native_vps.rs | 18 +-- crates/node/src/protocol.rs | 15 +- crates/sdk/src/validation.rs | 12 +- crates/tests/src/vm_host_env/ibc.rs | 22 +-- crates/trans_token/Cargo.toml | 3 +- crates/trans_token/src/vp.rs | 222 +++++++++++++++------------- wasm/Cargo.lock | 2 +- wasm_for_tests/Cargo.lock | 2 +- 9 files changed, 150 insertions(+), 147 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e324392e6a..1634134d71 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5564,6 +5564,7 @@ dependencies = [ "namada_tx", "namada_vm", "namada_vp", + "namada_vp_env", "thiserror", "tracing", ] diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index 4afe4ca92b..ef0d768b78 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -513,7 +513,7 @@ fn vp_multitoken(c: &mut Criterion) { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let multitoken = MultitokenVp::new(Ctx::new( + let ctx = Ctx::new( &Address::Internal(InternalAddress::Multitoken), &shell.state, &signed_tx.tx, @@ -523,18 +523,18 @@ fn vp_multitoken(c: &mut Criterion) { &keys_changed, &verifiers, shell.vp_wasm_cache.clone(), - )); + ); group.bench_function(bench_name, |b| { b.iter(|| { assert!( - multitoken - .validate_tx( - &signed_tx.to_ref(), - multitoken.ctx.keys_changed, - multitoken.ctx.verifiers, - ) - .is_ok() + MultitokenVp::validate_tx( + &ctx, + &signed_tx.to_ref(), + ctx.keys_changed, + ctx.verifiers, + ) + .is_ok() ) }) }); diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 4dbc5f9b44..f4b9c4e050 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -1266,14 +1266,13 @@ where ) .map_err(Error::NativeVpError), InternalAddress::Multitoken => { - let multitoken = MultitokenVp::new(ctx); - multitoken - .validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) + MultitokenVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError) } InternalAddress::Masp => { let masp = MaspVp::new(ctx); diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index 4004eb9c77..cd141aac92 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -68,13 +68,11 @@ pub type GovernanceVp<'ctx, CTX> = governance::vp::GovernanceVp< pub type PgfVp<'ctx, CTX> = governance::vp::pgf::PgfVp<'ctx, CTX>; /// Native multitoken VP -pub type MultitokenVp<'a, S, CA> = token::vp::MultitokenVp< - 'a, - S, - VpCache, - Eval, - ParamsPreStore<'a, S, CA>, - GovPreStore<'a, S, CA>, +pub type MultitokenVp<'ctx, CTX> = token::vp::MultitokenVp< + 'ctx, + CTX, + parameters::Store<>::Pre>, + governance::Store<>::Pre>, >; /// Native MASP VP diff --git a/crates/tests/src/vm_host_env/ibc.rs b/crates/tests/src/vm_host_env/ibc.rs index b11526f0b7..48e5146b9b 100644 --- a/crates/tests/src/vm_host_env/ibc.rs +++ b/crates/tests/src/vm_host_env/ibc.rs @@ -77,6 +77,7 @@ use namada_sdk::{ibc, proof_of_stake, token}; use namada_test_utils::TestWasms; use namada_tx_env::TxEnv; use namada_tx_prelude::BorshSerializeExt; +use namada_vm::wasm::run::VpEvalWasm; use namada_vm::{wasm, WasmCacheRwAccess}; use namada_vp::native_vp; use namada_vp::native_vp::{Ctx, NativeVp}; @@ -100,20 +101,6 @@ impl<'a> TestIbcVp<'a> { } } -pub struct TestMultitokenVp<'a> { - pub multitoken_vp: MultitokenVp<'a, TestState, WasmCacheRwAccess>, -} - -impl<'a> TestMultitokenVp<'a> { - pub fn validate(&self, batched_tx: &BatchedTxRef) -> native_vp::Result<()> { - self.multitoken_vp.validate_tx( - batched_tx, - self.multitoken_vp.ctx.keys_changed, - self.multitoken_vp.ctx.verifiers, - ) - } -} - /// Validate an IBC transaction with IBC VP. pub fn validate_ibc_vp_from_tx<'a>( tx_env: &'a TestTxEnv, @@ -170,12 +157,12 @@ pub fn validate_multitoken_vp_from_tx<'a>( ); } let (vp_wasm_cache, _vp_cache_dir) = - wasm::compilation_cache::common::testing::cache(); + wasm::compilation_cache::common::testing::vp_cache(); let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(10_000_000_000), )); - let ctx = Ctx::new( + let ctx = Ctx::<_, _, VpEvalWasm<_, _, _>>::new( &ADDRESS, &tx_env.state, batched_tx.tx, @@ -186,9 +173,8 @@ pub fn validate_multitoken_vp_from_tx<'a>( &verifiers, vp_wasm_cache, ); - let multitoken_vp = MultitokenVp::new(ctx); - TestMultitokenVp { multitoken_vp }.validate(batched_tx) + MultitokenVp::validate_tx(&ctx, batched_tx, ctx.keys_changed, ctx.verifiers) } /// Initialize the test storage. Requires initialized [`tx_host_env::ENV`]. diff --git a/crates/trans_token/Cargo.toml b/crates/trans_token/Cargo.toml index c1b9b76530..a016cef0d6 100644 --- a/crates/trans_token/Cargo.toml +++ b/crates/trans_token/Cargo.toml @@ -24,7 +24,7 @@ namada_events = { path = "../events", default-features = false } namada_state = { path = "../state" } namada_systems = { path = "../systems" } namada_tx = { path = "../tx" } -namada_vp = { path = "../vp" } +namada_vp_env = { path = "../vp_env" } konst.workspace = true linkme = {workspace = true, optional = true} @@ -40,5 +40,6 @@ namada_parameters = { path = "../parameters", features = ["testing"] } namada_state = { path = "../state", features = ["testing"] } namada_tx = { path = "../tx", features = ["testing"] } namada_vm = { path = "../vm", features = ["testing"] } +namada_vp = { path = "../vp" } assert_matches.workspace = true diff --git a/crates/trans_token/src/vp.rs b/crates/trans_token/src/vp.rs index 63baaa8b74..471ea24ae3 100644 --- a/crates/trans_token/src/vp.rs +++ b/crates/trans_token/src/vp.rs @@ -8,16 +8,12 @@ use namada_core::booleans::BoolResultUnitExt; use namada_core::collections::HashMap; use namada_core::storage::{Key, KeySeg}; use namada_core::token::Amount; -use namada_state::StateRead; use namada_systems::{governance, parameters}; use namada_tx::action::{ - Action, Bond, ClaimRewards, GovAction, PosAction, Read, Withdraw, + Action, Bond, ClaimRewards, GovAction, PosAction, Withdraw, }; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{ - Ctx, CtxPreStorageRead, Error, NativeVp, Result, VpEvaluator, -}; -use namada_vp::VpEnv; +use namada_vp_env::{Error, Result, VpEnv}; use crate::storage_key::{ is_any_minted_balance_key, is_any_minter_key, is_any_token_balance_key, @@ -33,43 +29,36 @@ enum Owner<'a> { } /// Multitoken VP -pub struct MultitokenVp<'ctx, S, CA, EVAL, Params, Gov> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct MultitokenVp<'ctx, CTX, Params, Gov> { /// Generic types for DI - pub _marker: PhantomData<(Params, Gov)>, + pub _marker: PhantomData<(&'ctx CTX, Params, Gov)>, } -impl<'view, 'ctx: 'view, S, CA, EVAL, Params, Gov> NativeVp<'view> - for MultitokenVp<'ctx, S, CA, EVAL, Params, Gov> +impl<'ctx, CTX, Params, Gov> MultitokenVp<'ctx, CTX, Params, Gov> where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Params: parameters::Read>, - Gov: governance::Read>, + CTX: VpEnv<'ctx> + namada_tx::action::Read, + Params: parameters::Read<>::Pre>, + Gov: governance::Read<>::Pre>, { - fn validate_tx( - &'view self, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, tx_data: &BatchedTxRef<'_>, keys_changed: &BTreeSet, verifiers: &BTreeSet
, ) -> Result<()> { // Is VP triggered by a governance proposal? if Gov::is_proposal_accepted( - &self.ctx.pre(), + &ctx.pre(), tx_data.tx.data(tx_data.cmt).unwrap_or_default().as_ref(), )? { return Ok(()); } - let native_token = self.ctx.pre().get_native_token()?; + let native_token = ctx.pre().get_native_token()?; let is_native_token_transferable = - Params::is_native_token_transferable(&self.ctx.pre())?; - let actions = self.ctx.read_actions()?; + Params::is_native_token_transferable(&ctx.pre())?; + let actions = ctx.read_actions()?; // The native token can be transferred to and out of the `PoS` and `Gov` // accounts, even if `is_native_token_transferable` is false let is_allowed_inc = |token: &Address, bal_owner: &Address| -> bool { @@ -109,8 +98,8 @@ where let mut dec_mints: HashMap = HashMap::new(); for key in keys_changed { if let Some([token, owner]) = is_any_token_balance_key(key) { - let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); - let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); + let pre: Amount = ctx.read_pre(key)?.unwrap_or_default(); + let post: Amount = ctx.read_post(key)?.unwrap_or_default(); match post.checked_sub(pre) { Some(diff) => { if !is_allowed_inc(token, owner) { @@ -158,8 +147,8 @@ where )); } - let pre: Amount = self.ctx.read_pre(key)?.unwrap_or_default(); - let post: Amount = self.ctx.read_post(key)?.unwrap_or_default(); + let pre: Amount = ctx.read_pre(key)?.unwrap_or_default(); + let post: Amount = ctx.read_post(key)?.unwrap_or_default(); match post.checked_sub(pre) { Some(diff) => { let mint = inc_mints.entry(token.clone()).or_default(); @@ -178,11 +167,11 @@ where } } // Check if the minter is set - self.is_valid_minter(token, verifiers)?; + Self::is_valid_minter(ctx, token, verifiers)?; } else if let Some(token) = is_any_minter_key(key) { - self.is_valid_minter(token, verifiers)?; + Self::is_valid_minter(ctx, token, verifiers)?; } else if is_any_token_parameter_key(key).is_some() { - return self.is_valid_parameter(tx_data); + return Self::is_valid_parameter(ctx, tx_data); } else if key.segments.first() == Some( &Address::Internal(InternalAddress::Multitoken).to_db_key(), @@ -243,28 +232,10 @@ where }) }) } -} - -impl<'view, 'ctx: 'view, S, CA, EVAL, Params, Gov> - MultitokenVp<'ctx, S, CA, EVAL, Params, Gov> -where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Params: parameters::Read>, - Gov: governance::Read>, -{ - /// Instantiate token VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, - } - } /// Return the minter if the minter is valid and the minter VP exists pub fn is_valid_minter( - &self, + ctx: &'ctx CTX, token: &Address, verifiers: &BTreeSet
, ) -> Result<()> { @@ -272,7 +243,7 @@ where Address::Internal(InternalAddress::IbcToken(_)) => { // Check if the minter is set let minter_key = minter_key(token); - match self.ctx.read_post::
(&minter_key)? { + match ctx.read_post::
(&minter_key)? { Some(minter) if minter == Address::Internal(InternalAddress::Ibc) => @@ -294,7 +265,7 @@ where /// Return if the parameter change was done via a governance proposal pub fn is_valid_parameter( - &'view self, + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, ) -> Result<()> { batched_tx.tx.data(batched_tx.cmt).map_or_else( @@ -304,7 +275,7 @@ where )) }, |data| { - Gov::is_proposal_accepted(&self.ctx.pre(), data.as_ref())? + Gov::is_proposal_accepted(&ctx.pre(), data.as_ref())? .ok_or_else(|| { Error::new_const( "Token parameter changes can only be performed by \ @@ -380,7 +351,7 @@ mod tests { use namada_ibc::trace::ibc_token; use namada_parameters::storage::get_native_token_transferable_key; use namada_state::testing::TestState; - use namada_state::{StorageWrite, TxIndex}; + use namada_state::{StateRead, StorageWrite, TxIndex}; use namada_tx::action::Write; use namada_tx::data::TxType; use namada_tx::{Authorization, BatchedTx, Code, Data, Section, Tx}; @@ -388,6 +359,7 @@ mod tests { use namada_vm::wasm::run::VpEvalWasm; use namada_vm::wasm::VpCache; use namada_vm::WasmCacheRwAccess; + use namada_vp::native_vp::{self, CtxPreStorageRead}; use super::*; use crate::storage_key::{balance_key, minted_balance_key}; @@ -395,22 +367,16 @@ mod tests { const ADDRESS: Address = Address::Internal(InternalAddress::Multitoken); type CA = WasmCacheRwAccess; - type Eval = VpEvalWasm< - ::D, - ::H, - CA, - >; - type Ctx<'ctx> = super::Ctx<'ctx, TestState, VpCache, Eval>; - type MultitokenVp<'ctx> = super::MultitokenVp< + type Eval = VpEvalWasm<::D, ::H, CA>; + type Ctx<'ctx, S> = native_vp::Ctx<'ctx, S, VpCache, Eval>; + type MultitokenVp<'ctx, S> = super::MultitokenVp< 'ctx, - TestState, - VpCache, - Eval, + Ctx<'ctx, S>, namada_parameters::Store< - CtxPreStorageRead<'ctx, 'ctx, TestState, VpCache, Eval>, + CtxPreStorageRead<'ctx, 'ctx, S, VpCache, Eval>, >, namada_governance::Store< - CtxPreStorageRead<'ctx, 'ctx, TestState, VpCache, Eval>, + CtxPreStorageRead<'ctx, 'ctx, S, VpCache, Eval>, >, >; @@ -494,10 +460,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_ok() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_ok() ); } @@ -535,10 +505,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_err() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() ); } @@ -599,10 +573,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_ok() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_ok() ); } @@ -659,10 +637,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_err() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() ); } @@ -712,10 +694,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_err() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() ); } @@ -774,10 +760,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_err() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() ); } @@ -816,10 +806,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_err() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() ); } @@ -859,10 +853,14 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers) - .is_err() + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ) + .is_err() ); } @@ -897,9 +895,13 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert_matches!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers), + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ), Err(_) ); } @@ -942,9 +944,13 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert_matches!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers), + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ), Ok(_) ); } @@ -985,9 +991,13 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert_matches!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers), + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ), Err(_) ); } @@ -1028,9 +1038,13 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert_matches!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers), + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ), Ok(_) ); } @@ -1076,9 +1090,13 @@ mod tests { vp_vp_cache, ); - let vp = MultitokenVp::new(ctx); assert_matches!( - vp.validate_tx(&tx.batch_ref_tx(&cmt), &keys_changed, &verifiers), + MultitokenVp::validate_tx( + &ctx, + &tx.batch_ref_tx(&cmt), + &keys_changed, + &verifiers + ), Ok(_) ); } diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index ab53eefae6..334a040a5b 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -4114,7 +4114,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "thiserror", "tracing", ] diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index 6d2ab03883..b731d19718 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -2277,7 +2277,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "thiserror", "tracing", ] From 4b472ba75cda70ea8f9b427ee63de33b18651998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Thu, 12 Sep 2024 17:30:19 +0200 Subject: [PATCH 09/10] shielded_token: env agnostic VP --- Cargo.lock | 2 +- crates/benches/native_vps.rs | 11 +- crates/node/src/protocol.rs | 33 ++-- crates/sdk/src/validation.rs | 16 +- crates/shielded_token/Cargo.toml | 2 +- crates/shielded_token/src/vp.rs | 228 +++++++++++-------------- crates/state/src/lib.rs | 2 +- crates/state/src/wl_state.rs | 32 +++- crates/storage/src/conversion_state.rs | 7 +- crates/storage/src/lib.rs | 5 +- crates/vp/src/native_vp.rs | 12 ++ wasm/Cargo.lock | 2 +- wasm_for_tests/Cargo.lock | 2 +- 13 files changed, 185 insertions(+), 169 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1634134d71..43626e40f8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5384,7 +5384,7 @@ dependencies = [ "namada_systems", "namada_trans_token", "namada_tx", - "namada_vp", + "namada_vp_env", "namada_wallet", "proptest", "rand 0.8.5", diff --git a/crates/benches/native_vps.rs b/crates/benches/native_vps.rs index ef0d768b78..f2350ca722 100644 --- a/crates/benches/native_vps.rs +++ b/crates/benches/native_vps.rs @@ -634,7 +634,7 @@ fn masp(c: &mut Criterion) { let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( &TxGasMeter::new(u64::MAX), )); - let masp = MaspVp::new(Ctx::new( + let ctx = Ctx::new( &Address::Internal(InternalAddress::Masp), &shell_read.state, &signed_tx.tx, @@ -644,14 +644,15 @@ fn masp(c: &mut Criterion) { &keys_changed, &verifiers, shell_read.vp_wasm_cache.clone(), - )); + ); b.iter(|| { assert!( - masp.validate_tx( + MaspVp::validate_tx( + &ctx, &signed_tx.to_ref(), - masp.ctx.keys_changed, - masp.ctx.verifiers, + ctx.keys_changed, + ctx.verifiers, ) .is_ok() ); diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index f4b9c4e050..12c999d311 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -38,6 +38,7 @@ use namada_vm::wasm::{TxCache, VpCache}; use namada_vm::{self, wasm, WasmCacheAccess}; use namada_vote_ext::EthereumTxData; use namada_vp::native_vp::NativeVp; +use namada_vp::state::ReadConversionState; use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; use smooth_operator::checked; use thiserror::Error; @@ -340,7 +341,11 @@ pub(crate) fn dispatch_inner_txs<'a, S, D, H, CA>( tx_wasm_cache: &'a mut TxCache, ) -> std::result::Result, DispatchError> where - S: 'static + State + Read + Sync, + S: 'static + + State + + Read + + ReadConversionState + + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, CA: 'static + WasmCacheAccess + Sync, @@ -457,6 +462,7 @@ where + State + Read + TxWrites + + ReadConversionState + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, @@ -531,6 +537,7 @@ where + StorageRead + TxWrites + Read + + ReadConversionState + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, @@ -693,6 +700,7 @@ where + State + StorageRead + Read + + ReadConversionState + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, @@ -870,6 +878,7 @@ where + State + StorageRead + Read + + ReadConversionState + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, @@ -940,7 +949,7 @@ fn apply_wasm_tx( shell_params: ShellParams<'_, S, D, H, CA>, ) -> Result where - S: 'static + State + Sync, + S: 'static + State + ReadConversionState + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, CA: 'static + WasmCacheAccess + Sync, @@ -1124,7 +1133,7 @@ fn check_vps( }: CheckVps<'_, S, CA>, ) -> Result where - S: 'static + State + Sync, + S: 'static + ReadConversionState + State + Sync, CA: 'static + WasmCacheAccess + Sync, { let (verifiers, keys_changed) = state @@ -1161,7 +1170,7 @@ fn execute_vps( vp_wasm_cache: &mut VpCache, ) -> Result<(VpsResult, namada_sdk::gas::Gas)> where - S: 'static + State + Sync, + S: 'static + ReadConversionState + State + Sync, CA: 'static + WasmCacheAccess + Sync, { let vps_result = verifiers @@ -1274,15 +1283,13 @@ where ) .map_err(Error::NativeVpError) } - InternalAddress::Masp => { - let masp = MaspVp::new(ctx); - masp.validate_tx( - batched_tx, - &keys_changed, - &verifiers, - ) - .map_err(Error::NativeVpError) - } + InternalAddress::Masp => MaspVp::validate_tx( + &ctx, + batched_tx, + &keys_changed, + &verifiers, + ) + .map_err(Error::NativeVpError), InternalAddress::EthBridge => { EthBridgeVp::validate_tx( &ctx, diff --git a/crates/sdk/src/validation.rs b/crates/sdk/src/validation.rs index cd141aac92..f2ed9307b7 100644 --- a/crates/sdk/src/validation.rs +++ b/crates/sdk/src/validation.rs @@ -76,15 +76,13 @@ pub type MultitokenVp<'ctx, CTX> = token::vp::MultitokenVp< >; /// Native MASP VP -pub type MaspVp<'a, S, CA> = token::vp::MaspVp< - 'a, - S, - VpCache, - Eval, - ParamsPreStore<'a, S, CA>, - GovPreStore<'a, S, CA>, - IbcPostStore<'a, S, CA>, - TokenPreStore<'a, S, CA>, +pub type MaspVp<'ctx, CTX> = token::vp::MaspVp< + 'ctx, + CTX, + parameters::Store<>::Pre>, + governance::Store<>::Pre>, + ibc::Store<>::Post>, + token::Store<>::Pre>, token::Transfer, >; diff --git a/crates/shielded_token/Cargo.toml b/crates/shielded_token/Cargo.toml index 5b62b4a034..4243999b25 100644 --- a/crates/shielded_token/Cargo.toml +++ b/crates/shielded_token/Cargo.toml @@ -47,7 +47,7 @@ namada_migrations = { path = "../migrations", optional = true } namada_state = { path = "../state" } namada_systems = { path = "../systems" } namada_tx = { path = "../tx" } -namada_vp = { path = "../vp" } +namada_vp_env = { path = "../vp_env" } namada_wallet = { path = "../wallet", optional = true } async-trait.workspace = true diff --git a/crates/shielded_token/src/vp.rs b/crates/shielded_token/src/vp.rs index 1adef13aba..1851cd6fcb 100644 --- a/crates/shielded_token/src/vp.rs +++ b/crates/shielded_token/src/vp.rs @@ -22,15 +22,12 @@ use namada_core::storage::Key; use namada_core::token; use namada_core::token::{Amount, MaspDigitPos}; use namada_core::uint::I320; -use namada_state::{ConversionState, OptionExt, ResultExt, StateRead}; +use namada_state::{ + ConversionState, OptionExt, ReadConversionState, ResultExt, +}; use namada_systems::{governance, ibc, parameters, trans_token}; -use namada_tx::action::Read; use namada_tx::BatchedTxRef; -use namada_vp::native_vp::{ - Ctx, CtxPostStorageRead, CtxPreStorageRead, Error, NativeVp, Result, - VpEvaluator, -}; -use namada_vp::VpEnv; +use namada_vp_env::{Error, Result, VpEnv}; use crate::storage_key::{ is_masp_key, is_masp_nullifier_key, is_masp_token_map_key, @@ -40,14 +37,10 @@ use crate::storage_key::{ use crate::validation::verify_shielded_tx; /// MASP VP -pub struct MaspVp<'ctx, S, CA, EVAL, Params, Gov, Ibc, TransToken, Transfer> -where - S: 'static + StateRead, -{ - /// Context to interact with the host structures. - pub ctx: Ctx<'ctx, S, CA, EVAL>, +pub struct MaspVp<'ctx, CTX, Params, Gov, Ibc, TransToken, Transfer> { /// Generic types for DI - pub _marker: PhantomData<(Params, Gov, Ibc, TransToken, Transfer)>, + pub _marker: + PhantomData<(&'ctx CTX, Params, Gov, Ibc, TransToken, Transfer)>, } // Balances changed by a transaction @@ -63,30 +56,65 @@ struct ChangedBalances { post: BTreeMap>, } -impl<'view, 'ctx: 'view, S, CA, EVAL, Params, Gov, Ibc, TransToken, Transfer> - MaspVp<'ctx, S, CA, EVAL, Params, Gov, Ibc, TransToken, Transfer> +impl<'view, 'ctx: 'view, CTX, Params, Gov, Ibc, TransToken, Transfer> + MaspVp<'ctx, CTX, Params, Gov, Ibc, TransToken, Transfer> where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Params: parameters::Read>, - Gov: governance::Read>, - Ibc: ibc::Read>, - TransToken: trans_token::Keys - + trans_token::Read>, + CTX: VpEnv<'ctx> + + namada_tx::action::Read + + ReadConversionState, + Params: parameters::Read<>::Pre>, + Gov: governance::Read<>::Pre>, + Ibc: ibc::Read<>::Post>, + TransToken: + trans_token::Keys + trans_token::Read<>::Pre>, Transfer: BorshDeserialize, { - /// Instantiate MASP VP - pub fn new(ctx: Ctx<'ctx, S, CA, EVAL>) -> Self { - Self { - ctx, - _marker: PhantomData, + /// Run the validity predicate + pub fn validate_tx( + ctx: &'ctx CTX, + tx_data: &BatchedTxRef<'_>, + keys_changed: &BTreeSet, + verifiers: &BTreeSet
, + ) -> Result<()> { + let masp_keys_changed: Vec<&Key> = + keys_changed.iter().filter(|key| is_masp_key(key)).collect(); + let non_allowed_changes = masp_keys_changed.iter().any(|key| { + !is_masp_transfer_key(key) && !is_masp_token_map_key(key) + }); + + // Check that the transaction didn't write unallowed masp keys + if non_allowed_changes { + return Err(Error::new_const( + "Found modifications to non-allowed masp keys", + )); + } + let masp_token_map_changed = masp_keys_changed + .iter() + .any(|key| is_masp_token_map_key(key)); + let masp_transfer_changes = masp_keys_changed + .iter() + .any(|key| is_masp_transfer_key(key)); + if masp_token_map_changed && masp_transfer_changes { + Err(Error::new_const( + "Cannot simultaneously do governance proposal and MASP \ + transfer", + )) + } else if masp_token_map_changed { + // The token map can only be changed by a successful governance + // proposal + Self::is_valid_parameter_change(ctx, tx_data) + } else if masp_transfer_changes { + // The MASP transfer keys can only be changed by a valid Transaction + Self::is_valid_masp_transfer(ctx, tx_data, keys_changed, verifiers) + } else { + // Changing no MASP keys at all is also fine + Ok(()) } } /// Return if the parameter change was done via a governance proposal pub fn is_valid_parameter_change( - &'view self, + ctx: &'ctx CTX, tx: &BatchedTxRef<'_>, ) -> Result<()> { tx.tx.data(tx.cmt).map_or_else( @@ -96,7 +124,7 @@ where )) }, |data| { - Gov::is_proposal_accepted(&self.ctx.pre(), data.as_ref())? + Gov::is_proposal_accepted(&ctx.pre(), data.as_ref())? .ok_or_else(|| { Error::new_const( "MASP parameter changes can only be performed by \ @@ -109,7 +137,7 @@ where // Check that the transaction correctly revealed the nullifiers, if needed fn valid_nullifiers_reveal( - &self, + ctx: &'ctx CTX, keys_changed: &BTreeSet, transaction: &Transaction, ) -> Result<()> { @@ -122,7 +150,7 @@ where .map_or(&vec![], |bundle| &bundle.shielded_spends) { let nullifier_key = masp_nullifier_key(&description.nullifier); - if self.ctx.has_key_pre(&nullifier_key)? + if ctx.has_key_pre(&nullifier_key)? || revealed_nullifiers.contains(&nullifier_key) { let error = Error::new_alloc(format!( @@ -138,8 +166,7 @@ where // and no delete) and carries no associated data (the latter not // strictly necessary for validation, but we don't expect any // value for this key anyway) - self.ctx - .read_bytes_post(&nullifier_key)? + ctx.read_bytes_post(&nullifier_key)? .is_some_and(|value| value.is_empty()) .ok_or_else(|| { Error::new_const( @@ -171,18 +198,16 @@ where // Check that a transaction carrying output descriptions correctly updates // the tree and anchor in storage fn valid_note_commitment_update( - &self, + ctx: &'ctx CTX, transaction: &Transaction, ) -> Result<()> { // Check that the merkle tree in storage has been correctly updated with // the output descriptions cmu let tree_key = masp_commitment_tree_key(); - let mut previous_tree: CommitmentTree = self - .ctx + let mut previous_tree: CommitmentTree = ctx .read_pre(&tree_key)? .ok_or(Error::new_const("Cannot read storage"))?; - let post_tree: CommitmentTree = self - .ctx + let post_tree: CommitmentTree = ctx .read_post(&tree_key)? .ok_or(Error::new_const("Cannot read storage"))?; @@ -214,7 +239,7 @@ where // Check that the spend descriptions anchors of a transaction are valid fn valid_spend_descriptions_anchor( - &self, + ctx: &'ctx CTX, transaction: &Transaction, ) -> Result<()> { for description in transaction @@ -224,7 +249,7 @@ where let anchor_key = masp_commitment_anchor_key(description.anchor); // Check if the provided anchor was published before - if !self.ctx.has_key_pre(&anchor_key)? { + if !ctx.has_key_pre(&anchor_key)? { let error = Error::new_const( "Spend description refers to an invalid anchor", ); @@ -238,14 +263,13 @@ where // Check that the convert descriptions anchors of a transaction are valid fn valid_convert_descriptions_anchor( - &'view self, + ctx: &'ctx CTX, transaction: &Transaction, ) -> Result<()> { if let Some(bundle) = transaction.sapling_bundle() { if !bundle.shielded_converts.is_empty() { let anchor_key = masp_convert_anchor_key(); - let expected_anchor = self - .ctx + let expected_anchor = ctx .read_pre::(&anchor_key)? .ok_or(Error::new_const("Cannot read storage"))?; @@ -270,26 +294,21 @@ where // Apply the balance change to the changed balances structure fn apply_balance_change( - &'view self, + ctx: &'ctx CTX, mut result: ChangedBalances, [token, counterpart]: [&Address; 2], ) -> Result { - let denom = TransToken::read_denom(&self.ctx.pre(), token)? - .ok_or_err_msg( - "No denomination found in storage for the given token", - )?; + let denom = TransToken::read_denom(&ctx.pre(), token)?.ok_or_err_msg( + "No denomination found in storage for the given token", + )?; // Record the token without an epoch to facilitate later decoding unepoched_tokens(token, denom, &mut result.tokens)?; let counterpart_balance_key = TransToken::balance_key(token, counterpart); - let pre_balance: Amount = self - .ctx - .read_pre(&counterpart_balance_key)? - .unwrap_or_default(); - let post_balance: Amount = self - .ctx - .read_post(&counterpart_balance_key)? - .unwrap_or_default(); + let pre_balance: Amount = + ctx.read_pre(&counterpart_balance_key)?.unwrap_or_default(); + let post_balance: Amount = + ctx.read_post(&counterpart_balance_key)?.unwrap_or_default(); // Public keys must be the hash of the sources/targets let addr_hash = addr_taddr(counterpart.clone()); // Enable the decoding of these counterpart addresses @@ -322,7 +341,7 @@ where // Check that transfer is pinned correctly and record the balance changes fn validate_state_and_get_transfer_data( - &'view self, + ctx: &'ctx CTX, keys_changed: &BTreeSet, tx_data: &[u8], ) -> Result { @@ -334,7 +353,7 @@ where // Apply the balance changes to the changed balances structure let mut changed_balances = counterparts_balances .try_fold(ChangedBalances::default(), |acc, account| { - self.apply_balance_change(acc, account) + Self::apply_balance_change(ctx, acc, account) })?; let ibc_addr = TAddrData::Addr(address::IBC); @@ -352,7 +371,7 @@ where } = changed_balances; let ibc::ChangedBalances { decoder, pre, post } = Ibc::apply_ibc_packet::( - &self.ctx.post(), + &ctx.post(), tx_data, ibc::ChangedBalances { decoder, pre, post }, keys_changed, @@ -367,24 +386,23 @@ where // Check that MASP Transaction and state changes are valid fn is_valid_masp_transfer( - &'view self, + ctx: &'ctx CTX, batched_tx: &BatchedTxRef<'_>, keys_changed: &BTreeSet, verifiers: &BTreeSet
, ) -> Result<()> { - let masp_epoch_multiplier = - Params::masp_epoch_multiplier(&self.ctx.pre())?; + let masp_epoch_multiplier = Params::masp_epoch_multiplier(&ctx.pre())?; let masp_epoch = MaspEpoch::try_from_epoch( - self.ctx.get_block_epoch()?, + ctx.get_block_epoch()?, masp_epoch_multiplier, ) .map_err(Error::new_const)?; - let conversion_state = self.ctx.state.in_mem().get_conversion_state(); + let conversion_state = ctx.conversion_state(); let tx_data = batched_tx .tx .data(batched_tx.cmt) .ok_or_err_msg("No transaction data")?; - let actions = self.ctx.read_actions()?; + let actions = ctx.read_actions()?; // Try to get the Transaction object from the tx first (IBC) and from // the actions afterwards let shielded_tx = if let Some(tx) = @@ -410,7 +428,7 @@ where })? }; - if u64::from(self.ctx.get_block_height()?) + if u64::from(ctx.get_block_height()?) > u64::from(shielded_tx.expiry_height()) { let error = Error::new_const("MASP transaction is expired"); @@ -419,8 +437,11 @@ where } // Check the validity of the keys and get the transfer data - let changed_balances = - self.validate_state_and_get_transfer_data(keys_changed, &tx_data)?; + let changed_balances = Self::validate_state_and_get_transfer_data( + ctx, + keys_changed, + &tx_data, + )?; // Some constants that will be used repeatedly let zero = ValueSum::zero(); @@ -451,10 +472,10 @@ where // nullifier is being revealed by the tx // 4. The transaction must correctly update the note commitment tree // in storage with the new output descriptions - self.valid_spend_descriptions_anchor(&shielded_tx)?; - self.valid_convert_descriptions_anchor(&shielded_tx)?; - self.valid_nullifiers_reveal(keys_changed, &shielded_tx)?; - self.valid_note_commitment_update(&shielded_tx)?; + Self::valid_spend_descriptions_anchor(ctx, &shielded_tx)?; + Self::valid_convert_descriptions_anchor(ctx, &shielded_tx)?; + Self::valid_nullifiers_reveal(ctx, keys_changed, &shielded_tx)?; + Self::valid_note_commitment_update(ctx, &shielded_tx)?; // Checks on the transparent bundle, if present let mut changed_bals_minus_txn = changed_balances.clone(); @@ -580,7 +601,7 @@ where } // Verify the proofs - verify_shielded_tx(&shielded_tx, |gas| self.ctx.charge_gas(gas)) + verify_shielded_tx(&shielded_tx, |gas| ctx.charge_gas(gas)) } } @@ -857,60 +878,3 @@ fn verify_sapling_balancing_value( Err(error) } } - -impl<'view, 'ctx: 'view, S, CA, EVAL, Params, Gov, Ibc, TransToken, Transfer> - NativeVp<'view> - for MaspVp<'ctx, S, CA, EVAL, Params, Gov, Ibc, TransToken, Transfer> -where - S: 'static + StateRead, - CA: 'static + Clone, - EVAL: 'static + VpEvaluator<'ctx, S, CA, EVAL>, - Params: parameters::Read>, - Gov: governance::Read>, - Ibc: ibc::Read>, - TransToken: trans_token::Keys - + trans_token::Read>, - Transfer: BorshDeserialize, -{ - fn validate_tx( - &'view self, - tx_data: &BatchedTxRef<'_>, - keys_changed: &BTreeSet, - verifiers: &BTreeSet
, - ) -> Result<()> { - let masp_keys_changed: Vec<&Key> = - keys_changed.iter().filter(|key| is_masp_key(key)).collect(); - let non_allowed_changes = masp_keys_changed.iter().any(|key| { - !is_masp_transfer_key(key) && !is_masp_token_map_key(key) - }); - - // Check that the transaction didn't write unallowed masp keys - if non_allowed_changes { - return Err(Error::new_const( - "Found modifications to non-allowed masp keys", - )); - } - let masp_token_map_changed = masp_keys_changed - .iter() - .any(|key| is_masp_token_map_key(key)); - let masp_transfer_changes = masp_keys_changed - .iter() - .any(|key| is_masp_transfer_key(key)); - if masp_token_map_changed && masp_transfer_changes { - Err(Error::new_const( - "Cannot simultaneously do governance proposal and MASP \ - transfer", - )) - } else if masp_token_map_changed { - // The token map can only be changed by a successful governance - // proposal - self.is_valid_parameter_change(tx_data) - } else if masp_transfer_changes { - // The MASP transfer keys can only be changed by a valid Transaction - self.is_valid_masp_transfer(tx_data, keys_changed, verifiers) - } else { - // Changing no MASP keys at all is also fine - Ok(()) - } - } -} diff --git a/crates/state/src/lib.rs b/crates/state/src/lib.rs index 9fb94b2e8f..53022ecda3 100644 --- a/crates/state/src/lib.rs +++ b/crates/state/src/lib.rs @@ -54,7 +54,7 @@ pub use namada_merkle_tree::{ }; pub use namada_storage as storage; pub use namada_storage::conversion_state::{ - ConversionLeaf, ConversionState, WithConversionState, + ConversionLeaf, ConversionState, ReadConversionState, WithConversionState, }; pub use namada_storage::types::{KVBytes, PatternIterator, PrefixIterator}; pub use namada_storage::{ diff --git a/crates/state/src/wl_state.rs b/crates/state/src/wl_state.rs index 0871bcf1fc..b4e3082d4e 100644 --- a/crates/state/src/wl_state.rs +++ b/crates/state/src/wl_state.rs @@ -13,7 +13,9 @@ use namada_events::{EmitEvents, EventToEmit}; use namada_gas::Gas; use namada_merkle_tree::NO_DIFF_KEY_PREFIX; use namada_replay_protection as replay_protection; -use namada_storage::conversion_state::{ConversionState, WithConversionState}; +use namada_storage::conversion_state::{ + ConversionState, ReadConversionState, WithConversionState, +}; use namada_storage::{ BlockHeight, BlockStateRead, BlockStateWrite, ResultExt, StorageRead, }; @@ -53,6 +55,16 @@ where pub diff_key_filter: fn(&storage::Key) -> bool, } +impl ReadConversionState for WlState +where + D: DB + for<'iter> DBIter<'iter>, + H: StorageHasher, +{ + fn conversion_state(&self) -> &ConversionState { + self.in_mem.get_conversion_state() + } +} + /// State with a temporary write log. This is used for dry-running txs and ABCI /// prepare and processs proposal, which must not modify the actual state. #[derive(Debug)] @@ -85,6 +97,16 @@ where pub(crate) in_mem: &'a InMemory, } +impl ReadConversionState for TempWlState<'_, D, H> +where + D: DB + for<'iter> DBIter<'iter>, + H: StorageHasher, +{ + fn conversion_state(&self) -> &ConversionState { + self.in_mem.get_conversion_state() + } +} + impl FullAccessState where D: 'static + DB + for<'iter> DBIter<'iter>, @@ -1228,7 +1250,7 @@ where } } -impl WithConversionState for FullAccessState +impl ReadConversionState for FullAccessState where D: 'static + DB + for<'iter> DBIter<'iter>, H: 'static + StorageHasher, @@ -1236,7 +1258,13 @@ where fn conversion_state(&self) -> &ConversionState { &self.in_mem().conversion_state } +} +impl WithConversionState for FullAccessState +where + D: 'static + DB + for<'iter> DBIter<'iter>, + H: 'static + StorageHasher, +{ fn conversion_state_mut(&mut self) -> &mut ConversionState { &mut self.in_mem_mut().conversion_state } diff --git a/crates/storage/src/conversion_state.rs b/crates/storage/src/conversion_state.rs index 3f86dde72e..9d9a0edabf 100644 --- a/crates/storage/src/conversion_state.rs +++ b/crates/storage/src/conversion_state.rs @@ -44,11 +44,14 @@ pub struct ConversionState { pub assets: BTreeMap, } -/// Able to borrow mutable conversion state. -pub trait WithConversionState { +/// Able to borrow conversion state. +pub trait ReadConversionState { /// Borrow immutable conversion state fn conversion_state(&self) -> &ConversionState; +} +/// Able to borrow mutable conversion state. +pub trait WithConversionState: ReadConversionState { /// Borrow mutable conversion state fn conversion_state_mut(&mut self) -> &mut ConversionState; } diff --git a/crates/storage/src/lib.rs b/crates/storage/src/lib.rs index 7c923f9255..ea1c0f948f 100644 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -304,6 +304,7 @@ where /// Helpers for testing components that depend on storage #[cfg(any(test, feature = "testing"))] pub mod testing { + use conversion_state::ReadConversionState; use namada_core::address; use namada_core::chain::ChainId; use namada_core::collections::HashMap; @@ -440,11 +441,13 @@ pub mod testing { } } - impl WithConversionState for TestStorage { + impl ReadConversionState for TestStorage { fn conversion_state(&self) -> &ConversionState { &self.conversion_state } + } + impl WithConversionState for TestStorage { fn conversion_state_mut(&mut self) -> &mut ConversionState { &mut self.conversion_state } diff --git a/crates/vp/src/native_vp.rs b/crates/vp/src/native_vp.rs index d3220e258c..56f3486124 100644 --- a/crates/vp/src/native_vp.rs +++ b/crates/vp/src/native_vp.rs @@ -11,6 +11,7 @@ use namada_core::borsh; use namada_core::borsh::BorshDeserialize; use namada_core::chain::{ChainId, Epochs}; use namada_gas::{Gas, GasMetering, VpGasMeter}; +use namada_state::{ConversionState, ReadConversionState}; use namada_tx::{BatchedTxRef, Tx, TxCommitments}; use super::vp_host_fns; @@ -536,6 +537,17 @@ where } } +impl<'a, S, CA, EVAL> ReadConversionState for Ctx<'a, S, CA, EVAL> +where + S: StateRead + ReadConversionState, + EVAL: 'static + VpEvaluator<'a, S, CA, EVAL>, + CA: 'static + Clone, +{ + fn conversion_state(&self) -> &ConversionState { + self.state.conversion_state() + } +} + #[cfg(any(test, feature = "testing"))] pub(super) mod testing { use namada_core::collections::HashMap; diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index 334a040a5b..d4edc5824d 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3978,7 +3978,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "namada_wallet", "proptest", "rand 0.8.5", diff --git a/wasm_for_tests/Cargo.lock b/wasm_for_tests/Cargo.lock index b731d19718..7f6e91c697 100644 --- a/wasm_for_tests/Cargo.lock +++ b/wasm_for_tests/Cargo.lock @@ -2178,7 +2178,7 @@ dependencies = [ "namada_state", "namada_systems", "namada_tx", - "namada_vp", + "namada_vp_env", "rand", "rand_core", "ripemd", From 63d11138cd8b71e24c903a9621094889a0490df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Fri, 13 Sep 2024 09:33:12 +0200 Subject: [PATCH 10/10] changelog: add #3807 --- .changelog/unreleased/improvements/3807-env-agnostic-vps.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3807-env-agnostic-vps.md diff --git a/.changelog/unreleased/improvements/3807-env-agnostic-vps.md b/.changelog/unreleased/improvements/3807-env-agnostic-vps.md new file mode 100644 index 0000000000..37901aaf59 --- /dev/null +++ b/.changelog/unreleased/improvements/3807-env-agnostic-vps.md @@ -0,0 +1,2 @@ +- Refactored most native VPs to be agnostic to VP environment (WASM or native). + ([\#3807](https://github.com/anoma/namada/pull/3807)) \ No newline at end of file