Address several issues pointed out by lint and vet.

This brings the entire tree closer to but not 100% goclean.sh clean.

btcwallet.go

@@ -138,14 +138,13 @@ func walletMain() error {
 				// that generating new wallets is ok.
 				server.SetWallet(nil)
 				return
-			} else {
+			}
 			// If the keystore file exists but another error was
 			// encountered, we cannot continue.
 			log.Errorf("Cannot load wallet files: %v", err)
 			walletOpenErrors <- err
 			return
 		}
-		}
 
 		server.SetWallet(w)
 

chain/chain.go

@@ -30,6 +30,8 @@ import (
 	"github.com/btcsuite/btcws"
 )
 
+// Client represents a persistent client connection to a bitcoin RPC server
+// for information regarding the current best block chain.
 type Client struct {
 	*btcrpcclient.Client
 	chainParams *chaincfg.Params
@@ -51,6 +53,12 @@ type Client struct {
 	quitMtx sync.Mutex
 }
 
+// NewClient creates a client connection to the server described by the connect
+// string.  If disableTLS is false, the remote RPC certificate must be provided
+// in the certs slice.  The connection is not established immediately, but must
+// be done using the Start method.  If the remote server does not operate on
+// the same bitcoin network as described by the passed chain parameters, the
+// connection will be disconnected.
 func NewClient(chainParams *chaincfg.Params, connect, user, pass string, certs []byte, disableTLS bool) (*Client, error) {
 	client := Client{
 		chainParams:         chainParams,
@@ -86,6 +94,11 @@ func NewClient(chainParams *chaincfg.Params, connect, user, pass string, certs [
 	return &client, nil
 }
 
+// Start attempts to establish a client connection with the remote server.
+// If successful, handler goroutines are started to process notifications
+// sent by the server.  After a limited number of connection attempts, this
+// function gives up, and therefore will not block forever waiting for the
+// connection to be established to a server that may not exist.
 func (c *Client) Start() error {
 	err := c.Connect(5) // attempt connection 5 tries at most
 	if err != nil {
@@ -112,6 +125,8 @@ func (c *Client) Start() error {
 	return nil
 }
 
+// Stop disconnects the client and signals the shutdown of all goroutines
+// started by Start.
 func (c *Client) Stop() {
 	c.quitMtx.Lock()
 	defer c.quitMtx.Unlock()
@@ -128,15 +143,67 @@ func (c *Client) Stop() {
 	}
 }
 
+// WaitForShutdown blocks until both the client has finished disconnecting
+// and all handlers have exited.
 func (c *Client) WaitForShutdown() {
 	c.Client.WaitForShutdown()
 	c.wg.Wait()
 }
 
+// Notification types.  These are defined here and processed from from reading
+// a notificationChan to avoid handling these notifications directly in
+// btcrpcclient callbacks, which isn't very Go-like and doesn't allow
+// blocking client calls.
+type (
+	// BlockConnected is a notification for a newly-attached block to the
+	// best chain.
+	BlockConnected keystore.BlockStamp
+
+	// BlockDisconnected is a notifcation that the block described by the
+	// BlockStamp was reorganized out of the best chain.
+	BlockDisconnected keystore.BlockStamp
+
+	// RecvTx is a notification for a transaction which pays to a wallet
+	// address.
+	RecvTx struct {
+		Tx    *btcutil.Tx    // Index is guaranteed to be set.
+		Block *txstore.Block // nil if unmined
+	}
+
+	// RedeemingTx is a notification for a transaction which spends an
+	// output controlled by the wallet.
+	RedeemingTx struct {
+		Tx    *btcutil.Tx    // Index is guaranteed to be set.
+		Block *txstore.Block // nil if unmined
+	}
+
+	// RescanProgress is a notification describing the current status
+	// of an in-progress rescan.
+	RescanProgress struct {
+		Hash   *wire.ShaHash
+		Height int32
+		Time   time.Time
+	}
+
+	// RescanFinished is a notification that a previous rescan request
+	// has finished.
+	RescanFinished struct {
+		Hash   *wire.ShaHash
+		Height int32
+		Time   time.Time
+	}
+)
+
+// Notifications returns a channel of parsed notifications sent by the remote
+// bitcoin RPC server.  This channel must be continually read or the process
+// may abort for running out memory, as unread notifications are queued for
+// later reads.
 func (c *Client) Notifications() <-chan interface{} {
 	return c.dequeueNotification
 }
 
+// BlockStamp returns the latest block notified by the client, or an error
+// if the client has been shut down.
 func (c *Client) BlockStamp() (*keystore.BlockStamp, error) {
 	select {
 	case bs := <-c.currentBlock:
@@ -146,33 +213,6 @@ func (c *Client) BlockStamp() (*keystore.BlockStamp, error) {
 	}
 }
 
-// Notification types.  These are defined here and processed from from reading
-// a notificationChan to avoid handling these notifications directly in
-// btcrpcclient callbacks, which isn't very Go-like and doesn't allow
-// blocking client calls.
-type (
-	BlockConnected    keystore.BlockStamp
-	BlockDisconnected keystore.BlockStamp
-	RecvTx            struct {
-		Tx    *btcutil.Tx    // Index is guaranteed to be set.
-		Block *txstore.Block // nil if unmined
-	}
-	RedeemingTx struct {
-		Tx    *btcutil.Tx    // Index is guaranteed to be set.
-		Block *txstore.Block // nil if unmined
-	}
-	RescanProgress struct {
-		Hash   *wire.ShaHash
-		Height int32
-		Time   time.Time
-	}
-	RescanFinished struct {
-		Hash   *wire.ShaHash
-		Height int32
-		Time   time.Time
-	}
-)
-
 // parseBlock parses a btcws definition of the block a tx is mined it to the
 // Block structure of the txstore package, and the block index.  This is done
 // here since btcrpcclient doesn't parse this nicely for us.

createtx.go

@@ -89,7 +89,9 @@ func (e InsufficientFundsError) Error() string {
 		e.fee, e.in)
 }
 
-var UnsupportedTransactionType = errors.New("Only P2PKH transactions are supported")
+// ErrUnsupportedTransactionType represents an error where a transaction
+// cannot be signed as the API only supports spending P2PKH outputs.
+var ErrUnsupportedTransactionType = errors.New("Only P2PKH transactions are supported")
 
 // ErrNonPositiveAmount represents an error where a bitcoin amount is
 // not positive (either negative, or zero).
@@ -103,6 +105,8 @@ var ErrNegativeFee = errors.New("fee is negative")
 // measured in satoshis) added to transactions requiring a fee.
 const defaultFeeIncrement = 1e3
 
+// CreatedTx holds the state of a newly-created transaction and the change
+// output (if one was added).
 type CreatedTx struct {
 	tx          *btcutil.Tx
 	changeAddr  btcutil.Address
@@ -385,7 +389,7 @@ func signMsgTx(msgtx *wire.MsgTx, prevOutputs []txstore.Credit, store *keystore.
 		}
 		apkh, ok := addrs[0].(*btcutil.AddressPubKeyHash)
 		if !ok {
-			return UnsupportedTransactionType
+			return ErrUnsupportedTransactionType
 		}
 
 		ai, err := store.Address(apkh)

txstore/json.go

@@ -114,7 +114,7 @@ const (
 	CreditImmature
 )
 
-// category returns the category of the credit.  The passed block chain height is
+// Category returns the category of the credit.  The passed block chain height is
 // used to distinguish immature from mature coinbase outputs.
 func (c *Credit) Category(chainHeight int32) CreditCategory {
 	c.s.mtx.RLock()

txstore/serialization.go

@@ -1146,7 +1146,9 @@ func (u *unconfirmedStore) WriteTo(w io.Writer) (int64, error) {
 	return n64, nil
 }
 
-// TODO: set this automatically.
+// MarkDirty marks that changes have been made to the transaction store.
+// This should be run after any modifications are performed to the store
+// or any of its records.
 func (s *Store) MarkDirty() {
 	s.mtx.Lock()
 	defer s.mtx.Unlock()
@@ -1154,6 +1156,8 @@ func (s *Store) MarkDirty() {
 	s.dirty = true
 }
 
+// WriteIfDirty writes the entire transaction store to permanent storage if
+// the dirty flag has been set (see MarkDirty).
 func (s *Store) WriteIfDirty() error {
 	s.mtx.RLock()
 	if !s.dirty {

votingpool/pool_test.go

@@ -589,18 +589,18 @@ func TestCannotReplaceEmpoweredSeries(t *testing.T) {
 	var seriesID uint32 = 1
 
 	if err := pool.CreateSeries(1, seriesID, 3, []string{pubKey0, pubKey1, pubKey2, pubKey3}); err != nil {
-		t.Fatalf("Failed to create series", err)
+		t.Fatalf("Failed to create series: %v", err)
 	}
 
 	// We need to unlock the manager in order to empower a series.
 	manager.Unlock(privPassphrase)
 
 	if err := pool.EmpowerSeries(seriesID, privKey1); err != nil {
-		t.Fatalf("Failed to empower series", err)
+		t.Fatalf("Failed to empower series: %v", err)
 	}
 
 	if err := pool.ReplaceSeries(1, seriesID, 2, []string{pubKey0, pubKey2, pubKey3}); err == nil {
-		t.Errorf("Replaced an empowered series. That should not be possible", err)
+		t.Errorf("Replaced an empowered series. That should not be possible: %v", err)
 	} else {
 		gotErr := err.(waddrmgr.ManagerError)
 		wantErrCode := waddrmgr.ErrorCode(waddrmgr.ErrSeriesAlreadyEmpowered)
@@ -693,13 +693,13 @@ func TestReplaceExistingSeries(t *testing.T) {
 		testID := data.testID
 
 		if err := pool.CreateSeries(data.orig.version, seriesID, data.orig.reqSigs, data.orig.pubKeys); err != nil {
-			t.Fatalf("Test #%d: failed to create series in replace series setup",
+			t.Fatalf("Test #%d: failed to create series in replace series setup: %v",
 				testID, err)
 		}
 
 		if err := pool.ReplaceSeries(data.replaceWith.version, seriesID,
 			data.replaceWith.reqSigs, data.replaceWith.pubKeys); err != nil {
-			t.Errorf("Test #%d: replaceSeries failed", testID, err)
+			t.Errorf("Test #%d: replaceSeries failed: %v", testID, err)
 		}
 
 		validateReplaceSeries(t, pool, testID, data.replaceWith)
@@ -959,7 +959,7 @@ func validateLoadAllSeries(t *testing.T, pool *votingpool.Pool, testID int, seri
 
 	sortedKeys := votingpool.CanonicalKeyOrder(seriesData.pubKeys)
 	if !reflect.DeepEqual(publicKeys, sortedKeys) {
-		t.Errorf("Test #%d, series #%d: public keys mismatch. Got %d, want %d",
+		t.Errorf("Test #%d, series #%d: public keys mismatch. Got %v, want %v",
 			testID, seriesData.id, sortedKeys, publicKeys)
 	}
 
@@ -973,7 +973,7 @@ func validateLoadAllSeries(t *testing.T, pool *votingpool.Pool, testID int, seri
 	foundPrivKeys = votingpool.CanonicalKeyOrder(foundPrivKeys)
 	privKeys := votingpool.CanonicalKeyOrder(seriesData.privKeys)
 	if !reflect.DeepEqual(privKeys, foundPrivKeys) {
-		t.Errorf("Test #%d, series #%d: private keys mismatch. Got %d, want %d",
+		t.Errorf("Test #%d, series #%d: private keys mismatch. Got %v, want %v",
 			testID, seriesData.id, foundPrivKeys, privKeys)
 	}
 }
@@ -1085,14 +1085,14 @@ func createTestPubKeys(t *testing.T, number, offset int) []*hdkeychain.ExtendedK
 	xpubRaw := "xpub661MyMwAqRbcFwdnYF5mvCBY54vaLdJf8c5ugJTp5p7PqF9J1USgBx12qYMnZ9yUiswV7smbQ1DSweMqu8wn7Jociz4PWkuJ6EPvoVEgMw7"
 	xpubKey, err := hdkeychain.NewKeyFromString(xpubRaw)
 	if err != nil {
-		t.Fatalf("Failed to generate new key", err)
+		t.Fatalf("Failed to generate new key: %v", err)
 	}
 
 	keys := make([]*hdkeychain.ExtendedKey, number)
 	for i := uint32(0); i < uint32(len(keys)); i++ {
 		chPubKey, err := xpubKey.Child(i + uint32(offset))
 		if err != nil {
-			t.Fatalf("Failed to generate child key", err)
+			t.Fatalf("Failed to generate child key: %v", err)
 		}
 		keys[i] = chPubKey
 	}
@@ -1303,7 +1303,7 @@ func TestSerialization(t *testing.T) {
 		}
 
 		if row.Active != test.active {
-			t.Errorf("Serialization #%d - active mismatch: got %d want %d",
+			t.Errorf("Serialization #%d - active mismatch: got %v want %v",
 				testNum, row.Active, test.active)
 		}
 

waddrmgr/manager.go

@@ -44,7 +44,7 @@ const (
 	// the underlying hierarchical deterministic key derivation.
 	MaxAddressesPerAccount = hdkeychain.HardenedKeyStart - 1
 
-	// importedAddrAccount is the account number to use for all imported
+	// ImportedAddrAccount is the account number to use for all imported
 	// addresses.  This is useful since normal accounts are derived from the
 	// root hierarchical deterministic key and imported addresses do not
 	// fit into that model.

wallet.go

@@ -36,13 +36,10 @@ import (
 	"github.com/btcsuite/btcwallet/txstore"
 )
 
-var (
+// ErrNotSynced describes an error where an operation cannot complete
-	ErrNoWalletFiles = errors.New("no wallet files")
+// due wallet being out of sync (and perhaps currently syncing with)
-
+// the remote chain server.
-	ErrWalletExists = errors.New("wallet already exists")
+var ErrNotSynced = errors.New("wallet is not synchronized with the chain server")
-
-	ErrNotSynced = errors.New("wallet is not synchronized with the chain server")
-)
 
 // networkDir returns the directory name of a network directory to hold wallet
 // files.
@@ -190,7 +187,7 @@ func (w *Wallet) ListenDisconnectedBlocks() (<-chan keystore.BlockStamp, error)
 	return w.disconnectedBlocks, nil
 }
 
-// ListenDisconnectedBlocks returns a channel that passes the current lock state
+// ListenKeystoreLockStatus returns a channel that passes the current lock state
 // of the wallet keystore anytime the keystore is locked or unlocked.  The value
 // is true for locked, and false for unlocked.  The channel must be read, or
 // other wallet methods will block.
@@ -429,7 +426,7 @@ func (w *Wallet) WaitForChainSync() {
 	<-w.chainSynced
 }
 
-// SynchedChainTip returns the hash and height of the block of the most
+// SyncedChainTip returns the hash and height of the block of the most
 // recently seen block in the main chain.  It returns errors if the
 // wallet has not yet been marked as synched with the chain.
 func (w *Wallet) SyncedChainTip() (*keystore.BlockStamp, error) {
@@ -522,9 +519,13 @@ out:
 	w.wg.Done()
 }
 
-func (w *Wallet) CreateSimpleTx(pairs map[string]btcutil.Amount,
+// CreateSimpleTx creates a new signed transaction spending unspent P2PKH
-	minconf int) (*CreatedTx, error) {
+// outputs with at laest minconf confirmations spending to any number of
-
+// address/amount pairs.  Change and an appropiate transaction fee are
+// automatically included, if necessary.  All transaction creation through
+// this function is serialized to prevent the creation of many transactions
+// which spend the same outputs.
+func (w *Wallet) CreateSimpleTx(pairs map[string]btcutil.Amount, minconf int) (*CreatedTx, error) {
 	req := createTxRequest{
 		pairs:   pairs,
 		minconf: minconf,
@@ -547,6 +548,11 @@ type (
 		err      chan error
 	}
 
+	// HeldUnlock is a tool to prevent the wallet from automatically
+	// locking after some timeout before an operation which needed
+	// the unlocked wallet has finished.  Any aquired HeldUnlock
+	// *must* be released (preferably with a defer) or the wallet
+	// will forever remain unlocked.
 	HeldUnlock chan struct{}
 )
 
@@ -653,7 +659,12 @@ func (w *Wallet) Locked() bool {
 	return <-w.lockState
 }
 
-// HoldUnlock prevents the wallet from being locked,
+// HoldUnlock prevents the wallet from being locked.  The HeldUnlock object
+// *must* be released, or the wallet will forever remain unlocked.
+//
+// TODO: To prevent the above scenario, perhaps closures should be passed
+// to the walletLocker goroutine and disallow callers from explicitly
+// handling the locking mechanism.
 func (w *Wallet) HoldUnlock() (HeldUnlock, error) {
 	req := make(chan HeldUnlock)
 	w.holdUnlockRequests <- req