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

reentrancy mutex gas optimization #1155

Merged
merged 2 commits into from Aug 6, 2018

Conversation

@yaronvel
Copy link
Contributor

commented Aug 5, 2018

Implementation of reentrancy mutex that is using only one SSTORE operation.
Expected to reduce the original mutex implementation by half. From roughly 10k gas to 5k gas.

@nventuro nventuro self-assigned this Aug 6, 2018
@nventuro nventuro added this to the v2.0 milestone Aug 6, 2018
Copy link
Member

left a comment

Thanks @yaronvel, I love this implementation! Extremely simple yet effective and cheap.

The gas costs during a revert may be higher when compared to the original implementation, since this will revert after the fact, as opposed to checking before the reentrancy occurs. I think this is a non-issue though, since we should only care about gas costs for the regular case.

*/
uint private reentrancyLock = REENTRANCY_GUARD_FREE;
/// @dev counter to allow mutex lock with only one SSTORE operation
uint private guardCounter = 1;

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 6, 2018

Member

Could you make the actual type explicit with uint256?

@@ -30,10 +21,9 @@ contract ReentrancyGuard {
* wrapper marked as `nonReentrant`.
*/
modifier nonReentrant() {
require(reentrancyLock == REENTRANCY_GUARD_FREE);
reentrancyLock = REENTRANCY_GUARD_LOCKED;
uint localCounter = ++guardCounter;

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 6, 2018

Member

I think this would read better if we split the increment and the assignment, the pre-increment syntax might be a bit too magic for something as sensitive as this.

guardCounter += 1;
uint256 localCounter = guardCounter;
@yaronvel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@nventuro did the changes you asked for.

@frangio
frangio approved these changes Aug 6, 2018
Copy link
Member

left a comment

Very cool @yaronvel!

@nventuro

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

Awesome work @yaronvel, thanks a lot!

@nventuro nventuro merged commit 31ac59b into OpenZeppelin:master Aug 6, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 95.232%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.