-
Notifications
You must be signed in to change notification settings - Fork 2.4k
refactor(gateway-contracts): remove redundant decryptionDone storage for public decryption #1532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor(gateway-contracts): remove redundant decryptionDone storage for public decryption #1532
Conversation
…for public decryption Optimize public decryption by removing redundant storage write. Changes: - Renamed decryptionDone to userDecryptionDone (only used for user decryption) - Modified publicDecryptionResponse() to check decryptionConsensusDigest instead of decryptionDone - Removed redundant SSTORE in publicDecryptionResponse() (~20k gas savings per request) - Updated isDecryptionDone() with type-aware logic based on decryption ID range - Public decryption now checks if decryptionConsensusDigest is set - User decryption continues using userDecryptionDone mapping Close zama-ai/fhevm-internal#751
🧪 CI InsightsHere's what we observed from your CI run for 5846288. 🟢 All jobs passed!But CI Insights is watching 👀 |
ClementWalter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| /// @notice Whether a (public, user, delegated user) decryption is done | ||
| mapping(uint256 decryptionId => bool decryptionDone) decryptionDone; | ||
| /// @notice Whether a (user, delegated user) decryption is done | ||
| mapping(uint256 decryptionId => bool userDecryptionDone) userDecryptionDone; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that, unfortunately, we can't rename state variables, but deprecate them and add the new ones at the end of the storage struct. This is because of upgrade conflicts between storage layouts. Also, we must bump the contract and the reinitializer versions in order to properly upgrade the contract once the change is introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok cool thanks for raising that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we must bump the contract and the reinitializer versions in order to properly upgrade the contract once the change is introduced.
what does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means:
The second one is relevant for upgrading the contract, so the reinitializerVX method can be called with the correct versioning. For example, on testnet we have deployed now v0.10.x with reinitializer version 4, so when we need to upgrade to v0.11.x we should have this reinitializer version incremented to 5 in order to properly call the reinitializerV4() method.
Summary
This PR optimizes public decryption by removing a redundant storage write in the
Decryptioncontract, saving approximately 20,000 gas per public decryption request.Changes
Storage Optimization
decryptionDonetouserDecryptionDone: The mapping is now scoped to only user/delegated user decryption, where it's actually neededpublicDecryptionResponse(): Public decryption consensus is now tracked solely throughdecryptionConsensusDigest, eliminating an unnecessary storage writeGas Impact
decryptionConsensusDigestwhich is already read/written in the same functionCompatibility
isDecryptionDone()works identically for both typesisDecryptionDone()which maintains the same behavior, not the internal mappingClose https://github.com/zama-ai/fhevm-internal/issues/751