Skip to content

[ETCM-746] Add support for PoW with Keccak-256 (ECIP-1049) #960

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

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

leo-bogastry
Copy link
Contributor

@leo-bogastry leo-bogastry commented Apr 7, 2021

Added support for PoW with Keccak-256 (ECIP-1049)
Renamed several classes from Ethash to PoW because they will be used in both Ethash Pow and Keccak PoW

Description

Add support for PoW with Keccak-256 (ECIP-1049)
Renamed several classes from Ethash to PoW because they will be used in both Ethash Pow and Keccak PoW

Proposed Solution

Added KeccakCalculation object that computes the new mixHash and validates it against the block difficulty.
In order to control if a block should be mined using the new Keccak-Pow or current Ethash-Pow a simple if clause was added to the EthashMiner class. In ETCM-759 this will be refactored.

Added KeccakBlockHeaderValidator object that validates a block header for a block mined with the Keccak-Pow.
Added PoWBlockHeaderValidator class to controls whether (new)KeccakBlockHeaderValidator or (current) EthashBlockHeaderValidator should be used for the validation

NOTE:
Class EthashDAGManager is not new code it was extracted from EthashMiner.
Because I renamed the package, EthashMiner is marked as a new file, but only the code related with if (isKeccak(blockNumber))

The total number of files modified is big because of renaming

Testing

The branch was tested against etc network with both FastSync and RegularSync synchronization.

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-746-support-keccak256-pow branch from 9ec6b5e to 128377c Compare April 7, 2021 16:18
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-746-support-keccak256-pow branch from 128377c to 487a22f Compare April 14, 2021 10:54
@bsuieric
Copy link
Contributor

So we don't use it for now, just for test purposes? in ECIP-1049 I see there is an activation block

@leo-bogastry
Copy link
Contributor Author

So we don't use it for now, just for test purposes? in ECIP-1049 I see there is an activation block

Where did you see that? I have the idea that that it is still not decided, on which block to do the switch, for mainnet. Or even if there's a final decision to really go forward with this

I don't understand what do you mean with "is this just for testing purposes?". Everything has to be tested before going to live (there's a test network called Astor that only mines with keccak, we will add support for that on ETCM-749)

Copy link
Contributor

@AnastasiiaL AnastasiiaL left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just minor comments about the code that was (probably) already there, but since we are touching these files - might as well improve it a bit


def calculateDagSize(blockNumber: Long, epoch: Long): (Array[Array[Int]], Long) = {
(currentEpoch, currentEpochDag, currentEpochDagSize) match {
case (Some(`epoch`), Some(dag), Some(dagSize)) => (dag, dagSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case (Some(`epoch`), Some(dag), Some(dagSize)) => (dag, dagSize)
case (Some(_), Some(dag), Some(dagSize)) => (dag, dagSize)

@bsuieric
Copy link
Contributor

So we don't use it for now, just for test purposes? in ECIP-1049 I see there is an activation block

Where did you see that? I have the idea that that it is still not decided, on which block to do the switch, for mainnet. Or even if there's a final decision to really go forward with this

I don't understand what do you mean with "is this just for testing purposes?". Everything has to be tested before going to live (there's a test network called Astor that only mines with keccak, we will add support for that on ETCM-749)

I saw isKeccak is always false based on the configuration

@leo-bogastry
Copy link
Contributor Author

So we don't use it for now, just for test purposes? in ECIP-1049 I see there is an activation block

Where did you see that? I have the idea that that it is still not decided, on which block to do the switch, for mainnet. Or even if there's a final decision to really go forward with this
I don't understand what do you mean with "is this just for testing purposes?". Everything has to be tested before going to live (there's a test network called Astor that only mines with keccak, we will add support for that on ETCM-749)

I saw isKeccak is always false based on the configuration

Yes, at the moment yes. It will only be tested in Astor for now

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-746-support-keccak256-pow branch from e95c459 to d511ed5 Compare April 15, 2021 12:48
@leo-bogastry
Copy link
Contributor Author

I realized that the tests for the EthashMiner never run by default. I forced then to run locally and all passed

Renamed several classes from Ethash to PoW because they will be used in both Ethash Pow and Keccak PoW
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-746-support-keccak256-pow branch from d511ed5 to 9b71140 Compare April 16, 2021 11:59
@leo-bogastry leo-bogastry merged commit 3ea65c1 into develop Apr 16, 2021
@leo-bogastry leo-bogastry deleted the feature/ETCM-746-support-keccak256-pow branch April 16, 2021 12: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.

6 participants