waddrmgr: fix data race between Manager lock/unlock and ScopedKeyManager

Manager.lock() and Manager.Unlock() directly accessed ScopedKeyManager
fields (acctInfo, addrs, deriveOnUnlock) while holding only m.mtx.
ScopedKeyManager methods accessed those same fields while holding only
s.mtx. Since these are different mutexes, there was no mutual exclusion
on the shared fields.

The fix moves responsibility for ScopedKeyManager's field lifecycle
into ScopedKeyManager itself via Lock(), Unlock(), Close(), and
ConvertToWatchingOnly() methods. Each acquires s.mtx internally,
ensuring all access to the shared fields is consistently protected
by s.mtx. Manager delegates to these methods instead of manipulating
fields directly.

A KeyManagerRoot interface replaces the *Manager back-reference in
ScopedKeyManager, preventing it from directly accessing Manager's
mutex or internal fields.

syncState is moved to a dedicated syncStateMtx, and
FetchScopedKeyManager is made lock-free (the scopedManagers map is
stable after wallet initialization), removing all s.mtx -> m.mtx
lock ordering violations.

Author: Aharonee

waddrmgr/address.go

@@ -401,7 +401,7 @@ func (a *managedAddress) PrivKey() (*btcec.PrivateKey, error) {
 	// Decrypt the key as needed.  Also, make sure it's a copy since the
 	// private key stored in memory can be cleared at any time.  Otherwise
 	// the returned private key could be invalidated from under the caller.
-	privKeyCopy, err := a.unlock(a.manager.rootManager.cryptoKeyPriv)
+	privKeyCopy, err := a.unlock(a.manager.rootManager.CryptoKeyPriv())
 	if err != nil {
 		return nil, err
 	}
@@ -421,7 +421,7 @@ func (a *managedAddress) ExportPrivKey() (*btcutil.WIF, error) {
 		return nil, err
 	}
 
-	return btcutil.NewWIF(pk, a.manager.rootManager.chainParams, a.compressed)
+	return btcutil.NewWIF(pk, a.manager.rootManager.ChainParams(), a.compressed)
 }
 
 // DerivationInfo contains the information required to derive the key that
@@ -559,7 +559,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager,
 
 		// First, we'll generate a normal p2wkh address from the pubkey hash.
 		witAddr, err := btcutil.NewAddressWitnessPubKeyHash(
-			pubKeyHash, m.rootManager.chainParams,
+			pubKeyHash, m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err
@@ -577,23 +577,23 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager,
 		// witnessProgram as the sigScript, then present the proper
 		// <sig, pubkey> pair as the witness.
 		address, err = btcutil.NewAddressScriptHash(
-			witnessProgram, m.rootManager.chainParams,
+			witnessProgram, m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err
 		}
 
 	case PubKeyHash:
 		address, err = btcutil.NewAddressPubKeyHash(
-			pubKeyHash, m.rootManager.chainParams,
+			pubKeyHash, m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err
 		}
 
 	case WitnessPubKey:
 		address, err = btcutil.NewAddressWitnessPubKeyHash(
-			pubKeyHash, m.rootManager.chainParams,
+			pubKeyHash, m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err
@@ -602,7 +602,7 @@ func newManagedAddressWithoutPrivKey(m *ScopedKeyManager,
 	case TaprootPubKey:
 		tapKey := txscript.ComputeTaprootKeyNoScript(pubKey)
 		address, err = btcutil.NewAddressTaproot(
-			schnorr.SerializePubKey(tapKey), m.rootManager.chainParams,
+			schnorr.SerializePubKey(tapKey), m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err
@@ -635,7 +635,7 @@ func newManagedAddress(s *ScopedKeyManager, derivationPath DerivationPath,
 	// NOTE: The privKeyBytes here are set into the managed address which
 	// are cleared when locked, so they aren't cleared here.
 	privKeyBytes := privKey.Serialize()
-	privKeyEncrypted, err := s.rootManager.cryptoKeyPriv.Encrypt(privKeyBytes)
+	privKeyEncrypted, err := s.rootManager.CryptoKeyPriv().Encrypt(privKeyBytes)
 	if err != nil {
 		str := "failed to encrypt private key"
 		return nil, managerError(ErrCrypto, str, err)
@@ -894,7 +894,7 @@ func (a *scriptAddress) Script() ([]byte, error) {
 	// Decrypt the script as needed.  Also, make sure it's a copy since the
 	// script stored in memory can be cleared at any time.  Otherwise,
 	// the returned script could be invalidated from under the caller.
-	return a.unlock(a.manager.rootManager.cryptoKeyScript)
+	return a.unlock(a.manager.rootManager.CryptoKeyScript())
 }
 
 // Enforce scriptAddress satisfies the ManagedScriptAddress interface.
@@ -905,7 +905,7 @@ func newScriptAddress(m *ScopedKeyManager, account uint32, scriptHash,
 	scriptEncrypted []byte) (*scriptAddress, error) {
 
 	address, err := btcutil.NewAddressScriptHashFromHash(
-		scriptHash, m.rootManager.chainParams,
+		scriptHash, m.rootManager.ChainParams(),
 	)
 	if err != nil {
 		return nil, err
@@ -989,9 +989,9 @@ func (a *witnessScriptAddress) Script() ([]byte, error) {
 		return nil, managerError(ErrLocked, errLocked, nil)
 	}
 
-	cryptoKey := a.manager.rootManager.cryptoKeyScript
+	cryptoKey := a.manager.rootManager.CryptoKeyScript()
 	if !a.isSecretScript {
-		cryptoKey = a.manager.rootManager.cryptoKeyPub
+		cryptoKey = a.manager.rootManager.CryptoKeyPub()
 	}
 
 	// Decrypt the script as needed. Also, make sure it's a copy since the
@@ -1012,7 +1012,7 @@ func newWitnessScriptAddress(m *ScopedKeyManager, account uint32, scriptIdent,
 	switch witnessVersion {
 	case witnessVersionV0:
 		address, err := btcutil.NewAddressWitnessScriptHash(
-			scriptIdent, m.rootManager.chainParams,
+			scriptIdent, m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err
@@ -1031,7 +1031,7 @@ func newWitnessScriptAddress(m *ScopedKeyManager, account uint32, scriptIdent,
 
 	case witnessVersionV1:
 		address, err := btcutil.NewAddressTaproot(
-			scriptIdent, m.rootManager.chainParams,
+			scriptIdent, m.rootManager.ChainParams(),
 		)
 		if err != nil {
 			return nil, err

waddrmgr/manager.go

@@ -345,6 +345,7 @@ type Manager struct {
 	externalAddrSchemas map[AddressType][]KeyScope
 	internalAddrSchemas map[AddressType][]KeyScope
 
+	syncStateMtx sync.RWMutex
 	syncState    syncState
 	watchingOnly atomic.Bool
 	birthday     time.Time
@@ -426,42 +427,25 @@ func (m *Manager) IsWatchOnlyAccount(ns walletdb.ReadBucket, keyScope KeyScope,
 //
 // TODO(yy): Rename this to wipe memory for priv keys.
 func (m *Manager) lock() {
-	for _, manager := range m.scopedManagers {
-		// Clear all of the account private keys.
-		for _, acctInfo := range manager.acctInfo {
-			if acctInfo.acctKeyPriv != nil {
-				acctInfo.acctKeyPriv.Zero()
-			}
-			acctInfo.acctKeyPriv = nil
-		}
-	}
+	// Mark locked first so callers checking IsLocked() see the locked
+	// state before we begin zeroing keys.
+	m.locked.Store(true)
 
-	// Remove clear text private keys and scripts from all address entries.
-	for _, manager := range m.scopedManagers {
-		for _, ma := range manager.addrs {
-			switch addr := ma.(type) {
-			case *managedAddress:
-				addr.lock()
-			case *scriptAddress:
-				addr.lock()
-			}
-		}
+	// Zero private key material in each scoped manager under s.mtx.
+	for _, sm := range m.scopedManagers {
+		sm.Lock()
 	}
 
-	// Remove clear text private master and crypto keys from memory.
+	// Zero manager-level crypto keys.
 	m.cryptoKeyScript.Zero()
 	m.cryptoKeyPriv.Zero()
 	m.masterKeyPriv.Zero()
-
-	// Zero the hashed passphrase.
 	zero.Bytea64(&m.hashedPrivPassphrase)
 
 	// NOTE: m.cryptoKeyPub is intentionally not cleared here as the address
 	// manager needs to be able to continue to read and decrypt public data
 	// which uses a separate derived key from the database even when it is
 	// locked.
-
-	m.locked.Store(true)
 }
 
 // Close cleanly shuts down the manager.  It makes a best try effort to remove
@@ -878,6 +862,42 @@ func (m *Manager) ChainParams() *chaincfg.Params {
 	return m.chainParams
 }
 
+// CryptoKeyPub returns the public crypto key used to encrypt/decrypt public
+// extended keys and addresses.
+func (m *Manager) CryptoKeyPub() EncryptorDecryptor {
+	return m.cryptoKeyPub
+}
+
+// CryptoKeyPriv returns the private crypto key used to encrypt/decrypt private
+// key material. The underlying key is zeroed when the manager is locked.
+func (m *Manager) CryptoKeyPriv() EncryptorDecryptor {
+	return m.cryptoKeyPriv
+}
+
+// CryptoKeyScript returns the script crypto key used to encrypt/decrypt script
+// data. The underlying key is zeroed when the manager is locked.
+func (m *Manager) CryptoKeyScript() EncryptorDecryptor {
+	return m.cryptoKeyScript
+}
+
+// SetStartBlock updates the start block if the given block is earlier than the
+// current start block. It persists the change to the database and updates
+// the in-memory state atomically.
+func (m *Manager) SetStartBlock(ns walletdb.ReadWriteBucket,
+	bs *BlockStamp) error {
+
+	m.syncStateMtx.Lock()
+	defer m.syncStateMtx.Unlock()
+
+	if bs.Height < m.syncState.startBlock.Height {
+		if err := putStartBlock(ns, bs); err != nil {
+			return err
+		}
+		m.syncState.startBlock = *bs
+	}
+	return nil
+}
+
 // ChangePassphrase changes either the public or private passphrase to the
 // provided value depending on the private flag.  In order to change the
 // private password, the address manager must not be watching-only.  The new
@@ -1076,32 +1096,9 @@ func (m *Manager) ConvertToWatchingOnly(ns walletdb.ReadWriteBucket) error {
 		m.lock()
 	}
 
-	// This section clears and removes the encrypted private key material
-	// that is ordinarily used to unlock the manager.  Since the the manager
-	// is being converted to watching-only, the encrypted private key
-	// material is no longer needed.
-
-	// Clear and remove all of the encrypted acount private keys.
+	// Clear encrypted private key material from each scoped manager.
 	for _, manager := range m.scopedManagers {
-		for _, acctInfo := range manager.acctInfo {
-			zero.Bytes(acctInfo.acctKeyEncrypted)
-			acctInfo.acctKeyEncrypted = nil
-		}
-	}
-
-	// Clear and remove encrypted private keys and encrypted scripts from
-	// all address entries.
-	for _, manager := range m.scopedManagers {
-		for _, ma := range manager.addrs {
-			switch addr := ma.(type) {
-			case *managedAddress:
-				zero.Bytes(addr.privKeyEncrypted)
-				addr.privKeyEncrypted = nil
-			case *scriptAddress:
-				zero.Bytes(addr.scriptEncrypted)
-				addr.scriptEncrypted = nil
-			}
-		}
+		manager.ConvertToWatchingOnly()
 	}
 
 	// Clear and remove encrypted private and script crypto keys.
@@ -1210,64 +1207,9 @@ func (m *Manager) Unlock(ns walletdb.ReadBucket, passphrase []byte) error {
 	// Use the crypto private key to decrypt all of the account private
 	// extended keys.
 	for _, manager := range m.scopedManagers {
-		for account, acctInfo := range manager.acctInfo {
-			decrypted, err := m.cryptoKeyPriv.Decrypt(acctInfo.acctKeyEncrypted)
-			if err != nil {
-				m.lock()
-				str := fmt.Sprintf("failed to decrypt account %d "+
-					"private key", account)
-				return managerError(ErrCrypto, str, err)
-			}
-
-			acctKeyPriv, err := hdkeychain.NewKeyFromString(string(decrypted))
-			zero.Bytes(decrypted)
-			if err != nil {
-				m.lock()
-				str := fmt.Sprintf("failed to regenerate account %d "+
-					"extended key", account)
-				return managerError(ErrKeyChain, str, err)
-			}
-			acctInfo.acctKeyPriv = acctKeyPriv
-		}
-
-		// We'll also derive any private keys that are pending due to
-		// them being created while the address manager was locked.
-		for _, info := range manager.deriveOnUnlock {
-			addressKey, _, _, err := manager.deriveKeyFromPath(
-				ns, info.managedAddr.InternalAccount(),
-				info.branch, info.index, true,
-			)
-			if err != nil {
-				m.lock()
-				return err
-			}
-
-			// It's ok to ignore the error here since it can only
-			// fail if the extended key is not private, however it
-			// was just derived as a private key.
-			privKey, _ := addressKey.ECPrivKey()
-			addressKey.Zero()
-
-			privKeyBytes := privKey.Serialize()
-			privKeyEncrypted, err := m.cryptoKeyPriv.Encrypt(privKeyBytes)
-			privKey.Zero()
-			if err != nil {
-				m.lock()
-				str := fmt.Sprintf("failed to encrypt private key for "+
-					"address %s", info.managedAddr.Address())
-				return managerError(ErrCrypto, str, err)
-			}
-
-			switch a := info.managedAddr.(type) {
-			case *managedAddress:
-				a.privKeyEncrypted = privKeyEncrypted
-				a.privKeyCT = privKeyBytes
-			case *scriptAddress:
-			}
-
-			// Avoid re-deriving this key on subsequent unlocks.
-			manager.deriveOnUnlock[0] = nil
-			manager.deriveOnUnlock = manager.deriveOnUnlock[1:]
+		if err := manager.Unlock(m.cryptoKeyPriv, ns); err != nil {
+			m.lock()
+			return err
 		}
 	}
 

waddrmgr/manager_test.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"sync"
 	"testing"
 	"time"
 
@@ -3294,3 +3295,51 @@ func TestManagedAddressValidation(t *testing.T) {
 	}
 
 }
+
+// TestLockUnlockRace verifies there is no data race between Manager.Lock()
+// and concurrent ScopedKeyManager.NextExternalAddresses calls.
+func TestLockUnlockRace(t *testing.T) {
+	teardown, db, mgr := setupManager(t)
+	defer teardown()
+
+	err := walletdb.View(db, func(tx walletdb.ReadTx) error {
+		ns := tx.ReadBucket(waddrmgrNamespaceKey)
+		return mgr.Unlock(ns, privPassphrase)
+	})
+	require.NoError(t, err)
+
+	scopedMgr, err := mgr.FetchScopedKeyManager(KeyScopeBIP0086)
+	require.NoError(t, err)
+
+	const iterations = 50
+
+	var wg sync.WaitGroup
+	wg.Add(2)
+
+	go func() {
+		defer wg.Done()
+		for i := 0; i < iterations; i++ {
+			_ = walletdb.Update(db, func(tx walletdb.ReadWriteTx) error {
+				ns := tx.ReadWriteBucket(waddrmgrNamespaceKey)
+				_, err := scopedMgr.NextExternalAddresses(
+					ns, 0, 1,
+				)
+				return err
+			})
+		}
+	}()
+
+	go func() {
+		defer wg.Done()
+		for i := 0; i < iterations; i++ {
+			_ = mgr.Lock()
+
+			_ = walletdb.View(db, func(tx walletdb.ReadTx) error {
+				ns := tx.ReadBucket(waddrmgrNamespaceKey)
+				return mgr.Unlock(ns, privPassphrase)
+			})
+		}
+	}()
+
+	wg.Wait()
+}