keys package: fundraiser compatibility and HD keys (BIP 39 & BIP 32 / BIP 44) by liamsi · Pull Request #118 · tendermint/go-crypto

liamsi · 2018-06-07T14:25:05Z

resolves #90
resolves #89
resolves #57

partly resolves #124
closes cosmos/cosmos-sdk#872

…approach # Conflicts: # signature_test.go

xla

👍 🍡

Some minor nitpicks. Given my limited understanding from the design doc this looks good to me.

xla · 2018-06-12T05:38:51Z

armor.go

 	w, err := armor.Encode(buf, blockType, headers)
 	if err != nil {
-		PanicSanity("Error encoding ascii armor: " + err.Error())
+		cmn.PanicSanity("Error encoding ascii armor: " + err.Error())


AFAIK we want to transition away from panic methods from common and use naked panic statements.

xla · 2018-06-12T05:44:57Z

keys/hd/fundraiser_test.go

+		seed := bip39.NewSeed(d.Mnemonic, "")
+
+		//fmt.Println("================================")
+		//fmt.Println("ROUND:", i, "MNEMONIC:", d.Mnemonic)


Why not always print those and use t.Log to have it only presented on verbosre test runs.

Sure, why not 👍done

xla · 2018-06-12T05:48:30Z

keys/hd/hdpath.go

+	}
+	// m / purpose' / coin_type' / account' / change / address_index
+	return fmt.Sprintf("%d'/%d'/%d'/%s/%d",
+		p.purpose, p.coinType, p.account, changeStr, p.addressIdx)


If we break method calls into multiple lines, can we consistently have the following format:

namespace.Method( arg1, arg2, arg3, )

xla · 2018-06-12T05:50:11Z

keys/hd/hdpath.go

