0xPolygonZero / plonky2

Apache License 2.0
763 stars 285 forks source link

Remove restriction to binary-only multiplicities #1577

Closed matthiasgoergens closed 5 months ago

matthiasgoergens commented 5 months ago

This is a simpler PR to prepare the ground for https://github.com/0xPolygonZero/plonky2/pull/1575

matthiasgoergens commented 5 months ago

Btw, why does starky lack tests?

Nashtare commented 5 months ago

Btw, why does starky lack tests?

We have an open issue for it (#1514). All tests were specific to the zkEVM and migrated to the new repo. I agree this definitely needs some more testing. I have pinged the person who assigned themselves this task but no answer yet.

Nashtare commented 5 months ago

Related to testing, have you tested this? Haven't reviewed yet but it's failing on our integration tests there https://github.com/0xPolygonZero/zk_evm/tree/develop.

matthiasgoergens commented 5 months ago

I ran all the tests in the plonky2 repository. I'm still running tests elsewhere. Thanks for pointing to your other repository, I'll try to run those tests, too.

Update: the develop branch on the zk_evm repo fails with the main branch here already, it doesn't build. So it would break even more with this branch developed against that main branch.

I'll see about making you a fix quickly.

matthiasgoergens commented 5 months ago

Here's your fix to make zk_evm compatible with plonky2's main: https://github.com/0xPolygonZero/zk_evm/pull/183

Nashtare commented 5 months ago

I already have a branch working with main. But moving deps to your current branch gives me an quotient polynomial evaluation error.

matthiasgoergens commented 5 months ago

I already have a branch working with main. But moving deps to your current branch gives me an quotient polynomial evaluation error.

Thanks. I found the bug.

I cleaned up the code a bit while fixing the bug.

 starky/src/lookup.rs | 84 ++++++++++++++++++++----------------------------------------------------------------
 1 file changed, 20 insertions(+), 64 deletions(-)
Nashtare commented 5 months ago

I already have a branch working with main. But moving deps to your current branch gives me an quotient polynomial evaluation error.

Thanks. I found the bug.

I cleaned up the code a bit while fixing the bug.

 starky/src/lookup.rs | 84 ++++++++++++++++++++----------------------------------------------------------------
 1 file changed, 20 insertions(+), 64 deletions(-)

For things touching starky, I usually run them against our zk_evm monorepo as sanity check, until we get some good coverage here. Priority wise it's a bit tough for us to dedicate time for it at the moment, but it will be done sooner or later. In the meantime, as long as myself and other maintainers are testing against zk_evm, we should be fine.

matthiasgoergens commented 5 months ago

Please feel free to merge when you are ready.