Implement a Clone method for abstract.Point about kyber HOT 11 CLOSED

Wouldn't it be better to have Clone() abstract.Point and Clone() abstract.Scalar?

from kyber.

Right, I didn't even mention which interface (I meant abstract.Point; updated above).
Scalar has a Set method. Not sure if it's good to add another method with (almost) the same functionality.

from kyber.

The only difference between Clone() abstract.Point and Clone(abstract.Point) would be that the first would always create a new point, while the latter would allow you to reuse an existing instance (but then Set would be a better name, right?). Should we use the Set approach for the sake of consistency? Or just add a Clone method to either both or only to Point?

from kyber.

So you would propose to have

type Scalar interface {
    Set(s Scalar) Scalar
    Clone() Scalar
}

type Point interface{
    Set(p Point) Point
    Clone() Point
}

?

from kyber.

Yes, or only Set (as it already exists for Scalar). But the above is also OK, I guess. Let's (briefly) discuss with @nikkolasg and @Daeinar about it (next week), OK?

from kyber.

I have considered at various points adding a Clone() method to these interfaces, but never quite got around to deciding whether or not to do it.

You can of course get the same thing from group.Point() or group.Scalar() followed by Set(), but Clone() obviously has two potential advantages: (1) you get a clone of an existing Point or Scalar with the same value in one operation rather than two (might be slightly more efficient), and (2) you can create Points or Scalars from some group without actually having a reference to the relevant Group object they came from, only references to one or more existing Points or Scalars. I'm not sure how much we would want to take advantage of property #2, but if we did, we might in principle be able to dispense with the passing around of some Suite references. In language terms, this would amount to moving from a "class-based object-oriented approach" (where a Group object represents a dynamically-bound class) to a more "prototype-based object-oriented approach" (where an existing Point/Scalar object acts as a prototype for creating more of the same).

Regardless, whatever we decide is best, Point and Scalar should be consistent with each other: i.e., both providing Clone() or neither, and both providing Set() or neither.

from kyber.

For the couple of times I did some crypto-math in the code, I did feel the need to have a Clone - less for Set. But in the Ed25519 it was nice, IIRC. So I propose to have both Set and Clone for Scalar and Point.

from kyber.

So I propose to have both Set and Clone for Scalar and Point

Let's see what @nikkolasg thinks. But I agree it is a good idea (although I don't like having two methods doing very similar things). I updated the todo list above accordingly.

from kyber.

The above mentioned 3 ways of implementing Clone aren't necessarily the most efficient ones: I didn't think about that one can also use the internal representation of the underlying implementation of the abstract.Point interface (which could be faster than using any of the mentioned operations).

For instance in ed25519 one could simply do sth along the lines of:

func (P *point) Clone() abstract.Point {
    p2 := &point{}
    p2.ge = P.ge
    return p2
}

Which would be fast. And Set would almost look the same.

So "benchmarking" doesn't really make sense here as the implementation of Clone etc depends on the underlying implementation of abstract.Point.

from kyber.

If someone is curious, here are the different results:

go test -bench=BenchmarkClone -benchmem
PASS
BenchmarkCloneInternal-4           50000             27887 ns/op             160 B/op          1 allocs/op
BenchmarkCloneMarshal-4            30000             55301 ns/op             192 B/op          2 allocs/op
BenchmarkCloneAdd-4                50000             28594 ns/op             160 B/op          1 allocs/op
BenchmarkCloneMul-4                 5000            312700 ns/op             272 B/op          4 allocs/op
ok      github.com/dedis/crypto/ed25519 64.262s

from kyber.

Done in #86

from kyber.