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.

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

Keccak256 mining #1882

Merged
merged 17 commits into from
Mar 5, 2021
Merged

Keccak256 mining #1882

merged 17 commits into from
Mar 5, 2021

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Feb 5, 2021

Signed-off-by: Antoine Toulme [email protected]

PR description

This PR introduces more flexibility around PoW operations, by renaming some classes that use the Ethash term to PoW. It also calls out explicitly the PoW algorithm used, if any. It introduces the ability to select the PoW algorithm in the hard fork schedule.

This PR also introduces a keccak256 hasher that can be used to validate solutions or run CPU mining.
The changes can be used with a new dev network named ecip1049_dev.

The changes are associated with ECIP1049

Changelog

@atoulme atoulme added the ETC Ethereum Classic label Feb 5, 2021
@atoulme atoulme force-pushed the mining_decoupling branch 3 times, most recently from 3b304fd to 97c04e7 Compare February 5, 2021 06:12
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.

The renaming are fine. I have some design concerns with how PoWAlgorithm is wired down through the protocol specs.

@atoulme
Copy link
Contributor Author

atoulme commented Feb 6, 2021

All good points and I can follow this pathway. I wanted to create a “dev-keccak256” network that would use keccak mining with a fixed difficulty. How would you go about that here? Set all fork blocks to 0 and add a new fork name for keccak? Would there still be a keccak config block next to the ethash one?

@shemnon
Copy link
Contributor

shemnon commented Feb 6, 2021

That is exactly what was done in the EIP-1559 test networks. There can be a keccak config block configuring the parameters of keccak. However I would expect this would only cover dev related options like fixeddifficulty as most of the parameters would be implicit in the keccak fork. I would recommend ecip1049 for the fork name.

@atoulme
Copy link
Contributor Author

atoulme commented Feb 7, 2021

I think the goquorum compatibility boolean confused me, apologies. This should be a bit nicer now. I am about to push this, with a TODO for the actual keccak hasher algorithm.

@atoulme atoulme force-pushed the mining_decoupling branch 5 times, most recently from 9dbbf86 to cbe4410 Compare February 7, 2021 20:13
@atoulme atoulme changed the title Decouple pow from ethash Keccak256 mining Feb 9, 2021
Bytes input = Bytes.wrap(Bytes.wrap(headerHash), Bytes.ofUnsignedLong(nonce));

MessageDigest digest = KECCAK_256.get();
digest.update(input.toArrayUnsafe());
Copy link
Contributor

Choose a reason for hiding this comment

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

Where's the mixhash? To replace the POW you replace the step where MixHash is sent to Dagger/hashimoto and instead send mixhash to keccack. That's the Ethereum way.

