From 2149157d4901af04415fb7fdaeb9c8740e3b9125 Mon Sep 17 00:00:00 2001
From: nsa <elzeigel@gmail.com>
Date: Mon, 13 Jul 2020 15:24:36 -0400
Subject: [PATCH] channeldb: filter out unsigned acked updates in
 AdvanceCommitChainTail

This commit moves the deletion of all updates under the unsigned
acked updates key from AppendRemoteCommitChain to
AdvanceCommitChainTail. This is done because if we went down after
signing for these updates but before receiving a revocation, we would
incorrectly reject their commitment signature:

Alice                Bob
      -----add----->
      -----sig----->
      <----rev------
      <----sig------
      -----rev----->
      <----fail-----
      <----sig------
      -----rev----->
      -----sig----->
      *reconnect*
      <----rev------
      <----add------
      x----sig------

It is also important to note that filtering is required when we
receive a revocation to ensure that we aren't erroneously deleting
remote updates. Take the following state transitions:

 Alice                Bob
       -----add----->
       -----sig----->
       <----rev------
       <----sig------
       -----rev----->
       -----add----->
       -----sig----->
       <----fail-----
       <----sig------
       -----rev-----> (alice stores updates here)
       <----rev------

In the above case, if Alice deleted all updates rather than filtering
when receiving the final revocation from Bob, then Alice would have
to force close the channel due to missing updates. Since Alice hasn't
signed for any of the unsigned acked updates, she should not filter any
of them out.
---
 channeldb/channel.go     | 46 +++++++++++++++++++++++++++++++++-------
 lnwallet/channel_test.go |  8 +++----
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/channeldb/channel.go b/channeldb/channel.go
index 4f5cd4471..5bec7a475 100644
--- a/channeldb/channel.go
+++ b/channeldb/channel.go
@@ -1980,14 +1980,6 @@ func (c *OpenChannel) AppendRemoteCommitChain(diff *CommitDiff) error {
 			return err
 		}
 
-		// Clear unsigned acked remote updates. We are signing now for
-		// all that we've got.
-		err = chanBucket.Delete(unsignedAckedUpdatesKey)
-		if err != nil {
-			return fmt.Errorf("unable to clear dangling remote "+
-				"updates: %v", err)
-		}
-
 		// TODO(roasbeef): use seqno to derive key for later LCP
 
 		// With the bucket retrieved, we'll now serialize the commit
@@ -2196,6 +2188,44 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg) error {
 			return err
 		}
 
+		// Persist the unsigned acked updates that are not included
+		// in their new commitment.
+		updateBytes := chanBucket.Get(unsignedAckedUpdatesKey)
+		if updateBytes == nil {
+			// If there are no updates to sign, we don't need to
+			// filter out any updates.
+			newRemoteCommit = &newCommit.Commitment
+			return nil
+		}
+
+		r := bytes.NewReader(updateBytes)
+		unsignedUpdates, err := deserializeLogUpdates(r)
+		if err != nil {
+			return err
+		}
+
+		var validUpdates []LogUpdate
+		for _, upd := range unsignedUpdates {
+			lIdx := upd.LogIndex
+
+			// Filter for updates that are not on the remote
+			// commitment.
+			if lIdx >= newCommit.Commitment.RemoteLogIndex {
+				validUpdates = append(validUpdates, upd)
+			}
+		}
+
+		var b bytes.Buffer
+		err = serializeLogUpdates(&b, validUpdates)
+		if err != nil {
+			return fmt.Errorf("unable to serialize log updates: %v", err)
+		}
+
+		err = chanBucket.Put(unsignedAckedUpdatesKey, b.Bytes())
+		if err != nil {
+			return fmt.Errorf("unable to store under unsignedAckedUpdatesKey: %v", err)
+		}
+
 		newRemoteCommit = &newCommit.Commitment
 
 		return nil
diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go
index 50931ef8b..5307d8674 100644
--- a/lnwallet/channel_test.go
+++ b/lnwallet/channel_test.go
@@ -7119,7 +7119,7 @@ func restoreAndAssert(t *testing.T, channel *LightningChannel, numAddsLocal,
 	assertInLog(t, newChannel.remoteUpdateLog, numAddsRemote, numFailsRemote)
 }
 
-// TesstChannelRestoreUpdateLogsFailedHTLC runs through a scenario where an
+// TestChannelRestoreUpdateLogsFailedHTLC runs through a scenario where an
 // HTLC is added and failed, and asserts along the way that we would restore
 // the update logs of the channel to the expected state at any point.
 func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) {
@@ -7224,10 +7224,10 @@ func TestChannelRestoreUpdateLogsFailedHTLC(t *testing.T) {
 	}
 
 	// When sending a new commitment, Alice will add a pending commit to
-	// here remote chain. In this case it doesn't contain any new updates,
-	// so it won't affect the restoration.
+	// her remote chain. Since the unsigned acked updates aren't deleted
+	// until we receive a revocation, the fail should still be present.
 	assertInLogs(t, aliceChannel, 1, 0, 0, 1)
-	restoreAndAssert(t, aliceChannel, 1, 0, 0, 0)
+	restoreAndAssert(t, aliceChannel, 1, 0, 0, 1)
 
 	// When Alice receives Bob's revocation, the Fail is irrevocably locked
 	// in on both sides. She should compact the logs, removing the HTLC and