Improve dry run

.changelog/unreleased/improvements/3746-fix-storage-gas-cost.md

@@ -0,0 +1,2 @@
+- Increased the gas cost for storage consumption and improved gas tracking for
+  masp fee payments. ([\#3746](https://github.com/anoma/namada/pull/3746))

.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))

crates/gas/src/lib.rs

@@ -64,7 +64,7 @@ const WRAPPER_TX_VALIDATION_GAS_RAW: u64 = 1_526_700;
 // space as a resource. This way, the storage occupation cost is not completely
 // free-floating but it's tied to the other costs
 const STORAGE_OCCUPATION_GAS_PER_BYTE_RAW: u64 =
-    PHYSICAL_STORAGE_LATENCY_PER_BYTE_RAW * (1 + 10);
+    PHYSICAL_STORAGE_LATENCY_PER_BYTE_RAW * (1 + 1_000);
 // NOTE: this accounts for the latency of a physical drive access. For read
 // accesses we have no way to tell if data was in cache or in storage. Moreover,
 // the latency shouldn't really be accounted per single byte but rather per
@@ -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
@@ -484,22 +485,6 @@ impl TxGasMeter {
             .checked_sub(self.transaction_gas)
             .unwrap_or_default()
     }
-
-    /// Copy the consumed gas from the other instance. Fails if this value
-    /// exceeds the gas limit of this gas meter or if overflow happened
-    pub fn copy_consumed_gas_from(&mut self, other: &Self) -> Result<()> {
-        self.transaction_gas = other.transaction_gas;
-        self.gas_overflow = other.gas_overflow;
-
-        if self.transaction_gas > self.tx_gas_limit {
-            return Err(Error::TransactionGasExceededError);
-        }
-        if self.gas_overflow {
-            return Err(Error::GasOverflow);
-        }
-
-        Ok(())
-    }
 }
 
 impl GasMetering for VpGasMeter {
@@ -528,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

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.",
             )));
         }
 

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;
@@ -36,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) => {
@@ -63,68 +61,43 @@ where
                 .into_storage_result()?;
 
                 state.write_log_mut().commit_tx_to_batch();
-                let available_gas = tx_gas_meter.borrow().get_available_gas();
-                (
-                    Some(tx.header_hash()),
-                    tx_result,
-                    TxGasMeter::new(available_gas),
-                )
+                (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)
                     .into_storage_result()?;
                 (
                     None,
                     TxResult::default().to_extended_result(None),
-                    TxGasMeter::new(gas_limit),
+                    RefCell::new(TxGasMeter::new(gas_limit)),
                 )
             }
         };
 
