Merge pull request #1294 from halseth/add-commit-height

lnwallet/channel: set add heights based on the locked in commits

lnwallet/channel.go

@@ -714,8 +714,8 @@ func (c *commitment) toDiskCommit(ourCommit bool) *channeldb.ChannelCommitment {
 // restore commitment state written do disk back into memory once we need to
 // restart a channel session.
 func (lc *LightningChannel) diskHtlcToPayDesc(feeRate SatPerKWeight,
-	commitHeight uint64, isPendingCommit bool, htlc *channeldb.HTLC,
+	commitHeight uint64, htlc *channeldb.HTLC, localCommitKeys,
-	localCommitKeys, remoteCommitKeys *CommitmentKeyRing) (PaymentDescriptor, error) {
+	remoteCommitKeys *CommitmentKeyRing) (PaymentDescriptor, error) {
 
 	// The proper pkScripts for this PaymentDescriptor must be
 	// generated so we can easily locate them within the commitment
@@ -770,18 +770,6 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate SatPerKWeight,
 		theirWitnessScript: theirWitnessScript,
 	}
 
-	// If this is a pending commit, then the HTLC was only included in the
-	// commitment of the remote party, so we only set that commit height.
-	// Otherwise, we'll set the commit height for both chains as the HTLC
-	// was written to disk after it was fully locked in.
-	if isPendingCommit {
-		pd.addCommitHeightRemote = commitHeight
-	} else {
-		pd.addCommitHeightRemote = commitHeight
-		pd.addCommitHeightLocal = commitHeight
-
-	}
-
 	return pd, nil
 }
 
@@ -790,8 +778,7 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate SatPerKWeight,
 // these payment descriptors can be re-inserted into the in-memory updateLog
 // for each side.
 func (lc *LightningChannel) extractPayDescs(commitHeight uint64,
-	isPendingCommit bool, feeRate SatPerKWeight,
+	feeRate SatPerKWeight, htlcs []channeldb.HTLC, localCommitKeys,
-	htlcs []channeldb.HTLC, localCommitKeys *CommitmentKeyRing,
 	remoteCommitKeys *CommitmentKeyRing) ([]PaymentDescriptor, []PaymentDescriptor, error) {
 
 	var (
@@ -808,7 +795,7 @@ func (lc *LightningChannel) extractPayDescs(commitHeight uint64,
 		// inadvertently trigger replays
 
 		payDesc, err := lc.diskHtlcToPayDesc(
-			feeRate, commitHeight, isPendingCommit, &htlc,
+			feeRate, commitHeight, &htlc,
 			localCommitKeys, remoteCommitKeys,
 		)
 		if err != nil {
@@ -828,9 +815,9 @@ func (lc *LightningChannel) extractPayDescs(commitHeight uint64,
 // diskCommitToMemCommit converts the on-disk commitment format to our
 // in-memory commitment format which is needed in order to properly resume
 // channel operations after a restart.
-func (lc *LightningChannel) diskCommitToMemCommit(isLocal, isPendingCommit bool,
+func (lc *LightningChannel) diskCommitToMemCommit(isLocal bool,
-	diskCommit *channeldb.ChannelCommitment,
+	diskCommit *channeldb.ChannelCommitment, localCommitPoint,
-	localCommitPoint, remoteCommitPoint *btcec.PublicKey) (*commitment, error) {
+	remoteCommitPoint *btcec.PublicKey) (*commitment, error) {
 
 	// First, we'll need to re-derive the commitment key ring for each
 	// party used within this particular state. If this is a pending commit
@@ -851,9 +838,8 @@ func (lc *LightningChannel) diskCommitToMemCommit(isLocal, isPendingCommit bool,
 	// HTLC"s into PaymentDescriptor's so we can re-insert them into our
 	// update log.
 	incomingHtlcs, outgoingHtlcs, err := lc.extractPayDescs(
-		diskCommit.CommitHeight, isPendingCommit,
+		diskCommit.CommitHeight, SatPerKWeight(diskCommit.FeePerKw),
-		SatPerKWeight(diskCommit.FeePerKw), diskCommit.Htlcs,
+		diskCommit.Htlcs, localCommitKeys, remoteCommitKeys,
-		localCommitKeys, remoteCommitKeys,
 	)
 	if err != nil {
 		return nil, err
@@ -1620,7 +1606,7 @@ func (lc *LightningChannel) restoreCommitState(
 	// commitment into our in-memory commitment format, inserting it into
 	// the local commitment chain.
 	localCommit, err := lc.diskCommitToMemCommit(
-		true, false, localCommitState, localCommitPoint,
+		true, localCommitState, localCommitPoint,
 		remoteCommitPoint,
 	)
 	if err != nil {
@@ -1636,7 +1622,7 @@ func (lc *LightningChannel) restoreCommitState(
 
 	// We'll also do the same for the remote commitment chain.
 	remoteCommit, err := lc.diskCommitToMemCommit(
-		false, false, remoteCommitState, localCommitPoint,
+		false, remoteCommitState, localCommitPoint,
 		remoteCommitPoint,
 	)
 	if err != nil {
@@ -1671,7 +1657,7 @@ func (lc *LightningChannel) restoreCommitState(
 		// corresponding state for the local commitment chain.
 		pendingCommitPoint := lc.channelState.RemoteNextRevocation
 		pendingRemoteCommit, err = lc.diskCommitToMemCommit(
-			false, true, &pendingRemoteCommitDiff.Commitment,
+			false, &pendingRemoteCommitDiff.Commitment,
 			nil, pendingCommitPoint,
 		)
 		if err != nil {
@@ -1716,12 +1702,52 @@ func (lc *LightningChannel) restoreStateLogs(
 	pendingRemoteCommitDiff *channeldb.CommitDiff,
 	pendingRemoteKeys *CommitmentKeyRing) error {
 
+	// We make a map of incoming HTLCs to the height of the remote
+	// commitment they were first added, and outgoing HTLCs to the height
+	// of the local commit they were first added. This will be used when we
+	// restore the update logs below.
+	incomingRemoteAddHeights := make(map[uint64]uint64)
+	outgoingLocalAddHeights := make(map[uint64]uint64)
+
+	// We start by setting the height of the incoming HTLCs on the pending
+	// remote commitment. We set these heights first since if there are
+	// duplicates, these will be overwritten by the lower height of the
+	// remoteCommitment below.
+	if pendingRemoteCommit != nil {
+		for _, r := range pendingRemoteCommit.incomingHTLCs {
+			incomingRemoteAddHeights[r.HtlcIndex] =
+				pendingRemoteCommit.height
+		}
+	}
+
+	// Now set the remote commit height of all incoming HTLCs found on the
+	// remote commitment.
+	for _, r := range remoteCommitment.incomingHTLCs {
+		incomingRemoteAddHeights[r.HtlcIndex] = remoteCommitment.height
+	}
+
+	// And finally we can do the same for the outgoing HTLCs.
+	for _, l := range localCommitment.outgoingHTLCs {
+		outgoingLocalAddHeights[l.HtlcIndex] = localCommitment.height
+	}
+
 	// For each incoming HTLC within the local commitment, we add it to the
 	// remote update log. Since HTLCs are added first to the receiver's
 	// commitment, we don't have to restore outgoing HTLCs, as they will be
 	// restored from the remote commitment below.
 	for i := range localCommitment.incomingHTLCs {
 		htlc := localCommitment.incomingHTLCs[i]
+
+		// We'll need to set the add height of the HTLC. Since it is on
+		// this local commit, we can use its height as local add
+		// height. As remote add height we consult the incoming HTLC
+		// map we created earlier. Note that if this HTLC is not in
+		// incomingRemoteAddHeights, the remote add height will be set
+		// to zero, which indicates that it is not added yet.
+		htlc.addCommitHeightLocal = localCommitment.height
+		htlc.addCommitHeightRemote = incomingRemoteAddHeights[htlc.HtlcIndex]
+
+		// Restore the htlc back to the remote log.
 		lc.remoteUpdateLog.restoreHtlc(&htlc)
 	}
 
@@ -1729,6 +1755,14 @@ func (lc *LightningChannel) restoreStateLogs(
 	// remote commitment, adding them to the local update log.
 	for i := range remoteCommitment.outgoingHTLCs {
 		htlc := remoteCommitment.outgoingHTLCs[i]
+
+		// As for the incoming HTLCs, we'll use the current remote
+		// commit height as remote add height, and consult the map
+		// created above for the local add height.
+		htlc.addCommitHeightRemote = remoteCommitment.height
+		htlc.addCommitHeightLocal = outgoingLocalAddHeights[htlc.HtlcIndex]
+
+		// Restore the htlc back to the local log.
 		lc.localUpdateLog.restoreHtlc(&htlc)
 	}
 

lnwallet/channel_test.go

@@ -5683,3 +5683,194 @@ func TestDuplicateSettleRejection(t *testing.T) {
 		t.Fatalf("unable to recv htlc cancel: %v", err)
 	}
 }
+
+// TestChannelRestoreCommitHeight tests that the local and remote commit
+// heights of HTLCs are set correctly across restores.
+func TestChannelRestoreCommitHeight(t *testing.T) {
+	t.Parallel()
+
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels()
+	if err != nil {
+		t.Fatalf("unable to create test channels: %v", err)
+	}
+	defer cleanUp()
+
+	// helper method to check add heights of the htlcs found in the given
+	// log after a restore.
+	restoreAndAssertCommitHeights := func(t *testing.T,
+		channel *LightningChannel, remoteLog bool, htlcIndex uint64,
+		expLocal, expRemote uint64) *LightningChannel {
+
+		channel.Stop()
+		newChannel, err := NewLightningChannel(
+			channel.Signer, nil, channel.channelState,
+		)
+		if err != nil {
+			t.Fatalf("unable to create new channel: %v", err)
+		}
+
+		var pd *PaymentDescriptor
+		if remoteLog {
+			if newChannel.localUpdateLog.lookupHtlc(htlcIndex) != nil {
+				t.Fatalf("htlc found in wrong log")
+			}
+			pd = newChannel.remoteUpdateLog.lookupHtlc(htlcIndex)
+
+		} else {
+			if newChannel.remoteUpdateLog.lookupHtlc(htlcIndex) != nil {
+				t.Fatalf("htlc found in wrong log")
+			}
+			pd = newChannel.localUpdateLog.lookupHtlc(htlcIndex)
+		}
+		if pd == nil {
+			t.Fatalf("htlc not found in log")
+		}
+
+		if pd.addCommitHeightLocal != expLocal {
+			t.Fatalf("expected local add height to be %d, was %d",
+				expLocal, pd.addCommitHeightLocal)
+		}
+		if pd.addCommitHeightRemote != expRemote {
+			t.Fatalf("expected remote add height to be %d, was %d",
+				expRemote, pd.addCommitHeightRemote)
+		}
+		return newChannel
+	}
+
+	// We'll send an HtLC from Alice to Bob.
+	htlcAmount := lnwire.NewMSatFromSatoshis(100000000)
+	htlcAlice, _ := createHTLC(0, htlcAmount)
+	if _, err := aliceChannel.AddHTLC(htlcAlice, nil); err != nil {
+		t.Fatalf("alice unable to add htlc: %v", err)
+	}
+	if _, err := bobChannel.ReceiveHTLC(htlcAlice); err != nil {
+		t.Fatalf("bob unable to recv add htlc: %v", err)
+	}
+
+	// Let Alice sign a new state, which will include the HTLC just sent.
+	aliceSig, aliceHtlcSigs, err := aliceChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatalf("unable to sign commitment: %v", err)
+	}
+
+	// The HTLC should only be on the pending remote commitment, so the
+	// only the remote add height should be set during a restore.
+	aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false,
+		0, 0, 1)
+
+	// Bob receives this commitment signature, and revokes his old state.
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	if err != nil {
+		t.Fatalf("unable to receive commitment: %v", err)
+	}
+	bobRevocation, _, err := bobChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatalf("unable to revoke commitment: %v", err)
+	}
+
+	// Now the HTLC is locked into Bob's commitment, a restoration should
+	// set only the local commit height, as it is not locked into Alice's
+	// yet.
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 0)
+
+	// Alice receives the revocation, ACKing her pending commitment.
+	_, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation)
+	if err != nil {
+		t.Fatalf("unable to recive revocation: %v", err)
+	}
+
+	// However, the HTLC is still not locked into her local commitment, so
+	// the local add height should still be 0 after a restoration.
+	aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false,
+		0, 0, 1)
+
+	// Now let Bob send the commitment signature making the HTLC lock in on
+	// Alice's commitment.
+	bobSig, bobHtlcSigs, err := bobChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatalf("unable to sign commitment: %v", err)
+	}
+
+	// At this stage Bob has a pending remote commitment. Make sure
+	// restoring at this stage correcly restores the HTLC add commit
+	// heights.
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 1)
+
+	err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
+	if err != nil {
+		t.Fatalf("unable to receive commitment: %v", err)
+	}
+	aliceRevocation, _, err := aliceChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatalf("unable to revoke commitment: %v", err)
+	}
+
+	// Now both the local and remote add heights should be properly set.
+	aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false,
+		0, 1, 1)
+
+	_, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation)
+	if err != nil {
+		t.Fatalf("unable to recive revocation: %v", err)
+	}
+
+	// Alice ACKing Bob's pending commitment shouldn't change the heights
+	// restored.
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 1, 1)
+
+	// Send andother HTLC from Alice to Bob, to test whether already
+	// existing HTLCs (the HTLC with index 0) keep getting the add heights
+	// restored properly.
+	htlcAlice, _ = createHTLC(1, htlcAmount)
+	if _, err := aliceChannel.AddHTLC(htlcAlice, nil); err != nil {
+		t.Fatalf("alice unable to add htlc: %v", err)
+	}
+	if _, err := bobChannel.ReceiveHTLC(htlcAlice); err != nil {
+		t.Fatalf("bob unable to recv add htlc: %v", err)
+	}
+
+	// Send a new signature from Alice to Bob, making Alice have a pending
+	// remote commitment.
+	aliceSig, aliceHtlcSigs, err = aliceChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatalf("unable to sign commitment: %v", err)
+	}
+
+	// A restoration should keep the add heights iof the first HTLC, and
+	// the new HTLC should have a remote add height 2.
+	aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false,
+		0, 1, 1)
+	aliceChannel = restoreAndAssertCommitHeights(t, aliceChannel, false,
+		1, 0, 2)
+
+	err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
+	if err != nil {
+		t.Fatalf("unable to receive commitment: %v", err)
+	}
+	bobRevocation, _, err = bobChannel.RevokeCurrentCommitment()
+	if err != nil {
+		t.Fatalf("unable to revoke commitment: %v", err)
+	}
+
+	// Since Bob just revoked another commitment, a restoration should
+	// increase the add height of the firt HTLC to 2, as we only keep the
+	// last unrevoked commitment. The new HTLC will also have a local add
+	// height of 2.
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 2, 1)
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 1, 2, 0)
+
+	// Sign a new state for Alice, making Bob have a pending remote
+	// commitment.
+	bobSig, bobHtlcSigs, err = bobChannel.SignNextCommitment()
+	if err != nil {
+		t.Fatalf("unable to sign commitment: %v", err)
+	}
+
+	// The signing of a new commitment for Alice should have given the new
+	// HTLC an add height.
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 0, 2, 1)
+	bobChannel = restoreAndAssertCommitHeights(t, bobChannel, true, 1, 2, 2)
+
+	aliceChannel.Stop()
+	bobChannel.Stop()
+}