Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plonky2 Merkle tree tutorial code #98

Merged
merged 41 commits into from
Sep 19, 2024
Merged

Plonky2 Merkle tree tutorial code #98

merged 41 commits into from
Sep 19, 2024

Conversation

Roee-87
Copy link
Contributor

@Roee-87 Roee-87 commented Sep 9, 2024

This PR adds the Plonky2 tutorial code to the public Sindri-resources repo. This tutorial is referenced in the documentation for the Plonky2 "how-to" guide.

To test the circuit, navigate to sindri-resources/circuit_tutorials/plonky2/merkle_tree.
Change sample.env to .env and enter your Sindri API key.
Run:

cargo run --release

@Roee-87 Roee-87 requested a review from grahamu September 9, 2024 19:25
@Roee-87 Roee-87 self-assigned this Sep 9, 2024
@grahamu
Copy link
Contributor

grahamu commented Sep 10, 2024

Successfully tested locally.

❯ cargo run --release
   Compiling sindri-script v0.1.0 (/Users/graham/code/sindri/sindri-resources/circuit_tutorials/plonky2/merkle_tree)
warning: unused import: `json`
  --> src/main.rs:12:18
   |
12 | use serde_json::{json, Value};
   |                  ^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `iop::target::Target`
  --> src/main.rs:23:5
   |
23 |     iop::target::Target,
   |     ^^^^^^^^^^^^^^^^^^^

warning: `sindri-script` (bin "sindri-script") generated 2 warnings (run `cargo fix --bin "sindri-script"` to apply 2 suggestions)
    Finished `release` profile [optimized] target(s) in 4.97s
     Running `target/release/sindri-script`
Compiling circuit
Circuit ID: "e5004c2d-78e3-4e0b-bb53-a96c8988eeb5"
Saving circuit details locally
Reading circuit details locally
Reading proof input from input.json file
Requesting a proof
Saving proof details locally
Proof has been verified!

@grahamu
Copy link
Contributor

grahamu commented Sep 10, 2024

@Roee-87 please rename the circuit directory from merkle_tree_circuit to circuit, as seen in our other framework tutorials. Update directory references as needed.

@Roee-87
Copy link
Contributor Author

Roee-87 commented Sep 10, 2024

@grahamu I've changed the name of the circuit directory per your comment: 6c0b283

Comment on lines 46 to 49
// while total_leaves > 1 {
// total_leaves = (total_leaves + 1) / 2;
// layers += 1;
// }
Copy link
Contributor

@grahamu grahamu Sep 12, 2024

Choose a reason for hiding this comment

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

Is the commented code useful for the example?

Suggested change
// while total_leaves > 1 {
// total_leaves = (total_leaves + 1) / 2;
// layers += 1;
// }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commented out block was removed in commit: af5823a

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually af5823a removed commented code in the circuit_tutorials path. This still exists in the circuit_database path and still needs removal.

Copy link
Contributor

@grahamu grahamu left a comment

Choose a reason for hiding this comment

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

This is getting better, but still a lot of code style issues.

println!("Verified: {:?}", verified);

// Sindri always returns this information for every Plonky2 circuit.
// The proof, the verifier data, and the common data contain all the infomration required
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The proof, the verifier data, and the common data contain all the infomration required
// The proof, the verifier data, and the common data contain all the information required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 3bb8121

writer.flush().unwrap();
}

// This function proves the circuit using the input data provided by the user and saves the proof in the /data/ directory.
Copy link
Contributor

@grahamu grahamu Sep 19, 2024

Choose a reason for hiding this comment

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

Perhaps this comment was added after the file was rustfmted for line length?

Suggested change
// This function proves the circuit using the input data provided by the user and saves the proof in the /data/ directory.
// This function proves the circuit using the input data provided by the user.

Can we add a rustfmt "check" step in the .github/workflows/ci.yaml file which verifies conformance to our accepted Rust formatting rules, whatever they are? This could be a followup task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a ticket to explore the feasibility of implementing such a check. According to [this discussion ](this discussion ), there is a field called wrap_comments which can be set to true in the rustfmt config file. However, this is not "stable" -- meaning that it's likely only available in nightly Rust. Which is a bit od...as the initial discussion post dates back to 2016...
Adding this to my do-do list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

committed your suggested change: aabc9d2

Comment on lines 59 to 60
// This function uploads the circuit to Sindri for compilation and outputs the circuit details as
// a JSON file in the the /data/ directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This function uploads the circuit to Sindri for compilation and outputs the circuit details as
// a JSON file in the the /data/ directory.
// This function uploads the circuit to Sindri for compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 7c64f23

Comment on lines 157 to 158
// This function verifies the proof using the proof data generated by Sindri and gets saved locally
// in the /data/ directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This function verifies the proof using the proof data generated by Sindri and gets saved locally
// in the /data/ directory.
// This function verifies the proof using the proof data generated by Sindri.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 1ff8df9

Copy link
Contributor

@grahamu grahamu left a comment

Choose a reason for hiding this comment

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

Nice work, nice improvements to the code documentation. Here are responses to your comments found elsewhere:

  1. The Plonky2 circuit directory in circuit_database contain a duplicate of the merkle_tree circuit. I've removed it since it's redundant (in line with how the other frameworks organize their sample circuits).

Great!

  1. The code in merkle_tree.rs is copied from a research group called hashcloak. In the how-to guide, I've linked to the original code. I'd be reluctant to modify the comments, as it's third party code which has explicit attribution in the how-to guide. I can add a note in the README.md to clarify that again.

Yes please. I vaguely recall you telling me just that a while back, so I apologize for complaining about the code quality. Let's make it very clear that merkle_tree.rs is imported from that source untouched.

  1. The main.rs is cleaned up but I think the comments might be a bit too busy. IMO the block of code in the main block doesn't need explicit comments explaining how circuit and proof artifacts are saved locally -- that's made explicit in comments for the function definitions.

Agreed. I made suggestions in this PR which simplify the docstrings.

Finally, yes I said 100 chars for max comment length, but frankly after reading the revised (with rustfmt?) codebase I'd prefer 80 chars maximum for comments since most of the lines don't exceed 80 chars. In Python we restrict line length to 100 chars, but try to keep block comments shorter. I'm not sure that's possible with rustfmt, maybe something to discuss.

Copy link
Contributor

@grahamu grahamu left a comment

Choose a reason for hiding this comment

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

LGTM pending one more test by @Roee-87 .

@Roee-87 Roee-87 merged commit 8047402 into main Sep 19, 2024
1 check passed
@Roee-87 Roee-87 deleted the rr-plonky2-merkle-tree branch September 19, 2024 16:42
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