-
Notifications
You must be signed in to change notification settings - Fork 835
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
Keccak256 mining #1882
Conversation
243c733
to
5d37695
Compare
3b304fd
to
97c04e7
Compare
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.
The renaming are fine. I have some design concerns with how PoWAlgorithm is wired down through the protocol specs.
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/PoWHasher.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/PoWHasher.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/PoWHasher.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolScheduleBuilder.java
Outdated
Show resolved
Hide resolved
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? |
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 |
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. |
9dbbf86
to
cbe4410
Compare
Bytes input = Bytes.wrap(Bytes.wrap(headerHash), Bytes.ofUnsignedLong(nonce)); | ||
|
||
MessageDigest digest = KECCAK_256.get(); | ||
digest.update(input.toArrayUnsafe()); |
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.
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.
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.
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 isethash(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.
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.
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.
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 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.
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/KeccakHasher.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/KeccakHasher.java
Outdated
Show resolved
Hide resolved
eb6b5e8
to
742c34f
Compare
.../src/test/java/org/hyperledger/besu/ethereum/difficulty/fixed/FixedProtocolScheduleTest.java
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/KeccakHasher.java
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/KeccakHasher.java
Outdated
Show resolved
Hide resolved
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.
why use Keccak256?
9c88313
to
853c9da
Compare
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." |
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:
|
Signed-off-by: Antoine Toulme <[email protected]>
Signed-off-by: Antoine Toulme <[email protected]>
…hes a bit less often 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]>
4d35273
to
a517f61
Compare
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.
Looks mostly good. Just a few more smaller comments.
besu/src/main/java/org/hyperledger/besu/chainimport/JsonBlockImporter.java
Outdated
Show resolved
Hide resolved
besu/src/main/java/org/hyperledger/besu/cli/config/EthNetworkConfig.java
Show resolved
Hide resolved
@@ -191,7 +192,7 @@ BesuControllerBuilder fromGenesisConfig( | |||
genesisConfig.getConfigOptions(genesisConfigOverrides); | |||
final BesuControllerBuilder builder; | |||
|
|||
if (configOptions.isEthHash()) { | |||
if (configOptions.getPowAlgorithm() != PowAlgorithm.UNSUPPORTED) { | |||
builder = new MainnetBesuControllerBuilder(); |
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.
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.
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/PoWSolution.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/EthHash.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/PoWSolverInputs.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolSpec.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolSpec.java
Outdated
Show resolved
Hide resolved
48a2407
to
e6a539d
Compare
Signed-off-by: Antoine Toulme <[email protected]>
e6a539d
to
bc70025
Compare
fc2c2fd
to
f25b452
Compare
Signed-off-by: Antoine Toulme <[email protected]>
f25b452
to
7d9acd0
Compare
* 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]>
it appears that [#1882] inadvertently broke retesteth no-op PoW mining. Signed-off-by: garyschulte <[email protected]>
* 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]>
it appears that [hyperledger#1882] inadvertently broke retesteth no-op PoW mining. Signed-off-by: garyschulte <[email protected]>
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