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

Distinguish between integer, field element and bool. #910

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

chriseth
Copy link
Member

No description provided.

@chriseth chriseth marked this pull request as ready for review January 15, 2024 16:50
@chriseth chriseth marked this pull request as draft January 15, 2024 16:51
@chriseth chriseth force-pushed the integers branch 7 times, most recently from 765b9b8 to f96cdfc Compare January 22, 2024 15:21
@chriseth chriseth changed the base branch from main to builtins January 22, 2024 15:22
@chriseth chriseth marked this pull request as ready for review January 23, 2024 08:28
@chriseth chriseth force-pushed the integers branch 3 times, most recently from 7b1bfea to 40ba2f0 Compare January 23, 2024 10:14
@chriseth chriseth force-pushed the integers branch 2 times, most recently from 8c3274e to 47ae649 Compare January 23, 2024 14:07
@chriseth chriseth force-pushed the builtins branch 2 times, most recently from c5e5597 to 3e3349d Compare January 23, 2024 19:30
Base automatically changed from builtins to main January 23, 2024 20:57
@chriseth chriseth changed the title Distinguish between integer and field element. Distinguish between integer, field element and bool. Jan 24, 2024
@chriseth chriseth changed the base branch from main to print January 24, 2024 07:25
@@ -229,23 +232,23 @@ namespace Arith(N);
// I have no idea where the "- 4 * prepend_zeros(p, 16)" comes from, but it was in the original constraints.
let product_with_p = (|| |x| |nr| product(p, x)(nr) - 4 * prepend_zeros(p, 16)(nr))();

let eq1 = (|| |nr| product(sf, x2f)(nr) - product(sf, x1f)(nr) - y2f(nr) + y1f(nr) + product_with_p(q0f)(nr))();
let eq1 = (|| |nr| std::field::modulus() + product(sf, x2f)(nr) - product(sf, x1f)(nr) - y2f(nr) + y1f(nr) + product_with_p(q0f)(nr))();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I need to check if we still need this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still needed.

Base automatically changed from special_identifiers to main January 24, 2024 13:22

Powdr-pil is usually side-effect free, but there are some situations where side-effects happen.
This includes the `std::debug::print` built-in function. Expressions are usually evaluated
left-to right and eagerly, but it is best to not rely on this behaviour.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be exhaustive on the exceptions

The lambda expression `|| 7` is a function that returns a constant (has no parameters).
Lambda functions can capture anything in their environment and thus form closures.

Due to the dynamic typing of powdr-pil, the exact type of a lambda expression can often not be determined.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds confusing. Why does it do dynamic typing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because nobody found the time to implement a proper generic type checker. This will be replaced once we have a proper type checker. Thibaut told me that I should not document what we plan but what we have...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea but still, we don't do dynamic typing, it's all compile-time

Copy link
Member Author

@chriseth chriseth Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it depends on what you mean by compiling. But even witness generation uses dynamic typing since you can call functions from the query callbacks and those functions could look like

|i| match i {
 0 => [],
 1 => 7,
 _ => (1,2)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wat, really?! Can we stop that :D

@chriseth chriseth force-pushed the integers branch 8 times, most recently from 1713e69 to 007b605 Compare January 25, 2024 12:14
@@ -55,7 +55,7 @@ machine Main {
assert_eq C, 14940024623104903507;
assert_eq D, 10772674582659927682;

A, B, C, D <== poseidon(18446744069414584321, 18446744069414584321, 18446744069414584321, 18446744069414584321, 18446744069414584321, 18446744069414584321, 18446744069414584321, 18446744069414584321, 0, 0, 0, 0);
A, B, C, D <== poseidon(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it panic otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the number is the goldilocks modulus. So the test should do exactly the same as it did before.

@georgwiese any reason you chose these numbers (and used explicit zeros for the last four elements)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test vectors are the same as polygon's. I guess they were chosen exactly to test that the result is the same as if you change the inputs to 0s. The last 4 are 0s because they're the cap that are used to "change" the hash function

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, they come from the JSON linked in the comment a few lines above.

leonardoalt
leonardoalt previously approved these changes Jan 25, 2024
@chriseth chriseth force-pushed the integers branch 4 times, most recently from a6075b9 to 5532b6a Compare January 25, 2024 15:14
@chriseth chriseth changed the base branch from main to safe_conversions January 25, 2024 15:14
Base automatically changed from safe_conversions to main January 25, 2024 16:33
@leonardoalt leonardoalt added this pull request to the merge queue Jan 25, 2024
Merged via the queue into main with commit 1fda7e8 Jan 25, 2024
2 checks passed
@leonardoalt leonardoalt deleted the integers branch January 25, 2024 20:04
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.

3 participants