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

[LUR-22] Replace pallas::Scalar as the default test field with BN256 #1018

Closed
huitseeker opened this issue Jan 4, 2024 · 0 comments · Fixed by #1039
Closed

[LUR-22] Replace pallas::Scalar as the default test field with BN256 #1018

huitseeker opened this issue Jan 4, 2024 · 0 comments · Fixed by #1039
Assignees
Labels

Comments

@huitseeker
Copy link
Contributor

huitseeker commented Jan 4, 2024

Among the instances of CurveCycleEquipped in proof::nova we mostly use the one pegged to pallas::Scalar for all of our tests and as the default test workhorse. Here are the instance counts for this field:

src/symbol.rs:1
src/field.rs:6
src/parser/syntax.rs:1
src/eval/lang.rs:1
src/z_data/z_cont.rs:1
src/public_parameters/mod.rs:1
src/parser/string.rs:1
src/z_data/z_expr.rs:1
src/parser/base.rs:1
src/z_data/z_ptr.rs:3
src/z_data/serde/mod.rs:1
src/coprocessor/trie/mod.rs:1
src/z_data/z_store.rs:1
src/circuit/gadgets/constraints.rs:1
examples/sha256_ivc.rs:1
examples/sha256_nivc.rs:1
src/circuit/circuit_frame.rs:1
examples/circom.rs:1
benches/fibonacci.rs:1
src/lem/store.rs:1
examples/tp_table.rs:1
benches/end2end.rs:1
src/lem/eval.rs:1
benches/public_params.rs:1
benches/sha256.rs:1
src/proof/nova.rs:1
src/proof/tests/nova_tests_lem.rs:1
src/num.rs:2
src/cli/mod.rs:4
src/lem/tests/eval_tests.rs:1
src/lem/tests/misc.rs:1
src/lem/tests/nivc_steps.rs:1

However, we're more likely to use the instance on the BN256 Scalar field as the one that will be most relevant to us:
https://github.com/lurk-lab/lurk-rs/blob/bc136099c58c1402510b949b29768d604642405b/src/proof/nova.rs#L69-L75

We should replace all instances of pallas::Scalar with BN256, (and correspondingly, any instances of pallas::Vesta with grumpkin's Scalar field), which will also help the https://github.com/lurk-lab/solidity-verifier project with test vectors.

Important

We should check this does not impact benchmarks significantly (after adoption of halo2curves 0.5.0, and our integration of grumpkin-msm, we have high hopes that this might indeed be true)

Tip

Resolve #1022 at the same time to ensure non-regression.

Warning

We're picky in how we refer to those scalar fields, don't forget to also update the disallowed-methods in .clippy.toml

This issue is not to be confused with #533 which is about application-level integration of different user-configurable fields. We're here talking about the field used by our unit tests, but we should probably set BN256 as the default nonetheless.

From SyncLinear.com | LUR-22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants