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] 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> =