From 151679f1d9193f6e1bea2e1ad3604ac21ee9f04c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 3 Sep 2024 17:47:39 +0200 Subject: [PATCH 1/7] Increases gas cost for storage --- crates/gas/src/lib.rs | 2 +- crates/node/src/shell/finalize_block.rs | 2 +- crates/sdk/src/lib.rs | 3 ++- crates/tests/src/integration/ledger_tests.rs | 4 ++-- crates/tests/src/integration/masp.rs | 15 +++++++++------ genesis/hardware/parameters.toml | 6 +++--- genesis/localnet/parameters.toml | 3 ++- genesis/starter/parameters.toml | 2 +- 8 files changed, 21 insertions(+), 16 deletions(-) diff --git a/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index 2847c9c0ac..78707c5352 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 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..55c14d5275 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -82,7 +82,8 @@ use tx::{ use wallet::{Wallet, WalletIo, WalletStorage}; /// Default gas-limit -pub const DEFAULT_GAS_LIMIT: u64 = 150_000; +// FIXME: lower +pub const DEFAULT_GAS_LIMIT: u64 = 200_000; #[allow(missing_docs)] #[cfg(not(feature = "async-send"))] 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..5e668d9f7a 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -685,8 +685,9 @@ fn masp_incentives() -> Result<()> { CHRISTEL, "--token", NAM, + // FIXME: lower if possible "--gas-limit", - "200000", + "300000", "--amount", "1.451732", "--signing-keys", @@ -2969,8 +2970,9 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { NAM, "--amount", "9000", + // FIXME: lower "--gas-limit", - "200000", + "300000", "--gas-price", "1", "--gas-spending-key", @@ -3028,7 +3030,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 +3151,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "--token", BTC, "--amount", - "200000", + "300000", "--gas-payer", ALBERT_KEY, "--ledger-address", @@ -3218,7 +3220,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("btc: 200000")); + assert!(captured.contains("btc: 300000")); // Masp fee payment with custom token and gas payer let captured = CapturedOutput::of(|| { @@ -3237,8 +3239,9 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "1", "--gas-token", BTC, + // FIXME: reduce this "--gas-limit", - "200000", + "300000", "--gas-price", "1", "--gas-spending-key", diff --git a/genesis/hardware/parameters.toml b/genesis/hardware/parameters.toml index f3b48ffece..60eda654c7 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 = 15_000_000 # Masp fee payment gas limit -masp_fee_payment_gas_limit = 150_000 +masp_fee_payment_gas_limit = 300_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..b6f91a135a 100644 --- a/genesis/localnet/parameters.toml +++ b/genesis/localnet/parameters.toml @@ -21,7 +21,8 @@ masp_epoch_multiplier = 2 # Max gas for block max_block_gas = 15_000_000 # Masp fee payment gas limit -masp_fee_payment_gas_limit = 200_000 +# FIXME: lower +masp_fee_payment_gas_limit = 300_000 # Gas scale gas_scale = 10_000 diff --git a/genesis/starter/parameters.toml b/genesis/starter/parameters.toml index 143c68fd5d..fd088caa75 100644 --- a/genesis/starter/parameters.toml +++ b/genesis/starter/parameters.toml @@ -21,7 +21,7 @@ masp_epoch_multiplier = 2 # Max gas for block max_block_gas = 15_000_000 # Masp fee payment gas limit -masp_fee_payment_gas_limit = 200_000 +masp_fee_payment_gas_limit = 300_000 # Gas scale gas_scale = 10_000 From 3d89802c2af700a488ea716f733d5f1ccaa587e2 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 3 Sep 2024 18:26:47 +0200 Subject: [PATCH 2/7] Gas for masp fee payment does not account for already used gas --- crates/gas/src/lib.rs | 16 ---------------- crates/node/src/protocol.rs | 24 +++++++++++------------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/crates/gas/src/lib.rs b/crates/gas/src/lib.rs index 78707c5352..dbbb22a13d 100644 --- a/crates/gas/src/lib.rs +++ b/crates/gas/src/lib.rs @@ -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/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) From 52d9117860d1051bc26453e4c961b6ba7b4fe68a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 3 Sep 2024 20:02:49 +0200 Subject: [PATCH 3/7] Updates gas limits --- crates/sdk/src/lib.rs | 1 - crates/tests/src/integration/masp.rs | 17 +++++++---------- genesis/hardware/parameters.toml | 4 ++-- genesis/localnet/parameters.toml | 5 ++--- genesis/starter/parameters.toml | 4 ++-- 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index 55c14d5275..517f160611 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -82,7 +82,6 @@ use tx::{ use wallet::{Wallet, WalletIo, WalletStorage}; /// Default gas-limit -// FIXME: lower pub const DEFAULT_GAS_LIMIT: u64 = 200_000; #[allow(missing_docs)] diff --git a/crates/tests/src/integration/masp.rs b/crates/tests/src/integration/masp.rs index 5e668d9f7a..479417e44d 100644 --- a/crates/tests/src/integration/masp.rs +++ b/crates/tests/src/integration/masp.rs @@ -685,9 +685,8 @@ fn masp_incentives() -> Result<()> { CHRISTEL, "--token", NAM, - // FIXME: lower if possible "--gas-limit", - "300000", + "250000", "--amount", "1.451732", "--signing-keys", @@ -2903,7 +2902,7 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { "--token", NAM, "--amount", - "300000", + "250000", "--ledger-address", validator_one_rpc, ], @@ -2953,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,9 +2969,8 @@ fn masp_fee_payment_with_custom_spending_key() -> Result<()> { NAM, "--amount", "9000", - // FIXME: lower "--gas-limit", - "300000", + "250000", "--gas-price", "1", "--gas-spending-key", @@ -3151,7 +3149,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "--token", BTC, "--amount", - "300000", + "250000", "--gas-payer", ALBERT_KEY, "--ledger-address", @@ -3220,7 +3218,7 @@ fn masp_fee_payment_with_different_token() -> Result<()> { ) }); assert!(captured.result.is_ok()); - assert!(captured.contains("btc: 300000")); + assert!(captured.contains("btc: 250000")); // Masp fee payment with custom token and gas payer let captured = CapturedOutput::of(|| { @@ -3239,9 +3237,8 @@ fn masp_fee_payment_with_different_token() -> Result<()> { "1", "--gas-token", BTC, - // FIXME: reduce this "--gas-limit", - "300000", + "250000", "--gas-price", "1", "--gas-spending-key", diff --git a/genesis/hardware/parameters.toml b/genesis/hardware/parameters.toml index 60eda654c7..584a4b5258 100644 --- a/genesis/hardware/parameters.toml +++ b/genesis/hardware/parameters.toml @@ -19,9 +19,9 @@ 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 = 300_000 +masp_fee_payment_gas_limit = 200_000 # Gas scale gas_scale = 10_000 diff --git a/genesis/localnet/parameters.toml b/genesis/localnet/parameters.toml index b6f91a135a..60f780cc1b 100644 --- a/genesis/localnet/parameters.toml +++ b/genesis/localnet/parameters.toml @@ -19,10 +19,9 @@ 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 -# FIXME: lower -masp_fee_payment_gas_limit = 300_000 +masp_fee_payment_gas_limit = 200_000 # Gas scale gas_scale = 10_000 diff --git a/genesis/starter/parameters.toml b/genesis/starter/parameters.toml index fd088caa75..ad8f91eaf4 100644 --- a/genesis/starter/parameters.toml +++ b/genesis/starter/parameters.toml @@ -19,9 +19,9 @@ 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 = 300_000 +masp_fee_payment_gas_limit = 200_000 # Gas scale gas_scale = 10_000 From 9938fca578a91bc378b7f4269e536ad119fe9d9e Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 3 Sep 2024 23:25:33 +0200 Subject: [PATCH 4/7] Increases gas limits for ibc e2e tests --- crates/tests/src/e2e/ibc_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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, ]); From e1267e194621ebff0fdd979e36827b31dd1d356c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 4 Sep 2024 11:19:41 +0200 Subject: [PATCH 5/7] Fixes gas in proposal e2e test --- crates/tests/src/e2e/ledger_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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, ]); From 89499b2633e611134e8e3cf3aa7ccd2ef5ee8cea Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 4 Sep 2024 23:54:14 +0200 Subject: [PATCH 6/7] Fixes consumed gas calculation in `dry_run` --- crates/node/src/dry_run_tx.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) 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); From d3112788157449d2d40bdc820b80661f7bceeb23 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 3 Sep 2024 20:04:37 +0200 Subject: [PATCH 7/7] Changelog #3746 --- .changelog/unreleased/improvements/3746-fix-storage-gas-cost.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3746-fix-storage-gas-cost.md 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