EspressoSystems / jellyfish

A Rust Implementation of the PLONK ZKP System and Extensions
https://jellyfish.docs.espressosys.com
MIT License
408 stars 106 forks source link

feat: let multiplicity depend on payload size #667

Closed akonring closed 2 months ago

akonring commented 3 months ago

closes: #663

This PR:

This PR does not:

Key places to review:


Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.

ggutoski commented 3 months ago

Status as of https://github.com/EspressoSystems/jellyfish/pull/667/commits/17d55fba72b2cb4739fb417525c57ac41e9523e0 :

The new test max_multiplicity fails with

thread 'advz::test::max_multiplicity' panicked at vid/src/advz.rs:882:13:
assertion `left == right` failed
  left: 4096
 right: 4

Here's the code: https://github.com/EspressoSystems/jellyfish/blob/17d55fba72b2cb4739fb417525c57ac41e9523e0/vid/src/advz.rs#L876-L883

The problem is that we're still calling FFT with the original self.eval_domain, which auto-magically resizes the polynomial degree to the size of self.eval_domain, which was originally set in the Advz constructor according to max_multiplicity. In the above test failure a small payload of size 4 field elements is resized to a large payload of size 4096.

Any fix for this issue must also address the above problem. But that's a bigger task. We now need the eval domain to change size at disperse-time to accommodate a small payload.

Options

  1. Construct small domains on-the-fly as needed at disperse-time: if at disperse-time we find that self.eval_domain is too big then construct a new small domain, otherwise use self.eval_domain.
    • It would be nice of arkworks EvaluationDomain has a convenient way to construct a subdomain from a given domain. But I don't see such a method. Fortunately the code to construct a new domain is not difficult.
  2. Perhaps we should delete the field eval_domain from struct Advz. Instead, we always construct this domain on-the-fly with the correct size at disperse-time. But that's a nontrivial change so I prefer to discuss with others.

[EDIT: I think it's cleaner to do option 2. That's what I'm currently doing.]

ggutoski commented 2 months ago

closing so that I can open a new PR for this branch so as to better facilitate review from others.