From 2fa2b545fc3a7a31358103aa74a137783d947b61 Mon Sep 17 00:00:00 2001
From: "Johan T. Halseth" <johanth@gmail.com>
Date: Fri, 23 Mar 2018 13:36:57 +0100
Subject: [PATCH] breacharbiter: reuse spend events instead of re-registering

This commit fixes a broadcast loop within the breach arbiter, that would
occur when HTLC outputs had been taken to the second level. The breach
arbiter would register for a spend event, but would immediately go on to
create and publish the justice, without waiting for the response to be
received on the spend channel. This lead to a race, where the outpoint
could actually already have been spent, but the notification would
arrive after the breach arbiter checked the channel, and publishing the
justice TX would fail because it was a double spend. This would create a
"broadcast loop", as seen in the logs from the integration test
revoked_uncooperative_close_retribution_remote_hodl.

This is fixed by reusing an existing spend event for the outputs,
meaning we will actually receive on the first channel we initiated,
making the broadcast loop more likely to terminate.
---
 breacharbiter.go | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/breacharbiter.go b/breacharbiter.go
index 94044083..c5478056 100644
--- a/breacharbiter.go
+++ b/breacharbiter.go
@@ -490,6 +490,8 @@ func (b *breachArbiter) exactRetribution(confChan *chainntnfs.ConfirmationEvent,
 	// construct a sweep transaction and write it to disk. This will allow
 	// the breach arbiter to re-register for notifications for the justice
 	// txid.
+	spendNtfns := make(map[wire.OutPoint]*chainntnfs.SpendEvent)
+
 secondLevelCheck:
 	if finalTx == nil {
 		// Before we create the justice tx, we need to check to see if
@@ -510,24 +512,32 @@ secondLevelCheck:
 				breachedOutput.outpoint, breachInfo.chanPoint)
 
 			// Now that we have an HTLC output, we'll quickly check
-			// to see if it has been spent or not.
-			spendNtfn, err := b.cfg.Notifier.RegisterSpendNtfn(
-				&breachedOutput.outpoint, breachInfo.breachHeight,
-			)
-			if err != nil {
-				brarLog.Errorf("unable to check for spentness "+
-					"of out_point=%v: %v",
-					breachedOutput.outpoint, err)
+			// to see if it has been spent or not. If we have
+			// already registered for a notification for this
+			// output, we'll reuse it.
+			spendNtfn, ok := spendNtfns[breachedOutput.outpoint]
+			if !ok {
+				spendNtfn, err = b.cfg.Notifier.RegisterSpendNtfn(
+					&breachedOutput.outpoint,
+					breachInfo.breachHeight,
+				)
+				if err != nil {
+					brarLog.Errorf("unable to check for "+
+						"spentness of out_point=%v: %v",
+						breachedOutput.outpoint, err)
 
-				// Registration may have failed if we've been
-				// instructed to shutdown. If so, return here to
-				// avoid entering an infinite loop.
-				select {
-				case <-b.quit:
-					return
-				default:
-					continue
+					// Registration may have failed if
+					// we've been instructed to shutdown.
+					// If so, return here to avoid entering
+					// an infinite loop.
+					select {
+					case <-b.quit:
+						return
+					default:
+						continue
+					}
 				}
+				spendNtfns[breachedOutput.outpoint] = spendNtfn
 			}
 
 			select {
@@ -536,6 +546,7 @@ secondLevelCheck:
 				if !ok {
 					return
 				}
+				delete(spendNtfns, breachedOutput.outpoint)
 
 				// In this case we'll morph our initial revoke
 				// spend to instead point to the second level