chanstate: match active htlcs by identity

ActiveHtlcs previously matched HTLCs across the local and remote
commitment snapshots by hashing the onion blob. The onion blob is
routing payload data and can be duplicated by buggy or malicious
senders, so it is not a reliable key for identifying the same HTLC on
both commitments.

Match on the HTLC's channel identity instead: the channel-level HTLC
index combined with the direction of the offer uniquely identifies an
offered HTLC within the channel state. A test is added to lock in the
new matching behavior.

chanstate/open_channel.go

@@ -1,7 +1,6 @@
 package chanstate
 
 import (
-	"crypto/sha256"
 	"errors"
 	"fmt"
 	"net"
@@ -822,17 +821,33 @@ func (c *OpenChannel) ActiveHtlcs() []HTLC {
 	c.RLock()
 	defer c.RUnlock()
 
+	// htlcKey uniquely identifies an HTLC within the channel state by its
+	// channel-level HTLC index and the direction of the offer. This is used
+	// to match the same HTLC across the local and remote commitment
+	// snapshots.
+	type htlcKey struct {
+		index    uint64
+		incoming bool
+	}
+
 	// We'll only return HTLC's that are locked into *both* commitment
 	// transactions. So we'll iterate through their set of HTLC's to note
 	// which ones are present on their commitment.
-	remoteHtlcs := make(map[[32]byte]struct{})
+	//
+	// HTLC identity is defined by the channel-level HTLC index plus the
+	// direction of the offer. The onion blob is routing payload data and
+	// can be duplicated by buggy or malicious senders, so it is not a
+	// robust key for matching the same HTLC across commitment snapshots.
+	remoteHtlcs := make(map[htlcKey]struct{})
 	for _, htlc := range c.RemoteCommitment.Htlcs {
 		log.Tracef("RemoteCommitment has htlc: id=%v, update=%v "+
 			"incoming=%v", htlc.HtlcIndex, htlc.LogIndex,
 			htlc.Incoming)
 
-		onionHash := sha256.Sum256(htlc.OnionBlob[:])
-		remoteHtlcs[onionHash] = struct{}{}
+		remoteHtlcs[htlcKey{
+			index:    htlc.HtlcIndex,
+			incoming: htlc.Incoming,
+		}] = struct{}{}
 	}
 
 	// Now that we know which HTLC's they have, we'll only mark the HTLC's
@@ -843,9 +858,12 @@ func (c *OpenChannel) ActiveHtlcs() []HTLC {
 			"incoming=%v", htlc.HtlcIndex, htlc.LogIndex,
 			htlc.Incoming)
 
-		onionHash := sha256.Sum256(htlc.OnionBlob[:])
-		if _, ok := remoteHtlcs[onionHash]; !ok {
-			log.Tracef("Skipped htlc due to onion mismatched: "+
+		_, ok := remoteHtlcs[htlcKey{
+			index:    htlc.HtlcIndex,
+			incoming: htlc.Incoming,
+		}]
+		if !ok {
+			log.Tracef("Skipped htlc due to identity mismatch: "+
 				"id=%v, update=%v incoming=%v",
 				htlc.HtlcIndex, htlc.LogIndex, htlc.Incoming)
 

chanstate/open_channel_test.go

@@ -0,0 +1,58 @@
+package chanstate
+
+import (
+	"testing"
+
+	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/stretchr/testify/require"
+)
+
+// TestActiveHtlcsMatchesByHTLCIdentity asserts that ActiveHtlcs matches HTLCs
+// by their channel identity, not by their onion blob. Onion blobs are routing
+// payload data and can be duplicated, while the HTLC index plus direction
+// identifies an offered HTLC within the channel state.
+func TestActiveHtlcsMatchesByHTLCIdentity(t *testing.T) {
+	t.Parallel()
+
+	var onionBlob [lnwire.OnionPacketSize]byte
+	onionBlob[0] = 1
+
+	matchingHTLC := HTLC{
+		HtlcIndex: 7,
+		LogIndex:  10,
+		Incoming:  false,
+		OnionBlob: onionBlob,
+	}
+	duplicateOnionHTLC := HTLC{
+		HtlcIndex: 8,
+		LogIndex:  11,
+		Incoming:  false,
+		OnionBlob: onionBlob,
+	}
+	oppositeDirectionHTLC := HTLC{
+		HtlcIndex: 7,
+		LogIndex:  12,
+		Incoming:  true,
+		OnionBlob: onionBlob,
+	}
+
+	channel := &OpenChannel{
+		LocalCommitment: ChannelCommitment{
+			Htlcs: []HTLC{
+				matchingHTLC,
+				duplicateOnionHTLC,
+				oppositeDirectionHTLC,
+			},
+		},
+		RemoteCommitment: ChannelCommitment{
+			Htlcs: []HTLC{
+				matchingHTLC,
+			},
+		},
+	}
+
+	activeHtlcs := channel.ActiveHtlcs()
+	require.Len(t, activeHtlcs, 1)
+	require.Equal(t, matchingHTLC.HtlcIndex, activeHtlcs[0].HtlcIndex)
+	require.Equal(t, matchingHTLC.Incoming, activeHtlcs[0].Incoming)
+}