This repository has been archived by the owner on Oct 28, 2021. It is now read-only.
Constantinople SSTORE refund fix #5316
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #5316 +/- ##
==========================================
- Coverage 61.87% 61.45% -0.42%
==========================================
Files 342 342
Lines 28063 28062 -1
Branches 3238 3238
==========================================
- Hits 17363 17245 -118
- Misses 9561 9670 +109
- Partials 1139 1147 +8 |
gumb0
reviewed
Oct 15, 2018
| @@ -90,7 +90,7 @@ struct SubState | |||
| { | |||
| std::set<Address> suicides; ///< Any accounts that have suicided. | |||
| LogEntries logs; ///< Any logs. | |||
| u256 refunds; ///< Refund counter for storage changes. | |||
| int64_t refunds = 0; ///< Refund counter for storage changes. | |||
There was a problem hiding this comment.
Wouldn't it be safer to use signed bigint?
There was a problem hiding this comment.
Yes, but if 63 bits are not enough I'd like to constructs a test for this case.
There was a problem hiding this comment.
Ok, you would need 3*10^13 millions gas in a transaction.
There was a problem hiding this comment.
Yeah. For this counter, geth uses u64, and parity uses i128 (which is basically signed u64). If we make every opcode give maximum amount of refund as we currently have, it would still take decades of execution for that to ever overflow.
There was a problem hiding this comment.
Thanks for clarification @sorpaas.
gumb0
approved these changes
Oct 15, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @sorpaas.
There are 2 commits: the first one cleans up some code to make the fix in the second commit more obvious.