contractcourt: fix HTLC success resolver phantom-input bug (#10840)

contractcourt/htlc_incoming_contest_resolver.go

@@ -90,9 +90,13 @@ func (h *htlcIncomingContestResolver) Launch() error {
 	}
 
 	h.log.Debugf("launching contest resolver...")
+	_, bestHeight, err := h.ChainIO.GetBestBlock()
+	if err != nil {
+		return err
+	}
 
 	// Query the preimage and apply it if we already know it.
-	applied, err := h.findAndapplyPreimage()
+	applied, err := h.findAndapplyPreimage(bestHeight)
 	if err != nil {
 		return err
 	}
@@ -615,7 +619,8 @@ var _ htlcContractResolver = (*htlcIncomingContestResolver)(nil)
 //
 // NOTE: Since we have two places to query the preimage, we need to check both
 // the preimage db and the invoice db to look up the preimage.
-func (h *htlcIncomingContestResolver) findAndapplyPreimage() (bool, error) {
+func (h *htlcIncomingContestResolver) findAndapplyPreimage(
+	currentHeight int32) (bool, error) {
 	// Query to see if we already know the preimage.
 	preimage, ok := h.PreimageDB.LookupPreimage(h.htlc.RHash)
 
@@ -658,13 +663,13 @@ func (h *htlcIncomingContestResolver) findAndapplyPreimage() (bool, error) {
 	// immediately, we'll assume we don't know it yet and let the `Resolve`
 	// handle the waiting.
 	//
-	// NOTE: we use a nil subscriber here and a zero current height as we
-	// are only interested in the settle resolution.
+	// NOTE: we use a nil subscriber here as we are only interested in the
+	// settle resolution.
 	//
 	// TODO(yy): move this logic to link and let the preimage be accessed
 	// via the preimage beacon.
 	resolution, err := h.Registry.NotifyExitHopHtlc(
-		h.htlc.RHash, h.htlc.Amt, h.htlcExpiry, 0,
+		h.htlc.RHash, h.htlc.Amt, h.htlcExpiry, currentHeight,
 		circuitKey, nil, h.htlc.CustomRecords, payload,
 	)
 	if err != nil {

contractcourt/htlc_incoming_contest_resolver_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"io"
 	"testing"
+	"time"
 
 	"github.com/btcsuite/btcd/wire"
 	sphinx "github.com/lightningnetwork/lightning-onion"
@@ -116,6 +117,119 @@ func TestHtlcIncomingResolverFwdTimeout(t *testing.T) {
 	ctx.waitForResult(false)
 }
 
+// TestHtlcIncomingResolverLaunchUsesKnownPreimageAfterExpiry asserts that
+// Launch still offers the HTLC to the sweeper when the preimage was already
+// learned before restart, even if the HTLC is now expired.
+func TestHtlcIncomingResolverLaunchUsesKnownPreimageAfterExpiry(t *testing.T) {
+	t.Parallel()
+	defer timeout()()
+
+	tests := []struct {
+		name   string
+		isExit bool
+		setup  func(*incomingResolverTestContext)
+		assert func(*testing.T, *incomingResolverTestContext)
+	}{
+		{
+			name:   "preimage db",
+			isExit: false,
+			setup: func(ctx *incomingResolverTestContext) {
+				ctx.witnessBeacon.lookupPreimage[testResHash] =
+					testResPreimage
+			},
+			assert: func(t *testing.T,
+				ctx *incomingResolverTestContext) {
+
+				require.Empty(t, ctx.registry.immediateNotify)
+			},
+		},
+		{
+			name:   "invoice registry replay",
+			isExit: true,
+			setup: func(ctx *incomingResolverTestContext) {
+				ctx.registry.notifyResolution =
+					invoices.NewSettleResolution(
+						testResPreimage,
+						testResCircuitKey,
+						testAcceptHeight,
+						invoices.ResultReplayToSettled,
+					)
+			},
+			assert: func(t *testing.T,
+				ctx *incomingResolverTestContext) {
+
+				require.Len(t, ctx.registry.immediateNotify, 1)
+				notify := ctx.registry.immediateNotify[0]
+				require.Equal(
+					t, ctx.chainIO.BestHeight,
+					notify.currentHeight,
+				)
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			// Arrange: restart at the HTLC expiry with the preimage
+			// already available from this test's source.
+			ctx := newIncomingResolverTestContext(t, test.isExit)
+			ctx.resolver.htlcExpiry = testInitialBlockHeight
+			test.setup(ctx)
+
+			// Act: launch without entering Resolve's block epoch
+			// loop.
+			require.NoError(t, ctx.resolver.Launch())
+
+			// Assert: the known preimage still starts the direct
+			// sweep path after expiry.
+			require.True(t, ctx.resolver.isLaunched())
+			preimage := lntypes.Preimage(
+				ctx.resolver.htlcResolution.Preimage,
+			)
+			require.Equal(
+				t, testResPreimage, preimage,
+			)
+
+			sweeper, ok := ctx.resolver.Sweeper.(*mockSweeper)
+			require.True(t, ok)
+			select {
+			case <-sweeper.sweptInputs:
+			case <-time.After(time.Second):
+				t.Fatal("expected known preimage sweep")
+			}
+
+			test.assert(t, ctx)
+		})
+	}
+}
+
+// TestHtlcIncomingResolverLaunchUsesCurrentHeight asserts that Launch uses the
+// current chain height for its immediate invoice-registry lookup instead of the
+// historical zero height placeholder.
+func TestHtlcIncomingResolverLaunchUsesCurrentHeight(t *testing.T) {
+	t.Parallel()
+	defer timeout()()
+
+	// Arrange: set up an exit-hop resolver with a registry resolution that
+	// Launch can consume immediately.
+	ctx := newIncomingResolverTestContext(t, true)
+	ctx.chainIO.BestHeight = testInitialBlockHeight + 1
+	ctx.registry.notifyResolution = invoices.NewSettleResolution(
+		testResPreimage, testResCircuitKey, testAcceptHeight,
+		invoices.ResultReplayToSettled,
+	)
+
+	// Act: launch performs the immediate invoice-registry lookup.
+	require.NoError(t, ctx.resolver.Launch())
+
+	// Assert: the registry saw the chain height returned by ChainIO.
+	require.Len(t, ctx.registry.immediateNotify, 1)
+	require.Equal(
+		t, ctx.chainIO.BestHeight,
+		ctx.registry.immediateNotify[0].currentHeight,
+	)
+}
+
 // TestHtlcIncomingResolverExitSettle tests resolution of an exit hop htlc for
 // which the invoice has already been settled when the resolver starts.
 func TestHtlcIncomingResolverExitSettle(t *testing.T) {
@@ -306,6 +420,7 @@ type incomingResolverTestContext struct {
 	witnessBeacon          *mockWitnessBeacon
 	resolver               *htlcIncomingContestResolver
 	notifier               *mock.ChainNotifier
+	chainIO                *mock.ChainIO
 	onionProcessor         *mockOnionProcessor
 	resolveErr             chan error
 	nextResolver           ContractResolver
@@ -320,6 +435,9 @@ func newIncomingResolverTestContext(t *testing.T, isExit bool) *incomingResolver
 		ConfChan:  make(chan *chainntnfs.TxConfirmation),
 	}
 	witnessBeacon := newMockWitnessBeacon()
+	chainIO := &mock.ChainIO{
+		BestHeight: testInitialBlockHeight,
+	}
 	registry := &mockRegistry{
 		notifyChan: make(chan notifyExitHopData, 1),
 	}
@@ -332,6 +450,7 @@ func newIncomingResolverTestContext(t *testing.T, isExit bool) *incomingResolver
 		registry:       registry,
 		witnessBeacon:  witnessBeacon,
 		notifier:       notifier,
+		chainIO:        chainIO,
 		onionProcessor: onionProcessor,
 		t:              t,
 	}
@@ -359,6 +478,7 @@ func newIncomingResolverTestContext(t *testing.T, isExit bool) *incomingResolver
 				return nil
 			},
 			Sweeper: newMockSweeper(),
+			ChainIO: chainIO,
 		},
 		PutResolverReport: func(_ kvdb.RwTx,
 			_ *channeldb.ResolverReport) error {

contractcourt/htlc_success_resolver.go

@@ -1,6 +1,7 @@
 package contractcourt
 
 import (
+	"bytes"
 	"encoding/binary"
 	"fmt"
 	"io"
@@ -10,6 +11,7 @@ import (
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
+	"github.com/lightningnetwork/lnd/chainntnfs"
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/fn/v2"
 	"github.com/lightningnetwork/lnd/graph/db/models"
@@ -232,6 +234,55 @@ func (h *htlcSuccessResolver) checkpointClaim(spendTx *chainhash.Hash) error {
 	return h.Checkpoint(h, reports...)
 }
 
+// checkpointForeignSpend checkpoints the resolver as failed when the original
+// HTLC outpoint was spent by a transaction that did not create our expected
+// second-level success output.
+func (h *htlcSuccessResolver) checkpointForeignSpend(
+	commitSpend *chainntnfs.SpendDetail) error {
+
+	err := h.ChainArbitratorConfig.PutFinalHtlcOutcome(
+		h.ChannelArbitratorConfig.ShortChanID, h.htlc.HtlcIndex, false,
+	)
+	if err != nil {
+		return err
+	}
+
+	h.ChainArbitratorConfig.HtlcNotifier.NotifyFinalHtlcEvent(
+		models.CircuitKey{
+			ChanID: h.ShortChanID,
+			HtlcID: h.htlc.HtlcIndex,
+		},
+		channeldb.FinalHtlcInfo{
+			Settled:  false,
+			Offchain: false,
+		},
+	)
+
+	var spendTxID *chainhash.Hash
+	if commitSpend != nil {
+		spendTxID = commitSpend.SpenderTxHash
+	}
+	h.log.Warnf("HTLC outpoint %v was spent by tx %v, which did not "+
+		"create our expected second-level success output", h.outpoint(),
+		spendTxID)
+
+	// A foreign spend of an incoming HTLC on our commitment means we cannot
+	// complete the success path. The known production case is the remote
+	// timeout reclaim.
+	report := &channeldb.ResolverReport{
+		OutPoint:        h.outpoint(),
+		Amount:          h.htlc.Amt.ToSatoshis(),
+		ResolverType:    channeldb.ResolverTypeIncomingHtlc,
+		ResolverOutcome: channeldb.ResolverOutcomeTimeout,
+		SpendTxID:       spendTxID,
+	}
+
+	h.outputIncubating = false
+	h.markResolved()
+
+	return h.Checkpoint(h, report)
+}
+
 // Stop signals the resolver to cancel any current resolution processes, and
 // suspend.
 //
@@ -431,6 +482,36 @@ func (h *htlcSuccessResolver) isTaprootFinal() bool {
 	return h.chanType.IsTaprootFinal()
 }
 
+// successTxOutpoint returns the second-level success output outpoint if the
+// spending transaction created the output that this resolver expects to sweep.
+func (h *htlcSuccessResolver) successTxOutpoint(
+	commitSpend *chainntnfs.SpendDetail) (wire.OutPoint, bool) {
+
+	if commitSpend == nil || commitSpend.SpendingTx == nil ||
+		commitSpend.SpenderTxHash == nil {
+
+		return wire.OutPoint{}, false
+	}
+
+	outputIndex := commitSpend.SpenderInputIndex
+	if outputIndex >= uint32(len(commitSpend.SpendingTx.TxOut)) {
+		return wire.OutPoint{}, false
+	}
+
+	expected := h.htlcResolution.SweepSignDesc.Output
+	actual := commitSpend.SpendingTx.TxOut[outputIndex]
+	if expected == nil || actual.Value != expected.Value ||
+		!bytes.Equal(actual.PkScript, expected.PkScript) {
+
+		return wire.OutPoint{}, false
+	}
+
+	return wire.OutPoint{
+		Hash:  *commitSpend.SpenderTxHash,
+		Index: outputIndex,
+	}, true
+}
+
 // sweepRemoteCommitOutput creates a sweep request to sweep the HTLC output on
 // the remote commitment via the direct preimage-spend.
 func (h *htlcSuccessResolver) sweepRemoteCommitOutput() error {
@@ -560,6 +641,10 @@ func (h *htlcSuccessResolver) sweepSuccessTxOutput() error {
 	if err != nil {
 		return err
 	}
+	op, ok := h.successTxOutpoint(commitSpend)
+	if !ok {
+		return h.checkpointForeignSpend(commitSpend)
+	}
 
 	// The HTLC success tx has a CSV lock that we must wait for, and if
 	// this is a lease enforced channel and we're the imitator, we may need
@@ -583,16 +668,6 @@ func (h *htlcSuccessResolver) sweepSuccessTxOutput() error {
 			h, h.htlc.RHash[:], waitHeight)
 	}
 
-	// We'll use this input index to determine the second-level output
-	// index on the transaction, as the signatures requires the indexes to
-	// be the same. We don't look for the second-level output script
-	// directly, as there might be more than one HTLC output to the same
-	// pkScript.
-	op := &wire.OutPoint{
-		Hash:  *commitSpend.SpenderTxHash,
-		Index: commitSpend.SpenderInputIndex,
-	}
-
 	// Let the sweeper sweep the second-level output now that the
 	// CSV/CLTV locks have expired.
 	var witType input.StandardWitnessType
@@ -605,7 +680,7 @@ func (h *htlcSuccessResolver) sweepSuccessTxOutput() error {
 		witType = input.HtlcAcceptedSuccessSecondLevel
 	}
 	inp := h.makeSweepInput(
-		op, witType,
+		&op, witType,
 		input.LeaseHtlcAcceptedSuccessSecondLevel,
 		&h.htlcResolution.SweepSignDesc,
 		h.htlcResolution.CsvDelay, uint32(commitSpend.SpendingHeight),
@@ -703,15 +778,9 @@ func (h *htlcSuccessResolver) resolveSuccessTx() error {
 	if err != nil {
 		return err
 	}
-
-	// We'll use this input index to determine the second-level output
-	// index on the transaction, as the signatures requires the indexes to
-	// be the same. We don't look for the second-level output script
-	// directly, as there might be more than one HTLC output to the same
-	// pkScript.
-	op := wire.OutPoint{
-		Hash:  *commitSpend.SpenderTxHash,
-		Index: commitSpend.SpenderInputIndex,
+	op, ok := h.successTxOutpoint(commitSpend)
+	if !ok {
+		return h.checkpointForeignSpend(commitSpend)
 	}
 
 	// If the 2nd-stage sweeping has already been started, we can