-
Notifications
You must be signed in to change notification settings - Fork 84
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
ProgPoW's handling of Ethash's uneven DAG size is underspecified #31
Comments
It has no effect on DAG generation where |
DAG generation creates all nodes as 128. |
Ethash DAG sizes are multiples of 128 bytes, but not of 256. ProgPoW's
README.md
says:ProgPoW accesses the DAG in 256-byte chunks, which is a documented change from Ethash. However, it doesn't specify how it's handling the last element of Ethash's original DAG, which is only 128 bytes long. Is that last element perhaps/hopefully never accessed? This needs to be specified fully, and implementations checked for potential unintended behavior.
Also, ProgPoW's bundled implementation of Ethash's DAG initialization has
ETHASH_MIX_BYTES
changed from 128 to 256. Since the DAG is supposed to be unchanged from Ethash's, this change is probably unneeded (at least for the host-side DAG initialization code that's also unused in the ProgPoW tree?), but it breaks the included (and also unused?)ethash_hash()
.The
ETHASH_MIX_BYTES
macro is reused by the CUDA and OpenCL DAG initialization code, but I guess it's not supposed to make a difference there as well? If so, should it perhaps be reverted to 128 to emphasize that the DAG is indeed unchanged from Ethash's? Or is the change to 256 needed e.g. not to waste resources on computing the last (unused?) 128 bytes? I suggest adjusting the code or comments to address or avoid these questions right in there.In the plain C implementation of ProgPoW I'm currently playing with, I left Ethash's DAG initialization as-is (with
ETHASH_MIX_BYTES
at 128) and it's producing the correct (matching yourtest-vectors.md
) digest for block 30k.The text was updated successfully, but these errors were encountered: