supranational / blst

Multilingual BLS12-381 signature library
Apache License 2.0
454 stars 171 forks source link

Rust Bindings: Replacing slices of references to iterators of references for aggregation #198

Open Coca162 opened 7 months ago

Coca162 commented 7 months ago

Currently, functions like aggregate require a slice of references as input, however these functions do not necessarily require a slice and can be easily changed to accept any iterator instead. I'd be interested in making a PR for this where AggregatePublicKey and AggregateSignature's aggregation functions replace &[&[T] with impl IntoIterator<Item = &T>. This would require some users of the library to change code, but allows them to avoid allocating to a vector, providing a performance benefit and simplifying code.

dot-asm commented 7 months ago

Formally speaking it's unreasonable to expect that users would have to modify their code unless we also increase the non-minor version. One can discuss that, but meanwhile why not start by suggesting an additional interface, say aggregate_iter? This would allow us to weigh pros and cons and maybe extend the approach...

Coca162 commented 7 months ago

Having a aggregate_iter would also work, and it would allow for deprecation warnings to be put on current aggregate if it is decided to be removed in the future.

As for how users will have to modify their code if it's replaced, I think the only scenario will be one like this:

fn test() {
    let references_collection: [&usize; 4] = [&0, &0, &0, &0];

    // Works
    slice_of_ref(&references_collection);

    // Does not work, is a IntoIterator<Item = &&usize>
    into_iter_of_ref(&references_collection);
}

fn slice_of_ref(slice: &[&usize]) { }

fn into_iter_of_ref<'a>(slice: impl IntoIterator<Item = &'a usize>) { }

And can be fixed in either of these 2 ways:

// Consume collection, most likely not ideal
 into_iter_of_ref(references_collection);

// Copy to remove one reference
into_iter_of_ref(references_collection.iter().copied());

This could be possibly be remedied using some other generic trait, though I don't know of such way currently, and is most likely is not possible in current rust.

Coca162 commented 7 months ago

unless we also increase the non-minor version

Since the crate is currently on major version 0 it is allowed to do breaking changes on minor versions:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

https://semver.org/#spec-item-4