Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

fix(acir): Fix Expression multiplication to correctly handle degree 1 terms #255

Merged
merged 3 commits into from
May 4, 2023

Conversation

TomAFrench
Copy link
Member

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves (link to issue)

Description

Summary of changes

There was a typo in the implementation of the Mul operator for Expressions.

while i1 < self.linear_combinations.len() {
let (a_c, a_w) = self.linear_combinations[i1];
output.linear_combinations.push((rhs.q_c * a_c, a_w));
i1 += 1;
}
while i2 < rhs.linear_combinations.len() {
let (b_c, b_w) = rhs.linear_combinations[i1];
output.linear_combinations.push((self.q_c * b_c, b_w));
i2 += 1;
}

Both of these while-loops use i1 to index into linear_combinations while the second should use i2. This results in either improper multiplication or panics when trying to read past the end of rhs.linear_combinations.

I've written a quick smoke test for the Mul operator along with another for Add (which was already covered by the smoketest for add_mul. The first commit shows this test catching the error and it is fixed in later commits.

I've also added an extra bit of logic to avoid zero-coefficient terms when multiplying Expressions together.

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@TomAFrench TomAFrench force-pushed the fix-expression-multiplication branch from f30ec8f to 93f7514 Compare May 2, 2023 07:56
@TomAFrench TomAFrench changed the title fix: Fix Expression multiplication to correctly handle degree 2 terms fix(acir): Fix Expression multiplication to correctly handle degree 2 terms May 2, 2023
@TomAFrench TomAFrench changed the title fix(acir): Fix Expression multiplication to correctly handle degree 2 terms fix(acir): Fix Expression multiplication to correctly handle degree 1 terms May 2, 2023
@kevaundray kevaundray enabled auto-merge May 4, 2023 14:00
@kevaundray kevaundray added this pull request to the merge queue May 4, 2023
Merged via the queue into master with commit e399396 May 4, 2023
@github-actions github-actions bot mentioned this pull request May 4, 2023
@phated phated deleted the fix-expression-multiplication branch May 4, 2023 15:21
TomAFrench added a commit that referenced this pull request May 16, 2023
* master: (49 commits)
  feat(acvm)!: Add CommonReferenceString backend trait (#231)
  fix(acir): Hide variants of WitnessMapError and export it from package (#283)
  feat!: Introduce WitnessMap data structure to avoid leaking internal structure (#252)
  feat!: use struct variants for blackbox function calls (#269)
  chore(acvm)!: Backend trait must implement Debug (#275)
  chore!: remove `OpcodeResolutionError::UnexpectedOpcode` (#274)
  chore(acvm)!: rename `hash_to_field128_security` to `hash_to_field_128_security` (#271)
  feat(acvm)!: update black box solver interfaces to match `pwg:black_box::solve` (#268)
  chore(acir_field): remove unnecessary `to_vec()` (#267)
  chore(acvm)!: expose separate solvers for AND and XOR opcodes (#266)
  feat(acvm)!: Simplification pass for ACIR (#151)
  changes the name of blake to be blakes2s256 (#261)
  update hash functions (#260)
  feat!: Remove `solve` from PWG trait & introduce separate solvers for each blackbox (#257)
  chore: Release 0.11.0 (#250)
  feat(acvm): Add generic error for failing to solve an opcode (#251)
  fix(acir): Fix `Expression` multiplication to correctly handle degree 1 terms (#255)
  chore(acir): organise opcodes definitions (#254)
  chore: remove usage of `insert_witness` with `insert_value` (#253)
  feat: Add Keccak Hash function (#259)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants