blockchain: fix inconsistent utxocache and database on reorg

[kcalvinalvin]

During reorgs, the assumption being made was that the utxocache is ignored and we're directly accessing the database for utxo set modifications.

However, after a block disconnect, the BlockChain.chainLock, the lock that prevents modifications to the utxo set, was being freed for the callback functions for the blockchain notifications.

btcd/blockchain/chain.go

Lines 853 to 858 in 13152b3

	// 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()

If a caller calls either of the two methods below, the utxocache could be loaded with utxos that are fetched from the database:

btcd/blockchain/utxoviewpoint.go

Lines 687 to 693 in 13152b3

	// FetchUtxoView loads unspent transaction outputs for the inputs referenced by
	// the passed transaction from the point of view of the end of the main chain.
	// It also attempts to fetch the utxos for the outputs of the transaction itself
	// so the returned view can be examined for duplicate transactions.
	//
	// This function is safe for concurrent access however the returned view is NOT.
	func (b *BlockChain) FetchUtxoView(tx *btcutil.Tx) (*UtxoViewpoint, error) {

btcd/blockchain/utxoviewpoint.go

Lines 722 to 732 in 13152b3

	// FetchUtxoEntry loads and returns the requested unspent transaction output
	// from the point of view of the end of the main chain.
	//
	// NOTE: Requesting an output for which there is no data will NOT return an
	// error. Instead both the entry and the error will be nil. This is done to
	// allow pruning of spent transaction outputs. In practice this means the
	// caller must check if the returned entry is nil before invoking methods on it.
	//
	// This function is safe for concurrent access however the returned entry (if
	// any) is NOT.
	func (b *BlockChain) FetchUtxoEntry(outpoint wire.OutPoint) (*UtxoEntry, error) {

Since the UTXOs fetched here are considered unmodified and therefore wouldn't be saved in the database when the utxoCache is flushed at the end of a reorganization, if a caller makes a call to the above two functions during a reorg, they could be mistakenly think that a utxo exists when it was removed from the utxo set.

Example:

There's a chain like so where a utxo created in b1001 is spent in b1002. Blocks b1002 and b1001 are being disconnected out by a longer chain.

b1000() -> b1001(b1000.tx[1].out[0]) -> b1002(b1001.tx[1].out[0])

Mempool tries to fetch b1001.tx[1].out[0] three times in total. 2, 8 and 11.
At 6 the utxo b1001.tx[1].out[0] get loaded up to the cache. The cache never gets reset and that utxo gets fetched at 12. When the reorg finishes, the mempool can no longer fetch b1001.tx[1].out[0]

         database                 UTXO cache                 BlockChain                                     Mempool

1.     {b1002.tx[0].out[0]}       {}                         chainLock.Lock() (for reorg)
2.     {b1002.tx[0].out[0]}       {}                                                                        Try to acquire lock (fetch output b1001.tx[1].out[0])
4.     {b1001.tx[1].out[0]}       {}                         disconnect b1002                               Try to acquire lock (fetch output b1001.tx[1].out[0])
5.     {}                         {}                         chainLock.Unlock() (subscriber callback)       Acquired lock (fetch output b1001.tx[1].out[0])
6.     {}                         {b1001.tx[1].out[0]}                                                      Fetched b1001.tx[1].out[0] (should happen)
7.     {}                         {b1001.tx[1].out[0]}       chainLock.Lock()   (finished callback)
8.     {}                         {b1001.tx[1].out[0]}                                                      Try to acquire lock (fetch output b1001.tx[1].out[0])
9.     {}                         {b1001.tx[1].out[0]}                                                      Try to acquire lock (fetch output b1001.tx[1].out[0])
10.    {b1000.tx[1].out[0]}       {b1001.tx[1].out[0]}       disconnect b1001                               Try to acquire lock (fetch output b1001.tx[1].out[0])
11.    {b1000.tx[1].out[0]}       {b1001.tx[1].out[0]}       chainLock.Unlock() (subscriber callback)       Acquired lock (fetch output b1001.tx[1].out[0])
12.    {b1000.tx[1].out[0]}       {b1001.tx[1].out[0]}                                                      Fetched b1001.tx[1].out[0] (shouldn't happen)
13.    {b1000.tx[1].out[0]}       {b1001.tx[1].out[0]}       chainLock.Lock()   (finished callback)
14.    {b1000.tx[1].out[0]}       {}                         Flush UTXO set
15.    {b1000.tx[1].out[0]}       {}                                                                        Try to acquire lock (fetch output b1001.tx[1].out[0])
16.    {b1000.tx[1].out[0]}       {}                                                                        Acquired lock (fetch output b1001.tx[1].out[0])
17.    {b1000.tx[1].out[0]}       {}                                                                        Did not find b1001.tx[1].out[0] (should happen)

[coveralls]

Pull Request Test Coverage Report for Build 8184050728

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 72 of 78 (92.31%) changed or added relevant lines in 4 files are covered.
• 22 unchanged lines in 4 files lost coverage.
• Overall coverage increased (+0.003%) to 56.779%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
blockchain/fullblocktests/generate.go	55	57	96.49%
blockchain/chain.go	14	18	77.78%

Files with Coverage Reduction	New Missed Lines	%
blockchain/chainio.go	2	60.99%
connmgr/connmanager.go	2	86.27%
blockchain/utxoviewpoint.go	9	72.54%
peer/peer.go	9	73.94%

Totals
Change from base Build 8117173129:	0.003%
Covered Lines:	29309
Relevant Lines:	51619

---

💛 - Coveralls

[guggero]

Thanks a lot for the fix! LGTM 🎉

[Roasbeef]

Nice work tracking down this fix! Have some questions around some lingering logic that looks redundant after this change, and a panic I get in the unit tests w/ the first commit (new tests), but w/o the second (the fix).

[Roasbeef]

LGTM 🍖