ifdefelse / ProgPOW

A Programmatic Proof-of-Work for Ethash. Forked from https://github.com/ethereum-mining/ethminer
GNU General Public License v3.0
257 stars 84 forks source link

22 Keccak rounds instead of 24 ? #32

Closed AndreaLanfranchi closed 5 years ago

AndreaLanfranchi commented 5 years ago

Unless I am missing something Keccak_f800 performs only 22 permutation rounds while instead they should be 24. Round constants indexed 22 and 23 get never applied.

lookfirst commented 5 years ago

https://github.com/ifdefelse/ProgPOW/blob/b45389b61f3de7a818edb512c3b47ee291a71b6f/libethash-cl/CLMiner_kernel.cl#L96

https://github.com/ifdefelse/ProgPOW/blob/b45389b61f3de7a818edb512c3b47ee291a71b6f/libethash-cuda/CUDAMiner_kernel.cu#L96

Also seems a bit weird. The README says 22, but the code says 21?

AndreaLanfranchi commented 5 years ago

@lookfirst 21 is counter within the loop. Immediately after there is another call with increment

In total are 22 calls to permutation.

lookfirst commented 5 years ago

https://github.com/ifdefelse/ProgPOW/blob/b45389b61f3de7a818edb512c3b47ee291a71b6f/libethash-cl/CLMiner_kernel.cl#L55

st[] > 22 could get updated there?

AndreaLanfranchi commented 5 years ago

No. That one is right.

lookfirst commented 5 years ago

I'm not saying it isn't right. I'm saying... does it matter that s[23],s[24],s[25] isn't set when it is passed into keccak_f800_round?

AndreaLanfranchi commented 5 years ago

The loop you're referring to is the one which processes the state[25] within a keccak round. That one is correct.

What imho is not correct is the fact that keccak_f800_round is called 22 times (instead of 24) thus this line https://github.com/ifdefelse/ProgPOW/blob/b45389b61f3de7a818edb512c3b47ee291a71b6f/libethash-cl/CLMiner_kernel.cl#L76_L78 never applies round constants indexed as 22 and 23

lookfirst commented 5 years ago

Ok. Cool. So I guess the next question I have is what effect this has on the algo. Not being an expert in crypto...

Does it make it less secure? Does it make it slower? Does it give bias anywhere? Does it make it easier to implement asics?

solardiz commented 5 years ago

@AndreaLanfranchi I agree it's weird, and it at least needs to be explained (or corrected).

This doesn't explain it at all, but FWIW per git blame the 22 rounds weirdness was already part of the 7feb8ddbbc6e1bf382acb78d6f13c9475e07c5c4 commit 10 months ago, which first introduced ProgPoW, in both CUDA and OpenCL at once.

@lookfirst It's probably none of those things you mention (and it probably makes it a bit faster, not slower), but the primary problem is it's unexplained weirdness.

ifdefelse commented 5 years ago

22 is the correct number of rounds. From the Keccak spec: https://keccak.team/keccak_specs_summary.html

We first start with the description of Keccak-f in the pseudo-code below. The number of rounds n depends on the permutation width, and is given by n=12+2l, where 2**l=w. This gives 24 rounds for Keccak-f[1600].

We use f800 because it has w=32-bit that matches GPUs register file size. That's l=5 which means n=12+2*5=22 rounds.

solardiz commented 5 years ago

Thanks @ifdefelse! This looks correct to me now. Perhaps drop the last two elements of keccakf_rndc, then?

AndreaLanfranchi commented 5 years ago

Thank you. I honestly didn't go through keccak specs summary. My fault.

Can be closed for me.