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

Resurrect lookup PR1263 [do not merge] #1489

Closed
wants to merge 11 commits into from

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Dec 8, 2023

This resurrects #1263 which was reverted in #1378 due to the bug that is to be understood within this PR.

Important! Note that the first commit /still/ has the bug MinaProtocol/mina#14657. It will be fixed in the following commits explicitly, so that the bug can be hunted down.

Plan before this can be re-merged:

  • Resurrect the PR as it is, squashing it all into a single commit, resolve conflicts
  • Check that the zero_row_zero_id still makes sense
  • Explicitly revert the "colliding runtime+lookup ids" behaviour.
  • Understand why the panic-within-panic bug happened in mina
  • Make desired changes, confirm the bug does not reproduce anymore, cover it with a test if possible

@volhovm volhovm changed the title Resurrect lookup PR1263 [MinaProtocol/mina#14674] Resurrect lookup PR1263 (DO NOT MERGE) Dec 8, 2023
@volhovm volhovm changed the title Resurrect lookup PR1263 (DO NOT MERGE) Resurrect lookup PR1263 [do not merge] Dec 8, 2023
@volhovm volhovm force-pushed the volhovm/mina14674-resurrect-lookups-pr branch 3 times, most recently from 56c9985 to 779d4f6 Compare December 8, 2023 13:07
@volhovm
Copy link
Member Author

volhovm commented Dec 8, 2023

@dannywillems I confirm that the test_zero_table_zero_row test that is resurrected within this PR captures the "nonzero row with zero entry" scenario we discussed earlier. In this scenario, the error must be raised, but it is not since has_zero_entry in current master returns false positives (current resurrected PR fixes this).

For the context, the scenario could be alternatively captured by adding such a row into https://github.com/o1-labs/proof-systems/pull/1454/files#diff-30d4661dcab0b5e55e4bb6f27ea2c313bded3d2fc088df9d16f6d0fdde981cc1R603 (by either adding zero into indices, or into values, but not into both).

@dannywillems dannywillems self-assigned this Dec 8, 2023
@dannywillems
Copy link
Member

dannywillems commented Dec 11, 2023

Please split in small PRs. Independent changes must go in different PRs and commits.

@volhovm volhovm force-pushed the volhovm/mina14674-resurrect-lookups-pr branch from 779d4f6 to f770876 Compare January 3, 2024 11:21
@volhovm volhovm force-pushed the volhovm/mina14674-resurrect-lookups-pr branch from f770876 to 07222ac Compare January 3, 2024 11:29
@volhovm
Copy link
Member Author

volhovm commented Jan 24, 2024

Most chunks of these were merged independently. What was not resurrected is creating a private LookupTable constructor, but that affects too much code and is not very useful, so low priority.

@volhovm volhovm closed this Jan 24, 2024
@volhovm volhovm deleted the volhovm/mina14674-resurrect-lookups-pr branch January 24, 2024 14:41
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