From f59b4d62bfe04387e81e3783684c4ba4b79ac3df Mon Sep 17 00:00:00 2001
From: Joost Jager <joost.jager@gmail.com>
Date: Mon, 23 Sep 2019 14:27:06 +0200
Subject: [PATCH] htlcswitch: check for signature owed in link

Previously the channel method FullySynced was used to decide whether to
send a new commit sig message. However, it could happen that FullySynced
was false, but that we didn't owe a commitment signature. Instead we
were waiting on the other party to send us a signature. If that
happened, we'd send out an empty commit sig. This commit modifies the
condition that triggers a new commit sig and fixes this deviation from
the spec.
---
 htlcswitch/link.go      |  4 ++--
 htlcswitch/link_test.go | 14 ++++++--------
 lnwallet/channel.go     | 22 +---------------------
 3 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/htlcswitch/link.go b/htlcswitch/link.go
index 4da9c0db..20b58f23 100644
--- a/htlcswitch/link.go
+++ b/htlcswitch/link.go
@@ -1103,7 +1103,7 @@ out:
 			// update in some time, check to see if we have any
 			// pending updates we need to commit due to our
 			// commitment chains being desynchronized.
-			if l.channel.FullySynced() {
+			if !l.channel.OweCommitment(true) {
 				continue
 			}
 
@@ -1808,7 +1808,7 @@ func (l *channelLink) handleUpstreamMsg(msg lnwire.Message) {
 		// If both commitment chains are fully synced from our PoV,
 		// then we don't need to reply with a signature as both sides
 		// already have a commitment with the latest accepted.
-		if l.channel.FullySynced() {
+		if !l.channel.OweCommitment(true) {
 			return
 		}
 
diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go
index 68d425b5..b09d0219 100644
--- a/htlcswitch/link_test.go
+++ b/htlcswitch/link_test.go
@@ -4699,11 +4699,10 @@ func TestChannelLinkNoEmptySig(t *testing.T) {
 	ctx.receiveRevAndAckAliceToBob()
 	ctx.sendRevAndAckBobToAlice()
 
-	// The situation now is that Alice still doesn't have her two htlcs on
-	// the local commit tx. Bob needs to send a new signature and Alice can
-	// only wait for that. However, Alice's log commit timer fires and Alice
-	// sends a commitment tx containing no updates. THIS SHOULD NOT HAPPEN!
-	ctx.receiveCommitSigAliceToBob(2)
+	// The commit txes are not in sync, but it is Bob's turn to send a new
+	// signature. We don't expect Alice to send out any message. This check
+	// allows some time for the log commit ticker to trigger for Alice.
+	ctx.assertNoMsgFromAlice(time.Second)
 }
 
 // TestChannelLinkBatchPreimageWrite asserts that a link will batch preimage
@@ -6109,9 +6108,8 @@ func TestChannelLinkReceiveEmptySig(t *testing.T) {
 	// Send RevokeAndAck Bob->Alice to ack the added htlc.
 	ctx.sendRevAndAckBobToAlice()
 
-	// No new updates to sign, Alice still sends out an empty sig. THIS
-	// SHOULD NOT HAPPEN!
-	ctx.receiveCommitSigAliceToBob(1)
+	// We received an empty commit sig, we accepted it, but there is nothing
+	// new to sign for us.
 
 	// No other messages are expected.
 	ctx.assertNoMsgFromAlice(time.Second)
diff --git a/lnwallet/channel.go b/lnwallet/channel.go
index 5a87aca8..c1ac5967 100644
--- a/lnwallet/channel.go
+++ b/lnwallet/channel.go
@@ -3535,7 +3535,7 @@ func (lc *LightningChannel) ProcessChanSyncMsg(
 		// but died before the signature was sent. We re-transmit our
 		// revocation, but also initiate a state transition to re-sync
 		// them.
-		if !lc.FullySynced() {
+		if lc.OweCommitment(true) {
 			commitSig, htlcSigs, _, err := lc.SignNextCommitment()
 			switch {
 
@@ -4225,26 +4225,6 @@ func (lc *LightningChannel) oweCommitment(local bool) bool {
 	return oweCommitment
 }
 
-// FullySynced returns a boolean value reflecting if both commitment chains
-// (remote+local) are fully in sync. Both commitment chains are fully in sync
-// if the tip of each chain includes the latest committed changes from both
-// sides.
-func (lc *LightningChannel) FullySynced() bool {
-	lc.RLock()
-	defer lc.RUnlock()
-
-	lastLocalCommit := lc.localCommitChain.tip()
-	lastRemoteCommit := lc.remoteCommitChain.tip()
-
-	localUpdatesSynced := (lastLocalCommit.ourMessageIndex ==
-		lastRemoteCommit.ourMessageIndex)
-
-	remoteUpdatesSynced := (lastLocalCommit.theirMessageIndex ==
-		lastRemoteCommit.theirMessageIndex)
-
-	return localUpdatesSynced && remoteUpdatesSynced
-}
-
 // RevokeCurrentCommitment revokes the next lowest unrevoked commitment
 // transaction in the local commitment chain. As a result the edge of our
 // revocation window is extended by one, and the tail of our local commitment