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

Account name #155

Merged
merged 1 commit into from Mar 19, 2015

Conversation

Projects
None yet
5 participants
@tuxcanfly
Contributor

tuxcanfly commented Dec 17, 2014

NOTE: This pull request requires #147

Updated waddrmgr to support account names and integrated with wallet RPC calls.

@davecgh

This comment has been minimized.

Member

davecgh commented Dec 17, 2014

Thanks @tuxcanfly. I'll go through and review this as time permits. It's obviously a huge change.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch 3 times, most recently from 0722d9c to a12fd58 Dec 18, 2014

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch from a12fd58 to 53ea9c5 Dec 27, 2014

// The returned value is a slice of account numbers which can be used to load
// the respective account rows.
func fetchAllAccounts(tx walletdb.Tx) ([]uint32, error) {
accounts := []uint32{}

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

Please add a TODO here to load the number of accounts from the db first and create the slice with enough space for them all at once. Otherwise, it's hard on the GC because it has to grow the backing array multiple times.

Also, super nit picky, but every other function has the bucket := ... line as the first one in the function, so I'd prefer that be consistent.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

I'm now using a 'meta' bucket to store metadata like no of accounts, no of addrs etc and using it to create sized slices.

Moved bucket := ... to the first line.

offset := uint32(0)
var acctName string
bucket.ForEach(func(k, v []byte) error {

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

Hmmm. Any reason this is iterating instead of simply looking up the account number by key? I realize currently there are only names in the key, but I suspect it would be better to store both in the index. That is to say the index bucket would have both:

bucket[acctName] = acctNum
bucket[acctNum] = acctName

EDIT: One thing to keep in mind if this is done is that an empty account name would be stored as [0 0 0 0], the same as account number 0, so it would need some type of special handling probably.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

I've added a new bucket to store in the index mapping account name to it's id.

}
// Write a null value keyed by the address hash
err = bucket.Put(addrHash, []byte{0})

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

I'd make a global const (will have to be a var actually, but treat it like a const) for this byte slice so it doesn't have to generate a new instance that escapes to the heap every time thereby generating extra garbage for the GC to clean up after.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Added a var called nullVal and using it as a global.

// The returned value is a slice address rows for each specific address type.
// The caller should use type assertions to ascertain the types.
func fetchAccountAddresses(tx walletdb.Tx, account uint32) ([]interface{}, error) {
var addrs []interface{}

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

The same as previously mentioned here. Please avoid using an unsized slice that will require the backing array to be reallocated and copied multiple times.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Yup, doing this now.

@davecgh

This comment has been minimized.

Member

davecgh commented Dec 31, 2014

I'm still reviewing, but one thing in general I want to bring up that we should discuss is how to avoid loading all of the addresses into a slice at once. That will not scale to millions/billions of addresses.

@@ -1657,6 +1861,19 @@ func checkBranchKeys(acctKey *hdkeychain.ExtendedKey) error {
return err
}
// ValidateAccountName validates the given account name and returns an error, if any.
func ValidateAccountName(name string) error {

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

I know that Go doesn't really care where the functions are placed, but the rest of code base strives to define functions before they are used. Please move this before NewAccount which is the first (and only) thing that uses it.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Done.

// ErrDuplicateAccount will be returned. Since creating a new account requires
// access to the cointype keys (from which extended account keys are derived),
// it requires the manager to be unlocked.
func (m *Manager) NewAccount(name string) (uint32, error) {

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

Since the manager has to be unlocked, this needs checks to return the appropriate errors when it's not. Also, this can't be done when the manager is watching only, so that needs an error too.

if m.watchingOnly {
    return managerError(ErrWatchingOnly, errWatchingOnly, nil)
}

m.mtx.Lock()
defer m.mtx.Unlock()

if m.locked {
    return 0, managerError(ErrLocked, errLocked, nil)
}

It's ok to access the watching only flag when not under the mutex because it can't be changed after creation, but the m.locked field requires the mutex.

This also obviously means the later calls to the exported methods which grab the mutex need unexported versions that are invoked with the mutex held. Even more importantly, the mutex needs to be held for the crypto operations later too so the manager is not unlocked out from under this code after the initial check.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Yup, adding the mutex lock and checks as described. We were calling LookupAccount with the mutex lock held so I've added a unexpected version lookupAccount to make it usable.

// RenameAccount renames an account stored in the manager based on the
// given account number with the given name. If an account with the same name
// already exists, ErrDuplicateAccount will be returned.
func (m *Manager) RenameAccount(account uint32, name string) error {

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

I believe all of these new exported functions generally need the manager mutex held for the duration of the function since otherwise you're opening up races in between write/reads from the database.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Checked and added mutex lock to newly exported funcs.

if err != nil {
return err
}
// TODO: needs to be atomic?

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

No. It's a local variable to the closure.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

OK, removed the TODO.

}
// Derive the account key using the cointype key
acctKeyPriv, err := deriveAccountKey(coinTypeKeyPriv, account)

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

There error isn't being checked here.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Oops... added the check now.

This comment has been minimized.

@davecgh

davecgh Jan 12, 2015

Member

The coin type extended key bytes need to be zeroed as soon as they are used.

coinTypeKeyPriv.Zero()

NOTE: This is intentionally before the error check so it happens regardless.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 12, 2015

Contributor

Zeroing the coinTypeKeyPriv now.

str := "failed to encrypt private key for account"
return managerError(ErrCrypto, str, err)
}
// We have the encrypted account extended keys, so save them to the database

This comment has been minimized.

@davecgh

davecgh Dec 31, 2014

Member

This looks odd as the comment isn't wrapped to 80 while the function is wrapped too early.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 2, 2015

Contributor

Re-formatted both to wrap properly.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch 10 times, most recently from 7da37c9 to 5d7176a Jan 1, 2015

@@ -862,14 +1225,108 @@ func existsAddress(tx walletdb.Tx, addressID []byte) bool {
return bucket.Get(addrHash[:]) != nil
}
// fetchAddrAccount returns the account to which the given address belongs to.
// It looks up the account using the addracctidx index which maps the address
// hash to it's corresponding account id.

This comment has been minimized.

@davecgh

davecgh Jan 12, 2015

Member

it's -> its

// fetchAccountAddresses loads information about addresses of an account from the database.
// The returned value is a slice address rows for each specific address type.
// The caller should use type assertions to ascertain the types.
func fetchAccountAddresses(tx walletdb.Tx, account uint32) ([]interface{}, error) {

This comment has been minimized.

@davecgh

davecgh Jan 12, 2015

Member

I'd rather see this function make use of fetchAddress than repeating the same code which would require updating it in two places when a new address type is added. I understand that fetchAddress expects the address whereas the account index stores the hash of the address (which is absolutely correct to do).

I'd suggest simply refactoring the common code into a separate function that both call with the hash of the address so that fetchAddress can take the hash of the passed address, while this function looks up the already hashed address and can pass it directly.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 12, 2015

Contributor

Added a func fetchAddressByHash which can be re-used from fetchAddress as well fetchAccountAddresses, fetchAllAddresses One problem here is we are expecting addressID in deserializeAddressRow but we don't have it when querying by addrHash. Using the addrHash as a placeholder for now.

This comment has been minimized.

@davecgh

davecgh Jan 12, 2015

Member

Fair point. It's only being used for the error message though. How about just removing that parameter from the function calls and error messages, and then, at the two call sites for fetchAddress:

if err != nil {
    if merr, ok := err.(*ManagerError); ok {
        desc := fmt.Sprintf("failed to fetch address '%s': %v",
            address.ScriptAddress(), merr.Description)
        merr.Description = desc
    }
}

For the call site that is querying by addrHash just change the message to failed to fetch address hash '%s': %v.

Then add a comment on the errors in the functions that they are later prefixed with the address or hash that caused the failure.

This comment has been minimized.

@tuxcanfly

tuxcanfly Jan 13, 2015

Contributor

Added modified err in callers for both fetchAddress and fetchAddressByHash and updated comments accordingly.

return []interface{}{}, nil
}
numAccountAddrs, err := fetchNumAccountAddrs(tx, account)

This comment has been minimized.

@davecgh

davecgh Jan 12, 2015

Member

Ignoring the error doesn't seem like proper behavior here. The called function will return zero if the bucket has not yet been initialized, so any error that occurs here is unexpected and should be handled appropriately.

Further, I think that when the bucket is created, all count fields should be initialized to zero at that time, and the fetchNumAccountAddrs should then treat the missing field as an error. Otherwise, it easily allows unexpected logic to creep in, since you're essentially hiding what should be an error condition by returning zero.

EDIT: Oh I missed you're returning the error. Please do return nil, err.

@@ -38,6 +39,8 @@ var (
latestMgrVersion uint32 = LatestMgrVersion
)
type obtainFunc func() ([]byte, error)

This comment has been minimized.

@davecgh

davecgh Mar 17, 2015

Member

This should be exported since it's used in the Confg struct. It'll need a comment too once exported.

@@ -1334,14 +1663,12 @@ func upgradeToVersion2(namespace walletdb.Namespace) error {
// upgradeManager upgrades the data in the provided manager namespace to newer
// versions as neeeded.
func upgradeManager(namespace walletdb.Namespace) error {
// Get the current version.
func upgradeManager(namespace walletdb.Namespace, pubPassPhrase []byte, obtainSeed, obtainPrivPassPhrase obtainFunc) error {

This comment has been minimized.

@davecgh

davecgh Mar 17, 2015

Member

As noted below, I suggest changing the signature to accept the entire config struct so it won't have to be changed in the future if any new information is needed.

@@ -1858,7 +2170,7 @@ func Open(namespace walletdb.Namespace, pubPassphrase []byte, chainParams *chain
}
// Upgrade the manager to the latest version as needed.
if err := upgradeManager(namespace); err != nil {
if err := upgradeManager(namespace, pubPassphrase, config.ObtainSeed, config.ObtainPrivatePass); err != nil {

This comment has been minimized.

@davecgh

davecgh Mar 17, 2015

Member

This would panic if a nil config (which is valid) is passed. I'd move the section below which sets the default config above this line and change the upgradeManager signature to take the entire config struct pointer instead of the individual entries.

ScryptN int
ScryptR int
ScryptP int
ObtainSeed obtainFunc

This comment has been minimized.

@davecgh

davecgh Mar 17, 2015

Member

Please add a comment for this field's purpose.

    // ObtainSeed is a callback function that is potentially invoked during
    // upgrades.  It is intended to be used to request the wallet seed
    // from the user (or any other mechanism the caller deems fit).
ScryptR int
ScryptP int
ObtainSeed obtainFunc
ObtainPrivatePass obtainFunc

This comment has been minimized.

@davecgh

davecgh Mar 17, 2015

Member

Please add a comment for this field's purpose.

    // ObtainPrivatePass is a callback function that is potentially invoked
    // during upgrades.  It is intended to be used to request the wallet
    // private passphrase from the user (or any other mechanism the caller
    // deems fit).

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch 4 times, most recently from 4c34e2d to fbd7aea Mar 17, 2015

@jrick

This comment has been minimized.

Member

jrick commented Mar 17, 2015

I think that the account names should not be set in the RPC handlers for gettransaction and listtransactions. These fields are deprecated in recent bitcoind wallet versions (which makes sense, since account balances there can change without an on-chain transaction) and we have plans for a proper ledger RPC to report credits and debits across accounts that is intentionally incompatible with bitcoind.

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented Mar 17, 2015

@jrick Agreed, that would simplify some hoops I had to jump through to make the RPC results compat with core.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch from c0911e0 to dd835b4 Mar 17, 2015

wallet.go Outdated
"github.com/btcsuite/btcwallet/chain"
"github.com/btcsuite/btcwallet/txstore"
"github.com/btcsuite/btcwallet/waddrmgr"
"github.com/btcsuite/btcwallet/walletdb"
"golang.org/x/crypto/ssh/terminal"

This comment has been minimized.

@jrick

jrick Mar 17, 2015

Member

Use "github.com/btcsuite/golangcrypto/ssh/terminal".

return []btcjson.Cmd{n}
}
func (d txDebit) notificationCmds(w *Wallet) []btcjson.Cmd {
blk := w.Manager.SyncedTo()
ltrs, err := txstore.Debits(d).ToJSON("", blk.Height, activeNet.Params)
acctName := waddrmgr.DefaultAccountName
if debitAccount, err := w.DebitAccount(txstore.Debits(d)); err == nil {

This comment has been minimized.

@jrick

jrick Mar 17, 2015

Member

I would also just use "" here (this is an old api and will eventually be replaced) instead of looking up the account. This is the last call to DebitAccount, so with this gone, you can remove that func as well.

wallet.go Outdated
// a given account. It returns true if atleast one address in the account was used
// and false if no address in the account was used.
func (w *Wallet) AccountUsed(account uint32) (bool, error) {
addrs, err := w.Manager.ActiveAccountAddresses(account)

This comment has been minimized.

@jrick

jrick Mar 17, 2015

Member

This is an assumption because currently ActiveAccountAddresses returns all addresses from the account, but may not in the future. If it is changed (when single use addrs are added) to only return unused addresses, the used account may be marked unused.

wallet.go Outdated
// the number of tx confirmations.
blk := w.Manager.SyncedTo()
for _, r := range w.TxStore.Records() {

This comment has been minimized.

@jrick

jrick Mar 17, 2015

Member

This should only be checking unspent outputs, not every record.

wallet.go Outdated
@@ -1179,6 +1323,22 @@ func (w *Wallet) ResendUnminedTxs() {
}
}
// SortedActiveAccountAddresses returns a slice of addresses of an account in a wallet.
func (w *Wallet) SortedActiveAccountAddresses(account uint32) ([]string, error) {

This comment has been minimized.

@jrick

jrick Mar 17, 2015

Member

I would just remove this. There's no reason to need addresses to be sorted. I think this was previously added to mimic an implementation detail in bitcoind (a std::map is usually implemented as an RB tree, so iterating over k/v pairs has a defined order). Instead, I would change GetAddressesByAccount to return a JSON array of all addresses (even those not marked active) in any order.

wallet.go Outdated
// TotalReceivedForAccount iterates through a wallet's transaction history,
// returning the total amount of bitcoins received for a single wallet
// account.
func (w *Wallet) TotalReceivedForAccount(account uint32, confirms int) (btcutil.Amount, uint64, error) {

This comment has been minimized.

@jrick

jrick Mar 17, 2015

Member

The previous function which did this and didn't take the account (TotalReceived) is no longer used anywhere and should be removed.

This comment has been minimized.

@tuxcanfly

tuxcanfly Mar 18, 2015

Contributor

We need this for getreceivedbyaccount, listreceivedbyaccount

This comment has been minimized.

@jrick

jrick Mar 18, 2015

Member

Right, we need TotalReceivedForAccount, but TotalReceived is no longer used and is dead code.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch from dd835b4 to 48371c9 Mar 18, 2015

}
err := checkAccountName(account)
err := checkAccountName(*cmd.Account)

This comment has been minimized.

@jrick

jrick Mar 18, 2015

Member

This is not safe. The field is optional and this may be nil.

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented Mar 18, 2015

@jrick Thanks, updated.

@tuxcanfly tuxcanfly closed this Mar 18, 2015

@tuxcanfly tuxcanfly reopened this Mar 18, 2015

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch from 088ac1e to 15b1b75 Mar 18, 2015

@davecgh

This comment has been minimized.

Member

davecgh commented Mar 18, 2015

Alright, this looks good to go! Please squash it down to a single commit and rebase to the latest master.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:account-name branch from 15b1b75 to 68a9168 Mar 19, 2015

@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented Mar 19, 2015

Done.

@conformal-deploy conformal-deploy merged commit 68a9168 into btcsuite:master Mar 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tuxcanfly

This comment has been minimized.

Contributor

tuxcanfly commented Mar 19, 2015

Great! Thanks for the patient reviews @davecgh @jrick

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