VRF-878 Gas Optimization V2 Plus #11982
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
| return false; | ||
| } | ||
| uint256 provingKeyHashesLength = s_provingKeyHashes.length; | ||
| if (provingKeyHashesLength == 0) { |
There was a problem hiding this comment.
we can assume contract always has proving keys so i think we can remove this check?
There was a problem hiding this comment.
I removed this check and the gas increased. I'm not sure if it's because of the way our tests are set up. Maybe it's testing for cases where there are no proving keys.
There was a problem hiding this comment.
This method is called from "CancelSubscription" and cancel subscription tests do not set up proving keys. So yeah, I think it's because most test cases that we have don't set up proving keys unless it's testing request/fulfillment flow
There was a problem hiding this comment.
Can we assume that the hot path will always have at least 1 consumer and at least 1 proving key? If that's the case I will remove both checks here. Note that this function is called by removeConsumer. cancelSubscription, and migrate.
The proving key length check is to prevent address consumer = consumers[i] in line 593 from being called if the inner loop wouldn't even be executed and the consumers length check is to prevent accessing s_provingKeyHashes storage in line 588 when the outer loop wouldn't be executed.
There was a problem hiding this comment.
we can assume there is always at least one proving key hash. When removeConsumer, cancelSubscription, migrate are called, consumers is more likely to be present but not always, so I think removing both checks make sense.
Can we assume that the hot path will always have at least 1 consumer and at least 1 proving key?
Hot path (request/fulfillment) won't be calling this method right? so optimization here is scoped to the functions you mentioned: removeConsumer, cancelSubscription, and migrate.
There was a problem hiding this comment.
Yea, I meant the hot path of removeConsumer, cancelSubscription, and migrate, and NOT the hot path of VRF as a whole.
| @@ -324,13 +326,14 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr | |||
| function getActiveSubscriptionIds( | |||
| uint256 startIndex, | |||
| uint256 maxCount | |||
| ) external view override returns (uint256[] memory) { | |||
| ) external view override returns (uint256[] memory ids) { | |||
There was a problem hiding this comment.
Just curious: So if you don't use named return variables, there's an additional cost (like an additional temp var copy operation)? What happens in the background if you don't use this approach?
There was a problem hiding this comment.
I am guessing that there will be an additional temp var copy operation. Not sure, will have to dive into the opcodes generated.
| @@ -379,13 +384,13 @@ abstract contract SubscriptionAPI is ConfirmedOwner, IERC677Receiver, IVRFSubscr | |||
| * @inheritdoc IVRFSubscriptionV2Plus | |||
| */ | |||
| function acceptSubscriptionOwnerTransfer(uint256 subId) external override nonReentrant { | |||
| if (s_subscriptionConfigs[subId].owner == address(0)) { | |||
| address oldOwner = s_subscriptionConfigs[subId].owner; | |||
| if (oldOwner == address(0)) { | |||
There was a problem hiding this comment.
Let's say we define a constant value called address ZERO_ADDRESS = address(0) somewhere in some shared file (of course, that increases the contract size a bit). In that case, we replace each occurrence of address(0) with the constant's name. Now, this value is re-used within all function calls that require it and no temp var is created just for the sake of the condition check. From the Solidity perspective, would that approach be better or worse and why? Again, I'm just curious and trying to learn a thing or two about contract optimization techniques.
There was a problem hiding this comment.
Seems like there will be no difference as the compiler would replace each constant values with the actual values wherever it is used. Gas cost for ISZERO and EQ are both 3.
These questions are good. Keep them coming. I wouldn't say I'm an expert in contract optimisation. I'm just tweaking things around and see which actually saves gas. But I do love to optimise things.
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* develop: (74 commits) VRF zero confirmation delay (#11947) add toml configs to paths that can cause e2e tests to run in ci (#12001) bump golang.org/x/... (#12042) [chore] Replace clock with specialized lib (#12031) Update style guide (#12041) plugins/cmd/chainlink-mercury: (re)move to chainlink-data-streams repo (#11994) bump go-plugin (#12033) Adds timeout on fuzz script execution (#12024) Add bytes type to abi_type (#12029) AUTO-8804: create chain specific modules for l1 gas calculations (#11896) VRF-878 Gas Optimization V2 Plus (#11982) Improving deletes performance by limiting number of records to scan (#12007) core/web: improve health CLI readabilty (#12021) Handle a 0 exit code from the remote runner instead of always failing (#12015) Add a simple Codec test (#12006) Allow for custom config poller onchain codec (LLO support) (#11957) Update Sonar properties (#11986) golangci-lint: revive: add early-return; fix issues (#12017) Implement NewPluginProvider (EVM) (#11995) Fix lock file version and minor NPM bumps (#11980) ...
* Optimize deregisterProvingKey * Optimize fulfillRandomWords * Optimize deregisterMigratableCoordinator * Optimize _isTargetRegistered * Optimize pendingRequestExists * Optimize _deleteSubscription * Optimize getActiveSubscriptionIds * Optimize requestRandomWords * Replace post-increment with pre-increment * Optimize _getFeedData * Optimize ownerCancelSubscription * Optimize getSubscription * Optimize createSubscription * Optimize requestSubscriptionOwnerTransfer * Optimize acceptSubscriptionOwnerTransfer * Optimize addConsumer * Update geth wrappers * Remove proving keys length check in pendingRequestExists
* VRF V2.5 gas optimisations (#11932) * VRF V2.5 gas optimisations * Minor changes * Removed changes in SubscriptionAPI.sol due to no actual gain in hot paths * Minor changes * VRF-878 Gas Optimization V2 Plus (#11982) * Optimize deregisterProvingKey * Optimize fulfillRandomWords * Optimize deregisterMigratableCoordinator * Optimize _isTargetRegistered * Optimize pendingRequestExists * Optimize _deleteSubscription * Optimize getActiveSubscriptionIds * Optimize requestRandomWords * Replace post-increment with pre-increment * Optimize _getFeedData * Optimize ownerCancelSubscription * Optimize getSubscription * Optimize createSubscription * Optimize requestSubscriptionOwnerTransfer * Optimize acceptSubscriptionOwnerTransfer * Optimize addConsumer * Update geth wrappers * Remove proving keys length check in pendingRequestExists * Add native payment to RandomWordsFulfilled event (#12085) * Add native payment to RandomWordsFulfilled event * Minor change --------- Co-authored-by: Sri Kidambi <[email protected]> --------- Co-authored-by: Sri Kidambi <[email protected]> Co-authored-by: Lee Yik Jiun <[email protected]> Co-authored-by: Lee Yik Jiun <[email protected]>
* Optimize deregisterProvingKey * Optimize fulfillRandomWords * Optimize deregisterMigratableCoordinator * Optimize _isTargetRegistered * Optimize pendingRequestExists * Optimize _deleteSubscription * Optimize getActiveSubscriptionIds * Optimize requestRandomWords * Replace post-increment with pre-increment * Optimize _getFeedData * Optimize ownerCancelSubscription * Optimize getSubscription * Optimize createSubscription * Optimize requestSubscriptionOwnerTransfer * Optimize acceptSubscriptionOwnerTransfer * Optimize addConsumer * Update geth wrappers * Remove proving keys length check in pendingRequestExists
* Optimize deregisterProvingKey * Optimize fulfillRandomWords * Optimize deregisterMigratableCoordinator * Optimize _isTargetRegistered * Optimize pendingRequestExists * Optimize _deleteSubscription * Optimize getActiveSubscriptionIds * Optimize requestRandomWords * Replace post-increment with pre-increment * Optimize _getFeedData * Optimize ownerCancelSubscription * Optimize getSubscription * Optimize createSubscription * Optimize requestSubscriptionOwnerTransfer * Optimize acceptSubscriptionOwnerTransfer * Optimize addConsumer * Update geth wrappers * Remove proving keys length check in pendingRequestExists
Optimise gas for
VRFCoordinatorV2_5.solandSubscriptionAPI.solGas changes per test compared with
developDepending on test set up, it may appear that more gas is reduced than actual.
Gas changes per function compared with
developfulfillRandomWordsgas reduced by around 140 to 190.requestRandomWordsgas reduced by around 130 to 150.There are opportunities to reduce gas usage in
removeConsumeras well but didn't do so as there appears to be insufficient test coverage.EDIT: Before removing proving keys length check
Gas changes per function compared with
developfulfillRandomWordsgas reduced by around 140 to 190.requestRandomWordsgas reduced by around 130 to 150.