Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(contracts): specify gas cost in SemaphoreVerifier precompile calls #883

Merged
merged 3 commits into from
Oct 31, 2024
Merged

refactor(contracts): specify gas cost in SemaphoreVerifier precompile calls #883

merged 3 commits into from
Oct 31, 2024

Conversation

jimmychu0807
Copy link
Contributor

@jimmychu0807 jimmychu0807 commented Oct 23, 2024 •
edited
Loading

Description

Close #871

Other Info

Original gas usage:

·-----------------------------------------------|---------------------------|-------------|-----------------------------·
|             Solc version: 0.8.23              ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
················································|···························|·············|······························
|  Methods                                                                                                              │
··············|·································|·············|·············|·············|···············|··············
|  Contract   ·  Method                         ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  acceptGroupAdmin               ·          -  ·          -  ·      29066  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  addMember                      ·     117970  ·     157140  ·     133880  ·           15  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  addMembers                     ·     222218  ·     302814  ·     288637  ·            6  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  createGroup                    ·      54745  ·      91757  ·      82504  ·            4  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  createGroup                    ·          -  ·          -  ·      91038  ·            3  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  createGroup                    ·      74496  ·      91596  ·      90281  ·           26  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  removeMember                   ·          -  ·          -  ·     128981  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  updateGroupAdmin               ·          -  ·          -  ·      48343  ·            4  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  updateGroupMerkleTreeDuration  ·          -  ·          -  ·      30748  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  updateMember                   ·          -  ·          -  ·     212377  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  validateProof                  ·          -  ·          -  ·     290144  ·            5  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Deployments                                  ·                                         ·  % of limit   ·             │
················································|·············|·············|·············|···············|··············
|  PoseidonT3                                   ·          -  ·          -  ·    3693362  ·       12.3 %  ·          -  │
················································|·············|·············|·············|···············|··············
|  Semaphore                                    ·          -  ·          -  ·    1820135  ·        6.1 %  ·          -  │
················································|·············|·············|·············|···············|··············
|  SemaphoreVerifier                            ·          -  ·          -  ·    3722864  ·       12.4 %  ·          -  │
·-----------------------------------------------|-------------|-------------|-------------|---------------|-------------·

With this PR (last updated 2024-10-31):

·-----------------------------------------------|---------------------------|-------------|-----------------------------·
|             Solc version: 0.8.23              ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
················································|···························|·············|······························
|  Methods                                                                                                              │
··············|·································|·············|·············|·············|···············|··············
|  Contract   ·  Method                         ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  acceptGroupAdmin               ·          -  ·          -  ·      29066  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  addMember                      ·     117970  ·     157140  ·     133880  ·           15  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  addMembers                     ·     222218  ·     302814  ·     288637  ·            6  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  createGroup                    ·      54745  ·      91757  ·      82504  ·            4  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  createGroup                    ·          -  ·          -  ·      91038  ·            3  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  createGroup                    ·      74496  ·      91596  ·      90281  ·           26  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  removeMember                   ·          -  ·          -  ·     128981  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  updateGroupAdmin               ·          -  ·          -  ·      48343  ·            4  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  updateGroupMerkleTreeDuration  ·          -  ·          -  ·      30748  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  updateMember                   ·          -  ·          -  ·     212377  ·            2  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Semaphore  ·  validateProof                  ·          -  ·          -  ·     290063  ·            5  ·          -  │
··············|·································|·············|·············|·············|···············|··············
|  Deployments                                  ·                                         ·  % of limit   ·             │
················································|·············|·············|·············|···············|··············
|  PoseidonT3                                   ·          -  ·          -  ·    3693362  ·       12.3 %  ·          -  │
················································|·············|·············|·············|···············|··············
|  Semaphore                                    ·          -  ·          -  ·    1820135  ·        6.1 %  ·          -  │
················································|·············|·············|·············|···············|··············
|  SemaphoreVerifier                            ·          -  ·          -  ·    3721580  ·       12.4 %  ·          -  │
·-----------------------------------------------|-------------|-------------|-------------|---------------|-------------·

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have run yarn format and yarn lint without getting any errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@jimmychu0807 jimmychu0807 requested a review from a team as a code owner October 23, 2024 14:40
@openzeppelin-code OpenZeppelin Code
Copy link

openzeppelin-code bot commented Oct 23, 2024 •
edited
Loading

refactor(contracts): specify gas cost in SemaphoreVerifier precompile calls

Generated at commit: 04a80924f7c7fe3ec24e6e16181bbb86afb7af71

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total		 0
0
0
4
15
19
Dependencies Critical
High
Medium
Low
Note
Total		 0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@cedoor cedoor mentioned this pull request Oct 23, 2024
Closed
@jimmychu0807 jimmychu0807 marked this pull request as draft October 24, 2024 03:07
@jimmychu0807
Copy link
Contributor Author

