Common protocol schedule config handling #250
Conversation
| builder -> | ||
| applyCliqueSpecificModifications( | ||
| epochManager, cliqueConfig.getBlockPeriodSeconds(), localNodeAddress, builder)) | ||
| .createProtocolSchedule(DEFAULT_CHAIN_ID); |
There was a problem hiding this comment.
Passing in a magic default is a bit of smell - reads as though we are creating a protocol schedule of the DEFAULT_CHAIN_ID (rather than this being an override).
Also - can't quite appreciate the difference between passing everything in at construction vs in the call to createProtocolSchedule.
There was a problem hiding this comment.
Yeah our handling of default chain ID is definitely weird, but not new here (I think it used to be a bit better hidden). I suspect we should default to no chain ID if one isn't in the config file which would clean this up but that probably requires some additional research and thought.
I have changed it so that the default chain ID is passed into the constructor instead of the createProtocolSchedule method to be more consistent.
...nsus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueProtocolSchedule.java
Outdated
Show resolved
Hide resolved
| @@ -87,4 +89,47 @@ private OptionalLong getOptionalLong(final String key) { | |||
| ? OptionalLong.of(configRoot.getLong(key)) | |||
| : OptionalLong.empty(); | |||
| } | |||
|
|
|||
| public static class Builder { | |||
There was a problem hiding this comment.
nit: This builder could/should also handle the consensus parameters - but given it is currently only used for testing, does not need repairing now.
There was a problem hiding this comment.
Replaced it with a dedicated test stub (which is currently hardcoded to ethash but sets a better pattern and gets what is really just test code out of the production tree).
| .getDaoForkBlock() | ||
| .ifPresent( | ||
| daoBlockNumber -> { | ||
| if (daoBlockNumber > 0) { |
There was a problem hiding this comment.
Slight concern that if the DAO fork occurs anywhere other than Frontier or Homestead, that the protocol spec will revert to an incorrect spec.
Given Dao fork should only occur on mainnet, and it definitively occurred in homestead - this is probably fine.
There was a problem hiding this comment.
Agreed on both counts. It's messy but looking at what the Dao fork does, chances of success on anything other than MainNet at the specific time of the Dao fork are almost certain to end in tears.
* Introduce ProtocolScheduleBuilder and use it for Clique, MainNet, IBFT and dev. * Remove default milestone blocks and simplify MainnetProtocolSchedule. All milestone blocks must now be defined in the genesis file (previously ethash chains would get Mainnet milestone blocks by default).
PR description
To ensure milestone configuration is consistent across consensus algorithms, introduce a
ProtocolScheduleBuilderthat sets up milestones.NOTE: This changes handling of ethash genesis config files so that if milestones are not listed in config, they are not activated instead of defaulting to the Mainnet activation block numbers. Our MainNet config is unaffected as it already included the scheduled milestones (as is typical for all genesis configs). This new behaviour seems to be consistent with other clients. It also avoids confusion when we schedule a Constantinople fork on MainNet as previously that would have automatically applied to all ethash configs that didn't explicitly disable Constantinople.
Fixed Issue(s)
Follow up to #245 to ensure we don't forget to support future milestones in Clique and other consensus algorithms.