Merge pull request #11223 from gijswijs/legacy-dust-retribution-fix

### contractcourt/breach_arbitrator.go
@@ -1253,10 +1253,10 @@ func newRetributionInfo(chanPoint *wire.OutPoint,
 
 	// Initialize a slice to hold the outputs we will attempt to sweep. The
 	// maximum capacity of the slice is set to 2+nHtlcs to handle the case
-	// where the local, remote, and all HTLCs are not dust outputs.  All
-	// HTLC outputs provided by the wallet are guaranteed to be non-dust,
-	// though the commitment outputs are conditionally added depending on
-	// the nil-ness of their sign descriptors.
+	// where the local, remote, and all HTLCs are not dust outputs. HTLC
+	// outputs provided by the wallet are expected to be non-dust, though
+	// the commitment outputs are conditionally added depending on the
+	// nil-ness of their sign descriptors.
 	breachedOutputs := make([]breachedOutput, 0, nHtlcs+2)
 
 	isTaproot := func() bool {
@@ -1352,9 +1352,18 @@ func newRetributionInfo(chanPoint *wire.OutPoint,
 
 	// Lastly, for each of the breached HTLC outputs, record each as a
 	// breached output with the appropriate witness type based on its
-	// directionality. All HTLC outputs provided by the wallet are assumed
-	// to be non-dust.
+	// directionality.
 	for i, breachedHtlc := range breachInfo.HtlcRetributions {
+		// Defensively skip blank entries. A nil sign descriptor
+		// output is an invariant violation, so we log it loudly
+		// instead of silently continuing.
+		if breachedHtlc.SignDesc.Output == nil {
+			brarLog.Warnf("Skipping blank HTLC retribution for "+
+				"ChannelPoint(%v), index=%d", chanPoint, i)
+
+			continue
+		}
+
 		// Using the breachedHtlc's incoming flag, determine the
 		// appropriate witness type that needs to be generated in order
 		// to sweep the HTLC output.

### contractcourt/breach_arbitrator_test.go
@@ -2520,6 +2520,39 @@ func TestNewRetributionInfoTaprootFinalWitnessTypes(t *testing.T) {
 	)
 }
 
+// TestNewRetributionInfoSkipsBlankHtlc verifies that a blank HTLC
+// retribution is skipped, since its nil sign descriptor output cannot be
+// used, and that the surviving outputs are the right ones.
+func TestNewRetributionInfoSkipsBlankHtlc(t *testing.T) {
+	t.Parallel()
+
+	// Arrange: Create a breach carrying one blank and one populated HTLC
+	// retribution.
+	signDesc := testTaprootBreachSignDesc(t)
+	breachInfo := &lnwallet.BreachRetribution{
+		LocalOutpoint:       wire.OutPoint{Index: 1},
+		LocalOutputSignDesc: signDesc,
+		ChanType:            channeldb.SimpleTaprootFeatureBit,
+		HtlcRetributions: []lnwallet.HtlcRetribution{
+			{},
+			{
+				SignDesc: *signDesc,
+				OutPoint: wire.OutPoint{Index: 3},
+			},
+		},
+	}
+
+	// Act: Convert the wallet retribution into the breach-arbitrator form.
+	// The blank entry must be skipped rather than dereferenced here.
+	retInfo := newRetributionInfo(&wire.OutPoint{}, breachInfo)
+
+	// Assert: Only the local output and the populated HTLC are swept, so
+	// the blank entry was skipped and the populated one survived.
+	require.Len(t, retInfo.breachedOutputs, 2)
+	require.EqualValues(t, 1, retInfo.breachedOutputs[0].outpoint.Index)
+	require.EqualValues(t, 3, retInfo.breachedOutputs[1].outpoint.Index)
+}
+
 // TestTaprootBriefcaseRoundTripFinalWitnessTypes verifies that final taproot
 // breach outputs survive taproot briefcase encoding and decoding with their
 // control blocks and auxiliary blobs intact.

### docs/release-notes/release-notes-0.20.5.md
@@ -51,6 +51,13 @@
   [lightning/bolts#1357](https://github.com/lightning/bolts/pull/1357), and
   is an interop consideration for any wallet emitting such invoices.
 
+* Breach retributions built from [legacy revocation log entries now skip
+  dust HTLCs without leaving blank entries
+  behind](https://github.com/lightningnetwork/lnd/pull/11223). HTLCs marked
+  as trimmed via their stored output index are also skipped, matching the
+  modern revocation log format, and the breach arbiter now skips and logs
+  any HTLC retribution with a nil sign descriptor output.
+
 # New Features
 
 ## Functional Enhancements

### docs/release-notes/release-notes-0.21.4.md
@@ -56,6 +56,13 @@
   [lightning/bolts#1357](https://github.com/lightning/bolts/pull/1357), and
   is an interop consideration for any wallet emitting such invoices.
 
+* Breach retributions built from [legacy revocation log entries now skip
+  dust HTLCs without leaving blank entries
+  behind](https://github.com/lightningnetwork/lnd/pull/11223). HTLCs marked
+  as trimmed via their stored output index are also skipped, matching the
+  modern revocation log format, and the breach arbiter now skips and logs
+  any HTLC retribution with a nil sign descriptor output.
+
 # New Features
 
 ## Functional Enhancements

### lnwallet/channel.go
@@ -2666,17 +2666,36 @@ func createBreachRetributionLegacy(revokedLog *channeldb.ChannelCommitment,
 
 	// With the commitment outputs located, we'll now generate all the
 	// retribution structs for each of the HTLC transactions active on the
-	// remote commitment transaction.
-	htlcRetributions := make([]HtlcRetribution, len(revokedLog.Htlcs))
-	for i, htlc := range revokedLog.Htlcs {
-		// If the HTLC is dust, then we'll skip it as it doesn't have
-		// an output on the commitment transaction.
-		if HtlcIsDust(
+	// remote commitment transaction. We densely pack the slice so that
+	// skipped dust HTLCs don't leave blank entries behind, as a zero-value
+	// HtlcRetribution carries a nil sign descriptor output that downstream
+	// consumers don't expect.
+	htlcRetributions := make([]HtlcRetribution, 0, len(revokedLog.Htlcs))
+	for _, htlc := range revokedLog.Htlcs {
+		isDust := HtlcIsDust(
 			chanState.ChanType, htlc.Incoming, lntypes.Remote,
 			chainfee.SatPerKWeight(revokedLog.FeePerKw),
 			htlc.Amt.ToSatoshis(),
 			chanState.RemoteChanCfg.DustLimit,
-		) {
+		)
+
+		// If the HTLC is dust, then we'll skip it as it doesn't have
+		// an output on the commitment transaction. As a hardening
+		// measure, we also skip HTLCs whose stored output index marks
+		// them as trimmed, which is how the modern revocation log
+		// format decides dust at write time.
+		if htlc.OutputIndex < 0 || isDust {
+			// If the stored output index and the dust check
+			// disagree, the log entry is suspect, so we log it
+			// loudly to be able to pinpoint the error.
+			if htlc.OutputIndex < 0 && !isDust {
+				walletLog.Warnf("Skipping HTLC(index=%v) "+
+					"in legacy revocation log for "+
+					"ChannelPoint(%v): stored output "+
+					"index negative, but not dust",
+					htlc.HtlcIndex,
+					chanState.FundingOutpoint)
+			}
 
 			continue
 		}
@@ -2694,7 +2713,7 @@ func createBreachRetributionLegacy(revokedLog *channeldb.ChannelCommitment,
 		if err != nil {
 			return nil, 0, 0, err
 		}
-		htlcRetributions[i] = hr
+		htlcRetributions = append(htlcRetributions, hr)
 	}
 
 	// Compute the balances in satoshis.

### lnwallet/channel_test.go
@@ -10401,6 +10401,85 @@ func TestCreateBreachRetributionLegacy(t *testing.T) {
 	require.Equal(t, theirOp.Value, theirAmt)
 }
 
+// TestCreateBreachRetributionLegacyDustHtlcs tests that
+// `createBreachRetributionLegacy` skips dust HTLCs without leaving blank
+// entries in the returned retributions. A blank entry carries a nil sign
+// descriptor output, which downstream consumers don't expect.
+func TestCreateBreachRetributionLegacyDustHtlcs(t *testing.T) {
+	t.Parallel()
+
+	// Create dummy values for the test.
+	dummyPrivate, _ := btcec.PrivKeyFromBytes([]byte{1})
+
+	// Create a test channel.
+	aliceChannel, _, err := CreateTestChannels(
+		t, channeldb.ZeroHtlcTxFeeBit,
+	)
+	require.NoError(t, err)
+
+	chanState := aliceChannel.channelState
+
+	// Prepare the params needed to call the function. Note that the values
+	// here are not necessary "cryptography-correct", we just use them to
+	// construct the retribution.
+	leaseExpiry, keyRing, _ := deriveDummyRetributionParams(chanState)
+
+	// Use the remote commitment as our revocation log.
+	revokedLog := chanState.RemoteCommitment
+
+	ourOp := revokedLog.CommitTx.TxOut[0]
+	theirOp := revokedLog.CommitTx.TxOut[1]
+
+	// Create the dummy scripts.
+	ourScript := &WitnessScriptDesc{
+		OutputScript: ourOp.PkScript,
+	}
+	theirScript := &WitnessScriptDesc{
+		OutputScript: theirOp.PkScript,
+	}
+
+	// Add three HTLCs to the revocation log: one dust HTLC marked as
+	// trimmed via its stored output index, one dust HTLC by amount with a
+	// non-negative output index, and one non-dust HTLC. The channel type
+	// has zero HTLC transaction fees, so an HTLC is dust iff its amount is
+	// below the remote party's dust limit.
+	dustLimit := chanState.RemoteChanCfg.DustLimit
+	nonDustAmt := dustLimit + 1000
+	revokedLog.Htlcs = []channeldb.HTLC{
+		{
+			Incoming:    true,
+			Amt:         lnwire.NewMSatFromSatoshis(dustLimit - 1),
+			OutputIndex: -1,
+		},
+		{
+			Incoming:    true,
+			Amt:         lnwire.NewMSatFromSatoshis(dustLimit - 1),
+			OutputIndex: 3,
+		},
+		{
+			Incoming:    true,
+			Amt:         lnwire.NewMSatFromSatoshis(nonDustAmt),
+			OutputIndex: 2,
+		},
+	}
+
+	// Create the breach retribution using the legacy format.
+	br, _, _, err := createBreachRetributionLegacy(
+		&revokedLog, chanState, keyRing, dummyPrivate, ourScript,
+		theirScript, leaseExpiry,
+	)
+	require.NoError(t, err)
+
+	// Both dust HTLCs must be skipped entirely, leaving a single, fully
+	// populated retribution behind.
+	require.Len(t, br.HtlcRetributions, 1)
+
+	hr := br.HtlcRetributions[0]
+	require.NotNil(t, hr.SignDesc.Output)
+	require.EqualValues(t, nonDustAmt, hr.SignDesc.Output.Value)
+	require.EqualValues(t, 2, hr.OutPoint.Index)
+}
+
 // TestNewBreachRetribution tests that the function `NewBreachRetribution`
 // behaves as expected.
 func TestNewBreachRetribution(t *testing.T) {