Skip to content
This repository was archived by the owner on Apr 20, 2020. It is now read-only.

Configurable check for valid Equihash <n,k> solutions#2

Merged
josephnicholas merged 5 commits into
HorizenOfficial:zencash-developmentfrom
litecoinz-project:master
Jun 14, 2018
Merged

Configurable check for valid Equihash <n,k> solutions#2
josephnicholas merged 5 commits into
HorizenOfficial:zencash-developmentfrom
litecoinz-project:master

Conversation

@litecoinz-project

@litecoinz-project litecoinz-project commented Jun 6, 2018

Copy link
Copy Markdown
Contributor

This PR, together with those to be applied on node-stratum-pool and z-nomp, allows to install a pool server based on z-nomp able to manage and validate the following equihash solutions:

  • n = 48, k = 5
  • n = 96, k = 3
  • n = 96, k = 5
  • n = 192, k = 7
  • n = 200, k = 9

Dependencies:
z-classic/node-stratum-pool#35
z-classic/z-nomp#348

@albertog78

Copy link
Copy Markdown
Contributor

Hi, the PR reintroduce the equihash c implementation that was substituted for a known vulnerability.
The same configurable approach should be applied using the current implementation.

@litecoinz-project

Copy link
Copy Markdown
Contributor Author

Hi! Please check now.

@albertog78

Copy link
Copy Markdown
Contributor

Hi! Yes it's ok now.
For backward compatibility it's needed to have the verify method if called with two parameters defaulting to the previous n,k values and seems equihashverify.cc has not been modified to get the n,k parameters dynamically.
The test probably did not detect the missing commit because it was only testing the 200, 9 n k. To prevent this, I would suggest to re-introduce a test without specifying n, k parameters, keep the current one with 200,9 and introducing new ones with other n,k combination.

@tarrenj

tarrenj commented Jun 7, 2018

Copy link
Copy Markdown

Concept ACK.

@litecoinz-project

litecoinz-project commented Jun 7, 2018

Copy link
Copy Markdown
Contributor Author

Sorry I forgot to copy changes from my local repository to github master branch.

@renuzit

renuzit commented Jun 10, 2018

Copy link
Copy Markdown

It maybe out of the scope of this PR, but BTG and BTCZ both have plans to use a different personalization string than ZCash. In BTCZ's case it will change from ZcashPOW to BitcoinZ and BTG will use BgoldPOW. A new issue probably needs to be created to implement a configurable personalization string.

@albertog78

Copy link
Copy Markdown
Contributor

Hi, for backward compatibility it's needed to have the verify method if called with two parameters defaulting to the previous n,k values.
Please update the test to verify results with or without passing n k.

@litecoinz-project

Copy link
Copy Markdown
Contributor Author

Hi! I added backward compatibility and updated the test to verify results with or without passing n, k.

@josephnicholas josephnicholas changed the base branch from master to zcash_implementation June 14, 2018 09:11
@josephnicholas josephnicholas changed the base branch from zcash_implementation to master June 14, 2018 09:12
@josephnicholas josephnicholas changed the base branch from master to zencash-development June 14, 2018 09:15
@josephnicholas

Copy link
Copy Markdown
Contributor

utACK

@josephnicholas josephnicholas merged commit 352bf13 into HorizenOfficial:zencash-development Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants