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

Update ReentrancyGuard for Istanbul Hard Fork #1992

Merged
merged 4 commits into from
Nov 13, 2019

Conversation

ericDeCourcy
Copy link
Contributor

@ericDeCourcy ericDeCourcy commented Nov 12, 2019

Changes:
Added L37, _guardCounter = 1;

Rationale:
The planned Istanbul Hard Fork will implement EIP 2200, which implements "net gas metering" for sstore operations. If the final value of _guardCounter is unchanged relative to the original value of it, a gas refund will be applied and charges for changing the value of _guardCounter will effectively not exist. This ends up being cheaper than the current implementation ONLY AFTER Istanbul. Before Istanbul, the added line actually ends up costing more gas.

Note that if _guardCounter is 0 initially, the initial cost and subsequent refund will both be larger than if _guardCounter is 1 initially. Although in both cases, the net gas cost (gasCost - gasRefund) are equal, it's better in terms of cost to have both the gas cost and refund smaller, as there is some limit to the percentage of a gas refund that can actually be realized.

Changes:
Added L37, `_guardCounter = 1;`

Rationale:
The planned _Istanbul Hard Fork_ will implement [EIP 2200](https://github.com/ethereum/EIPs/blob/e4d4ea348e06c54d0075c400dc7b72430d427ff1/EIPS/eip-2200.md), which implements "net gas metering" for `sstore` operations. If the final value of `_guardCounter` is unchanged relative to the original value of it, a gas refund will be applied and charges for changing the value of `_guardCounter` will effectively not exist. This ends up being cheaper than the current implementation ONLY AFTER Istanbul. Before Istanbul, the added line actually ends up costing more gas.

Note that if `_guardCounter` is `0` initially, the initial cost and subsequent refund will both be larger than if `_guardCounter` is `1` initially. Although in both cases, the net gas cost (`gasCost - gasRefund`) are equal, it's better in terms of cost to have both the gas cost and refund smaller, as there is some limit to the percentage of a gas refund that can actually be realized.
@nventuro nventuro added the contracts Smart contract code. label Nov 12, 2019
@nventuro
Copy link
Contributor

Thanks a lot @ericDeCourcy! Could you add an entry for this change on the changelog file?

@nventuro
Copy link
Contributor

Thanks! I took the liberty of changing it up a bit so that the improvements introduced by this PR are more evident by the reader, who may not be aware of the implications of the actual code change.

I also added a note on the documentation for the module, to entice people to try out the new version.

@nventuro nventuro marked this pull request as ready for review November 13, 2019 17:46
@nventuro nventuro merged commit 33047ff into OpenZeppelin:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants