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

Modulus in brillig bigint is checked but never written #5373

Closed
sirasistant opened this issue Jul 1, 2024 · 0 comments
Closed

Modulus in brillig bigint is checked but never written #5373

sirasistant opened this issue Jul 1, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@sirasistant
Copy link
Contributor

sirasistant commented Jul 1, 2024

Aim

Use bigint in brillig

Expected Behavior

Moduli should be checked correctly on operations

Bug

Modulus is half implemented in brillig, it's checked by manually emitting opcodes to check it, but it's never written to, so it fails randomly when the stack offset reserved for it was used for different things previously.

To Reproduce

This code fails to execute since the stack offset of modulus of a and b was reused, and since it's never written to, it contains old non-matching values.

unconstrained fn add_unconstrained(values: [[u8; 32]; 2], modulus: [[u8; 32]; 2]) -> [u8; 32] {
    let a = BigInt::from_le_bytes(values[0].as_slice(), modulus[0].as_slice());
    // dep::std::println(a.modulus);

    let b = BigInt::from_le_bytes(values[1].as_slice(), modulus[1].as_slice());
    // dep::std::println(b.modulus);

    a.bigint_add(b).to_le_bytes()
}

fn main() {
    let values = [27, 3];
    let moduli = [256, 256];

    let values_as_bytes = values.map(|value: Field| value.to_le_bytes(32).as_array());
    let moduli_as_bytes = moduli.map(|modulus: Field| modulus.to_le_bytes(32).as_array());

    let result = add_unconstrained(values_as_bytes, moduli_as_bytes);

    assert_eq(result, 30.to_le_bytes(32).as_array());
}

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@sirasistant sirasistant added the bug Something isn't working label Jul 1, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 1, 2024
@sirasistant sirasistant changed the title Moduli in brillig bigint is checked but never written Modulus in brillig bigint is checked but never written Jul 1, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 11, 2024
# Description

## Problem\*

Resolves #5373

## Summary\*

Bigint modulus was checked but never written. Since changing
bigint::to_le_bytes to write a modulus ID would require a breaking
change, this fixes the modulus issue without changing the opcode spec,
by completely offloading modulus checks to the runtime itself.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

1 participant