From b60270f3f7b949ad72c4b7a992c9bae7a2aeacea Mon Sep 17 00:00:00 2001
From: Olaoluwa Osuntokun <laolu32@gmail.com>
Date: Sat, 16 Jul 2016 18:12:36 -0700
Subject: [PATCH] lnwallet: include htlcs to settle in returned set of htlc's
 to forward
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This commit fixes a slight bug in the channel state machine’s code
executed when processing a revocation messages. With this commit after
processing a revocation, log entries which we should forward to the
downstream or upstream peer for settling/adding HTLC’s are now properly
returned.

The testa have also been updated to ensure to correct htlc’s are
returned “for forwarding”.
---
 lnwallet/channel.go      | 45 ++++++++++++++++++++++++++--------------
 lnwallet/channel_test.go | 17 ++++++++++-----
 2 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/lnwallet/channel.go b/lnwallet/channel.go
index cfe0c20a..709b5251 100644
--- a/lnwallet/channel.go
+++ b/lnwallet/channel.go
@@ -113,6 +113,12 @@ type PaymentDescriptor struct {
 	// remote party added, or one that we added locally.
 	Index uint32
 
+	// ParentIndex is the index of the log entry that this HTLC update
+	// settles or times out. If IsIncoming is false, then this refers to an
+	// index within our local log, otherwise this refers to an entry int he
+	// remote peer's log.
+	ParentIndex uint32
+
 	// Payload is an opaque blob which is used to complete multi-hop routing.
 	Payload []byte
 
@@ -135,7 +141,7 @@ type PaymentDescriptor struct {
 	addCommitHeightRemote uint64
 	addCommitHeightLocal  uint64
 
-	/// removeCommitHeight[Remote|Local] encodes the height of the
+	// removeCommitHeight[Remote|Local] encodes the height of the
 	//commitment which removed the parent pointer of this PaymentDescriptor
 	//either due to a timeout or a settle. Once both these heights are
 	//above the tail of both chains, the log entries can safely be removed.
@@ -942,25 +948,32 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.CommitRevocation) (
 		next = e.Next()
 		htlc := e.Value.(*PaymentDescriptor)
 
-		if htlc.entryType != Add {
-			// If this entry is either a timeout or settle, then we
-			// can remove it from our log once the update it locked
-			// into both of our chains.
-			if remoteChainTail >= htlc.removeCommitHeightRemote &&
-				localChainTail >= htlc.removeCommitHeightLocal {
-				parentLink := htlc.parent
-				lc.stateUpdateLog.Remove(e)
-				lc.stateUpdateLog.Remove(parentLink)
+		if htlc.isForwarded {
+			continue
+		}
+
+		// If this entry is either a timeout or settle, then we
+		// can remove it from our log once the update it locked
+		// into both of our chains.
+		if htlc.entryType != Add &&
+			remoteChainTail >= htlc.removeCommitHeightRemote &&
+			localChainTail >= htlc.removeCommitHeightLocal {
+
+			parentLink := htlc.parent
+			lc.stateUpdateLog.Remove(e)
+			lc.stateUpdateLog.Remove(parentLink)
+
+			if !htlc.IsIncoming {
+				htlc.ParentIndex = parentLink.Value.(*PaymentDescriptor).Index
+				htlcsToForward = append(htlcsToForward, htlc)
 			}
-		} else if !htlc.isForwarded {
+		} else if remoteChainTail >= htlc.addCommitHeightRemote &&
+			localChainTail >= htlc.addCommitHeightLocal {
+
 			// Once an HTLC has been fully locked into both of our
 			// chains, then we can safely forward it to the next
 			// hop.
-			// TODO(roasbeef): only incoming htcls?
-			//  * re-visit once multi-hop from jcp
-			if remoteChainTail >= htlc.addCommitHeightRemote &&
-				localChainTail >= htlc.addCommitHeightLocal {
-
+			if htlc.IsIncoming {
 				htlc.isForwarded = true
 				htlcsToForward = append(htlcsToForward, htlc)
 			}
diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go
index 969a7294..8840a5f5 100644
--- a/lnwallet/channel_test.go
+++ b/lnwallet/channel_test.go
@@ -2,7 +2,6 @@ package lnwallet
 
 import (
 	"bytes"
-	"fmt"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -355,13 +354,17 @@ func TestSimpleAddSettleWorkflow(t *testing.T) {
 	}
 	if htlcs, err := bobChannel.ReceiveRevocation(aliceRevocation2); err != nil {
 		t.Fatalf("bob unable to process alice's revocation: %v", err)
-	} else {
-		fmt.Println("bob forward htlcs: %v", htlcs)
+	} else if len(htlcs) != 0 {
+		t.Fatalf("bob shouldn't forward any HTLC's after outgoing settle, "+
+			"instead can forward: %v", spew.Sdump(htlcs))
 	}
 	if htlcs, err := aliceChannel.ReceiveRevocation(bobRevocation2); err != nil {
 		t.Fatalf("alice unable to process bob's revocation: %v", err)
-	} else {
-		fmt.Println("alice forward htlcs: %v", spew.Sdump(htlcs))
+	} else if len(htlcs) != 1 {
+		// Alice should now be able to forward the settlement HTLC to
+		// any down stream peers.
+		t.Fatalf("alice should be able to forward a single HTLC, "+
+			"instead can forward %v: %v", len(htlcs), spew.Sdump(htlcs))
 	}
 
 	// At this point, bob should have 6BTC settled, with Alice still having
@@ -416,3 +419,7 @@ func TestSimpleAddSettleWorkflow(t *testing.T) {
 			"instead", bobLogLen)
 	}
 }
+
+func TestCooperativeChannelClosure(t *testing.T) {
+	// * add validation of their sig
+}