Merge pull request #3746 from anoma/grarco/fix-storage-gas-cost

Update storage gas cost

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

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
@@ -484,22 +484,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 {

crates/node/src/dry_run_tx.rs

@@ -37,7 +37,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) =
+    let (wrapper_hash, extended_tx_result, tx_gas_meter, gas_used) =
         match tx.header().tx_type {
             TxType::Wrapper(wrapper) => {
                 let gas_limit = wrapper
@@ -64,10 +64,12 @@ where
 
                 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,
                 )
             }
             _ => {
@@ -81,6 +83,7 @@ where
                     None,
                     TxResult::default().to_extended_result(None),
                     TxGasMeter::new(gas_limit),
+                    0.into(),
                 )
             }
         };
@@ -119,9 +122,9 @@ where
         );
     }
     // Account gas for both batch and wrapper
-    let gas_used = tx_gas_meter
-        .borrow()
-        .get_tx_consumed_gas()
+    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);

crates/node/src/protocol.rs

@@ -701,26 +701,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
@@ -730,7 +728,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,
@@ -795,7 +793,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)

crates/node/src/shell/finalize_block.rs

@@ -1352,7 +1352,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 @@ pub use {namada_io as io, namada_wallet as wallet};
 use crate::masp::ShieldedContext;
 
 /// Default gas-limit
-pub const DEFAULT_GAS_LIMIT: u64 = 150_000;
+pub const DEFAULT_GAS_LIMIT: u64 = 200_000;
 
 #[cfg_attr(feature = "async-send", async_trait::async_trait)]
 #[cfg_attr(not(feature = "async-send"), async_trait::async_trait(?Send))]

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

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

crates/tests/src/integration/masp.rs

@@ -687,7 +687,7 @@ fn masp_incentives() -> Result<()> {
             "--token",
             NAM,
             "--gas-limit",
-            "200000",
+            "250000",
             "--amount",
             "1.451732",
             "--signing-keys",
@@ -3067,7 +3067,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> {
             "--token",
             NAM,
             "--amount",
-            "300000",
+            "250000",
             "--ledger-address",
             validator_one_rpc,
         ],
@@ -3117,7 +3117,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> {
         )
     });
     assert!(captured.result.is_ok());
-    assert!(captured.contains("nam: 300000"));
+    assert!(captured.contains("nam: 250000"));
 
     // Masp fee payment with custom gas payer
     let captured = CapturedOutput::of(|| {
@@ -3135,7 +3135,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> {
                 "--amount",
                 "9000",
                 "--gas-limit",
-                "200000",
+                "250000",
                 "--gas-price",
                 "1",
                 "--gas-spending-key",
@@ -3193,7 +3193,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> {
         )
     });
     assert!(captured.result.is_ok());
-    assert!(captured.contains("nam: 101000"));
+    assert!(captured.contains("nam: 1000"));
 
     let captured = CapturedOutput::of(|| {
         run(
@@ -3314,7 +3314,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> {
             "--token",
             BTC,
             "--amount",
-            "200000",
+            "250000",
             "--gas-payer",
             ALBERT_KEY,
             "--ledger-address",
@@ -3383,7 +3383,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> {
         )
     });
     assert!(captured.result.is_ok());
-    assert!(captured.contains("btc: 200000"));
+    assert!(captured.contains("btc: 250000"));
 
     // Masp fee payment with custom token and gas payer
     let captured = CapturedOutput::of(|| {
@@ -3403,7 +3403,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> {
                 "--gas-token",
                 BTC,
                 "--gas-limit",
-                "200000",
+                "250000",
                 "--gas-price",
                 "1",
                 "--gas-spending-key",

genesis/hardware/parameters.toml

@@ -19,11 +19,11 @@ epochs_per_year = 31_536_000
 # The multiplier for masp epochs
 masp_epoch_multiplier = 2
 # Max gas for block
-max_block_gas = 20000000
+max_block_gas = 20_000_000
 # Masp fee payment gas limit
-masp_fee_payment_gas_limit = 150_000
+masp_fee_payment_gas_limit = 200_000
 # Gas scale
-gas_scale = 10_000_000
+gas_scale = 10_000
 
 # Map of the cost per gas unit for every token allowed for fee payment
 [parameters.minimum_gas_price]

genesis/localnet/parameters.toml

@@ -19,7 +19,7 @@ epochs_per_year = 31_536_000
 # The multiplier for masp epochs
 masp_epoch_multiplier = 2
 # Max gas for block
-max_block_gas = 15_000_000
+max_block_gas = 20_000_000
 # Masp fee payment gas limit
 masp_fee_payment_gas_limit = 200_000
 # Gas scale

genesis/starter/parameters.toml

@@ -19,7 +19,7 @@ epochs_per_year = 31_536_000
 # The multiplier for masp epochs
 masp_epoch_multiplier = 2
 # Max gas for block
-max_block_gas = 15_000_000
+max_block_gas = 20_000_000
 # Masp fee payment gas limit
 masp_fee_payment_gas_limit = 200_000
 # Gas scale