Merge pull request #1531 from halseth/only-vonfirmed-spends

Only act on confirmed spends

breacharbiter.go

@@ -357,7 +357,7 @@ func (b *breachArbiter) waitForSpendEvent(breachInfo *retributionInfo,
 			var err error
 			spendNtfn, err = b.cfg.Notifier.RegisterSpendNtfn(
 				&breachedOutput.outpoint,
-				breachInfo.breachHeight, true,
+				breachInfo.breachHeight,
 			)
 			if err != nil {
 				brarLog.Errorf("unable to check for spentness "+

chainntnfs/bitcoindnotify/bitcoind.go

@@ -7,7 +7,6 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/btcsuite/btcd/btcjson"
 	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
@@ -16,6 +15,7 @@ import (
 	"github.com/btcsuite/btcutil"
 	"github.com/btcsuite/btcwallet/chain"
 	"github.com/btcsuite/btcwallet/wtxmgr"
+	"github.com/lightningnetwork/lnd/chainntnfs"
 )
 
 const (
@@ -331,6 +331,14 @@ out:
 // handleRelevantTx notifies any clients of a relevant transaction.
 func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int32) {
 	msgTx := tx.TxRecord.MsgTx
+
+	// We only care about notifying on confirmed spends, so in case this is
+	// a mempool spend, we can continue, and wait for the spend to appear
+	// in chain.
+	if tx.Block == nil {
+		return
+	}
+
 	// First, check if this transaction spends an output
 	// that has an existing spend notification for it.
 	for i, txIn := range msgTx.TxIn {
@@ -349,57 +357,22 @@ func (b *BitcoindNotifier) handleRelevantTx(tx chain.RelevantTx, bestHeight int3
 				SpendingTx:        &msgTx,
 				SpenderInputIndex: uint32(i),
 			}
-			// 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
-			}
 
-			// Keep spendNotifications that are
+			for _, ntfn := range clients {
-			// waiting for a confirmation around.
+				chainntnfs.Log.Infof("Dispatching confirmed "+
-			// They will be notified when we find
+					"spend notification for outpoint=%v "+
-			// the spend within a block.
+					"at height %v", ntfn.targetOutpoint,
-			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 at height %v",
-					confStr, ntfn.targetOutpoint,
 					spendDetails.SpendingHeight)
 				ntfn.spendChan <- spendDetails
 
-				// Close spendChan to ensure that any calls to Cancel will not
+				// Close spendChan to ensure that any calls to
-				// block. This is safe to do since the channel is buffered, and the
+				// Cancel will not block. This is safe to do
+				// since the channel is buffered, and the
 				// message can still be read by the receiver.
 				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
-			}
 		}
 	}
 }
@@ -560,8 +533,6 @@ type spendNotification struct {
 	spendID uint64
 
 	heightHint uint32
-
-	mempool bool
 }
 
 // spendCancel is a message sent to the BitcoindNotifier when a client wishes
@@ -580,13 +551,12 @@ 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, mempool bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 
 	ntfn := &spendNotification{
 		targetOutpoint: outpoint,
 		spendChan:      make(chan *chainntnfs.SpendDetail, 1),
 		spendID:        atomic.AddUint64(&b.spendClientCounter, 1),
-		mempool:        mempool,
 	}
 
 	select {

chainntnfs/btcdnotify/btcd.go

@@ -377,6 +377,14 @@ out:
 		// rescan spends. It might get removed entirely in the future.
 		case item := <-b.txUpdates.ChanOut():
 			newSpend := item.(*txUpdate)
+
+			// We only care about notifying on confirmed spends, so
+			// in case this is a mempool spend, we can continue,
+			// and wait for the spend to appear in chain.
+			if newSpend.details == nil {
+				continue
+			}
+
 			spendingTx := newSpend.tx
 
 			// First, check if this transaction spends an output
@@ -397,57 +405,27 @@ out:
 						SpendingTx:        spendingTx.MsgTx(),
 						SpenderInputIndex: uint32(i),
 					}
-					// TODO(roasbeef): after change to
-					// loadfilter, only notify on block
-					// inclusion?
-
-					confirmedSpend := false
-					if newSpend.details != nil {
-						confirmedSpend = true
 					spendDetails.SpendingHeight = newSpend.details.Height
-					} else {
-						spendDetails.SpendingHeight = currentHeight + 1
-					}
 
-					// Keep spendNotifications that are
+					for _, ntfn := range clients {
-					// waiting for a confirmation around.
+						chainntnfs.Log.Infof("Dispatching "+
-					// They will be notified when we find
+							"confirmed spend "+
-					// the spend within a block.
+							"notification for "+
-					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 at height %v",
-							confStr, ntfn.targetOutpoint,
+							ntfn.targetOutpoint,
 							spendDetails.SpendingHeight)
 						ntfn.spendChan <- spendDetails
 
-						// Close spendChan to ensure that any calls to Cancel will not
+						// Close spendChan to ensure
-						// block. This is safe to do since the channel is buffered, and the
+						// that any calls to Cancel
-						// message can still be read by the receiver.
+						// will not block. This is safe
+						// to do since the channel is
+						// buffered, and the message
+						// can still be read by the
+						// receiver.
 						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
-					}
 				}
 			}
 
@@ -611,8 +589,6 @@ func (b *BtcdNotifier) handleBlockConnected(newBlock *filteredBlock) error {
 				continue
 			}
 
