-
Notifications
You must be signed in to change notification settings - Fork 419
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
fix: bug with unaligned short read #1557
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
assert_eq!(num_read, len as usize); | ||
debug_assert!(num_read <= len as usize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This debug_assert
will get compiled into the guest. We should instead make the assert test the more accurate condition we need to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this was just changed because one of the tests doesn't uphold this (since it does a valid short read). Not sure why I switched it to a debug assert though, that was a mistype
Happy to refactor this if that test specifically is kept.
After discussion, we confirmed this issue but clarified the expected behavior. When the guest requests more data than the host provides, the circuit constrains the copy-in will overwrite exactly as many words as the number of bytes requested, rounded down to the nearest multiple of |
@@ -44,6 +44,14 @@ fn main() { | |||
let args: Vec<String> = std::env::args().collect(); | |||
risc0_zkvm::guest::env::commit(&args); | |||
} | |||
"BUF_READ" => { | |||
use std::io::BufRead; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go to the top of file
let mut reader = | ||
std::io::BufReader::with_capacity(capacity, risc0_zkvm::guest::env::stdin()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A use would be good here
Looks like the Ethereum CI is failing with an issue on recursive checkout. I am going to look into that issue. Also, I feel quite confident that this PR is not at risk of breaking the |
So what's currently happening is when requesting an unaligned amount of bytes and the read is short, such that it crosses a word boundary, the guest will overwrite the last word with the lastword return from the host. This is a workaround to avoid changing the host.
Right now what this does is:
and then the last word writes over the end of the buffer, zeroing the bytes written in the last word until aligned:
Alternative being the host doesn't statically set the guest memory based on
#requested.floor_div(4)
, but instead only create aSyscallRecord
with the full words written, and re-use this fill from last word logic.where currently what it does in this case (from changes in this PR) is just removing writing the lastword if a short read past a word boundary:Edit: this functionality was changed to write zero bytes in the lastword slot, such that zeroes are filled until the request lengthNote that with this proposed implementation, as well as the current implementation, if a lot more bytes are requested than available, there are more words in the
SyscallRecord
created than necessary, setting all of the buffer bytes to 0. Unclear to me how much of an impact this has on proving.I don't know what is more ideal, but hope this workaround or at least one of these tests is helpful. Those tests were just created to help narrow down the issue, happy to change them if this PR actually comes through :)