From 5838215e60c331c1b85a2fb49073fcc20d12379c Mon Sep 17 00:00:00 2001 From: Alex Akselrod Date: Mon, 25 Nov 2024 13:36:12 -0800 Subject: [PATCH 1/3] waddrmgr: test ScopedManager with root manager lock held --- waddrmgr/manager_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/waddrmgr/manager_test.go b/waddrmgr/manager_test.go index e3ea359231..13bc171342 100644 --- a/waddrmgr/manager_test.go +++ b/waddrmgr/manager_test.go @@ -2439,6 +2439,14 @@ func TestScopedKeyManagerManagement(t *testing.T) { err = walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { ns := tx.ReadWriteBucket(waddrmgrNamespaceKey) + // For this test, since we've fetched the scoped key manager, + // we want to check that this runs correctly while the root + // manager is locked for another task. This will be possible + // with Postgres-backed wallets having multiple connections to + // the database backend, executing transactions in parallel. + mgr.mtx.Lock() + defer mgr.mtx.Unlock() + // We'll now create a new external address to ensure we // retrieve the proper type. externalAddr, err = scopedMgr.NextExternalAddresses( From 3f4b45aaad8eb2b96bb22dcc9e625f78d086a42e Mon Sep 17 00:00:00 2001 From: Alex Akselrod Date: Mon, 25 Nov 2024 14:22:18 -0800 Subject: [PATCH 2/3] waddrmgr: use atomic bool instead of mutex for WatchOnly() --- waddrmgr/manager.go | 36 +++++++++++++----------------------- waddrmgr/scoped_manager.go | 4 ++-- 2 files changed, 15 insertions(+), 25 deletions(-) diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index ae586eb8e0..9146b7ed74 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -9,6 +9,7 @@ import ( "crypto/sha512" "fmt" "sync" + "sync/atomic" "time" "github.com/btcsuite/btcd/btcutil" @@ -345,7 +346,7 @@ type Manager struct { internalAddrSchemas map[AddressType][]KeyScope syncState syncState - watchingOnly bool + watchingOnly atomic.Bool birthday time.Time locked bool closed bool @@ -392,18 +393,7 @@ type Manager struct { // WatchOnly returns true if the root manager is in watch only mode, and false // otherwise. func (m *Manager) WatchOnly() bool { - m.mtx.RLock() - defer m.mtx.RUnlock() - - return m.watchOnly() -} - -// watchOnly returns true if the root manager is in watch only mode, and false -// otherwise. -// -// NOTE: This method requires the Manager's lock to be held. -func (m *Manager) watchOnly() bool { - return m.watchingOnly + return m.watchingOnly.Load() } // IsWatchOnlyAccount determines if the account with the given key scope is set @@ -489,7 +479,7 @@ func (m *Manager) Close() { } // Attempt to clear private key material from memory. - if !m.watchingOnly && !m.locked { + if !m.WatchOnly() && !m.locked { m.lock() } @@ -517,7 +507,7 @@ func (m *Manager) NewScopedKeyManager(ns walletdb.ReadWriteBucket, defer m.mtx.Unlock() var rootPriv *hdkeychain.ExtendedKey - if !m.watchingOnly { + if !m.WatchOnly() { // If the manager is locked, then we can't create a new scoped // manager. if m.locked { @@ -588,7 +578,7 @@ func (m *Manager) NewScopedKeyManager(ns walletdb.ReadWriteBucket, return nil, err } - if !m.watchingOnly { + if !m.WatchOnly() { // With the database state created, we'll now derive the // cointype key using the master HD private key, then encrypt // it along with the first account using our crypto keys. @@ -888,7 +878,7 @@ func (m *Manager) ChangePassphrase(ns walletdb.ReadWriteBucket, oldPassphrase, newPassphrase []byte, private bool, config *ScryptOptions) error { // No private passphrase to change for a watching-only address manager. - if private && m.watchingOnly { + if private && m.WatchOnly() { return managerError(ErrWatchingOnly, errWatchingOnly, nil) } @@ -1053,7 +1043,7 @@ func (m *Manager) ConvertToWatchingOnly(ns walletdb.ReadWriteBucket) error { defer m.mtx.Unlock() // Exit now if the manager is already watching-only. - if m.watchingOnly { + if m.WatchOnly() { return nil } @@ -1118,7 +1108,7 @@ func (m *Manager) ConvertToWatchingOnly(ns walletdb.ReadWriteBucket) error { m.masterKeyPriv = nil // Mark the manager watching-only. - m.watchingOnly = true + m.watchingOnly.Store(true) return nil } @@ -1149,7 +1139,7 @@ func (m *Manager) isLocked() bool { // manager. func (m *Manager) Lock() error { // A watching-only address manager can't be locked. - if m.watchingOnly { + if m.WatchOnly() { return managerError(ErrWatchingOnly, errWatchingOnly, nil) } @@ -1175,7 +1165,7 @@ func (m *Manager) Lock() error { // manager. func (m *Manager) Unlock(ns walletdb.ReadBucket, passphrase []byte) error { // A watching-only address manager can't be unlocked. - if m.watchingOnly { + if m.WatchOnly() { return managerError(ErrWatchingOnly, errWatchingOnly, nil) } @@ -1330,7 +1320,7 @@ func (m *Manager) LookupAccount(ns walletdb.ReadBucket, name string) (KeyScope, func (m *Manager) selectCryptoKey(keyType CryptoKeyType) (EncryptorDecryptor, error) { if keyType == CKTPrivate || keyType == CKTScript { // The manager must be unlocked to work with the private keys. - if m.locked || m.watchingOnly { + if m.locked || m.WatchOnly() { return nil, managerError(ErrLocked, errLocked, nil) } } @@ -1412,8 +1402,8 @@ func newManager(chainParams *chaincfg.Params, masterKeyPub *snacl.SecretKey, scopedManagers: scopedManagers, externalAddrSchemas: make(map[AddressType][]KeyScope), internalAddrSchemas: make(map[AddressType][]KeyScope), - watchingOnly: watchingOnly, } + m.watchingOnly.Store(watchingOnly) for _, sMgr := range m.scopedManagers { externalType := sMgr.AddrSchema().ExternalAddrType diff --git a/waddrmgr/scoped_manager.go b/waddrmgr/scoped_manager.go index 1ce03d2ade..f8f56e705e 100644 --- a/waddrmgr/scoped_manager.go +++ b/waddrmgr/scoped_manager.go @@ -447,7 +447,7 @@ func (s *ScopedKeyManager) loadAccountInfo(ns walletdb.ReadBucket, // The wallet will only contain private keys for default accounts if the // wallet's not set up as watch-only and it's been unlocked. - watchOnly := s.rootManager.watchOnly() + watchOnly := s.rootManager.WatchOnly() hasPrivateKey := !s.rootManager.isLocked() && !watchOnly // Create the new account info with the known information. The rest of @@ -777,7 +777,7 @@ func (s *ScopedKeyManager) chainAddressRowToManaged(ns walletdb.ReadBucket, // Since the manger's mutex is assumed to held when invoking this // function, we use the internal isLocked to avoid a deadlock. - private := !s.rootManager.isLocked() && !s.rootManager.watchOnly() + private := !s.rootManager.isLocked() && !s.rootManager.WatchOnly() addressKey, acctKey, masterKeyFingerprint, err := s.deriveKeyFromPath( ns, row.account, row.branch, row.index, private, From df51fa6dad296da8cb3a5c886a7f71b7b0986674 Mon Sep 17 00:00:00 2001 From: Alex Akselrod Date: Mon, 25 Nov 2024 14:28:58 -0800 Subject: [PATCH 3/3] waddrmgr: use atomic.Bool instead of mutex for IsLocked() --- waddrmgr/manager.go | 38 ++++++++++++++------------------------ waddrmgr/scoped_manager.go | 8 +++----- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index 9146b7ed74..8e937d2418 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -348,7 +348,7 @@ type Manager struct { syncState syncState watchingOnly atomic.Bool birthday time.Time - locked bool + locked atomic.Bool closed bool chainParams *chaincfg.Params @@ -459,7 +459,7 @@ func (m *Manager) lock() { // which uses a separate derived key from the database even when it is // locked. - m.locked = true + m.locked.Store(true) } // Close cleanly shuts down the manager. It makes a best try effort to remove @@ -479,7 +479,7 @@ func (m *Manager) Close() { } // Attempt to clear private key material from memory. - if !m.WatchOnly() && !m.locked { + if !m.WatchOnly() && !m.IsLocked() { m.lock() } @@ -510,7 +510,7 @@ func (m *Manager) NewScopedKeyManager(ns walletdb.ReadWriteBucket, if !m.WatchOnly() { // If the manager is locked, then we can't create a new scoped // manager. - if m.locked { + if m.IsLocked() { return nil, managerError(ErrLocked, errLocked, nil) } @@ -969,7 +969,7 @@ func (m *Manager) ChangePassphrase(ns walletdb.ReadWriteBucket, oldPassphrase, // If unlocked, create the new passphrase hash with the new // passphrase and salt. var hashedPassphrase [sha512.Size]byte - if m.locked { + if m.IsLocked() { newMasterKey.Zero() } else { saltedPassphrase := append(passphraseSalt[:], @@ -1062,7 +1062,7 @@ func (m *Manager) ConvertToWatchingOnly(ns walletdb.ReadWriteBucket) error { // Lock the manager to remove all clear text private key material from // memory if needed. - if !m.locked { + if !m.IsLocked() { m.lock() } @@ -1117,19 +1117,7 @@ func (m *Manager) ConvertToWatchingOnly(ns walletdb.ReadWriteBucket) error { // unlocked, the decryption key needed to decrypt private keys used for signing // is in memory. func (m *Manager) IsLocked() bool { - m.mtx.RLock() - defer m.mtx.RUnlock() - - return m.isLocked() -} - -// isLocked is an internal method returning whether or not the address manager -// is locked via an unprotected read. -// -// NOTE: The caller *MUST* acquire the Manager's mutex before invocation to -// avoid data races. -func (m *Manager) isLocked() bool { - return m.locked + return m.locked.Load() } // Lock performs a best try effort to remove and zero all secret keys associated @@ -1147,7 +1135,7 @@ func (m *Manager) Lock() error { defer m.mtx.Unlock() // Error on attempt to lock an already locked manager. - if m.locked { + if m.IsLocked() { return managerError(ErrLocked, errLocked, nil) } @@ -1174,7 +1162,7 @@ func (m *Manager) Unlock(ns walletdb.ReadBucket, passphrase []byte) error { // Avoid actually unlocking if the manager is already unlocked // and the passphrases match. - if !m.locked { + if !m.IsLocked() { saltedPassphrase := append(m.privPassphraseSalt[:], passphrase...) hashedPassphrase := sha512.Sum512(saltedPassphrase) @@ -1273,7 +1261,7 @@ func (m *Manager) Unlock(ns walletdb.ReadBucket, passphrase []byte) error { } } - m.locked = false + m.locked.Store(false) saltedPassphrase := append(m.privPassphraseSalt[:], passphrase...) m.hashedPrivPassphrase = sha512.Sum512(saltedPassphrase) zero.Bytes(saltedPassphrase) @@ -1320,7 +1308,7 @@ func (m *Manager) LookupAccount(ns walletdb.ReadBucket, name string) (KeyScope, func (m *Manager) selectCryptoKey(keyType CryptoKeyType) (EncryptorDecryptor, error) { if keyType == CKTPrivate || keyType == CKTScript { // The manager must be unlocked to work with the private keys. - if m.locked || m.WatchOnly() { + if m.IsLocked() || m.WatchOnly() { return nil, managerError(ErrLocked, errLocked, nil) } } @@ -1389,7 +1377,6 @@ func newManager(chainParams *chaincfg.Params, masterKeyPub *snacl.SecretKey, m := &Manager{ chainParams: chainParams, syncState: *syncInfo, - locked: true, birthday: birthday, masterKeyPub: masterKeyPub, masterKeyPriv: masterKeyPriv, @@ -1403,7 +1390,10 @@ func newManager(chainParams *chaincfg.Params, masterKeyPub *snacl.SecretKey, externalAddrSchemas: make(map[AddressType][]KeyScope), internalAddrSchemas: make(map[AddressType][]KeyScope), } + + // Store the atomic.Bool values for the manager. m.watchingOnly.Store(watchingOnly) + m.locked.Store(true) for _, sMgr := range m.scopedManagers { externalType := sMgr.AddrSchema().ExternalAddrType diff --git a/waddrmgr/scoped_manager.go b/waddrmgr/scoped_manager.go index f8f56e705e..ca55714708 100644 --- a/waddrmgr/scoped_manager.go +++ b/waddrmgr/scoped_manager.go @@ -448,7 +448,7 @@ func (s *ScopedKeyManager) loadAccountInfo(ns walletdb.ReadBucket, // The wallet will only contain private keys for default accounts if the // wallet's not set up as watch-only and it's been unlocked. watchOnly := s.rootManager.WatchOnly() - hasPrivateKey := !s.rootManager.isLocked() && !watchOnly + hasPrivateKey := !s.rootManager.IsLocked() && !watchOnly // Create the new account info with the known information. The rest of // the fields are filled out below. @@ -775,9 +775,7 @@ func (s *ScopedKeyManager) deriveKeyFromPath(ns walletdb.ReadBucket, func (s *ScopedKeyManager) chainAddressRowToManaged(ns walletdb.ReadBucket, row *dbChainAddressRow) (ManagedAddress, error) { - // Since the manger's mutex is assumed to held when invoking this - // function, we use the internal isLocked to avoid a deadlock. - private := !s.rootManager.isLocked() && !s.rootManager.WatchOnly() + private := !s.rootManager.IsLocked() && !s.rootManager.WatchOnly() addressKey, acctKey, masterKeyFingerprint, err := s.deriveKeyFromPath( ns, row.account, row.branch, row.index, private, @@ -1203,7 +1201,7 @@ func (s *ScopedKeyManager) nextAddresses(ns walletdb.ReadWriteBucket, // Add the new managed address to the list of addresses // that need their private keys derived when the // address manager is next unlocked. - if s.rootManager.isLocked() && !watchOnly { + if s.rootManager.IsLocked() && !watchOnly { s.deriveOnUnlock = append(s.deriveOnUnlock, info) } }