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.

Sign up for GitHub

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

Jump to bottom

[develop] Reset feature flags before assertion + tests #14405

Merged
merged 10 commits into from
Jan 29, 2024
Merged

[develop] Reset feature flags before assertion + tests #14405

merged 10 commits into from
Jan 29, 2024

Conversation

mrmr1993
Copy link
Member

This is #14402, with some additional tests using infrastructure that isn't available in that branch, as well as a temporary hack around a bug in proof-systems to make the test work. This also carries some changes from the merge with it.

From #14402:

This PR changes the handling of feature_flags in Pickles for wrap circuits. Previously, we were propagating the flags from the step circuit, which could cause a sanity-check assertion to fail when we convert the 'full' set of values back to the 'wrap only' set of values. This PR fixes that by always resetting the flags before entering that conversion.

This PR does not change any circuits, protocol constants, etc.; the behavior is only changed when those offending flags are set. As such, this can be deployed to snark workers as a compatible bugfix version.

Tests to follow.

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them

@mrmr1993 mrmr1993 requested review from a team as code owners October 20, 2023 21:01
@mrmr1993 mrmr1993 changed the title Feature/custom gate zkapp test [develop] Reset feature flags before assertion + tests Oct 20, 2023
@mrmr1993
Copy link
Member Author

!ci-build-me

jspada
jspada approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@jspada jspada left a comment

Choose a reason for hiding this comment

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

Approved with one minor thing caught with sign coefficient. Other than that a couple questions and suggestions.

src/app/zkapps_examples/test/optional_custom_gates/zkapp_optional_custom_gates_tests.ml

let tag, _cache_handle, proof, Pickles.Provers.[ prove ] =
Zkapps_examples.compile ~auxiliary_typ:Typ.unit
~branches:(module Nat.N1)
Copy link
Contributor

Choose a reason for hiding this comment

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

aside: maybe we should call this parameter choice_count instead of branches or rename choices to branches and call it branch_count

src/lib/gossip_net/libp2p.ml Outdated
@@ -217,7 +217,7 @@ module Make (Rpc_intf : Network_peer.Rpc_intf.Rpc_interface_intf) :
| Mina_net2.Libp2p_helper_died_unexpectedly ->
on_unexpected_termination ()
| _ ->
raise exn
Exn.reraise exn "Mina_net2 raised an exception"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is unrelated?

src/lib/pickles/fix_domains.ml Outdated
range_check0 || range_check1 || rot
in
let range_check_table_used_again =
(* FIXME: This is a hack around a bug in proof-systems. *)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened here?

...ib/pickles/test/optional_custom_gates/test_gadgets/pickles_optional_custom_gates_circuits.ml
; foreign_field_modulus0 = Field.Constant.of_int 1
; foreign_field_modulus1 = Field.Constant.of_int 0
; foreign_field_modulus2 = Field.Constant.of_int 0
; sign = Field.Constant.of_int 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right to me. The sign should be +1 (for addition) or -1 (for subtraction).

deepthiskumar
deepthiskumar approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@deepthiskumar deepthiskumar left a comment

Choose a reason for hiding this comment

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

Changes in app/zkapps_examples/test look good

@mergify mergify bot mentioned this pull request Nov 3, 2023
Closed
@mitschabaude mitschabaude mentioned this pull request Nov 3, 2023
Merged
@mrmr1993
Copy link
Member Author

This now includes o1-labs/proof-systems#1334

@mitschabaude mitschabaude mentioned this pull request Nov 16, 2023
Merged
dannywillems
dannywillems approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Bumping others approval (did not review). Only a comment: was there not a decision to go with Alcotest instead of ppx_inline_tests?

@rbonichon
Copy link
Contributor

was there not a decision to go with Alcotest instead of ppx_inline_tests?

I recall we agreed this would be nicer. At this point, it might be better to change the framework in one go

@mrmr1993
Copy link
Member Author

!ci-build-me

@mrmr1993
Copy link
Member Author

!ci-build-me

@mrmr1993
Copy link
Member Author

!ci-build-me

@mrmr1993 mrmr1993 merged commit 4a05e19 into develop Jan 29, 2024
40 checks passed
@mrmr1993 mrmr1993 deleted the feature/custom-gate-zkapp-test branch January 29, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepthiskumar deepthiskumar deepthiskumar approved these changes

@dannywillems dannywillems dannywillems approved these changes

@jspada jspada jspada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mrmr1993 @rbonichon @jspada @dannywillems @deepthiskumar