Skip to content

feat: proof parser + query-wise verifier#12

Draft
Al-Kindi-0 wants to merge 1 commit into
mainfrom
proof-parser
Draft

feat: proof parser + query-wise verifier#12
Al-Kindi-0 wants to merge 1 commit into
mainfrom
proof-parser

Conversation

@Al-Kindi-0

@Al-Kindi-0 Al-Kindi-0 commented Aug 12, 2022

Copy link
Copy Markdown
Owner

This PR proposes a draft implementation of a FRI verifier which runs the query phase of the FRI protocol in a query-by-query fashion. To do this, this PR also includes a potential modification to the verifier part of the channel so that the proof generated by the prover, which looks at all the queries in certain layer in one go, can be parsed for the new verifier implementation in this PR.
The interface of both the unbatch function as well as the verify_generic_query function is, in my opinion, suboptimal. The reason being that, in order to parse the proof, the unbatch function requires the array of queried positions and this makes the design of the interfaces, based on the current one, a challenge.

@Al-Kindi-0 Al-Kindi-0 requested a review from bobbinth August 12, 2022 18:50

@bobbinth bobbinth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! And sorry for such a delay with the review.

This is not a full review yet - just a few suggestions about high-level changes to improve code clarity/organization. Feel free to disagree with any of these.

Unbatch PR

I'd rename BatchMerkleProof::unbatch() method into something like BatchMerkleProof::into_paths() and move this work into a separate PR. We should probably make this PR against the main Winterfell repo - I think it would be pretty useful to have this functionality there.

Verify query

I'd create a stand-alone function for verifying a single query (it seems like the current verify_generic_query still iterates over a set of positions). I'd also try to make it as as self-contained as possible - so that it is super clear what the function requires as inputs / produces as outputs. For example, I'd probably not have it take self and channel as arguments but instead would pass w/e is needed from these structs. I think the arguments we need are (but I may be missing something):

  • layer commitments
  • layer alphas
  • position
  • evaluation
  • a Merkle path per layer
  • options

I think it is OK to have these many parameters as we need this primarily for reference purposes rather than production code.

Also, it would be cool to add some kind of hash counter so that we can estimate how many invocations of a hash function required for verifying a given FRI proof.

FRI changes in Winterfell

I wonder if we should make a PR to update FRI implementation in Winterfell to allow the following:

  • Folding factor of 2 (currently, 4 is the smallest).
  • Go down to the smallest possible remainder (to simplify remainder testing).

Though, I guess these should be driven by our understanding of how much not having the above would complicate things inside Miden VM.

@bobbinth

Copy link
Copy Markdown
Collaborator

One more thought regarding verify query: instead of passing a Merkle path per layer, let's pass a struct which closely mimics the mtree_get instruction. That is, it exposes a function with roughly this interface:

fn get_node(root: Digest, depth: usize, index: usize) -> Digest

The above function would return the value of the node from the tree with the specified root. The node would be located in the tree at the specified index and depth. If the tree with the specified root is not present in the struct, or the specified node is not present in the tree - the function should panic.

To implement this struct, you can probably adapt the code form AdviceProvider and MerklePathSet structs.

@Al-Kindi-0

Al-Kindi-0 commented Aug 22, 2022

Copy link
Copy Markdown
Owner Author

Thank you Bobbin!
Unbatch PR: It makes sense, I have gathered the code as well as relevant tests into a separate PR.
Verify query: I have factored the portion you mentioned into a separate function let me know if it is what you were thinking. (I have made a separate PR based on the Unbatch PR).
FRI changes: That should be straightforward, I'll work on it.

@Al-Kindi-0

Copy link
Copy Markdown
Owner Author

One more thought regarding verify query: instead of passing a Merkle path per layer, let's pass a struct which closely mimics the mtree_get instruction. That is, it exposes a function with roughly this interface:

fn get_node(root: Digest, depth: usize, index: usize) -> Digest

The above function would return the value of the node from the tree with the specified root. The node would be located in the tree at the specified index and depth. If the tree with the specified root is not present in the struct, or the specified node is not present in the tree - the function should panic.

To implement this struct, you can probably adapt the code form AdviceProvider and MerklePathSet structs.

Here I am bit not sure about what you mean. More specifically:

  • Should get_node return a Word instead?
  • Should the data structure in question include all Merkle paths for all layers?
  • How does this change the way we do the verification of the Merkle proofs?
    Thank you!

@bobbinth

Copy link
Copy Markdown
Collaborator

@Al-Kindi-0 Thank you! I'll leave some comments in the other PRs. To answer the above questions:

  1. Word and Digest are basically the same thing for Rescue Prime (a digest consists of 4 field elements). I think it should be relatively easy to convert from one to the other.
  2. Yes, it would contain all paths for all layers. We should probably organize it similar to AdviceProvider where we have AdviceSets indexed by root, and then within each advice set we have a collection of paths. To simplify things, we could probably just use MerklePathSet struct - since we know that in our case, we won't have any other types of advice sets.
  3. I think we should assume that if get_node returns a value and doesn't panic, then the path for the node in question has been verified. But maybe I misunderstood the question?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants