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

fix nightly tests #2115

Merged
merged 3 commits into from
Nov 20, 2024
Merged

fix nightly tests #2115

merged 3 commits into from
Nov 20, 2024

Conversation

leonardoalt
Copy link
Member

No description provided.

@@ -879,7 +879,7 @@ namespace main_vm(64..128);
fn permutation_instructions() {
let expected = r#"namespace main(128);
let _operation_id;
query |__i| std::prover::provide_if_unknown(_operation_id, __i, || 13);
query |__i| std::prover::provide_if_unknown(_operation_id, __i, || 9);
Copy link
Member Author

Choose a reason for hiding this comment

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

these were caused by changing the asm test

Copy link
Member

Choose a reason for hiding this comment

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

🃏

.collect::<Vec<_>>()
.into_iter()
.max()
.unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

this function is called for both Composite and Mono proofs, and it assumed all degrees were unique ranges before. This now takes the max of all max degrees for the halo2 setup

instr or X, Y -> Z link ~> Z = bin.or(X, Y);
instr or_into_B X, Y link ~> B' = bin.or(X, Y);
Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately Halo2 has a bug somewhere and we can't do this.
This is testes in other places (including RISCV tests) with witgen

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to disable halo2 for the test instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

we still want to run halo2 proofs for these tests to test permutations between machines

Copy link
Member

Choose a reason for hiding this comment

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

But do we test the previous functionality somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

language/compiler/witgen wise yes, the RISCV machine has this stuff and more

instr or X, Y -> Z link ~> Z = bin.or(X, Y);
instr or_into_B X, Y link ~> B' = bin.or(X, Y);
Copy link
Member Author

Choose a reason for hiding this comment

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

same here re halo2

// instructions activating multiple permutations
instr add_one X -> Y
link ~> B = arith.add(X, 2)
link ~> Y = arith.sub(B, 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

same here and below re halo2

Comment on lines 272 to 273
.collect::<Vec<_>>()
.into_iter()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.collect::<Vec<_>>()
.into_iter()

Does this not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

you pasted the same code

Copy link
Member Author

Choose a reason for hiding this comment

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

ah read it wrong, the suggestion is only removal, let me check

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 works, changed

@chriseth chriseth added this pull request to the merge queue Nov 20, 2024
Merged via the queue into main with commit 82798c8 Nov 20, 2024
14 checks passed
@chriseth chriseth deleted the fix-nightly branch November 20, 2024 10:20
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