feat: add PlonkVerifier contracts and tests #722

alxiong · 2023-10-24T14:42:07Z

Resolves #702

~~Create issues tracking TODOs in the code~~
update dep on jellyfish from more-test-api branch to a rev after test: Expose more internal functions for test jellyfish#398 is merged
Add diff-test executable in PATH and directly invoking it instead of cargo run --bin diff-test

Bug Fixes

In this PR, we have fixed two bugs:

in 69079d9, we fix the discrepancy introduced in an updated version of Jellyfish. This is not technically a bug, but an inconsistency of types due to jellyfish's update on arkwork dep, which further affect the Transcript.state computation.
in 1d7c6c6, we add a missing validity check on zetaOmega field of a plonk proof. This is important since all of the following computation assume these fields are valid. (credit to @sveitser for spotting it, and we have added automated fuzz-test to detect such error)

alxiong · 2023-11-08T17:20:29Z

I will deal with some minor TODOs and fix CI failure tmr, but I think it's good to start reviewing this PR now.

@sveitser @philippecamacho

~~the final test on batchVerify is currently blocked by #741~~

contracts/test/PlonkVerifier.t.sol

sveitser · 2023-11-13T16:16:15Z

contracts/test/PlonkVerifier.t.sol

+contract PlonkVerifier_preparePcsInfo_Test is PlonkVerifierCommonTest {
+    /// forge-config: default.fuzz.runs = 5
+    /// @dev Test `preparePcsInfo` matches that of Jellyfish
+    function testFuzz_preparePcsInfo_matches(


Encountered 1 failing test in contracts/test/PlonkVerifier.t.sol:PlonkVerifier_preparePcsInfo_Test [FAIL. Reason: InvalidPolyEvalArgs() Counterexample: calldata=0x0x99fdc07100000000000000000000000000000000000000000000000000000000000005fe000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000003b2, args=[1534, [], 0x00000000000000000000000000000000000000000000000000000000000003b2]] testFuzz_preparePcsInfo_matches(uint64,uint256[],bytes) (runs: 1, μ: 8001859, ~: 8001859)

good catch, I will add test logic for empty publicInput array.
(nothing wrong with code, just test is not careful)

done in 90bf8d1

sveitser · 2023-11-13T16:22:39Z

foundry.toml

@@ -26,3 +27,6 @@ sepolia = { key = "${ETHERSCAN_API_KEY}" }
 line_length=100
 bracket_spacing=true
 wrap_comments=true
+
+[profile.ci]
+fuzz = { run = 50 }


Suggested change

fuzz = { run = 50 }

fuzz = { runs = 50 }

Should we run a bit more?

I also noticed that the inline comments seems to take precedence over setting it via env var or in foundry.toml which seems weird. In that case should we not use the inline comments?

I use inline ones previously because some tests run slower than others, but now the released built diff-test binary, all of these tests are fast, so I will remove any inline configs for fuzzer (just use 256 runs by default) and even remove this as well (256 for ci seems good enough)?

Or we could add something like the following and run it once a day on the CI.

[profile.fuzz] fuzz = { runs = 10000 }

I use inline ones previously because some tests run slower than others, but now the released built diff-test binary, all of these tests are fast, so I will remove any inline configs for fuzzer (just use 256 runs by default) and even remove this as well (256 for ci seems good enough)?

Yeah if that's not too slow that is fine I think.

Since it's almost free and only costs CPU time I think it's not a bad idea to run more iterations on the CI occasionally, but we don't have to do it as part of this PR.

done in 90bf8d1

flake.nix

contracts/rust/src/bin/diff_test.rs

sveitser · 2023-11-13T16:59:33Z

contracts/test/PlonkVerifier.t.sol

+                // muteate the x coordinate
+                mstore(mload(add(proof, mul(0x20, nthPoint))), 0x1234)
+            }
+            // else, mutate y coordinate


The y coordinate is always mutated.

fixed in b914f77

contracts/test/PlonkVerifier.t.sol

…fuzz.runs

This reverts commit 1039690.

philippecamacho

Great work!

sveitser

LGTM

Replace explorer prefix search with exact match for block and transaction hash

alxiong self-assigned this Oct 24, 2023

alxiong added 3 commits October 26, 2023 01:10

move over PlonkVerifier and fix solhint

3eb78b7

fix: flake.nix for solhint dep

66966f2

add ffi & fuzz test example for polyeval

1e09fb3

alxiong force-pushed the plonk-verifier branch from 3d55b9a to 1e09fb3 Compare October 27, 2023 15:20

alxiong added 11 commits October 31, 2023 11:28

wip: test evalDataGen

4546436

wip: put aside evalDataEval failed test

271d019

pass Transcript's appendMessage fuzz test

296af4e

more tests, and fix prior failure

71dce98

bugfix: fix type inconsistency leading to diff transcript

69079d9

finish tests for Transcript.sol

2887bba

vm.assume -> bound when possible

6807ac8

add extra fuzzy input constraint on zeta value

e0a3e31

wip: two more plonkverifier test

6bc9d0b

wip: add most verifier tests

566ee05

Merge remote-tracking branch 'origin/main' into plonk-verifier

1dd6540

alxiong marked this pull request as ready for review November 8, 2023 17:18

alxiong requested review from nomaxg, philippecamacho, ImJeremyHe, sveitser and jbearer as code owners November 8, 2023 17:18

alxiong requested review from sveitser and removed request for sveitser and ImJeremyHe November 8, 2023 17:19

alxiong mentioned this pull request Nov 8, 2023

test: Expose more internal functions for test EspressoSystems/jellyfish#398

Merged

alxiong added 2 commits November 9, 2023 14:55

add eval domain support for size=32 for testing

f5d1b37

test: finish all tests

388ddd1

alxiong force-pushed the plonk-verifier branch from b94fd1a to 388ddd1 Compare November 9, 2023 14:00

sveitser reviewed Nov 13, 2023

View reviewed changes

contracts/test/PlonkVerifier.t.sol Outdated Show resolved Hide resolved

sveitser reviewed Nov 13, 2023

View reviewed changes

flake.nix Outdated Show resolved Hide resolved

nomaxg reviewed Nov 13, 2023

View reviewed changes

contracts/rust/src/bin/diff_test.rs Outdated Show resolved Hide resolved

nomaxg reviewed Nov 13, 2023

View reviewed changes

contracts/rust/src/bin/diff_test.rs Show resolved Hide resolved

nomaxg reviewed Nov 13, 2023

View reviewed changes

contracts/rust/src/bin/diff_test.rs Show resolved Hide resolved

nomaxg reviewed Nov 13, 2023

View reviewed changes

contracts/rust/src/bin/diff_test.rs Show resolved Hide resolved

sveitser reviewed Nov 13, 2023

View reviewed changes

philippecamacho reviewed Nov 13, 2023

View reviewed changes

contracts/test/PlonkVerifier.t.sol Outdated Show resolved Hide resolved

philippecamacho reviewed Nov 13, 2023

View reviewed changes

contracts/test/PlonkVerifier.t.sol Outdated Show resolved Hide resolved

philippecamacho reviewed Nov 13, 2023

View reviewed changes

contracts/test/PlonkVerifier.t.sol Show resolved Hide resolved

philippecamacho reviewed Nov 13, 2023

View reviewed changes

contracts/test/PlonkVerifier.t.sol Show resolved Hide resolved

alxiong added 4 commits November 14, 2023 11:57

fix: allow empty public input, remove default.fuzz.runs, increase ci.…

90bf8d1

…fuzz.runs

address comments

b914f77

improve diff-test

fc1484e

Revert "add stake table interfaces"

9268423

This reverts commit 1039690.

alxiong requested review from sveitser, nomaxg and philippecamacho November 14, 2023 09:09

philippecamacho approved these changes Nov 15, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into plonk-verifier

ba1aaee

sveitser approved these changes Nov 15, 2023

View reviewed changes

Merge branch 'main' into plonk-verifier

217518c

alxiong enabled auto-merge (squash) November 15, 2023 14:28

alxiong disabled auto-merge November 15, 2023 14:28

Merge branch 'main' into plonk-verifier

e70ac7f

alxiong merged commit 54a22fc into main Nov 15, 2023

alxiong deleted the plonk-verifier branch November 15, 2023 14:42

sveitser pushed a commit that referenced this pull request Feb 4, 2025

Merge pull request #722 from EspressoSystems/ab/rm-prefix-search

509ba8c

Replace explorer prefix search with exact match for block and transaction hash

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add PlonkVerifier contracts and tests #722

feat: add PlonkVerifier contracts and tests #722

alxiong commented Oct 24, 2023 •

edited

Loading

alxiong commented Nov 8, 2023 •

edited

Loading

sveitser Nov 13, 2023

alxiong Nov 13, 2023 •

edited

Loading

alxiong Nov 14, 2023

sveitser Nov 13, 2023

alxiong Nov 13, 2023

sveitser Nov 13, 2023

sveitser Nov 13, 2023

alxiong Nov 14, 2023

sveitser Nov 13, 2023

alxiong Nov 14, 2023

philippecamacho left a comment

sveitser left a comment

feat: add PlonkVerifier contracts and tests #722

feat: add PlonkVerifier contracts and tests #722

Conversation

alxiong commented Oct 24, 2023 • edited Loading

Bug Fixes

alxiong commented Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

alxiong Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippecamacho left a comment

Choose a reason for hiding this comment

sveitser left a comment

Choose a reason for hiding this comment

alxiong commented Oct 24, 2023 •

edited

Loading

alxiong commented Nov 8, 2023 •

edited

Loading

alxiong Nov 13, 2023 •

edited

Loading