Skip to content
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

Move byte-checks of Keccak to Syscall side and include lookups for length bytes #2276

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

querolita
Copy link
Member

@querolita querolita commented May 31, 2024

This PR aims at constraining the byte-ness of the length prefix read from the oracle in the first few bytes before reading the actual preimage bytes.

In order to simplify the Keccak lookups, I also moved the actual preimage byte-checks from the Keccak side to the MIPS side (this means 4 lookups per SyscallReadPreimage row instead of 200 lookups per absorb row).

Together with the length bytes checks, this means 8 lookups derived from the request_preimage_write() function.

I made this PR because I think it is good for soundness, but I am sure there's a better way to do it. In particular, it's absurd to perform 8 lookups when you know at most you read 4 bytes. We should think of a better way to "merge" length and preimage bytes in the same vector instead of having one for each.

I am unsure if the 8 bytes of the preimage are always read in two rows of 4 bytes each, or if instead it could combine things like: first call 3 bytes, second call 3 bytes, third call 2 bytes of length and 2 bytes of preimage. Of course, if it always read 4 and 4, then the length bytes and preimage bytes would never have to "share" the vector, as they would always live in separate rows. And then removing the extra 4 lookups is just trivial.

After checking unit tests, the 8 bytes of length can be read in combination with preimage bytes, as it depends on the address (whether or not it is aligned).

Nonetheless, if that is not the case, we might want to think of smarter ways to combine the vector in a way that is compatible with the preimage chunk computation and the rest of preimage constraints.

@querolita
Copy link
Member Author

Checked demo, this does not halt the VM and the SyscallReadPreimage constraints hold.

@dannywillems
Copy link
Member

I'm waiting for 2274 and 2305 to be merged to fix conflicts and continue reviewing. Taking care of solving conflicts.

Base automatically changed from zkvm/mips/fix-syscall-preimage to master June 17, 2024 18:21
@dannywillems
Copy link
Member

Solving conflicts now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants