wallet: rescan and recover addresses in all relevant key scopes

waddrmgr/manager.go

@@ -738,16 +738,35 @@ func (m *Manager) ForEachActiveAddress(ns walletdb.ReadBucket, fn func(addr btcu
 	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,
-	fn func(maddr ManagedAddress) error) error {
+// 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 {
-		err := scopedMgr.ForEachAccountAddress(ns, account, fn)
+		// 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
 		}
@@ -756,22 +775,17 @@ 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 {
+// 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,
+	fn func(maddr ManagedAddress) 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 {
+	for _, scopedMgr := range m.scopedManagers {
+		err := scopedMgr.ForEachAccountAddress(ns, account, fn)
+		if err != nil {
 			return err
 		}
 	}

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
+}

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

wallet/wallet.go

@@ -303,17 +303,15 @@ 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(
+	err := w.Manager.ForEachRelevantActiveAddress(
 		addrmgrNs, func(addr btcutil.Address) error {
 			addrs = append(addrs, addr)
 			return nil
@@ -639,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 {
@@ -714,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 {
@@ -739,55 +739,19 @@ 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.
 //  3) Record all internal and external addresses found in the block.
 //  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,
@@ -2946,7 +2910,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 +2932,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
 	}