[ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration #1421

edwardmack · 2020-10-06T21:13:47Z

PR description

This PR introduces code to implement ECIP 1099: Calbrate Epoch Duration https://ecips.ethereumclassic.org/ECIPs/ecip-1099

Created new genesis config ecip1099Block to define the block that epoch length change activation should be activated for Ethereum Classic Networks.
Implement code to build function that calculates epoch.

Fixed Issue(s)

Changelog

I thought about the changelog and included a changelog update if required.

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>

shemnon

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.

shemnon · 2020-10-06T21:46:55Z

config/src/main/resources/mordor.json

@@ -5,7 +5,9 @@
    "aghartaBlock": 301243,
    "phoenixBlock": 999983,
    "ecip1017EraRounds": 2000000,
-    "ethash": {}
+    "etchash": {


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>

edwardmack · 2020-10-08T01:04:16Z

@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>

matkt · 2020-10-09T08:49:12Z

config/src/main/java/org/hyperledger/besu/config/EthashConfigOptions.java

  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));


How about putting epochLength (or something like that) in place of a just to make it easier to read

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).

matkt · 2020-10-09T09:06:11Z

config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java

+  public OptionalLong getEcip1099BlockNumber() {
+    return getOptionalLong("ecip1099block");
+  }
+


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));

Signed-off-by: Edward Mack <ed@edwardmack.com>

matkt

LGTM

shemnon · 2020-10-09T16:22:03Z

CHANGELOG.md

@@ -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)


This will need to go into a new 20.10.0-RC2 section. RC1 has shipped.

LGTM once fixed.

bobsummerwill · 2020-10-09T22:05:29Z

@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 ^^^

q9f · 2020-10-12T11:03:48Z

Core Geth only has a release for Mordor Testnet, yet.

	{999983, ID{Hash: checksumToBytes(0xf42f5539), Next: 2_520_000}},
	{999984, ID{Hash: checksumToBytes(0xf42f5539), Next: 2_520_000}},
	{2_519_999, ID{Hash: checksumToBytes(0xf42f5539), Next: 2_520_000}},
	{2_520_000, ID{Hash: checksumToBytes(0x66b5c286), Next: 0}},

https://github.com/etclabscore/core-geth/blob/master/core/forkid/forkid_test.go#L199-L202

I'll expect a mainnet release the coming days.

shemnon · 2020-10-12T16:44:48Z

Can we name the fork "Thanos"?

q9f · 2020-10-13T10:17:25Z

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>

edwardmack added 12 commits September 29, 2020 16:44

Add genesis config parameter ecip1099Block

349e753

Signed-off-by: Edward Mack <ed@edwardmack.com>

add stub for epoch activation config

f70f838

Signed-off-by: Edward Mack <ed@edwardmack.com>

add stubs for EtcBesuControllerBuilder

9ba8670

that will handle ECIP 1099 Colibrate Epoch Duration functionality Signed-off-by: Edward Mack <ed@edwardmack.com>

introduce EtcHashMinerExecutor

22973df

Signed-off-by: Edward Mack <ed@edwardmack.com>

implement EtcHashMinerExecutor

cae9122

Signed-off-by: Edward Mack <ed@edwardmack.com>

remove ecip1099 genesis config option

413019a

Signed-off-by: Edward Mack <ed@edwardmack.com>

apply spotless to code

524bfa8

Signed-off-by: Edward Mack <ed@edwardmack.com>

add test for Etc Hash epoch calculation

e58f103

Signed-off-by: Edward Mack <ed@edwardmack.com>

cleanup comments, apply spotless

15c7ba4

Signed-off-by: Edward Mack <ed@edwardmack.com>

make needed variables final

a859587

Signed-off-by: Edward Mack <ed@edwardmack.com>

update changelog

1a68a21

Signed-off-by: Edward Mack <ed@edwardmack.com>

Merge branch 'master' into ed/ecip_1099

7a53f9d

edwardmack self-assigned this Oct 6, 2020

edwardmack added the ETC Ethereum Classic label Oct 6, 2020

edwardmack requested a review from shemnon October 6, 2020 21:21

shemnon reviewed Oct 6, 2020

View reviewed changes

