From a7de96552621c1831c9d7119f863e15e9e485460 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 5 Sep 2024 16:54:27 +0200 Subject: [PATCH 1/6] Fixes bugs in `dry_run` and improves gas tracking --- crates/node/src/dry_run_tx.rs | 63 +++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/crates/node/src/dry_run_tx.rs b/crates/node/src/dry_run_tx.rs index 678373daf6..661faa9557 100644 --- a/crates/node/src/dry_run_tx.rs +++ b/crates/node/src/dry_run_tx.rs @@ -19,6 +19,9 @@ use namada_vm::WasmCacheAccess; use crate::protocol; use crate::protocol::ShellParams; +// FIXME: at the end try to use dispatch_tx andh change everything needed +// FIXME: or maybe just change dispatch_inner_tx to handle S isntead of +// WlState?? /// Dry run a transaction pub fn dry_run_tx( mut state: namada_sdk::state::TempWlState<'static, D, H>, @@ -37,7 +40,7 @@ where let gas_scale = parameters::get_gas_scale(&state)?; // Wrapper dry run to allow estimating the gas cost of a transaction - let (wrapper_hash, extended_tx_result, tx_gas_meter, gas_used) = + let (wrapper_hash, extended_tx_result, tx_gas_meter) = match tx.header().tx_type { TxType::Wrapper(wrapper) => { let gas_limit = wrapper @@ -63,14 +66,7 @@ where .into_storage_result()?; state.write_log_mut().commit_tx_to_batch(); - let available_gas = tx_gas_meter.borrow().get_available_gas(); - let consumed_gas = tx_gas_meter.borrow().get_tx_consumed_gas(); - ( - Some(tx.header_hash()), - tx_result, - TxGasMeter::new(available_gas), - consumed_gas, - ) + (Some(tx.header_hash()), tx_result, tx_gas_meter) } _ => { // If dry run only the inner tx, use the max block gas as @@ -82,8 +78,7 @@ where ( None, TxResult::default().to_extended_result(None), - TxGasMeter::new(gas_limit), - 0.into(), + RefCell::new(TxGasMeter::new(gas_limit)), ) } }; @@ -93,7 +88,6 @@ where ref masp_tx_refs, ibc_tx_data_refs, } = extended_tx_result; - let tx_gas_meter = RefCell::new(tx_gas_meter); for cmt in protocol::get_batch_txs_to_execute(&tx, masp_tx_refs, &ibc_tx_data_refs) { @@ -108,26 +102,37 @@ where &mut tx_wasm_cache, ), ); - let is_accepted = - matches!(&batched_tx_result, Ok(result) if result.is_accepted()); - if is_accepted { - state.write_log_mut().commit_tx_to_batch(); - } else { - state.write_log_mut().drop_tx(); + match &batched_tx_result { + Err(crate::protocol::Error::GasError(err)) => { + // Gas errors interrupt the execution of the batch + Err(crate::protocol::Error::GasError(err.to_owned())) + .into_storage_result()?; + } + res => { + let is_accepted = + matches!(res, Ok(result) if result.is_accepted()); + if is_accepted { + state.write_log_mut().commit_tx_to_batch(); + } else { + state.write_log_mut().drop_tx(); + } + + tx_result.insert_inner_tx_result( + wrapper_hash.as_ref(), + either::Right(cmt), + batched_tx_result, + ); + } } - tx_result.insert_inner_tx_result( - wrapper_hash.as_ref(), - either::Right(cmt), - batched_tx_result, - ); } - // Account gas for both batch and wrapper - let gas_used = gas_used - .checked_add(tx_gas_meter.borrow().get_tx_consumed_gas()) - .unwrap_or(u64::MAX.into()) - .get_whole_gas_units(gas_scale); let tx_result_string = tx_result.to_result_string(); - let dry_run_result = DryRunResult(tx_result_string, gas_used); + let dry_run_result = DryRunResult( + tx_result_string, + tx_gas_meter + .borrow() + .get_tx_consumed_gas() + .get_whole_gas_units(gas_scale), + ); Ok(EncodedResponseQuery { data: dry_run_result.serialize_to_vec(), From 064f81156b605f2569258d711230f126312421d7 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 5 Sep 2024 16:55:17 +0200 Subject: [PATCH 2/6] Misc improvements to comments --- crates/gas/src/lib.rs | 4 +++- crates/shielded_token/src/utils.rs | 2 +- crates/trans_token/src/storage.rs | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index dbbb22a13d..e97c8abc09 100644 --- a/crates/gas/src/lib.rs +++ b/crates/gas/src/lib.rs @@ -367,7 +367,7 @@ pub trait GasMetering { ) } - /// Get the gas consumed by the tx alone + /// Get the gas consumed by the tx fn get_tx_consumed_gas(&self) -> Gas; /// Get the gas limit @@ -434,6 +434,7 @@ impl GasMetering for TxGasMeter { Ok(()) } + /// Get the entire gas used by the transaction up until this point fn get_tx_consumed_gas(&self) -> Gas { if !self.gas_overflow { self.transaction_gas @@ -512,6 +513,7 @@ impl GasMetering for VpGasMeter { Ok(()) } + /// Get the gas consumed by the tx alone before the vps were executed fn get_tx_consumed_gas(&self) -> Gas { if !self.gas_overflow { self.initial_gas diff --git a/crates/shielded_token/src/utils.rs b/crates/shielded_token/src/utils.rs index 9593c93a20..5202e39350 100644 --- a/crates/shielded_token/src/utils.rs +++ b/crates/shielded_token/src/utils.rs @@ -29,7 +29,7 @@ fn reveal_nullifiers( /// Appends the note commitments of the provided transaction to the merkle tree /// and updates the anchor /// NOTE: this function is public as a temporary workaround because of an issue -/// when running this function in WASM +/// when running it in WASM (https://github.com/anoma/masp/issues/73) pub fn update_note_commitment_tree( ctx: &mut (impl StorageRead + StorageWrite), transaction: &Transaction, diff --git a/crates/trans_token/src/storage.rs b/crates/trans_token/src/storage.rs index 02281fec91..fb497f3675 100644 --- a/crates/trans_token/src/storage.rs +++ b/crates/trans_token/src/storage.rs @@ -310,7 +310,7 @@ where ) .ok_or_else(underflow_err)? }; - // Wite the new balance + // Write the new balance storage.write(&owner_key, new_owner_balance)?; } Ok(debited_accounts) From e51834c71168194c41e1c43062167e987245bab6 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 5 Sep 2024 16:58:51 +0200 Subject: [PATCH 3/6] Removes unused vars from host env fn --- crates/vm/src/host_env.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/vm/src/host_env.rs b/crates/vm/src/host_env.rs index 02288e7779..77f2b1dcb0 100644 --- a/crates/vm/src/host_env.rs +++ b/crates/vm/src/host_env.rs @@ -2134,8 +2134,6 @@ where H: 'static + StorageHasher, CA: WasmCacheAccess, { - let _sentinel = unsafe { env.ctx.sentinel.get() }; - let _gas_meter = unsafe { env.ctx.gas_meter.get() }; let (serialized_transaction, gas) = env .memory .read_bytes(transaction_ptr, transaction_len.try_into()?) From 58383f8fa6ab503dbe750e1bcb1a8b358ccb1ff9 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 5 Sep 2024 17:07:06 +0200 Subject: [PATCH 4/6] Improves governance error message --- crates/governance/src/vp/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/governance/src/vp/mod.rs b/crates/governance/src/vp/mod.rs index 2ec68c3076..2471d2ef21 100644 --- a/crates/governance/src/vp/mod.rs +++ b/crates/governance/src/vp/mod.rs @@ -566,7 +566,7 @@ where if !proposal_type.is_default_with_wasm() { return Err(Error::new_alloc(format!( "Proposal with id {proposal_id} modified a proposal code key, \ - but its type is not default.", + but its type is not allowed this change.", ))); } From bfaef8a0b277e89f654ff1757f7cc7fcd866d3c7 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 5 Sep 2024 17:37:48 +0200 Subject: [PATCH 5/6] Code reusage in `dry_run_tx` --- crates/node/src/dry_run_tx.rs | 69 +++++++++-------------------------- crates/node/src/protocol.rs | 11 +++--- 2 files changed, 23 insertions(+), 57 deletions(-) diff --git a/crates/node/src/dry_run_tx.rs b/crates/node/src/dry_run_tx.rs index 661faa9557..17d3f9f64f 100644 --- a/crates/node/src/dry_run_tx.rs +++ b/crates/node/src/dry_run_tx.rs @@ -9,9 +9,7 @@ use namada_sdk::queries::{EncodedResponseQuery, RequestQuery}; use namada_sdk::state::{ DBIter, Result, ResultExt, StorageHasher, TxIndex, DB, }; -use namada_sdk::tx::data::{ - DryRunResult, ExtendedTxResult, GasLimit, TxResult, TxType, -}; +use namada_sdk::tx::data::{DryRunResult, GasLimit, TxResult, TxType}; use namada_sdk::tx::Tx; use namada_vm::wasm::{TxCache, VpCache}; use namada_vm::WasmCacheAccess; @@ -19,9 +17,6 @@ use namada_vm::WasmCacheAccess; use crate::protocol; use crate::protocol::ShellParams; -// FIXME: at the end try to use dispatch_tx andh change everything needed -// FIXME: or maybe just change dispatch_inner_tx to handle S isntead of -// WlState?? /// Dry run a transaction pub fn dry_run_tx( mut state: namada_sdk::state::TempWlState<'static, D, H>, @@ -39,7 +34,7 @@ where let gas_scale = parameters::get_gas_scale(&state)?; - // Wrapper dry run to allow estimating the gas cost of a transaction + // Wrapper dry run to allow estimating the entire gas cost of a transaction let (wrapper_hash, extended_tx_result, tx_gas_meter) = match tx.header().tx_type { TxType::Wrapper(wrapper) => { @@ -69,8 +64,8 @@ where (Some(tx.header_hash()), tx_result, tx_gas_meter) } _ => { - // If dry run only the inner tx, use the max block gas as - // the gas limit + // When dry running only the inner tx(s), use the max block gas + // as the gas limit let max_block_gas = parameters::get_max_block_gas(&state)?; let gas_limit = GasLimit::from(max_block_gas) .as_scaled_gas(gas_scale) @@ -83,49 +78,19 @@ where } }; - let ExtendedTxResult { - mut tx_result, - ref masp_tx_refs, - ibc_tx_data_refs, - } = extended_tx_result; - for cmt in - protocol::get_batch_txs_to_execute(&tx, masp_tx_refs, &ibc_tx_data_refs) - { - let batched_tx = tx.batch_ref_tx(cmt); - let batched_tx_result = protocol::apply_wasm_tx( - &batched_tx, - &TxIndex(0), - ShellParams::new( - &tx_gas_meter, - &mut state, - &mut vp_wasm_cache, - &mut tx_wasm_cache, - ), - ); - match &batched_tx_result { - Err(crate::protocol::Error::GasError(err)) => { - // Gas errors interrupt the execution of the batch - Err(crate::protocol::Error::GasError(err.to_owned())) - .into_storage_result()?; - } - res => { - let is_accepted = - matches!(res, Ok(result) if result.is_accepted()); - if is_accepted { - state.write_log_mut().commit_tx_to_batch(); - } else { - state.write_log_mut().drop_tx(); - } - - tx_result.insert_inner_tx_result( - wrapper_hash.as_ref(), - either::Right(cmt), - batched_tx_result, - ); - } - } - } - let tx_result_string = tx_result.to_result_string(); + let extended_tx_result = protocol::dispatch_inner_txs( + &tx, + wrapper_hash.as_ref(), + extended_tx_result, + TxIndex(0), + &tx_gas_meter, + &mut state, + &mut vp_wasm_cache, + &mut tx_wasm_cache, + ) + .map_err(|err| err.error) + .into_storage_result()?; + let tx_result_string = extended_tx_result.tx_result.to_result_string(); let dry_run_result = DryRunResult( tx_result_string, tx_gas_meter diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 8a09a3b873..66bd751e66 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -329,17 +329,18 @@ pub(crate) fn get_batch_txs_to_execute<'a>( } #[allow(clippy::too_many_arguments)] -fn dispatch_inner_txs<'a, D, H, CA>( +pub(crate) fn dispatch_inner_txs<'a, S, D, H, CA>( tx: &Tx, wrapper_hash: Option<&'a Hash>, mut extended_tx_result: ExtendedTxResult, tx_index: TxIndex, tx_gas_meter: &'a RefCell, - state: &'a mut WlState, + state: &'a mut S, vp_wasm_cache: &'a mut VpCache, tx_wasm_cache: &'a mut TxCache, ) -> std::result::Result, DispatchError> where + S: 'static + State + Read + Sync, D: 'static + DB + for<'iter> DBIter<'iter> + Sync, H: 'static + StorageHasher + Sync, CA: 'static + WasmCacheAccess + Sync, @@ -931,9 +932,9 @@ where } } -/// Apply a transaction going via the wasm environment. Gas will be metered and -/// validity predicates will be triggered in the normal way. -pub fn apply_wasm_tx( +// Apply a transaction going via the wasm environment. Gas will be metered and +// validity predicates will be triggered in the normal way. +fn apply_wasm_tx( batched_tx: &BatchedTxRef<'_>, tx_index: &TxIndex, shell_params: ShellParams<'_, S, D, H, CA>, From 1646626324e5c1d2c430925e9b9f3da525f4501b Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 5 Sep 2024 17:47:14 +0200 Subject: [PATCH 6/6] Changelog #3758 --- .changelog/unreleased/improvements/3758-improve-dry-run.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3758-improve-dry-run.md diff --git a/.changelog/unreleased/improvements/3758-improve-dry-run.md b/.changelog/unreleased/improvements/3758-improve-dry-run.md new file mode 100644 index 0000000000..910bef0f32 --- /dev/null +++ b/.changelog/unreleased/improvements/3758-improve-dry-run.md @@ -0,0 +1,2 @@ +- Miscellaneous improvements and bug fixes to the dry run command. + ([\#3758](https://github.com/anoma/namada/pull/3758)) \ No newline at end of file