Merge pull request #9889 from yyforyongyu/fix-htlcindex

Use `BigSizeT` for `HtlcIndex` in retribution log

channeldb/revocation_log.go

@@ -2,6 +2,7 @@ package channeldb
 
 import (
 	"bytes"
+	"encoding/binary"
 	"errors"
 	"io"
 	"math"
@@ -165,7 +166,7 @@ type HTLCEntry struct {
 	CustomBlob tlv.OptionalRecordT[tlv.TlvType5, tlv.Blob]
 
 	// HtlcIndex is the index of the HTLC in the channel.
-	HtlcIndex tlv.OptionalRecordT[tlv.TlvType6, uint16]
+	HtlcIndex tlv.OptionalRecordT[tlv.TlvType6, tlv.BigSizeT[uint64]]
 }
 
 // toTlvStream converts an HTLCEntry record into a tlv representation.
@@ -182,7 +183,9 @@ func (h *HTLCEntry) toTlvStream() (*tlv.Stream, error) {
 		records = append(records, r.Record())
 	})
 
-	h.HtlcIndex.WhenSome(func(r tlv.RecordT[tlv.TlvType6, uint16]) {
+	h.HtlcIndex.WhenSome(func(r tlv.RecordT[tlv.TlvType6,
+		tlv.BigSizeT[uint64]]) {
+
 		records = append(records, r.Record())
 	})
 
@@ -207,8 +210,8 @@ func NewHTLCEntryFromHTLC(htlc HTLC) (*HTLCEntry, error) {
 		Amt: tlv.NewRecordT[tlv.TlvType4](
 			tlv.NewBigSizeT(htlc.Amt.ToSatoshis()),
 		),
-		HtlcIndex: tlv.SomeRecordT(tlv.NewPrimitiveRecord[tlv.TlvType6](
+		HtlcIndex: tlv.SomeRecordT(tlv.NewRecordT[tlv.TlvType6](
-			uint16(htlc.HtlcIndex),
+			tlv.NewBigSizeT(htlc.HtlcIndex),
 		)),
 	}
 
@@ -510,13 +513,22 @@ func deserializeRevocationLog(r io.Reader) (RevocationLog, error) {
 // deserializeHTLCEntries deserializes a list of HTLC entries based on tlv
 // format.
 func deserializeHTLCEntries(r io.Reader) ([]*HTLCEntry, error) {
-	var htlcs []*HTLCEntry
+	var (
+		htlcs []*HTLCEntry
+
+		// htlcIndexBlob defines the tlv record type to be used when
+		// decoding from the disk. We use it instead of the one defined
+		// in `HTLCEntry.HtlcIndex` as previously this field was encoded
+		// using `uint16`, thus we will read it as raw bytes and
+		// deserialize it further below.
+		htlcIndexBlob tlv.OptionalRecordT[tlv.TlvType6, tlv.Blob]
+	)
 
 	for {
 		var htlc HTLCEntry
 
 		customBlob := htlc.CustomBlob.Zero()
-		htlcIndex := htlc.HtlcIndex.Zero()
+		htlcIndex := htlcIndexBlob.Zero()
 
 		// Create the tlv stream.
 		records := []tlv.Record{
@@ -549,7 +561,14 @@ func deserializeHTLCEntries(r io.Reader) ([]*HTLCEntry, error) {
 		}
 
 		if t, ok := parsedTypes[htlcIndex.TlvType()]; ok && t == nil {
-			htlc.HtlcIndex = tlv.SomeRecordT(htlcIndex)
+			record, err := deserializeHtlcIndexCompatible(
+				htlcIndex.Val,
+			)
+			if err != nil {
+				return nil, err
+			}
+
+			htlc.HtlcIndex = record
 		}
 
 		// Append the entry.
@@ -559,6 +578,55 @@ func deserializeHTLCEntries(r io.Reader) ([]*HTLCEntry, error) {
 	return htlcs, nil
 }
 
+// deserializeHtlcIndexCompatible takes raw bytes and decodes it into an
+// optional record that's assigned to the entry's HtlcIndex.
+//
+// NOTE: previously this `HtlcIndex` was a tlv record that used `uint16` to
+// encode its value. Given now its value is encoded using BigSizeT, and for any
+// BigSizeT, its possible length values are 1, 3, 5, and 8. This means if the
+// tlv record has a length of 2, we know for sure it must be an old record
+// whose value was encoded using uint16.
+func deserializeHtlcIndexCompatible(rawBytes []byte) (
+	tlv.OptionalRecordT[tlv.TlvType6, tlv.BigSizeT[uint64]], error) {
+
+	var (
+		// record defines the record that's used by the HtlcIndex in the
+		// entry.
+		record tlv.OptionalRecordT[
+			tlv.TlvType6, tlv.BigSizeT[uint64],
+		]
+
+		// htlcIndexVal is the decoded uint64 value.
+		htlcIndexVal uint64
+	)
+
+	// If the length of the tlv record is 2, it must be encoded using uint16
+	// as the BigSizeT encoding cannot have this length.
+	if len(rawBytes) == 2 {
+		// Decode the raw bytes into uint16 and convert it into uint64.
+		htlcIndexVal = uint64(binary.BigEndian.Uint16(rawBytes))
+	} else {
+		// This value is encoded using BigSizeT, we now use the decoder
+		// to deserialize the raw bytes.
+		r := bytes.NewBuffer(rawBytes)
+
+		// Create a buffer to be used in the decoding process.
+		buf := [8]byte{}
+
+		// Use the BigSizeT's decoder.
+		err := tlv.DBigSize(r, &htlcIndexVal, &buf, 8)
+		if err != nil {
+			return record, err
+		}
+	}
+
+	record = tlv.SomeRecordT(tlv.NewRecordT[tlv.TlvType6](
+		tlv.NewBigSizeT(htlcIndexVal),
+	))
+
+	return record, nil
+}
+
 // writeTlvStream is a helper function that encodes the tlv stream into the
 // writer.
 func writeTlvStream(w io.Writer, s *tlv.Stream) error {

channeldb/revocation_log_test.go

@@ -2,6 +2,7 @@ package channeldb
 
 import (
 	"bytes"
+	"encoding/binary"
 	"io"
 	"math"
 	"math/rand"
@@ -59,13 +60,13 @@ var (
 		CustomBlob: tlv.SomeRecordT(
 			tlv.NewPrimitiveRecord[tlv.TlvType5](blobBytes),
 		),
-		HtlcIndex: tlv.SomeRecordT(
+		HtlcIndex: tlv.SomeRecordT(tlv.NewRecordT[tlv.TlvType6](
-			tlv.NewPrimitiveRecord[tlv.TlvType6, uint16](0x33),
+			tlv.NewBigSizeT(uint64(0x33)),
-		),
+		)),
 	}
 	testHTLCEntryBytes = []byte{
-		// Body length 45.
+		// Body length 44.
-		0x2d,
+		0x2c,
 		// Rhash tlv.
 		0x0, 0x0,
 		// RefundTimeout tlv.
@@ -79,8 +80,8 @@ var (
 		// Custom blob tlv.
 		0x5, 0x11, 0xfe, 0x00, 0x01, 0x00, 0x01, 0x0b, 0x63, 0x75, 0x73,
 		0x74, 0x6f, 0x6d, 0x20, 0x64, 0x61, 0x74, 0x61,
-		// HLTC index tlv.
+		// HTLC index tlv.
-		0x6, 0x2, 0x0, 0x33,
+		0x6, 0x1, 0x33,
 	}
 
 	testHTLCEntryHash = HTLCEntry{
@@ -133,7 +134,7 @@ var (
 			OutputIndex:   int32(testHTLCEntry.OutputIndex.Val),
 			HtlcIndex: uint64(
 				testHTLCEntry.HtlcIndex.ValOpt().
-					UnsafeFromSome(),
+					UnsafeFromSome().Int(),
 			),
 			Incoming: testHTLCEntry.Incoming.Val,
 			Amt: lnwire.NewMSatFromSatoshis(
@@ -303,7 +304,7 @@ func TestSerializeHTLCEntries(t *testing.T) {
 	partialBytes := testHTLCEntryBytes[3:]
 
 	// Write the total length and RHash tlv.
-	expectedBytes := []byte{0x4d, 0x0, 0x20}
+	expectedBytes := []byte{0x4c, 0x0, 0x20}
 	expectedBytes = append(expectedBytes, rHashBytes...)
 
 	// Append the rest.
@@ -785,3 +786,63 @@ func createTestRevocationLogBuckets(tx kvdb.RwTx) (kvdb.RwBucket,
 
 	return chanBucket, logBucket, nil
 }
+
+// TestDeserializeHTLCEntriesLegacy checks that the legacy encoding of the
+// HtlcIndex can be correctly read as BigSizeT. The field `HtlcIndex` was
+// encoded using `uint16` and should now be deserialized into BigSizeT
+// on-the-fly.
+func TestDeserializeHTLCEntriesLegacy(t *testing.T) {
+	t.Parallel()
+
+	// rawBytes defines the bytes read from the disk for the testing HTLC
+	// entry.
+	rawBytes := []byte{
+		// Body length 45.
+		0x2d,
+		// Rhash tlv.
+		0x0, 0x0,
+		// RefundTimeout tlv.
+		0x1, 0x4, 0x0, 0xb, 0x4a, 0xa0,
+		// OutputIndex tlv.
+		0x2, 0x2, 0x0, 0xa,
+		// Incoming tlv.
+		0x3, 0x1, 0x1,
+		// Amt tlv.
+		0x4, 0x5, 0xfe, 0x0, 0xf, 0x42, 0x40,
+		// Custom blob tlv.
+		0x5, 0x11, 0xfe, 0x00, 0x01, 0x00, 0x01, 0x0b, 0x63, 0x75, 0x73,
+		0x74, 0x6f, 0x6d, 0x20, 0x64, 0x61, 0x74, 0x61,
+
+		// HTLC index tlv.
+		//
+		// NOTE: We are missing two bytes in the end, which is appended
+		// below.
+		0x6, 0x2,
+	}
+
+	// Iterate through all possible values encoded using uint16. They should
+	// be correctly read as uint16 and converted to BigSizeT.
+	for i := range math.MaxUint16 + 1 {
+		// Encode the index using two bytes.
+		rawHtlcIndex := make([]byte, 2)
+		binary.BigEndian.PutUint16(rawHtlcIndex, uint16(i))
+
+		// Copy the raw bytes and append the htlc index.
+		rawEntry := bytes.Clone(rawBytes)
+		rawEntry = append(rawEntry, rawHtlcIndex...)
+
+		// Read the tlv stream.
+		buf := bytes.NewBuffer(rawEntry)
+		htlcs, err := deserializeHTLCEntries(buf)
+		require.NoError(t, err)
+
+		// Check the bytes are read as expected.
+		require.Len(t, htlcs, 1)
+
+		// Assert that the uint16 is converted to BigSizeT.
+		record := tlv.SomeRecordT(tlv.NewRecordT[tlv.TlvType6](
+			tlv.NewBigSizeT(uint64(i)),
+		))
+		require.Equal(t, record, htlcs[0].HtlcIndex)
+	}
+}

docs/release-notes/release-notes-0.19.2.md

@@ -0,0 +1,64 @@
+# Release Notes
+- [Bug Fixes](#bug-fixes)
+- [New Features](#new-features)
+    - [Functional Enhancements](#functional-enhancements)
+    - [RPC Additions](#rpc-additions)
+    - [lncli Additions](#lncli-additions)
+- [Improvements](#improvements)
+    - [Functional Updates](#functional-updates)
+    - [RPC Updates](#rpc-updates)
+    - [lncli Updates](#lncli-updates)
+    - [Breaking Changes](#breaking-changes)
+    - [Performance Improvements](#performance-improvements)
+    - [Deprecations](#deprecations)
+- [Technical and Architectural Updates](#technical-and-architectural-updates)
+    - [BOLT Spec Updates](#bolt-spec-updates)
+    - [Testing](#testing)
+    - [Database](#database)
+    - [Code Health](#code-health)
+    - [Tooling and Documentation](#tooling-and-documentation)
+
+# Bug Fixes
+
+- [Use](https://github.com/lightningnetwork/lnd/pull/9889) `BigSizeT` instead of
+  `uint16` for the htlc index that's used in the revocation log.
+
+# New Features
+
+## Functional Enhancements
+
+## RPC Additions
+
+## lncli Additions
+
+# Improvements
+
+## Functional Updates
+
+## RPC Updates
+
+## lncli Updates
+
+## Code Health
+
+## Breaking Changes
+
+## Performance Improvements
+
+## Deprecations
+
+# Technical and Architectural Updates
+
+## BOLT Spec Updates
+
+## Testing
+
+## Database
+
+## Code Health
+
+## Tooling and Documentation
+
+# Contributors (Alphabetical Order)
+
+* Yong Yu

lnwallet/channel.go

@@ -2340,18 +2340,21 @@ func createHtlcRetribution(chanState *channeldb.OpenChannel,
 	// We'll generate the original second level witness script now, as
 	// we'll need it if we're revoking an HTLC output on the remote
 	// commitment transaction, and *they* go to the second level.
+	//nolint:ll
 	secondLevelAuxLeaf := fn.FlatMapOption(
 		func(l CommitAuxLeaves) fn.Option[input.AuxTapLeaf] {
-			return fn.MapOption(func(val uint16) input.AuxTapLeaf {
+			return fn.MapOption(
-				idx := input.HtlcIndex(val)
+				func(val tlv.BigSizeT[uint64]) input.AuxTapLeaf {
+					idx := val.Int()
 
-				if htlc.Incoming.Val {
+					if htlc.Incoming.Val {
-					leaves := l.IncomingHtlcLeaves[idx]
+						leaves := l.IncomingHtlcLeaves[idx]
-					return leaves.SecondLevelLeaf
+						return leaves.SecondLevelLeaf
-				}
+					}
 
-				return l.OutgoingHtlcLeaves[idx].SecondLevelLeaf
+					return l.OutgoingHtlcLeaves[idx].SecondLevelLeaf
-			})(htlc.HtlcIndex.ValOpt())
+				},
+			)(htlc.HtlcIndex.ValOpt())
 		},
 	)(auxLeaves)
 	secondLevelScript, err := SecondLevelHtlcScript(
@@ -2365,21 +2368,24 @@ func createHtlcRetribution(chanState *channeldb.OpenChannel,
 
 	// If this is an incoming HTLC, then this means that they were the
 	// sender of the HTLC (relative to us). So we'll re-generate the sender
-	// HTLC script. Otherwise, is this was an outgoing HTLC that we sent,
+	// HTLC script. Otherwise, if this was an outgoing HTLC that we sent,
 	// then from the PoV of the remote commitment state, they're the
 	// receiver of this HTLC.
+	//nolint:ll
 	htlcLeaf := fn.FlatMapOption(
 		func(l CommitAuxLeaves) fn.Option[input.AuxTapLeaf] {
-			return fn.MapOption(func(val uint16) input.AuxTapLeaf {
+			return fn.MapOption(
-				idx := input.HtlcIndex(val)
+				func(val tlv.BigSizeT[uint64]) input.AuxTapLeaf {
+					idx := val.Int()
 
-				if htlc.Incoming.Val {
+					if htlc.Incoming.Val {
-					leaves := l.IncomingHtlcLeaves[idx]
+						leaves := l.IncomingHtlcLeaves[idx]
-					return leaves.AuxTapLeaf
+						return leaves.AuxTapLeaf
-				}
+					}
 
-				return l.OutgoingHtlcLeaves[idx].AuxTapLeaf
+					return l.OutgoingHtlcLeaves[idx].AuxTapLeaf
-			})(htlc.HtlcIndex.ValOpt())
+				},
+			)(htlc.HtlcIndex.ValOpt())
 		},
 	)(auxLeaves)
 	scriptInfo, err := genHtlcScript(