-    let ExtendedTxResult {
-        mut tx_result,
-        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)
-    {
-        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,
-            ),
-        );
-        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();
-        }
-        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 = tx_gas_meter
-        .borrow()
-        .get_tx_consumed_gas()
-        .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 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
+            .borrow()
+            .get_tx_consumed_gas()
+            .get_whole_gas_units(gas_scale),
+    );
 
     Ok(EncodedResponseQuery {
         data: dry_run_result.serialize_to_vec(),

crates/node/src/protocol.rs

@@ -323,17 +323,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<Error>,
     tx_index: TxIndex,
     tx_gas_meter: &'a RefCell<TxGasMeter>,
-    state: &'a mut WlState<D, H>,
+    state: &'a mut S,
     vp_wasm_cache: &'a mut VpCache<CA>,
     tx_wasm_cache: &'a mut TxCache<CA>,
 ) -> std::result::Result<ExtendedTxResult<Error>, DispatchError>
 where
+    S: 'static + State<D = D, H = H> + Read<Err = state::Error> + Sync,
     D: 'static + DB + for<'iter> DBIter<'iter> + Sync,
     H: 'static + StorageHasher + Sync,
     CA: 'static + WasmCacheAccess + Sync,
@@ -695,26 +696,24 @@ where
         "The first transaction in the batch failed to pay fees via the MASP.";
 
     // The fee payment is subject to a gas limit imposed by a protocol
-    // parameter. Here we instantiate a custom gas meter for this step and
-    // initialize it with the already consumed gas. The gas limit should
-    // actually be the lowest between the protocol parameter and the actual gas
-    // limit of the transaction
+    // parameter. Here we instantiate a custom gas meter for this step where the
+    // gas limit is actually the lowest between the protocol parameter and the
+    // actual remaining gas of the transaction. The latter is because we only
+    // care about the masp execution, not any gas used before this step, which
+    // could prevent some transactions (e.g. batches consuming a lot of gas for
+    // their size) from being accepted
     let max_gas_limit = state
         .read::<u64>(&parameters::storage::get_masp_fee_payment_gas_limit_key())
         .expect("Error reading the storage")
         .expect("Missing masp fee payment gas limit in storage")
-        .min(tx_gas_meter.borrow().tx_gas_limit.into());
+        .min(tx_gas_meter.borrow().get_available_gas().into());
     let gas_scale = get_gas_scale(&**state).map_err(Error::Error)?;
 
-    let mut gas_meter = TxGasMeter::new(
+    let masp_gas_meter = RefCell::new(TxGasMeter::new(
         Gas::from_whole_units(max_gas_limit.into(), gas_scale).ok_or_else(
             || Error::GasError("Overflow in gas expansion".to_string()),
         )?,
-    );
-    gas_meter
-        .copy_consumed_gas_from(&tx_gas_meter.borrow())
-        .map_err(|e| Error::GasError(e.to_string()))?;
-    let ref_unshield_gas_meter = RefCell::new(gas_meter);
+    ));
 
     let valid_batched_tx_result = {
         let first_tx = tx
@@ -724,7 +723,7 @@ where
             &first_tx,
             tx_index,
             ShellParams {
-                tx_gas_meter: &ref_unshield_gas_meter,
+                tx_gas_meter: &masp_gas_meter,
                 state: *state,
                 vp_wasm_cache,
                 tx_wasm_cache,
@@ -789,7 +788,7 @@ where
 
     tx_gas_meter
         .borrow_mut()
-        .copy_consumed_gas_from(&ref_unshield_gas_meter.borrow())
+        .consume(masp_gas_meter.borrow().get_tx_consumed_gas().into())
         .map_err(|e| Error::GasError(e.to_string()))?;
 
     Ok(valid_batched_tx_result)
@@ -927,9 +926,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<S, D, H, CA>(
+// 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<S, D, H, CA>(
     batched_tx: &BatchedTxRef<'_>,
     tx_index: &TxIndex,
     shell_params: ShellParams<'_, S, D, H, CA>,

crates/node/src/shell/finalize_block.rs

@@ -1269,7 +1269,7 @@ mod test_finalize_block {
     };
     use crate::tendermint::abci::types::Validator;
 
-    const WRAPPER_GAS_LIMIT: u64 = 1_500_000;
+    const WRAPPER_GAS_LIMIT: u64 = 10_000_000;
     const STORAGE_VALUE: &str = "test_value";
 
     /// Make a wrapper tx and a processed tx from the wrapped tx that can be

crates/sdk/src/lib.rs

@@ -82,7 +82,7 @@ use tx::{
 use wallet::{Wallet, WalletIo, WalletStorage};
 
 /// Default gas-limit
-pub const DEFAULT_GAS_LIMIT: u64 = 150_000;
+pub const DEFAULT_GAS_LIMIT: u64 = 200_000;
 
 #[allow(missing_docs)]
 #[cfg(not(feature = "async-send"))]

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,

crates/tests/src/e2e/ibc_tests.rs

@@ -1053,7 +1053,7 @@ fn transfer(
         "--port-id",
         &port_id,
         "--gas-limit",
-        "200000",
+        "250000",
         "--node",
         &rpc,
     ]);
@@ -1229,7 +1229,7 @@ fn propose_inflation(test: &Test) -> Result<Epoch> {
         "--data-path",
         proposal_json_path.to_str().unwrap(),
         "--gas-limit",
-        "4000000",
+        "10000000",
         "--node",
         &rpc,
     ]);

crates/tests/src/e2e/ledger_tests.rs

@@ -2044,7 +2044,7 @@ fn proposal_change_shielded_reward() -> Result<()> {
         "--data-path",
         valid_proposal_json_path.to_str().unwrap(),
         "--gas-limit",
-        "4000000",
+        "10000000",
         "--node",
         &validator_one_rpc,
     ]);

crates/tests/src/integration/ledger_tests.rs

@@ -724,7 +724,7 @@ fn proposal_submission() -> Result<()> {
         "--data-path",
         valid_proposal_json_path.to_str().unwrap(),
         "--gas-limit",
-        "5000000",
+        "10000000",
         "--node",
         &validator_one_rpc,
     ]);
@@ -1463,7 +1463,7 @@ fn implicit_account_reveal_pk() -> Result<()> {
                 "--signing-keys",
                 source,
                 "--gas-limit",
-                "2000000",
+                "3500000",
                 "--node",
                 &validator_one_rpc,
             ]