feat(gas-keys): charge compute for gas key nonce removal#15068
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 @@
## master #15068 +/- ##
==========================================
+ Coverage 68.67% 69.10% +0.43%
==========================================
Files 921 921
Lines 204391 204519 +128
Branches 204391 204519 +128
==========================================
+ Hits 140359 141338 +979
+ Misses 58246 57382 -864
- Partials 5786 5799 +13
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:
|
4d3cffa to
d6819cd
Compare
In [NEP-611](https://github.com/near/NEPs/pull/611/files), it is specified: > * Cost of `TransferToGasKey` and `WithdrawFromGasKey` will be based on `Transfer`, as it is the most similar existing action. `WithdrawFromGasKey` and `TransferToGasKey` do additional trie modifications on sending and on execution. These will be accounted for based on trie operations. This PR provides the concrete implementation for this specification. **Send Fees** _TransferToGasKey / WithdrawFromGasKey_: gas_key_transfer_base.send + gas_key_key_byte.send * public_key_len _AddKey (gas key variant) - permission_send_fees_: add_{function_call_key_base,full_access_key}.send + ...method_name_bytes... + gas_key_byte.send * GasKeyInfo::borsh_len() **Exec Fees** _TransferToGasKey / WithdrawFromGasKey_: gas_key_transfer_base.exec + gas_key_byte.exec * ak_key_len + gas_key_byte.exec * estimated_value_len where ak_key_len = access_key_key_len(receiver_id, public_key) and estimated_value_len = AccessKey::min_gas_key_borsh_len(). _AddKey (gas key variant) - permission_exec_fees_: add_{function_call_key_base,full_access_key}.exec + ...method_name_bytes... + num_nonces * (gas_key_nonce_write_base.exec + gas_key_byte.exec * nonce_key_len + gas_key_byte.exec * nonce_value_len) where nonce_key_len = gas_key_nonce_key_len(account_id, public_key) and nonce_value_len = NONCE_VALUE_LEN. **Values (derived from ExtCosts)** gas_key_transfer_base: send=transfer; exec=send+storage_read_base+storage_write_base ──────────────────────────────────────── gas_key_byte: send=new_data_receipt_byte; exec=storage_read_key_byte+storage_write_key_byte ──────────────────────────────────────── gas_key_nonce_write_base: send=0; exec=storage_write_base N = `num_nonces` - DeleteKey & DeleteAccount can only charge variable compute for num nonces, will address in a separate [PR](#15068). - Adds `AccessKey::NONCE_VALUE_LEN`, `min_gas_key_borsh_len()`, and trie key length helpers as shared primitives - Uses `min_gas_key_borsh_len()` as estimated value length where the actual gas key variant is unknown at fee computation time
62f0a71 to
334280d
Compare
334280d to
7c32b23
Compare
# Gas Keys ([NEP-611](near/NEPs#611)) Gas keys are a new type of access key that carries a pre-funded NEAR balance and multiple independent nonce sequences. When a transaction is signed with a gas key, gas costs are deducted from the gas key's balance rather than the signer's account balance (deposits like NEAR transfers still come from the account). ## Why do we need gas keys? - Enabling SPICE without a DoS vector. The SPICE project (Separation of Execution from Consensus) decouples transaction execution from block production: transactions are included in blocks first, and executed asynchronously later. This means chunk producers can no longer verify account balances at inclusion time. An attacker could drain an account's balance in one transaction, then flood the chain with many transactions signed by that account. All would be accepted (since execution hasn't caught up), consuming block space for free when they inevitably fail. Gas keys close this gap by requiring gas to be pre-committed in the key itself, so chunk producers can verify gas availability without access to the latest state. - Parallel transaction submission. Today, programmatic users who submit many transactions in parallel must manage multiple access keys (each with its own nonce sequence). A gas key supports up to 1,024 independent nonce slots via a single key, removing that complexity. # Context - NEP (if exists): near/NEPs#611 - Implementation: - near#14868 - near#14869 - near#14883 - near#14891 - near#14892 - near#14911 - near#14924 - near#14925 - near#14930 - near#14933 - near#14969 - near#14985 - near#15033 - near#15034 - near#15067 - near#15068 - near#15100 - near#15101 - near#15186 # Testing and QA - unit tests - test-loop: https://github.com/near/nearcore/blob/master/test-loop-tests/src/tests/gas_keys.rs - forknet (automated): near#15207 - forknet (manual testing, planned) # Checklist - [x] Link to nightly nayduck run: https://nayduck.nearone.org/#/run/4756 - [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
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