Implement bip32 #1676

someone235 · 2021-04-12T09:50:39Z

No description provided.

codecov · 2021-04-12T09:59:21Z

Codecov Report

Merging #1676 (2b6abdf) into v0.11.0-dev (c28366e) will increase coverage by 0.15%.
The diff coverage is 67.30%.

❗ Current head 2b6abdf differs from pull request most recent head e91d7ff. Consider uploading reports for the commit e91d7ff to get more accurate results

@@               Coverage Diff               @@
##           v0.11.0-dev    #1676      +/-   ##
===============================================
+ Coverage        59.14%   59.29%   +0.15%     
===============================================
  Files              556      565       +9     
  Lines            22035    22399     +364     
===============================================
+ Hits             13032    13281     +249     
- Misses            6930     7008      +78     
- Partials          2073     2110      +37

Impacted Files	Coverage Δ
...awallet/libkaspawallet/bip32/base58/base58check.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/libkaspawallet/bip32/bip32.go	`15.78% <15.78%> (ø)`
.../kaspawallet/libkaspawallet/bip32/base58/base58.go	`53.33% <53.33%> (ø)`
...d/kaspawallet/libkaspawallet/bip32/extended_key.go	`65.11% <65.11%> (ø)`
...allet/libkaspawallet/bip32/child_key_derivation.go	`68.67% <68.67%> (ø)`
cmd/kaspawallet/libkaspawallet/bip32/path.go	`73.33% <73.33%> (ø)`
cmd/kaspawallet/libkaspawallet/bip32/hash.go	`78.94% <78.94%> (ø)`
.../kaspawallet/libkaspawallet/bip32/serialization.go	`84.94% <84.94%> (ø)`
cmd/kaspawallet/libkaspawallet/bip32/version.go	`95.83% <95.83%> (ø)`
...s/processes/difficultymanager/difficultymanager.go	`81.33% <100.00%> (+0.51%)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c28366e...e91d7ff. Read the comment docs.

svarogg · 2021-04-12T11:41:14Z

cmd/kaspawallet/libkaspawallet/bip32/bip32.go

@@ -0,0 +1,41 @@
+package bip32


I don't think we want the word "bip32" in our code, this is good time to think about a more general name, that does not reference other coins.

Sounds like this is code that is more general then libkaspawallet, I'd place it on the same level as the bech32 package (a.k.a. under "util")

This is an implementation of bip32. I don't see any problem with this name.

bip32 is related only to wallets, so I think this place is ok

svarogg · 2021-04-12T11:45:34Z

cmd/kaspawallet/libkaspawallet/bip32/bip32_test.go

+func TestBIP32SpecVectors(t *testing.T) {
+	type testPath struct {
+		path   string
+		extPub string


Please use full words for names.

svarogg · 2021-04-12T11:46:53Z

cmd/kaspawallet/libkaspawallet/bip32/bip32_test.go

+
+	type testVector struct {
+		seed   string
+		pathes []testPath


typo: paths

svarogg · 2021-04-12T11:58:42Z

cmd/kaspawallet/libkaspawallet/bip32/version.go

+
+import "github.com/pkg/errors"
+
+var BitcoinMainnetPrivate = [4]byte{


Why is it needed? My guess is only to verify against bitcoin-provided test vectors.
As such, I'd rather if this was only in test code.

EDIT: Read some more of the code, now I understand this is not as trivial as I thought.
As such, I'd rather not have bitcoin vectors at all.

This is an implementation of the bip32 spec, so it makes sense to use the test vectors that are specified by the spec

OK, but this should be in test code only

Yeah, but then how can we use toPublicVersion?

svarogg · 2021-04-12T12:05:00Z

cmd/kaspawallet/libkaspawallet/bip32/bip32_test.go

+			t.Fatalf("DecodeString: %+v", err)
+		}
+
+		masterKey, err := NewMaster(seed, BitcoinMainnetPrivate)


Please add a few test vectors with Kaspa master

svarogg · 2021-04-12T12:48:08Z

cmd/kaspawallet/libkaspawallet/bip32/hash.go

+	return sha2.Sum(nil)
+}
+
+func validateChecksum(data []byte) error {


Please add a comment that explains what's the expected structure of data (I'd also rename it to dataWithChecksum)

svarogg · 2021-04-12T12:51:51Z

cmd/kaspawallet/libkaspawallet/bip32/path.go

+
+func parsePath(pathString string) (*path, error) {
+	parts := strings.Split(pathString, "/")
+	if len(parts) == 0 {


Why can't a path be just "m" or "M"?
It is being used this way, for example, here: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#full-wallet-sharing-m

Not sure it's even going to happen in the context of this function, but as a general-purpose function I think just "m" should be supported

Yeah, you are right. I think this condition can never happen

svarogg · 2021-04-12T12:54:14Z

cmd/kaspawallet/libkaspawallet/bip32/path.go

+}
+
+func parseIndex(indexString string) (uint32, error) {
+	const isHardenedSuffix = "'"


This is really annoying that the spec uses the unprintable _H while everyone in the world uses '.

As such, I'd make this suffix much more visible (either in some readme, or somewhere else where it's clear what this means).

svarogg · 2021-04-12T12:59:14Z

cmd/kaspawallet/libkaspawallet/bip32/serialization.go

+	checkSumLen
+
+func DeserializeExtendedPrivateKey(extKeyString string) (*ExtendedKey, error) {
+	serializedBytes := base58.Decode(extKeyString)


Do we really want to encode this in base58?
And even worse - using btcutil library for doing that...

This is part of the spec. I can copy the library though

I think we want to copy the library then.

I think you missed copying the library.

svarogg · 2021-04-12T13:25:16Z

cmd/kaspawallet/libkaspawallet/bip32/version.go

+}
+
+var KaspaMainnetPrivate = [4]byte{
+	0x01,


I think we want something human-recognizable here.

elichai · 2021-04-21T09:26:53Z

cmd/kaspawallet/libkaspawallet/bip32/ckd.go

+	var iL, iR [32]byte
+	copy(iL[:], I[:32])
+	copy(iR[:], I[32:])
+
+	privateKey, err := secp256k1.DeserializeECDSAPrivateKeyFromSlice(iL[:])


nit,

var iL secp256k1.SerializedPrivateKey copy(iL[:], I[:32]) privateKey, err := secp256k1.DeserializeECDSAPrivateKey(&iL)

is a little more efficient

elichai · 2021-04-21T09:28:04Z

cmd/kaspawallet/libkaspawallet/bip32/extended_key.go

+	publicKey   *secp256k1.ECDSAPublicKey
+	Version     [4]byte
+	Depth       uint8
+	Fingerprint [4]byte


I think "ParentFingerprint" is more correct

elichai · 2021-04-21T09:38:41Z

cmd/kaspawallet/libkaspawallet/bip32/ckd.go

+	return &kCopy, nil
+}
+
+func calcI(extKey *ExtendedKey, i uint32) ([]byte, error) {


maybe make this a function on ExtendedKey?

elichai · 2021-04-21T09:42:46Z

cmd/kaspawallet/libkaspawallet/bip32/ckd.go

+	if isHardened(i) {
+		if !extKey.IsPrivate() {
+			return nil, errors.Errorf("Cannot calculate hardened child for public key")
+		}
+
+		mac := newHMACWriter(extKey.ChainCode[:])
+		mac.InfallibleWrite([]byte{0x00})
+		mac.InfallibleWrite(extKey.PrivateKey.Serialize()[:])
+		mac.InfallibleWrite(ser32(i))
+		return mac.Sum(nil), nil
+	}
+
+	mac := newHMACWriter(extKey.ChainCode[:])
+	publicKey, err := extKey.PublicKey()
+	if err != nil {
+		return nil, err
+	}
+
+	serializedPublicKey, err := publicKey.Serialize()
+	if err != nil {
+		return nil, errors.Wrap(err, "error serializing public key")
+	}
+
+	mac.InfallibleWrite(serializedPublicKey[:])
+	mac.InfallibleWrite(ser32(i))
+	return mac.Sum(nil), nil
+}


Nit, I find that writing it like this allows to easily see the difference between hardened and non-hardened (and reduces code duplication):

func calcI(extKey *ExtendedKey, i uint32) ([]byte, error) { if isHardened(i) && !extKey.IsPrivate() { return nil, errors.Errorf("Cannot calculate hardened child for public key") } mac := newHMACWriter(extKey.ChainCode[:]) if isHardened(i) { mac.InfallibleWrite([]byte{0x00}) mac.InfallibleWrite(extKey.PrivateKey.Serialize()[:]) } else { publicKey, err := extKey.PublicKey() if err != nil { return nil, err } serializedPublicKey, err := publicKey.Serialize() if err != nil { return nil, errors.Wrap(err, "error serializing public key") } mac.InfallibleWrite(serializedPublicKey[:]) } mac.InfallibleWrite(ser32(i)) return mac.Sum(nil), nil }

elichai · 2021-04-21T09:49:31Z

cmd/kaspawallet/libkaspawallet/bip32/ckd.go

+	return fingerPrintFromPoint(point)
+}
+
+func fingerPrintFromPoint(point *secp256k1.ECDSAPublicKey) ([4]byte, error) {


nit, maybe make Fingerprint() a public function on ExtendedKey? I think users will want it (especially as ExtendedKey already exports the parent's fingerprint publicly)

I'm not sure why users may need it. For now I'll keep it private

elichai · 2021-04-21T10:10:04Z

cmd/kaspawallet/libkaspawallet/bip32/ckd.go

+	return fingerprint, nil
+}
+
+func hash160(data []byte) []byte {


Please move this to hash.go

elichai · 2021-04-21T10:11:52Z

cmd/kaspawallet/libkaspawallet/bip32/hash.go

+	sha1 := sha256.New()
+	sha2 := sha256.New()
+	sha1.Write(data)
+	sha2.Write(sha1.Sum(nil))
+	return sha2.Sum(nil)
+}


nit, you can do:

inner := sha256.Sum256(data) outer := sha256.Sum256(inner[:]) return outer[:]

elichai · 2021-04-21T10:24:23Z

cmd/kaspawallet/libkaspawallet/bip32/serialization.go

+
+func (extKey *ExtendedKey) serialize() ([]byte, error) {
+	var serialized [extendedKeySerializationLen]byte
+	copy(serialized[:versionSerializationLen], extKey.Version[:])


It's a little weird that this is the only place that you specify the end of the slice

elichai · 2021-04-21T10:31:42Z

cmd/kaspawallet/libkaspawallet/bip32/serialization.go

+		serialized[versionSerializationLen+depthSerializationLen+fingerprintSerializationLen+childNumberSerializationLen+chainCodeSerializationLen] = 0
+		copy(
+			serialized[versionSerializationLen+depthSerializationLen+fingerprintSerializationLen+childNumberSerializationLen+chainCodeSerializationLen+1:],
+			extKey.PrivateKey.Serialize()[:],
+		)


nit, This is too long for my IDE

elichai · 2021-04-21T12:25:15Z

cmd/kaspawallet/libkaspawallet/bip32/serialization.go

+	checkSum := doubleSha256(serialized[:len(serialized)-checkSumLen])
+	copy(
+		serialized[versionSerializationLen+depthSerializationLen+fingerprintSerializationLen+childNumberSerializationLen+chainCodeSerializationLen+keySerializationLen:],
+		checkSum,
+	)
+	return serialized[:], nil


nit, I think the checksum is more part of the base58 encoding than part of the serialization,
but serialize is currently a private function so we don't care about the distinction (it also saves use from another allocation+copy)

svarogg · 2021-04-27T12:29:20Z

cmd/kaspawallet/libkaspawallet/bip32/extended_key.go

+	"github.com/pkg/errors"
+)
+
+type ExtendedKey struct {


Hmmm... oh well I guess.

svarogg · 2021-04-27T12:36:21Z

cmd/kaspawallet/libkaspawallet/bip32/bip32_test.go

-			pathes: []testPath{
+			seed:    "000102030405060708090a0b0c0d0e0f",
+			version: BitcoinMainnetPrivate,
+			paths: []testPath{


You have no test vectors that check public key deriviation (a.k.a capital M paths), maybe write some such as well?
(Or alternatively, for each test-vector here, also test what happens when you replace m with M)

TestExtendedKey_DeriveFromPath checks that.

svarogg · 2021-04-27T12:42:00Z

cmd/kaspawallet/libkaspawallet/bip32/bip32_test.go

 				t.Fatalf("Test (%d, %d): deserializing and serializing the ext pub didn't preserve the data", i, j)
 			}
 		}
 	}
 }

-func TestExtendedPublicKey_Path(t *testing.T) {
+// TestExtendedKey_DeriveFromPath checks that path that derive from extended public key and extended


Suspected typo: derive from extended public key and extended public key

I think you want to replace one "public" to "private"

svarogg · 2021-04-27T12:46:40Z

cmd/kaspawallet/libkaspawallet/bip32/serialization.go

+	checkSumLen
+
+func DeserializeExtendedPrivateKey(extKeyString string) (*ExtendedKey, error) {
+	serializedBytes := base58.Decode(extKeyString)


I think you missed copying the library.

svarogg · 2021-04-27T12:52:37Z

cmd/kaspawallet/libkaspawallet/bip32/version.go

 var BitcoinMainnetPublic = [4]byte{
 	0x04,
 	0x88,
 	0xb2,
 	0x1e,
 }

+// KaspaMainnetPrivate is the version that is used for


Do we maybe want to define versions for kaspa testnet/mainnet/simnet as well?

svarogg · 2021-04-28T11:48:30Z

cmd/kaspawallet/libkaspawallet/bip32/version.go

+// KaspaTestnetPrivate is the version that is used for
+// kaspa testnet bip32 public extended keys.
+var KaspaTestnetPrivate = [4]byte{
+	0x03,


Please use either decimal or hex for these values, but not a mixture. (This also refers to the fact that the previous versions were all hexa, and the new ones are mostly decimal).

Also, maybe write in the comment what this encodes to? (a.k.a. kpriv/tpub/etc)

someone235 added 3 commits April 11, 2021 18:57

Implement bip32

04dad84

Unite private and public extended keys

efbc12d

Merge branch 'v0.10.0-dev' into bip32

dfad3fe

svarogg requested changes Apr 12, 2021

View reviewed changes

Merge branch 'v0.10.0-dev' into bip32

a712c20

stasatdaglabs changed the base branch from v0.10.0-dev to v0.11.0-dev April 20, 2021 10:41

elichai reviewed Apr 21, 2021

View reviewed changes

someone235 added 19 commits April 22, 2021 17:40

Change variable names

454e02d

Change test name and add comment

9acbbce

Rename var name

244784e

Rename ckd.go to child_key_derivation.go

53b5eec

Rename ser32 -> serializeUint32

7ad1181

Add PrivateKey method

5b6fc37

Rename Path -> DeriveFromPath

bca5ed3

Add comment to validateChecksum

1ce94f7

Remove redundant condition from parsePath

c68e87d

Rename Fingerprint->ParentFingerprint

e755f23

Merge hardened and non-hardened paths in calcI

d697905

Change fingerPrintFromPoint to method

bf8fa12

Move hash160 to hash.go

a629d20

Fix a bug in calcI

84d9bf6

Simplify doubleSha256

810874a

Remove slice end bound

17bd382

Split long line

d15f8de

Change KaspaMainnetPrivate/public to represent kprv/kpub

2c7f05e

Add comments

f688424

elichai previously approved these changes Apr 27, 2021

View reviewed changes

Merge branch 'v0.11.0-dev' into bip32

dca18a6

svarogg reviewed Apr 27, 2021

View reviewed changes

svarogg requested changes Apr 27, 2021

View reviewed changes

someone235 added 3 commits April 27, 2021 18:04

Fix comment

376e5d2

Merge branch 'bip32' of github.com:kaspanet/kaspad into bip32

5300321

Copy base58 library to kaspad

396f8be

someone235 dismissed elichai’s stale review via 396f8be April 27, 2021 15:08

Add versions for all networks

5cd1b00

svarogg requested changes Apr 28, 2021

View reviewed changes

someone235 added 2 commits April 28, 2021 15:11

Change versions to hex

cdea723

Add comments

e91d7ff

svarogg approved these changes Apr 28, 2021

View reviewed changes

svarogg merged commit fa16c30 into v0.11.0-dev Apr 28, 2021

svarogg deleted the bip32 branch April 28, 2021 12:27

dravenk mentioned this pull request May 10, 2023

support seed BIP39 #2206

Closed


		import "github.com/pkg/errors"

		var BitcoinMainnetPrivate = [4]byte{

Implement bip32 #1676

Implement bip32 #1676

Conversation

someone235 commented Apr 12, 2021

codecov bot commented Apr 12, 2021 • edited Loading

Codecov Report

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai Apr 21, 2021 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2021 •

edited

Loading

elichai Apr 21, 2021 •

edited

Loading