Skip to content
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.

Sign up for GitHub

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

Method SetIntString in the Scalar interface to create arbitrary large scalars from strings #534

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Method SetIntString in the Scalar interface to create arbitrary large scalars from strings #534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ type Scalar interface {
// SetInt64 sets the receiver to a small integer value.
SetInt64(v int64) Scalar

// SetIntString sets the receiver to a string encoded integer value.
SetIntString(v string) (Scalar, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my main worry with making the Scalar interface any bigger is that it'll break all implementations using custom scalars.

That being said we're en route to V4 so we could do it... Opinions @pierluca ?

Copy link
Contributor

@pierluca pierluca Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share your concern, @AnomalRoil . Could you elaborate a little bit more about the use case, @pmcampones ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It says "string encoded integer value", which definitely looks like it should be done using SetInt64. Other possibilities I see:

  • SetBigInt which could use more than 64 bits
  • SetBytes which already exists :)

So I suppose this should be something like SetInt64(strconv.Atoi()) or use the SetBytes for bigger integers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe SetBigInt would be a better name for the method.
In the comment thread bellow we discussed using only SetBytes instead of the SetIntString.
I find using SetIntString a bit more convenient and less error prone than using SetBytes.


// Set to the additive identity (0).
Zero() Scalar

Expand Down
8 changes: 8 additions & 0 deletions group/edwards25519/scalar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ func (s *scalar) SetInt64(v int64) kyber.Scalar {
return s.setInt(mod.NewInt64(v, primeOrder))
}

func (s *scalar) SetIntString(v string) (kyber.Scalar, error) {
i, ok := new(big.Int).SetString(v, 0)
if !ok {
return nil, errors.New("invalid scalar string")
}
return s.setInt(mod.NewInt(i, primeOrder)), nil
}

func (s *scalar) toInt() *mod.Int {
return mod.NewIntBytes(s.v[:], primeOrder, defaultEndianess)
}
Expand Down
10 changes: 10 additions & 0 deletions group/mod/int.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"crypto/cipher"
"encoding/hex"
"errors"
"fmt"
"io"
"math/big"

Expand Down Expand Up @@ -173,6 +174,15 @@ func (i *Int) SetInt64(v int64) kyber.Scalar {
return i
}

func (i *Int) SetIntString(v string) (kyber.Scalar, error) {
bigV := new(big.Int)
bigV, ok := bigV.SetString(v, 0)
if !ok {
return nil, fmt.Errorf("unable to set string number: %v", v)
}
return i.Init(bigV, i.M), nil
}

Comment on lines +177 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this implementation I don't see why the code couldn't live in your own codebase as a helper function: you don't seem to need any internal or non-exported function to achieve this, no?

Like you could do something like:

func NewIntString(v string) (kyber.Scalar, error) {
	bigV := new(big.Int)
	bigV, ok := bigV.SetString(v, 0)
	if !ok {
		return nil, fmt.Errorf("unable to set string number: %v", v)
	}
	i := new(mod.Int)
	return i.Init(bigV, yourModulus), nil
}

assuming you work with a fixed modulus, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply!

I agree that touching the interface is contentious. And I didn't even consider the fact that other people using Kyber could have their own Scalar implementations, which now would have to be changed.

The mod.Int struct is public and has the methods to create a scalar from any pair of big ints, as seen in Init.
However this is not possible for the remaining implementations of Scalar.
For instance, I needed to use the scalar struct in the edwards25519.scalar file. The functionalities of the edwards curve require that specific implementation of the scalar.
The methods associated with this curve only require the Scalar interface in their signature. However, in the implementation of the methods a downcast is made to force a specific Scalar implementation.

I suppose the bigger problem lies in having methods requiring specific Scalar implementations, contrary to what the signature indicates. But just adding the SetIntString method was my quick and easy solution for the problem.

We could have the SetIntString method just in the edwards25519.scalar, instead of all scalars. It wouldn't force a change in the interface on one hand, on the other it would reveal more of the internals of edwards25519, because now the user that wants to create a scalar must know that specific implementation is used.
I'm not sure which solution is better. Placing the method just in edwards25519 could be less of an hassle for people that have their own Scalar implementations, but changing the interface would be cleaner for people (like me) who are just using the Scalar types provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your argument and I'm open to discussing it, albeit I would need a very convincing use case. At the moment, I don't see why the SetBytes function wouldn't be sufficient to develop the kind of auxiliary functions that you're referring to.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that there is nothing you can't do with SetBytes. I just find the SetIntString more user friendly.
Someone that doesn't know that big.Int has a method called Bytes would be stumped trying to compute the byte array representation of a large number.

