VerifyingKey Serialisation

[nalinbhardwaj]

Closes #449 by storing selectors (the input to compress_selectors) as part of the serialised vkey. On reads, reruns the selector compression on the cs object. This also partly completes #643 (by creating the "post' version as the default, afaict no place needs serialisation for the pre version in the current codebase).

Tested using the circuits from https://github.com/nalinbhardwaj/zordle and https://github.com/icemelon/halo2-examples/tree/master/src/fibonacci. This is also extremely useful for faster wasm verifiers!

[parazyd]

Are there any plans to merge this?

[daira]

Please write an 0x01 version byte first. On read, error if this byte is not 0x01. [Done.]

(Why 0x01 rather than 0x00? Because forks that have already merged some similar patch, such as privacy-scaling-explorations#103 , will have written the number of fixed commitments as big-endian, and so they will have written the first byte as 0x00 for any number of fixed commitments < $2^{24}$, i.e. all practical circuits. This allows descendants of those forks to be able to distinguish the two formats, and we will correctly reject the incompatible format early.)

[daira]

My initial intuition was that this should be the compressed selectors, because including the uncompressed selectors makes the representation dependent on the selector compression algorithm when it needn't be.

On the other hand, because the compressed selector columns use full field elements, their representation ends up being much larger, despite the "compression". So the approach used by this PR is also a reasonable option.

[daira]

I see that I said the same thing in #449 (comment) . I'm nothing if not consistent, even though I'd completely forgotten making that comment!

[jonathanpwang]

I agree with you. In retrospect serialization should definitely use the compressed selectors. Ideally VerifyingKey::read should not need to know the type of ConcreteCircuit or even be supplied with params (the latter is only used to generate domain, cs and get k, n I believe).

Right now I have to create a ConcreteCircuit ahead of time and then trick Rust to do some type inference to call read.

[daira]

At least the change to little-endian for the length of fixed commitments is blocking. [Done.]

[daira]

Reviewed; rebased to fix conflicts and add my changes in the last commit. This will need tests.

[jonathanpwang]

LGTM!

I leave it up to you whether you want to make the change to compressed selectors before merging.

[jonathanpwang]

I agree with you. In retrospect serialization should definitely use the compressed selectors. Ideally VerifyingKey::read should not need to know the type of ConcreteCircuit or even be supplied with params (the latter is only used to generate domain, cs and get k, n I believe).

Right now I have to create a ConcreteCircuit ahead of time and then trick Rust to do some type inference to call read.

[str4d]

We discussed this PR in Halo 2 Office Hours today, and decided that we will not merge it as-is. The core issue is that the serialization format being defined here is tightly coupled to the current implementation, and will be very difficult to retain support for after the planned optimisation / FloorPlanner refactorings occur.

For example, during keygen we extend the set of fixed column commitments to include commitments to the post-selector-combining selector columns:

halo2/halo2_proofs/src/plonk/keygen.rs

Lines 222 to 227 in 5678a50

	let (cs, selector_polys) = cs.compress_selectors(assembly.selectors);
	fixed.extend(
	selector_polys
	.into_iter()
	.map(|poly| domain.lagrange_from_vec(poly)),
	);

Then in VerifyingKey::read we read the post-selector-combining fixed commitments, read the pre-selector-combining selector definitions, re-compress the selector columns, and throw away the resulting selector polynomials (without checking that the effect of the compression on the constraint system's gates was correct or consistent with the parsed fixed commitments):

halo2/halo2_proofs/src/plonk.rs

Lines 101 to 103 in 58e797d

	let fixed_commitments: Vec<_> = (0..num_fixed_columns)
	.map(|_| C::read(reader))
	.collect::<io::Result<_>>()?;

halo2/halo2_proofs/src/plonk.rs

Line 129 in 58e797d

let (cs, _) = cs.compress_selectors(selectors.clone());

So as-is, the implicit serialization format is cutting half-way through the selector-combining optimisation, instead of picking a side. This specific issue could for instance be averted by not serializing the fixed commitments for the selectors, and always re-deriving them afterwards. Alternatively we could re-derive them and check they match, but then we're doing the same computational work during parsing with no benefit to encoding size. Solving this specific issue is a blocker on this PR (and it would be good to check for any other similar specific issues).

But if we did make that fix, we are then in a situation where the implicit serialization format is still cutting part-way through the logic that we want the FloorPlanner to be responsible for. Maintaining compatibility with this format would place strong constraints on how we designed the new FloorPlanner APIs, and probably require special-casing the selector-combining optimisation near-indefinitely (rather than it becoming an implementation detail of specific floor planners).

The most productive route forward would I think be to work on creating a post-optimisation VerifyingKey struct (the second half of #643), and then defining an encoding format for it. This can be done without blocking on the FloorPlanner refactoring, as it's about defining the type that would exist on the output side of the FloorPlanner rather than anything within its boundaries. It would require defining canonical encodings for polynomials as well as the polynomial ASTs we use to represent gates; we probably want to look at https://crates.io/crates/postcard for actually producing the encoding format.

[jonathanpwang]

I'm curious - how do you feel about just using #[derive(Serialize, Deserialize)]? That would avoid all of this. I have yet to benchmark but I would guess the bincode serialization/deserialization will be faster than what's currently implemented. This would involve the PrimeField to also implement Serialize, Deserialize.

[str4d]

I'm curious - how do you feel about just using #[derive(Serialize, Deserialize)]? That would avoid all of this.

That would not solve the above problem, and in fact would make the implicit encoding even more tightly coupled to the current implementation.

Once the types issue is addressed however, and a VerifyingKey type exists that contains precisely what should be encoded in precisely the internal form required for circuit correctness, then that could be an approach taken (using postcard as I suggested above).

Decided not to merge as-is.

[str4d]

The most productive route forward would I think be to work on creating a post-optimisation VerifyingKey struct (the second half of #643), and then defining an encoding format for it.

I'm going to try working on this some time next week.

[str4d]

The most productive route forward would I think be to work on creating a post-optimisation VerifyingKey struct (the second half of #643), and then defining an encoding format for it.

I'm going to try working on this some time next week.

We did the first phase of this in Halo 2 Office Hours today. The resulting exploratory changes are in #781, along with the conclusions we drew from them about how the real refactor should work.

[parazyd]

@str4d For retaining compatibility and more comfortable API usage for users which do not use serde, why not define the encoding and decoding format explicitly in the halo2 crate?

It seems like it would give you finer-grained control over the format, and it is also low overhead for people that use different serialization methods (i.e. they only have to wrap the defined halo2 encoding format in whatever trait their serialization library needs).

[hanzhichao]

any update about the provingkey and verifyingkey serialization?

[themighty1]

@hanzhichao , PSE fork supports key serialization https://github.com/privacy-scaling-explorations/halo2/blob/main/book/src/user/experimental-features.md#provingkey--verifyingkey-deserialization-and-serdeformat