Skip to content

Implement Phoenix EVM and Protocol Upgrades#434

Merged
edwardmack merged 11 commits into
besu-eth:masterfrom
ChainSafe:ed/etc_phoenix_update
Mar 9, 2020
Merged

Implement Phoenix EVM and Protocol Upgrades#434
edwardmack merged 11 commits into
besu-eth:masterfrom
ChainSafe:ed/etc_phoenix_update

Conversation

@edwardmack
Copy link
Copy Markdown
Contributor

PR description

This PR implements EVM and Protocol upgrades as outlined in ECIP 1088 https://ecips.ethereumclassic.org/ECIPs/ecip-1088

Fixed Issue(s)

Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Implement Phoenix EVM and protocol upgrades as per ECIP-1088

Signed-off-by: Edward Mack <ed@edwardmack.com>
@edwardmack edwardmack requested a review from shemnon February 28, 2020 18:14
Copy link
Copy Markdown
Contributor

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +166 to +173
.contractCreationProcessorBuilder(
(gasCalculator, evm) ->
new MainnetContractCreationProcessor(
gasCalculator,
evm,
true,
Collections.singletonList(MaxCodeSizeRule.of(contractSizeLimit)),
1))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This appears to be identical to the definition already inherited from Atlantis. Am I missing something, should something have changed or is this just not required?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is indeed the same as Atlantis. If this isn't specified here, will it automatically use the previous one defined?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes. You should only need to specify the changes between each milestone. So the only time you'd have two milestones with the same declaration is if something changed and then changed back (you'd have to override the changed version back to the original).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, this makes sense, I didn't think in was required, but I was following the pattern in MainnetProtocolSpec where contractCreationProcessBuilder is defined for SpuriouDragon then with the same definition for Istanbul without any changes is between. I wasn't sure why that was done, but I followed that pattern. I'll remove it from Phoenix since it has already been defined.

Copy link
Copy Markdown
Contributor

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

Given that the contractCreationProcessorBuilder definition for Phoenix is exactly the same as the previously defined in Atlantis, we don't need to specify it again.

remove unnecessary contractCreationProcessorBuilder from Phoenix
ClassicProtocolSpec.

Signed-off-by: Edward Mack <ed@edwardmack.com>
@edwardmack
Copy link
Copy Markdown
Contributor Author

I've updated this, should be all set now.

@edwardmack edwardmack merged commit c955f67 into besu-eth:master Mar 9, 2020
@edwardmack edwardmack mentioned this pull request Mar 12, 2020
@edwardmack edwardmack deleted the ed/etc_phoenix_update branch March 20, 2020 14:29
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.

4 participants