Add allocation-free MultiscalarMul method

[daxpedda]

This renames MultiscalarMul::multiscalar_mul() to multiscalar_alloc_mul() and adds a new multiscalar_mul() method taking fixed-sized arrays. This enables the usage of allocation-free multiscalar multiplication.

To that end the following changes were made:

• Backend implementations were refactored so both multiscalar_mul() and multiscalar_alloc_mul() can share the same code.
• straus module is now exposed without the alloc crate feature.
• MultiscalarMul::multiscalar_alloc_mul() is now guarded behind cfg(feature = "alloc"). If we intend users to implement MultiscalarMul on their own custom curves having a required trait method behind a crate feature is pretty bad UX.

This could be extended to cover VartimeMultiscalarMul and VartimePrecomputedMultiscalarMul, but I don't have a use-case personally.
The new fixed-array method also looses a lot of the flexibility the new multiscalar_alloc_mul() offers. I'm happy to sync that flexibility, but it would require calling it like so: RistrettoPoint::multiscalar_mul::<12, _, _>(...). Basically the compiler can't infer const N.

This change isn't exactly small and wasn't discussed beforehand. I'm fully prepared to consider any large changes to implement this feature.

[tarcieri]

As a data point, here's how this was handled in elliptic-curve: with a trait that supports multiple overlapping impls

https://docs.rs/elliptic-curve/0.14.0-rc.10/elliptic_curve/ops/trait.LinearCombination.html

See also:

https://docs.rs/primeorder/0.14.0-pre.7/primeorder/struct.ProjectivePoint.html#impl-LinearCombination%3C%5B(ProjectivePoint%3CC%3E,+%3CC+as+CurveArithmetic%3E::Scalar)%5D%3E-for-ProjectivePoint%3CC%3E

[daxpedda]

Its not exactly possible to adapt it to this model while keeping all of the current features in this case.
The parameter for the existing trait is I: IntoIterator, but the new functionality has [_; N]. This would cause a conflicting trait implementation error.

As I mentioned above we could keep IntoIterator for the fixed array implementation as well, but the user has to pass const N as a separate generic, which can't be inferred by any of the parameters. We could then do something like this:

impl MultiscalarMul<Vec<?>> for Point {
    ...
}

impl<const N: usize> MultiscalarMul<[?; N]> for Point {
    ...
}

We could maybe put Scalar in place of the ? here. In any case, this would have even worse UX:

<EdwardsPoint as MultiscalarMul<[?; 12]>>::multiscalar_mul(...);

So yeah, I'm open to new ideas.

Alternatively we could go down the elliptic_curve path and just drop the IntoIterator functionality. I think it might be possible in the future with specialization?

[tarcieri]

The parameter for the existing trait is I: IntoIterator, but the new functionality has [_; N]. This would cause a conflicting trait implementation error.

I think you missed the two overlapping impls:

https://docs.rs/primeorder/0.14.0-pre.7/primeorder/struct.ProjectivePoint.html#impl-LinearCombination%3C%5B(ProjectivePoint%3CC%3E,+%3CC+as+CurveArithmetic%3E::Scalar)%5D%3E-for-ProjectivePoint%3CC%3E

https://docs.rs/primeorder/0.14.0-pre.7/primeorder/struct.ProjectivePoint.html#impl-LinearCombination%3C%5B(ProjectivePoint%3CC%3E,+%3CC+as+CurveArithmetic%3E::Scalar)%5D%3E-for-ProjectivePoint%3CC%3E

The latter is notably:

impl<C, const N: usize> LinearCombination<[(ProjectivePoint<C>, <C as CurveArithmetic>::Scalar); N]> for ProjectivePoint<C>

[daxpedda]

The current trait looks like this:

pub trait MultiscalarMul {
    type Point;

    fn multiscalar_alloc_mul<I, J>(scalars: I, points: J) -> Self::Point
    where
        I: IntoIterator,
        I::Item: Borrow<Scalar>,
        J: IntoIterator,
        J::Item: Borrow<Self::Point>;
}

So if we change the trait to be like LinearCombination:

pub trait MultiscalarMul<Input> {
    type Point;

    fn multiscalar_alloc_mul(input: Input) -> Self::Point;
}

We can implement it with the current parameter types like this:

impl<I: IntoIterator<Item: Borrow<Scalar>>, J: IntoIterator<Item: Borrow<EdwardsPoint>>> MultiscalarMul<(I, J)> for EdwardsPoint {
    type Point = EdwardsPoint;

    ...
}

However adding an implementation for fixed arrays won't work:

impl<const N: usize> MultiscalarMul<(&[Scalar; N], &[EdwardsPoint; N])> for EdwardsPoint {
    type Point = EdwardsPoint;

    ...
}

Which yields the "conflicting implementations" error.

The reason it works in primeorder is because its two distinctive types that are used as the parameter, not generics.

We can do this here as well, but we will loose the functionality that IntoIterator provided until now, which personally I'm fine with.

Let me know if I missed something.

[tarcieri]

The trait in elliptic-curve has an AsRef bound on its input which can be used to produce an iterator if desired.

Yes, as I said earlier the trait in elliptic-curve has a generic parameter for the input, which is what enables the overlapping impls. If you wanted to do something similar in curve25519-dalek, the same thing would be required.

