Skip to content
This repository was archived by the owner on Dec 17, 2024. It is now read-only.

Optimize Keccak implementation and remove Crypto++ dependency - #47

Open
chfast wants to merge 3 commits into
ethereum:masterfrom
chfast:keccak
Open

Optimize Keccak implementation and remove Crypto++ dependency#47
chfast wants to merge 3 commits into
ethereum:masterfrom
chfast:keccak

Conversation

@chfast

@chfast chfast commented Apr 29, 2015

Copy link
Copy Markdown
Member

As I talked with @LefterisJP and @gavofyork some time ago this is some code from my pet project https://github.com/chfast/ethminer.

Review on Reviewable

Comment thread src/libethash/keccak.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fails on Windows since msvc seems to not havenoexcept.
Can you add something like:

#ifndef _MSC_VER
#define NOEXCEPT noexcept
#else
#define NOEXCEPT
#endif

and use that instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not needed in the function? So you mean you will remove the attribute in general?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep. Compiler will know that without my help.

@LefterisJP

Copy link
Copy Markdown
Contributor

Can you also update python's setup.py?
https://github.com/ethereum/ethash/blob/master/setup.py

sha3.c would need to be renamed to keccak.cpp there too.
You can see that travis fails there on the python build.

@chriseth

Copy link
Copy Markdown
Contributor

Some license headers are missing.
Can we re-license "CC0, attribution kindly requested, by external author" as GPL?

@chfast

chfast commented Apr 29, 2015

Copy link
Copy Markdown
Member Author

Not sure about CC0 - GPL compatibility.

@gavofyork

Copy link
Copy Markdown
Contributor

fails on the CI?

@LefterisJP

Copy link
Copy Markdown
Contributor

Hmmm .. this will also need rebasing now. As for the CI it's the python tests that fail since python's setup.py needs updating after the file changes.

@chfast

chfast commented May 5, 2015

Copy link
Copy Markdown
Member Author

CI fails is because my added file is C++11 and requires -std=c++11 flag and the rest is C that requires -std=gnu99. Python module setup script do not allow you to set separated flags for C and C++.

The solution might be to restrict implementation to C or C++ only.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants