lnwallet: only forward freshly locked in HTLC's in ReceiveRevocation

This commit fixes a nasty bug that has been lingering within lnd, and
has been noticed due to the added retransmission logic. Before this
commit, upon a restart, if we had an active HTLC and received a new
commitment update, then we would re-forward ALL active HTLC’s. This
could at times lead to a nasty cycle:
  * We re-forward an HTLC already processed.
  * We then notice that the time-lock is out of date (retransmitted
HTLC), so we go to fail it.
  * This is detected as a replay attack, so we send an
UpdateMalformedHTLC
  * This second failure ends up creating a nil entry in the log,
leading to a panic.
  * Remote party disconnects.
  * Upon reconnect we send again as we need to retransmit the changes,
this goes on forever.

In order to fix this, we now ensure that we only forward HTLC’s that
have been newly locked in at this next state. With this, we now avoid
the loop described above, and also ensure that we don’t accidentally
attempt an HTLC replay attack on our selves.

Fixes #528.

Fixes #545.

lnwallet/channel.go

@@ -3905,18 +3905,27 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ([]*P
 			continue
 		}
 
+		// We'll only forward any new HTLC additions iff, it's "freshly
+		// locked in". Meaning that the HTLC was only *just* considered
+		// locked-in at this new state. By doing this we ensure that we
+		// don't re-forward any already processed HTLC's after a
+		// restart.
 		if htlc.EntryType == Add &&
-			remoteChainTail >= htlc.addCommitHeightRemote &&
+			remoteChainTail == htlc.addCommitHeightRemote &&
 			localChainTail >= htlc.addCommitHeightLocal {
 
 			htlc.isForwarded = true
 			htlcsToForward = append(htlcsToForward, htlc)
-		} else if htlc.EntryType != Add &&
+			continue
+		}
+
+		if htlc.EntryType != Add &&
 			remoteChainTail >= htlc.removeCommitHeightRemote &&
 			localChainTail >= htlc.removeCommitHeightLocal {
 
 			htlc.isForwarded = true
 			htlcsToForward = append(htlcsToForward, htlc)
+			continue
 		}
 	}
 

lnwallet/channel_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"math/big"
 	"math/rand"
 	"os"
 	"reflect"
@@ -4074,6 +4075,146 @@ func TestSignCommitmentFailNotLockedIn(t *testing.T) {
 	}
 }
 
+// TestLockedInHtlcForwardingSkipAfterRestart ensures that after a restart, a
+// state machine doesn't attempt to re-forward any HTLC's that were already
+// locked in, but in a prior state.
+func TestLockedInHtlcForwardingSkipAfterRestart(t *testing.T) {
+	t.Parallel()
+
+	// First, we'll make a channel between Alice and Bob.
+	aliceChannel, bobChannel, cleanUp, err := createTestChannels(1)
+	if err != nil {
+		t.Fatalf("unable to create test channels: %v", err)
+	}
+	defer cleanUp()
+
+	// We'll now add two HTLC's from Bob to Alice, then Bob will initiate a
+	// state transition.
+	var htlcAmt lnwire.MilliSatoshi = 100000
+	htlc, _ := createHTLC(0, htlcAmt)
+	if _, err := aliceChannel.AddHTLC(htlc); err != nil {
+		t.Fatalf("unable to add htlc: %v", err)
+	}
+	if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
+		t.Fatalf("unable to recv htlc: %v", err)
+	}
+	htlc2, _ := createHTLC(1, htlcAmt)
+	if _, err := aliceChannel.AddHTLC(htlc2); err != nil {
+		t.Fatalf("unable to add htlc2: %v", err)
+	}
+	if _, err := bobChannel.ReceiveHTLC(htlc2); err != nil {
+		t.Fatalf("unable to recv htlc2: %v", err)
+	}
+
+	// We'll now manually initiate a state transition between Alice and
+	// bob.
+	aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	if err != nil {
+		t.Fatal(err)
+	}
+	bobRevocation, err := bobChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+	bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Alice should detect that she doesn't need to forward any HTLC's.
+	aliceHtlcsToForward, err := aliceChannel.ReceiveRevocation(
+		bobRevocation,
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(aliceHtlcsToForward) != 0 {
+		t.Fatalf("alice shouldn't forward any HTLC's, instead wants to "+
+			"forward %v htlcs", len(aliceHtlcsToForward))
+	}
+
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	if err != nil {
+		t.Fatal(err)
+	}
+	aliceRevocation, err := aliceChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Bob on the other hand, should detect that he now has 2 incoming
+	// HTLC's that he can forward along.
+	bobHtlcsToForward, err := bobChannel.ReceiveRevocation(aliceRevocation)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(bobHtlcsToForward) != 2 {
+		t.Fatalf("bob should forward 2 hltcs, instead has %v",
+			len(bobHtlcsToForward))
+	}
+
+	// We'll now restart both Alice and Bob. This emulates a reconnection
+	// between the two peers.
+	aliceChannel, err = restartChannel(aliceChannel)
+	if err != nil {
+		t.Fatalf("unable to restart alice: %v", err)
+	}
+	bobChannel, err = restartChannel(bobChannel)
+	if err != nil {
+		t.Fatalf("unable to restart bob: %v", err)
+	}
+
+	// With both nodes restarted, Bob will now attempt to cancel one of
+	// Alice's HTLC's.
+	err = bobChannel.FailHTLC(htlc2.ID, []byte("failreason"))
+	if err != nil {
+		t.Fatalf("unable to cancel HTLC: %v", err)
+	}
+	err = aliceChannel.ReceiveFailHTLC(htlc2.ID, []byte("bad"))
+	if err != nil {
+		t.Fatalf("unable to recv htlc cancel: %v", err)
+	}
+
+	// We'll now initiate another state transition, but this time Bob will
+	// lead.
+	bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	if err != nil {
+		t.Fatal(err)
+	}
+	aliceRevocation, err = aliceChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+	aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// At this point, Bob receives the revocation from Alice, which is now
+	// his signal to examine all the HTLC's that have been locked in to
+	// process.
+	bobHtlcsToForward, err = bobChannel.ReceiveRevocation(aliceRevocation)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Bob should detect that he doesn't need to forward *any* HTLC's, as
+	// he was the one that initiated extending the commitment chain of
+	// Alice.
+	if len(bobHtlcsToForward) != 0 {
+		t.Fatalf("bob shouldn't forward any htlcs, but has: %v",
+			spew.Sdump(bobHtlcsToForward))
+	}
+}
+
 // TestInvalidCommitSigError tests that if the remote party sends us an invalid
 // commitment signature, then we'll reject it and return a special error that
 // contains information to allow the remote party to debug their issues.