Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ethash: implement Progpow #9762

Merged
merged 53 commits into from
Feb 20, 2019
Merged

ethash: implement Progpow #9762

merged 53 commits into from
Feb 20, 2019

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Oct 16, 2018

This is a proof-of-concept implementation of progpow, this is still work in progress and I don't intend this to be merged yet.

References:

@andresilva andresilva added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Oct 16, 2018
@andresilva andresilva changed the title ethcore, ethash: implement Progpow ethash: implement Progpow Oct 16, 2018
@andresilva
Copy link
Contributor Author

Benchmarks of baseline performance:

test benchmarks::bench_hashimoto_light       ... bench:     981,214 ns/iter (+/- 32,031)
test benchmarks::bench_progpow_light         ... bench:  16,766,912 ns/iter (+/- 1,429,102)
test benchmarks::bench_progpow_optimal_light ... bench:   5,055,690 ns/iter (+/- 2,379,231)

bench_progpow_optimal_light - doesn't take into account cdag creation, this is realistic since the cdag can be cached during it's epoch (50 blocks).

Will have a look at what can be optimized now that it's passing all test vectors.

@5chdn 5chdn added this to the 2.3 milestone Oct 17, 2018
@andresilva andresilva force-pushed the andre/progpow branch 2 times, most recently from b9fb187 to 948a75b Compare October 18, 2018 11:07
@holiman
Copy link
Contributor

holiman commented Oct 18, 2018

doesn't take into account cdag creation, this is realistic since the cdag can be cached during it's epoch (50 blocks).

Isn't the cDag epoch the same as dag epoch, 30k blocks?

@andresilva
Copy link
Contributor Author

@holiman Yes, you're right, the only thing that changes every 50 blocks is the program generated from the cDag, but the cDag itself has the same epoch as the ethash dag.

@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. and removed A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 18, 2019
@andresilva
Copy link
Contributor Author

I think I've addressed all grumbles. The only unsafe code that is kept is for transmuting from [u32; 8] to [u8; 32], I think doing the byte shuffling is unnecessary but can remove it if you think otherwise. There is probably a lot of low hanging fruit for optimization here. Completely unrolling the hash function kecak_f800 like we do for keccak_f1600 would probably speed it up a bit.

@andresilva
Copy link
Contributor Author

There's a test with a bunch of test vectors (shared among implementations) that might take a little while to run. We might want to disable this test on the CI.

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

This looks good to me, I agree some tests takes to long for CI (even for my pc) and should be disabled somehow before merging.

@@ -40,9 +41,15 @@ pub struct ProofOfWork {
pub mix_hash: H256,
}

enum Algorithm {
Hashimoto,
Progpow(CDag),
Copy link
Collaborator

@niklasad1 niklasad1 Feb 19, 2019

Choose a reason for hiding this comment

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

Does it make sense to Box CDag in the Algorithm enum?
It is quite big (16384 bytes), thus it will make the enum Algorithm as big.

EDIT:
Have seen any regression on Hashimoto after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. No I didn't see any regression in the benchmarks with all the changes I've done so far.

@andresilva
Copy link
Contributor Author

Boxed the cdag inside algorithm and adapted an existing cargo feature to skip the slow progpow test vectors in CI.

@andresilva
Copy link
Contributor Author

Actually since CI is running the tests with --release the test vectors don't take that long to run (20s on my laptop vs like 4 minutes for a debug build), so I'm going to keep them.

they don't take too long when running in release mode which is the case
for CI.
ethash/src/progpow.rs Outdated Show resolved Hide resolved
Co-Authored-By: andresilva <andre.beat@gmail.com>
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

I'm in favor of merging this, good work Andre 👍

optimization(s) can be fixed in follow-up PRs

@niklasad1 niklasad1 added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Feb 19, 2019
@andresilva
Copy link
Contributor Author

I removed the verification that triggered the compile error, it wasn't working in the first place the reason it wasn't being triggered is because the feature flags were using different names. As far as I understand since json_tests is a part of ethcore it will be compiled in non-test builds (e.g. evmbin directly uses json_tests functionality).

@andresilva andresilva added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Feb 19, 2019
@5chdn 5chdn merged commit b457f46 into master Feb 20, 2019
@5chdn 5chdn deleted the andre/progpow branch February 20, 2019 09:05
@5chdn
Copy link
Contributor

5chdn commented Feb 20, 2019

🎉

ordian added a commit that referenced this pull request Apr 5, 2019
* master:
  fix(trace_main! macro): don't re-export (#10384)
  exchanged old(azure) bootnodes with new(ovh) ones (#10309)
  ethash: implement Progpow (#9762)
  snap: add the removable-media plug (#10377)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants