multi: use secp256k1 types and fields directly. #1211

dnldd · 2018-05-16T21:30:21Z

The chainec package is scheduled to be removed. This PR replaces chainec function calls within the hdkeychain package with the underlining secp256k1 types and fields which breaks its dependence on a package soon to be removed. memwallet, a member of rpctest which calls hdkeychain functions is also updated.

This is work towards #1191

davecgh · 2018-05-16T21:34:40Z

hdkeychain/extendedkey.go

-		pkx, pky := chainec.Secp256k1.ScalarBaseMult(k.key)
-		pubKey := chainec.Secp256k1.NewPublicKey(pkx, pky)
+		pkx, pky := secp256k1.S256().ScalarBaseMult(k.key)
+		pubKey := secp256k1.NewPublicKey(pkx, pky)


Would prefer the following to match upstream and avoid an unnecessary GC:

pubKey := secp256k1.PublicKey{Curve: secp256k1.S256(), X: pkx, Y: pky}

davecgh · 2018-05-16T21:37:26Z

hdkeychain/extendedkey.go

 	ilNum := new(big.Int).SetBytes(il)
-	if ilNum.Cmp(chainec.Secp256k1.GetN()) >= 0 || ilNum.Sign() == 0 {
+	if ilNum.Cmp(curve.Params().N) >= 0 || ilNum.Sign() == 0 {


Why call .Params()? N is already available from the curve struct.

davecgh · 2018-05-16T21:37:37Z

hdkeychain/extendedkey.go

@@ -271,22 +273,22 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) {
 		// childKey = parse256(Il) + parenKey
 		keyNum := new(big.Int).SetBytes(k.key)
 		ilNum.Add(ilNum, keyNum)
-		ilNum.Mod(ilNum, chainec.Secp256k1.GetN())
+		ilNum.Mod(ilNum, curve.Params().N)


davecgh · 2018-05-16T21:40:46Z

hdkeychain/extendedkey.go

@@ -295,9 +297,9 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) {
 		// derive the final child key.
 		//
 		// childKey = serP(point(parse256(Il)) + parentKey)
-		childX, childY := chainec.Secp256k1.Add(ilx, ily, pubKey.GetX(),
+		childX, childY := curve.Add(ilx, ily, pubKey.GetX(),


Why call .GetX? X is available on the struct directly. In fact, GetX should ultimately be removed entirely from secp256k1.PublicKey (it can't yet since it's still required for the chainec interface) since it doesn't even follow effective Go guidelines about getters.

davecgh · 2018-05-16T21:40:58Z

hdkeychain/extendedkey.go

@@ -295,9 +297,9 @@ func (k *ExtendedKey) Child(i uint32) (*ExtendedKey, error) {
 		// derive the final child key.
 		//
 		// childKey = serP(point(parse256(Il)) + parentKey)
-		childX, childY := chainec.Secp256k1.Add(ilx, ily, pubKey.GetX(),
+		childX, childY := curve.Add(ilx, ily, pubKey.GetX(),
 			pubKey.GetY())


Same deal with .Y.

davecgh · 2018-05-16T21:42:50Z

hdkeychain/extendedkey.go

 			pubKey.GetY())
-		pk := chainec.Secp256k1.NewPublicKey(childX, childY)
+		pk := secp256k1.NewPublicKey(childX, childY)


Once again, please use the struct directly to avoid the GC overhead in crypto and to more closely match upstream.

pk := secp256k1.PublicKey{Curve: secp256k1.S256(), X: childX, Y: childY}

davecgh · 2018-05-16T21:46:43Z

hdkeychain/extendedkey.go

@@ -472,7 +474,7 @@ func NewMaster(seed []byte, net *chaincfg.Params) (*ExtendedKey, error) {

 	// Ensure the key in usable.
 	secretKeyNum := new(big.Int).SetBytes(secretKey)
-	if secretKeyNum.Cmp(chainec.Secp256k1.GetN()) >= 0 ||
+	if secretKeyNum.Cmp(secp256k1.S256().Params().N) >= 0 ||


Same deal here regarding .N.

davecgh · 2018-05-16T21:46:49Z

hdkeychain/extendedkey.go

@@ -520,13 +522,13 @@ func NewKeyFromString(key string) (*ExtendedKey, error) {
 		// of the order of the secp256k1 curve and not be 0.
 		keyData = keyData[1:]
 		keyNum := new(big.Int).SetBytes(keyData)
-		if keyNum.Cmp(chainec.Secp256k1.GetN()) >= 0 || keyNum.Sign() == 0 {
+		if keyNum.Cmp(secp256k1.S256().Params().N) >= 0 || keyNum.Sign() == 0 {


davecgh · 2018-05-16T21:48:56Z

The current description would cause the issue to be closed. Please reword it so it won't. Also, please add more description to the commit message which describes why it's being changed. Github might go away one day, so the commit messages need to make it clear what is happening without relying on pointing to external issue numbers. Not to mention it frustrates forks with wrong numbers if you put them in commits.

davecgh · 2018-05-16T22:35:35Z

hdkeychain/extendedkey.go

-		childX, childY := chainec.Secp256k1.Add(ilx, ily, pubKey.GetX(),
-			pubKey.GetY())
-		pk := chainec.Secp256k1.NewPublicKey(childX, childY)
+		childX, childY := curve.Add(ilx, ily, pubKey.X,


Should be able to fit on the same line without issue.

davecgh · 2018-05-17T00:09:25Z

rpctest/memwallet.go

-func keyToAddr(key chainec.PrivateKey, net *chaincfg.Params) (dcrutil.Address, error) {
-	pubKey := chainec.Secp256k1.NewPublicKey(key.Public())
+func keyToAddr(key *secp256k1.PrivateKey, net *chaincfg.Params) (dcrutil.Address, error) {
+	pubKey := secp256k1.NewPublicKey(key.Public())


No need to pull out the X and Y of the public key just to recreate it and cause more allocations that have to be GC'd. Just cast it.

pubKey := (*secp256k1.PublicKey)(&key.PublicKey)

The chainec package is scheduled to be removed. This PR replaces chainec function calls within the hdkeychain package with the underlining secp256k1 types and fields which breaks its dependence on a package soon to be removed. memwallet, a member of rpctest which calls hdkeychain functions is also updated.

multi: update gcs indexing and serving code to match latest BIP 158 instance

davecgh requested changes May 16, 2018

View reviewed changes

dnldd force-pushed the remove_hdkeychain_chainecrefs branch 3 times, most recently from 70521f3 to adbd701 Compare May 16, 2018 22:24

davecgh requested changes May 16, 2018

View reviewed changes

dnldd force-pushed the remove_hdkeychain_chainecrefs branch 4 times, most recently from 387fd0e to f48e68a Compare May 16, 2018 23:44

davecgh requested changes May 17, 2018

View reviewed changes

dnldd force-pushed the remove_hdkeychain_chainecrefs branch 2 times, most recently from c304d5b to 10cbe0a Compare May 17, 2018 00:34

dnldd changed the title ~~hdkeychain: use secp256k1 types and fields directly.~~ multi: use secp256k1 types and fields directly. May 17, 2018

dnldd force-pushed the remove_hdkeychain_chainecrefs branch from 10cbe0a to 2e293f2 Compare May 17, 2018 00:47

davecgh approved these changes May 17, 2018

View reviewed changes

davecgh merged commit 2e293f2 into decred:master May 17, 2018

dnldd deleted the remove_hdkeychain_chainecrefs branch June 4, 2018 23:50

JFixby pushed a commit to JFixby/dcrd that referenced this pull request Oct 21, 2018

Merge pull request decred#1211 from wpaulino/gcs-modifications

fdfc190

multi: update gcs indexing and serving code to match latest BIP 158 instance

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: use secp256k1 types and fields directly. #1211

multi: use secp256k1 types and fields directly. #1211

dnldd commented May 16, 2018 •

edited

Loading

davecgh May 16, 2018

davecgh May 16, 2018

davecgh May 16, 2018

davecgh May 16, 2018 •

edited

Loading

davecgh May 16, 2018

davecgh May 16, 2018

davecgh May 16, 2018

davecgh May 16, 2018

davecgh commented May 16, 2018 •

edited

Loading

davecgh May 16, 2018

davecgh May 17, 2018

multi: use secp256k1 types and fields directly. #1211

multi: use secp256k1 types and fields directly. #1211

Conversation

dnldd commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented May 16, 2018 •

edited

Loading

davecgh May 16, 2018 •

edited

Loading

davecgh commented May 16, 2018 •

edited

Loading