Add snapshot test of privacy bootloader program hash#335
Add snapshot test of privacy bootloader program hash#335YairVaknin-starkware wants to merge 1 commit into
Conversation
e644cb1 to
ceac2de
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
=======================================
Coverage 53.16% 53.16%
=======================================
Files 35 35
Lines 5295 5295
=======================================
Hits 2815 2815
Misses 2480 2480 🚀 New features to boost your workflow:
|
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and YairVaknin-starkware).
crates/privacy_circuit_verify/src/consts.rs line 30 at r1 (raw file):
// branch: "dev" // commit: "f65af209cc2a245a7d1c90711b49555ade65dd8c" pub const PRIVACY_BOOTLOADER_BYTES: &[u8] = include_bytes!(
Please add md5sum of the json (so that we know the comment refers to the right binary and not obsolete)
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and yuvalsw).
crates/privacy_circuit_verify/src/consts.rs line 30 at r1 (raw file):
Previously, yuvalsw wrote…
Please add md5sum of the json (so that we know the comment refers to the right binary and not obsolete)
See my comment in slack. debug data is pretty dynamic, so md5sum won't help here. We can add the program hash and specify how to calculate it.
53e449b to
6988ac0
Compare
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw made 1 comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and YairVaknin-starkware).
crates/privacy_circuit_verify/src/consts.rs line 30 at r1 (raw file):
Previously, YairVaknin-starkware wrote…
See my comment in slack. debug data is pretty dynamic, so md5sum won't help here. We can add the program hash and specify how to calculate it.
md5sum of the json added to the repo. The idea is - if you change the file but forget to change this comment (hopefully won't happen), when you read it next time you realize that this happened and don't wonder why it doesn't produce the same program.
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw made 1 comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on Yael-Starkware and YairVaknin-starkware).
crates/privacy_prove/src/tests.rs line 14 at r2 (raw file):
// Source code for the compiled privacy bootloader producing the following hash can be found at // Starkware's internal main repo at commit "f65af209cc2a245a7d1c90711b49555ade65dd8c".
Can you add the compiler version/hash that was used to compile and the exact command to compile it?
6988ac0 to
a44db8c
Compare
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and yuvalsw).
crates/privacy_prove/src/tests.rs line 14 at r2 (raw file):
Previously, yuvalsw wrote…
Can you add the compiler version/hash that was used to compile and the exact command to compile it?
Done
crates/privacy_circuit_verify/src/consts.rs line 30 at r1 (raw file):
Previously, yuvalsw wrote…
md5sum of the json added to the repo. The idea is - if you change the file but forget to change this comment (hopefully won't happen), when you read it next time you realize that this happened and don't wonder why it doesn't produce the same program.
"md5sum of the json added to the repo." Added already you mean? Where? No sure I get the meaning of this sentence. But what I mean is we can have the file be different if compiled on different machine or from a different path, but from the same commit, so the comment shouldn't change. Do you still want it there just so we are mindful of that if for any reason this scenario occurs?
The test I added should take care of the program segment, so we know of any changes to the bytecode.
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware reviewed 4 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and yuvalsw).
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on Yael-Starkware and YairVaknin-starkware).
crates/privacy_circuit_verify/src/consts.rs line 30 at r1 (raw file):
Previously, YairVaknin-starkware wrote…
"md5sum of the json added to the repo." Added already you mean? Where? No sure I get the meaning of this sentence. But what I mean is we can have the file be different if compiled on different machine or from a different path, but from the same commit, so the comment shouldn't change. Do you still want it there just so we are mindful of that if for any reason this scenario occurs?
The test I added should take care of the program segment, so we know of any changes to the bytecode.
I meant "md5sum of the json that was added to the repo".
crates/privacy_prove/src/tests.rs line 13 at r3 (raw file):
.expect("Failed to compute program hash."); // Source code for the compiled privacy bootloader producing the following hash can be found at
here. please add main repo branch, and md5sum of the json (compiled BL) as discussed in the other comment.
Make it a template so that it's well updated everytime.
Starkware's internal main repo (branch+commit):
Compiled with cairo-compile (version/branch+commit):
md5sum of the compiled BL (privacy_simple_bootloader_compiled.json):
Compilation command:
crates/privacy_circuit_verify/src/consts.rs line 26 at r2 (raw file):
RECURSIVE_PROOF_UNCOMPRESSED_BYTES * PROOF_MAX_DECOMPRESSED_RATIO; pub const PRIVACY_BOOTLOADER_BYTES: &[u8] = include_bytes!(
Add a comment here saying the details of how this was created can be found at ...
a44db8c to
310e2df
Compare
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on Yael-Starkware and yuvalsw).
crates/privacy_prove/src/tests.rs line 13 at r3 (raw file):
Previously, yuvalsw wrote…
here. please add main repo branch, and md5sum of the json (compiled BL) as discussed in the other comment.
Make it a template so that it's well updated everytime.
Starkware's internal main repo (branch+commit):
Compiled with cairo-compile (version/branch+commit):
md5sum of the compiled BL (privacy_simple_bootloader_compiled.json):
Compilation command:
Done.
crates/privacy_circuit_verify/src/consts.rs line 30 at r1 (raw file):
Previously, yuvalsw wrote…
I meant "md5sum of the json that was added to the repo".
I'm not sure what md5sum will give us now that we have the "important" stuff covered by the new test. Or do you just want it so we know there is any change to it even if the same program essentially? Anyway, done in the test. But can be added to the snapshot test if we want, so it's not left unmaintained.
crates/privacy_circuit_verify/src/consts.rs line 26 at r2 (raw file):
Previously, yuvalsw wrote…
Add a comment here saying the details of how this was created can be found at ...
Done.
yuvalsw
left a comment
There was a problem hiding this comment.
@yuvalsw reviewed 3 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and YairVaknin-starkware).
crates/privacy_prove/src/tests.rs line 17 at r4 (raw file):
// Branch: "dev". // md5sum of "privacy_simple_bootloader_compiled.json": "88560d8862eb38dc482096b868f5ef7e" // Compiled with cairo-compile v0.14.2 (from same commit as the bootloader):
not sure I understand. 0.14.2 or the commit specified for the BL above? (f65...)
Also, what is cairo_path? another repo?
Code quote:
(from same commit as the bootloader):
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Yael-Starkware and yuvalsw).
crates/privacy_prove/src/tests.rs line 17 at r4 (raw file):
Previously, yuvalsw wrote…
not sure I understand. 0.14.2 or the commit specified for the BL above? (f65...)
Also, what is cairo_path? another repo?
see answer in slack
Type
Description
Breaking changes?
Note
Low Risk
Low risk: adds test-only coverage and a new dev dependency without changing runtime behavior. Potential risk is minor CI churn if the embedded bootloader bytes/program hashing changes.
Overview
Adds a new unit snapshot test in
privacy-provethat computes the privacy bootloader’s stripped-program hash (Blake) and asserts it against a pinned expected value, including a comment pointing to the source commit for the compiled artifact.Introduces
expect-testas a workspace dependency and as aprivacy-provedev-dependency to support the snapshot assertion.Written by Cursor Bugbot for commit 6988ac0. This will update automatically on new commits. Configure here.
This change is