Merge pull request #3142 from cfromknecht/htlc-tie-breaker

lnwallet: add commitment transaction sorting with CLTV tie breaker

lnwallet/channel.go

@@ -2411,7 +2411,13 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment,
 	// We'll now add all the HTLC outputs to the commitment transaction.
 	// Each output includes an off-chain 2-of-2 covenant clause, so we'll
 	// need the objective local/remote keys for this particular commitment
-	// as well.
+	// as well. For any non-dust HTLCs that are manifested on the commitment
+	// transaction, we'll also record its CLTV which is required to sort the
+	// commitment transaction below. The slice is initially sized to the
+	// number of existing outputs, since any outputs already added are
+	// commitment outputs and should correspond to zero values for the
+	// purposes of sorting.
+	cltvs := make([]uint32, len(commitTx.TxOut))
 	for _, htlc := range filteredHTLCView.ourUpdates {
 		if htlcIsDust(false, c.isOurs, c.feePerKw,
 			htlc.Amount.ToSatoshis(), c.dustLimit) {
@@ -2422,6 +2428,7 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment,
 		if err != nil {
 			return err
 		}
+		cltvs = append(cltvs, htlc.Timeout)
 	}
 	for _, htlc := range filteredHTLCView.theirUpdates {
 		if htlcIsDust(true, c.isOurs, c.feePerKw,
@@ -2433,6 +2440,7 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment,
 		if err != nil {
 			return err
 		}
+		cltvs = append(cltvs, htlc.Timeout)
 	}
 
 	// Set the state hint of the commitment transaction to facilitate
@@ -2446,7 +2454,7 @@ func (lc *LightningChannel) createCommitmentTx(c *commitment,
 	// Sort the transactions according to the agreed upon canonical
 	// ordering. This lets us skip sending the entire transaction over,
 	// instead we'll just send signatures.
-	txsort.InPlaceSort(commitTx)
+	InPlaceCommitSort(commitTx, cltvs)
 
 	// Next, we'll ensure that we don't accidentally create a commitment
 	// transaction which would be invalid by consensus.

lnwallet/commit_sort.go

@@ -0,0 +1,114 @@
+package lnwallet
+
+import (
+	"bytes"
+	"sort"
+
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/wire"
+)
+
+// InPlaceCommitSort performs an in-place sort of a commitment transaction,
+// given an unsorted transaction and a list of CLTV values for the HTLCs.
+//
+// The sort applied is a modified BIP69 sort, that uses the CLTV values of HTLCs
+// as a tie breaker in case two HTLC outputs have an identical amount and
+// pkscript. The pkscripts can be the same if they share the same payment hash,
+// but since the CLTV is enforced via the nLockTime of the second-layer
+// transactions, the script does not directly commit to them. Instead, the CLTVs
+// must be supplied separately to act as a tie-breaker, otherwise we may produce
+// invalid HTLC signatures if the receiver produces an alternative ordering
+// during verification.
+//
+// NOTE: Commitment outputs should have a 0 CLTV corresponding to their index on
+// the unsorted commitment transaction.
+func InPlaceCommitSort(tx *wire.MsgTx, cltvs []uint32) {
+	if len(tx.TxOut) != len(cltvs) {
+		panic("output and cltv list size mismatch")
+	}
+
+	sort.Sort(sortableInputSlice(tx.TxIn))
+	sort.Sort(sortableCommitOutputSlice{tx.TxOut, cltvs})
+}
+
+// sortableInputSlice is a slice of transaction inputs that supports sorting via
+// BIP69.
+type sortableInputSlice []*wire.TxIn
+
+// Len returns the length of the sortableInputSlice.
+//
+// NOTE: Part of the sort.Interface interface.
+func (s sortableInputSlice) Len() int { return len(s) }
+
+// Swap exchanges the position of inputs i and j.
+//
+// NOTE: Part of the sort.Interface interface.
+func (s sortableInputSlice) Swap(i, j int) {
+	s[i], s[j] = s[j], s[i]
+}
+
+// Less is the BIP69 input comparison function. The sort is first applied on
+// input hash (reversed / rpc-style), then index. This logic is copied from
+// btcutil/txsort.
+//
+// NOTE: Part of the sort.Interface interface.
+func (s sortableInputSlice) Less(i, j int) bool {
+	// Input hashes are the same, so compare the index.
+	ihash := s[i].PreviousOutPoint.Hash
+	jhash := s[j].PreviousOutPoint.Hash
+	if ihash == jhash {
+		return s[i].PreviousOutPoint.Index < s[j].PreviousOutPoint.Index
+	}
+
+	// At this point, the hashes are not equal, so reverse them to
+	// big-endian and return the result of the comparison.
+	const hashSize = chainhash.HashSize
+	for b := 0; b < hashSize/2; b++ {
+		ihash[b], ihash[hashSize-1-b] = ihash[hashSize-1-b], ihash[b]
+		jhash[b], jhash[hashSize-1-b] = jhash[hashSize-1-b], jhash[b]
+	}
+	return bytes.Compare(ihash[:], jhash[:]) == -1
+}
+
+// sortableCommitOutputSlice is a slice of transaction outputs on a commitment
+// transaction and the corresponding CLTV values of any HTLCs. Commitment
+// outputs should have a CLTV of 0 and the same index in cltvs.
+type sortableCommitOutputSlice struct {
+	txouts []*wire.TxOut
+	cltvs  []uint32
+}
+
+// Len returns the length of the sortableCommitOutputSlice.
+//
+// NOTE: Part of the sort.Interface interface.
+func (s sortableCommitOutputSlice) Len() int {
+	return len(s.txouts)
+}
+
+// Swap exchanges the position of outputs i and j, as well as their
+// corresponding CLTV values.
+//
+// NOTE: Part of the sort.Interface interface.
+func (s sortableCommitOutputSlice) Swap(i, j int) {
+	s.txouts[i], s.txouts[j] = s.txouts[j], s.txouts[i]
+	s.cltvs[i], s.cltvs[j] = s.cltvs[j], s.cltvs[i]
+}
+
+// Less is a modified BIP69 output comparison, that sorts based on value, then
+// pkscript, then CLTV value.
+//
+// NOTE: Part of the sort.Interface interface.
+func (s sortableCommitOutputSlice) Less(i, j int) bool {
+	outi, outj := s.txouts[i], s.txouts[j]
+
+	if outi.Value != outj.Value {
+		return outi.Value < outj.Value
+	}
+
+	pkScriptCmp := bytes.Compare(outi.PkScript, outj.PkScript)
+	if pkScriptCmp != 0 {
+		return pkScriptCmp < 0
+	}
+
+	return s.cltvs[i] < s.cltvs[j]
+}

lnwallet/commit_sort_test.go

@@ -0,0 +1,253 @@
+package lnwallet_test
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/btcsuite/btcd/wire"
+	"github.com/lightningnetwork/lnd/lnwallet"
+)
+
+type commitSortTest struct {
+	name     string
+	tx       *wire.MsgTx
+	cltvs    []uint32
+	intrans  map[int]int // input transformation
+	outtrans map[int]int // output transformation
+}
+
+func (t *commitSortTest) expTxIns() []*wire.TxIn {
+	if len(t.intrans) == 0 {
+		return nil
+	}
+
+	expTxIns := make([]*wire.TxIn, len(t.intrans))
+	for start, end := range t.intrans {
+		expTxIns[end] = t.tx.TxIn[start]
+	}
+
+	return expTxIns
+}
+
+func (t *commitSortTest) expTxOuts() []*wire.TxOut {
+	if len(t.outtrans) == 0 {
+		return nil
+	}
+
+	expTxOuts := make([]*wire.TxOut, len(t.outtrans))
+	for start, end := range t.outtrans {
+		expTxOuts[end] = t.tx.TxOut[start]
+	}
+
+	return expTxOuts
+}
+
+var commitSortTests = []commitSortTest{
+	{
+		name: "sort inputs on prevoutpoint txid",
+		tx: &wire.MsgTx{
+			TxIn: []*wire.TxIn{
+				{
+					PreviousOutPoint: wire.OutPoint{
+						Hash: [32]byte{0x01},
+					},
+				},
+				{
+					PreviousOutPoint: wire.OutPoint{
+						Hash: [32]byte{0x00},
+					},
+				},
+			},
+		},
+		intrans: map[int]int{
+			0: 1,
+			1: 0,
+		},
+	},
+	{
+		name: "sort inputs on prevoutpoint index",
+		tx: &wire.MsgTx{
+			TxIn: []*wire.TxIn{
+				{
+					PreviousOutPoint: wire.OutPoint{
+						Index: 1,
+					},
+				},
+				{
+					PreviousOutPoint: wire.OutPoint{
+						Index: 0,
+					},
+				},
+			},
+		},
+		intrans: map[int]int{
+			0: 1,
+			1: 0,
+		},
+	},
+	{
+		name: "inputs already sorted",
+		tx: &wire.MsgTx{
+			TxIn: []*wire.TxIn{
+				{
+					PreviousOutPoint: wire.OutPoint{
+						Index: 0,
+					},
+				},
+				{
+					PreviousOutPoint: wire.OutPoint{
+						Index: 1,
+					},
+				},
+			},
+		},
+		intrans: map[int]int{
+			0: 0,
+			1: 1,
+		},
+	},
+	{
+		name: "sort outputs on value",
+		tx: &wire.MsgTx{
+			TxOut: []*wire.TxOut{
+				{
+					Value:    2,
+					PkScript: []byte{0x0},
+				},
+				{
+					Value:    1,
+					PkScript: []byte{0x0},
+				},
+			},
+		},
+		cltvs: []uint32{0, 0},
+		outtrans: map[int]int{
+			0: 1,
+			1: 0,
+		},
+	},
+	{
+		name: "sort outputs on pkscript",
+		tx: &wire.MsgTx{
+			TxOut: []*wire.TxOut{
+				{
+					Value:    1,
+					PkScript: []byte{0x2},
+				},
+				{
+					Value:    1,
+					PkScript: []byte{0x1},
+				},
+			},
+		},
+		cltvs: []uint32{0, 0},
+		outtrans: map[int]int{
+			0: 1,
+			1: 0,
+		},
+	},
+	{
+		name: "sort outputs on cltv",
+		tx: &wire.MsgTx{
+			TxOut: []*wire.TxOut{
+				{
+					Value:    1,
+					PkScript: []byte{0x1},
+				},
+				{
+					Value:    1,
+					PkScript: []byte{0x1},
+				},
+			},
+		},
+		cltvs: []uint32{2, 1},
+		outtrans: map[int]int{
+			0: 1,
+			1: 0,
+		},
+	},
+	{
+		name: "sort complex outputs",
+		tx: &wire.MsgTx{
+			TxOut: []*wire.TxOut{
+				{
+					Value:    100000,
+					PkScript: []byte{0x01, 0x02},
+				},
+				{
+					Value:    200000,
+					PkScript: []byte{0x03, 0x02},
+				},
+				{
+					Value:    1000,
+					PkScript: []byte{0x03},
+				},
+				{
+					Value:    1000,
+					PkScript: []byte{0x02},
+				},
+				{
+					Value:    1000,
+					PkScript: []byte{0x1},
+				},
+				{
+					Value:    1000,
+					PkScript: []byte{0x1},
+				},
+			},
+		},
+		cltvs: []uint32{0, 0, 100, 90, 70, 80},
+		outtrans: map[int]int{
+			0: 4,
+			1: 5,
+			2: 3,
+			3: 2,
+			4: 0,
+			5: 1,
+		},
+	},
+	{
+		name: "outputs already sorted",
+		tx: &wire.MsgTx{
+			TxOut: []*wire.TxOut{
+				{
+					Value:    1,
+					PkScript: []byte{0x1},
+				},
+				{
+					Value:    1,
+					PkScript: []byte{0x1},
+				},
+			},
+		},
+		cltvs: []uint32{1, 2},
+		outtrans: map[int]int{
+			0: 0,
+			1: 1,
+		},
+	},
+}
+
+// TestCommitSort asserts that the outputs of a transaction are properly sorted
+// using InPlaceCommitSort. The outputs should always be sorted by value, then
+// lexicographically by pkscript, and then CLTV value.
+func TestCommitSort(t *testing.T) {
+	for _, test := range commitSortTests {
+		t.Run(test.name, func(t *testing.T) {
+			expTxIns := test.expTxIns()
+			expTxOuts := test.expTxOuts()
+
+			lnwallet.InPlaceCommitSort(test.tx, test.cltvs)
+
+			if !reflect.DeepEqual(test.tx.TxIn, expTxIns) {
+				t.Fatalf("commit inputs mismatch, want: %v, "+
+					"got: %v", expTxIns, test.tx.TxIn)
+			}
+
+			if !reflect.DeepEqual(test.tx.TxOut, expTxOuts) {
+				t.Fatalf("commit outputs mismatch, want: %v, "+
+					"got: %v", expTxOuts, test.tx.TxOut)
+			}
+		})
+	}
+}