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

ethash: implement Progpow #9762

Merged
merged 53 commits into from Feb 20, 2019

Conversation

@andresilva
Copy link
Member

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 changed the title ethcore, ethash: implement Progpow ethash: implement Progpow Oct 16, 2018

@andresilva andresilva force-pushed the andre/progpow branch from 5a7c7fa to 7303491 Oct 16, 2018

@andresilva

This comment has been minimized.

Copy link
Member Author

andresilva commented Oct 16, 2018

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 Oct 17, 2018

@holiman

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

andresilva commented Oct 18, 2018

@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 force-pushed the andre/progpow branch from 6d6465a to 6952091 Oct 18, 2018

@andresilva

This comment has been minimized.

Copy link
Member Author

andresilva commented Feb 18, 2019

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

This comment has been minimized.

Copy link
Member Author

andresilva commented Feb 19, 2019

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.

@cheme

cheme approved these changes Feb 19, 2019

Copy link
Contributor

cheme left a comment

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

This comment has been minimized.

@niklasad1

niklasad1 Feb 19, 2019

Member

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?

This comment has been minimized.

@andresilva

andresilva Feb 19, 2019

Author Member

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

@andresilva

This comment has been minimized.

Copy link
Member Author

andresilva commented Feb 19, 2019

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

@andresilva

This comment has been minimized.

Copy link
Member Author

andresilva commented Feb 19, 2019

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.

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

niklasad1 left a comment

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

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

@andresilva

This comment has been minimized.

Copy link
Member Author

andresilva commented Feb 19, 2019

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

@5chdn 5chdn merged commit b457f46 into master Feb 20, 2019

3 checks passed

continuous-integration/gitlab-build-android Build stage: optional; status: success
Details
continuous-integration/gitlab-test-audit Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux Build stage: test; status: success
Details

@5chdn 5chdn deleted the andre/progpow branch Feb 20, 2019

@5chdn

This comment has been minimized.

Copy link
Contributor

5chdn commented Feb 20, 2019

🎉

@holiman holiman referenced this pull request Feb 22, 2019

Open

Progpow integration #17731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.