-
Notifications
You must be signed in to change notification settings - Fork 8
Make domain size configurable #29
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: georgepisaltu <[email protected]>
|
|
||
| #[cfg(feature = "std")] | ||
| fn ring_prover_params() -> &'static bandersnatch::RingProofParams { | ||
| fn ring_prover_params(domain_size: RingDomainSize) -> &'static bandersnatch::RingProofParams { |
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.
I think it could be enough to just use the spin lock variant of ring_prover_params. I dont think we will have many threads call this during the initialization phase.
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.
I added a todo to address it in the future.
src/ring_vrf_impl.rs
Outdated
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.
Maybe the opening should also be its own trait and not be implemented for BandersnatchVrfVerifiable unless no-std-prover is provided.
Just to avoid possibly panic paths in the runtime. But can be done in another MR for sure.
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.
Gated access to the function.
src/ring_vrf_impl.rs
Outdated
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.
Also should maybe be moved into another trait in the future.
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.
Gated access to the function.
src/ring_vrf_impl.rs
Outdated
| #[ignore = "ring builder generator - generates Domain11 ring data (max 255 members)"] | ||
| fn generate_empty_ring_builder_domain11() { | ||
| generate_ring_builder_for_domain( | ||
| RingDomainSize::Domain11, | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/ring-data/ring-builder-domain11.bin" | ||
| ), | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/ring-data/ring-builder-params-domain11.bin" | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "ring builder generator - generates Domain12 ring data (max 767 members)"] | ||
| fn generate_empty_ring_builder_domain12() { | ||
| generate_ring_builder_for_domain( | ||
| RingDomainSize::Domain12, | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/ring-data/ring-builder-domain12.bin" | ||
| ), | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/ring-data/ring-builder-params-domain12.bin" | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "ring builder generator - generates Domain16 ring data (max 16127 members)"] | ||
| fn generate_empty_ring_builder_domain16() { | ||
| generate_ring_builder_for_domain( | ||
| RingDomainSize::Domain16, | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/ring-data/ring-builder-domain16.bin" | ||
| ), | ||
| concat!( | ||
| env!("CARGO_MANIFEST_DIR"), | ||
| "/src/ring-data/ring-builder-params-domain16.bin" | ||
| ), | ||
| ); | ||
| } |
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.
Could be put in to a function and use the RingDomainSize::ALL to do it for all sizes.
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.
Done.
src/lib.rs
Outdated
| /// Trait for domain size types used in ring operations. | ||
| /// | ||
| /// The domain size determines the maximum ring size that can be supported. | ||
| pub trait DomainSize: Clone + Copy { |
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.
The main concern here is that DomainSize outside the ring implementation doesn't make much sense
(as you correctly wrote: domain size types used in ring operations).
This can instead be confusing, as downstream users may have no knowledge of polynomial commitment schemes or related concepts. These are strictly implementation details of RingProof when it is based on such schemes. From the user's perspective, the only relevant factor is the number of members they are allowed to include.
I suggest removing this trait from this generic module and replacing it with a more appropriate abstraction.
In the GenerateVerifiable trait, you could introduce an associated type such as Capacity, for example:
trait Capacity {
const fn size(&self) -> usize;
}(feel free to choose a better name :-) )
With this approach, the ring implementation can use the ark-vrf helper to derive the required PCS size
(ark_vrf::ring::pcs_domain_size(ring_size)), and then select the appropriate static data based on that value.
In the ring implementation you have:
enum RingSize {
/// Domain size 2^11, max ring size 255
Small,
/// Domain size 2^12, max ring size 767
Mid,
/// Domain size 2^16, max ring size 16127
Big,
}
impl Capacity for RingSize {
const fn size(&self) -> usize {
match self {
Small => max_ring_size_from_pcs_domain_size(1 << 11),
Mid => max_ring_size_from_pcs_domain_size(1 << 12),
Big => max_ring_size_from_pcs_domain_size(1 << 16),
}
}
}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.
I removed the exponent function and redefined the trait and type to be Capacity.
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
…sizes Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Changes in this PR:
RingDomainSizeenum supporting three domain sizes:DomainSizetype with trait toGenerateVerifiableallowingdomain_sizeto be passed to start_members(), open(), validate()builder-params.generate-keysfeature to gate theranddependency used in the--bincompilation to generate keys.