lnwallet: verify new optional ChannelReestablish fields in ProcessChanSyncMsg

In this commit, we add fully verification (other than checking the
commitment point matches after the fact) of the new optional fields
added to the lnwire.ChannelReestablish message. Two scenarios can
arise: we realize the remote party is on a prior state (and possibly
lost data), or we realize that *we* are on a prior state with the
remote party verifiably proving that they’re on a newer state.

lnwallet/channel.go

@@ -57,6 +57,18 @@ var (
 	// message, the state machine deems that is unable to properly
 	// synchronize states with the remote peer.
 	ErrCannotSyncCommitChains = fmt.Errorf("unable to sync commit chains")
+
+	// ErrInvalidLastCommitSecret is returned in the case that the
+	// commitment secret sent by the remote party in their
+	// ChannelReestablish message doesn't match the last secret we sent.
+	ErrInvalidLastCommitSecret = fmt.Errorf("commit secret is incorrect")
+
+	// ErrCommitSyncDataLoss is returned in the case that we receive a
+	// valid commit secret within the ChannelReestablish message from the
+	// remote node AND they advertise a RemoteCommitTailHeight higher than
+	// our current known height.
+	ErrCommitSyncDataLoss = fmt.Errorf("possible commitment state data " +
+		"loss")
 )
 
 // channelState is an enum like type which represents the current state of a
@@ -2022,6 +2034,8 @@ func (lc *LightningChannel) closeObserver(channelCloseNtfn *chainntnfs.SpendEven
 				err)
 		}
 
+		// TODO(roasbeef): need to handle case of if >
+
 		// First, we'll generate the commitment point and the
 		// revocation point so we can re-construct the HTLC state and
 		// also our payment key.
@@ -3054,10 +3068,41 @@ func (lc *LightningChannel) ProcessChanSyncMsg(msg *lnwire.ChannelReestablish) (
 	// resync.
 	var updates []lnwire.Message
 
+	// If the remote party included the optional fields, then we'll verify
+	// their correctness first, as it will influence our decisions below.
+	hasRecoveryOptions := msg.LocalUnrevokedCommitPoint != nil
+	commitSecretCorrect := true
+	if hasRecoveryOptions && msg.RemoteCommitTailHeight != 0 {
+		// We'll check that they've really sent a valid commit
+		// secret from our shachain for our prior height, but only if
+		// this isn't the first state.
+		heightSecret, err := lc.channelState.RevocationProducer.AtIndex(
+			msg.RemoteCommitTailHeight - 1,
+		)
+		if err != nil {
+			return nil, err
+		}
+		commitSecretCorrect = bytes.Equal(
+			heightSecret[:], msg.LastRemoteCommitSecret[:],
+		)
+	}
+
+	// TODO(roasbeef): check validity of commitment point after the fact
+
+	// If the commit secret they sent is incorrect then we'll fail the
+	// channel as the remote node has an inconsistent state.
+	if !commitSecretCorrect {
+		// In this case, we'll return an error to indicate the remote
+		// node sent us the wrong values. This will let the caller act
+		// accordingly.
+		return nil, ErrInvalidLastCommitSecret
+	}
+
+	switch {
 	// If we owe the remote party a revocation message, then we'll re-send
 	// the last revocation message that we sent. This will be the
 	// revocation message for our prior chain tail.
-	if oweRevocation {
+	case oweRevocation:
 		revocationMsg, err := lc.generateRevocation(
 			localChainTail.height - 1,
 		)
@@ -3087,10 +3132,21 @@ func (lc *LightningChannel) ProcessChanSyncMsg(msg *lnwire.ChannelReestablish) (
 			})
 		}
 
-	} else if !oweRevocation && localChainTail.height != msg.RemoteCommitTailHeight {
-		// If we don't owe them a revocation, and the height of our
-		// commitment chain reported by the remote party is not equal
-		// to our chain tail, then we cannot sync.
+	// If we don't owe the remote party a revocation, but their value for
+	// what our remote chain tail should be doesn't match up, and their
+	// purported commitment secrete matches up, then we'll behind!
+	case (msg.RemoteCommitTailHeight > localChainTail.height &&
+		hasRecoveryOptions && commitSecretCorrect):
+
+		// In this case, we've likely lost data and shouldn't proceed
+		// with channel updates. So we'll return the appropriate error
+		// to signal to the caller the current state.
+		return nil, ErrCommitSyncDataLoss
+
+	// If we don't owe them a revocation, and the height of our commitment
+	// chain reported by the remote party is not equal to our chain tail,
+	// then we cannot sync.
+	case !oweRevocation && localChainTail.height != msg.RemoteCommitTailHeight:
 		return nil, ErrCannotSyncCommitChains
 	}
 

lnwallet/channel_test.go

@@ -3407,6 +3407,97 @@ func TestChanSyncUnableToSync(t *testing.T) {
 	}
 }
 
+// TestChanSyncInvalidLastSecret ensures that if Alice and Bob have completed
+// state transitions in an existing channel, and then send a ChannelReestablish
+// message after a restart, the following holds: if Alice has lost data, so she
+// sends an invalid commit secret then both parties recognize this as possible
+// data loss.
+func TestChanSyncInvalidLastSecret(t *testing.T) {
+	t.Parallel()
+
+	// Create a test channel which will be used for the duration of this
+	// unittest. The channel will be funded evenly with Alice having 5 BTC,
+	// and Bob having 5 BTC.
+	aliceChannel, bobChannel, cleanUp, err := createTestChannels(1)
+	if err != nil {
+		t.Fatalf("unable to create test channels: %v", err)
+	}
+	defer cleanUp()
+
+	// We'll create a new instances of Alice before doing any state updates
+	// such that we have the initial in memory state at the start of the
+	// channel.
+	aliceOld, err := restartChannel(aliceChannel)
+	if err != nil {
+		t.Fatalf("unable to restart alice")
+	}
+
+	// First, we'll add an HTLC, and then initiate a state transition
+	// between the two parties such that we actually have a prior
+	// revocation to send.
+	var paymentPreimage [32]byte
+	copy(paymentPreimage[:], bytes.Repeat([]byte{1}, 32))
+	paymentHash := sha256.Sum256(paymentPreimage[:])
+	htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin)
+	htlc := &lnwire.UpdateAddHTLC{
+		PaymentHash: paymentHash,
+		Amount:      htlcAmt,
+		Expiry:      uint32(5),
+	}
+	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)
+	}
+
+	// Then we'll initiate a state transition to lock in this new HTLC.
+	if err := forceStateTransition(aliceChannel, bobChannel); err != nil {
+		t.Fatalf("unable to complete alice's state transition: %v", err)
+	}
+
+	// Next, we'll restart both parties in order to simulate a connection
+	// re-establishment.
+	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)
+	}
+
+	// Next, we'll produce the ChanSync messages for both parties.
+	aliceChanSync, err := aliceChannel.ChanSyncMsg()
+	if err != nil {
+		t.Fatalf("unable to generate chan sync msg: %v", err)
+	}
+	bobChanSync, err := bobChannel.ChanSyncMsg()
+	if err != nil {
+		t.Fatalf("unable to generate chan sync msg: %v", err)
+	}
+
+	// We'll modify Alice's sync message to have an invalid commitment
+	// secret.
+	aliceChanSync.LastRemoteCommitSecret[4] ^= 0x01
+
+	// Alice's former self should conclude that she possibly lost data as
+	// Bob is sending a valid commit secret for the latest state.
+	_, err = aliceOld.ProcessChanSyncMsg(bobChanSync)
+	if err != ErrCommitSyncDataLoss {
+		t.Fatalf("wrong error, expected ErrCommitSyncDataLoss "+
+			"instead got: %v", err)
+	}
+
+	// Bob should conclude that he should force close the channel, as Alice
+	// cannot continue operation.
+	_, err = bobChannel.ProcessChanSyncMsg(aliceChanSync)
+	if err != ErrInvalidLastCommitSecret {
+		t.Fatalf("wrong error, expected ErrInvalidLastCommitSecret, "+
+			"instead got: %v", err)
+	}
+}
+
 // TestChanAvailableBandwidth tests the accuracy of the AvailableBalance()
 // method. The value returned from this message should reflect the value
 // returned within the commitment state of a channel after the transition is