jimmychu0807 commented Oct 27, 2024 •
edited
Loading

@chancehudson @cedoor
After some experimenting, I decide to code on the defensive side and specify the gas cost in the precompile calls (@chancehudson has suggested that as well).

Based on EVM code reference (https://www.evm.codes/precompiled), 0x07 ecMul is fixed to use 6000 gas, and 0x06 ecAdd is fixed to use 150 gas when inputs are valid. If inputs are invalid, gas are consumed and the following check iszero(success) will become true and return.

For the 0x08 ecPairing gas cost, it used to be 80000*k + 100000, where k is the number of points or, equivalently, the length of the input divided by 192. But in the latest EVM it seems to become 34000*k + 45000, this is also confirmed in the above EVM code reference after you specify the input byte size.

Screenshot 2024-10-27 at 14 05 58

This change is very EVM implementation-specific, but as EVM iterates I believe the gas cost will likely only decrease and not increase, so specifying the max gas cost to use should be fine.

@jimmychu0807 jimmychu0807 marked this pull request as ready for review October 27, 2024 06:23
@jimmychu0807
Copy link
Contributor Author

On a side note, I have this experimenting code here:
https://github.com/jimmychu0807/semaphore/blob/jc%2Fplayground/packages/contracts/contracts/base/SemaphoreVerifier.sol#L38-L117

Even if I pass in invalid input parameters to ecAdd, ecMul, and ecPairing calls, or pass in insufficient gas to these calls, they won't revert. They will just fail and have success returns 0, which is good.

But I am not sure if this is hardhat node implementation specific or is part of the specification, so erring on the defensive side.

@jimmychu0807 jimmychu0807 changed the title chore(contracts): remove sub() from gas() in staticcall at SemaphoreVerifier refactor(contracts): remove sub() from gas() in staticcall at SemaphoreVerifier Oct 27, 2024
@jimmychu0807 jimmychu0807 changed the title refactor(contracts): remove sub() from gas() in staticcall at SemaphoreVerifier refactor(contracts): specify gas cost in SemaphoreVerifier precompile calls Oct 27, 2024
@cedoor
Copy link
Member

cedoor commented Oct 28, 2024

@jimmychu0807 Thanks for this PR, it looks like the best solution to me.

This change is very EVM implementation-specific, but as EVM iterates I believe the gas cost will likely only decrease and not increase, so specifying the max gas cost to use should be fine.

I'm not sure about this. I'd like to know what @chancehudson thinks about it.

Also a review from @0xDatapunk and @thogiti would be great!

@jimmychu0807
Copy link
Contributor Author

Thanks 0xDatapunk for the review. @chancehudson @thogiti if you have time these few days could you take a look at this PR. The code change is just 3 lines, changing from gas() to a fixed gas cost. Please take a look at the discussion above on my reasoning and see if this makes sense to you. Thank you for your time!

@chancehudson
Copy link
Contributor

chancehudson commented Oct 31, 2024 •
edited
Loading

I'd recommend sending ~20% extra gas with each call - I've seen other similar cases that did this. The sent gas will simply be refunded so it should not change the final price of the transaction.

Hardcoding gas numbers like this is very brittle so safety measures should be taken. The calculated gas numbers look correct though. This change should include a note to dependent packages too noting the risk here. If a contract is deployed as immutable it's possible it gets permanently and unavoidably broken in the future by changes to precompile pricing.

@jimmychu0807
Copy link
Contributor Author

This makes sense to me. Should add some extra gas as buffer.

@jimmychu0807
Copy link
Contributor Author

jimmychu0807 commented Oct 31, 2024 •
edited
Loading

Increased gas cost with safety buffer of 33.3%, just want to reserve a bit wider margin. Gas report generated by yarn test:gas-report is updated at the PR desc, still less than before the PR.

@thogiti
Copy link

thogiti commented Oct 31, 2024

Looks good.

Please add more detailed documentation on these changes.

Please make sure to update the fixed gas costs numbers for any future updates.

cedoor
cedoor approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@cedoor cedoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's ready to be merged!

Thanks @jimmychu0807 🙏🏽

@cedoor cedoor merged commit 901d095 into semaphore-protocol:main Oct 31, 2024
5 checks passed
@jimmychu0807 jimmychu0807 deleted the jc/dev branch October 31, 2024 12:57
@jimmychu0807
Copy link
Contributor Author

thank you @chancehudson @thogiti @0xDatapunk for the input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cedoor cedoor cedoor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove sub() from gas() in SemaphoreVerifier.sol
4 participants
@jimmychu0807 @cedoor @chancehudson @thogiti