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

Differential test to verify all implementations #24

Open
0xbok opened this issue May 9, 2023 · 9 comments
Open

Differential test to verify all implementations #24

0xbok opened this issue May 9, 2023 · 9 comments

Comments

@0xbok
Copy link

0xbok commented May 9, 2023

It's important to have consistency across these impls. A few examples:

  • sha256 in js, circom vs sha512 in rust.
  • c is returned as a plain number in js, circom vs a field element in rust.
  • More issues may be uncovered, like encoding/decoding across different formats.

You should run a differential test on random values for message and private key (maybe pre-generate and freeze them).

@0xbok 0xbok changed the title Create a test which verifies all implementations Differential test verify all implementations May 9, 2023
@0xbok 0xbok changed the title Differential test verify all implementations Differential test to verify all implementations May 9, 2023
@skaunov
Copy link
Collaborator

skaunov commented Sep 22, 2023

So, to implement this we need to add CI which would raise a warning when some consistency checks are failed? Or is there a better approach?

If CI it is, is there any preference towards a particular system?

@Divide-By-0
Copy link
Member

Divide-By-0 commented Sep 22, 2023

Yup, CI is great! All the tests should read from the same configuration file or something that has the inputs and outputs, so the different languages are all verified to match!

@skaunov
Copy link
Collaborator

skaunov commented Sep 23, 2023

So, we need following?

  1. GitHub Actions workflows which would check PR (based on files changed in perspective) against three test sets which are included for each language; and
  2. introduce a (config) file with data that those aforementioned test sets could use and be synchronized that way.

@Divide-By-0
Copy link
Member

So, we need following?

  1. GitHub Actions workflows which would check PR (based on files changed in perspective) against three test sets which are included for each language; and
  2. introduce a (config) file with data that those aforementioned test sets could use and be synchronized that way.

Yup

@skaunov skaunov self-assigned this Sep 23, 2023
@skaunov
Copy link
Collaborator

skaunov commented Sep 23, 2023

I see no good way to actually provide a single source for automated reading test vectors while autotest. It will degrade development experience significantly: introduction of file reading during running tests, decoding of the values into native format, using a file outside of working dir (referencing top level).

There's few strategies to tackle this one; though they're not simple.

  • Add reporting to test runs and a workflow that would analyze the reports. Requires adding tests reporting features to each implementation.
  • Add a workflow which would run each implementation with test values and check their output (kind of additional integration testing done with GA). Seems that such a workflow will require frequent maintenance when an implementation is changed.
  • Setting up some build system in repo could be a solution. Though I never focused on DevOps, so it's hard to me to recommend/compare them.

@skaunov
Copy link
Collaborator

skaunov commented Sep 23, 2023

Yup, CI is great! All the tests should read from the same configuration file or something that has the inputs and outputs, so the different languages are all verified to match!

So, I added basic CI in 9a068af and ccd26a8 for all four entities the repo provides as you suggested here and at #30 (comment). Let me unassign myself while some direction regarding the analysis in the previous comment would be chosen.

@skaunov skaunov removed their assignment Sep 23, 2023
@Divide-By-0
Copy link
Member

I see no good way to actually provide a single source for automated reading test vectors while autotest. It will degrade development experience significantly: introduction of file reading during running tests, decoding of the values into native format, using a file outside of working dir (referencing top level).

There's few strategies to tackle this one; though they're not simple.

  • Add reporting to test runs and a workflow that would analyze the reports. Requires adding tests reporting features to each implementation.
  • Add a workflow which would run each implementation with test values and check their output (kind of additional integration testing done with GA). Seems that such a workflow will require frequent maintenance when an implementation is changed.
  • Setting up some build system in repo could be a solution. Though I never focused on DevOps, so it's hard to me to recommend/compare them.

These are great points and ideas. I was thinking more a test.env in each of the repos and a CI that verifies it is the same, rather han a config file per se, but your point eegarding parsing etc still stands. An 80/20 for now is independent CIs for now with comments that they should be kept consistent with the other files. I agree with you that the easiest thing is to add logging/reporting to a file and ensure the outputs are the same in all cases.

@skaunov
Copy link
Collaborator

skaunov commented Sep 23, 2023

I didn't really looked below Jest in circuits until now, and now I see it just run circom, also I don't see any GA for it (I guess it's too expensive). So with a light CI this is reduced to crates and javascript testing workflows. (I'll remove excessive stuff from <.github/workflows/npm.yml>.)

So I personally would tackle this one further in the following two streams.

  • Decide on circuits CI: how/when to setup it. Great chances are it will be powerful enough to enable what's costly and non-trivial without it.
  • While we're with anything lightweight as GA, start with just a spec of test vectors that should be shared across implementations. When this spec will be filled/stabilized best current/lightweight approach might pop-up quite naturally.

So I'd propose to convert this issue into a test vectors document with known/potential inconsistencies section. Then based on this doc start to introduce sync automation. Which could eventually be absorbed by circom capable CI.

@Divide-By-0
Copy link
Member

Divide-By-0 commented Sep 23, 2023

Sweet, that sounds great. A circom test can use circom-tester library to do witness generation at least within tests, which is fast; proofgen will work 99% of the time if witnessgen works so I think it's pretty good substitute.

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

No branches or pull requests

3 participants