-			// TODO(roasbeef): many integration tests expect spend to be
-			// notified within the mempool.
 			spendDetails := &chainntnfs.SpendDetail{
 				SpentOutPoint:     &prevOut,
 				SpenderTxHash:     &txSha,
@@ -673,8 +649,6 @@ type spendNotification struct {
 
 	spendID uint64
 
-	mempool bool
-
 	heightHint uint32
 }
 
@@ -694,14 +668,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 *BtcdNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, mempool bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 
 	ntfn := &spendNotification{
 		targetOutpoint: outpoint,
 		spendChan:      make(chan *chainntnfs.SpendDetail, 1),
 		spendID:        atomic.AddUint64(&b.spendClientCounter, 1),
 		heightHint:     heightHint,
-		mempool:        mempool,
 	}
 
 	select {

chainntnfs/interface.go

@@ -43,15 +43,13 @@ type ChainNotifier interface {
 	// The heightHint denotes the earliest height in the blockchain in
 	// which the target output could have been created.
 	//
-	// NOTE: If mempool=true is set, then this notification should be
+	// NOTE: The notification should only be triggered when the spending
-	// triggered on a best-effort basis once the transaction is *seen* on
+	// transaction receives a single confirmation.
-	// the network. If mempool=false, it should only be triggered when the
-	// spending transaction receives a single confirmation.
 	//
 	// NOTE: Dispatching notifications to multiple clients subscribed to a
 	// spend of the same outpoint MUST be supported.
-	RegisterSpendNtfn(outpoint *wire.OutPoint, heightHint uint32,
+	RegisterSpendNtfn(outpoint *wire.OutPoint,
-		mempool bool) (*SpendEvent, error)
+		heightHint uint32) (*SpendEvent, error)
 
 	// RegisterBlockEpochNtfn registers an intent to be notified of each
 	// new block connected to the tip of the main chain. The returned

chainntnfs/interface_test.go

@@ -17,9 +17,6 @@ import (
 	"github.com/btcsuite/btcwallet/walletdb"
 	"github.com/lightninglabs/neutrino"
 	"github.com/lightningnetwork/lnd/chainntnfs"
-	"github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify"
-	"github.com/lightningnetwork/lnd/chainntnfs/btcdnotify"
-	"github.com/lightningnetwork/lnd/chainntnfs/neutrinonotify"
 	"github.com/ltcsuite/ltcd/btcjson"
 
 	"github.com/btcsuite/btcd/btcec"
@@ -30,6 +27,18 @@ import (
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcutil"
 
+	// Required to auto-register the bitcoind backed ChainNotifier
+	// implementation.
+	_ "github.com/lightningnetwork/lnd/chainntnfs/bitcoindnotify"
+
+	// Required to auto-register the btcd backed ChainNotifier
+	// implementation.
+	_ "github.com/lightningnetwork/lnd/chainntnfs/btcdnotify"
+
+	// Required to auto-register the neutrino backed ChainNotifier
+	// implementation.
+	_ "github.com/lightningnetwork/lnd/chainntnfs/neutrinonotify"
+
 	// Required to register the boltdb walletdb implementation.
 	_ "github.com/btcsuite/btcwallet/walletdb/bdb"
 )
@@ -403,7 +412,7 @@ func testSpendNotification(miner *rpctest.Harness,
 	spendClients := make([]*chainntnfs.SpendEvent, numClients)
 	for i := 0; i < numClients; i++ {
 		spentIntent, err := notifier.RegisterSpendNtfn(outpoint,
-			uint32(currentHeight), false)
+			uint32(currentHeight))
 		if err != nil {
 			t.Fatalf("unable to register for spend ntfn: %v", err)
 		}
@@ -451,6 +460,22 @@ func testSpendNotification(miner *rpctest.Harness,
 	case <-time.After(mempoolSpendTimeout):
 	}
 
+	// Make sure registering a client after the tx is in the mempool still
+	// doesn't trigger a notification.
+	spentIntent, err := notifier.RegisterSpendNtfn(outpoint,
+		uint32(currentHeight))
+	if err != nil {
+		t.Fatalf("unable to register for spend ntfn: %v", err)
+	}
+
+	select {
+	case <-spentIntent.Spend:
+		t.Fatalf("did not expect to get notification before " +
+			"block was mined")
+	case <-time.After(mempoolSpendTimeout):
+	}
+	spendClients = append(spendClients, spentIntent)
+
 	// Now we mine a single block, which should include our spend. The
 	// notification should also be sent off.
 	if _, err := miner.Node.Generate(1); err != nil {
@@ -475,139 +500,6 @@ func testSpendNotification(miner *rpctest.Harness,
 	}
 }
 
-func testSpendNotificationMempoolSpends(miner *rpctest.Harness,
-	notifier chainntnfs.ChainNotifier, t *testing.T) {
-
-	// Skip this test for neutrino and bitcoind backends, as they currently
-	// don't support notifying about mempool spends.
-	switch notifier.(type) {
-	case *neutrinonotify.NeutrinoNotifier:
-		return
-	case *bitcoindnotify.BitcoindNotifier:
-		return
-	case *btcdnotify.BtcdNotifier:
-		// Go on to test this implementation.
-	default:
-		t.Fatalf("unknown notifier type: %T", notifier)
-	}
-
-	// We first create a new output to our test target address.
-	outpoint, pkScript := createSpendableOutput(miner, t)
-
-	_, currentHeight, err := miner.Node.GetBestBlock()
-	if err != nil {
-		t.Fatalf("unable to get current height: %v", err)
-	}
-
-	// Now that we have a output index and the pkScript, register for a
-	// spentness notification for the newly created output with multiple
-	// clients in order to ensure the implementation can support
-	// multi-client spend notifications.
-
-	// We first create a list of clients that will be notified on mempool
-	// spends.
-	const numClients = 5
-	spendClientsMempool := make([]*chainntnfs.SpendEvent, numClients)
-	for i := 0; i < numClients; i++ {
-		spentIntent, err := notifier.RegisterSpendNtfn(outpoint,
-			uint32(currentHeight), true)
-		if err != nil {
-			t.Fatalf("unable to register for spend ntfn: %v", err)
-		}
-
-		spendClientsMempool[i] = spentIntent
-	}
-
-	// Next, create a new transaction spending that output.
-	spendingTx := createSpendTx(outpoint, pkScript, t)
-
-	// Broadcast our spending transaction.
-	spenderSha, err := miner.Node.SendRawTransaction(spendingTx, true)
-	if err != nil {
-		t.Fatalf("unable to broadcast tx: %v", err)
-	}
-
-	err = waitForMempoolTx(miner, spenderSha)
-	if err != nil {
-		t.Fatalf("tx not relayed to miner: %v", err)
-	}
-
-	// Make sure the mempool spend clients are correctly notified.
-	for _, client := range spendClientsMempool {
-		select {
-		case ntfn, ok := <-client.Spend:
-			if !ok {
-				t.Fatalf("channel closed unexpectedly")
-			}
-
-			checkNotificationFields(ntfn, outpoint, spenderSha,
-				currentHeight+1, t)
-
-		case <-time.After(5 * time.Second):
-			t.Fatalf("did not receive notification")
-		}
-	}
-
-	// Create new clients that register after the tx is in the mempool
-	// already, but should still be notified.
-	newSpendClientsMempool := make([]*chainntnfs.SpendEvent, numClients)
-	for i := 0; i < numClients; i++ {
-		spentIntent, err := notifier.RegisterSpendNtfn(outpoint,
-			uint32(currentHeight), true)
-		if err != nil {
-			t.Fatalf("unable to register for spend ntfn: %v", err)
-		}
-
-		newSpendClientsMempool[i] = spentIntent
-	}
-
-	// Make sure the new mempool spend clients are correctly notified.
-	for _, client := range newSpendClientsMempool {
-		select {
-		case ntfn, ok := <-client.Spend:
-			if !ok {
-				t.Fatalf("channel closed unexpectedly")
-			}
-
-			checkNotificationFields(ntfn, outpoint, spenderSha,
-				currentHeight+1, t)
-
-		case <-time.After(5 * time.Second):
-			t.Fatalf("did not receive notification")
-		}
-	}
-
-	// Now we mine a single block, which should include our spend. The
-	// notification should not be sent off again.
-	if _, err := miner.Node.Generate(1); err != nil {
-		t.Fatalf("unable to generate single block: %v", err)
-	}
-
-	// When a block is mined, the mempool notifications we registered should
-	// not be sent off again, and the channel should be closed.
-	for _, c := range spendClientsMempool {
-		select {
-		case _, ok := <-c.Spend:
-			if ok {
-				t.Fatalf("channel should have been closed")
-			}
-		case <-time.After(30 * time.Second):
-			t.Fatalf("expected clients to be closed.")
-		}
-	}
-	for _, c := range newSpendClientsMempool {
-		select {
-		case _, ok := <-c.Spend:
-			if ok {
-				t.Fatalf("channel should have been closed")
-			}
-		case <-time.After(30 * time.Second):
-			t.Fatalf("expected clients to be closed.")
-		}
-	}
-
-}
-
 func testBlockEpochNotification(miner *rpctest.Harness,
 	notifier chainntnfs.ChainNotifier, t *testing.T) {
 
@@ -1062,14 +954,13 @@ func testSpendBeforeNtfnRegistration(miner *rpctest.Harness,
 
 	// checkSpends registers two clients to be notified of a spend that has
 	// already happened. The notifier should dispatch a spend notification
-	// immediately. We register one that also listen for mempool spends,
+	// immediately.
-	// both should be notified the same way, as the spend is already mined.
 	checkSpends := func() {
 		const numClients = 2
 		spendClients := make([]*chainntnfs.SpendEvent, numClients)
 		for i := 0; i < numClients; i++ {
 			spentIntent, err := notifier.RegisterSpendNtfn(outpoint,
-				uint32(currentHeight), i%2 == 0)
+				uint32(currentHeight))
 			if err != nil {
 				t.Fatalf("unable to register for spend ntfn: %v",
 					err)
@@ -1149,7 +1040,7 @@ func testCancelSpendNtfn(node *rpctest.Harness,
 	spendClients := make([]*chainntnfs.SpendEvent, numClients)
 	for i := 0; i < numClients; i++ {
 		spentIntent, err := notifier.RegisterSpendNtfn(outpoint,
-			uint32(currentHeight), true)
+			uint32(currentHeight))
 		if err != nil {
 			t.Fatalf("unable to register for spend ntfn: %v", err)
 		}
@@ -1446,10 +1337,6 @@ var ntfnTests = []testCase{
 		name: "spend ntfn",
 		test: testSpendNotification,
 	},
-	{
-		name: "spend ntfn mempool",
-		test: testSpendNotificationMempoolSpends,
-	},
 	{
 		name: "block epoch",
 		test: testBlockEpochNotification,

chainntnfs/neutrinonotify/neutrino.go

@@ -566,7 +566,7 @@ type spendCancel struct {
 // target outpoint has been detected, the details of the spending event will be
 // sent across the 'Spend' channel.
 func (n *NeutrinoNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 
 	n.heightMtx.RLock()
 	currentHeight := n.bestHeight

contractcourt/chain_arbitrator.go

@@ -497,13 +497,16 @@ func (c *ChainArbitrator) Stop() error {
 // NOTE: This must be launched as a goroutine.
 func (c *ChainArbitrator) watchForChannelClose(closeInfo *channeldb.ChannelCloseSummary) {
 	spendNtfn, err := c.cfg.Notifier.RegisterSpendNtfn(
-		&closeInfo.ChanPoint, closeInfo.CloseHeight, true,
+		&closeInfo.ChanPoint, closeInfo.CloseHeight,
 	)
 	if err != nil {
 		log.Errorf("unable to register for spend: %v", err)
 		return
 	}
 
+	log.Infof("Waiting for ChannelPoint(%v) to be coop closed on chain",
+		closeInfo.ChanPoint)
+
 	var (
 		commitSpend *chainntnfs.SpendDetail
 		ok          bool
@@ -517,37 +520,13 @@ func (c *ChainArbitrator) watchForChannelClose(closeInfo *channeldb.ChannelClose
 		return
 	}
 
-	confNtfn, err := c.cfg.Notifier.RegisterConfirmationsNtfn(
-		commitSpend.SpenderTxHash, 1,
-		uint32(commitSpend.SpendingHeight),
-	)
-	if err != nil {
-		log.Errorf("unable to register for "+
-			"conf: %v", err)
-		return
-	}
-
-	log.Infof("Waiting for txid=%v to close ChannelPoint(%v) on chain",
-		commitSpend.SpenderTxHash, closeInfo.ChanPoint)
-
-	select {
-	case confInfo, ok := <-confNtfn.Confirmed:
-		if !ok {
-			return
-		}
-
 	log.Infof("ChannelPoint(%v) is fully closed, at height: %v",
-			closeInfo.ChanPoint, confInfo.BlockHeight)
+		closeInfo.ChanPoint, commitSpend.SpendingHeight)
 
-		err := c.resolveContract(closeInfo.ChanPoint, nil)
+	if err := c.resolveContract(closeInfo.ChanPoint, nil); err != nil {
-		if err != nil {
 		log.Errorf("unable to resolve contract: %v", err)
 	}
 
-	case <-c.quit:
-		return
-	}
-
 }
 
 // ContractSignals wraps the two signals that affect the state of a channel

contractcourt/chain_watcher.go

@@ -173,7 +173,7 @@ func (c *chainWatcher) Start() error {
 	}
 
 	spendNtfn, err := c.cfg.notifier.RegisterSpendNtfn(
-		fundingOut, heightHint, false,
+		fundingOut, heightHint,
 	)
 	if err != nil {
 		return err

contractcourt/chain_watcher_test.go

@@ -36,7 +36,7 @@ func (m *mockNotifier) Stop() error {
 	return nil
 }
 func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 	return &chainntnfs.SpendEvent{
 		Spend:  m.spendChan,
 		Cancel: func() {},

contractcourt/contract_resolvers.go

@@ -169,45 +169,17 @@ func (h *htlcTimeoutResolver) Resolve() (ContractResolver, error) {
 	// spent, and the spending transaction has been fully confirmed.
 	waitForOutputResolution := func() error {
 		// We first need to register to see when the HTLC output itself
-		// has been spent so we can wait for the spending transaction
+		// has been spent by a confirmed transaction.
-		// to confirm.
 		spendNtfn, err := h.Notifier.RegisterSpendNtfn(
 			&h.htlcResolution.ClaimOutpoint,
-			h.broadcastHeight, true,
+			h.broadcastHeight,
 		)
 		if err != nil {
 			return err
 		}
 
-		var spendDetail *chainntnfs.SpendDetail
 		select {
-		case s, ok := <-spendNtfn.Spend:
+		case _, ok := <-spendNtfn.Spend:
-			if !ok {
-				return fmt.Errorf("notifier quit")
-			}
-
-			spendDetail = s
-
-		case <-h.Quit:
-			return fmt.Errorf("quitting")
-		}
-
-		// Now that the output has been spent, we'll also wait for the
-		// transaction to be confirmed before proceeding.
-		confNtfn, err := h.Notifier.RegisterConfirmationsNtfn(
-			spendDetail.SpenderTxHash, 1,
-			uint32(spendDetail.SpendingHeight-1),
-		)
-		if err != nil {
-			return err
-		}
-
-		log.Infof("%T(%v): waiting for spending (txid=%v) to be fully "+
-			"confirmed", h, h.htlcResolution.ClaimOutpoint,
-			spendDetail.SpenderTxHash)
-
-		select {
-		case _, ok := <-confNtfn.Confirmed:
 			if !ok {
 				return fmt.Errorf("notifier quit")
 			}
@@ -608,7 +580,7 @@ func (h *htlcSuccessResolver) Resolve() (ContractResolver, error) {
 	// To wrap this up, we'll wait until the second-level transaction has
 	// been spent, then fully resolve the contract.
 	spendNtfn, err := h.Notifier.RegisterSpendNtfn(
-		&h.htlcResolution.ClaimOutpoint, h.broadcastHeight, true,
+		&h.htlcResolution.ClaimOutpoint, h.broadcastHeight,
 	)
 	if err != nil {
 		return nil, err
@@ -819,8 +791,7 @@ func (h *htlcOutgoingContestResolver) Resolve() (ContractResolver, error) {
 	// First, we'll register for a spend notification for this output. If
 	// the remote party sweeps with the pre-image, we'll  be notified.
 	spendNtfn, err := h.Notifier.RegisterSpendNtfn(
-		&outPointToWatch,
+		&outPointToWatch, h.broadcastHeight,
-		h.broadcastHeight, true,
 	)
 	if err != nil {
 		return nil, err
@@ -1316,7 +1287,7 @@ func (c *commitSweepResolver) Resolve() (ContractResolver, error) {
 		// until the commitment output has been spent.
 		spendNtfn, err := c.Notifier.RegisterSpendNtfn(
 			&c.commitResolution.SelfOutPoint,
-			c.broadcastHeight, true,
+			c.broadcastHeight,
 		)
 		if err != nil {
 			return nil, err

discovery/gossiper_test.go

@@ -263,8 +263,8 @@ func (m *mockNotifier) RegisterConfirmationsNtfn(txid *chainhash.Hash,
 	return nil, nil
 }
 
-func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, _ uint32,
+func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	_ bool) (*chainntnfs.SpendEvent, error) {
+	_ uint32) (*chainntnfs.SpendEvent, error) {
 	return nil, nil
 }
 

fundingmanager_test.go

@@ -126,7 +126,7 @@ func (m *mockNotifier) Stop() error {
 }
 
 func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 	return &chainntnfs.SpendEvent{
 		Spend:  make(chan *chainntnfs.SpendDetail),
 		Cancel: func() {},

htlcswitch/mock.go

@@ -801,7 +801,7 @@ func (m *mockNotifier) Stop() error {
 }
 
 func (m *mockNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, mempool bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 
 	return &chainntnfs.SpendEvent{
 		Spend: make(chan *chainntnfs.SpendDetail),

lnd_test.go

@@ -1771,7 +1771,7 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) {
 		t.Fatalf("htlc mismatch: %v", err)
 	}
 
-	// As we'll be querying the state of Carol's channels frequently we'll
+	// As we'll be querying the state of Alice's channels frequently we'll
 	// create a closure helper function for the purpose.
 	getAliceChanInfo := func() (*lnrpc.Channel, error) {
 		req := &lnrpc.ListChannelsRequest{}
@@ -2253,7 +2253,7 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) {
 		t.Fatalf("no user funds should be left in limbo after incubation")
 	}
 
-	// At this point, Carol should now be aware of his new immediately
+	// At this point, Bob should now be aware of his new immediately
 	// spendable on-chain balance, as it was Alice who broadcast the
 	// commitment transaction.
 	carolBalResp, err = net.Bob.WalletBalance(ctxb, carolBalReq)
@@ -5867,17 +5867,17 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness,
 	// Query the mempool for Dave's justice transaction, this should be
 	// broadcast as Carol's contract breaching transaction gets confirmed
 	// above. Since Carol might have had the time to take some of the HTLC
-	// outputs to the second level before Alice broadcasts her justice tx,
+	// outputs to the second level before Dave broadcasts his justice tx,
 	// we'll search through the mempool for a tx that matches the number of
 	// expected inputs in the justice tx.
-	// TODO(halseth): change to deterministic check if/when only acting on
-	// confirmed second level spends?
 	var predErr error
 	var justiceTxid *chainhash.Hash
-	err = lntest.WaitPredicate(func() bool {
+	errNotFound := errors.New("justice tx not found")
+	findJusticeTx := func() (*chainhash.Hash, error) {
 		mempool, err := net.Miner.Node.GetRawMempool()
 		if err != nil {
-			t.Fatalf("unable to get mempool from miner: %v", err)
+			return nil, fmt.Errorf("unable to get mempool from "+
+				"miner: %v", err)
 		}
 
 		for _, txid := range mempool {
@@ -5885,22 +5885,46 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness,
 			// of inputs.
 			tx, err := net.Miner.Node.GetRawTransaction(txid)
 			if err != nil {
-				predErr = fmt.Errorf("unable to query for "+
+				return nil, fmt.Errorf("unable to query for "+
 					"txs: %v", err)
-				return false
 			}
 
 			exNumInputs := 2 + numInvoices
 			if len(tx.MsgTx().TxIn) == exNumInputs {
+				return txid, nil
+			}
+		}
+		return nil, errNotFound
+	}
+
+	err = lntest.WaitPredicate(func() bool {
+		txid, err := findJusticeTx()
+		if err != nil {
+			predErr = err
+			return false
+		}
+
 		justiceTxid = txid
 		return true
-			}
+	}, time.Second*10)
-
+	if err != nil && predErr == errNotFound {
-		}
+		// If Dave is unable to broadcast his justice tx on first
-
+		// attempt because of the second layer transactions, he will
-		predErr = fmt.Errorf("justice tx not found")
+		// wait until the next block epoch before trying again. Because
+		// of this, we'll mine a block if we cannot find the justice tx
+		// immediately.
+		mineBlocks(t, net, 1)
+		err = lntest.WaitPredicate(func() bool {
+			txid, err := findJusticeTx()
+			if err != nil {
+				predErr = err
 				return false
-	}, time.Second*15)
+			}
+
+			justiceTxid = txid
+			return true
+		}, time.Second*10)
+	}
 	if err != nil {
 		t.Fatalf(predErr.Error())
 	}
@@ -8276,7 +8300,8 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 	// At this point, Bob decides that he wants to exit the channel
 	// immediately, so he force closes his commitment transaction.
 	ctxt, _ := context.WithTimeout(ctxb, timeout)
-	closeChannelAndAssert(ctxt, t, net, net.Bob, aliceChanPoint, true)
+	bobForceClose := closeChannelAndAssert(ctxt, t, net, net.Bob,
+		aliceChanPoint, true)
 
 	// We'll now mine enough blocks so Carol decides that she needs to go
 	// on-chain to claim the HTLC as Bob has been inactive.
@@ -8324,31 +8349,58 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 
 	// After the force close transacion is mined, Carol should broadcast
 	// her second level HTLC transacion. Bob will braodcast a sweep tx to
-	// sweep his output in the channel with Carol. When Bob notices Carol's
+	// sweep his output in the channel with Carol. He can do this
-	// second level transaction in the mempool, he will extract the
+	// immediately, as the output is not timelocked since Carol was the one
-	// preimage and broadcast a second level tx to claim the HTLC in his
+	// force closing.
-	// (already closed) channel with Alice.
+	commitSpends, err := waitForNTxsInMempool(net.Miner.Node, 2,
-	secondLevelHashes, err := waitForNTxsInMempool(net.Miner.Node, 3,
 		time.Second*20)
 	if err != nil {
 		t.Fatalf("transactions not found in mempool: %v", err)
 	}
 
-	// Carol's second level transaction should be spending from
+	// Both Carol's second level transaction and Bob's sweep should be
-	// the commitment transaction.
+	// spending from the commitment transaction.
-	var secondLevelHash *chainhash.Hash
+	for _, txid := range commitSpends {
-	for _, txid := range secondLevelHashes {
 		tx, err := net.Miner.Node.GetRawTransaction(txid)
 		if err != nil {
 			t.Fatalf("unable to get txn: %v", err)
 		}
 
-		if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash == *commitHash {
+		if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *commitHash {
-			secondLevelHash = txid
+			t.Fatalf("tx did not spend from commitment tx")
 		}
 	}
-	if secondLevelHash == nil {
+
-		t.Fatalf("Carol's second level tx not found")
+	// Mine a block to confirm the two transactions (+ the coinbase).
+	block = mineBlocks(t, net, 1)[0]
+	if len(block.Transactions) != 3 {
+		t.Fatalf("expected 3 transactions in block, got %v",
+			len(block.Transactions))
+	}
+	for _, txid := range commitSpends {
+		assertTxInBlock(t, block, txid)
+	}
+
+	// Keep track of the second level tx maturity.
+	carolSecondLevelCSV := defaultCSV
+
+	// When Bob notices Carol's second level transaction in the block, he
+	// will extract the preimage and broadcast a second level tx to claim
+	// the HTLC in his (already closed) channel with Alice.
+	bobSecondLvlTx, err := waitForTxInMempool(net.Miner.Node,
+		time.Second*20)
+	if err != nil {
+		t.Fatalf("transactions not found in mempool: %v", err)
+	}
+
+	// It should spend from the commitment in the channel with Alice.
+	tx, err := net.Miner.Node.GetRawTransaction(bobSecondLvlTx)
+	if err != nil {
+		t.Fatalf("unable to get txn: %v", err)
+	}
+
+	if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *bobForceClose {
+		t.Fatalf("tx did not spend from bob's force close tx")
 	}
 
 	// At this point, Bob should have broadcast his second layer success
@@ -8388,41 +8440,63 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 				return false
 			}
 		}
-
 		return true
 	}, time.Second*15)
 	if err != nil {
 		t.Fatalf("bob didn't hand off time-locked HTLC: %v", predErr)
 	}
 
-	// We'll now mine a block which should confirm the two second layer
+	// We'll now mine a block which should confirm Bob's second layer
-	// transactions and the commit sweep.
+	// transaction.
 	block = mineBlocks(t, net, 1)[0]
-	if len(block.Transactions) != 4 {
+	if len(block.Transactions) != 2 {
-		t.Fatalf("expected 4 transactions in block, got %v",
+		t.Fatalf("expected 2 transactions in block, got %v",
 			len(block.Transactions))
 	}
-	assertTxInBlock(t, block, secondLevelHash)
+	assertTxInBlock(t, block, bobSecondLvlTx)
 
-	// If we then mine 4 additional blocks, Bob and Carol should sweep the
+	// Keep track of Bob's second level maturity, and decrement our track
-	// outputs destined for them.
+	// of Carol's.
-	if _, err := net.Miner.Node.Generate(defaultCSV); err != nil {
+	bobSecondLevelCSV := defaultCSV
+	carolSecondLevelCSV--
+
+	// If we then mine 3 additional blocks, Carol's second level tx should
+	// mature, and she can pull the funds from it with a sweep tx.
+	if _, err := net.Miner.Node.Generate(carolSecondLevelCSV); err != nil {
 		t.Fatalf("unable to generate block: %v", err)
 	}
+	bobSecondLevelCSV -= carolSecondLevelCSV
 
-	sweepTxs, err := waitForNTxsInMempool(net.Miner.Node, 2, time.Second*10)
+	carolSweep, err := waitForTxInMempool(net.Miner.Node, time.Second*10)
 	if err != nil {
-		t.Fatalf("unable to find sweeping transactions: %v", err)
+		t.Fatalf("unable to find Carol's sweeping transaction: %v", err)
 	}
 
-	// At this point, Bob should detect that he has no pending channels
+	// Mining one additional block, Bob's second level tx is mature, and he
-	// anymore, as this just resolved it by the confirmation of the sweep
+	// can sweep the output.
-	// transaction we detected above.
+	block = mineBlocks(t, net, bobSecondLevelCSV)[0]
-	block = mineBlocks(t, net, 1)[0]
+	assertTxInBlock(t, block, carolSweep)
-	for _, sweepTx := range sweepTxs {
+
-		assertTxInBlock(t, block, sweepTx)
+	bobSweep, err := waitForTxInMempool(net.Miner.Node, time.Second*10)
+	if err != nil {
+		t.Fatalf("unable to find bob's sweeping transaction")
 	}
 
+	// Make sure it spends from the second level tx.
+	tx, err = net.Miner.Node.GetRawTransaction(bobSweep)
+	if err != nil {
+		t.Fatalf("unable to get txn: %v", err)
+	}
+	if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *bobSecondLvlTx {
+		t.Fatalf("tx did not spend from bob's second level tx")
+	}
+
+	// When we mine one additional block, that will confirm Bob's sweep.
+	// Now Bob should have no pending channels anymore, as this just
+	// resolved it by the confirmation of the sweep transaction.
+	block = mineBlocks(t, net, 1)[0]
+	assertTxInBlock(t, block, bobSweep)
+
 	err = lntest.WaitPredicate(func() bool {
 		pendingChanResp, err := net.Bob.PendingChannels(
 			ctxb, pendingChansRequest,
@@ -8437,7 +8511,54 @@ func testMultiHopHtlcLocalChainClaim(net *lntest.NetworkHarness, t *harnessTest)
 				"but shouldn't: %v", spew.Sdump(pendingChanResp))
 			return false
 		}
+		req := &lnrpc.ListChannelsRequest{}
+		chanInfo, err := net.Bob.ListChannels(ctxb, req)
+		if err != nil {
+			predErr = fmt.Errorf("unable to query for open "+
+				"channels: %v", err)
+			return false
+		}
+		if len(chanInfo.Channels) != 0 {
+			predErr = fmt.Errorf("Bob should have no open "+
+				"channels, instead he has %v",
+				len(chanInfo.Channels))
+			return false
+		}
+		return true
+	}, time.Second*15)
+	if err != nil {
+		t.Fatalf(predErr.Error())
+	}
 
+	// Also Carol should have no channels left (open nor pending).
+	err = lntest.WaitPredicate(func() bool {
+		pendingChanResp, err := carol.PendingChannels(
+			ctxb, pendingChansRequest,
+		)
+		if err != nil {
+			predErr = fmt.Errorf("unable to query for pending "+
+				"channels: %v", err)
+			return false
+		}
+		if len(pendingChanResp.PendingForceClosingChannels) != 0 {
+			predErr = fmt.Errorf("bob carol has pending channels "+
+				"but shouldn't: %v", spew.Sdump(pendingChanResp))
+			return false
+		}
+
+		req := &lnrpc.ListChannelsRequest{}
+		chanInfo, err := carol.ListChannels(ctxb, req)
+		if err != nil {
+			predErr = fmt.Errorf("unable to query for open "+
+				"channels: %v", err)
+			return false
+		}
+		if len(chanInfo.Channels) != 0 {
+			predErr = fmt.Errorf("carol should have no open "+
+				"channels, instead she has %v",
+				len(chanInfo.Channels))
+			return false
+		}
 		return true
 	}, time.Second*15)
 	if err != nil {
@@ -8506,7 +8627,8 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 	// immediately force close the channel by broadcast her commitment
 	// transaction.
 	ctxt, _ := context.WithTimeout(ctxb, timeout)
-	closeChannelAndAssert(ctxt, t, net, net.Alice, aliceChanPoint, true)
+	aliceForceClose := closeChannelAndAssert(ctxt, t, net, net.Alice,
+		aliceChanPoint, true)
 
 	// We'll now mine enough blocks so Carol decides that she needs to go
 	// on-chain to claim the HTLC as Bob has been inactive.
@@ -8555,58 +8677,68 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 
 	// After the force close transacion is mined, Carol should broadcast
 	// her second level HTLC transacion. Bob will braodcast a sweep tx to
-	// sweep his output in the channel with Carol. When Bob notices Carol's
+	// sweep his output in the channel with Carol. He can do this
-	// second level transaction in the mempool, he will extract the
+	// immediately, as the output is not timelocked since Carol was the one
-	// preimage and broadcast a second level tx to claim the HTLC in his
+	// force closing.
-	// (already closed) channel with Alice.
+	commitSpends, err := waitForNTxsInMempool(net.Miner.Node, 2,
-	secondLevelHashes, err := waitForNTxsInMempool(net.Miner.Node, 3,
 		time.Second*20)
 	if err != nil {
 		t.Fatalf("transactions not found in mempool: %v", err)
 	}
 
-	// Carol's second level transaction should be spending from
+	// Both Carol's second level transaction and Bob's sweep should be
-	// the commitment transaction.
+	// spending from the commitment transaction.
-	var secondLevelHash *chainhash.Hash
+	for _, txid := range commitSpends {
-	for _, txid := range secondLevelHashes {
 		tx, err := net.Miner.Node.GetRawTransaction(txid)
 		if err != nil {
 			t.Fatalf("unable to get txn: %v", err)
 		}
 
-		if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash == *commitHash {
+		if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *commitHash {
-			secondLevelHash = txid
+			t.Fatalf("tx did not spend from commitment tx")
 		}
 	}
-	if secondLevelHash == nil {
-		t.Fatalf("Carol's second level tx not found")
-	}
 
-	// We'll now mine a block which should confirm the two second layer
+	// Mine a block to confirm the two transactions (+ coinbase).
-	// transactions and the commit sweep.
 	block = mineBlocks(t, net, 1)[0]
-	if len(block.Transactions) != 4 {
+	if len(block.Transactions) != 3 {
-		t.Fatalf("expected 4 transactions in block, got %v",
+		t.Fatalf("expected 3 transactions in block, got %v",
 			len(block.Transactions))
 	}
-	assertTxInBlock(t, block, secondLevelHash)
+	for _, txid := range commitSpends {
-
+		assertTxInBlock(t, block, txid)
-	// If we then mine 4 additional blocks, Bob should pull the output
-	// destined for him.
-	if _, err := net.Miner.Node.Generate(defaultCSV); err != nil {
-		t.Fatalf("unable to generate block: %v", err)
 	}
 
-	_, err = waitForNTxsInMempool(net.Miner.Node, 1, time.Second*15)
+	// Keep track of the second level tx maturity.
+	carolSecondLevelCSV := defaultCSV
+
+	// When Bob notices Carol's second level transaction in the block, he
+	// will extract the preimage and broadcast a sweep tx to directly claim
+	// the HTLC in his (already closed) channel with Alice.
+	bobHtlcSweep, err := waitForTxInMempool(net.Miner.Node,
+		time.Second*20)
 	if err != nil {
-		t.Fatalf("unable to find bob's sweeping transaction: %v", err)
+		t.Fatalf("transactions not found in mempool: %v", err)
 	}
 
-	// We'll now mine another block, this should confirm the sweep
+	// It should spend from the commitment in the channel with Alice.
-	// transaction that Bob broadcast in the prior stage.
+	tx, err := net.Miner.Node.GetRawTransaction(bobHtlcSweep)
-	if _, err := net.Miner.Node.Generate(1); err != nil {
+	if err != nil {
-		t.Fatalf("unable to generate block: %v", err)
+		t.Fatalf("unable to get txn: %v", err)
 	}
+	if tx.MsgTx().TxIn[0].PreviousOutPoint.Hash != *aliceForceClose {
+		t.Fatalf("tx did not spend from alice's force close tx")
+	}
+
+	// We'll now mine a block which should confirm Bob's HTLC sweep
+	// transaction.
+	block = mineBlocks(t, net, 1)[0]
+	if len(block.Transactions) != 2 {
+		t.Fatalf("expected 2 transactions in block, got %v",
+			len(block.Transactions))
+	}
+	assertTxInBlock(t, block, bobHtlcSweep)
+	carolSecondLevelCSV--
 
 	// Now that the sweeping transaction has been confirmed, Bob should now
 	// recognize that all contracts have been fully resolved, and show no
@@ -8632,6 +8764,44 @@ func testMultiHopHtlcRemoteChainClaim(net *lntest.NetworkHarness, t *harnessTest
 	if err != nil {
 		t.Fatalf(predErr.Error())
 	}
+
+	// If we then mine 3 additional blocks, Carol's second level tx will
+	// mature, and she should pull the funds.
+	if _, err := net.Miner.Node.Generate(carolSecondLevelCSV); err != nil {
+		t.Fatalf("unable to generate block: %v", err)
+	}
+
+	carolSweep, err := waitForTxInMempool(net.Miner.Node, time.Second*10)
+	if err != nil {
+		t.Fatalf("unable to find Carol's sweeping transaction: %v", err)
+	}
+
+	// When Carol's sweep gets confirmed, she should have no more pending
+	// channels.
+	block = mineBlocks(t, net, 1)[0]
+	assertTxInBlock(t, block, carolSweep)
+
+	pendingChansRequest = &lnrpc.PendingChannelsRequest{}
+	err = lntest.WaitPredicate(func() bool {
+		pendingChanResp, err := carol.PendingChannels(
+			ctxb, pendingChansRequest,
+		)
+		if err != nil {
+			predErr = fmt.Errorf("unable to query for pending "+
+				"channels: %v", err)
+			return false
+		}
+		if len(pendingChanResp.PendingForceClosingChannels) != 0 {
+			predErr = fmt.Errorf("carol still has pending channels "+
+				"but shouldn't: %v", spew.Sdump(pendingChanResp))
+			return false
+		}
+
+		return true
+	}, time.Second*15)
+	if err != nil {
+		t.Fatalf(predErr.Error())
+	}
 }
 
 // testSwitchCircuitPersistence creates a multihop network to ensure the sender

lntest/node.go

@@ -335,9 +335,11 @@ func (hn *HarnessNode) start(lndError chan<- error) error {
 	// Launch a new goroutine which that bubbles up any potential fatal
 	// process errors to the goroutine running the tests.
 	hn.processExit = make(chan struct{})
+	hn.wg.Add(1)
 	go func() {
-		err := hn.cmd.Wait()
+		defer hn.wg.Done()
 
+		err := hn.cmd.Wait()
 		if err != nil {
 			lndError <- errors.Errorf("%v\n%v\n", err, errb.String())
 		}

mock.go

@@ -106,7 +106,7 @@ func (m *mockNotfier) Stop() error {
 	return nil
 }
 func (m *mockNotfier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 	return &chainntnfs.SpendEvent{
 		Spend:  make(chan *chainntnfs.SpendDetail),
 		Cancel: func() {},
@@ -131,7 +131,7 @@ func makeMockSpendNotifier() *mockSpendNotifier {
 }
 
 func (m *mockSpendNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint,
-	heightHint uint32, _ bool) (*chainntnfs.SpendEvent, error) {
+	heightHint uint32) (*chainntnfs.SpendEvent, error) {
 	m.mtx.Lock()
 	defer m.mtx.Unlock()