keccak-air generate_trace_rows_for_perm uses a non-standard state
#672
Comments
|
Hmm, happy to try and fix this (And I agree adding some unit tests would be good). Would it be enough to just add a transpose to |
|
I believe all you need to do is change in Plonky3/keccak-air/src/generation.rs Line 53 in 5d8115d current_state[x][y] to current_state[y][x].
I can make the change, but I wouldn't be able to immediately have time to add the code for unit tests. |
|
Hi , i've just saw this issue and have checked this line : it remains the same until now , can i try to implement the unit test for this ? |
|
Sure that would be great. The main thing to ensure is that proving times for the KECCAK examples don't change. E.g. running: Should successfully run the prover/verifier and take roughly the same amount of time. |
Plonky3/keccak-air/src/generation.rs
Line 50 in 5d8115d
Plonky3/keccak-air/src/generation.rs
Line 53 in 5d8115d
After #621, the
generate_trace_rows_for_permnow takesstate: [u64;25]and considers it as a[x][y]5x5 matrix whereindex = x * 5 + y. You then setpreimage[y][x] = state[x][y], so thatpreimage[y][x] = state[x * 5 + y]. However in the official keccak spec (https://keccak.team/keccak_specs_summary.html), the absorb is done usingstate[x + 5 * y].This does not change the correctness of the keccak-f AIR, since you are still permuting a state correctly, but it does affect the overall API and compatibility with usage of this AIR in any implementation of the full keccak hash function. (I discovered this in trying to update OpenVM's keccak256 AIR to the new commit.)
I have checked and believe that
tiny-keccak'skeccakf(state: [u64;25])function does take the indexing viastate[x + 5 * y]. I suggest that thegenerate_trace_rows_for_permAPI is changed to matchtiny-keccak: the fix is simple, just change the ordering in L53.Moreover from this I noticed that there are no unit tests of the keccak-air on the trace generation's outputs against either known test vectors or against a known implementation such as
tiny-keccak. I would suggest adding such unit tests so these issues are caught more easily (and also to show the expected API).The text was updated successfully, but these errors were encountered: