passing test for gfmulx #48
Conversation
|
implemented tests and logic for ghash and polyval mulx component. had to take special care while implementing polyval to correctly implement little-endian byte-shifting. There's a concerning conflict between rust-crypto and ietf's tests in polyval. From IETF we implement: // ref: https://datatracker.ietf.org/doc/html/rfc8452#appendix-A
it("compute IETF test 2", async () => {
let bits = hexToBitArray("9c98c04df9387ded828175a92ba652d8");
let expect = "3931819bf271fada0503eb52574ca5f2";
...and from rustcrypto we implement a conflicting test case. Not sure what to do here. repro: currently prioritizes passing the rust-crypto impl as it looks plausibly more correct. I've left a comment in the circom: for (var i = 0; i < 128; i++) {
// if (i==7 || i == 120 || i==121 || i==126) { // passes rust-crypto
if (i==7 || i==121 || i==126) { // passes ietf spec?switching these lines seems clearly logically wrong, as it would encode the wrong polynomial for polyval, but switching between them allows for different tests to pass. I did check that I copied the correct strings from the IETF spec, but would appreciate a second pair of eyes. recall that little endian is a bit annoying to work with, making polyval more annoying to implement than Ghash (which is implemented and complete with no weirdness). example:
resolved: |
|
Amazing work on this. Weird that the RFC has a type in it. Good find on that. I am down to merge this in it looks really nice and has good documentation. Thank you for setting a nice standard on quality here, the work is beautiful and speaks for itself. |
| for (var i = 1; i < 128; i++) { v[i] <== in[i-1]; } | ||
|
|
||
| // if MSB set, assign irreducible poly bits, otherwise zero | ||
| // irreducible_poly has 1s at positions 1, 2, 7, 127 |
There was a problem hiding this comment.
Should this be
at positions 1, 2, 7, 128
| // ...1100 0010 <== encodes 121, 126, 127 | ||
| // ...0100 0010 <== encodes 121, 126 | ||
| for (var i = 0; i < 128; i++) { | ||
| if (i==7 || i == 120 || i==121 || i==126) { |
There was a problem hiding this comment.
Be more explicit about LE stuff
| // mid2= [0 a b c d e f g, h i j k l m n o] // shift bits right by 1 | ||
| // out = [g f e d c b a 0, o n m l k j i h] // swap order of bits in each byte | ||
| // TODO(TK 2024-08-15): optimize | ||
| template LeftShiftLE(shift) { |
There was a problem hiding this comment.
Very readable. Don't optimize until necessary
| it("should have correct output", async () => { | ||
| const witness = await circuit.calculateWitness({ in: bit_array }); | ||
| const witness = await circuit.expectPass({ in: bit_array}, { out: expected_output }); | ||
| circuit.expectPass({in: bit_array}); |
There was a problem hiding this comment.
I recommend that tests get back the output and compare that to expected output instead of using expectPass to be consistent with the expressiveness of most testing frameworks.
const expect = ...
let result = await circuit.compute(input, ["out"]);
assert.equal(result, expected);If not, remove the second circuit.expectPass and remove printing out the witness below.
| import { bitArrayToHex, circomkit, hexToBitArray } from "../common"; | ||
|
|
||
| // Disable truncation of arrays in error messages | ||
| chai.config.truncateThreshold = 0; |
There was a problem hiding this comment.
Better to have this in a single test-setup file than at the start of each file imo.
We can create a test/setup.ts file, then either include it with --require ./test/setup.js or in mocha.opts (Which is better option)
Closes #47