waddrmgr: Make create/update logic more explicit.

This commit makes the creation and updating of the address manager more
explicit so it's easier to upgrade in the future.

In particular, rather than treating the initial creation as an upgrade by
relying on creating the initial buckets on the fly on each load, the code
now explicitly provides distinct create and upgrade paths that are invoked
from the Create and Open functions, respectively.

It also adds some commented out sample code to illustrate how upgrades
should be done and a check to ensure bumping the version number without
writing upgrade code results in a new error, ErrUpgrade, being returned.

Finally, a test has been added for the new functionality.

waddrmgr/db.go

@@ -32,6 +32,12 @@ const (
 	LatestMgrVersion = 1
 )
 
+var (
+	// latestMgrVersion is the most recent manager version as a variable so
+	// the tests can change it to force errors.
+	latestMgrVersion uint32 = LatestMgrVersion
+)
+
 // maybeConvertDbError converts the passed error to a ManagerError with an
 // error code of ErrDatabase if it is not already a ManagerError.  This is
 // useful for potential errors returned from managed transaction an other parts
@@ -158,6 +164,27 @@ var (
 	acctNumAcctsName = []byte("numaccts")
 )
 
+// uint32ToBytes converts a 32 bit unsigned integer into a 4-byte slice in
+// little-endian order: 1 -> [1 0 0 0].
+func uint32ToBytes(number uint32) []byte {
+	buf := make([]byte, 4)
+	binary.LittleEndian.PutUint32(buf, number)
+	return buf
+}
+
+// putManagerVersion stores the provided version to the database.
+func putManagerVersion(tx walletdb.Tx, version uint32) error {
+	bucket := tx.RootBucket().Bucket(mainBucketName)
+
+	verBytes := uint32ToBytes(version)
+	err := bucket.Put(mgrVersionName, verBytes)
+	if err != nil {
+		str := "failed to store version"
+		return managerError(ErrDatabase, str, err)
+	}
+	return nil
+}
+
 // fetchMasterKeyParams loads the master key parameters needed to derive them
 // (when given the correct user-supplied passphrase) from the database.  Either
 // returned value can be nil, but in practice only the private key params will
@@ -308,14 +335,6 @@ func putWatchingOnly(tx walletdb.Tx, watchingOnly bool) error {
 	return nil
 }
 
-// uint32ToBytes converts a 32 bit unsigned integer into a 4-byte slice in
-// little-endian order: 1 -> [1 0 0 0].
-func uint32ToBytes(number uint32) []byte {
-	buf := make([]byte, 4)
-	binary.LittleEndian.PutUint32(buf, number)
-	return buf
-}
-
 // deserializeAccountRow deserializes the passed serialized account information.
 // This is used as a common base for the various account types to deserialize
 // the common parts.
@@ -1188,75 +1207,53 @@ func managerExists(namespace walletdb.Namespace) (bool, error) {
 	return exists, nil
 }
 
