crypto/secp256k1: Add godocs, remove indirection in privkeys (#2017)

* crypto/secp256k1: Add godocs, remove indirection in privkeys

The following was previously done for creating secp256k1 private keys:

First obtain privkey bytes. Then create a private key in the
underlying library, with scalar exponent equal to privKeyBytes.
(The method called was secp256k1.PrivKeyFromBytes,
fb90c334df/btcec/privkey.go (L21))

Then the private key was serialized using the underlying library, which just
returns back the bytes that comprised the scalar exponent, but padded to be
exactly 32 bytes.
fb90c334df/btcec/privkey.go (L70)

Thus the entire indirection of calling the underlying library can be avoided
by just ensuring that we pass in a 32 byte value. A test case has even be written
to show this more clearly in review.

* crypto/secp256k1: Address PR comments

Squash this commit

* crypto: Remove note about re-registering amino paths when unnecessary.

This commit should be squashed.

crypto/ed25519/ed25519.go

@@ -26,11 +26,6 @@ const (
 var cdc = amino.NewCodec()
 
 func init() {
-	// NOTE: It's important that there be no conflicts here,
-	// as that would change the canonical representations,
-	// and therefore change the address.
-	// TODO: Add feature to go-amino to ensure that there
-	// are no conflicts.
 	cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
 	cdc.RegisterConcrete(PubKeyEd25519{},
 		Ed25519PubKeyAminoRoute, nil)

crypto/encoding/amino/amino.go

@@ -13,8 +13,9 @@ func init() {
 	// NOTE: It's important that there be no conflicts here,
 	// as that would change the canonical representations,
 	// and therefore change the address.
-	// TODO: Add feature to go-amino to ensure that there
+	// TODO: Remove above note when
-	// are no conflicts.
+	// https://github.com/tendermint/go-amino/issues/9
+	// is resolved
 	RegisterAmino(cdc)
 }
 

crypto/secp256k1/secp256k1.go

@@ -23,11 +23,6 @@ const (
 var cdc = amino.NewCodec()
 
 func init() {
-	// NOTE: It's important that there be no conflicts here,
-	// as that would change the canonical representations,
-	// and therefore change the address.
-	// TODO: Add feature to go-amino to ensure that there
-	// are no conflicts.
 	cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
 	cdc.RegisterConcrete(PubKeySecp256k1{},
 		Secp256k1PubKeyAminoRoute, nil)
@@ -45,27 +40,31 @@ func init() {
 
 var _ crypto.PrivKey = PrivKeySecp256k1{}
 
-// Implements PrivKey
+// PrivKeySecp256k1 implements PrivKey.
 type PrivKeySecp256k1 [32]byte
 
+// Bytes marshalls the private key using amino encoding.
 func (privKey PrivKeySecp256k1) Bytes() []byte {
 	return cdc.MustMarshalBinaryBare(privKey)
 }
 
+// Sign creates an ECDSA signature on curve Secp256k1, using SHA256 on the msg.
 func (privKey PrivKeySecp256k1) Sign(msg []byte) (crypto.Signature, error) {
-	priv__, _ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey[:])
+	priv, _ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey[:])
-	sig__, err := priv__.Sign(crypto.Sha256(msg))
+	sig, err := priv.Sign(crypto.Sha256(msg))
 	if err != nil {
 		return nil, err
 	}
-	return SignatureSecp256k1(sig__.Serialize()), nil
+	return SignatureSecp256k1(sig.Serialize()), nil
 }
 
+// PubKey performs the point-scalar multiplication from the privKey on the
+// generator point to get the pubkey.
 func (privKey PrivKeySecp256k1) PubKey() crypto.PubKey {
-	_, pub__ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey[:])
+	_, pubkeyObject := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey[:])
-	var pub PubKeySecp256k1
+	var pubkeyBytes PubKeySecp256k1
-	copy(pub[:], pub__.SerializeCompressed())
+	copy(pubkeyBytes[:], pubkeyObject.SerializeCompressed())
-	return pub
+	return pubkeyBytes
 }
 
 // Equals - you probably don't need to use this.
@@ -73,54 +72,48 @@ func (privKey PrivKeySecp256k1) PubKey() crypto.PubKey {
 func (privKey PrivKeySecp256k1) Equals(other crypto.PrivKey) bool {
 	if otherSecp, ok := other.(PrivKeySecp256k1); ok {
 		return subtle.ConstantTimeCompare(privKey[:], otherSecp[:]) == 1
-	} else {
-		return false
 	}
+	return false
 }
 
-/*
+// GenPrivKey generates a new ECDSA private key on curve secp256k1 private key.
-// Deterministically generates new priv-key bytes from key.
+// It uses OS randomness in conjunction with the current global random seed
-func (key PrivKeySecp256k1) Generate(index int) PrivKeySecp256k1 {
+// in tendermint/libs/common to generate the private key.
-	newBytes := cdc.BinarySha256(struct {
-		PrivKey [64]byte
-		Index   int
-	}{key, index})
-	var newKey [64]byte
-	copy(newKey[:], newBytes)
-	return PrivKeySecp256k1(newKey)
-}
-*/
-
 func GenPrivKey() PrivKeySecp256k1 {
 	privKeyBytes := [32]byte{}
 	copy(privKeyBytes[:], crypto.CRandBytes(32))
-	priv, _ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKeyBytes[:])
+	// crypto.CRandBytes is guaranteed to be 32 bytes long, so it can be
-	copy(privKeyBytes[:], priv.Serialize())
+	// casted to PrivKeySecp256k1.
 	return PrivKeySecp256k1(privKeyBytes)
 }
 
+// GenPrivKeySecp256k1 hashes the secret with SHA2, and uses
+// that 32 byte output to create the private key.
 // NOTE: secret should be the output of a KDF like bcrypt,
 // if it's derived from user input.
-func GenPrivKeyFromSecret(secret []byte) PrivKeySecp256k1 {
+func GenPrivKeySecp256k1(secret []byte) PrivKeySecp256k1 {
-	privKey32 := crypto.Sha256(secret) // Not Ripemd160 because we want 32 bytes.
+	privKey32 := sha256.Sum256(secret)
-	priv, _ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey32)
+	// sha256.Sum256() is guaranteed to be 32 bytes long, so it can be
-	privKeyBytes := [32]byte{}
+	// casted to PrivKeySecp256k1.
-	copy(privKeyBytes[:], priv.Serialize())
+	return PrivKeySecp256k1(privKey32)
-	return PrivKeySecp256k1(privKeyBytes)
 }
 
 //-------------------------------------
 
 var _ crypto.PubKey = PubKeySecp256k1{}
 
+// PubKeySecp256k1Size is comprised of 32 bytes for one field element
+// (the x-coordinate), plus one byte for the parity of the y-coordinate.
 const PubKeySecp256k1Size = 33
 
-// Implements crypto.PubKey.
+// PubKeySecp256k1 implements crypto.PubKey.
-// Compressed pubkey (just the x-cord),
+// It is the compressed form of the pubkey. The first byte depends is a 0x02 byte
-// prefixed with 0x02 or 0x03, depending on the y-cord.
+// if the y-coordinate is the lexicographically largest of the two associated with
+// the x-coordinate. Otherwise the first byte is a 0x03.
+// This prefix is followed with the x-coordinate.
 type PubKeySecp256k1 [PubKeySecp256k1Size]byte
 
-// Implements Bitcoin style addresses: RIPEMD160(SHA256(pubkey))
+// Address returns a Bitcoin style addresses: RIPEMD160(SHA256(pubkey))
 func (pubKey PubKeySecp256k1) Address() crypto.Address {
 	hasherSHA256 := sha256.New()
 	hasherSHA256.Write(pubKey[:]) // does not error
@@ -131,6 +124,7 @@ func (pubKey PubKeySecp256k1) Address() crypto.Address {
 	return crypto.Address(hasherRIPEMD160.Sum(nil))
 }
 
+// Bytes returns the pubkey marshalled with amino encoding.
 func (pubKey PubKeySecp256k1) Bytes() []byte {
 	bz, err := cdc.MarshalBinaryBare(pubKey)
 	if err != nil {
@@ -139,22 +133,22 @@ func (pubKey PubKeySecp256k1) Bytes() []byte {
 	return bz
 }
 
-func (pubKey PubKeySecp256k1) VerifyBytes(msg []byte, sig_ crypto.Signature) bool {
+func (pubKey PubKeySecp256k1) VerifyBytes(msg []byte, interfaceSig crypto.Signature) bool {
 	// and assert same algorithm to sign and verify
-	sig, ok := sig_.(SignatureSecp256k1)
+	sig, ok := interfaceSig.(SignatureSecp256k1)
 	if !ok {
 		return false
 	}
 
-	pub__, err := secp256k1.ParsePubKey(pubKey[:], secp256k1.S256())
+	pub, err := secp256k1.ParsePubKey(pubKey[:], secp256k1.S256())
 	if err != nil {
 		return false
 	}
-	sig__, err := secp256k1.ParseDERSignature(sig[:], secp256k1.S256())
+	parsedSig, err := secp256k1.ParseDERSignature(sig[:], secp256k1.S256())
 	if err != nil {
 		return false
 	}
-	return sig__.Verify(crypto.Sha256(msg), pub__)
+	return parsedSig.Verify(crypto.Sha256(msg), pub)
 }
 
 func (pubKey PubKeySecp256k1) String() string {
@@ -164,16 +158,15 @@ func (pubKey PubKeySecp256k1) String() string {
 func (pubKey PubKeySecp256k1) Equals(other crypto.PubKey) bool {
 	if otherSecp, ok := other.(PubKeySecp256k1); ok {
 		return bytes.Equal(pubKey[:], otherSecp[:])
-	} else {
-		return false
 	}
+	return false
 }
 
 //-------------------------------------
 
 var _ crypto.Signature = SignatureSecp256k1{}
 
-// Implements crypto.Signature
+// SignatureSecp256k1 implements crypto.Signature
 type SignatureSecp256k1 []byte
 
 func (sig SignatureSecp256k1) Bytes() []byte {

crypto/secp256k1/secpk256k1_test.go

@@ -10,6 +10,8 @@ import (
 
 	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/secp256k1"
+
+	underlyingSecp256k1 "github.com/btcsuite/btcd/btcec"
 )
 
 type keyData struct {
@@ -63,3 +65,24 @@ func TestSignAndValidateSecp256k1(t *testing.T) {
 
 	assert.False(t, pubKey.VerifyBytes(msg, sig))
 }
+
+// This test is intended to justify the removal of calls to the underlying library
+// in creating the privkey.
+func TestSecp256k1LoadPrivkeyAndSerializeIsIdentity(t *testing.T) {
+	numberOfTests := 256
+	for i := 0; i < numberOfTests; i++ {
+		// Seed the test case with some random bytes
+		privKeyBytes := [32]byte{}
+		copy(privKeyBytes[:], crypto.CRandBytes(32))
+
+		// This function creates a private and public key in the underlying libraries format.
+		// The private key is basically calling new(big.Int).SetBytes(pk), which removes leading zero bytes
+		priv, _ := underlyingSecp256k1.PrivKeyFromBytes(underlyingSecp256k1.S256(), privKeyBytes[:])
+		// this takes the bytes returned by `(big int).Bytes()`, and if the length is less than 32 bytes,
+		// pads the bytes from the left with zero bytes. Therefore these two functions composed
+		// result in the identity function on privKeyBytes, hence the following equality check
+		// always returning true.
+		serializedBytes := priv.Serialize()
+		require.Equal(t, privKeyBytes[:], serializedBytes)
+	}
+}