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

Improve gas efficiency in ECDSA #3853

Merged
merged 13 commits into from
Feb 8, 2023
Merged

Improve gas efficiency in ECDSA #3853

merged 13 commits into from
Feb 8, 2023

Conversation

TheGreatHB
Copy link
Contributor

@TheGreatHB TheGreatHB commented Dec 4, 2022 •
edited
Loading

Fixes #3853

This PR reduces gas usage of toTypedDataHash toEthSignedMessageHash(bytes32) toEthSignedMessageHash(bytes).

Here are some results:

[toTypedDataHash]

Input                                                                          | Current | New  | Gas saved
-------------------------------------------------------------------------------|---------|------|-----------------
0x6afaff4708d235e0fbf2e9ecd1fbfeb0326cd4801f43d3ca2c0e52d8c2967801 ,           | 198     | 126  | -72   (-36.36%)
0x07785110ea31d256c912208a2875b68e6db3099b5b1d9b3e3035a03fc513f5fa             |         |      |  

[toEthSignedMessageHash(bytes32)]

Input                                                                          | Current | New  | Gas saved
-------------------------------------------------------------------------------|---------|------|-----------------
0x6afaff4708d235e0fbf2e9ecd1fbfeb0326cd4801f43d3ca2c0e52d8c2967801             | 166     | 73   | -93   (-56.02%)

[toEthSignedMessageHash(bytes)]

Input                                                                          | Current | New  | Gas saved
-------------------------------------------------------------------------------|---------|------|-----------------
0x (0 bytes)                                                                   | 971     | 706  | -265  (-27.29%)
                                                                               |         |      | 
0xd6 (1 bytes)                                                                 | 1038    | 773  | -265  (-25.53%)
                                                                               |         |      | 
0xdd66 (2 bytes)                                                               | 1038    | 773  | -265  (-25.53%)
                                                                               |         |      | 
0xb41fcf9e (4 bytes)                                                           | 1038    | 773  | -265  (-25.53%)
                                                                               |         |      | 
0x096739f04d2a1f8f (8 bytes)                                                   | 1047    | 779  | -268  (-25.60%)
                                                                               |         |      | 
0x9e9c1d8a527a7a2c96478f2cd656e80c (16 bytes)                                  | 1124    | 856  | -268  (-23.84%)
                                                                               |         |      | 
0xc561a711bf976ba7ce3e2d4a9de2910c29343df56787ddb6580bde1d26e5250a (32 bytes)  | 1124    | 856  | -268  (-23.84%)
                                                                               |         |      | 
0xaa6a93e1d641813798556c75d1f94b8889fe7b85dc20583f7dc4b3e1a5a296eed69dec40e4be4| 1200    | 932  | -268  (-22.33%)
d25ad4445528eec261b15ee7fcb184f910d8c497403a12e22f3 (64 bytes)                 |         |      | 
                                                                               |         |      | 
0xf127b0025a8bef80a123dd132d39eca568ee23a65790e88b10e1b63e78c898331e48d28f771d6| 1439    | 1171 | -268  (-18.62%)
6bb0acb92bf504474b78c11ff1a17608c98ad010d8f4958d4bbd1d1bb5d75106d79fd1129c4ec28|         |      | 
4a95b2b5363b6dfc39cc3ebf2f4b456e172e434fd5051878276f7ca21254af6572c20b3c9a7f72a|         |      | 
8ba8fc83d5f6fb2c24ef6 (128 bytes)                                              |         |      | 
                                                                               |         |      | 
0xf96785d724671d6026400cf52bd054817d8fcb44a6ada4d049f6a2688765ac2180da016c2a6b0| 1744    | 1476 | -268  (-15.37%)
b0a4a9ac6c0ae5f7e567e7ff3a8602390fd6857894fafc9d5ebd6ad58461f6ddf9efbbb15e59c8f|         |      | 
adbf78ceed91b1042846f6aa0505d08ba78655f9d5863346d2baf4a914a0f13baf64459d3481c0f|         |      | 
9f435a7e806cdadd5ce9c6ad7d84608b76994e103dc8c8e893dad51b785de9591116b3638beefaa|         |      | 
d3d13b3580a61037b2e4ed9c054119fac5fa77eeec65ed6eb25e1fd82f7a92ba41af6f932416ebd|         |      | 
a9285902f8b340a169d471d264080ec9f17dd76bd3173c33a19bf4c684b4c58d865bc0b74f5c159|         |      | 
9f2580cd3ac24ff8fc6067abfabc2df1b1f4e98a (256 bytes)                           |         |      | 
                                                                               |         |      | 
0xaa....bb (512 bytes)                                                         | 2354    | 2086 |  -268  (-11.38%)
                                                                               |         |      | 
0xcc....dd (1024 bytes)                                                        | 3652    | 3384 |  -268  (-7.34% )
                                                                               |         |      | 
0xee....ff (2048 bytes)                                                        | 6104    | 5835 |  -269  (-4.41% )
                                                                               |         |      | 
0x00....11 (4096 bytes)                                                        | 11043   | 10773|  -270  (-2.44% )
                                                                               |         |      |

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@TheGreatHB TheGreatHB changed the title Ecdsa improve gas efficiency ECDSA improve gas efficiency Dec 4, 2022
@TheGreatHB TheGreatHB changed the title ECDSA improve gas efficiency Improve gas efficiency in ECDSA Dec 4, 2022
@k06a
Copy link
Contributor

k06a commented Dec 6, 2022

I agree that toEthSignedMessageHash and toTypedDataHash functions are probably the most efficient implementations written in Solidity Assembly, btw we are using the same in @1inch: https://github.com/1inch/solidity-utils/blob/95bc66ff4267ae112f97b8296c2515a6dd999449/contracts/libraries/ECDSA.sol#L260-L282

frangio
frangio requested changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks!

contracts/utils/cryptography/ECDSA.sol Outdated Show resolved Hide resolved
contracts/utils/cryptography/ECDSA.sol Show resolved Hide resolved
contracts/utils/cryptography/ECDSA.sol Outdated Show resolved Hide resolved
@TheGreatHB
Copy link
Contributor Author

I agree that toEthSignedMessageHash and toTypedDataHash functions are probably the most efficient implementations written in Solidity Assembly, btw we are using the same in @1inch: https://github.com/1inch/solidity-utils/blob/95bc66ff4267ae112f97b8296c2515a6dd999449/contracts/libraries/ECDSA.sol#L260-L282

I've never seen it before, but it's a good. Thank you letting me know about your good work.

@TheGreatHB TheGreatHB requested a review from frangio December 7, 2022 11:16
@TheGreatHB
Copy link
Contributor Author

@frangio Hello? Should I do anything more for merging?

Amxx
Amxx reviewed Jan 30, 2023
View reviewed changes
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

IMO this is mostly good. Should target 4.9

contracts/utils/cryptography/ECDSA.sol Outdated Show resolved Hide resolved
contracts/utils/cryptography/ECDSA.sol Show resolved Hide resolved
contracts/utils/cryptography/ECDSA.sol Outdated Show resolved Hide resolved
@changeset-bot
Copy link

changeset-bot bot commented Jan 30, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: c24ead1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

TheGreatHB and others added 2 commits February 4, 2023 17:02
@TheGreatHB
Update ECDSA.sol
4f54d4e
@TheGreatHB @Amxx
Apply suggestions from code review
77970f8 
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Amxx
Amxx reviewed Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Not sure why we have changes to lib/forge-std in this PR.

@TheGreatHB
Copy link
Contributor Author

Not sure why we have changes to lib/forge-std in this PR.

Maybe this PR was created before #3885?

@pcaversaccio @Amxx
Update forge-std submodule to v1.2.0 (OpenZeppelin#3885)
c24ead1 
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@Amxx
Copy link
Collaborator

Amxx commented Feb 6, 2023

Not sure why we have changes to lib/forge-std in this PR.

Maybe this PR was created before #3885?

at some point it was "discarded" during a merge of master into this. I cherry picked the corresponding commit to fix things.

Amxx
Amxx approved these changes Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

LGTM

frangio
frangio approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Very nice thank you!

@frangio frangio merged commit 8177c46 into OpenZeppelin:master Feb 8, 2023
@gitpoap-bot
Copy link

gitpoap-bot bot commented Feb 8, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 OpenZeppelin Contracts Contributor:

GitPOAP: 2023 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

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

@Amxx Amxx Amxx approved these changes

@frangio frangio frangio 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.
5 participants
@TheGreatHB @k06a @Amxx @frangio @pcaversaccio