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] soundness bug in BasicDynLookupConfig::assign_virtual_table_to_raw #224

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

jonathanpwang
Copy link
Contributor

@jonathanpwang jonathanpwang commented Nov 29, 2023

The problem is that in assign_virtual_table_to_raw it was using assign_virtual_to_raw, which has the following danger:

  • It will "assign" a virtual cell to a raw Halo2 cell by inserting the assignment into the HashMap of assigned_advices. However later the BaseCircuitBuilder raw_assign might overwrite this assignment without adding any real equality constraints. Therefore the first raw cell becomes a dangling unconstrained cell.

The fix is that now assign_virtual_table_to_raw uses constrain_virtual_equals_external, and we have modified constrain_virtual_equals_external so that it checks whether the virtual cell has already been assigned or not. It is only allowed to not have been assigned if the type_id of the virtual cell was marked as EXTERNAL_CELL_TYPE_ID. This marks the virtual cell as safe from ever being reassigned by a different virtual region manager.

  • If the virtual cell is already assigned to a raw cell, we constrain equality between the raw cell and the new external cell.
  • If the virtual cell has not been assigned and is marked EXTERNAL_CELL_TYPE_ID, then we assign the virtual cell to the new external cell.

The usage in the latter case is demonstrated in the memory.rs test.

We add a check whenever we do assigned_advice.insert that there was not already a cell present or that the occupied raw cell equals the raw cell to be inserted (this latter case is just because keygen_vk and keygen_pk each call synthesize and we cannot call clear because of mutable borrows).

@jonathanpwang jonathanpwang merged commit b6625fa into release-0.4.1-rc Nov 30, 2023
2 checks passed
@jonathanpwang jonathanpwang deleted the fix/dyn_lookup_dest_table branch November 30, 2023 00:50
@jonathanpwang jonathanpwang mentioned this pull request Jan 18, 2024
zemse pushed a commit to zemse/axiom-halo2-lib that referenced this pull request Jan 19, 2024
In the future we will not squash merge so as to maintain history.

## Bug Fixes
* axiom-crypto#224

## Other PRs
* axiom-crypto#218
* axiom-crypto#231
* axiom-crypto#232
* axiom-crypto#227
* axiom-crypto#235
* axiom-crypto#237
* axiom-crypto#242
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.

2 participants