Merge pull request #5231 from Crypt-iQ/unsigned_ack_patch_04212021

lnwallet: use tail not tip in getUnsignedAckedUpdates

channeldb/channel.go

@@ -2396,8 +2396,9 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg,
 		// 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.
+			// This shouldn't normally happen as we always store
+			// the number of updates, but could still be
+			// encountered by nodes that are upgrading.
 			newRemoteCommit = &newCommit.Commitment
 			return nil
 		}

lnwallet/channel.go

@@ -2120,6 +2120,8 @@ func (lc *LightningChannel) restorePendingLocalUpdates(
 	// If we did have a dangling commit, then we'll examine which updates
 	// we included in that state and re-insert them into our update log.
 	for _, logUpdate := range pendingRemoteCommitDiff.LogUpdates {
+		logUpdate := logUpdate
+
 		payDesc, err := lc.logUpdateToPayDesc(
 			&logUpdate, lc.remoteUpdateLog, pendingHeight,
 			chainfee.SatPerKWeight(pendingCommit.FeePerKw),
@@ -3258,7 +3260,7 @@ func (lc *LightningChannel) getUnsignedAckedUpdates() []channeldb.LogUpdate {
 	chanID := lnwire.NewChanIDFromOutPoint(&lc.channelState.FundingOutpoint)
 
 	// Fetch the last remote update that we have signed for.
-	lastRemoteCommitted := lc.remoteCommitChain.tip().theirMessageIndex
+	lastRemoteCommitted := lc.remoteCommitChain.tail().theirMessageIndex
 
 	// Fetch the last remote update that we have acked.
 	lastLocalCommitted := lc.localCommitChain.tail().theirMessageIndex

lnwallet/channel_test.go

@@ -9521,3 +9521,125 @@ func TestChannelLocalUnsignedUpdatesFailure(t *testing.T) {
 	err = newAliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
 	require.NoError(t, err)
 }
+
+// TestChannelSignedAckRegression tests a previously-regressing state
+// transition no longer causes channel desynchronization.
+//
+// The full state transition of this test is:
+//
+// Alice                   Bob
+//        <----add-------
+//        <----sig-------
+//        -----rev------>
+//        -----sig------>
+//        <----rev-------
+//        ----settle---->
+//        -----sig------>
+//        <----rev-------
+//        <----sig-------
+//        -----add------>
+//        -----sig------>
+//        <----rev-------
+//                        *restarts*
+//        -----rev------>
+//        <----sig-------
+//
+func TestChannelSignedAckRegression(t *testing.T) {
+	t.Parallel()
+
+	// Create test channels to test out this state transition.
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(
+		channeldb.SingleFunderTweaklessBit,
+	)
+	require.NoError(t, err)
+	defer cleanUp()
+
+	// Create an HTLC that Bob will send to Alice.
+	htlc, preimage := createHTLC(0, lnwire.MilliSatoshi(5000000))
+
+	// <----add------
+	_, err = bobChannel.AddHTLC(htlc, nil)
+	require.NoError(t, err)
+	_, err = aliceChannel.ReceiveHTLC(htlc)
+	require.NoError(t, err)
+
+	// Force a state transition to lock in the HTLC.
+	// <----sig------
+	// -----rev----->
+	// -----sig----->
+	// <----rev------
+	err = ForceStateTransition(bobChannel, aliceChannel)
+	require.NoError(t, err)
+
+	// Alice settles the HTLC back to Bob.
+	// ----settle--->
+	err = aliceChannel.SettleHTLC(preimage, uint64(0), nil, nil, nil)
+	require.NoError(t, err)
+	err = bobChannel.ReceiveHTLCSettle(preimage, uint64(0))
+	require.NoError(t, err)
+
+	// -----sig---->
+	aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment()
+	require.NoError(t, err)
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	require.NoError(t, err)
+
+	// <----rev-----
+	bobRevocation, _, err := bobChannel.RevokeCurrentCommitment()
+	require.NoError(t, err)
+	_, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation)
+	require.NoError(t, err)
+
+	// <----sig-----
+	bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment()
+	require.NoError(t, err)
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	require.NoError(t, err)
+
+	// Create an HTLC that Alice will send to Bob.
+	htlc2, _ := createHTLC(0, lnwire.MilliSatoshi(5000000))
+
+	// -----add---->
+	_, err = aliceChannel.AddHTLC(htlc2, nil)
+	require.NoError(t, err)
+	_, err = bobChannel.ReceiveHTLC(htlc2)
+	require.NoError(t, err)
+
+	// -----sig---->
+	aliceSig, aliceHtlcSigs, _, err = aliceChannel.SignNextCommitment()
+	require.NoError(t, err)
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	require.NoError(t, err)
+
+	// <----rev-----
+	bobRevocation, _, err = bobChannel.RevokeCurrentCommitment()
+	require.NoError(t, err)
+	_, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation)
+	require.NoError(t, err)
+
+	// Restart Bob's channel state here.
+	newBobChannel, err := NewLightningChannel(
+		bobChannel.Signer, bobChannel.channelState,
+		bobChannel.sigPool,
+	)
+	require.NoError(t, err)
+
+	// -----rev---->
+	aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment()
+	require.NoError(t, err)
+	fwdPkg, _, _, _, err := newBobChannel.ReceiveRevocation(aliceRevocation)
+	require.NoError(t, err)
+
+	// Assert that the fwdpkg is not empty.
+	require.Equal(t, len(fwdPkg.SettleFails), 1)
+
+	// Bob should no longer fail to sign this commitment due to faulty
+	// update logs.
+	// <----sig-----
+	bobSig, bobHtlcSigs, _, err = newBobChannel.SignNextCommitment()
+	require.NoError(t, err)
+
+	// Alice should receive the new commitment without hiccups.
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	require.NoError(t, err)
+}