-
Notifications
You must be signed in to change notification settings - Fork 818
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
[ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration #1421
Conversation
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
that will handle ECIP 1099 Colibrate Epoch Duration functionality Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the design of a separate "EtcHash" series of classes. A separate controller and the genesis config seem a bit wonky.
What I think would be better would be to add a field Function<Long, Long> epochCalculator;
to EthHash
and default it with block-> Long.divideUnsigned(block, EPOCH_LENGTH)
and have the epoch
method call that lambda. Then when ECIP1099 is activated we use the protocol schedule to set a new calculator, like is currently done with the difficulty calculators.
One other question: Does ECIP1099 impact ETH/64 fork identifier? We should settle this with core-geth before too long. It in every way smells and acts like a fork so I expect the answer is yes.
@@ -5,7 +5,9 @@ | |||
"aghartaBlock": 301243, | |||
"phoenixBlock": 999983, | |||
"ecip1017EraRounds": 2000000, | |||
"ethash": {} | |||
"etchash": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is atypical form for activated forks. I would have expected "ecip1099Block": 2520000
instead of a nested construct. It's the same algorithm with different parameters, not a new algorithm.
As suggested in comment removed EtcHash series of classes to keep existing controller. Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
Signed-off-by: Edward Mack <ed@edwardmack.com>
@shemnon I've updated this based on your comments and our discussion. I think it's in good shape now. |
Signed-off-by: Edward Mack <ed@edwardmack.com>
Map<String, Object> asMap() { | ||
final ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder(); | ||
getFixedDifficulty().ifPresent(l -> builder.put("fixeddifficulty", l)); | ||
getEpochLengthActivationBlock().ifPresent(a -> builder.put("epochlengthactivation", a)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about putting epochLength
(or something like that) in place of a
just to make it easier to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed getEpochLengthActivationBlock
from EthashConfigOptions
because I didn't need it (I created the param in JsonGenesisConfigOptions instead, and forgot to remove it from here).
public OptionalLong getEcip1099BlockNumber() { | ||
return getOptionalLong("ecip1099block"); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need to add this ecip here in the code (in the asMap method)
besu/config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java
Line 336 in 1616d36
getEcip1017EraRounds().ifPresent(l -> builder.put("ecip1017EraRounds", l)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Signed-off-by: Edward Mack <ed@edwardmack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -34,6 +34,7 @@ Hyperledger Besu is moving its versioning scheme to [CalVer](https://calver.org/ | |||
* Added support for multi-tenancy when using the early access feature of [onchain privacy group management](https://besu.hyperledger.org/en/stable/Concepts/Privacy/Onchain-PrivacyGroups/) | |||
* Fixed memory leak in eth/65 subprotocol behavior. It is now enabled by default. [\#1420](https://github.com/hyperledger/besu/pull/1420), [#1348](https://github.com/hyperledger/besu/pull/1348), [#1321](https://github.com/hyperledger/besu/pull/1321) | |||
* Added `--privacy-flexible-groups-enabled` as an alternative for `--privacy-onchain-groups-enabled` CLI option | |||
* Added support for ECIP-1099: Calibrate Epoch Duration. [\#1421](https://github.com/hyperledger/besu/pull/1421) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to go into a new 20.10.0-RC2
section. RC1 has shipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once fixed.
@shemnon "One other question: Does ECIP1099 impact ETH/64 fork identifier? We should settle this with core-geth before too long. It in every way smells and acts like a fork so I expect the answer is yes." Yes. Thanks for that reminder, Danno. This is a hard-fork - we just don't have a name for it yet, and ETH/64 identifier will be needed in both ETC clients. @meowsbits @q9f ^^^ |
Core Geth only has a release for Mordor Testnet, yet.
https://github.com/etclabscore/core-geth/blob/master/core/forkid/forkid_test.go#L199-L202 I'll expect a mainnet release the coming days. |
Can we name the fork "Thanos"? |
Ok. |
…r#1421) * Add genesis config parameter ecip1099Block Signed-off-by: Edward Mack <ed@edwardmack.com> * add stub for epoch activation config Signed-off-by: Edward Mack <ed@edwardmack.com> * add stubs for EtcBesuControllerBuilder that will handle ECIP 1099 Colibrate Epoch Duration functionality Signed-off-by: Edward Mack <ed@edwardmack.com> * introduce EtcHashMinerExecutor Signed-off-by: Edward Mack <ed@edwardmack.com> * implement EtcHashMinerExecutor Signed-off-by: Edward Mack <ed@edwardmack.com> * remove ecip1099 genesis config option Signed-off-by: Edward Mack <ed@edwardmack.com> * apply spotless to code Signed-off-by: Edward Mack <ed@edwardmack.com> * add test for Etc Hash epoch calculation Signed-off-by: Edward Mack <ed@edwardmack.com> * cleanup comments, apply spotless Signed-off-by: Edward Mack <ed@edwardmack.com> * make needed variables final Signed-off-by: Edward Mack <ed@edwardmack.com> * update changelog Signed-off-by: Edward Mack <ed@edwardmack.com> * refactor code to add epochCalculator to EthHash As suggested in comment removed EtcHash series of classes to keep existing controller. Signed-off-by: Edward Mack <ed@edwardmack.com> * use gas limit calculator Signed-off-by: Edward Mack <ed@edwardmack.com> * fix imports Signed-off-by: Edward Mack <ed@edwardmack.com> * run spotless apply Signed-off-by: Edward Mack <ed@edwardmack.com> * fix imports Signed-off-by: Edward Mack <ed@edwardmack.com> * fix comments Signed-off-by: Edward Mack <ed@edwardmack.com> * add ecip1099Block option to asMap method Signed-off-by: Edward Mack <ed@edwardmack.com> (cherry picked from commit 787871a) Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
* [ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration (#1421) (cherry picked from commit 787871a) Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com> * rename genesis config option Ecip1099Block to ThanosBlock (#1462) (cherry picked from commit 2d7cdf0) Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com> Co-authored-by: Edward Mack <ed@edwardmack.com>
PR description
This PR introduces code to implement ECIP 1099: Calbrate Epoch Duration https://ecips.ethereumclassic.org/ECIPs/ecip-1099
ecip1099Block
to define the block that epoch length change activation should be activated for Ethereum Classic Networks.Fixed Issue(s)
Changelog