Optimize gas fee

[huyhuynh3103]

Issue: #399: Optimize gas fee for deregisterProvingKey function in VRFCoordinator contract

Change description:

• [1] Cache length of s_provingKeyHashes array to memory before running for loop.
• [2] Default value of uint type is 0 so we don't need to assign it value to 0 again.
• [3] Proving key hashes will be pushed by only contract owner so it can't be overflow 256 bit. So we can add ++i inside unchecked keyword

[martinkersner]

Hello @huyhuynh3103

Thank you for your contribution!

As you probably know the length of s_provingKeyHashes is not expected to be very large, and the gas savings will be proportional to the number of keys in the array. We need to balance the optimal gas usage together with a good code readability.

Did you compute the gas savings for this particular fix?

1. What is the gas savings for caching storage variable s_provingKeyHashes compared to repeated reading of its length?
2. What is the gas savings of unchecked iterator increment?

I believe that the first proposal would be good to include, but I am more hesitant about the second one. I do not expect the gas saving be significant while the code readability suffers from the second one.

I am looking forward to reading your answer!

Cheers,
Martin

[huyhuynh3103]

Hello @martinkersner
I calculate the number of gas that the iterator used in this way:

 uint gas = gasleft();
 // for loop code
 // ...
 gas -= gasleft();
 emit GasUsed(gas);

I have tested on an array with 4 elements
Procedure:

1. Execute registerProvingKey 4 times to push 4 key hashes into s_provingKeyHashes
2. Execute deregisterProvingKey with the last proving key

Gas used for each test case is:

• before fix: 19105
• cache length of array: 18792
• unchecked iterator increments: 18730
• combine two of optimized gas way: 18417

I believe my proposal will reduce gas significantly if s_provingKeyHashes grows very large.

[martinkersner]

@huyhuynh3103 Thank you for the explanation!

If I understand it correctly, you are testing the worst case scenario when we need to remove the item located at the end of the array. To generalize the gas savings per iteration, we would end up with following

• (19,105-18,792) / 4 = 78.25 gas by caching length array
• (19,105 - 18,730) / 4 = 93.75 gas by unchecking iterator increments

(The gas values are in floating point because you did not deduct gas used for your gas measurement.)

As you have mentioned in your comment, the optimization reduce gas significantly if s_provingKeyHashes grows very large. The problem with this assumption is that it will never grow large, there is no reason for it. The number of items in the array will be most likely always kept in low single digits, and it will not be executed often.

I would like to suggest to keep only the optimization for caching length of array, and please format your code according to standards in this repository. After those changes, I am going to merge it.

[huyhuynh3103]

@martinkersner, I have just updated upon your suggestion. Please review my changes again.

[martinkersner]

@huyhuynh3103 Please fix the code formatting.

[huyhuynh3103]

@martinkersner I've done

[martinkersner]

LGTM!