From 4e47e4e7f13af76ddb4125dda4efe90f1e7b05d4 Mon Sep 17 00:00:00 2001
From: "Johan T. Halseth" <johanth@gmail.com>
Date: Fri, 13 Jul 2018 12:35:29 +0200
Subject: [PATCH 1/2] chainntnfs/bitcoindnotify: filter out mempool spends from
 relevant txs

This commit fix a bug within the bitcoind notifier logic, which would
ignore the passed mempool argument, and notify spentness whether the
spending transaction was confirmed or not. The logic used to fix this is
similar to what is already done for the btcd backend.
---
 chainntnfs/bitcoindnotify/bitcoind.go | 41 ++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go
index 2ea3deba..b6362746 100644
--- a/chainntnfs/bitcoindnotify/bitcoind.go
+++ b/chainntnfs/bitcoindnotify/bitcoind.go
@@ -352,16 +352,40 @@ func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int3
 			// TODO(roasbeef): after change to
 			// loadfilter, only notify on block
 			// inclusion?
+
+			confirmedSpend := false
 			if tx.Block != nil {
+				confirmedSpend = true
 				spendDetails.SpendingHeight = tx.Block.Height
 			} else {
 				spendDetails.SpendingHeight = bestHeight + 1
 			}
 
-			for _, ntfn := range clients {
-				chainntnfs.Log.Infof("Dispatching "+
+			// Keep spendNotifications that are
+			// waiting for a confirmation around.
+			// They will be notified when we find
+			// the spend within a block.
+			rem := make(map[uint64]*spendNotification)
+			for c, ntfn := range clients {
+				// If this is a mempool spend,
+				// and this client didn't want
+				// to be notified on mempool
+				// spends, store it for later.
+				if !confirmedSpend && !ntfn.mempool {
+					rem[c] = ntfn
+					continue
+				}
+
+				confStr := "unconfirmed"
+				if confirmedSpend {
+					confStr = "confirmed"
+				}
+
+				chainntnfs.Log.Infof("Dispatching %s "+
 					"spend notification for "+
-					"outpoint=%v", ntfn.targetOutpoint)
+					"outpoint=%v at height %v",
+					confStr, ntfn.targetOutpoint,
+					spendDetails.SpendingHeight)
 				ntfn.spendChan <- spendDetails
 
 				// Close spendChan to ensure that any calls to Cancel will not
@@ -370,6 +394,12 @@ func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int3
 				close(ntfn.spendChan)
 			}
 			delete(b.spendNotifications, prevOut)
+
+			// If we had any clients left, add them
+			// back to the map.
+			if len(rem) > 0 {
+				b.spendNotifications[prevOut] = rem
+			}
 		}
 	}
 }
@@ -530,6 +560,8 @@ type spendNotification struct {
 	spendID uint64
 
 	heightHint uint32
+
+	mempool bool
 }
 
 // spendCancel is a message sent to the BitcoindNotifier when a client wishes
@@ -548,12 +580,13 @@ type spendCancel struct {
 // across the 'Spend' channel. The heightHint should represent the earliest
 // height in the chain where the transaction could have been spent in.
 func (b *BitcoindNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32, mempool bool) (*chainntnfs.SpendEvent, error) {
 
 	ntfn := &spendNotification{
 		targetOutpoint: outpoint,
 		spendChan:      make(chan *chainntnfs.SpendDetail, 1),
 		spendID:        atomic.AddUint64(&b.spendClientCounter, 1),
+		mempool:        mempool,
 	}
 
 	select {

From 07618ca0c6d16f7fa0d7dad333091118c46a2f5e Mon Sep 17 00:00:00 2001
From: "Johan T. Halseth" <johanth@gmail.com>
Date: Fri, 13 Jul 2018 12:35:29 +0200
Subject: [PATCH 2/2] chainntnfs/interface_test: increase wait time for mempool
 spends

This commit increases the time we wait for a spend client to notify a
mempool spend from 50ms to 10s. This is done to catch the case where
bitcoind would use up to 7 seconds before notifying about a mempool
spend, which wasn't caught by the test.
---
 chainntnfs/interface_test.go | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/chainntnfs/interface_test.go b/chainntnfs/interface_test.go
index c2bd8a13..cee8c514 100644
--- a/chainntnfs/interface_test.go
+++ b/chainntnfs/interface_test.go
@@ -425,14 +425,30 @@ func testSpendNotification(miner *rpctest.Harness,
 		t.Fatalf("tx not relayed to miner: %v", err)
 	}
 
-	// Make sure notifications are not yet sent.
+	// Make sure notifications are not yet sent. We launch a go routine for
+	// all the spend clients, such that we can wait for them all in
+	// parallel.
+	//
+	// Since bitcoind is at times very slow at notifying about txs in the
+	// mempool, we use a quite large timeout of 10 seconds.
+	// TODO(halseth): change this when mempool spends are removed.
+	mempoolSpendTimeout := 10 * time.Second
+	mempoolSpends := make(chan *chainntnfs.SpendDetail, numClients)
 	for _, c := range spendClients {
-		select {
-		case <-c.Spend:
-			t.Fatalf("did not expect to get notification before " +
-				"block was mined")
-		case <-time.After(50 * time.Millisecond):
-		}
+		go func(client *chainntnfs.SpendEvent) {
+			select {
+			case s := <-client.Spend:
+				mempoolSpends <- s
+			case <-time.After(mempoolSpendTimeout):
+			}
+		}(c)
+	}
+
+	select {
+	case <-mempoolSpends:
+		t.Fatalf("did not expect to get notification before " +
+			"block was mined")
+	case <-time.After(mempoolSpendTimeout):
 	}
 
 	// Now we mine a single block, which should include our spend. The