Serialize and hash custom predicates #90
Conversation
There was a problem hiding this comment.
Nice!
A concern I have is that current code allows to define a Params.hash_size=4 but on the same time having the method that returns a size different than 4.
This is, each type (Statement, Hash, Value, etc) self-defines which is their amount of fields returned in their .to_field method, which is not affected by the Params.hash_size value.
For example, the PodId.to_fields gets a hash_size in the given
Params.hash_size passed as input at
src/middleware/mod.rs#L192 but then internally this is going to return the amount of fields defined at the method .to_fields from the Hash type, which depends only on the definition of the type itself, not on the given parameters Params; which currently is 4, but it could be that a Hash has 8 field elements (in the case of for example using ~32 bit fields as in Plonky3), so that that method would be returning a vector of length 8 despite the Params.hash_size=4. We then should fix it and make the code values match, but ideally we find a way in which the code enforces this match without us having to pay attention (for example not compiling if those values don't match).
One thing I'm thinking is that maybe we should define a parameter/constant that defines the "minimal unit length" (in plonky2 this is 4, in plonky3 this is 8, in circom this is 1 (or maybe 2 if we want to support values between 253 and 256 bits too)). And that this parameter should be defined by the backend, so then if we swap backends, the middleware and frontend know this parameter from the backend being used; and this parameter is what would be used for the length when returning at the methods .to_fields().
| fn to_fields(&self, params: Params) -> (Vec<F>, usize) { | ||
| (self.0.to_vec(), 4) |
There was a problem hiding this comment.
should it use the params.hash_size instead of 4?
| // Key(hash_or_wildcard1, hash_or_wildcard2) | ||
| // => (2, [hash_or_wildcard1], [hash_or_wildcard2]) | ||
| // In all three cases, we pad to 2 * hash_size + 1 = 9 field elements | ||
| let hash_size = 4; |
There was a problem hiding this comment.
Should it take the value from params.hash_size?
This resolves #75
Please take a look at my code and give me pointers for coding style.
paramsas an input toto_fieldsso thatto_fieldscan pad everything to a fixed length. I think we have to do this because eventually when we do everything in-circuit, we're going to need to be able to find everything at a constant offset. Is there a better way to deal with this issue?to_fields()implementations chain together a bunch of vectors, my code is ugly, please suggest more idiomatic Rust.