diff --git a/.changelog/unreleased/improvements/3746-fix-storage-gas-cost.md b/.changelog/unreleased/improvements/3746-fix-storage-gas-cost.md new file mode 100644 index 0000000000..283f4b5e0e --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index 2847c9c0ac..dbbb22a13d 100644 --- a/crates/gas/src/lib.rs +++ b/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 { diff --git a/crates/node/src/dry_run_tx.rs b/crates/node/src/dry_run_tx.rs index 794b509d12..33fbd764d2 100644 --- a/crates/node/src/dry_run_tx.rs +++ b/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); diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index be8c2e3d17..8049de646b 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -695,26 +695,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::(¶meters::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 +722,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 +787,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) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 57c52ffac6..df9bb3ad55 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/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 diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index 1ebec43146..517f160611 100644 --- a/crates/sdk/src/lib.rs +++ b/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"))] diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 6c14698c13..5d0d1864bb 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/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 { "--data-path", proposal_json_path.to_str().unwrap(), "--gas-limit", - "4000000", + "10000000", "--node", &rpc, ]); diff --git a/crates/tests/src/e2e/ledger_tests.rs b/crates/tests/src/e2e/ledger_tests.rs index 4470a85e39..74ef0e0b9d 100644 --- a/crates/tests/src/e2e/ledger_tests.rs +++ b/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, ]); diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 9926204a54..0bb471bd30 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/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, ] diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 19d092dbb3..479417e44d 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -686,7 +686,7 @@ fn masp_incentives() -> Result<()> { "--token", NAM, "--gas-limit", - "200000", + "250000", "--amount", "1.451732", "--signing-keys", @@ -2902,7 +2902,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { "--token", NAM, "--amount", - "300000", + "250000", "--ledger-address", validator_one_rpc, ], @@ -2952,7 +2952,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(|| { @@ -2970,7 +2970,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { "--amount", "9000", "--gas-limit", - "200000", + "250000", "--gas-price", "1", "--gas-spending-key", @@ -3028,7 +3028,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( @@ -3149,7 +3149,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "--token", BTC, "--amount", - "200000", + "250000", "--gas-payer", ALBERT_KEY, "--ledger-address", @@ -3218,7 +3218,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(|| { @@ -3238,7 +3238,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "--gas-token", BTC, "--gas-limit", - "200000", + "250000", "--gas-price", "1", "--gas-spending-key", diff --git a/genesis/hardware/parameters.toml b/genesis/hardware/parameters.toml index f3b48ffece..584a4b5258 100644 --- a/genesis/hardware/parameters.toml +++ b/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] diff --git a/genesis/localnet/parameters.toml b/genesis/localnet/parameters.toml index 66cbefe091..60f780cc1b 100644 --- a/genesis/localnet/parameters.toml +++ b/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 diff --git a/genesis/starter/parameters.toml b/genesis/starter/parameters.toml index 143c68fd5d..ad8f91eaf4 100644 --- a/genesis/starter/parameters.toml +++ b/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