Merge branch 'master' into btcsuite#2106

CHANGES

@@ -782,7 +782,7 @@ Changes in 0.8.0-beta (Sun May 25 2014)
       recent reference client changes
       (https://github.com/conformal/btcd/issues/100)
     - Raise the maximum signature script size to support standard 15-of-15
-      multi-signature pay-to-sript-hash transactions with compressed pubkeys
+      multi-signature pay-to-script-hash transactions with compressed pubkeys
       to remain compatible with the reference client
       (https://github.com/conformal/btcd/issues/128)
     - Reduce max bytes allowed for a standard nulldata transaction to 40 for

addrmgr/addrmanager_internal_test.go

@@ -1,7 +1,6 @@
 package addrmgr
 
 import (
-	"io/ioutil"
 	"math/rand"
 	"net"
 	"os"
@@ -12,7 +11,7 @@ import (
 )
 
 // randAddr generates a *wire.NetAddressV2 backed by a random IPv4/IPv6
-// address.
+// address.  Some of the returned addresses may not be routable.
 func randAddr(t *testing.T) *wire.NetAddressV2 {
 	t.Helper()
 
@@ -40,6 +39,23 @@ func randAddr(t *testing.T) *wire.NetAddressV2 {
 	)
 }
 
+// routableRandAddr generates a *wire.NetAddressV2 backed by a random IPv4/IPv6
+// address that is always routable.
+func routableRandAddr(t *testing.T) *wire.NetAddressV2 {
+	t.Helper()
+
+	var addr *wire.NetAddressV2
+
+	// If the address is not routable, try again.
+	routable := false
+	for !routable {
+		addr = randAddr(t)
+		routable = IsRoutable(addr)
+	}
+
+	return addr
+}
+
 // assertAddr ensures that the two addresses match. The timestamp is not
 // checked as it does not affect uniquely identifying a specific address.
 func assertAddr(t *testing.T, got, expected *wire.NetAddressV2) {
@@ -91,7 +107,7 @@ func TestAddrManagerSerialization(t *testing.T) {
 
 	// We'll start by creating our address manager backed by a temporary
 	// directory.
-	tempDir, err := ioutil.TempDir("", "addrmgr")
+	tempDir, err := os.MkdirTemp("", "addrmgr")
 	if err != nil {
 		t.Fatalf("unable to create temp dir: %v", err)
 	}
@@ -104,9 +120,9 @@ func TestAddrManagerSerialization(t *testing.T) {
 
 	expectedAddrs := make(map[string]*wire.NetAddressV2, numAddrs)
 	for i := 0; i < numAddrs; i++ {
-		addr := randAddr(t)
+		addr := routableRandAddr(t)
 		expectedAddrs[NetAddressKey(addr)] = addr
-		addrMgr.AddAddress(addr, randAddr(t))
+		addrMgr.AddAddress(addr, routableRandAddr(t))
 	}
 
 	// Now that the addresses have been added, we should be able to retrieve
@@ -131,7 +147,7 @@ func TestAddrManagerV1ToV2(t *testing.T) {
 
 	// We'll start by creating our address manager backed by a temporary
 	// directory.
-	tempDir, err := ioutil.TempDir("", "addrmgr")
+	tempDir, err := os.MkdirTemp("", "addrmgr")
 	if err != nil {
 		t.Fatalf("unable to create temp dir: %v", err)
 	}
@@ -149,9 +165,9 @@ func TestAddrManagerV1ToV2(t *testing.T) {
 
 	expectedAddrs := make(map[string]*wire.NetAddressV2, numAddrs)
 	for i := 0; i < numAddrs; i++ {
-		addr := randAddr(t)
+		addr := routableRandAddr(t)
 		expectedAddrs[NetAddressKey(addr)] = addr
-		addrMgr.AddAddress(addr, randAddr(t))
+		addrMgr.AddAddress(addr, routableRandAddr(t))
 	}
 
 	// Then, we'll persist these addresses to disk and restart the address
@@ -168,7 +184,7 @@ func TestAddrManagerV1ToV2(t *testing.T) {
 	addrMgr.loadPeers()
 	addrs := addrMgr.getAddresses()
 	if len(addrs) != len(expectedAddrs) {
-		t.Fatalf("expected to find %d adddresses, found %d",
+		t.Fatalf("expected to find %d addresses, found %d",
 			len(expectedAddrs), len(addrs))
 	}
 	for _, addr := range addrs {

blockchain/accept.go

@@ -84,9 +84,11 @@ func (b *BlockChain) maybeAcceptBlock(block *btcutil.Block, flags BehaviorFlags)
 	// Notify the caller that the new block was accepted into the block
 	// chain.  The caller would typically want to react by relaying the
 	// inventory to other peers.
-	b.chainLock.Unlock()
-	b.sendNotification(NTBlockAccepted, block)
-	b.chainLock.Lock()
+	func() {
+		b.chainLock.Unlock()
+		defer b.chainLock.Lock()
+		b.sendNotification(NTBlockAccepted, block)
+	}()
 
 	return isMainChain, nil
 }

blockchain/chain.go

@@ -569,7 +569,7 @@ func (b *BlockChain) getReorganizeNodes(node *blockNode) (*list.List, *list.List
 //
 // This function MUST be called with the chain state lock held (for writes).
 func (b *BlockChain) connectBlock(node *blockNode, block *btcutil.Block,
-	view *UtxoViewpoint, stxos []SpentTxOut) error {
+	stxos []SpentTxOut) error {
 
 	// Make sure it's extending the end of the best chain.
 	prevHash := &block.MsgBlock().Header.PrevBlock
@@ -611,18 +611,6 @@ func (b *BlockChain) connectBlock(node *blockNode, block *btcutil.Block,
 		curTotalTxns+numTxns, CalcPastMedianTime(node),
 	)
 
-	// If a utxoviewpoint was passed in, we'll be writing that viewpoint
-	// directly to the database on disk.  In order for the database to be
-	// consistent, we must flush the cache before writing the viewpoint.
-	if view != nil {
-		err = b.db.Update(func(dbTx database.Tx) error {
-			return b.utxoCache.flush(dbTx, FlushRequired, state)
-		})
-		if err != nil {
-			return err
-		}
-	}
-
 	// Atomically insert info into the database.
 	err = b.db.Update(func(dbTx database.Tx) error {
 		// If the pruneTarget isn't 0, we should attempt to delete older blocks
@@ -676,16 +664,6 @@ func (b *BlockChain) connectBlock(node *blockNode, block *btcutil.Block,
 			return err
 		}
 
-		// Update the utxo set using the state of the utxo view.  This
-		// entails removing all of the utxos spent and adding the new
-		// ones created by the block.
-		//
-		// A nil viewpoint is a no-op.
-		err = dbPutUtxoView(dbTx, view)
-		if err != nil {
-			return err
-		}
-
 		// Update the transaction spend journal by adding a record for
 		// the block that contains all txos spent by it.
 		err = dbPutSpendJournalEntry(dbTx, block.Hash(), stxos)
@@ -709,12 +687,6 @@ func (b *BlockChain) connectBlock(node *blockNode, block *btcutil.Block,
 		return err
 	}
 
-	// Prune fully spent entries and mark all entries in the view unmodified
-	// now that the modifications have been committed to the database.
-	if view != nil {
-		view.commit()
-	}
-
 	// This node is now the end of the best chain.
 	b.bestChain.SetTip(node)
 
@@ -730,9 +702,11 @@ func (b *BlockChain) connectBlock(node *blockNode, block *btcutil.Block,
 	// Notify the caller that the block was connected to the main chain.
 	// The caller would typically want to react with actions such as
 	// updating wallets.
-	b.chainLock.Unlock()
-	b.sendNotification(NTBlockConnected, block)
-	b.chainLock.Lock()
+	func() {
+		b.chainLock.Unlock()
+		defer b.chainLock.Lock()
+		b.sendNotification(NTBlockConnected, block)
+	}()
 
 	// Since we may have changed the UTXO cache, we make sure it didn't exceed its
 	// maximum size.  If we're pruned and have flushed already, this will be a no-op.
@@ -796,6 +770,15 @@ func (b *BlockChain) disconnectBlock(node *blockNode, block *btcutil.Block, view
 			return err
 		}
 
+		// Flush the cache on every disconnect.  Since the code for
+		// reorganization modifies the database directly, the cache
+		// will be left in an inconsistent state if we don't flush it
+		// prior to the dbPutUtxoView that happens below.
+		err = b.utxoCache.flush(dbTx, FlushRequired, state)
+		if err != nil {
+			return err
+		}
+
 		// Update the utxo set using the state of the utxo view.  This
 		// entails restoring all of the utxos spent and removing the new
 		// ones created by the block.
@@ -853,9 +836,11 @@ func (b *BlockChain) disconnectBlock(node *blockNode, block *btcutil.Block, view
 	// Notify the caller that the block was disconnected from the main
 	// chain.  The caller would typically want to react with actions such as
 	// updating wallets.
-	b.chainLock.Unlock()
-	b.sendNotification(NTBlockDisconnected, block)
-	b.chainLock.Lock()
+	func() {
+		b.chainLock.Unlock()
+		defer b.chainLock.Lock()
+		b.sendNotification(NTBlockDisconnected, block)
+	}()
 
 	return nil
 }
@@ -880,22 +865,16 @@ func countSpentOutputs(block *btcutil.Block) int {
 //
 // This function may modify node statuses in the block index without flushing.
 //
+// This function never leaves the utxo set in an inconsistent state for block
+// disconnects.
+//
 // This function MUST be called with the chain state lock held (for writes).
 func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error {
 	// Nothing to do if no reorganize nodes were provided.
 	if detachNodes.Len() == 0 && attachNodes.Len() == 0 {
 		return nil
 	}
 
-	// The rest of the reorg depends on all STXOs already being in the database
-	// so we flush before reorg.
-	err := b.db.Update(func(dbTx database.Tx) error {
-		return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
-	})
-	if err != nil {
-		return err
-	}
-
 	// Ensure the provided nodes match the current best chain.
 	tip := b.bestChain.Tip()
 	if detachNodes.Len() != 0 {
@@ -957,7 +936,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 
 		// Load all of the utxos referenced by the block that aren't
 		// already in the view.
-		err = view.fetchInputUtxos(b.db, nil, block)
+		err = view.fetchInputUtxos(b.utxoCache, block)
 		if err != nil {
 			return err
 		}
@@ -1024,7 +1003,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		// checkConnectBlock gets skipped, we still need to update the UTXO
 		// view.
 		if b.index.NodeStatus(n).KnownValid() {
-			err = view.fetchInputUtxos(b.db, nil, block)
+			err = view.fetchInputUtxos(b.utxoCache, block)
 			if err != nil {
 				return err
 			}
@@ -1061,11 +1040,21 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 		newBest = n
 	}
 
+	// Flush the utxo cache for the block disconnect below.  The disconnect
+	// code assumes that it's directly modifying the database so the cache
+	// will be left in an inconsistent state.  It needs to be flushed beforehand
+	// in order for that to not happen.
+	err := b.db.Update(func(dbTx database.Tx) error {
+		return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
+	})
+	if err != nil {
+		return err
+	}
+
 	// Reset the view for the actual connection code below.  This is
 	// required because the view was previously modified when checking if
 	// the reorg would be successful and the connection code requires the
-	// view to be valid from the viewpoint of each block being connected or
-	// disconnected.
+	// view to be valid from the viewpoint of each block being disconnected.
 	view = NewUtxoViewpoint()
 	view.SetBestHash(&b.bestChain.Tip().hash)
 
@@ -1076,7 +1065,7 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 
 		// Load all of the utxos referenced by the block that aren't
 		// already in the view.
-		err := view.fetchInputUtxos(b.db, nil, block)
+		err := view.fetchInputUtxos(b.utxoCache, block)
 		if err != nil {
 			return err
 		}
@@ -1089,51 +1078,39 @@ func (b *BlockChain) reorganizeChain(detachNodes, attachNodes *list.List) error
 			return err
 		}
 
-		// Update the database and chain state.
+		// Update the database and chain state. The cache will be flushed
+		// here before the utxoview modifications happen to the database.
 		err = b.disconnectBlock(n, block, view)
 		if err != nil {
 			return err
 		}
 	}
 
-	// Connect the new best chain blocks.
+	// Connect the new best chain blocks using the utxocache directly.  It's more
+	// efficient and since we already checked that the blocks are correct and that
+	// the transactions connect properly, it's ok to access the cache.  If we suddenly
+	// crash here, we are able to recover as well.
 	for i, e := 0, attachNodes.Front(); e != nil; i, e = i+1, e.Next() {
 		n := e.Value.(*blockNode)
 		block := attachBlocks[i]
 
-		// Load all of the utxos referenced by the block that aren't
-		// already in the view.
-		err := view.fetchInputUtxos(b.db, nil, block)
-		if err != nil {
-			return err
-		}
-
-		// Update the view to mark all utxos referenced by the block
+		// Update the cache to mark all utxos referenced by the block
 		// as spent and add all transactions being created by this block
 		// to it.  Also, provide an stxo slice so the spent txout
 		// details are generated.
 		stxos := make([]SpentTxOut, 0, countSpentOutputs(block))
-		err = view.connectTransactions(block, &stxos)
+		err = b.utxoCache.connectTransactions(block, &stxos)
 		if err != nil {
 			return err
 		}
 
 		// Update the database and chain state.
-		err = b.connectBlock(n, block, view, stxos)
+		err = b.connectBlock(n, block, stxos)
 		if err != nil {
 			return err
 		}
 	}
 
-	// We call the flush at the end to update the last flush hash to the new
-	// best tip.
-	err = b.db.Update(func(dbTx database.Tx) error {
-		return b.utxoCache.flush(dbTx, FlushRequired, b.BestSnapshot())
-	})
-	if err != nil {
-		return err
-	}
-
 	// Log the point where the chain forked and old and new best chain
 	// heads.
 	if forkNode != nil {
@@ -1225,7 +1202,7 @@ func (b *BlockChain) connectBestChain(node *blockNode, block *btcutil.Block, fla
 		}
 
 		// Connect the block to the main chain.
-		err = b.connectBlock(node, block, nil, stxos)
+		err = b.connectBlock(node, block, stxos)
 		if err != nil {
 			// If we got hit with a rule error, then we'll mark
 			// that status of the block as invalid and flush the