Skip to content

Commit

Permalink
chore: reproduce AVM ecadd bug (#9019)
Browse files Browse the repository at this point in the history
Please read [contributing guidelines](CONTRIBUTING.md) and remove this
line.

---------

Co-authored-by: Ilyas Ridhuan <ilyas@aztecprotocol.com>
  • Loading branch information
sirasistant and IlyasRidhuan authored Oct 8, 2024
1 parent a8ec91a commit 757ccef
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,11 @@ contract AvmTest {
a % 2
}

#[public]
fn elliptic_curve_add(lhs: Point, rhs: Point) -> Point {
lhs + rhs
}

#[public]
fn elliptic_curve_add_and_double() -> Point {
let g = Point { x: 1, y: 17631683881184975370165255887551781615748388533673675138860, is_infinite: false };
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/foundation/src/fields/point.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ export class Point {
* Check this is consistent with how bb is encoding the point at infinity
*/
public get inf() {
return this.x.isZero() && this.y.isZero() && this.isInfinite;
return this.isInfinite;
}

isOnGrumpkin() {
Expand Down
19 changes: 19 additions & 0 deletions yarn-project/simulator/src/avm/avm_simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,25 @@ describe('AVM simulator: transpiled Noir contracts', () => {
expect(results.output).toEqual([new Fr(1), new Fr(2), new Fr(3)]);
});

it('ec_add should not revert', async () => {
// This test performs the same doubling as in elliptic_curve_add_and_double
// But the optimizer is not able to optimize out the addition
const calldata: Fr[] = [
new Fr(1), // P1x
new Fr(17631683881184975370165255887551781615748388533673675138860n), // P1y
new Fr(0), // P1inf
new Fr(1), // P2x
new Fr(17631683881184975370165255887551781615748388533673675138860n), // P2y
new Fr(0), // P2inf
];
const context = initContext({ env: initExecutionEnvironment({ calldata }) });

const bytecode = getAvmTestContractBytecode('elliptic_curve_add');
const results = await new AvmSimulator(context).executeBytecode(bytecode);

expect(results.reverted).toBe(false);
});

it('elliptic curve operations', async () => {
const context = initContext();

Expand Down
9 changes: 8 additions & 1 deletion yarn-project/simulator/src/avm/opcodes/ec_add.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,14 @@ export class EcAdd extends Instruction {
}

const grumpkin = new Grumpkin();
let dest = grumpkin.add(p1, p2);
let dest;
if (p1IsInfinite) {
dest = p2;
} else if (p2IsInfinite) {
dest = p1;
} else {
dest = grumpkin.add(p1, p2);
}
// Temporary,
if (p1IsInfinite) {
dest = p2;
Expand Down
24 changes: 16 additions & 8 deletions yarn-project/simulator/src/avm/opcodes/multi_scalar_mul.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,8 @@ export class MultiScalarMul extends Instruction {
// Now we need to reconstruct the points and scalars into something we can operate on.
const grumpkinPoints: Point[] = [];
for (let i = 0; i < numPoints; i++) {
const p: Point = new Point(pointsVector[3 * i].toFr(), pointsVector[3 * i + 1].toFr(), false);
// Include this later when we have a standard for representing infinity
// const isInf = pointsVector[i + 2].toBoolean();

const isInf = pointsVector[3 * i + 2].toNumber() === 1;
const p: Point = new Point(pointsVector[3 * i].toFr(), pointsVector[3 * i + 1].toFr(), isInf);
if (!p.isOnGrumpkin()) {
throw new InstructionExecutionError(`Point ${p.toString()} is not on the curve.`);
}
Expand All @@ -93,10 +91,20 @@ export class MultiScalarMul extends Instruction {
const [firstBaseScalarPair, ...rest]: Array<[Point, Fq]> = grumpkinPoints.map((p, idx) => [p, scalarFqVector[idx]]);
// Fold the points and scalars into a single point
// We have to ensure get the first point, since the identity element (point at infinity) isn't quite working in ts
const outputPoint = rest.reduce(
(acc, curr) => grumpkin.add(acc, grumpkin.mul(curr[0], curr[1])),
grumpkin.mul(firstBaseScalarPair[0], firstBaseScalarPair[1]),
);
const outputPoint = rest.reduce((acc, curr) => {
if (curr[1] === Fq.ZERO) {
// If we multiply by 0, the result will the point at infinity - so we ignore it
return acc;
} else if (curr[0].inf) {
// If we multiply the point at infinity by a scalar, it's still the point at infinity
return acc;
} else if (acc.inf) {
// If we accumulator is the point at infinity, we can just return the current point
return curr[0];
} else {
return grumpkin.add(acc, grumpkin.mul(curr[0], curr[1]));
}
}, grumpkin.mul(firstBaseScalarPair[0], firstBaseScalarPair[1]));
const output = outputPoint.toFields().map(f => new Field(f));

memory.setSlice(outputOffset, output);
Expand Down

0 comments on commit 757ccef

Please sign in to comment.