New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import external watch-only addresses #204

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@tuxcanfly
Contributor

tuxcanfly commented Mar 2, 2015

Added RPC commands to import external watch-only addresses both with and without pubkeys. WIP on finishing test coverage and docs.

Refs #192

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 3 times, most recently from 1a1eb59 to 7f91bd7 Mar 6, 2015

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from 7f91bd7 to cfbd1cb Mar 12, 2015

@@ -549,14 +581,14 @@ func putNumAccounts(tx walletdb.Tx, numAccounts uint32) error {
// the common parts.
func deserializeAddressRow(addressID, serializedAddress []byte) (*dbAddressRow, error) {
// The serialized address format is:
// <addrType><account><addedTime><syncStatus><rawdata>
// <addrType><account><addedTime><syncStatus><watchingOnly><rawdata>

This comment has been minimized.

@davecgh

davecgh Mar 19, 2015

Member

Why do we need a watching only flag as well as an adtWatching address type? It would be ideal to avoid changing this format because, if you do, you'll have to write upgrade code and modify every existing address in the database which I think we really don't want to do.

This comment has been minimized.

@davecgh

davecgh Mar 19, 2015

Member

After thinking about it some more, I think having the extra field is more flexible. However, at the moment there is some redundancy with the adtWatching type that doesn't quite feel right.

If we look at it from the perspective of what a user will need to do for watching-only purposes:

  • Import BIP0032 public key chains
  • Import "loose" public keys (those which are not a part of BIP0032 key chain)
  • Import the hash160 associated with a pay-to-script-hash

Considering that, the first case is an adtChain with a watching-only flag, the second is an adtImport with a watching-only flag, and the third is an adtScript with a watching-only flag.

This comment has been minimized.

@tuxcanfly

tuxcanfly Mar 23, 2015

Contributor

Updated to only use the watchingOnly flag and removed adtWatching type. Importing "loose" public keys and P2SH hashes works using the importpubkey and importaddress commands. Now working on importing public key chains.

Now working on importing BIP0032 public key chains.

// byteAsBool converts a byte to a bool.
func byteAsBool(b byte) bool {
return b != 0

This comment has been minimized.

@davecgh

davecgh Mar 19, 2015

Member

Not that I find 0 hard to read here, but just above you've used constants for trueByte and falseByte, so shouldn't this be consistent with b != falseByte?

case adtScript:
return deserializeScriptAddress(addressID, row)
case adtWatching:
return deserializeImportedHash160AddressRow(addressID, row)

This comment has been minimized.

@davecgh

davecgh Mar 19, 2015

Member

Given the above comments, this line will end up going away with the removal of the adtWatching type.

Of note, there is already code to deal with watching-only address conversion in deletePrivateKeys which is used when converting the entire manager to watching-only, so, outside of the addition of the watching-only flag to each address, there shouldn't be any need to modify the raw data serialization format for each address type at all.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from cfbd1cb to a04ef44 Mar 19, 2015

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from 3e647c4 to 7d2bf04 Mar 20, 2015

internal bool
compressed bool
used bool
publicAddress

This comment has been minimized.

@davecgh

davecgh Mar 23, 2015

Member

What is the purpose of splitting this out?

This comment has been minimized.

@tuxcanfly

tuxcanfly Mar 23, 2015

Contributor

I thought it might be logical to separate the structs for loose vs keychain addrs for future e.g if we wanted to add a method specific to loose addrs.

@@ -263,6 +272,21 @@ func (a *managedAddress) ExportPrivKey() (*btcutil.WIF, error) {
return btcutil.NewWIF(pk, a.manager.chainParams, a.compressed)
}
// newAddress returns a new address based on the passed account and address
// pubkeyhash.
func newAddress(m *Manager, account uint32, addressID []byte) (*publicAddress, error) {

This comment has been minimized.

@davecgh

davecgh Mar 23, 2015

Member

If the publicAddress bits aren't separated out (see previous comment), this can simply be:

newManagedPublicAddress(m *Manager, account uint32, addressID []byte) (*managedAddress, error)

This comment has been minimized.

@tuxcanfly

tuxcanfly Mar 23, 2015

Contributor

Thanks, I've reverted to managedAddress and updated the method name.

@davecgh

This comment has been minimized.

Member

davecgh commented Mar 23, 2015

Since the format of existing addresses in the db is being changed here, the db version needs to be bumped and it needs upgrade code to handle the new serialization. You should be able to do it without requiring the private passphrase by specifically targetting the new field offset.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 3 times, most recently to c634317 Mar 23, 2015

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented Mar 23, 2015

Yeah, we will need a upgrade to initialize the watchingOnly field, I guess it can be inferred by checking that pubkey is not nil.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 2 times, most recently from 3a3c1eb to b0e29ec Mar 23, 2015

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented Mar 24, 2015

@davecgh Added upgrade path, addressed items reviewed above. Tested that we support importing pubkeys, p2pkh and p2sh addresses now.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 3 times, most recently from 0d839f6 to 4f843cb Mar 24, 2015

@@ -902,35 +927,48 @@ func deserializeImportedAddress(row *dbAddressRow) (*dbImportedAddressRow, error
dbAddressRow: *row,
}
pubLen := binary.LittleEndian.Uint32(row.rawData[0:4])
pkHashLen := binary.LittleEndian.Uint32(row.rawData[0:4])

This comment has been minimized.

@davecgh

davecgh Mar 27, 2015

Member

Please stick with the existing offset approach. There is less chance of error when updating the code with that approach.

This comment has been minimized.

@jrick

jrick May 4, 2015

Member

Reminder to take care of this. I don't particularly care for offset variables, but to be consistent with the rest of the package, it should create the offset var first, and then slice between [offset:offset+fieldlen].

This comment has been minimized.

@davecgh

davecgh May 4, 2015

Member

They are superior to 4+4+8+len(thing)+x+y+len(z.foo). And then each field has to repeat that from the last, etc.

EDIT: The recent conversation from the ppcd folks having to deal with all those magic offsets everywhere is a prime example of how they help as well.

This comment has been minimized.

@jrick

jrick May 4, 2015

Member

With variable-sized data I agree

// This is required to import addresses without the associated public key.
func upgradeToVersion4(namespace walletdb.Namespace) error {
err := namespace.Update(func(tx walletdb.Tx) error {
currentMgrVersion := uint32(4)

This comment has been minimized.

@davecgh

davecgh Mar 27, 2015

Member

No need for this local. Just set it directly in the call to putManagerVersion below.

}
// Insert watchingOnly bool at byte 14
// Since importing watching-only addrs was added in this version we

This comment has been minimized.

@davecgh

davecgh Mar 27, 2015

Member

Minor nit: I've made it a point to avoid personal pronouns in the address manager. Please change this to:

            // Since importing watching-only addrs was not added
            // until this version, mark all existing addresses as
            // not being watching-only.
@@ -1361,7 +1402,7 @@ func deletePrivateKeys(tx walletdb.Tx) error {
// Reserialize the imported address without the private
// key and store it.
row.rawData = serializeImportedAddress(
irow.encryptedPubKey, nil)
irow.encryptedPubKeyHash, irow.encryptedPubKey, nil)

This comment has been minimized.

@davecgh

davecgh Mar 27, 2015

Member

Please respect column 80.

binary.LittleEndian.PutUint32(rawData[offset:offset+4], privLen)
offset += 4
copy(rawData[offset:offset+privLen], encryptedPrivKey)
rawData := make([]byte, 12+pkHashLen+pubLen+privLen)

This comment has been minimized.

@davecgh

davecgh Mar 27, 2015

Member

As above, please stick with the existing offset approach.

// Insert watchingOnly bool at byte 14
// Since importing watching-only addrs was added in this version we
// can safely initialize this to falseByte
x := make([]byte, len(v)+1)

This comment has been minimized.

@davecgh

davecgh Mar 27, 2015

Member

Please choose a better name than x.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 3 times, most recently from 8ea8bed to 8847744 May 1, 2015

@@ -62,11 +65,11 @@ type ManagedPubKeyAddress interface {
ManagedAddress
// PubKey returns the public key associated with the address.
PubKey() *btcec.PublicKey
PubKey() (*btcec.PublicKey, error)

This comment has been minimized.

@jrick

jrick May 1, 2015

Member

I think we should change the signature not to introduce an error (for the not-exists case) but to return the public key and a boolean for whether the pubkey is compressed or not. Since watching addresses can be imported without the pubkey, not having the pubkey exist is now a totally valid state, and the caller must determine whether not existing is an error or not.

For the not-exists case, we would either need another (a second) bool return, or return a nil pubkey. I'm leaning towards the additional return to avoid situations where the caller forgets to check for the nil case, since the Go type system isn't strong enough to enforce this.

This comment has been minimized.

@tuxcanfly

tuxcanfly May 4, 2015

Contributor

OK, we already have a db field called compressed on managedAddress which we are using for determining whether the pubkey is compressed or not. If we change the signature we might want to remove that field to avoid ambiguity.

Also, the PubKey signature is consistent with PrivKey which returns ErrWatchingOnly when private key is not available.

// ExportPubKey returns the public key associated with the address
// serialized as a hex encoded string.
ExportPubKey() string
ExportPubKey() (string, error)

This comment has been minimized.

@jrick

jrick May 1, 2015

Member

This may not be needed anymore with the PubKey method changes, since the caller would be able to create the exported key in compressed or uncompressed serialized formats themselves.

@@ -113,6 +116,10 @@ var _ ManagedPubKeyAddress = (*managedAddress)(nil)
// used by the caller without worrying about it being zeroed during an address
// lock.
func (a *managedAddress) unlock(key EncryptorDecryptor) ([]byte, error) {
if a.WatchingOnly() {
return nil, managerError(ErrWatchingOnly, errWatchingOnly, nil)

This comment has been minimized.

@jrick

jrick May 1, 2015

Member

Are there any locations where managed addresses are unlocked before being checked to be watching-only? If so, returning the error up the stack would be incorrect.

This comment has been minimized.

@tuxcanfly

tuxcanfly May 4, 2015

Contributor

Don't think so, I see unlock being called from scriptAddress.Script and managedAddress.PrivKey both of which start with the check if a.WatchingOnly() { return nil, managerError(ErrWatchingOnly, errWatchingOnly, nil) }

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from 8847744 to b854a06 May 2, 2015

}
var compressed bool
switch n := len(pubKeyBytes); n {

This comment has been minimized.

@jrick

jrick May 4, 2015

Member

I think this whole switch can be removed. The correct length is checked by ParsePubKey below. All you would need to pass to ImportPubKey is the bool len(pubKeyBytes) == btcec.PubKeyBytesLenCompressed.

I kind of think that the btcec type should save whether the pubkey was compressed or not, but oh well.

This comment has been minimized.

@tuxcanfly

tuxcanfly May 4, 2015

Contributor

Thanks, removed the swtich and simplified this.

case btcec.PubKeyBytesLenCompressed:
compressed = true
default:
e := errors.New("Invalid serialized pubkey length")

This comment has been minimized.

@jrick

jrick May 4, 2015

Member

This error will be going away with the rest of the switch statement, but fwiw Go errors should not be capitalized (golint will complain).

The rpc server should probably capitalize all of them at a higher level though, to match the style used by core. Not a huge deal, but some people might be annoyed by the inconsistencies.

This comment has been minimized.

@tuxcanfly

tuxcanfly May 11, 2015

Contributor

Thanks, noted.

// Make sure the public key is available, break out if it's not.
pubKey, err := ma.ExportPubKey()
if err != nil {
break

This comment has been minimized.

@jrick

jrick May 4, 2015

Member

If we are not changing the signature of ExportPubKey (or PubKey), then we must explicitly check that the error is the one we expect, and return all others. This is why I had mentioned returning an optional pubkey, but not erroring (because code like this ends up being written and real errors may be thrown away).

IMO, it is not an error for a managed address to not include the pubkey. That is simply one of several valid states. It is the caller's responsibility to determine whether this is an error condition or not.

pubLen := binary.LittleEndian.Uint32(row.rawData[0:4])
pkHashLen := binary.LittleEndian.Uint32(row.rawData[0:4])
retRow.encryptedPubKeyHash = make([]byte, pkHashLen)
copy(retRow.encryptedPubKeyHash, row.rawData[4:4+pkHashLen])

This comment has been minimized.

@jrick

jrick May 4, 2015

Member

For the record, this is not correctly bounds-checked, and may panic on a bad value. These programming errors are all throughout this package though; we'll fix them up later.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 2 times, most recently from 1019144 to 7b3e16f May 4, 2015

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented May 11, 2015

OK, updated PubKey to return compressed as well as ok. Refactored related methods accordingly.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch 2 times, most recently from c1be9db to e670502 May 11, 2015

@jrick

This comment has been minimized.

Member

jrick commented May 14, 2015

The changed methods need better documentation to describe what all the return values are. Either the interface definition can use named returns to document the values, or comments should be added to describe what each bool means.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from e670502 to 057597f May 15, 2015

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented May 15, 2015

Thanks, using named returns in the interface and also updated docs about the return values.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from 7a447bd to c8edcc6 May 26, 2015

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from c8edcc6 to 3d6cc83 Aug 24, 2015

tuxcanfly added some commits Jan 27, 2015

Updated ImportAddress in accordance with 6ee1f9b
In 6ee1f9b, Manager.Address was updated to convert PK addr to PKH addr
so that the db is always keyed by script hash. This commit fixes
ImportAddress to use the same key while fetching the addr from the db.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:import-address branch from ba7c06e to 1444b51 Nov 13, 2015

@csmcanarney

This comment has been minimized.

csmcanarney commented Aug 27, 2016

What's the status on this? The corresponding PRs btcsuite/btcd#279 and btcsuite/btcjson#41 were merged a long time ago.

Did #192 "supersede" this PR?

@jrick

This comment has been minimized.

Member

jrick commented Aug 29, 2016

There are several conflicts preventing this from being merged. There will be even more conflicts if decred#315 is backported to btcwallet.

We're also moving away from the idea of importing keys into a HD wallet, both to simplify our code and to make the entire wallet recoverable from just the seed. Would need a discussion about if we still even want this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment