Skip to content

Improve gas efficiency in ECDSA#3853

Merged
frangio merged 13 commits intoOpenZeppelin:masterfrom
TheGreatHB:ECDSA-improve-gas-efficiency
Feb 8, 2023
Merged

Improve gas efficiency in ECDSA#3853
frangio merged 13 commits intoOpenZeppelin:masterfrom
TheGreatHB:ECDSA-improve-gas-efficiency

Conversation

@TheGreatHB
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

Comment thread contracts/utils/cryptography/ECDSA.sol Outdated
Comment thread contracts/utils/cryptography/ECDSA.sol
Comment thread contracts/utils/cryptography/ECDSA.sol Outdated
@TheGreatHB
Copy link
Copy Markdown
Contributor Author

TheGreatHB 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

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
Copy Markdown
Contributor Author

TheGreatHB commented Jan 29, 2023

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

Amxx
Amxx reviewed Jan 30, 2023
View reviewed changes
Copy link
Copy Markdown
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

Comment thread contracts/utils/cryptography/ECDSA.sol
Comment thread contracts/utils/cryptography/ECDSA.sol
Comment thread contracts/utils/cryptography/ECDSA.sol Outdated
@changeset-bot
Copy link
Copy Markdown

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

Amxx
Amxx reviewed Feb 6, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

TheGreatHB commented Feb 6, 2023

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

Maybe this PR was created before #3885?

@Amxx
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@frangio frangio frangio approved these changes

@Amxx Amxx Amxx 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