Notably the LinearCombination trait in elliptic-curve also has the advantage of ensuring that there are always an equal number of points and scalars, since it receives them as 2-tuples.

[daxpedda]

The trait in elliptic-curve has an AsRef bound on its input which can be used to produce an iterator if desired.

AsRef<Point> produce an Iterator? I'm not following, sorry.

I am definitely unable to use the current LinearCombination trait in combination with an Iterator.

Yes, as I said earlier the trait in elliptic-curve has a generic parameter for the input, which is what enables the overlapping impls. If you wanted to do something similar in curve25519-dalek, the same thing would be required.

Yes, I've outlined why this doesn't work while preserving the Iterator functionality. My example uses a generic for the input.

Notably the LinearCombination trait in elliptic-curve also has the advantage of ensuring that there are always an equal number of points and scalars, since it receives them as 2-tuples.

I'm happy to do the same here!

---

I don't seem to get what you are trying to tell me.

If you are telling me that we can preserve the Iterator functionality with the LinearCombination model, I would appreciate a couple lines of code showing how, because I can't seem to figure it out.

If you are telling me to use LinearCombination model and drop the Iterator functionality, I'm happy to do that as well!

[tarcieri]

AsRef produce an Iterator? I'm not following, sorry.

Again, I'm talking about the LinearCombination trait as it exists in elliptic-curve. It has this AsRef bound:

pub trait LinearCombination<PointsAndScalars>: CurveGroup
where
    PointsAndScalars: AsRef<[(Self, Self::Scalar)]> + ?Sized,

i.e.

AsRef<[(Self, Self::Scalar)]>

That allows you to get access to a slice of a 2-tuples of points and scalars, and the slice primitive type impls IntoIterator:

https://doc.rust-lang.org/std/primitive.slice.html#iteration

So anything relying on iterating generically can use AsRef and IntoIterator in combination as a replacement.

[daxpedda]

So anything relying on iterating generically can use AsRef and IntoIterator in combination as a replacement.

Ah, I see where the confusion is!

I'm talking about the API side, not the implementation. The current functionality in MultiscalarMul allows users to pass iterators, it isn't about the implementation requiring an iterator. In fact it doesn't seeing how I implemented the new method not taking iterators.

The advantage of being able to pass iterators is that users don't have to put all the input in a contiguous slice of memory, but can just produce them in-place in an Iterator from whatever data structure it sits in. LinearCombination doesn't support that. While slices are iterators, they still force the user to pass a slice directly or through AsRef<[...]>.

let values: HashMap<_, Foo> = ...;

let result = EdwardsPoint::multiscalar_mul(
    values.iter().map(|foo| &foo.scalar),
    values.iter().map(|foo| &foo.point),
);

let result = ProjectivePoint::lincomb(?);
// We can't produce a slice from a `HashMap` without allocating memory, e.g. collect them in a `Vec`.

[daxpedda]

Let me just say again that I personally have no stake in the whole Iterator feature and I'm happy to drop it and follow the elliptic-curve model in this PR if that's the decision here.

[tarcieri]

Instead of an AsRef bound it could be IntoIterator instead. We could potentially even make a corresponding change to elliptic-curve.

[tarcieri]

@daxpedda here's a elliptic-curve PR which uses IntoIterator instead of AsRef for the PointsAndScalars of LinearCombination: https://github.com/RustCrypto/traits/pull/1936/files

It's nearly API compatible with the old version except for the addition of a lifetime, which I might see if I can remove.

Edit: hmm, removing it seems hard while remaining compatible with slices, since the slice iterator needs to borrow its items

[tarcieri]

Alright, seems that PR isn't helpful after all since the existing trait is generic over iterators.

Looking at some real-world code examples it seems most people store the inputs in a Vec, but are also using the iterator functionality, e.g. combining points from multiple sources, prepending a point to the beginning of an iterator over the remaining points in a Vec, etc:

https://github.com/search?q=curve25519_dalek+multiscalar_mul+language%3ARust&type=code&l=Rust

So we shouldn't remove that functionality either.

[daxpedda]

I think at a minimum, we could change it from taking two iterators to one, while not taking away any functionality. Multiple sources can still be supported with zip() and such.
WDYT?

[tarcieri]

Yeah, I'm definitely a big fan of eliminating the error case of mismatched iterator lengths by using a 2-tuple.

I'm not sure how wild people will be about updating their code though. Perhaps there could be a deprecated legacy method that accepts the two iterators and builds an iterator over 2-tuples for you.

[tarcieri]

I should probably also repeat here what I noted in RustCrypto/traits#1936, that there seems to be a fundamental tradeoff between having a trait that can work in both alloc and no-alloc cases, and a trait which is generic over iterators, since supporting the latter requires alloc.

That said, we can conditionally impl LinearCombination when elliptic-curve support is eventually added, and that can provide a common trait-based API that covers both the alloc and no-alloc use cases.

[daxpedda]

So in the meantime is just adding a second method to MultiscalarMul (this PR) acceptable?
Or do you want to wait for elliptic-curve integration?

[tarcieri]

I think we should probably consider changes to MultiscalarMul and friends in a separate PR, and what's in this PR is fine for now as a start, though I need to do a more detailed review