Implement BIP 39 and HD wallet features #1705

someone235 · 2021-05-11T15:15:27Z

…he relevant endpoints

codecov · 2021-05-11T15:24:55Z

Codecov Report

Merging #1705 (3bc52f0) into v0.11.0-dev (010df3b) will decrease coverage by 0.69%.
The diff coverage is 18.39%.

@@               Coverage Diff               @@
##           v0.11.0-dev    #1705      +/-   ##
===============================================
- Coverage        59.48%   58.78%   -0.70%     
===============================================
  Files              565      577      +12     
  Lines            22445    22774     +329     
===============================================
+ Hits             13351    13388      +37     
- Misses            6974     7257     +283     
- Partials          2120     2129       +9

Impacted Files	Coverage Δ
cmd/kaspawallet/balance.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/broadcast.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/common.go	`0.00% <ø> (ø)`
cmd/kaspawallet/config.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/create.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/create_unsigned_tx.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/client/client.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/server/address.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/server/balance.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/server/broadcast.go	`0.00% <0.00%> (ø)`
... and 38 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 010df3b...3bc52f0. Read the comment docs.

svarogg · 2021-05-12T08:48:07Z

cmd/kaspawallet/config.go

@@ -62,13 +66,20 @@ type signConfig struct {
 }

 type broadcastConfig struct {
-	RPCServer   string `long:"rpcserver" short:"s" description:"RPC server to connect to"`
-	Transaction string `long:"transaction" short:"t" description:"The signed transaction to broadcast (encoded in hex)" required:"true"`
+	ServerAddress string `long:"walletserver" short:"s" description:"Wallet server to connect to"`


I don't like the fact that it's usually called daemon, but here it's called "Server".
Please pick one and use everywhere.

Also, I'd add (default: localhost) in the description. (It is the default, right?)

I think you missed this part:
Also, I'd add (default: localhost) in the description. (It is the default, right?)

cmd/kaspawallet/create.go

svarogg · 2021-05-12T10:31:09Z

cmd/kaspawallet/create.go

 	"os"

 	"github.com/kaspanet/kaspad/cmd/kaspawallet/keys"
-	"github.com/kaspanet/kaspad/cmd/kaspawallet/libkaspawallet"
 	"github.com/pkg/errors"
 )

 func create(conf *createConfig) error {


I'd rather if all the logic in this function (and similar functions as well) will be in libkaspawallet.
(Just extract the needed info out of config first, so that the lib doesn't rely on config, and maybe also the creation of the keys file)

We want a re-usable library that can be used for multiple wallets, even ones that don't want to use the daemon.

This is the way it worked before and not related to this PR

OK, Created issue, feel free to add info to the issue / fight me if you think this is not what we want.
#1724

cmd/kaspawallet/libkaspawallet/bip39.go

svarogg · 2021-05-13T13:15:05Z

cmd/kaspawallet/libkaspawallet/bip39.go

+		return bip32.KaspaSimnetPrivate, nil
+	}
+
+	return [4]byte{}, errors.Errorf("unknown network %s", params.Name)


Not very critical, but I'd put this inside default:

Looks like you missed this comment (if you purposefully ignored because you disagree - that's fine, just LMK)

cmd/kaspawallet/send.go

cmd/kaspawallet/sign.go

go.mod

svarogg · 2021-05-18T09:10:57Z

cmd/kaspawallet/daemon/server/sync.go

+}
+
+// collectUTXOsFromFarAddresses collects UTXOs
+// from s.nextSyncStartIndex to s.nextSyncStartIndex+numIndexesToQuery


I'd rather if these comments were more accessible (a.k.a. didn't assume person knows the meaning of variables such as s.nextSyncStartIndex.

In other words, explain the difference between Close and Far addresses in laymen terms.

svarogg · 2021-05-18T09:32:44Z

cmd/kaspawallet/create.go

 	}

