diff --git a/btcwallet.go b/btcwallet.go index 08aa705..716ae43 100644 --- a/btcwallet.go +++ b/btcwallet.go @@ -138,13 +138,12 @@ 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 } + // 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) diff --git a/chain/chain.go b/chain/chain.go index 863142e..afbf437 100644 --- a/chain/chain.go +++ b/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. diff --git a/createtx.go b/createtx.go index 3d975b6..be88a9c 100644 --- a/createtx.go +++ b/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) diff --git a/txstore/json.go b/txstore/json.go index d78d5bd..4c81d39 100644 --- a/txstore/json.go +++ b/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() diff --git a/txstore/serialization.go b/txstore/serialization.go index 201ad2e..2b2c7a1 100644 --- a/txstore/serialization.go +++ b/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 { diff --git a/votingpool/pool_test.go b/votingpool/pool_test.go index b6f02b6..f3d183f 100644 --- a/votingpool/pool_test.go +++ b/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) } diff --git a/waddrmgr/manager.go b/waddrmgr/manager.go index df964b3..0b625dd 100644 --- a/waddrmgr/manager.go +++ b/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. diff --git a/wallet.go b/wallet.go index 32e3260..217f22e 100644 --- a/wallet.go +++ b/wallet.go @@ -36,13 +36,10 @@ import ( "github.com/btcsuite/btcwallet/txstore" ) -var ( - ErrNoWalletFiles = errors.New("no wallet files") - - ErrWalletExists = errors.New("wallet already exists") - - ErrNotSynced = errors.New("wallet is not synchronized with the chain server") -) +// ErrNotSynced describes an error where an operation cannot complete +// due wallet being out of sync (and perhaps currently syncing with) +// the remote chain server. +var 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, - minconf int) (*CreatedTx, error) { - +// CreateSimpleTx creates a new signed transaction spending unspent P2PKH +// 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