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

Why check public signals less than q instead of r in generated solidity code? #479

Closed
Stumble opened this issue Apr 3, 2024 · 4 comments

Comments

@Stumble
Copy link
Contributor

Stumble commented Apr 3, 2024

Hey team,

If i understand correctly, in the following solidity proof verification contract template, it checks that all the public signals are less than q to make sure that signals are not "aliases".

if iszero(lt(v, q)) {

I'm a bit confused about why it checks v < q instead of v < r. Isn't r the default prime for the finite field in circom?

@Divide-By-0
Copy link

Divide-By-0 commented Apr 4, 2024

bn254 is defined over the numbers 0-q, you can check the definition of q in that file corresponds to the size of the prime for BN254 in i.e. https://hackmd.io/@jpw/bn254 . It seems the public inputs should be checked against the size of the scalar field, not the base field, so I agree with this note.

But you're right that tornado cash checks those same public inputs against the scalar prime: https://github.com/tornadocash/tornado-core/blob/1ef6a263ac6a0e476d063fcb269a9df65a1bd56a/contracts/Verifier.sol#L199

@Stumble
Copy link
Contributor Author

Stumble commented Apr 5, 2024

@Stumble
Copy link
Contributor Author

Stumble commented Apr 5, 2024

I'm leaning more towards the possibility that it's a bug now, since I found that the templates for fflonk and plonk are accurately checking against the scalar field size.

image
image

@Stumble
Copy link
Contributor Author

Stumble commented Apr 5, 2024

Although I think it is neither a security issue or causing any trouble on production, but still created a PR to fix it: #480

@Stumble Stumble closed this as completed Apr 8, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 9, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 11, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 12, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 14, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 16, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 16, 2024
arnaucube added a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 17, 2024
github-merge-queue bot pushed a commit to privacy-scaling-explorations/sonobe that referenced this issue Apr 25, 2024
* Add solidity verifier of the nova+cyclefold, and add method to prepare the calldata from Decider's proof. Missing conversion of the point coordinates into limbs (ark compatible)

* chore: adding comments linking to the contract's signature

* chore: update .gitignore

* chore: add num-bigint as dev dependency

* fix: work with abs path for storing generated sol code

* chore: update comment

* feat: solidity verifier working on single and multi-input circuits

* feat: multi-input folding verification working + fixing encoding of additive identity in calldata

* chore: make bigint a dependency

* refactor: import utils functions from utils.rs and make them available from anywhere

* chore: make utils and evm available publicly

* fix: pub mod instead

* chore: make relevant method public and add `get_decider_template_for_cyclefold_decider` to exported objects

* solidity-verifiers: move tests to their corresponding files

* small update: Cyclefold -> CycleFold at the missing places

* abstract nova-cyclefold solidity verifiers tests to avoid code duplication, and abstract also the computed setup params (FS & Decider) to compute them only once for all related tests to save test time

* small polish after rebase to last main branch changes

* rm unneeded Option for KZGData::g1_crs_batch_points

* add checks modifying z_0 & z_i to nova_cyclefold_solidity_verifier test

* add light-test feature to decider_eth_circuit to use it in solidity-verifier tests without the big circuit

* solidity-verifiers: groth16 template: port the fix from iden3/snarkjs#480 & iden3/snarkjs#479

* add print warning msg for light-test in DeciderEthCircuit

* solidity-verifiers: update limbs logic to nonnative last version, parametrize limbs params

solidity-verifiers:
* update solidity limbs logic to last nonnative impl version, and to
  last u_i.x impl
* parametrize limbs params
* add light-test feature: replace the '#[cfg(not(test))]' by the
  'light-test' feature that by default is not enabled, so when running
  the github actions we enable the feature 'light-tests', and then we can
  have a full-test that runs the test without the 'light-tests' flag, but
  we don't run this big test every time.  The choice of a feature is to
  allow us to control this from other-crates tests (for example for the
  solidity-verifier separated crate tests, to avoid running the full heavy
  circuit in the solidity tests)

* move solidity constants into template constants for auto compute of params

* polishing

* revm use only needed feature

This is to avoid c depencency for c-kzg which is behind the c-kzg flag
and not needed.

* nova_cyclefold_decider.sol header

* rearrange test helpers position, add error for min number of steps

* in solidity-verifiers: 'data'->'vk/verifier key'

* add From for NovaCycleFoldVerifierKey from original vks to simplify dev flow, also conditionally template the batchCheck related structs and methods from the KZG10 solidity template

---------

Co-authored-by: dmpierre <[email protected]>
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

2 participants