Remove restriction to binary-only multiplicities

[matthiasgoergens]

This is a simpler PR to prepare the ground for #1575

[matthiasgoergens]

Btw, why does starky lack tests?

[Nashtare]

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]

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]

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]

Here's your fix to make zk_evm compatible with plonky2's main: 0xPolygonZero/zk_evm#183

[Nashtare]

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

[matthiasgoergens]

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(-)

[matthiasgoergens]

I removed the duplication of logic here, and replaced it with a single call to reduce further down.

[matthiasgoergens]

Iterators in Rust have a size hint, so collect automatically does what we were trying to do manually via with_capacity here.

[Nashtare]

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.

[LindaGuiga]

If I remember correctly, the reason why the previous code did not use batch_multiplicative_inverse (and was therefore more complicated). was because the filter column is often (not always!) sparse, so we wanted to avoid inverting a lot of values that would be later zeroed out. However, I am not sure how much of an impact it would actually have, and this code does look much nicer!

[matthiasgoergens]

Yes, though the code in main already uses batch multiplicative inverse.

[muursh]

I'm good with this

[matthiasgoergens]

Please feel free to merge when you are ready.

[Nashtare]

LGTM too