-// upgradeManager opens the manager using the specified namespace or creates and
-// initializes it if it does not already exist.  It also provides facilities to
-// upgrade the data in the namespace to newer versions.
-func upgradeManager(namespace walletdb.Namespace) error {
-	// Initialize the buckets and main db fields as needed.
-	var version uint32
-	var createDate uint64
+// createManagerNS creates the initial namespace structure needed for all of the
+// manager data.  This includes things such as all of the buckets as well as the
+// version and creation date.
+func createManagerNS(namespace walletdb.Namespace) error {
 	err := namespace.Update(func(tx walletdb.Tx) error {
 		rootBucket := tx.RootBucket()
-		mainBucket, err := rootBucket.CreateBucketIfNotExists(
-			mainBucketName)
+		mainBucket, err := rootBucket.CreateBucket(mainBucketName)
 		if err != nil {
 			str := "failed to create main bucket"
 			return managerError(ErrDatabase, str, err)
 		}
 
-		_, err = rootBucket.CreateBucketIfNotExists(addrBucketName)
+		_, err = rootBucket.CreateBucket(addrBucketName)
 		if err != nil {
 			str := "failed to create address bucket"
 			return managerError(ErrDatabase, str, err)
 		}
 
-		_, err = rootBucket.CreateBucketIfNotExists(acctBucketName)
+		_, err = rootBucket.CreateBucket(acctBucketName)
 		if err != nil {
 			str := "failed to create account bucket"
 			return managerError(ErrDatabase, str, err)
 		}
 
-		_, err = rootBucket.CreateBucketIfNotExists(addrAcctIdxBucketName)
+		_, err = rootBucket.CreateBucket(addrAcctIdxBucketName)
 		if err != nil {
 			str := "failed to create address index bucket"
 			return managerError(ErrDatabase, str, err)
 		}
 
-		_, err = rootBucket.CreateBucketIfNotExists(syncBucketName)
+		_, err = rootBucket.CreateBucket(syncBucketName)
 		if err != nil {
 			str := "failed to create sync bucket"
 			return managerError(ErrDatabase, str, err)
 		}
 
-		// Save the most recent database version if it isn't already
-		// there, otherwise keep track of it for potential upgrades.
-		verBytes := mainBucket.Get(mgrVersionName)
-		if verBytes == nil {
-			version = LatestMgrVersion
-
-			var buf [4]byte
-			binary.LittleEndian.PutUint32(buf[:], version)
-			err := mainBucket.Put(mgrVersionName, buf[:])
-			if err != nil {
-				str := "failed to store latest database version"
-				return managerError(ErrDatabase, str, err)
-			}
-		} else {
-			version = binary.LittleEndian.Uint32(verBytes)
+		if err := putManagerVersion(tx, latestMgrVersion); err != nil {
+			return err
 		}
 
-		createBytes := mainBucket.Get(mgrCreateDateName)
-		if createBytes == nil {
-			createDate = uint64(time.Now().Unix())
-			var buf [8]byte
-			binary.LittleEndian.PutUint64(buf[:], createDate)
-			err := mainBucket.Put(mgrCreateDateName, buf[:])
-			if err != nil {
-				str := "failed to store database creation time"
-				return managerError(ErrDatabase, str, err)
-			}
-		} else {
-			createDate = binary.LittleEndian.Uint64(createBytes)
+		createDate := uint64(time.Now().Unix())
+		var dateBytes [8]byte
+		binary.LittleEndian.PutUint64(dateBytes[:], createDate)
+		err = mainBucket.Put(mgrCreateDateName, dateBytes[:])
+		if err != nil {
+			str := "failed to store database creation time"
+			return managerError(ErrDatabase, str, err)
 		}
 
 		return nil
@@ -1266,9 +1263,63 @@ func upgradeManager(namespace walletdb.Namespace) error {
 		return managerError(ErrDatabase, str, err)
 	}
 
-	// Upgrade the manager as needed.
-	if version < LatestMgrVersion {
-		// No upgrades yet.
+	return nil
+}
+
+// upgradeManager upgrades the data in the provided manager namespace to newer
+// versions as neeeded.
+func upgradeManager(namespace walletdb.Namespace) error {
+	// Get the current version.
+	var version uint32
+	err := namespace.View(func(tx walletdb.Tx) error {
+		mainBucket := tx.RootBucket().Bucket(mainBucketName)
+		verBytes := mainBucket.Get(mgrVersionName)
+		version = binary.LittleEndian.Uint32(verBytes)
+		return nil
+	})
+	if err != nil {
+		str := "failed to fetch version for update"
+		return managerError(ErrDatabase, str, err)
+	}
+
+	// NOTE: There are currently no upgrades, but this is provided here as a
+	// template for how to properly do upgrades.  Each function to upgrade
+	// to the next version must include serializing the new version as a
+	// part of the same transaction so any failures in upgrades to later
+	// versions won't leave the database in an inconsistent state.  The
+	// putManagerVersion function provides a convenient mechanism for that
+	// purpose.
+	//
+	// Upgrade one version at a time so it is possible to upgrade across
+	// an aribtary number of versions without needing to write a bunch of
+	// additional code to go directly from version X to Y.
+	// if version < 2 {
+	// 	// Upgrade from version 1 to 2.
+	//	if err := upgradeToVersion2(namespace); err != nil {
+	//		return err
+	//	}
+	//
+	//	// The manager is now at version 2.
+	//	version = 2
+	// }
+	// if version < 3 {
+	// 	// Upgrade from version 2 to 3.
+	//	if err := upgradeToVersion3(namespace); err != nil {
+	//		return err
+	//	}
+	//
+	//	// The manager is now at version 3.
+	//	version = 3
+	// }
+
+	// Ensure the manager is upraded to the latest version.  This check is
+	// to intentionally cause a failure if the manager version is updated
+	// without writing code to handle the upgrade.
+	if version < latestMgrVersion {
+		str := fmt.Sprintf("the latest manager version is %d, but the "+
+			"current version after upgrades is only %d",
+			latestMgrVersion, version)
+		return managerError(ErrUpgrade, str, nil)
 	}
 
 	return nil

waddrmgr/error.go

@@ -57,6 +57,11 @@ const (
 	// set to the underlying error returned from the database.
 	ErrDatabase ErrorCode = iota
 
+	// ErrUpgrade indicates the manager needs to be upgraded.  This should
+	// not happen in practice unless the version number has been increased
+	// and there is not yet any code written to upgrade.
+	ErrUpgrade
+
 	// ErrKeyChain indicates an error with the key chain typically either
 	// due to the inability to create an extended key or deriving a child
 	// extended key.  When this error code is set, the Err field of the
@@ -128,6 +133,7 @@ const (
 // Map of ErrorCode values back to their constant names for pretty printing.
 var errorCodeStrings = map[ErrorCode]string{
 	ErrDatabase:          "ErrDatabase",
+	ErrUpgrade:           "ErrUpgrade",
 	ErrKeyChain:          "ErrKeyChain",
 	ErrCrypto:            "ErrCrypto",
 	ErrInvalidKeyType:    "ErrInvalidKeyType",

waddrmgr/error_test.go

@@ -30,6 +30,7 @@ func TestErrorCodeStringer(t *testing.T) {
 		want string
 	}{
 		{waddrmgr.ErrDatabase, "ErrDatabase"},
+		{waddrmgr.ErrUpgrade, "ErrUpgrade"},
 		{waddrmgr.ErrKeyChain, "ErrKeyChain"},
 		{waddrmgr.ErrCrypto, "ErrCrypto"},
 		{waddrmgr.ErrInvalidKeyType, "ErrInvalidKeyType"},

waddrmgr/internal_test.go

@@ -33,6 +33,10 @@ import (
 // when tests are run.
 var TstMaxRecentHashes = maxRecentHashes
 
+// TstLatestMgrVersion makes the unexported latestMgrVersion variable available
+// 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.

waddrmgr/manager.go

@@ -1841,8 +1841,7 @@ func Open(namespace walletdb.Namespace, pubPassphrase []byte, chainParams *chain
 		return nil, managerError(ErrNoExist, str, nil)
 	}
 
-	// Upgrade the manager to the latest version as needed.  This includes
-	// the initial creation.
+	// Upgrade the manager to the latest version as needed.
 	if err := upgradeManager(namespace); err != nil {
 		return nil, err
 	}
@@ -1882,9 +1881,8 @@ func Create(namespace walletdb.Namespace, seed, pubPassphrase, privPassphrase []
 		return nil, managerError(ErrAlreadyExists, errAlreadyExists, nil)
 	}
 
-	// Upgrade the manager to the latest version as needed.  This includes
-	// the initial creation.
-	if err := upgradeManager(namespace); err != nil {
+	// Perform the initial bucket creation and database namespace setup.
+	if err := createManagerNS(namespace); err != nil {
 		return nil, err
 	}
 

waddrmgr/manager_test.go

@@ -1508,6 +1508,16 @@ func TestManager(t *testing.T) {
 	})
 	mgr.Close()
 
+	// Ensure the expected error is returned if the latest manager version
+	// constant is bumped without writing code to actually do the upgrade.
+	*waddrmgr.TstLatestMgrVersion++
+	_, err = waddrmgr.Open(mgrNamespace, pubPassphrase,
+		&chaincfg.MainNetParams, fastScrypt)
+	if !checkManagerError(t, "Upgrade needed", err, waddrmgr.ErrUpgrade) {
+		return
+	}
+	*waddrmgr.TstLatestMgrVersion--
+
 	// Open the manager and run all the tests again in open mode which
 	// avoids reinserting new addresses like the create mode tests do.
 	mgr, err = waddrmgr.Open(mgrNamespace, pubPassphrase,