edwardmack added 7 commits October 7, 2020 19:48

refactor code to add epochCalculator to EthHash

6235ce2

As suggested in comment removed EtcHash series of classes to keep existing controller. Signed-off-by: Edward Mack <ed@edwardmack.com>

Merge branch 'master' into ed/ecip_1099

760f80a

Merge branch 'master' into ed/ecip_1099

665d591

use gas limit calculator

a94b15f

Signed-off-by: Edward Mack <ed@edwardmack.com>

fix imports

77889cc

Signed-off-by: Edward Mack <ed@edwardmack.com>

run spotless apply

f43c2df

Signed-off-by: Edward Mack <ed@edwardmack.com>

fix imports

6e2bb67

Signed-off-by: Edward Mack <ed@edwardmack.com>

edwardmack added 2 commits October 7, 2020 21:13

fix comments

4f86ab5

Signed-off-by: Edward Mack <ed@edwardmack.com>

Merge branch 'master' into ed/ecip_1099

7539202

edwardmack requested a review from shemnon October 8, 2020 14:35

edwardmack added 2 commits October 8, 2020 11:06

Merge branch 'master' into ed/ecip_1099

b72ee99

Merge branch 'master' into ed/ecip_1099

1616d36

matkt requested changes Oct 9, 2020

View reviewed changes

edwardmack added 2 commits October 9, 2020 10:50

Merge branch 'master' into ed/ecip_1099

104f9a3

add ecip1099Block option to asMap method

bde851f

Signed-off-by: Edward Mack <ed@edwardmack.com>

edwardmack requested a review from matkt October 9, 2020 15:16

matkt approved these changes Oct 9, 2020

View reviewed changes

shemnon reviewed Oct 9, 2020

View reviewed changes

shemnon approved these changes Oct 9, 2020

View reviewed changes

edwardmack merged commit 787871a into hyperledger:master Oct 9, 2020

edwardmack deleted the ed/ecip_1099 branch October 9, 2020 16:30

q9f mentioned this pull request Oct 12, 2020

ECIP-1099: move epoch duration to accepted ethereumclassic/ECIPs#390

Merged

edwardmack mentioned this pull request Oct 15, 2020

rename genesis config option Ecip1099Block to ThanosBlock #1462

Merged

1 task

shemnon added RC Backport PRs that need to be backported to the latest RC branch. Remove after backport. and removed RC Backport PRs that need to be backported to the latest RC branch. Remove after backport. labels Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration #1421

[ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration #1421

edwardmack commented Oct 6, 2020 •

edited

Loading

shemnon left a comment

shemnon Oct 6, 2020

edwardmack commented Oct 8, 2020

matkt Oct 9, 2020

edwardmack Oct 9, 2020

matkt Oct 9, 2020

edwardmack Oct 9, 2020

matkt left a comment

shemnon Oct 9, 2020

shemnon Oct 9, 2020

bobsummerwill commented Oct 9, 2020

q9f commented Oct 12, 2020

shemnon commented Oct 12, 2020

q9f commented Oct 13, 2020

[ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration #1421

[ECIP-1099] Implement ECIP 1099: Calibrate Epoch Duration #1421

Conversation

edwardmack commented Oct 6, 2020 • edited Loading

PR description

Fixed Issue(s)

Changelog

shemnon left a comment

Choose a reason for hiding this comment

shemnon Oct 6, 2020

Choose a reason for hiding this comment

edwardmack commented Oct 8, 2020

matkt Oct 9, 2020

Choose a reason for hiding this comment

edwardmack Oct 9, 2020

Choose a reason for hiding this comment

matkt Oct 9, 2020

Choose a reason for hiding this comment

edwardmack Oct 9, 2020

Choose a reason for hiding this comment

matkt left a comment

Choose a reason for hiding this comment

shemnon Oct 9, 2020

Choose a reason for hiding this comment

shemnon Oct 9, 2020

Choose a reason for hiding this comment

bobsummerwill commented Oct 9, 2020

q9f commented Oct 12, 2020

shemnon commented Oct 12, 2020

q9f commented Oct 13, 2020

edwardmack commented Oct 6, 2020 •

edited

Loading