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.

Sign up for GitHub

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

Jump to bottom

ETC "DieHard" support #155

Closed
wants to merge 1 commit into from
Closed

ETC "DieHard" support #155

wants to merge 1 commit into from

Conversation

GregTheGreek
Copy link
Contributor

PR description

This PR introduces ETC changes for the DieHard fork

Fixed Issue(s)

N/a

@shemnon shemnon changed the title Greg/backport 3 ETC "DieHard" support Nov 4, 2019
shemnon
shemnon reviewed Nov 4, 2019
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mostly minor revisions, focusing on keeping the ETC fork blocks grouped together and in ETC order.

besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java Show resolved Hide resolved
config/src/main/java/org/hyperledger/besu/config/GenesisConfigOptions.java Outdated Show resolved Hide resolved
config/src/main/java/org/hyperledger/besu/config/GenesisConfigOptions.java Show resolved Hide resolved
config/src/test-support/java/org/hyperledger/besu/config/StubGenesisConfigOptions.java Show resolved Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClassicProtocolSpecs.java Outdated Show resolved Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClassicProtocolSpecs.java Outdated Show resolved Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClassicProtocolSpecs.java
final Optional<BigInteger> chainId,
final OptionalInt contractSizeLimit,
final OptionalInt configStackSizeLimit) {
return MainnetProtocolSpecs.tangerineWhistleDefinition(contractSizeLimit, configStackSizeLimit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the defuse fork build off of diehard? Does it not use the SpuriousDragon gas schedule?

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClassicProtocolSpecs.java
configContractSizeLimit.orElse(SPURIOUS_DRAGON_CONTRACT_SIZE_LIMIT);
final int stackSizeLimit = configStackSizeLimit.orElse(MessageFrame.DEFAULT_MAX_STACK_SIZE);

return MainnetProtocolSpecs.tangerineWhistleDefinition(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this build off of the defuse fork?

Copy link
Contributor

Choose a reason for hiding this comment

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

This will actually build from the gotham fork which is defined in future commits.

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClassicProtocolSpecs.java
final boolean enableRevertReason) {
final int contractSizeLimit =
configContractSizeLimit.orElse(SPURIOUS_DRAGON_CONTRACT_SIZE_LIMIT);
return MainnetProtocolSpecs.constantinopleFixDefinition(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this build off of atlantis?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is corrected in future commits.

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetDifficultyCalculators.java
@@ -67,6 +67,17 @@ private MainnetDifficultyCalculators() {}
return periodCount > 1 ? adjustForPeriod(periodCount, difficulty) : difficulty;
};

public static DifficultyCalculator<Void> DIFFICULTY_BOMB_REMOVED =
Copy link
Contributor

Choose a reason for hiding this comment

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

Hot take: could this instead become NO_DIFFICULTY_BOMB and have HOMESTEAD wrap it? So this comes first then homestad with the bomb adjustment, making no difficulty the base case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, no difficulty bomb should be the base case, however homestead does have the difficulty bomb and it come before the bomb was removed. I was reluctant to refactor existing code.

Copy link
Contributor

@shemnon shemnon Nov 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should refactor this one. Even thought it breaks the other pattern of "what was done first and then later adjusted" it results in duplication of the difficulty calculation code. Perhaps as an alternative that calculation code can be pulled out as a method and both calculators can call the same method.

@edwardmack edwardmack force-pushed the greg/backport-3 branch 3 times, most recently from 76d8a70 to 9e481da Compare November 5, 2019 16:43
shemnon
shemnon reviewed Nov 6, 2019
View reviewed changes
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolScheduleBuilder.java Outdated
@@ -114,11 +114,21 @@ private ProtocolScheduleBuilder(
config.getTangerineWhistleBlockNumber(),
MainnetProtocolSpecs.tangerineWhistleDefinition(
config.getContractSizeLimit(), config.getEvmStackSize()));
addProtocolSpec(
Copy link
Contributor

Choose a reason for hiding this comment

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

The classic protocol specs should be grouped together.

shemnon
shemnon approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

Once the regrouping in ProtocolSpec is complete and the DCO and Jenkins build are happy I'll merge this in.

It may be easiest to do a squash merge on top of the git master branch to satisfy the DCO check.

shemnon
shemnon suggested changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

ProtocolScheduleBuilder still needs the classic protocol specs grouped together.

@GregTheGreek GregTheGreek force-pushed the greg/backport-3 branch 4 times, most recently from a048f3a to dd1d3b6 Compare November 6, 2019 21:13
@GregTheGreek
adds diehard
25107f4 
Signed-off-by: Gregory Markou <gregorymarkou@gmail.com>
@GregTheGreek
Copy link
Contributor Author

Closing because DCO

This was referenced Feb 22, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwardmack edwardmack edwardmack left review comments

@shemnon shemnon shemnon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GregTheGreek @shemnon @edwardmack