Allow arbitrary xpub account imports. #1471

jrick · 2019-06-03T14:40:31Z

This commit implements a new JSON-RPC method "importxpub" which
records an arbitrary HD extended pubkey to the wallet database and
allows new addresses to be derived for the account. It takes two
positional parameters: an account name and the HD extended pubkey in
string format.

Addresses of imported xpub accounts are not watched and transactions
are not saved. This may change later, but the purpose of this feature
is to allow the automatic ticket buyer to derive unique voting
addresses in sequence, avoiding address reuse.

This change contains a database upgrade and it is not possible to
downgrade a wallet database once run. Reverting to older code
requires restoring the database from backup or seed restoring.

jrick · 2019-06-03T14:46:08Z

As mentioned, this change contains a database upgrade to version 12. This conflicts with #1330 and I would like to merge that PR to master first, and then I will rebase over and upgrade this PR to use version 13.

jrick · 2019-06-06T00:40:14Z

This is causing the getaddressesbyaccount RPC to error with -4: wallet.AccountBranchAddressRange: item does not exist: account 2147483648 when called with an imported xpub account. It should return all addresses of the account returned so far. Found by @davecgh.

matheusd

Seems reasonable. Testing on decrediton after importing via CLI returned the following error when generating a new address for the imported account:

2019-06-06 09:23:01.985 [ERR] GRPC: Unary method /walletrpc.WalletService/NextAddress invoked by 127.0.0.1:58648 errored: rpc error: code = NotFound desc = wallet.PubKeyForAddress: item does not exist: no address TsmDjaV6Ck1LCjUba9UvCU15is3Ev94kURP

This is the specific call that's failing:

dcrwallet/rpc/rpcserver/server.go

Line 484 in 6867c05

pubKey, err := s.wallet.PubKeyForAddress(addr)

jrick · 2019-06-06T13:40:06Z

Hmm that is going to need an interesting fix. It fails because it is trying to look up the public key (which it just derived internally), saved by the DB (which it is not in these imported accounts, and I don't think it's a good idea to until we start watching). If instead of Wallet.New{Internal,External}Address it called an api which returned both the xpub and child index, the RPC method handler could derive both.

jrick · 2019-06-06T13:48:38Z

Thinking ahead to some things we may want to do with accounts in the future (such as creating an account which derives p2sh multisig addresses from two or more other xpub accounts in sequence), returning an interface (which Address already is so we could piggy back off that) and then type asserting as a more specific interface with xpub and child accessors would be a possible way to support this without adding many new APIs. Unfortunately, there's probably code out there that expects all p2pkh addresses returned to be the dcrutil.AddressPubKeyHash concrete type.

matheusd · 2019-06-06T14:42:30Z

Unfortunately, there's probably code out there that expects all p2pkh addresses returned to be the dcrutil.AddressPubKeyHash concrete type.

Next{Internal,Externa}Address aren't documented as returning only p2pkh keys though I guess we could implicitly assume that is the case given their names.

The solution to this specific error might be to just tweak the grpc endpoint to check for the special imported accounts and not return the public key for the moment (until the address is watched and saved in the db as you point out). Or add an "AddressKind" or something to the grpc response so that in the future we can indicate whether the returned address is from an imported account, multisig voting account, etc.

Unless the multisig-generating account will also be another fixed account (similar to the imported account for scripts), in that case the address type in the response would be implied by which account you tried to generate the address in.

jrick · 2019-06-06T15:00:38Z

Yes, idea with that account is that it would only be used for p2sh-multisig from other stored xpubs. This would enable the stakepool idea in #1217.

It's true that code should not be switching on the concrete type since it's not guaranteed by the API. Perhaps we should try it and see what breaks?

matheusd · 2019-06-06T15:05:58Z

It's true that code should not be switching on the concrete type since it's not guaranteed by the API.

Heh, I'd argue that since the concrete type is not enforced (since it's the interface type that is returned) and it's not explicitly documented that only p2pkh's are returned, then it's fine to change the concrete type.

Perhaps we should try it and see what breaks?

Sure thing.

jrick · 2019-06-06T15:15:18Z

Have something like this in mind:

package wallet