-	err = keys.WriteKeysFile(
-		conf.NetParams(), conf.KeysFile, encryptedPrivateKeys, publicKeys, conf.MinimumSignatures, conf.ECDSA)
+	cosignerIndex, err := libkaspawallet.MinimumCosignerIndex(signerExtendedPublicKeys, extendedPublicKeys)


The sort is an implementation detail, why should the user be aware of it?

The sort is not an implementation detail, it is defined in the BIP.

The assumption is that keys are always ordered except in exported functions
Couldn't it be nice if the assumption had some (even if weak) guarantee using the type-system.

Correct me if I'm wrong, but it seems like addressesToQuery assumes s.keysFile.ExtendedPublicKeys is sorted, while in reality it isn't so.
(also dumpUnencrypted Data, where it's not as critical, but still could be nice)

Even if it is, I'd really love it if you had to call sortPublicKeys only once, not every time you need this assumption - this is very error prone, and someone will forget.

cmd/kaspawallet/daemon/server/address.go

svarogg · 2021-05-18T09:51:08Z

cmd/kaspawallet/keys/keys.go

@@ -239,19 +241,24 @@ func getAEAD(password, salt []byte) (cipher.AEAD, error) {
 	return chacha20poly1305.NewX(key)
 }

-func decryptPrivateKey(encryptedPrivateKey *EncryptedPrivateKey, password []byte) ([]byte, error) {
+func decryptMnemonic(encryptedPrivateKey *EncryptedMnemonic, password []byte) (string, error) {
 	aead, err := getAEAD(password, encryptedPrivateKey.salt)


And... are everyone supposed to know that?

svarogg · 2021-05-18T09:53:45Z

cmd/kaspawallet/libkaspawallet/bip39.go

+		return bip32.KaspaSimnetPrivate, nil
+	}
+
+	return [4]byte{}, errors.Errorf("unknown network %s", params.Name)


Looks like you missed this comment (if you purposefully ignored because you disagree - that's fine, just LMK)

svarogg · 2021-05-18T09:55:13Z

cmd/kaspawallet/daemon/server/create_unsigned_transaction.go

+		return nil, err
+	}
+
+	psTx, err := libkaspawallet.CreateUnsignedTransaction(s.keysFile.ExtendedPublicKeys,


Please search all over the code for "psTx" and rename to something that everyone understand.

elichai · 2021-05-18T15:25:39Z

cmd/kaspawallet/keys/create.go

-		privateKey, publicKey, err := keyPairFunction(i)
+	encryptedPrivateKeys = make([]*EncryptedMnemonic, 0, len(mnemonics))
+	for _, mnemonic := range mnemonics {
+		extendedPublicKey, err := libkaspawallet.MasterPublicKeyFromMnemonic(params, mnemonic, isMultisig)


So now we save the mnemonics in the DB?
Is there any reason why it is necessary?
why not internally only use extended keys and only in backup/recover use mnemonics?
This will drastically decrease the attack vector that bip39 imposes

someone235 added 15 commits May 3, 2021 15:17

Naive bip39 with address reuse

9ab0946

Avoid address reuse in libkaspawallet

696b696

Add wallet daemon

5f060f4

Use daemon everywhere

b5ec249

Add forceOverride

1182bbc

Make CreateUnsignedTransaction endpoint receive amount in sompis

538497e

Collect close UTXOs

2be4ab6

Filter out non-spendable UTXOs from selectUTXOs

a913aa0

Use different paths for multisig and non multisig

334513a

Fix tests to use non zero path

9717ca6

Fix multisig cosigner index detection

b111b34

Add comments

e13b58a

Fix dump_unencrypted_data.go according to bip39 and bip32

c46bbdb

Fix wrong derivation path for multisig on wallet creation

153a493

Remove IsSynced endpoint and add validation if wallet is synced for t…

317d365

…he relevant endpoints

svarogg requested changes May 13, 2021

View reviewed changes

someone235 added 13 commits May 16, 2021 10:36

Rename server address to daemon address

0836519

Fix capacity for extendedPublicKeys

81c3f62

Use ReadBytes instead of ReadLine

095f62f

Add validation when importing

1701033

Increment before using index value, and use it as is

10d117e

Save keys file exactly where needed

15cdb33

Use %+v printErrorAndExit

7a2db9c

Remove redundant consts

3c5fcea

Rnemae collectCloseUTXOs and collectFarUTXOs

44dcccc

Move typedefs around

d855c4d

Add comment to addressesToQuery

0f8fefb

Update collectUTXOsFromRecentAddresses comment about locks

1ae2ef5

Split collectUTXOs to small functions

448554c

someone235 added 10 commits May 16, 2021 12:13

Add sanity check

25a188d

Add addEntryToUTXOSet function

e8cbbf9

Change validateIsSynced to isSynced

24c4a53

Simplify createKeyPairsFromFunction logic

9258df1

Rename .Sync() to .Save()

63e6ec5

Fix typo

93a3daa

Create bip39BitSize const

72e2bee

Add consts to purposes

d4509d6

Add multisig check for 'send'

d76f41b

Rename updatedPSTxBytes to partiallySignedTransaction

f49dcf5

someone235 force-pushed the bip39 branch from 68ca8e2 to f49dcf5 Compare May 16, 2021 09:41

svarogg reviewed May 18, 2021

View reviewed changes

svarogg requested changes May 18, 2021

View reviewed changes

someone235 added 3 commits May 18, 2021 13:27

Change collectUTXOsFromFarAddresses's comment

eea7398

Use setters for last used indexes

3919dc6

Don't use the pstx acronym

00e671d

svarogg previously approved these changes May 18, 2021

View reviewed changes

svarogg dismissed their stale review via c92f65c May 18, 2021 13:19

someone235 added 2 commits May 18, 2021 16:52

Fix SetPath

f9b0a2f

Remove spaces when reading lines

25275d5

someone235 force-pushed the bip39 branch from f142b21 to 25275d5 Compare May 18, 2021 14:02

someone235 added 3 commits May 18, 2021 17:08

Fix walletserver to daemonaddress

e5bc879

Merge remote-tracking branch 'origin/v0.11.0-dev' into bip39

adc26bc

Fix isUTXOSpendable to use DAA score

7eef750

elichai reviewed May 18, 2021

View reviewed changes

svarogg approved these changes May 19, 2021

View reviewed changes

Merge branch 'v0.11.0-dev' into bip39

3bc52f0

svarogg merged commit 4658f9d into v0.11.0-dev May 19, 2021

svarogg deleted the bip39 branch May 19, 2021 07:03

dravenk mentioned this pull request May 10, 2023

support seed BIP39 #2206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement BIP 39 and HD wallet features #1705

Implement BIP 39 and HD wallet features #1705

someone235 commented May 11, 2021 •

edited

Loading

codecov bot commented May 11, 2021 •

edited

Loading

svarogg May 12, 2021

svarogg May 18, 2021

svarogg May 12, 2021

someone235 May 13, 2021

svarogg May 18, 2021

svarogg May 13, 2021

svarogg May 18, 2021

svarogg May 18, 2021

svarogg May 18, 2021 •

edited

Loading

svarogg May 18, 2021

svarogg May 18, 2021

svarogg May 18, 2021

elichai May 18, 2021

someone235 May 18, 2021

Implement BIP 39 and HD wallet features #1705

Implement BIP 39 and HD wallet features #1705

Conversation

someone235 commented May 11, 2021 • edited Loading

codecov bot commented May 11, 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

svarogg May 18, 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

someone235 commented May 11, 2021 •

edited

Loading

codecov bot commented May 11, 2021 •

edited

Loading

svarogg May 18, 2021 •

edited

Loading