+// ComputeMastersFromSeed returns the master public key, master secret, and chain code in hex.
+func ComputeMastersFromSeed(seed []byte) (secret [32]byte, chainCode [32]byte) {
+	masterSecret := []byte("Bitcoin seed")
+	secret, chainCode = i64(masterSecret, seed)


Is there significance to the named returns, or can we just return the result from i64 directly?

xla · 2018-06-12T05:52:16Z

keys/hd/hdpath.go

+
+// DerivePrivateKeyForPath derives the private key by following the BIP 32/44 path from privKeyBytes,
+// using the given chainCode.
+func DerivePrivateKeyForPath(privKeyBytes [32]byte, chainCode [32]byte, path string) (derivedKey [32]byte, err error) {


Named returns for function bodies of this complexity seem hard to decypher what the state at what point is. It could be clearer with early and explicit returns.

xla · 2018-06-12T05:59:32Z

keys/hd/hdpath_test.go

+	fmt.Println()
+
+	seed = bip39.MnemonicToSeed(
+		"advice process birth april short trust crater change bacon monkey medal garment " +


Why not use an unescaped raw string?

xla · 2018-06-12T06:03:42Z

keys/keybase.go

+const (
+	// English is the default language to create a mnemonic.
+	// It is the only supported language by this package.
+	English Language = iota


Would be nice to have the first value at iota + 1 to prevent accidental zero value passing.

Should we consider prefixing with Language.

iota+1 it is 👍

Should we consider prefixing with Language.

You mean s/English/LanguageEnglish/ ? Why? Isn't that a bit redundant?

There is a case to make for either on the caller side it's a bit more clear when reading, yet the type system should inform that already. Could be either way, in general it's good to prefix if the namespace of the package gets a bit more crowded.

xla · 2018-06-12T06:04:58Z

keys/keybase.go

-	priv, err := generate(algo, secret)
+func (kb dbKeybase) CreateMnemonic(name string, language Language, passwd string, algo SigningAlgo) (info Info, mnemonic string, err error) {
+	if language != English {
+		return nil, "", fmt.Errorf("unsupported language: currently only english is supported")


Would be nice to have solid error handling, either with sentinal values or types, so that callers get control flow.

There is not much the callers can control here: In these cases (currently) only one possible language is supported (all other will err). This means, if the caller calls this with a different language then engl. he (currently) isn't using the lib correctly (and he can't handle this error case differently then by picking a supported language). Maybe, the sentinel types are still useful as they additionally document the behaviour (additionally to the documentation).

xla · 2018-06-12T06:06:15Z

keys/keybase.go

 	}
+	// TODO(ismail): we have to be careful with the separator in non-ltr languages. Ideally, our package should provide
+	// a helper function for that
+	mnemonic = strings.Join(mnemonicS, " ")


The least we can do with the separators is to have them in constants, and ideally later configurable. This is the second occurrence of the literal string and a TODO, looks like a good candidate to fix before merge.

xla · 2018-06-12T06:12:18Z

keys/keys.go

-	}
-}
+	// Secp256k1 uses the Bitcoin secp256k1 ECDSA parameters.
+	Secp256k1 = SigningAlgo("secp256k1")


Again would prefix.

Same as above.

… for word list and supported curve/params for signing

cwgoes

A few minor nits, needs a rebase on develop for the new PubKey() API

cwgoes · 2018-06-13T01:54:56Z

keys/bip39/wordcodec.go

+	return
+}
+
+// MnemonicToSeedWithErrChecking is completely equivalent to MnemonicToSeed.


This comment and the one two lines below seem to contradict each other - by "equivalent" do you mean that it returns the same seed?

cwgoes · 2018-06-13T01:58:25Z

keys/hd/hdpath.go

+// m / 44' / 118' / account' / 0 / address_index
+// The fixed parameters (purpose', coin_type', and change) are determined by what was used in the fundraiser.
+func NewFundraiserParams(account uint32, addressIdx uint32) *BIP44Params {
+	return &BIP44Params{


Might be a bit more idiomatic to call our own NewParams here

cwgoes · 2018-06-13T01:59:08Z

keys/hd/hdpath.go

+// using the given chainCode.
+func DerivePrivateKeyForPath(privKeyBytes [32]byte, chainCode [32]byte, path string) (derivedKey [32]byte, err error) {
+	data := privKeyBytes
+	// TODO(ismail): refactor this out and provide a constructor for BIP44


Can we turn this into an issue if it will not be included in this PR?

cwgoes · 2018-06-13T02:00:21Z

keys/hd/hdpath.go

+		data = append([]byte{byte(0)}, privKeyBytes[:]...)
+	} else {
+		// this can't return an error:
+		pubkey, err := crypto.PrivKeySecp256k1(privKeyBytes).PubKey()


Can be rebased on develop, which no longer errors on .PubKey()

cwgoes · 2018-06-13T02:01:26Z

keys/hd/hdpath.go

+}
+
+// derivePrivateKey derives the private key with index and chainCode.
+// If harden is true, the derivation is 'hardened'.


What is the purposing of "hardening" - can we link to the spec?

Added link to the spec which contains further info.

cwgoes · 2018-06-13T02:03:01Z

keys/hd/hdpath_test.go

+
+func ExampleStringifyPathParams() {
+	path := NewParams(44, 0, 0, false, 0)
+	fmt.Println(path.String())


Why does this print instead of checking the result?

https://blog.golang.org/examples

cwgoes · 2018-06-13T02:03:26Z

keys/hd/hdpath_test.go

+	// Output: 44'/0'/0'/0/0
+}
+
+func ExampleSomeBIP32TestVecs() {


Why do these print instead of checking whether or not the outputs are correct in the testcase?

This prints do exactly that: https://blog.golang.org/examples

cwgoes · 2018-06-13T02:19:26Z

keys/keybase.go

+	if passwd != "" {
+		info = kb.writeLocalKey(crypto.PrivKeySecp256k1(derivedPriv), name, passwd)
+	} else {
+		pubk, err := crypto.PrivKeySecp256k1(derivedPriv).PubKey()


Error check can be removed with a rebase on develop

cwgoes · 2018-06-13T02:20:38Z

random.go

 	copy(hashBytes32[:], hashBytes)
 	ri.seedBytes = xorBytes32(ri.seedBytes, hashBytes32)
-	// Create new cipher.Block
+	// CreateMnemonic new cipher.Block


Accidental find replace?

Good catch!

…approach

- fix comments - call NewParams - remove accidental change

- remove accidental changes

…ip39-2nd-approach

liamsi added 4 commits June 5, 2018 23:30

WIP: bip39/32/44 things

16678ac

WIP: bip39/32/44 things, delete code

b584bdb

WIP: bip39/32/44 things: delete more code!

134795e

some test vectors, delete obsolete tests, params moved to hd package

310c22e

liamsi requested review from ebuchman and jaekwon as code owners June 7, 2018 14:25

liamsi added 2 commits June 7, 2018 11:20

minor cleanups

fc16d36

unexport methods for a cleaner interface, some code cleanup

bdff560

liamsi mentioned this pull request Jun 8, 2018

Unify local and external keys in keybase interface #117

Merged

cleanup, renaming according to latest discussions

ca85172

liamsi force-pushed the bip39-2nd-approach branch from 0802437 to ca85172 Compare June 8, 2018 04:22

liamsi added 5 commits June 7, 2018 22:25

reactivate and adapt some tests

9f5eabf

reactivate and adapt more tests

b702429

all tests working again

55f3a86

some quick refactoring: use enum instead of strings for language

e625d6f

WIP: properly document everything ...

58a13c9

liamsi mentioned this pull request Jun 11, 2018

Improve error message for the word codec? #124

Closed

liamsi added 3 commits June 11, 2018 15:57

merging changes from development

8e46bed

Merge remote-tracking branch 'remotes/origin/develop' into bip39-2nd-…

e5dff33

…approach # Conflicts: # signature_test.go

Merge remote-tracking branch 'remotes/origin/develop' into bip39-2nd-…

af28f3a

…approach # Conflicts: # signature_test.go

liamsi force-pushed the bip39-2nd-approach branch from adbcf91 to 8ef5d95 Compare June 12, 2018 05:14

document things

54b9bd5

liamsi force-pushed the bip39-2nd-approach branch from 8ef5d95 to 54b9bd5 Compare June 12, 2018 05:19

liamsi changed the title ~~WIP: BIP 39 & BIP 32 / BIP 44 things~~ BIP 39 & BIP 32 / BIP 44 things Jun 12, 2018

liamsi requested review from cwgoes and xla June 12, 2018 05:35

document more things

e9506fc

liamsi force-pushed the bip39-2nd-approach branch from 0e710fb to e9506fc Compare June 12, 2018 05:59

remove TODOs (we currently only support english)

9d30e65

xla approved these changes Jun 12, 2018

View reviewed changes

liamsi changed the title ~~BIP 39 & BIP 32 / BIP 44 things~~ keys package: fundraiser compatibility and HD keys (BIP 39 & BIP 32 / BIP 44) Jun 12, 2018

liamsi added 5 commits June 12, 2018 00:38

replace common.Panic* with vanilla panic

2251ce0

print using t.Log(f)

64ea127

consistent line breaks for method params

1b6c7b8

sentinel errors / 'constant' errors to further expose suppported lang…

1b92b7f

… for word list and supported curve/params for signing

review comment: iota+1

301a30e

liamsi mentioned this pull request Jun 12, 2018

keys: Recover from seed without persisting to disk #57

Closed

cwgoes suggested changes Jun 13, 2018

View reviewed changes

liamsi added 2 commits June 19, 2018 16:28

Merge remote-tracking branch 'remotes/origin/develop' into bip39-2nd-…

723ac19

…approach

catch up with development

5c539d8

liamsi mentioned this pull request Jun 20, 2018

Move keybase to cosmos #141

Closed

2 tasks

liamsi added 2 commits June 20, 2018 13:24

review comments:

7e69a9a

- fix comments - call NewParams - remove accidental change

review comments:

86405ce

- remove accidental changes

cwgoes approved these changes Jun 20, 2018

View reviewed changes

Merge remote-tracking branch 'remotes/fork/bip39-2nd-approach' into b…

6a675f5

…ip39-2nd-approach

liamsi merged commit 4634063 into tendermint:develop Jun 20, 2018

This was referenced Jul 3, 2018

Word codec should implement BIP39 #89

Closed

keys: Expose HD tooling #90

Closed

Conversation

liamsi commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwgoes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

liamsi commented Jun 7, 2018 •

edited

Loading