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.

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

ReentrancyGuard gas optimization #1996

Merged
merged 3 commits into from
Nov 23, 2019
Merged

Conversation

nventuro
Copy link
Contributor

This reverts the changes made in #1057 and #1155 in light of the Istanbul hardfork and EIP2200.

#1992 introduced gas savings by taking advantage of the net gas metering changes, but this can be further improved upon by removing unnecessary SLOAD operations. Thanks @jflatow for proposing this change!

@nventuro
Copy link
Contributor Author

The previous version of ReentrancyGuard caused the inner-most re-entrant call to succeed, placing the revert in the immediately outer call (e.g. if there where five re-entries, the revert would happen as the fourth one was exiting).

The new version instead reverts immediately as soon as a re-entrant call is made (arguably allowing for easier understanding of what happened), which is why our suite detected the revert reason is different.

Given that the end result is the same (re-entrant calls are ultimately reverted), I don't think this qualifies as a breaking change.

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.

Agreed on backwards compatibility. LGTM. Thank you!

@nventuro nventuro merged commit 28b95ef into OpenZeppelin:master Nov 23, 2019
@nventuro nventuro deleted the reentrancy-gas branch November 23, 2019 23:44
@0xboredagain
Copy link

is there anywhere you documented the intended gas savings from #1996 ?

@nventuro
Copy link
Contributor Author

As mentioned above, it should reduce the number of SLOADs by one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants