From 60fce250f41d5102063a31a497562b2631ee8469 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 30 Mar 2020 15:30:53 -0700 Subject: [PATCH 1/4] wallet: derive change addresses from the provided key scope Previously, the wallet would determine the key scope to use for change addresses by locating the one compatible with P2WPKH addresses, but this wasn't always safe like in the case when multiple key scopes that supported these addresses existed within the address manager, leading the change address to be created outside of the intended key scope. --- wallet/createtx.go | 16 +++++++++++----- wallet/wallet.go | 11 +++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/wallet/createtx.go b/wallet/createtx.go index 9fa0973..6bc5aae 100644 --- a/wallet/createtx.go +++ b/wallet/createtx.go @@ -135,15 +135,21 @@ func (w *Wallet) txToOutputs(outputs []*wire.TxOut, account uint32, inputSource := makeInputSource(eligible) changeSource := func() ([]byte, error) { - // Derive the change output script. As a hack to allow - // spending from the imported account, change addresses are - // created from account 0. + // Derive the change output script. We'll use the default key + // scope responsible for P2WPKH addresses to do so. As a hack to + // allow spending from the imported account, change addresses + // are created from account 0. var changeAddr btcutil.Address var err error + changeKeyScope := waddrmgr.KeyScopeBIP0084 if account == waddrmgr.ImportedAddrAccount { - changeAddr, err = w.newChangeAddress(addrmgrNs, 0) + changeAddr, err = w.newChangeAddress( + addrmgrNs, 0, changeKeyScope, + ) } else { - changeAddr, err = w.newChangeAddress(addrmgrNs, account) + changeAddr, err = w.newChangeAddress( + addrmgrNs, account, changeKeyScope, + ) } if err != nil { return nil, err diff --git a/wallet/wallet.go b/wallet/wallet.go index a3e092c..b760618 100644 --- a/wallet/wallet.go +++ b/wallet/wallet.go @@ -2946,7 +2946,7 @@ func (w *Wallet) NewChangeAddress(account uint32, err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error { addrmgrNs := tx.ReadWriteBucket(waddrmgrNamespaceKey) var err error - addr, err = w.newChangeAddress(addrmgrNs, account) + addr, err = w.newChangeAddress(addrmgrNs, account, scope) return err }) if err != nil { @@ -2968,14 +2968,9 @@ func (w *Wallet) NewChangeAddress(account uint32, // method in order to detect when an on-chain transaction pays to the address // being created. func (w *Wallet) newChangeAddress(addrmgrNs walletdb.ReadWriteBucket, - account uint32) (btcutil.Address, error) { + account uint32, scope waddrmgr.KeyScope) (btcutil.Address, error) { - // As we're making a change address, we'll fetch the type of manager - // that is able to make p2wkh output as they're the most efficient. - scopes := w.Manager.ScopesForExternalAddrType( - waddrmgr.WitnessPubKey, - ) - manager, err := w.Manager.FetchScopedKeyManager(scopes[0]) + manager, err := w.Manager.FetchScopedKeyManager(scope) if err != nil { return nil, err } From 43e19da86842b2d997f02f228a9bc02381968443 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 30 Mar 2020 15:31:44 -0700 Subject: [PATCH 2/4] Revert "waddrmgr+wallet: only watch addresses within default key scopes" The commit being reverted resulted in the discovery of a bug in which change addresses could at times be created outside of the default key scopes, causing us to not properly determine their spends. --- waddrmgr/manager.go | 23 ----------------------- wallet/wallet.go | 20 ++++++++------------ 2 files changed, 8 insertions(+), 35 deletions(-) diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index f2895ca..b94c6ee 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -756,29 +756,6 @@ func (m *Manager) ForEachAccountAddress(ns walletdb.ReadBucket, account uint32, return nil } -// ForEachDefaultScopeActiveAddress calls the given function with each active -// address stored in the manager within the default scopes, breaking early on -// error. -func (m *Manager) ForEachDefaultScopeActiveAddress(ns walletdb.ReadBucket, - fn func(addr btcutil.Address) error) error { - - m.mtx.RLock() - defer m.mtx.RUnlock() - - for _, keyScope := range DefaultKeyScopes { - scopedMgr, ok := m.scopedManagers[keyScope] - if !ok { - return fmt.Errorf("manager for default key scope with "+ - "purpose %v not found", keyScope.Purpose) - } - if err := scopedMgr.ForEachActiveAddress(ns, fn); err != nil { - return err - } - } - - return nil -} - // ChainParams returns the chain parameters for this address manager. func (m *Manager) ChainParams() *chaincfg.Params { // NOTE: No need for mutex here since the net field does not change diff --git a/wallet/wallet.go b/wallet/wallet.go index b760618..cdb1862 100644 --- a/wallet/wallet.go +++ b/wallet/wallet.go @@ -303,22 +303,18 @@ func (w *Wallet) SetChainSynced(synced bool) { w.chainClientSyncMtx.Unlock() } -// activeData returns the currently-active receiving addresses that exist within -// the wallet's default key scopes and all unspent outputs. This is primarily -// intended to provide the parameters for a rescan request. -func (w *Wallet) activeData(dbtx walletdb.ReadTx) ([]btcutil.Address, - []wtxmgr.Credit, error) { - +// activeData returns the currently-active receiving addresses and all unspent +// outputs. This is primarely intended to provide the parameters for a +// rescan request. +func (w *Wallet) activeData(dbtx walletdb.ReadTx) ([]btcutil.Address, []wtxmgr.Credit, error) { addrmgrNs := dbtx.ReadBucket(waddrmgrNamespaceKey) txmgrNs := dbtx.ReadBucket(wtxmgrNamespaceKey) var addrs []btcutil.Address - err := w.Manager.ForEachDefaultScopeActiveAddress( - addrmgrNs, func(addr btcutil.Address) error { - addrs = append(addrs, addr) - return nil - }, - ) + err := w.Manager.ForEachActiveAddress(addrmgrNs, func(addr btcutil.Address) error { + addrs = append(addrs, addr) + return nil + }) if err != nil { return nil, nil, err } From 12850499231eaf6c38c165ef2e3365d0a8a2a157 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 30 Mar 2020 15:32:34 -0700 Subject: [PATCH 3/4] wallet: include addresses from relevant key scopes in rescan Due to a no longer existing bug within the wallet, it was possible for change addresses to be created outside of their intended key scope (the default), so wallets affected by this now need to ensure they scan the chain for all addresses within the default key scopes (as expected), and all _internal_ addresses (branch used for change addresses) within any other registered key scopes to reflect their proper balance. --- waddrmgr/manager.go | 37 +++++++++++++++++++++++++++++++++++++ waddrmgr/scoped_manager.go | 27 +++++++++++++++++++++++++++ wallet/wallet.go | 10 ++++++---- 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index b94c6ee..8dc279a 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -738,6 +738,43 @@ func (m *Manager) ForEachActiveAddress(ns walletdb.ReadBucket, fn func(addr btcu return nil } +// ForEachRelevantActiveAddress invokes the given closure on each active +// address relevant to the wallet. Ideally, only addresses within the default +// key scopes would be relevant, but due to a bug (now fixed) in which change +// addresses could be created outside of the default key scopes, we now need to +// check for those as well. +func (m *Manager) ForEachRelevantActiveAddress(ns walletdb.ReadBucket, + fn func(addr btcutil.Address) error) error { + + m.mtx.RLock() + defer m.mtx.RUnlock() + + for _, scopedMgr := range m.scopedManagers { + // If the manager is for a default key scope, we'll return all + // addresses, otherwise we'll only return internal addresses, as + // that's the branch used for change addresses. + isDefaultKeyScope := false + for _, defaultKeyScope := range DefaultKeyScopes { + if scopedMgr.Scope() == defaultKeyScope { + isDefaultKeyScope = true + break + } + } + + var err error + if isDefaultKeyScope { + err = scopedMgr.ForEachActiveAddress(ns, fn) + } else { + err = scopedMgr.ForEachInternalActiveAddress(ns, fn) + } + if err != nil { + return err + } + } + + return nil +} + // ForEachAccountAddress calls the given function with each address of // the given account stored in the manager, breaking early on error. func (m *Manager) ForEachAccountAddress(ns walletdb.ReadBucket, account uint32, diff --git a/waddrmgr/scoped_manager.go b/waddrmgr/scoped_manager.go index b9acda3..16bd83a 100644 --- a/waddrmgr/scoped_manager.go +++ b/waddrmgr/scoped_manager.go @@ -1760,3 +1760,30 @@ func (s *ScopedKeyManager) ForEachActiveAddress(ns walletdb.ReadBucket, return nil } + +// ForEachInternalActiveAddress invokes the given closure on each _internal_ +// active address belonging to the scoped key manager, breaking early on error. +func (s *ScopedKeyManager) ForEachInternalActiveAddress(ns walletdb.ReadBucket, + fn func(addr btcutil.Address) error) error { + + s.mtx.Lock() + defer s.mtx.Unlock() + + addrFn := func(rowInterface interface{}) error { + managedAddr, err := s.rowInterfaceToManaged(ns, rowInterface) + if err != nil { + return err + } + // Skip any non-internal branch addresses. + if !managedAddr.Internal() { + return nil + } + return fn(managedAddr.Address()) + } + + if err := forEachActiveAddress(ns, &s.scope, addrFn); err != nil { + return maybeConvertDbError(err) + } + + return nil +} diff --git a/wallet/wallet.go b/wallet/wallet.go index cdb1862..3a8167b 100644 --- a/wallet/wallet.go +++ b/wallet/wallet.go @@ -311,10 +311,12 @@ func (w *Wallet) activeData(dbtx walletdb.ReadTx) ([]btcutil.Address, []wtxmgr.C txmgrNs := dbtx.ReadBucket(wtxmgrNamespaceKey) var addrs []btcutil.Address - err := w.Manager.ForEachActiveAddress(addrmgrNs, func(addr btcutil.Address) error { - addrs = append(addrs, addr) - return nil - }) + err := w.Manager.ForEachRelevantActiveAddress( + addrmgrNs, func(addr btcutil.Address) error { + addrs = append(addrs, addr) + return nil + }, + ) if err != nil { return nil, nil, err } From 31c027e19f1dc3c996e50d8b3d688f4a4a215278 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Mon, 30 Mar 2020 15:33:23 -0700 Subject: [PATCH 4/4] wallet: perform recovery on all registered key scopes In similar fashion to the previous commit, due to a no longer existing bug within the wallet, it was possible for change addresses to be created outside of their intended key scope (the default), so wallets affected by this now need to ensure upon recovery that they scan the chain for _all_ existing key scopes, rather than just the default ones, to reflect their proper balance. Through manual testing, it was shown that the impact of recovering the additional key scopes is negligible in most cases for both full nodes and light clients. --- wallet/wallet.go | 62 +++++++++++------------------------------------- 1 file changed, 14 insertions(+), 48 deletions(-) diff --git a/wallet/wallet.go b/wallet/wallet.go index 3a8167b..aaa9ff6 100644 --- a/wallet/wallet.go +++ b/wallet/wallet.go @@ -637,13 +637,15 @@ func (w *Wallet) recovery(chainClient chain.Interface, ) // In the event that this recovery is being resumed, we will need to - // repopulate all found addresses from the database. For basic recovery, - // we will only do so for the default scopes. - scopedMgrs, err := w.defaultScopeManagers() - if err != nil { - return err + // repopulate all found addresses from the database. Ideally, for basic + // recovery, we would only do so for the default scopes, but due to a + // bug in which the wallet would create change addresses outside of the + // default scopes, it's necessary to attempt all registered key scopes. + scopedMgrs := make(map[waddrmgr.KeyScope]*waddrmgr.ScopedKeyManager) + for _, scopedMgr := range w.Manager.ActiveScopedKeyManagers() { + scopedMgrs[scopedMgr.Scope()] = scopedMgr } - err = walletdb.View(w.db, func(tx walletdb.ReadTx) error { + err := walletdb.View(w.db, func(tx walletdb.ReadTx) error { txMgrNS := tx.ReadBucket(wtxmgrNamespaceKey) credits, err := w.TxStore.UnspentOutputs(txMgrNS) if err != nil { @@ -712,9 +714,9 @@ func (w *Wallet) recovery(chainClient chain.Interface, return err } } - return w.recoverDefaultScopes( + return w.recoverScopedAddresses( chainClient, tx, ns, recoveryBatch, - recoveryMgr.State(), + recoveryMgr.State(), scopedMgrs, ) }) if err != nil { @@ -737,48 +739,10 @@ func (w *Wallet) recovery(chainClient chain.Interface, return nil } -// defaultScopeManagers fetches the ScopedKeyManagers from the wallet using the -// default set of key scopes. -func (w *Wallet) defaultScopeManagers() ( - map[waddrmgr.KeyScope]*waddrmgr.ScopedKeyManager, error) { - - scopedMgrs := make(map[waddrmgr.KeyScope]*waddrmgr.ScopedKeyManager) - for _, scope := range waddrmgr.DefaultKeyScopes { - scopedMgr, err := w.Manager.FetchScopedKeyManager(scope) - if err != nil { - return nil, err - } - - scopedMgrs[scope] = scopedMgr - } - - return scopedMgrs, nil -} - -// recoverDefaultScopes attempts to recover any addresses belonging to any -// active scoped key managers known to the wallet. Recovery of each scope's -// default account will be done iteratively against the same batch of blocks. -// TODO(conner): parallelize/pipeline/cache intermediate network requests -func (w *Wallet) recoverDefaultScopes( - chainClient chain.Interface, - tx walletdb.ReadWriteTx, - ns walletdb.ReadWriteBucket, - batch []wtxmgr.BlockMeta, - recoveryState *RecoveryState) error { - - scopedMgrs, err := w.defaultScopeManagers() - if err != nil { - return err - } - - return w.recoverScopedAddresses( - chainClient, tx, ns, batch, recoveryState, scopedMgrs, - ) -} - -// recoverAccountAddresses scans a range of blocks in attempts to recover any +// recoverScopedAddresses scans a range of blocks in attempts to recover any // previously used addresses for a particular account derivation path. At a high // level, the algorithm works as follows: +// // 1) Ensure internal and external branch horizons are fully expanded. // 2) Filter the entire range of blocks, stopping if a non-zero number of // address are contained in a particular block. @@ -786,6 +750,8 @@ func (w *Wallet) recoverDefaultScopes( // 4) Record any outpoints found in the block that should be watched for spends // 5) Trim the range of blocks up to and including the one reporting the addrs. // 6) Repeat from (1) if there are still more blocks in the range. +// +// TODO(conner): parallelize/pipeline/cache intermediate network requests func (w *Wallet) recoverScopedAddresses( chainClient chain.Interface, tx walletdb.ReadWriteTx,