Repository navigation
Conversation
|
Overall success rate on test cases: 100.0% No test improvements or size changes detected. Comparison Base DetailsCompared against: STATUS.md artifact from |
|
Overall success rate on test cases: 100.0% No test improvements or size changes detected. Comparison Base DetailsCompared against: Committed STATUS.md at |
|
Overall success rate on test cases: 100.0% No test improvements or size changes detected. Comparison Base DetailsCompared against: Committed STATUS.md at |
iamrecursion
left a comment
iamrecursion
left a comment
There was a problem hiding this comment.
Good stuff in general, but a few concerns:
- We no longer have a digest that covers just the circuit. The only digest now covers bytecode and ABI JSON, so a codegen change can move the digest even if the circuit is the same. I think we should additionally expose a
circuit_digest()that covers the R1CS plus the manifest, and the program digest should cover that plus the bytecode and ABI. - As is, the compiler version is hard to use for diagnostics as it can only be read after a fully successful parse. Make it peekable so that it can be used for errors in place of the static
COMPILER_VERSIONorCARGO_PKG_VERSION. verifier.jsonis not tied to the artifact it came from in any way. It should be.- Nothing runs witgen or AD from
artifact.bytecode()withartifact.r1cs().compile_writes_one_artifactonly checks the header'sentry_blob_field_count. One round-trip run would cover how consumers actually use the file, so should be added.
|
Overall success rate on test cases: 100.0% No test improvements or size changes detected. Comparison Base DetailsCompared against: STATUS.md artifact from |
|
Overall success rate on test cases: 100.0% No test improvements or size changes detected. Comparison Base DetailsCompared against: Committed STATUS.md at |
iamrecursion
left a comment
iamrecursion
left a comment
There was a problem hiding this comment.
The style of the new documentation is extremely verbose and hard to follow. Please try and make it much more clear and concise.
| //! The magic, format version and compiler version form the [`ArtifactPreamble`], laid out the same | ||
| //! in every format version so that a reader can name the compiler that wrote a file it cannot |
There was a problem hiding this comment.
It is not laid out the same in every format version. If you mean to make a promise about future compatibility, then please make that explicitly.
| ("entry past the end", |b| b[17] = 99), | ||
| ("no code after the header", |b| b.truncate(19)), | ||
| ("no AD entry", |b| b[18] = NO_ENTRY_POINT as u64), | ||
| ("witgen entry only", |b| { |
There was a problem hiding this comment.
This case never reaches the missing-AD branch as the case removes word 19 but the remaining entry still points at 19. The bytecode is now only 19 words long, so it fails on the "header points past its own end" check instead.
| } | ||
|
|
||
| /// The digest is a compatibility promise to consumers that record it: pin its encoding. | ||
| /// Words: 0 magic, 1 version, 2 field, 3 layout tag, 4..12 layouts, 12 pool length, |
There was a problem hiding this comment.
This word map is off by one from word 12 on.
| return Err("the bytecode's constraint layouts disagree with the R1CS".to_string()); | ||
| } | ||
| let len = bytecode.len(); | ||
| if header.code_start > len |
There was a problem hiding this comment.
header.code_start > len is always false as far as I can tell.
| || header | ||
| .entry_points | ||
| .iter() | ||
| .any(|&entry| entry != NO_ENTRY_POINT && entry >= len) |
There was a problem hiding this comment.
This bounds check is not as strong as the layout it guards. An entry is a function marker, with its frame size at +1 and its first opcode at +2. Yet entry >= len accepts an entry at the last word or inside the header.
| /// bytecode and ABI: the identity of the whole file, which the header carries. Pin it to mean | ||
| /// "these exact bytes". |
There was a problem hiding this comment.
This is inaccurate as the compiler string is outside the digest. Say what it actually means.
| matrices are required at runtime. Compare the verifier's `circuit_digest` with the artifact's before | ||
| trusting the pair. Verifier execution uses a dedicated forward bytecode entry point: witness writes |
There was a problem hiding this comment.
This claims more than the check actually provides. The circuit_digest in verifier.json is a field the file declares about itself. Nothing hashes the verifier bytecode or binds it to that value. So the comparison catches mixing up files from different circuits, not a forged verifier.
The base's wording was much more accurate.
bn254bn254 success rate on test cases: 100.0% No test improvements or size changes detected. goldilocksgoldilocks success rate on test cases: 50.5% No test improvements or size changes detected. Comparison Base DetailsCompared against: STATUS.md artifact from |
bn254bn254 success rate on test cases: 100.0% No test improvements or size changes detected. goldilocksgoldilocks success rate on test cases: 51.6% No test improvements or size changes detected. Comparison Base DetailsCompared against: Committed STATUS.md at |
Summary
mavros compileused to write two files that only work as a pair:r1cs.bin(header, digest, manifest, R1CS) andbasic.json(bytecode and ABI, unversioned, outside the digest). Nothing tied them together, so a stale pair could make the manifest and the bytecode disagree on the witness layout. This PR folds everything into one file (target/program.mavros).Supersedes #424, which carried the same three commits stacked on #414 from a fork.
Closes #418