import (
        "github.com/decred/dcrd/dcrutil"
        "github.com/decred/dcrd/hdkeychain/v2"
)

type AddressPubKeyHashV0 interface {
        dcrutil.Address
        PubKeyHash160() *[20]byte
}

type XpubPather interface {
        XpubPath() (xpub *hdkeychain.ExtendedKey, branch, child uint32)
}

type xpubAddress struct {
        *dcrutil.AddressPubKeyHash
        xpub *hdkeychain.ExtendedKey
        branch, child uint32
}

func (x *xpubAddress) PubKeyHash160() *[20]byte {
        return x.AddressPubKeyHash.Hash160()
}

func (x *xpubAddress) XpubPath() (xpub *hdkeychain.ExtendedKey, branch, child uint32) {
        return x.xpub, x.branch, x.child
}

jrick · 2019-06-06T15:59:51Z

Revised https://play.golang.org/p/ovZYC9uJ5bC

matheusd · 2019-06-06T16:40:13Z

Seems nice!

jrick · 2019-06-06T19:14:11Z

See #1474.

matheusd

(I know you're aware of these, leaving here for completenes' sake)

Address discovery requires restarting after importing the xpub and waiting for a block to be mined. We can solve this by either documenting or creating an endpoint to perform a rescan+address discovery (either completely or only of specific accounts).
Imported accounts show a balance in some situations via gRPC (annotated inline).

matheusd · 2019-08-29T11:57:23Z

rpc/jsonrpc/methods.go

