perf: avoid double-hashing the asset vault key in fungible add/remove#3073
Open
partylikeits1983 wants to merge 2 commits into
Open
perf: avoid double-hashing the asset vault key in fungible add/remove#3073partylikeits1983 wants to merge 2 commits into
partylikeits1983 wants to merge 2 commits into
Conversation
Hash the raw asset vault key once at the start of each asset vault modifier and have peek_asset/get_asset accept the already-hashed key, removing one poseidon2 hash per fungible asset add/remove (including the fee-removing path in the epilogue). The set_asset helper becomes a bare smt::set wrapper and is removed. Worst-case post-compute_fee cycles drop from 863 to 843; VAULT_KEY_HASH_CYCLES is re-measured from 50 to 30 (same 45-cycle margin). Closes #3059
Comment on lines
+802
to
808
| # hash the asset vault key before using it as the SMT key | ||
| exec.asset_vault::hash_asset_key | ||
| # => [ASSET_KEY_HASH, vault_root_ptr] | ||
|
|
||
| # get the asset | ||
| exec.asset_vault::get_asset | ||
| # => [ASSET_VALUE] |
Contributor
There was a problem hiding this comment.
I would keep key hashing internal to the vault, so that callers of the module's procedure do not have to think about this detail.
So, get_asset stays as it was before the PR (takes ASSET_KEY).
And if we change peek and set asset to this:
#! Inputs: [ASSET_KEY_HASH, vault_root_ptr]
#! Outputs: [ASSET_VALUE]
pub proc peek_asset
#! Inputs: [ASSET_VALUE, ASSET_KEY_HASH, VAULT_ROOT]
#! Outputs: [OLD_VALUE, NEW_VAULT_ROOT]
proc set_asset
Then we should be able to avoid double hashing in add_fungible_asset and remove_fungible_asset as well, right? We should be able to keep hash_asset_key private.
That way, all asset key hashing is done in asset_vault and callers always provide ASSET_KEY uniformly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3059.
add_fungible_assetandremove_fungible_assethashed the raw asset vault key twice: once insidepeek_assetand again insideset_asset. This PR hashes the key once at the start of each vault modifier and changespeek_asset/get_assetto accept the already-hashedASSET_KEY_HASH, removing oneposeidon2::hashper fungible add/remove, including the fee-removing path in the epilogue. Theset_assethelper became a baresmt::setwrapper and is removed;hash_asset_keyis nowpubfor external callers (account.masmget-asset wrappers and test code).Cycle constants
Re-measured via
num_tx_cycles_after_compute_fee_are_less_than_estimated: worst-case post-compute_feecycles dropped from 863 to 843 (one poseidon2 hash, 20 cycles).VAULT_KEY_HASH_CYCLESis lowered from 50 to 30 inepilogue.masmand its mirror intest_fee.rs, keeping the same 45-cycle safety margin. The zero-fee baseline (NUM_POST_COMPUTE_FEE_CYCLES) is unaffected.Testing
make formatandmake testpass locally (1193/1193).