Merge pull request #535 from cfromknecht/notify-addrs-after-db-commit

wallet/wallet: notify addrs+props after db commit

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 {

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
+var (
-// function used so tests can provide a version that fails for testing error
+	// secretKeyGen is the inner method that is executed when calling
-// paths.
+	// newSecretKey.
-var newSecretKey = defaultNewSecretKey
+	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.

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
+	oldKeyGen := waddrmgr.SetSecretKeyGen(failingSecretKeyGen)
-	waddrmgr.TstRunWithReplacedNewSecretKey(func() {
+	err := walletdb.Update(tc.db, func(tx walletdb.ReadWriteTx) error {
-		err = walletdb.Update(tc.db, func(tx walletdb.ReadWriteTx) error {
+		ns := tx.ReadWriteBucket(waddrmgrNamespaceKey)
-			ns := tx.ReadWriteBucket(waddrmgrNamespaceKey)
+		return tc.rootManager.ChangePassphrase(
-			return tc.rootManager.ChangePassphrase(
+			ns, pubPassphrase, pubPassphrase2, false, fastScrypt,
-				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(

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
+		return nil, 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
-		}
 	}
 
 	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 {
+		return nil, nil, err
-		w.NtfnServer.notifyAccountProperties(props)
 	}
 
-	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.
+	return addrs[0].Address(), nil
-	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
 }
 
 // confirmed checks whether a transaction at height txHeight has met minconf