Conversation
RogerTaule
force-pushed
the
fix/proof-size
branch
from
84ab4fb to
eed49ea
Compare
|
|
||
|
|
||
| def get_size_of_merkle_path_bits(num_leafs: int, tuple_size: int, element_size_bits: int, hash_size_bits: int) -> int: | ||
| def get_size_of_merkle_path_bits_fri(num_leafs: int, leaf_size_bits: int, hash_size_bits: int, arity: int, last_level_verification: int) -> int: |
There was a problem hiding this comment.
Can you document the new parameters? Especially last_level_verification seems to need some explanation.
There was a problem hiding this comment.
Sure, I've added a description in the PR, but I will also add it to the code
|
|
||
|
|
||
| def get_size_of_merkle_path_bits(num_leafs: int, tuple_size: int, element_size_bits: int, hash_size_bits: int) -> int: | ||
| def get_size_of_merkle_path_bits_fri(num_leafs: int, leaf_size_bits: int, hash_size_bits: int, arity: int, last_level_verification: int) -> int: |
There was a problem hiding this comment.
Does it make sense to have two functions here? Why not make one function and just set arity = 2 and last_level_verification = 0 as a default, and then call it without setting these parameters in WHIR?
There was a problem hiding this comment.
Probably not, the reason is that I am not familiar with WHIR, and I didnt want to mess up with other PR recently merged such as #34
| batch_size=self.batch_size, | ||
| base_field_size_bits=self.field.base_field_element_size_bits(), | ||
| extension_field_size_bits=self.field.extension_field_element_size_bits(), | ||
| num_columns=self.num_columns, |
There was a problem hiding this comment.
I think it is confusing that batch_size is no longer a parameter of this function. Intuitively, the proof size should depend on the batch size. Also, num_columns seems to be a parameter related to the thing that uses FRI, and does not directly appear as a concept in FRI. Can we remove it, or name it as it is named in FRI?
| # note that we don't need to send the full function, but can just send | ||
| # the polynomial that describes it | ||
| size_bits += rate * n * field_size_bits | ||
| size_bits += n * extension_field_size_bits |
There was a problem hiding this comment.
why did you remove the rate? n is the domain size but the honest prover only needs to send the coefficients, which are rate * n many?
There was a problem hiding this comment.
This is an optimization we do not have, and hence the change, but you are right. Will rollback it
|
|
||
| # one root and one path per query | ||
| size_bits += hash_size_bits + num_queries * get_size_of_merkle_path_bits(num_leafs, tuple_size, field_size_bits, hash_size_bits) | ||
| size_bits += hash_size_bits + num_queries * get_size_of_merkle_path_bits_fri(num_leafs, tuple_size, hash_size_bits, merkle_tree_arity, last_level_verification) |
There was a problem hiding this comment.
It seems odd to me that this is now independent of the field size. At least the opened leaf itself needs to be set as one element. Or did you implicitly change what tuple_size means? I think this should be documented in the get_size_of_merkle_paths_bits_fri function then.
| for c in num_columns: | ||
| size_bits += hash_size_bits + num_queries * get_size_of_merkle_path_bits_fri(num_leafs, c * base_field_size_bits, hash_size_bits, merkle_tree_arity, last_level_verification) | ||
There was a problem hiding this comment.
similar question as above. I think it is weird that the batch size does no longer show up. Can you explain this?
| field: FieldParams | ||
| # Total columns of AIR table | ||
| num_columns: int | ||
| num_columns: list[int] |
There was a problem hiding this comment.
Kinda confused on what is a list of num_columns. I would expect number of columns of the AIR trace to stay static within a FRI run. Is this implying something else? Let's document what it means.
3 participants
This PR modifies how proof size is calculated so that it matches Zisk proof size. (There is a small discrepancy still, I will check it out when I have some availability).
It introduces two new parameters:
It also modifies num_columns to provide number of columns on each stage, since we have different merkle trees for different stages. This is backwards compatible with current solution for other zkvm