feat(gas-keys): charge compute for gas key nonce removal#15068
feat(gas-keys): charge compute for gas key nonce removal#15068darioush wants to merge 2 commits intodarioush/gaskeys-txfeesfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements proper compute cost charging for gas key nonce removal operations during DeleteKey and DeleteAccount actions, replacing the previous Gas::ZERO placeholder implementation.
Changes:
- Introduces a
storage_removes_computehelper function that calculates compute costs based on storage removal operation counts and byte sizes - Updates
DeleteKeyaction to charge accurate per-nonce compute costs based on exact key and value lengths when deleting gas keys - Modifies
DeleteAccountaction to track and charge compute costs for all gas key nonces removed during account deletion
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/runtime/src/config.rs | Adds storage_removes_compute helper function to calculate compute costs for storage removals |
| runtime/runtime/src/access_keys.rs | Updates action_delete_key and delete_gas_key to accept RuntimeConfig and charge proper compute costs for gas key nonce removals |
| runtime/runtime/src/actions.rs | Updates action_delete_account to charge compute costs using information from RemoveAccountResult |
| core/store/src/utils/mod.rs | Adds RemoveAccountResult struct and updates remove_account to track nonce count and key bytes during iteration |
| runtime/runtime/src/lib.rs | Updates function calls to pass RuntimeConfig instead of just RuntimeFeesConfig |
| runtime/runtime/src/actions_test_utils.rs | Updates test helper to pass RuntimeConfig parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let compute = storage_removes_compute( | ||
| &config.wasm_config.ext_costs, | ||
| remove_result.gas_key_nonce_count, | ||
| remove_result.gas_key_nonce_total_key_bytes, | ||
| AccessKey::NONCE_VALUE_LEN * remove_result.gas_key_nonce_count, |
There was a problem hiding this comment.
The multiplication AccessKey::NONCE_VALUE_LEN * remove_result.gas_key_nonce_count could potentially overflow. For consistency with similar code patterns in the codebase, consider using checked arithmetic (e.g., checked_mul().unwrap() or returning a Result on overflow).
| let compute = storage_removes_compute( | |
| &config.wasm_config.ext_costs, | |
| remove_result.gas_key_nonce_count, | |
| remove_result.gas_key_nonce_total_key_bytes, | |
| AccessKey::NONCE_VALUE_LEN * remove_result.gas_key_nonce_count, | |
| let nonce_value_bytes = AccessKey::NONCE_VALUE_LEN | |
| .checked_mul(remove_result.gas_key_nonce_count) | |
| .ok_or_else(|| { | |
| StorageError::StorageInconsistentState( | |
| "gas_key_nonce_count overflow when computing nonce bytes".to_string(), | |
| ) | |
| })?; | |
| let compute = storage_removes_compute( | |
| &config.wasm_config.ext_costs, | |
| remove_result.gas_key_nonce_count, | |
| remove_result.gas_key_nonce_total_key_bytes, | |
| nonce_value_bytes, |
| ext.compute_cost(ExtCosts::storage_remove_base) * count | ||
| + ext.compute_cost(ExtCosts::storage_remove_key_byte) * total_key_bytes | ||
| + ext.compute_cost(ExtCosts::storage_remove_ret_value_byte) * total_value_bytes |
There was a problem hiding this comment.
The arithmetic operations in storage_removes_compute should use checked arithmetic for consistency with similar functions like storage_read_gas and storage_write_gas. These functions use checked_mul().unwrap() and checked_add().unwrap() to catch potential overflows during development. Without checked arithmetic, silent integer overflow could lead to incorrect compute cost calculations.
| ext.compute_cost(ExtCosts::storage_remove_base) * count | |
| + ext.compute_cost(ExtCosts::storage_remove_key_byte) * total_key_bytes | |
| + ext.compute_cost(ExtCosts::storage_remove_ret_value_byte) * total_value_bytes | |
| ext.compute_cost(ExtCosts::storage_remove_base) | |
| .checked_mul(count) | |
| .unwrap() | |
| .checked_add( | |
| ext.compute_cost(ExtCosts::storage_remove_key_byte) | |
| .checked_mul(total_key_bytes) | |
| .unwrap(), | |
| ) | |
| .unwrap() | |
| .checked_add( | |
| ext.compute_cost(ExtCosts::storage_remove_ret_value_byte) | |
| .checked_mul(total_value_bytes) | |
| .unwrap(), | |
| ) | |
| .unwrap() |
| let nonce_remove_compute = storage_removes_compute( | ||
| &config.wasm_config.ext_costs, | ||
| num_nonces, | ||
| nonce_key_len * num_nonces, | ||
| AccessKey::NONCE_VALUE_LEN * num_nonces, |
There was a problem hiding this comment.
The multiplication nonce_key_len * num_nonces could potentially overflow. For consistency with similar code patterns in the codebase, consider using checked arithmetic (e.g., checked_mul().unwrap() or returning a Result on overflow).
| let nonce_remove_compute = storage_removes_compute( | |
| &config.wasm_config.ext_costs, | |
| num_nonces, | |
| nonce_key_len * num_nonces, | |
| AccessKey::NONCE_VALUE_LEN * num_nonces, | |
| let nonce_keys_total_len = | |
| nonce_key_len.checked_mul(num_nonces).ok_or(IntegerOverflowError)?; | |
| let nonce_values_total_len = | |
| AccessKey::NONCE_VALUE_LEN.checked_mul(num_nonces).ok_or(IntegerOverflowError)?; | |
| let nonce_remove_compute = storage_removes_compute( | |
| &config.wasm_config.ext_costs, | |
| num_nonces, | |
| nonce_keys_total_len, | |
| nonce_values_total_len, |
| let nonce_remove_compute = storage_removes_compute( | ||
| &config.wasm_config.ext_costs, | ||
| num_nonces, | ||
| nonce_key_len * num_nonces, | ||
| AccessKey::NONCE_VALUE_LEN * num_nonces, |
There was a problem hiding this comment.
The multiplication AccessKey::NONCE_VALUE_LEN * num_nonces could potentially overflow. For consistency with similar code patterns in the codebase, consider using checked arithmetic (e.g., checked_mul().unwrap() or returning a Result on overflow).
| let nonce_remove_compute = storage_removes_compute( | |
| &config.wasm_config.ext_costs, | |
| num_nonces, | |
| nonce_key_len * num_nonces, | |
| AccessKey::NONCE_VALUE_LEN * num_nonces, | |
| let total_nonce_key_len = | |
| nonce_key_len.checked_mul(num_nonces).ok_or(IntegerOverflowError)?; | |
| let total_nonce_value_len = | |
| AccessKey::NONCE_VALUE_LEN | |
| .checked_mul(num_nonces) | |
| .ok_or(IntegerOverflowError)?; | |
| let nonce_remove_compute = storage_removes_compute( | |
| &config.wasm_config.ext_costs, | |
| num_nonces, | |
| total_nonce_key_len, | |
| total_nonce_value_len, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## darioush/gaskeys-txfees #15068 +/- ##
===========================================================
- Coverage 68.83% 68.83% -0.01%
===========================================================
Files 921 921
Lines 203760 203868 +108
Branches 203760 203868 +108
===========================================================
+ Hits 140262 140333 +71
- Misses 57515 57520 +5
- Partials 5983 6015 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Charge compute costs when removing gas key nonces during
DeleteKeyandDeleteAccountactions.storage_removes_computehelper computingstorage_remove_{base,key_byte,ret_value_byte}costs fromExtCostsDeleteKeyon a gas key: charges per-nonce compute based on exactgas_key_nonce_key_lenandNONCE_VALUE_LENDeleteAccount:remove_accountnow returnsRemoveAccountResultwith nonce count and total key bytes collected during iteration, used in runtime to charge per-byte compute