From 503f591e8818f7bc579862e7e842ffdde0347de0 Mon Sep 17 00:00:00 2001 From: Josh Rickmar Date: Tue, 12 Nov 2013 14:53:38 -0500 Subject: [PATCH] Process tx notifications before new blocks. This change modifies the order in which transaction to watched addresses are processed and when frontend notifications occur. Due to btcd notifying all transactions before sending the blockconnected notification, the UTXO and transaction stores can be modified without sending any frontend notifications, and then a single frontend notification is sent when the blockconnected notification arrives. The order in which each file is synced to disk was also changed to write out the UTXO and transaction stores before writing the wallet. This is to prevent a race where wallet closes after writing the dirty wallet, but before the dirty UTXO store is written. In this situation, newly added UTXOs will be missed and not found again on the next wallet open during the rescan. Writing the wallet (which holds the synced-to-block information) last prevents this. An issue where the unconfirmed change UTXO created from a new transaction never being properly notified to frontends is fixed now as well. --- cmd.go | 17 +++-------------- cmdmgr.go | 3 ++- disksync.go | 37 +++++++++++++++++++++---------------- sockets.go | 45 +++++++++++++++++++++++++++++++++++---------- 4 files changed, 61 insertions(+), 41 deletions(-) diff --git a/cmd.go b/cmd.go index 6ee5554..44a97ab 100644 --- a/cmd.go +++ b/cmd.go @@ -652,10 +652,6 @@ func (w *BtcWallet) newBlockTxOutHandler(result interface{}, e *btcjson.Error) b w.TxStore.dirty = true w.TxStore.Unlock() - if err = w.writeDirtyToDisk(); err != nil { - log.Errorf("cannot sync dirty wallet: %v", err) - } - // Add to UtxoStore if unspent. if !spent { // First, iterate through all stored utxos. If an unconfirmed utxo @@ -677,9 +673,6 @@ func (w *BtcWallet) newBlockTxOutHandler(result interface{}, e *btcjson.Error) b w.UtxoStore.dirty = true w.UtxoStore.Unlock() - if err = w.writeDirtyToDisk(); err != nil { - log.Errorf("cannot sync dirty wallet: %v", err) - } return false } } @@ -701,14 +694,10 @@ func (w *BtcWallet) newBlockTxOutHandler(result interface{}, e *btcjson.Error) b w.UtxoStore.s = append(w.UtxoStore.s, u) w.UtxoStore.dirty = true w.UtxoStore.Unlock() - if err = w.writeDirtyToDisk(); err != nil { - log.Errorf("cannot sync dirty wallet: %v", err) - } - confirmed := w.CalculateBalance(1) - unconfirmed := w.CalculateBalance(0) - confirmed - NotifyWalletBalance(frontendNotificationMaster, w.name, confirmed) - NotifyWalletBalanceUnconfirmed(frontendNotificationMaster, w.name, unconfirmed) + // If this notification came from mempool (TODO: currently + // unimplemented) notify the new unconfirmed balance immediately. + // Otherwise, wait until the blockconnection notifiation is processed. } // Never remove this handler. diff --git a/cmdmgr.go b/cmdmgr.go index bce41ed..9225a38 100644 --- a/cmdmgr.go +++ b/cmdmgr.go @@ -521,7 +521,8 @@ func handleSendRawTxReply(frontend chan []byte, icmd btcjson.Cmd, log.Errorf("cannot sync dirty wallet: %v", err) } - // Notify all frontends of new account balances. + // Notify all frontends of account's new unconfirmed and + // confirmed balance. confirmed := w.CalculateBalance(1) unconfirmed := w.CalculateBalance(0) - confirmed NotifyWalletBalance(frontendNotificationMaster, w.name, confirmed) diff --git a/disksync.go b/disksync.go index 6f5639c..85d9c77 100644 --- a/disksync.go +++ b/disksync.go @@ -74,27 +74,32 @@ func (w *BtcWallet) writeDirtyToDisk() error { txfilepath := filepath.Join(wdir, "tx.bin") utxofilepath := filepath.Join(wdir, "utxo.bin") - // Wallet - if w.dirty { - w.mtx.Lock() - defer w.mtx.Unlock() - tmpfilepath := wfilepath + "-" + timeStr + // UTXOs and transactions are synced to disk first. This prevents + // any races from saving a wallet marked to be synced with block N + // and btcwallet closing while the UTXO and Tx files are only synced + // with block N-1. + + // UTXOs + if w.UtxoStore.dirty { + w.UtxoStore.Lock() + defer w.UtxoStore.Unlock() + tmpfilepath := utxofilepath + "-" + timeStr tmpfile, err := os.Create(tmpfilepath) if err != nil { return err } - if _, err = w.WriteTo(tmpfile); err != nil { + if _, err = w.UtxoStore.s.WriteTo(tmpfile); err != nil { return err } tmpfile.Close() // TODO(jrick): this should be atomic on *nix, but is not on // Windows. Use _windows.go to provide atomic renames. - if err = os.Rename(tmpfilepath, wfilepath); err != nil { + if err = os.Rename(tmpfilepath, utxofilepath); err != nil { return err } - w.dirty = false + w.UtxoStore.dirty = false } // Transactions @@ -120,27 +125,27 @@ func (w *BtcWallet) writeDirtyToDisk() error { w.TxStore.dirty = false } - // UTXOs - if w.UtxoStore.dirty { - w.UtxoStore.Lock() - defer w.UtxoStore.Unlock() - tmpfilepath := utxofilepath + "-" + timeStr + // Wallet + if w.dirty { + w.mtx.Lock() + defer w.mtx.Unlock() + tmpfilepath := wfilepath + "-" + timeStr tmpfile, err := os.Create(tmpfilepath) if err != nil { return err } - if _, err = w.UtxoStore.s.WriteTo(tmpfile); err != nil { + if _, err = w.WriteTo(tmpfile); err != nil { return err } tmpfile.Close() // TODO(jrick): this should be atomic on *nix, but is not on // Windows. Use _windows.go to provide atomic renames. - if err = os.Rename(tmpfilepath, utxofilepath); err != nil { + if err = os.Rename(tmpfilepath, wfilepath); err != nil { return err } - w.UtxoStore.dirty = false + w.dirty = false } return nil diff --git a/sockets.go b/sockets.go index 17f1e09..ed50f6f 100644 --- a/sockets.go +++ b/sockets.go @@ -381,10 +381,7 @@ func NtfnBlockConnected(n btcws.Notification) { return } - // btcd notifies btcwallet about transactions first, and then sends - // the block notification. This prevents any races from saving a - // synced-to block before all notifications from the block have been - // processed. + // Update the blockstamp for the newly-connected block. bs := &wallet.BlockStamp{ Height: bcn.Height, Hash: *hash, @@ -392,16 +389,44 @@ func NtfnBlockConnected(n btcws.Notification) { curBlock.Lock() curBlock.BlockStamp = *bs curBlock.Unlock() + + // btcd notifies btcwallet about transactions first, and then sends + // the new block notification. New balance notifications for txs + // in blocks are therefore sent here after all tx notifications + // have arrived. + // + // TODO(jrick): send frontend tx notifications once that's + // implemented. for _, w := range wallets.m { - // We do not write synced info immediatelly out to disk. - // If btcd is performing an IBD, that would result in - // writing out the wallet to disk for each processed block. - // Instead, mark as dirty and let another goroutine process - // the dirty wallet. + // Mark wallet as being synced with the new blockstamp. w.mtx.Lock() w.Wallet.SetSyncedWith(bs) - w.dirty = true w.mtx.Unlock() + + // The UTXO store will be dirty if it was modified + // from a tx notification. + if w.UtxoStore.dirty { + // Notify all frontends of account's new unconfirmed + // and confirmed balance. + confirmed := w.CalculateBalance(1) + unconfirmed := w.CalculateBalance(0) - confirmed + NotifyWalletBalance(frontendNotificationMaster, + w.name, confirmed) + NotifyWalletBalanceUnconfirmed(frontendNotificationMaster, + w.name, unconfirmed) + } + + // The account is intentionaly not immediately synced to disk. + // If btcd is performing an IBD, writing the wallet file for + // each newly-connected block would result in too many + // unnecessary disk writes. The UTXO and transaction stores + // could be written, but in the case of btcwallet closing + // before writing the dirty wallet, both would have to be + // pruned anyways. + // + // Instead, the wallet is queued to be written to disk at the + // next scheduled disk sync. + w.dirty = true dirtyWallets.Lock() dirtyWallets.m[w] = true dirtyWallets.Unlock()