From ba58d5357faa247cba3b55726ba1af5238878e97 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 31 Aug 2018 16:27:04 -0700 Subject: [PATCH 1/4] waddrmgr/manager: guard access to newSecretKey This commit places a mutex around calls to newSecretKey, since the inner function needs to be swapped out during testing. Prior to this change, the race detector would panic since the mutation was unprotected. --- waddrmgr/manager.go | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index 3be1387..a2f8962 100644 --- a/waddrmgr/manager.go +++ b/waddrmgr/manager.go @@ -174,15 +174,46 @@ type unlockDeriveInfo struct { index uint32 } +// SecretKeyGenerator is the function signature of a method that can generate +// secret keys for the address manager. +type SecretKeyGenerator func( + passphrase *[]byte, config *ScryptOptions) (*snacl.SecretKey, error) + // defaultNewSecretKey returns a new secret key. See newSecretKey. -func defaultNewSecretKey(passphrase *[]byte, config *ScryptOptions) (*snacl.SecretKey, error) { +func defaultNewSecretKey(passphrase *[]byte, + config *ScryptOptions) (*snacl.SecretKey, error) { return snacl.NewSecretKey(passphrase, config.N, config.R, config.P) } -// newSecretKey is used as a way to replace the new secret key generation -// function used so tests can provide a version that fails for testing error -// paths. -var newSecretKey = defaultNewSecretKey +var ( + // secretKeyGen is the inner method that is executed when calling + // newSecretKey. + secretKeyGen = defaultNewSecretKey + + // secretKeyGenMtx protects access to secretKeyGen, so that it can be + // replaced in testing. + secretKeyGenMtx sync.RWMutex +) + +// SetSecretKeyGen replaces the existing secret key generator, and returns the +// previous generator. +func SetSecretKeyGen(keyGen SecretKeyGenerator) SecretKeyGenerator { + secretKeyGenMtx.Lock() + oldKeyGen := secretKeyGen + secretKeyGen = keyGen + secretKeyGenMtx.Unlock() + + return oldKeyGen +} + +// newSecretKey generates a new secret key using the active secretKeyGen. +func newSecretKey(passphrase *[]byte, + config *ScryptOptions) (*snacl.SecretKey, error) { + + secretKeyGenMtx.RLock() + defer secretKeyGenMtx.RUnlock() + return secretKeyGen(passphrase, config) +} // EncryptorDecryptor provides an abstraction on top of snacl.CryptoKey so that // our tests can use dependency injection to force the behaviour they need. From 644fd2bda03ca80bb178d731751169bf6b58cd2e Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 31 Aug 2018 16:28:20 -0700 Subject: [PATCH 2/4] waddrmgr/internal_test: remove TstRunWithReplacedSecretKey --- waddrmgr/internal_test.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/waddrmgr/internal_test.go b/waddrmgr/internal_test.go index 7997a41..6279ecb 100644 --- a/waddrmgr/internal_test.go +++ b/waddrmgr/internal_test.go @@ -21,20 +21,6 @@ import ( // for change when the tests are run. var TstLatestMgrVersion = &latestMgrVersion -// Replace the Manager.newSecretKey function with the given one and calls -// the callback function. Afterwards the original newSecretKey -// function will be restored. -func TstRunWithReplacedNewSecretKey(callback func()) { - orig := newSecretKey - defer func() { - newSecretKey = orig - }() - newSecretKey = func(passphrase *[]byte, config *ScryptOptions) (*snacl.SecretKey, error) { - return nil, snacl.ErrDecryptFailed - } - callback() -} - // TstCheckPublicPassphrase returns true if the provided public passphrase is // correct for the manager. func (m *Manager) TstCheckPublicPassphrase(pubPassphrase []byte) bool { From 85c75de4a57004291ed40448394ac2425630d605 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 31 Aug 2018 16:28:38 -0700 Subject: [PATCH 3/4] waddrmgr/manager_test: use SetSecretKeyGen to safely swap keygen --- waddrmgr/manager_test.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/waddrmgr/manager_test.go b/waddrmgr/manager_test.go index c355355..67edc7b 100644 --- a/waddrmgr/manager_test.go +++ b/waddrmgr/manager_test.go @@ -16,6 +16,7 @@ import ( "github.com/btcsuite/btcd/chaincfg" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcutil" + "github.com/btcsuite/btcwallet/snacl" "github.com/btcsuite/btcwallet/waddrmgr" "github.com/btcsuite/btcwallet/walletdb" "github.com/davecgh/go-spew/spew" @@ -33,6 +34,13 @@ func newHash(hexStr string) *chainhash.Hash { return hash } +// failingSecretKeyGen is a waddrmgr.SecretKeyGenerator that always returns +// snacl.ErrDecryptFailed. +func failingSecretKeyGen(passphrase *[]byte, + config *waddrmgr.ScryptOptions) (*snacl.SecretKey, error) { + return nil, snacl.ErrDecryptFailed +} + // testContext is used to store context information about a running test which // is passed into helper functions. The useSpends field indicates whether or // not the spend data should be empty or figure it out based on the specific @@ -1099,14 +1107,12 @@ func testChangePassphrase(tc *testContext) bool { // that intentionally errors. testName := "ChangePassphrase (public) with invalid new secret key" - var err error - waddrmgr.TstRunWithReplacedNewSecretKey(func() { - err = walletdb.Update(tc.db, func(tx walletdb.ReadWriteTx) error { - ns := tx.ReadWriteBucket(waddrmgrNamespaceKey) - return tc.rootManager.ChangePassphrase( - ns, pubPassphrase, pubPassphrase2, false, fastScrypt, - ) - }) + oldKeyGen := waddrmgr.SetSecretKeyGen(failingSecretKeyGen) + err := walletdb.Update(tc.db, func(tx walletdb.ReadWriteTx) error { + ns := tx.ReadWriteBucket(waddrmgrNamespaceKey) + return tc.rootManager.ChangePassphrase( + ns, pubPassphrase, pubPassphrase2, false, fastScrypt, + ) }) if !checkManagerError(tc.t, testName, err, waddrmgr.ErrCrypto) { return false @@ -1114,6 +1120,7 @@ func testChangePassphrase(tc *testContext) bool { // Attempt to change public passphrase with invalid old passphrase. testName = "ChangePassphrase (public) with invalid old passphrase" + waddrmgr.SetSecretKeyGen(oldKeyGen) err = walletdb.Update(tc.db, func(tx walletdb.ReadWriteTx) error { ns := tx.ReadWriteBucket(waddrmgrNamespaceKey) return tc.rootManager.ChangePassphrase( From e508a127b6eb0c07dc7d9e6380a08eb0c95df7ae Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 30 Aug 2018 18:29:26 -0700 Subject: [PATCH 4/4] wallet/wallet: notify addrs+props after db commit This PR moves any address notifications outside of the db transaction that creates them. This is known to have resulted in deadlocks, since chainClient.NotifyReceived could block the db transaction from committing. Doing so also prevents the situation where we send notifications about the new addresses, but the db txn fails to commit and the addresses are in fact never created. --- wallet/wallet.go | 136 ++++++++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 48 deletions(-) diff --git a/wallet/wallet.go b/wallet/wallet.go index a1684da..d828b1e 100644 --- a/wallet/wallet.go +++ b/wallet/wallet.go @@ -1434,33 +1434,62 @@ func (w *Wallet) CalculateAccountBalances(account uint32, confirms int32) (Balan // been used (there is at least one transaction spending to it in the // blockchain or btcd mempool), the next chained address is returned. func (w *Wallet) CurrentAddress(account uint32, scope waddrmgr.KeyScope) (btcutil.Address, error) { + chainClient, err := w.requireChainClient() + if err != nil { + return nil, err + } + manager, err := w.Manager.FetchScopedKeyManager(scope) if err != nil { return nil, err } - var addr btcutil.Address + var ( + addr btcutil.Address + props *waddrmgr.AccountProperties + ) err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error { addrmgrNs := tx.ReadWriteBucket(waddrmgrNamespaceKey) maddr, err := manager.LastExternalAddress(addrmgrNs, account) if err != nil { - // If no address exists yet, create the first external address + // If no address exists yet, create the first external + // address. if waddrmgr.IsError(err, waddrmgr.ErrAddressNotFound) { - addr, err = w.newAddress(addrmgrNs, account, scope) + addr, props, err = w.newAddress( + addrmgrNs, account, scope, + ) } return err } - // Get next chained address if the last one has already been used. + // Get next chained address if the last one has already been + // used. if maddr.Used(addrmgrNs) { - addr, err = w.newAddress(addrmgrNs, account, scope) + addr, props, err = w.newAddress( + addrmgrNs, account, scope, + ) return err } addr = maddr.Address() return nil }) - return addr, err + if err != nil { + return nil, err + } + + // If the props have been initially, then we had to create a new address + // to satisfy the query. Notify the rpc server about the new address. + if props != nil { + err = chainClient.NotifyReceived([]btcutil.Address{addr}) + if err != nil { + return nil, err + } + + w.NtfnServer.notifyAccountProperties(props) + } + + return addr, nil } // PubKeyForAddress looks up the associated public key for a P2PKH address. @@ -2763,69 +2792,94 @@ func (w *Wallet) SortedActivePaymentAddresses() ([]string, error) { // NewAddress returns the next external chained address for a wallet. func (w *Wallet) NewAddress(account uint32, - scope waddrmgr.KeyScope) (addr btcutil.Address, err error) { + scope waddrmgr.KeyScope) (btcutil.Address, error) { + chainClient, err := w.requireChainClient() + if err != nil { + return nil, err + } + + var ( + addr btcutil.Address + props *waddrmgr.AccountProperties + ) err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error { addrmgrNs := tx.ReadWriteBucket(waddrmgrNamespaceKey) var err error - addr, err = w.newAddress(addrmgrNs, account, scope) + addr, props, err = w.newAddress(addrmgrNs, account, scope) return err }) - return + if err != nil { + return nil, err + } + + // Notify the rpc server about the newly created address. + err = chainClient.NotifyReceived([]btcutil.Address{addr}) + if err != nil { + return nil, err + } + + w.NtfnServer.notifyAccountProperties(props) + + return addr, nil } func (w *Wallet) newAddress(addrmgrNs walletdb.ReadWriteBucket, account uint32, - scope waddrmgr.KeyScope) (btcutil.Address, error) { + scope waddrmgr.KeyScope) (btcutil.Address, *waddrmgr.AccountProperties, error) { manager, err := w.Manager.FetchScopedKeyManager(scope) if err != nil { - return nil, err + return nil, nil, err } // Get next address from wallet. addrs, err := manager.NextExternalAddresses(addrmgrNs, account, 1) if err != nil { - return nil, err - } - - // Request updates from btcd for new transactions sent to this address. - utilAddrs := make([]btcutil.Address, len(addrs)) - for i, addr := range addrs { - utilAddrs[i] = addr.Address() - } - w.chainClientLock.Lock() - chainClient := w.chainClient - w.chainClientLock.Unlock() - if chainClient != nil { - err := chainClient.NotifyReceived(utilAddrs) - if err != nil { - return nil, err - } + return nil, nil, err } props, err := manager.AccountProperties(addrmgrNs, account) if err != nil { log.Errorf("Cannot fetch account properties for notification "+ "after deriving next external address: %v", err) - } else { - w.NtfnServer.notifyAccountProperties(props) + return nil, nil, err } - return utilAddrs[0], nil + return addrs[0].Address(), props, nil } // NewChangeAddress returns a new change address for a wallet. -func (w *Wallet) NewChangeAddress(account uint32, scope waddrmgr.KeyScope) (addr btcutil.Address, err error) { +func (w *Wallet) NewChangeAddress(account uint32, + scope waddrmgr.KeyScope) (btcutil.Address, error) { + + chainClient, err := w.requireChainClient() + if err != nil { + return nil, err + } + + var addr btcutil.Address err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error { addrmgrNs := tx.ReadWriteBucket(waddrmgrNamespaceKey) var err error addr, err = w.newChangeAddress(addrmgrNs, account) return err }) - return + if err != nil { + return nil, err + } + + // Notify the rpc server about the newly created address. + err = chainClient.NotifyReceived([]btcutil.Address{addr}) + if err != nil { + return nil, err + } + + return addr, nil } -func (w *Wallet) newChangeAddress(addrmgrNs walletdb.ReadWriteBucket, account uint32) (btcutil.Address, error) { +func (w *Wallet) newChangeAddress(addrmgrNs walletdb.ReadWriteBucket, + account uint32) (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( @@ -2842,21 +2896,7 @@ func (w *Wallet) newChangeAddress(addrmgrNs walletdb.ReadWriteBucket, account ui return nil, err } - // Request updates from btcd for new transactions sent to this address. - utilAddrs := make([]btcutil.Address, len(addrs)) - for i, addr := range addrs { - utilAddrs[i] = addr.Address() - } - - chainClient, err := w.requireChainClient() - if err == nil { - err = chainClient.NotifyReceived(utilAddrs) - if err != nil { - return nil, err - } - } - - return utilAddrs[0], nil + return addrs[0].Address(), nil } // confirmed checks whether a transaction at height txHeight has met minconf