-
Notifications
You must be signed in to change notification settings - Fork 58
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
Slot allocation of bit decomposition #738
Conversation
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 a lot! I added a few commentaries, the main one being about documentation.
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 is ok by me, but let's wait for more reviews
); | ||
} | ||
let AllocatedVal::Boolean(lt) = lt else { panic!("Expected boolean") }; | ||
// To find out whether a < b, we will use the following reasoning: |
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.
@gabriel-barrett Since I have you here, perhaps you'd care to give a review to #563 ?
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.
My point being that it informs details and limitations of the less_than semantics.
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.
and I've added the final comparison to these tests.
abade4d
to
c598b7b
Compare
- add a direct comparison from argumentcomputer#738
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 LGTM, thanks a bunch!
); | ||
} | ||
let AllocatedVal::Boolean(lt) = lt else { panic!("Expected boolean") }; | ||
// To find out whether a < b, we will use the following reasoning: |
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.
and I've added the final comparison to these tests.
@@ -55,56 +49,56 @@ impl<F: LurkField> VarMap<Val<F>> { | |||
} | |||
|
|||
#[derive(Clone, Debug, Default)] | |||
/// `Preimages` hold the non-deterministic advices for hashes and `Func` calls. | |||
/// `Advices` hold the non-deterministic advices for hashes and `Func` calls. |
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.
Advices
is a bit of an awkward formulation. Maybe this could be Hints
?
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.
Looks good, but please see naming suggestion.
- add a direct comparison from argumentcomputer#738
- add a direct comparison from argumentcomputer#738
- add a direct comparison from argumentcomputer#738
* test: Enhance `Num<Fr>` testing - Enhanced property testing coverage and introduced new tests for `U64` and `Scalar` types. - Provided comprehensive test scenarios including basic operations, assertions, checking sign, and "lesser" properties. - Improved coverage for scalars and u64, encompassing overflow/underflow cases and edge conditions. Closes #52 * refactor: Refine numeric comparison in `prop_lesser` function - add a direct comparison from #738
This PR replaces slot allocation for "less-than" operations with slot allocation for bit decomposition.
Slot allocating bit decomposition allows us to reuse bit decomposition in truncate operations, leading to 1k less constraints/auxiliaries