-
Notifications
You must be signed in to change notification settings - Fork 413
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: ensure witness size is power of two #338
base: main
Are you sure you want to change the base?
fix: ensure witness size is power of two #338
Conversation
@DeVikingMark thanks for taking a pass at this. Looks like tests are failing at the moment, however. |
by chance, do these changes seem alright to you? |
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.
Thanks for this, looks good to me.
@DeVikingMark looks like it needs some formatting + clippy fixes. |
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.
Withdrawing acceptance till pending tests are fixed, but overall design still looks good.
@DeVikingMark what is the reason for removing an existing test? Also, please run |
Summary: Originally, the MemAccessSize value is `n - 1` for short masking code, turn out we didn't use, let's revert it back to original size. Test Plan: Co-authored-by: duc-nx <>
Summary: Originally, the MemAccessSize value is `n - 1` for short masking code, turn out we didn't use, let's revert it back to original size. Test Plan: Co-authored-by: duc-nx <>
Hey Samuel, I've made a change:
Note: The cargo clippy command shows errors related to the pprof library on Windows, but these are unrelated to my code changes. |
@DeVikingMark can you answer:
|
@sjudson I've deleted them by accident, and forgot to add it, sorry that's my fault I've made some updates, probably now everything should be fine, I'll be waiting for tests to be completed |
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.
Almost there, but there is still some inconsistent indentation.
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.
LGTM pending tests passing...
@DeVikingMar clippy + fmt lints are still failing |
@sjudson Could it be that these errors are not related to my changes and were here before my intervention? |
Fixes #287
Added check to ensure witness size is power of two in
CRR1CSProof::prove()
function to prevent potential index out of bounds error when reading from empty vector.Changes made:
This prevents the issue where reading index of zero from an empty vector could occur if the witness size is not a power of two.