-
Notifications
You must be signed in to change notification settings - Fork 5
Equihash updates. #4
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
base: master
Are you sure you want to change the base?
Conversation
- Expose default engine setting. - Proxy main calls to default engine.
- Refactor to make data easier to access. - Fix k check comment.
- Switch verify() to use a callback. - Process verify() with AsyncWorker. - Add more tests.
- Check value length is correct for k. - Check indices are distinct. - Check indices are ordered properly.
- Add API docs. - Use "seed" vs "inputs". - Use "solution" vs "value". - Add "nonceLength" option. - Add "maxNonces" option. - Add "nonce" option. - Return solution as native JS array. - Add sanity checks. - Allow any length seed. - Remove "Seed" class. - Partial handling of arbitrary length nonces. - Partial ability to start at any nonce. - Use streaming blake2b API. - Adapt tests that depended on older fixed sizes. - Comment out odd length seed tests. - Misc cleanups.
- Add "personal" bytes support, as used by reference blake2b implementation. - Add support function to create zcash personal bytes. - Minimize params passed around in addon. - Add all data to created Proof.
- Support buffers for nonces. - Allow solution as number if nonceLength provided. - Current nonce limitations that could be lifted: - size >= 4 bytes. - only first 4 bytes mutated as a counter. - Don't return seed in result proof.
| - `personal`: buffer of "personal" bytes used in some blake2 initializations | ||
| (optional) | ||
| - `nonce`: initial value of nonce (engine dependent type) (optional) | ||
| - `maxNonces`: max number of nonces to check (optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Engine name specific options should be nested under the engine name. Potentially also nested under "engine".
README.md
Outdated
| `proof`: | ||
| - `n`: equihash `n` parameter | ||
| - `k`: equihash `k` parameter | ||
| - `personal`: buffer of bytes used for "personal" bytes (engine dependent, optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Engine specific output should be nested under the engine name as a property. Potentially also nested under "engine".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the engine name itself should also be included in the proof -- as an "algorithm". So "algorithm": "khot...". Then, from that, "algorithmParameters" can include any algorithm-specific parameters like "personal".
README.md
Outdated
| `proof`: same as output from `solve()` | ||
|
|
||
| `options` (engine specific): | ||
| - `personal`: buffer of "personal" bytes used in some blake2 initializations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments above regarding engine specific information.
lib/index.js
Outdated
| return engine().verify(...arguments); | ||
| }, | ||
| get PERSONALBYTES() { | ||
| return engine().PERSONALBYTES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems engine/hash specific... why is this exposed as a core constant?
lib/index.js
Outdated
| get PERSONALBYTES() { | ||
| return engine().PERSONALBYTES; | ||
| }, | ||
| zcashPersonal(n, k) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be _zcashPersonal as a private helper thing we're not exposing as part of the public API right now until we figure it out better. I would instead expect to find this, in the future, under .engine('zcash').PERSONAL_BYTES (we aren't implementing it now).
| const b = Buffer.from(ab); | ||
| b.fill(0); | ||
| const dv = new DataView(ab); | ||
| dv.setUint32(0, nonce, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is big-endian, yay 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nonce is written here as little-endian. The nonce output is just arbitrary bytes so it doesn't really matter. This is just initializing the starting point to solve from and using little-endian as a convention mostly because zcash was doing so with its 256 bit nonce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I was just crazy when I read this before. Endianness doesn't matter here, but it does matter with how the solution is serialized ... but that isn't done in this lib.
| const stride = 1 << _k; | ||
| for(let i = 0; i < solution.length; i += (2 * stride)) { | ||
| if(solution[i] >= solution[i + stride]) { | ||
| return callback(null, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why false here instead of an invalid solution? Where is the line between an "invalid" solution and a "false" one? I would have thought that a solution that is properly ordered, has no duplicates -- but is just wrong, is "false" ... anything else is invalid and generates an error. Not saying we have to do it that way, just saying that was my expectation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's open for debate where the line should be between an error and just a false return value. Or if it should all be one way or the other. Currently errors are mostly out of range or similar vs just bad solution data. Can revisit this as needed.
- Add 'algorithm' field, return in proof. - Add 'algorithmParamters' field, return in proof. - Move personalization to algorithmParamters. - Many updates to README. - Guess engine based on algorithm. - Move personalization constant to khovratovich engine. - Use more specific errors.
- Fix benchmark.js usage. Use onStart and onCycle to change seed. - Add predictable random values and crypto random values. - Print out more stats: min/max/mean/etc. - Due to large variations based on input, ensure at least 10 samples.
- Check solutions as they are found. - Order and return first solution that passes checks. - Small refactorings.
- Use stdint.h vs cstdint. - Define used endian.h macros.
C++11 initialization can cause crazy runtime behavior with older compilers. Change to use older initialization style.
No description provided.