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

Implement eip 3529 #2238

Merged
merged 15 commits into from
May 10, 2021
Merged

Implement eip 3529 #2238

merged 15 commits into from
May 10, 2021

Conversation

matkt
Copy link
Contributor

@matkt matkt commented May 6, 2021

PR description

Implement eip 3529 (version of the spec ethereum/EIPs@6079eba)

Fixed Issue(s)

Changelog

matkt and others added 11 commits May 6, 2021 12:55
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking feedback. Will let you decide to change or merge

}

@Test
public void shouldCalculateGasAccordingToEip3539() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, 3529

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{"0x60016000556000600055", 0, 20112, 19900},
{"0x60016000556002600055", 0, 20112, 0},
{"0x60016000556001600055", 0, 20112, 0},
{"0x60006000556000600055", 1, 3012, 4800},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a good test case for maxRefund <= gasUsed / 5. We don't really cover that case in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no complex logic. Trying to test it I found myself mocking everything. I think it will be more useful to check this part thanks to the reference tests

Comment on lines +41 to +45
// defined in Berlin, but re-implemented with new constants
@Override
// As per https://eips.ethereum.org/EIPS/eip-3529
public Gas calculateStorageRefundAmount(
final Account account, final UInt256 key, final UInt256 newValue) {
Copy link
Contributor

@garyschulte garyschulte May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is identical to the berlin gas calculator, how about just protected accessors for the 2 changed constants? The method is kinda funky to read, that is the only reason I ask

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I wanted to do this but it seems that this is what was also done for Berlin/Istanbul. Can be to improve readability or explained that this method has been updated. To maintain a consistency I wanted to keep that but we can in a future PR change that if you find it really necessary

// redefinitions for EIP-3529
private static final Gas SSTORE_CLEARS_SCHEDULE = SSTORE_RESET_GAS.plus(ACCESS_LIST_STORAGE_COST);

protected static final Gas NEGATIVE_SSTORE_CLEARS_SCHEDULE =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't need to be protected does it? it is only used in this class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

macfarla and others added 4 commits May 8, 2021 11:51
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
@matkt matkt merged commit 3267501 into hyperledger:master May 10, 2021
@matkt matkt deleted the feature/eip-3529 branch June 24, 2021 12:18
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Signed-off-by: Karim TAAM <karim.t2am@gmail.com>
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