@@ -682,6 +683,11 @@ func (s *Server) getBalance(ctx context.Context, icmd interface{}) (interface{},
 		result.Balances = make([]types.GetAccountBalanceResult, balancesLen)

 		for _, bal := range balances {
+			// Transactions are not tracked for imported xpub accounts.
+			if bal.Account > udb.ImportedAddrAccount {


This check needs to be added to grpc as well if this is going to be done at this level of abstraction (rpc endpoints).

Another alternative would be to add this directly in the address manager when calculating the balances so it's consistent over all current (and future) comm endpoints.

itswisdomagain · 2019-10-08T03:34:37Z

wallet/addresses.go

+	pk, err := childKey.ECPubKey()
+	if err != nil {
+		return nil, errors.E(op, err)
+	}
+	pkh := dcrutil.Hash160(pk.Serialize())
+	apkh, err := dcrutil.NewAddressPubKeyHash(pkh, w.chainParams,
+		dcrec.STEcdsaSecp256k1)


Can simplify to apkh, err := compat.HD2Address(childKey, w.chainParams)

itswisdomagain · 2019-10-08T03:35:57Z

wallet/udb/addressmanager.go

@@ -576,7 +576,7 @@ func (m *Manager) AccountExtendedPubKey(dbtx walletdb.ReadTx, account uint32) (*
 // unlocked for this operation to complete.
 func (m *Manager) AccountExtendedPrivKey(dbtx walletdb.ReadTx, account uint32) (*hdkeychain.ExtendedKey, error) {
 	if account == ImportedAddrAccount {
-		return nil, errors.E(errors.Invalid, "imported account has no extended pubkey")
+		return nil, errors.E(errors.Invalid, "imported address account has no extended pubkey")


Suggested change

return nil, errors.E(errors.Invalid, "imported address account has no extended pubkey")

return nil, errors.E(errors.Invalid, "imported address account has no extended privkey")

itswisdomagain · 2019-10-08T04:14:28Z

wallet/udb/addressmanager.go

+		return err
+	}
+
+	// There may not be an account by the same name


Suggested change

// There may not be an account by the same name

// There may already be an account by the same name

itswisdomagain · 2019-10-08T04:17:52Z

wallet/udb/addressmanager.go

+		return err
+	}
+	account++
+	if account < MaxAccountNum {


This check should be unnecessary as fetchLastImportedAccount is guaranteed to return account >= ImportedAddrAccount and ImportedAddrAccount > MaxAccountNum

itswisdomagain · 2019-10-08T04:24:02Z

wallet/udb/addressmanager.go

+	// We have the encrypted account extended keys, so save them to the
+	// database
+	row := bip0044AccountInfo(acctPubEnc, nil, 0, 0,
+		^uint32(0), ^uint32(0), ^uint32(0), ^uint32(0), name, DBVersion)


importedXpubAccountVersion instead of DBVersion?

This commit implements a new JSON-RPC method "importxpub" which records an arbitrary HD extended pubkey to the wallet database and allows new addresses to be derived for the account. It takes two positional parameters: an account name and the HD extended pubkey in string format. This change contains a database upgrade and it is not possible to downgrade a wallet database once run. Reverting to older code requires restoring the database from backup or seed restoring.

jrick mentioned this pull request Jun 3, 2019

multi: Implement xpub import from other wallet and use for address generation #1137

Closed

matheusd reviewed Jun 6, 2019

View reviewed changes

jrick force-pushed the xpubaccount branch from 22316b2 to 925dac1 Compare June 6, 2019 13:28

jrick force-pushed the xpubaccount branch 3 times, most recently from 4c2b098 to eac9a4f Compare June 13, 2019 19:48

jrick force-pushed the xpubaccount branch 3 times, most recently from a1f4777 to 34d56b7 Compare June 20, 2019 18:07

xaur mentioned this pull request Jul 8, 2019

Maximize stakeholder privacy decredcommunity/issues#25

Open

12 tasks

jrick force-pushed the xpubaccount branch from ef55265 to b7d9088 Compare August 2, 2019 15:50

JoeGruffins mentioned this pull request Aug 23, 2019

Derive a new fee address for every ticket. decred/dcrstakepool#504

Closed

jrick force-pushed the xpubaccount branch 2 times, most recently from 9b70e21 to f8745ab Compare August 28, 2019 15:27

jrick mentioned this pull request Aug 28, 2019

Add CoinShuffle++ support #1541

Merged

jrick force-pushed the xpubaccount branch 2 times, most recently from dc9f541 to 56e2049 Compare August 28, 2019 18:39

matheusd reviewed Aug 29, 2019

View reviewed changes

JoeGruffins mentioned this pull request Sep 5, 2019

search addresses derived from imported xpub accounts for pool fees #1545

Closed

itswisdomagain suggested changes Oct 8, 2019

View reviewed changes

jrick force-pushed the xpubaccount branch from 56e2049 to b37c352 Compare October 22, 2019 18:40

jrick merged commit b37c352 into decred:master Oct 22, 2019

jrick deleted the xpubaccount branch October 22, 2019 18:52

xaur mentioned this pull request Nov 9, 2019

v1.5 rc1 Release Notes feedback decred/decred-binaries#119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow arbitrary xpub account imports. #1471

Allow arbitrary xpub account imports. #1471

jrick commented Jun 3, 2019

jrick commented Jun 3, 2019

jrick commented Jun 6, 2019

matheusd left a comment

jrick commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd commented Jun 6, 2019

jrick commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd left a comment

matheusd Aug 29, 2019

itswisdomagain Oct 8, 2019

itswisdomagain Oct 8, 2019 •

edited

Loading

itswisdomagain Oct 8, 2019

itswisdomagain Oct 8, 2019

itswisdomagain Oct 8, 2019

	return nil, errors.E(errors.Invalid, "imported address account has no extended pubkey")
	return nil, errors.E(errors.Invalid, "imported address account has no extended privkey")

	// There may not be an account by the same name
	// There may already be an account by the same name

Allow arbitrary xpub account imports. #1471

Allow arbitrary xpub account imports. #1471

Conversation

jrick commented Jun 3, 2019

jrick commented Jun 3, 2019

jrick commented Jun 6, 2019

matheusd left a comment

Choose a reason for hiding this comment

jrick commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd commented Jun 6, 2019

jrick commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd commented Jun 6, 2019

jrick commented Jun 6, 2019

matheusd left a comment

Choose a reason for hiding this comment

matheusd Aug 29, 2019

Choose a reason for hiding this comment

itswisdomagain Oct 8, 2019

Choose a reason for hiding this comment

itswisdomagain Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

itswisdomagain Oct 8, 2019

Choose a reason for hiding this comment

itswisdomagain Oct 8, 2019

Choose a reason for hiding this comment

itswisdomagain Oct 8, 2019

Choose a reason for hiding this comment

itswisdomagain Oct 8, 2019 •

edited

Loading