Doing it this way has deep implications to block header validation for the rest of the Besu client. Preserving the meaning and utility of MixHash (which isn't part of Ethash, just what feeds the Ethash) is necessary for the stability of Besu.

Choose a reason for hiding this comment

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

Hi folks, author of ECIP-1049 here just weighing in. The proposal has compatibility as its top priority, so there should be no changes to the block construction itself. Also the way the work is verified currently, using keccak256 of mixhash and nonce. The way I envision the system in the proposal is:

  • Block is valid if keccak256(keccak256(rlp(unsealed header)), nonce) <= 2^256 / difficulty
  • mixhash = keccak256(rlp(unsealed header)) which is stored in the block. Currently this process is ethash(rlp(unsealed header))
  • So to validate a block we check keccak256(mixhash, nonce) <= 2^256 / difficulty

Let me know if this mental model is valid, since this is what I am basing my analysis of the code on.

=================================

Left 2 comments in the code as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the Implementation section of ECIP-1049 doesn't have that level of clarity (this one correct? ethereumclassic/ECIPs#13). It seems to be an extension of the rational section. That is perhaps my biggest beef in that the spec doesn't in fact specify anything with clarity. It has some hints but is missing actual details, such as what is H sub 'n bar'?

There needs to be a "specification" section in ECIP-1049, as noted by the ecip template. These three bullet points should be in that section. There also needs to be clear specification as to the proposed difficulty bump. And some justification of the 100x difficulty bump, it appears to be pulled from thin air.

Also, there are so many versions of ECIP floating around I think the PR description and ultimate merge commit needs to reference the "official" location of the ECIP it's implmementing.

Copy link

@antsankov antsankov Feb 11, 2021

Choose a reason for hiding this comment

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

I sincerely apologize about any confusion with the proposal, the roll out plan is this ECIP is officially here: ethereumclassic/ECIPs#394. This is the re-draft that will be finalized into ECIP-1049, once we have a block number we feel comfortable to activate on. I'm adding to it based on your points, and welcome the feedback.

Once this PR to Besu is merged, we have a number of hardware engineers, and other full node teams (Mantis, and core-geth) that'll base their implementation of nodes and hardware/mining pools software off of what Besu supports, and then we'll activate mainnet once it's supported by the nodes.

  • The 100x difficulty bump is based on discussions with experienced chip designer and former ATI Technologies VP Henry Quan report here, and was selected on estimates of how much higher we predict initial Keccak-256 hash rate will be compared to Ethash, when first launched.
  • H sub n refers to nonce.

@atoulme atoulme force-pushed the mining_decoupling branch 4 times, most recently from eb6b5e8 to 742c34f Compare February 12, 2021 09:56
Copy link

@peterwillcn peterwillcn left a comment

Choose a reason for hiding this comment

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

why use Keccak256?

@shemnon
Copy link
Contributor

shemnon commented Feb 23, 2021

I'm going to be a bit of a hard nose about this, but until the referenced ECIP (https://ecips.ethereumclassic.org/ECIPs/ecip-1049) specifies what has been discussed in this PR I'm not going to give it a serious review. If we are implementing what should be specified in an ECIP then the ECIP should actually specify something.

I'm not looking for yellow paper equations but at least "this field now has this data (xxxx)" "this field now has this data (yyy)" "to calculate that value do XYZ." "If I J and K then the block is valid, otherwise it is invalid."

@antsankov
Copy link

antsankov commented Feb 24, 2021

ACK @shemnon - Proposal will be sealed shortly, and only thing left after sealing is to determine what block number it's activated on. See changes:

  • Added in an implementation standard here which outlines exactly how to implement ECIP-1049 and the formal of definition of the keccak256 proof of work algorithm.
  • Added in a verified miner test vector in the Appendix of the ECIP, which shows a single solved block with all of the inputs and outputs of the PoW specified.

Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
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.

Looks mostly good. Just a few more smaller comments.

@@ -191,7 +192,7 @@ BesuControllerBuilder fromGenesisConfig(
genesisConfig.getConfigOptions(genesisConfigOverrides);
final BesuControllerBuilder builder;

if (configOptions.isEthHash()) {
if (configOptions.getPowAlgorithm() != PowAlgorithm.UNSUPPORTED) {
builder = new MainnetBesuControllerBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but I think it's time we renamed a bunch of our Mainnet* classes to better names. Default, PoW, etc etc.

@atoulme atoulme force-pushed the mining_decoupling branch 2 times, most recently from 48a2407 to e6a539d Compare March 2, 2021 07:14
Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
@atoulme atoulme enabled auto-merge (squash) March 5, 2021 08:28
@atoulme atoulme merged commit db23aef into hyperledger:master Mar 5, 2021
@atoulme atoulme deleted the mining_decoupling branch March 5, 2021 08:48
RichardH92 pushed a commit to RichardH92/besu that referenced this pull request Mar 29, 2021
* Decouple PoW from ethash

Signed-off-by: Antoine Toulme <[email protected]>

* Address code review comments, create a dev network for ecip1049, prepare for keccak hasher

Signed-off-by: Antoine Toulme <[email protected]>

* Add PoW function and a few simple tests as test vectors

Signed-off-by: Antoine Toulme <[email protected]>

* Make the PoWHasher hash function a bit easier to understand

Signed-off-by: Antoine Toulme <[email protected]>

* simplify and call out the code of the keccak hash function

Signed-off-by: Antoine Toulme <[email protected]>

* support fixed difficulty for keccak mining

Signed-off-by: Antoine Toulme <[email protected]>

* Fix the dev network config

Signed-off-by: Antoine Toulme <[email protected]>

* Add comment to KeccakHasher

Signed-off-by: Antoine Toulme <[email protected]>

* Increase fixed difficulty for the ecip1049 dev network to produce hashes a bit less often

Signed-off-by: Antoine Toulme <[email protected]>

* spotless

Signed-off-by: Antoine Toulme <[email protected]>

* fix test expectations

Signed-off-by: Antoine Toulme <[email protected]>

* Fix javadoc issue

Signed-off-by: Antoine Toulme <[email protected]>

* add acceptance test using keccak mining

Signed-off-by: Antoine Toulme <[email protected]>

* add changelog entry

Signed-off-by: Antoine Toulme <[email protected]>

* Address code review comments

Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Richard Hart <[email protected]>
@garyschulte garyschulte mentioned this pull request Mar 30, 2021
1 task
RatanRSur pushed a commit that referenced this pull request Apr 7, 2021
it appears that [#1882] inadvertently broke retesteth no-op PoW mining.

Signed-off-by: garyschulte <[email protected]>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* Decouple PoW from ethash

Signed-off-by: Antoine Toulme <[email protected]>

* Address code review comments, create a dev network for ecip1049, prepare for keccak hasher

Signed-off-by: Antoine Toulme <[email protected]>

* Add PoW function and a few simple tests as test vectors

Signed-off-by: Antoine Toulme <[email protected]>

* Make the PoWHasher hash function a bit easier to understand

Signed-off-by: Antoine Toulme <[email protected]>

* simplify and call out the code of the keccak hash function

Signed-off-by: Antoine Toulme <[email protected]>

* support fixed difficulty for keccak mining

Signed-off-by: Antoine Toulme <[email protected]>

* Fix the dev network config

Signed-off-by: Antoine Toulme <[email protected]>

* Add comment to KeccakHasher

Signed-off-by: Antoine Toulme <[email protected]>

* Increase fixed difficulty for the ecip1049 dev network to produce hashes a bit less often

Signed-off-by: Antoine Toulme <[email protected]>

* spotless

Signed-off-by: Antoine Toulme <[email protected]>

* fix test expectations

Signed-off-by: Antoine Toulme <[email protected]>

* Fix javadoc issue

Signed-off-by: Antoine Toulme <[email protected]>

* add acceptance test using keccak mining

Signed-off-by: Antoine Toulme <[email protected]>

* add changelog entry

Signed-off-by: Antoine Toulme <[email protected]>

* Address code review comments

Signed-off-by: Antoine Toulme <[email protected]>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
it appears that [hyperledger#1882] inadvertently broke retesteth no-op PoW mining.

Signed-off-by: garyschulte <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ETC Ethereum Classic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants