Merge pull request #4431 from Crypt-iQ/unsigned_acked_fix_0702

channeldb/channel.go: remove unsignedAckedUpdatesKey when we receive revocation

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

lnwallet/channel.go

@@ -1867,6 +1867,7 @@ func (lc *LightningChannel) restoreStateLogs(
 	// in our next signature.
 	err := lc.restorePendingRemoteUpdates(
 		unsignedAckedUpdates, localCommitment.height,
+		pendingRemoteCommit,
 	)
 	if err != nil {
 		return err
@@ -1879,7 +1880,8 @@ func (lc *LightningChannel) restoreStateLogs(
 // haven't yet signed for.
 func (lc *LightningChannel) restorePendingRemoteUpdates(
 	unsignedAckedUpdates []channeldb.LogUpdate,
-	localCommitmentHeight uint64) error {
+	localCommitmentHeight uint64,
+	pendingRemoteCommit *commitment) error {
 
 	lc.log.Debugf("Restoring %v dangling remote updates",
 		len(unsignedAckedUpdates))
@@ -1894,12 +1896,38 @@ func (lc *LightningChannel) restorePendingRemoteUpdates(
 			return err
 		}
 
+		logIdx := payDesc.LogIndex
+
 		// Sanity check that we are not restoring a remote log update
 		// that we haven't received a sig for.
-		if payDesc.LogIndex >= lc.remoteUpdateLog.logIndex {
+		if logIdx >= lc.remoteUpdateLog.logIndex {
 			return fmt.Errorf("attempted to restore an "+
 				"unsigned remote update: log_index=%v",
-				payDesc.LogIndex)
+				logIdx)
+		}
+
+		// We previously restored Adds along with all the other upates,
+		// but this Add restoration was a no-op as every single one of
+		// these Adds was already restored since they're all incoming
+		// htlcs on the local commitment.
+		if payDesc.EntryType == Add {
+			continue
+		}
+
+		var (
+			height    uint64
+			heightSet bool
+		)
+
+		// If we have a pending commitment for them, and this update
+		// is included in that commit, then we'll use this commitment
+		// height as this commitment will include these updates for
+		// their new remote commitment.
+		if pendingRemoteCommit != nil {
+			if logIdx < pendingRemoteCommit.theirMessageIndex {
+				height = pendingRemoteCommit.height
+				heightSet = true
+			}
 		}
 
 		// Insert the update into the log. The log update index doesn't
@@ -1907,23 +1935,20 @@ func (lc *LightningChannel) restorePendingRemoteUpdates(
 		// final value was properly persisted with the last local
 		// commitment update.
 		switch payDesc.EntryType {
-		case Add:
-			lc.remoteUpdateLog.restoreHtlc(payDesc)
-
-			// Sanity check to be sure that we are not restoring an
-			// add update that the remote hasn't signed for yet.
-			if payDesc.HtlcIndex >= lc.remoteUpdateLog.htlcCounter {
-				return fmt.Errorf("attempted to restore an "+
-					"unsigned remote htlc: htlc_index=%v",
-					payDesc.HtlcIndex)
+		case FeeUpdate:
+			if heightSet {
+				payDesc.addCommitHeightRemote = height
+				payDesc.removeCommitHeightRemote = height
 			}
 
-		case FeeUpdate:
 			lc.remoteUpdateLog.restoreUpdate(payDesc)
 
 		default:
-			lc.remoteUpdateLog.restoreUpdate(payDesc)
+			if heightSet {
+				payDesc.removeCommitHeightRemote = height
+			}
 
+			lc.remoteUpdateLog.restoreUpdate(payDesc)
 			lc.localUpdateLog.markHtlcModified(payDesc.ParentIndex)
 		}
 	}

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
@@ -9096,3 +9096,119 @@ func TestProcessAddRemoveEntry(t *testing.T) {
 		})
 	}
 }
+
+// TestChannelUnsignedAckedFailure tests that unsigned acked updates are
+// properly restored after signing for them and disconnecting.
+//
+// The full state transition of this test is:
+//
+// Alice                   Bob
+//         -----add----->
+//         -----sig----->
+//         <----rev------
+//         <----sig------
+//         -----rev----->
+//         <----fail-----
+//         <----sig------
+//         -----rev----->
+//         -----sig-----X (does not reach Bob! Alice dies!)
+//
+//         -----sig----->
+//         <----rev------
+//         <----add------
+//         <----sig------
+//
+// The last sig was rejected with the old behavior of deleting unsigned
+// acked updates from the database after signing for them. The current
+// behavior of filtering them for deletion upon receiving a revocation
+// causes Alice to accept the signature as valid.
+func TestChannelUnsignedAckedFailure(t *testing.T) {
+	t.Parallel()
+
+	// Create a test channel so that we can test the buggy behavior.
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(
+		channeldb.SingleFunderTweaklessBit,
+	)
+	require.NoError(t, err)
+	defer cleanUp()
+
+	// First we create an HTLC that Alice sends to Bob.
+	htlc, _ := createHTLC(0, lnwire.MilliSatoshi(500000))
+
+	// -----add----->
+	_, err = aliceChannel.AddHTLC(htlc, nil)
+	require.NoError(t, err)
+	_, err = bobChannel.ReceiveHTLC(htlc)
+	require.NoError(t, err)
+
+	// Force a state transition to lock in this add on both commitments.
+	// -----sig----->
+	// <----rev------
+	// <----sig------
+	// -----rev----->
+	err = ForceStateTransition(aliceChannel, bobChannel)
+	require.NoError(t, err)
+
+	// Now Bob should fail the htlc back to Alice.
+	// <----fail-----
+	err = bobChannel.FailHTLC(0, []byte("failreason"), nil, nil, nil)
+	require.NoError(t, err)
+	err = aliceChannel.ReceiveFailHTLC(0, []byte("bad"))
+	require.NoError(t, err)
+
+	// Bob should send a commitment signature to Alice.
+	// <----sig------
+	bobSig, bobHtlcSigs, _, err := bobChannel.SignNextCommitment()
+	require.NoError(t, err)
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	require.NoError(t, err)
+
+	// Alice should reply with a revocation.
+	// -----rev----->
+	aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment()
+	require.NoError(t, err)
+	_, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation)
+	require.NoError(t, err)
+
+	// Alice should sign the next commitment and go down before
+	// sending it.
+	// -----sig-----X
+	aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment()
+	require.NoError(t, err)
+
+	newAliceChannel, err := NewLightningChannel(
+		aliceChannel.Signer, aliceChannel.channelState,
+		aliceChannel.sigPool,
+	)
+	require.NoError(t, err)
+
+	// Bob receives Alice's signature.
+	// -----sig----->
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	require.NoError(t, err)
+
+	// Bob revokes his current commitment and sends a revocation
+	// to Alice.
+	// <----rev------
+	bobRevocation, _, err := bobChannel.RevokeCurrentCommitment()
+	require.NoError(t, err)
+	_, _, _, _, err = newAliceChannel.ReceiveRevocation(bobRevocation)
+	require.NoError(t, err)
+
+	// Now Bob sends an HTLC to Alice.
+	htlc2, _ := createHTLC(0, lnwire.MilliSatoshi(500000))
+
+	// <----add------
+	_, err = bobChannel.AddHTLC(htlc2, nil)
+	require.NoError(t, err)
+	_, err = newAliceChannel.ReceiveHTLC(htlc2)
+	require.NoError(t, err)
+
+	// Bob sends the final signature to Alice and Alice should not
+	// reject it, given that we properly restore the unsigned acked
+	// updates and therefore our update log is structured correctly.
+	bobSig, bobHtlcSigs, _, err = bobChannel.SignNextCommitment()
+	require.NoError(t, err)
+	err = newAliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	require.NoError(t, err)
+}