Conversation
e660c7a to
27129a6
Compare
|
I wrote cleaner traits for EC VRFs and EC ring VRFs here : In particular, they handle multiple input-output pairs better, which many ring VRF protocols require. |
Perhaps this is not related to this PR :-) In this PR, we make the Verifiable trait generic over the specific Bandersnatch implementation. This enables the use of the implementation backed by the host calls we are currently working on. |
ggwpez
left a comment
There was a problem hiding this comment.
Is it rebased? GH wont show me
src/ring/bandersnatch.rs
Outdated
| use crate::ring::{Bls12_381RingData, RingSuiteExt, RingVrfVerifiable}; | ||
| use ark_vrf::suites::bandersnatch::{BandersnatchSha512Ell2, RingProofParams}; | ||
|
|
||
| #[cfg(any(feature = "std", feature = "no-std-prover"))] |
There was a problem hiding this comment.
| #[cfg(any(feature = "std", feature = "no-std-prover"))] | |
| #[cfg(feature = "prover")] |
And then enable that by feature std.
|
|
||
| /// Tests that require the `builder-params` feature. | ||
| #[cfg(all(test, feature = "builder-params"))] | ||
| mod builder_tests { |
There was a problem hiding this comment.
Could put into bandersnatch_tests.rs file or so
There was a problem hiding this comment.
In that case, I would also be inclined to move the entire set of Bandersnatch-related tests (the mod test above) into that file. However, why move the tests to a separate file? The current module does not appear particularly cluttered.
| type Handle = alloc::boxed::Box<ark_vrf::ring::RingProofParams<S>>; | ||
|
|
||
| fn get(domain_size: RingDomainSize) -> Self::Handle { | ||
| alloc::boxed::Box::new(make_ring_prover_params::<S>(domain_size)) |
There was a problem hiding this comment.
The decoding is not cached with a Once cell anymore?
There was a problem hiding this comment.
I introduced RingProofParamsCache as an associated type of the RingSuiteExt trait.
We provide two concrete implementations of this trait:
(): This is the implementation you observed. It performs no parameter caching. Perhaps I can define aNullCachetype, which may be more explicit.BandersnatchParamsCache: Defined in thebandersnatch.rsmodule. It implements caching similarly to the previous approach, except that it uses thespincrate for bothstdandno_stdenvironments.
This abstraction enables users to define a custom caching strategy if the default implementations provided by the crate do not meet their requirements
There was a problem hiding this comment.
Okay makes sense, thanks. I must have missed it in the review.
There was a problem hiding this comment.
I support calling it NullCache or NoCache or anything like that.
I force-pushed 🫣 |
|
@ggwpez @georgepisaltu As I'd like to move forward, If there are no objections by tomorrow, I will proceed with merging this. |
georgepisaltu
left a comment
There was a problem hiding this comment.
Sorry for the late review. Just a couple of comments, but looks good.
| type Handle = alloc::boxed::Box<ark_vrf::ring::RingProofParams<S>>; | ||
|
|
||
| fn get(domain_size: RingDomainSize) -> Self::Handle { | ||
| alloc::boxed::Box::new(make_ring_prover_params::<S>(domain_size)) |
There was a problem hiding this comment.
I support calling it NullCache or NoCache or anything like that.
src/ring/mod.rs
Outdated
| } | ||
|
|
||
| pub trait EncodedTypesBounds: | ||
| Clone + Eq + FullCodec + core::fmt::Debug + TypeInfo + MaxEncodedLen + AsRef<[u8]> + AsMut<[u8]> |
There was a problem hiding this comment.
Can we also add DecodeWithMemTracking since this will be used in the runtime? It's not covered by FullCodec.
src/ring/mod.rs
Outdated
|
|
||
| #[inline(always)] | ||
| fn make_alias<S: RingSuiteExt>(output: &ark_vrf::Output<S>) -> Alias { | ||
| Alias::try_from(&output.hash()[..32]).expect("Suite hash should be at least 32 bytes") |
There was a problem hiding this comment.
Technically this will panic when you try to access the slice like &output.hash()[..32] before we get to the try_from. There's not much of a difference even if we write this differently and guard against it, but maybe we can statically check with a test or something that the hash output on the ark_vrf::Output<S> we use is at least 32 bytes?
Major refactory:
RingSuiteExt: Enables easy integration of alternative implementations, such as those backed by polkadot-sdk hostcalls (see draft: Bandersnatch with EC hostcalls polkadot-sdk#10259).
RingCurveData: Centralizes pairing-curve-specific data. Bandersnatch uses BLS12-381, which can also be reused by other curves (i.e. bandersnatch alternatives hosted by BLS12-381), as well as by third-party Bandersnatch implementations (e.g., the polkadot-sdk hostcall-based version mentioned above).
RingProofParamsCache: Supports multiple strategies for caching pre-built parameters.
Bandersnatch concrete implementation (based on suite provided by ark-vrf)
Codebase reorganization