-
Notifications
You must be signed in to change notification settings - Fork 155
Add CompactBinary proof serialization #1185
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
base: dev
Are you sure you want to change the base?
Add CompactBinary proof serialization #1185
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some tests assert_eq!(deserialize(serialize(a))
- add the doc describing the spec (might also be good to add it above the trait
if let Type::Path(type_path) = &f.ty { | ||
let segments = &type_path.path.segments; | ||
if let Some(seg) = segments.last() { | ||
if let syn::PathArguments::AngleBracketed(ref args) = seg.arguments { | ||
args.args.iter().any(|arg| { | ||
if let syn::GenericArgument::Type(Type::Path(type_path)) = arg { | ||
type_path | ||
.path | ||
.segments | ||
.last() | ||
.is_some_and(|s| s.ident == "H") | ||
} else { | ||
false | ||
} | ||
}) | ||
} else { | ||
false | ||
} | ||
} else { | ||
false | ||
} | ||
} else { | ||
false | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this crate is using nightly so i think you should use if-let-chains
which would make the whole thing cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Done in 27587b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @0xLucqs)
crates/stwo/src/core/compact_binary.rs
line 384 at r3 (raw file):
#[cfg(test)] mod tests { use num_traits::One;
Move the tests to a file module.
Code quote:
#[cfg(test)]
mod tests {
use num_traits::One;
Thank you, done in the latest commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 9 files at r2, 1 of 2 files at r4, all commit messages.
Reviewable status: 5 of 11 files reviewed, 3 unresolved discussions (waiting on @0xLucqs)
crates/compact-binary/README.md
line 10 at r4 (raw file):
- `cairo-serde`: the proof is first converted into a `Vec<FieldElement>`, that is then serialized in a serde json format. The aim is to design and implement a third proof serialization format, `compact-binary`, where the proof is serialized as a `Vec<u8>` in a compact way.
Suggestion:
This crate implements a third proof serialization format, `compact-binary`, where the proof is serialized as a `Vec<u8>` in a compact way.
crates/compact-binary/src/lib.rs
line 43 at r4 (raw file):
/// | example_proof.cairo_serde | cairo-serde | 2 448 494 | - 3.1 % | /// | example_proof.compact_bin_unzipped | compact-binary | 834 606 | - 67.0 % | /// | example_proof.compact_bin_zipped | compact-binary | 582 932 | - 76.9 % |
This appears in the readme, I think it can be removed from here.
Code quote:
/// ## Benchmarks
/// The following table shows the size of a proof serialized in different formats, using the
/// `compact-binary` format with and without zipping.
///
/// | File | Format | Size on disk (bytes) | Gain |
/// |------------------------------------|-------------------|---------------------:|---------:|
/// | example_proof.base_json | json | 2 528 114 | -- |
/// | example_proof.cairo_serde | cairo-serde | 2 448 494 | - 3.1 % |
/// | example_proof.compact_bin_unzipped | compact-binary | 834 606 | - 67.0 % |
/// | example_proof.compact_bin_zipped | compact-binary | 582 932 | - 76.9 % |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 11 files reviewed, 4 unresolved discussions (waiting on @0xLucqs and @Leo-Besancon)
crates/compact-binary/README.md
line 37 at r4 (raw file):
Note that the proc macro supports the `#[zipped]` attribute to specify that a given field should be zipped. - Added error handling through `CompactDeserializeError` enum and `CompactSerializeError` struct - Implemented the trait for structures in `stwo` crate used for CairoProofs.
I’d suggest changing the wording here from past tense (“Added”, “Implemented”) to present/descriptive tense, since this section should describe the current state of the code rather than the history of changes. For example:
- “Added
CompactBinary
trait …” → “CompactBinary
trait incrates/compact-binary/src/lib.rs
…” - “Implemented this trait …” → “Implementations of this trait …”
- “Added error handling …” → “Error handling through …”
Code quote:
- Added `CompactBinary` trait in `crates/compact-binary/src/lib.rs`, and implemented this trait for all structures needed.
- Added a `#[derive(CompactBinary)]` proc macro to implement the trait for structures composed of fields implementing it. Note that the proc macro is only expected to produce a `0` version, if a given structure is to be updated it's implementation should be done manually, while keeping back-compatibility of all previous serialization versions for this structure. See `crates/compact-binary-derive/src/lib.rs`
Note that the proc macro supports the `#[zipped]` attribute to specify that a given field should be zipped.
- Added error handling through `CompactDeserializeError` enum and `CompactSerializeError` struct
- Implemented the trait for structures in `stwo` crate used for CairoProofs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gilbens-starkware reviewed 1 of 7 files at r1, 3 of 9 files at r2, 1 of 1 files at r3, 1 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @0xLucqs)
This PR, along with it's counterpart in stwo-cairo (starkware-libs/stwo-cairo#1136), aims to provide compact serialization of Starknet proofs in order to limit network usage.
CompactBinary
trait instwo_cairo_prover/crates/cairo-serialize/src/compact.rs
, and implemented this trait for all structures needed.#[derive(CompactBinary)]
proc macro to implement the trait for structures composed of fields implementing it. Note that the proc macro is only expected to produce a0
version, if a given structure is to be updated it's implementation should be done manually, while keeping back-compatibility of all previous serialization versions for this structure. Seestwo_cairo_prover/crates/cairo-serialize-derive/src/lib.rs