refactor: make sha3 exterior interface unmodified in patch#9
Conversation
gdmlcjs
force-pushed
the
chore/sha3-v0.10.8-patch
branch
from
e10bc81 to
f8daa2a
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn p1600(state: &mut [u64; PLEN], round_count: usize) { | ||
| #[cfg(target_os = "zkvm")] | ||
| if round_count == DEFAULT_ROUND_COUNT { | ||
| unsafe { | ||
| openvm_keccak256_guest::native_keccakf(state.as_mut_ptr() as *mut u8); | ||
| } | ||
| return; | ||
| } | ||
| keccak::p1600(state, round_count); | ||
| } |
There was a problem hiding this comment.
📝 Info: p1600 cfg pattern: native acceleration only for 24-round keccak-f
The p1600 function at sha3/src/state.rs:24-33 uses #[cfg(target_os = "zkvm")] on the if statement only, meaning the keccak::p1600(state, round_count) call at line 32 is compiled on ALL targets. On zkvm, when round_count == DEFAULT_ROUND_COUNT (24), it uses native_keccakf and returns early; otherwise it falls through to the software keccak::p1600. This correctly handles TurboSHAKE (12 rounds) by falling back to software, and is not a bug — but the pattern is subtle. A future maintainer adding a new cfg branch or reordering could easily break the fallthrough semantics. A comment explaining the intentional fallthrough would improve maintainability.
Was this helpful? React with 👍 or 👎 to provide feedback.
jonathanpwang
left a comment
jonathanpwang
left a comment
There was a problem hiding this comment.
LGTM but please add the comment explaining the p1600 function's conditional compilation as Devin suggested.
Also same note about follow-up PR to add zkvm CI.
|
okay, just added the comment. also made a linear ticket for the CI |
2 participants
Refactors the sha3 patch to use the lower level
openvm-keccak256-guestcrate instead of theopenvm-keccak256crate.The internal implementations are swapped to use the openvm functions for the zkvm target. The
native_keccakfandnative_xorinfunctions are used everywhere in all of the sha3 hashes (Sha3, Keccak, Shake, TurboShake, CShake) except for the 12-round permutation inTurboShake.The hashing structs and traits are now untouched so that the interface of the patched version is equivalent to the upstream version.