Fix two TODOs #1090
Fix two TODOs #1090
Conversation
| .wrapping_add(s0) | ||
| .wrapping_add(w[i - 7]) | ||
| .wrapping_add(s1); | ||
| // TODO: why doesn't sp1 use wrapping_add? |
There was a problem hiding this comment.
Succint uses manual + operators that wrap in release mode and panic in debug. This is a hashing function so wrapping is the right intent IMO.
| // TODO(Matthias): this is a bit inefficient, | ||
| // we could fill whole u32 words at a time. | ||
| // But it's just for testing. |
There was a problem hiding this comment.
Fills 4 bytes at once. Shouldn't matter because the function is a mock but regardless 🤷
ceno_rt/src/lib.rs
Outdated
| // The random numbers here are only good enough to make eg | ||
| // HashMap work. | ||
| fn step() -> u32 { | ||
| static X: AtomicU32 = AtomicU32::new(0xae569764); |
There was a problem hiding this comment.
This crate provides the runtime for programs running on the Ceno VM.
IIUC it is OK to use atomics in this scenario. If so, static mut is unnecessary.
| #[unsafe(no_mangle)] | ||
| #[linkage = "weak"] | ||
| pub unsafe extern "C" fn sys_rand(recv_buf: *mut u8, words: usize) { | ||
| unsafe fn step() -> u32 { |
There was a problem hiding this comment.
Do you have any insights on:
- the difference in generated assembly code?
- whether using AtomicU32 introduces any additional sys-calls?
There was a problem hiding this comment.
https://godbolt.org/z/TP3TsE91T shows the usage of the lock instruction, which is a native CPU feature (not a syscall). Optimized builds are much smaller -> https://godbolt.org/z/ojY14joaY
The current code has a similar optimized structure -> https://godbolt.org/z/3TEoa3PbP
AtomicU32 is more about removing one layer of unsafe but I can revert back to static mut if you want.
There was a problem hiding this comment.
Since we are working on riscv zkvm so one of the concern is the emulator are limited to support native cpu feature in particular for those which is more bind to hardware, such as lock, as we need to "prove" lock behaviour with a dedicated zk circuit which is hard to implemented. So I would suggest retain static mut behaviours as it doesn't introduce instruction outside of riv32im
There was a problem hiding this comment.
Sure, no problem! Thanks for the review
|
The code should be in good shape now. I am trying to get more hands-on experience with ZK so can you indicate something feasible/solvable for someone new? Looking at the issues alone didn't provide much context. |
2 participants
Also swaps
FIXMEwithTODOfor consistency