On that note, the Bytes array on big.Int is big endian, while the default byte order of the kyber scalars are little endian.
I think this is bound to cause some bugs.
This is compounded by the fact that different Scalar implementations handle endianess differently. For instance, the mod.Int SetBytes implementation inverts the byte array if the default endian order is Little Endian; while the edwards25519 SetBytes implementation just assumes the byte array received comes in the correct order.
A user that makes auxiliary functions using SetBytes must read the different implementations and decide whether or not to invert the array depending on the specific Scalar implementation to be used.

Finally, while its not a problem in my use case, I think that using the Bytes method in big.Int and SetIntBytes of Scalar implementations might cause some performance hiccups.
I did no benchmark so this is just speculation.
With SetIntBytes we have a slowish operation for converting a string into a big.Int, and fast operations to use the contents of the big.Int in the Scalar.
With an auxiliary function using SetBytes, we would first have to make a big.Int from a string, then convert it to a byte array representation, only to then call SetBytes which would do the opposite, converting the byte array representation into a large number.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As others have said, I also think this is a very specific use-case and should be handled locally in your code. I think a SetBigInt could be useful, if others would agree. But doing the decimal-to-BigInt conversion in all curves seems wrong: why not hexadecimal-to-BigInt? Or octal-to-BigInt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the commit stands currently, the SetIntString method does not need the argument to be decimal. It can be decimal, hexadecimal, octal or even binary, as long as it has the appropriate prefix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I didn't see it uses big.Int.SetString. There's still two things that bother me:

  • Scalar should be more low level than handling strings
  • if we add a SetBigInt, how should a Scalar which is not built on big.Int handle this? I think currently all scalars are based on big.Int, but I'm not sure.

// Int64 returns the int64 representation of the value.
// If the value is not representable in an int64 the result is undefined.
func (i *Int) Int64() int64 {
Expand Down
9 changes: 9 additions & 0 deletions pairing/bls12381/circl/scalar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import (
"crypto/cipher"
"fmt"
"io"
"math/big"

Expand Down Expand Up @@ -62,6 +63,14 @@
return s
}

func (s *Scalar) SetIntString(v string) (kyber.Scalar, error) {
err := s.inner.SetString(v)
if err != nil {
return nil, fmt.Errorf("unable to set string number: %v", err)

Check failure on line 69 in pairing/bls12381/circl/scalar.go

View workflow job for this annotation

GitHub Actions / lint 

non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
}
return s, nil
}

func (s *Scalar) Zero() kyber.Scalar { s.inner.SetUint64(0); return s }

func (s *Scalar) Add(a, b kyber.Scalar) kyber.Scalar {
Expand Down
19 changes: 17 additions & 2 deletions share/vss/pedersen/vss_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ func TestMinimumT(t *testing.T) {

func TestVSSWhole(t *testing.T) {
dealer, verifiers := genAll()
vssWhole(t, dealer, verifiers, secret)
}

func vssWhole(t *testing.T, dealer *Dealer, verifiers []*Verifier, secret kyber.Scalar) {
// 1. dispatch deal
resps := make([]*Response, nbVerifiers)
encDeals, err := dealer.EncryptedDeals()
Expand Down Expand Up @@ -591,6 +594,15 @@ func TestVSSContext(t *testing.T) {
assert.Len(t, c, suite.Hash().Size())
}

func TestDeterministicStringSecret(t *testing.T) {
deterministicSec, err := suite.Scalar().SetIntString("0x123456789abcdef")
require.NoError(t, err)
dealer, err := NewDealer(suite, dealerSec, deterministicSec, verifiersPub, vssThreshold)
require.NoError(t, err)
verifiers := genVerifiers()
vssWhole(t, dealer, verifiers, deterministicSec)
}

func genPair() (kyber.Scalar, kyber.Point) {
secret := suite.Scalar().Pick(suite.RandomStream())
public := suite.Point().Mul(secret, nil)
Expand All @@ -612,13 +624,16 @@ func genDealer() *Dealer {
}

func genAll() (*Dealer, []*Verifier) {
dealer := genDealer()
return genDealer(), genVerifiers()
}

func genVerifiers() []*Verifier {
var verifiers = make([]*Verifier, nbVerifiers)
for i := 0; i < nbVerifiers; i++ {
v, _ := NewVerifier(suite, verifiersSec[i], dealerPub, verifiersPub)
verifiers[i] = v
}
return dealer, verifiers
return verifiers
}

func randomBytes(n int) []byte {
Expand Down
Loading