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

Rust implementations to replace data tables (#161) #482

Merged
merged 1 commit into from Feb 25, 2016

Conversation

peterjoel
Copy link
Contributor

A few points to discuss, as it's my first PR to this project:

I introduced a dependency on Primal. I couldn't see a better alternative and could never get the same speed without it.

My tests look at the highest and lowest values found in the original hard-coded data tables, with extra cases on the step boundaries. The functions will support much higher input values, which I didn't search for corresponding test values for, since they would only be needed 20 years in the future.

I wasn't sure about which constants to use versus introducing new ones. For example the python code I borrowed from Ethereum had HASH_BYTES = 64, while this project had a constant NODE_BYTES with the same value. I'm not confident enough of what these values actually mean to know if it would be correct to use them to mean the same thing.

The codebase seems to have a lot of files with lines ending in whitespace. My editor is correcting those (I think this is what is happening at least), so my commits are riddled with unnecessary whitespace changes. I'm not 100% sure how to prevent this, short of manually modifying all my commits afterwards. I left them in for now, so you can see. For comparison, the Servo project does not permit trailing whitespace at all, which prevents this issue.

@parity-cla-bot
Copy link

It looks like @peterjoel hasn't signed our Contributor License Agreement, yet.

Appreciation of efforts,

clabot

@peterjoel peterjoel force-pushed the issue_161 branch 2 times, most recently from b4998e2 to a127d8b Compare February 20, 2016 15:22
@@ -34,6 +34,10 @@ pub const ETHASH_MIX_BYTES: usize = 128;
pub const ETHASH_ACCESSES:usize = 64;
Copy link
Contributor

Choose a reason for hiding this comment

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

double space.

@gavofyork
Copy link
Contributor

logic and tests look reasonable. more attention needed to consistency of whitespace.

@gavofyork gavofyork added the A4-clasignoffneeded 📛 Pull request requires author to sign off on CLA before review/merge can begin. label Feb 20, 2016
@tomusdrw
Copy link
Collaborator

@peterjoel Try running rustfmt with those settings: https://github.com/ethcore/parity/blob/a8045ed37ea09d6650e871fdfd35836ec84ab67a/rustfmt.toml
should fix all whitespace issues.

@arkpar
Copy link
Collaborator

arkpar commented Feb 20, 2016

looks good to me

Implementations of get_cache_size and get_data_size in Rust (issue openethereum#161)
Removed sizes module, containing replaced data tables

Fixed whitespace issues after code review
@gavofyork
Copy link
Contributor

@ethcore-cla-bot help

@gavofyork
Copy link
Contributor

[clabot:check]

@parity-cla-bot
Copy link

It looks like @peterjoel hasn't signed our Contributor License Agreement, yet.

Appreciation of efforts,

clabot

@peterjoel
Copy link
Contributor Author

Where can I find the CLA? The bot itself doesn't provide any links and there is no mention of it in the project's README.

@gavofyork
Copy link
Contributor

sorry about that - clabot appears to have misconfigured itself.

link is https://cla.ethcore.io/

@peterjoel
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

@ethcore It looks like @peterjoel signed our Contributor License Agreement. 👍

Many thanks,

Ethcore CLA Bot

@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A4-clasignoffneeded 📛 Pull request requires author to sign off on CLA before review/merge can begin. labels Feb 23, 2016
NikVolf added a commit that referenced this pull request Feb 25, 2016
Rust implementations to replace data tables (#161)
@NikVolf NikVolf merged commit b6a6bc0 into openethereum:master Feb 25, 2016
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants