diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index 12e4619f3a..14df4a8f0a 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -984,8 +984,6 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { }, }, } - closeTxid := closeTx.TxHash() - htlcOp := wire.OutPoint{ Hash: closeTx.TxHash(), Index: 0, @@ -1115,10 +1113,12 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { // Notify resolver that the HTLC output of the commitment has been // spent. + timeoutTx := outgoingRes.SignedTimeoutTx + timeoutTxid := timeoutTx.TxHash() oldNotifier.SpendChan <- &chainntnfs.SpendDetail{ - SpendingTx: closeTx, - SpentOutPoint: &wire.OutPoint{}, - SpenderTxHash: &closeTxid, + SpendingTx: timeoutTx, + SpentOutPoint: &htlcOp, + SpenderTxHash: &timeoutTxid, } // Finally, we should also receive a resolution message instructing the @@ -1147,6 +1147,7 @@ func TestChannelArbitratorLocalForceClosePendingHtlc(t *testing.T) { } // Notify resolver that the output of the timeout tx has been spent. + closeTxid := closeTx.TxHash() oldNotifier.SpendChan <- &chainntnfs.SpendDetail{ SpendingTx: closeTx, SpentOutPoint: &wire.OutPoint{}, diff --git a/contractcourt/htlc_success_resolver.go b/contractcourt/htlc_success_resolver.go index ebe1933bcd..f99d6f3957 100644 --- a/contractcourt/htlc_success_resolver.go +++ b/contractcourt/htlc_success_resolver.go @@ -31,6 +31,9 @@ var errInvalidSpendDetails = errors.New("invalid spend details") // errInvalidSuccessResolver identifies malformed success resolver state. var errInvalidSuccessResolver = errors.New("invalid success resolver") +// errInvalidSecondLevelOutput identifies malformed second-level output data. +var errInvalidSecondLevelOutput = errors.New("invalid second-level output") + // htlcSuccessResolver is a resolver that's capable of sweeping an incoming // HTLC output on-chain. If this is the remote party's commitment, we'll sweep // it directly from the commitment output *immediately*. If this is our @@ -639,9 +642,8 @@ func (h *htlcSuccessResolver) validatedSpendInput( // // The HTLC input uses SINGLE|ANYONECANPAY, so it commits to the transaction // output at the same index. A match returns that output's actual outpoint. -func (h *htlcSuccessResolver) matchSecondLevelOutput( - spendingTx *wire.MsgTx, - outputIndex uint32) (wire.OutPoint, bool, error) { +func matchSecondLevelOutput(spendingTx *wire.MsgTx, outputIndex uint32, + expected *wire.TxOut) (wire.OutPoint, bool, error) { var zeroOutpoint wire.OutPoint if spendingTx == nil { @@ -650,11 +652,10 @@ func (h *htlcSuccessResolver) matchSecondLevelOutput( ) } - expected := h.htlcResolution.SweepSignDesc.Output if expected == nil { return zeroOutpoint, false, fmt.Errorf( - "%w: missing expected output for %v", - errInvalidSuccessResolver, h.outpoint(), + "%w: missing expected output", + errInvalidSecondLevelOutput, ) } @@ -822,8 +823,9 @@ func (h *htlcSuccessResolver) sweepSuccessTxOutput() error { if err != nil { return err } - secondLevelOutpoint, matches, err := h.matchSecondLevelOutput( + secondLevelOutpoint, matches, err := matchSecondLevelOutput( commitSpend.SpendingTx, commitSpend.SpenderInputIndex, + h.htlcResolution.SweepSignDesc.Output, ) if err != nil { return err @@ -978,8 +980,9 @@ func (h *htlcSuccessResolver) resolveSuccessTx() error { if err != nil { return err } - secondLevelOutpoint, matches, err := h.matchSecondLevelOutput( + secondLevelOutpoint, matches, err := matchSecondLevelOutput( commitSpend.SpendingTx, commitSpend.SpenderInputIndex, + h.htlcResolution.SweepSignDesc.Output, ) if err != nil { return err diff --git a/contractcourt/htlc_success_resolver_test.go b/contractcourt/htlc_success_resolver_test.go index 99d3e8885c..314576c32d 100644 --- a/contractcourt/htlc_success_resolver_test.go +++ b/contractcourt/htlc_success_resolver_test.go @@ -528,11 +528,13 @@ func TestHtlcSuccessSingleStageClassification(t *testing.T) { require.Empty(t, ctx.htlcNotifier.finalHtlcEvents) } -// TestHtlcSuccessMatchSecondLevelOutput tests matching the success transaction -// output against the sweep descriptor. -func TestHtlcSuccessMatchSecondLevelOutput(t *testing.T) { +// TestMatchSecondLevelOutput tests matching a second-level transaction output +// against an expected sweep output. +func TestMatchSecondLevelOutput(t *testing.T) { + // Arrange a canonical second-level transaction with the expected sweep + // output at a nonzero index so each table case starts from valid data. claim := wire.OutPoint{Index: 2} - newMatch := func() (*htlcSuccessResolver, *wire.MsgTx) { + newMatch := func() (*wire.TxOut, *wire.MsgTx) { resolution := newSuccessTestResolution(claim) tx := &wire.MsgTx{ TxIn: []*wire.TxIn{ @@ -547,14 +549,14 @@ func TestHtlcSuccessMatchSecondLevelOutput(t *testing.T) { }, } - return &htlcSuccessResolver{ - htlcResolution: resolution, - }, tx + return resolution.SweepSignDesc.Output, tx } testCases := []struct { - name string - prepare func(*htlcSuccessResolver, *wire.MsgTx) *wire.MsgTx + name string + prepare func( + *wire.TxOut, *wire.MsgTx, + ) (*wire.TxOut, *wire.MsgTx) matches bool expectedErr error }{ @@ -564,59 +566,72 @@ func TestHtlcSuccessMatchSecondLevelOutput(t *testing.T) { }, { name: "commitment descriptor decoy", - prepare: func(resolver *htlcSuccessResolver, - tx *wire.MsgTx) *wire.MsgTx { + prepare: func(expected *wire.TxOut, + tx *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { // This decoy proves the matcher uses the sweep // descriptor, not the commitment descriptor. - resolution := &resolver.htlcResolution - signDetails := resolution.SignDetails - tx.TxOut[1] = cloneTxOut( - signDetails.SignDesc.Output, - ) + tx.TxOut[1] = cloneTxOut(testSignDesc.Output) + + return expected, tx + }, + }, + { + name: "value mismatch", + prepare: func(expected *wire.TxOut, + tx *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { + + tx.TxOut[1].Value++ + + return expected, tx + }, + }, + { + name: "script mismatch", + prepare: func(expected *wire.TxOut, + tx *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { + + tx.TxOut[1].PkScript = []byte{txscript.OP_FALSE} - return tx + return expected, tx }, }, { name: "missing indexed output", - prepare: func(_ *htlcSuccessResolver, - tx *wire.MsgTx) *wire.MsgTx { + prepare: func(expected *wire.TxOut, + tx *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { tx.TxOut = tx.TxOut[:1] - return tx + return expected, tx }, }, { name: "missing expected output", - prepare: func(resolver *htlcSuccessResolver, - tx *wire.MsgTx) *wire.MsgTx { + prepare: func(_ *wire.TxOut, + tx *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { - resolution := &resolver.htlcResolution - resolution.SweepSignDesc.Output = nil - - return tx + return nil, tx }, - expectedErr: errInvalidSuccessResolver, + expectedErr: errInvalidSecondLevelOutput, }, { name: "nil indexed output", - prepare: func(_ *htlcSuccessResolver, - tx *wire.MsgTx) *wire.MsgTx { + prepare: func(expected *wire.TxOut, + tx *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { tx.TxOut[1] = nil - return tx + return expected, tx }, expectedErr: errInvalidSpendDetails, }, { name: "nil transaction", - prepare: func(_ *htlcSuccessResolver, - _ *wire.MsgTx) *wire.MsgTx { + prepare: func(expected *wire.TxOut, + _ *wire.MsgTx) (*wire.TxOut, *wire.MsgTx) { - return nil + return expected, nil }, expectedErr: errInvalidSpendDetails, }, @@ -624,13 +639,21 @@ func TestHtlcSuccessMatchSecondLevelOutput(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - resolver, tx := newMatch() + // Arrange a valid transaction and clone it. Apply only + // the case-specific mutation. + expected, tx := newMatch() if testCase.prepare != nil { - tx = testCase.prepare(resolver, tx) + expected, tx = testCase.prepare(expected, tx) } - outpoint, matches, err := - resolver.matchSecondLevelOutput(tx, 1) + // Act by matching the committed index against the + // expected sweep output. + outpoint, matches, err := matchSecondLevelOutput( + tx, 1, expected, + ) + + // Assert malformed inputs return their sentinel. Formed + // inputs report an outpoint only for a complete match. if testCase.expectedErr != nil { require.ErrorIs(t, err, testCase.expectedErr) return diff --git a/contractcourt/htlc_timeout_resolver.go b/contractcourt/htlc_timeout_resolver.go index 3d63843bd2..115e8888af 100644 --- a/contractcourt/htlc_timeout_resolver.go +++ b/contractcourt/htlc_timeout_resolver.go @@ -128,6 +128,47 @@ func (h *htlcTimeoutResolver) ResolverKey() []byte { return key[:] } +// validateSpend validates a notifier spend of the resolver's HTLC output. +func (h *htlcTimeoutResolver) validateSpend( + spend *chainntnfs.SpendDetail) error { + + if spend == nil { + return fmt.Errorf( + "%w: missing spend detail", errInvalidSpendDetails, + ) + } + if spend.SpendingTx == nil { + return fmt.Errorf( + "%w: missing spending tx", errInvalidSpendDetails, + ) + } + if spend.SpenderInputIndex >= uint32(len(spend.SpendingTx.TxIn)) { + return fmt.Errorf( + "%w: input index %d out of range", + errInvalidSpendDetails, spend.SpenderInputIndex, + ) + } + + spendingInput := spend.SpendingTx.TxIn[spend.SpenderInputIndex] + if spendingInput == nil { + return fmt.Errorf( + "%w: missing input %d", errInvalidSpendDetails, + spend.SpenderInputIndex, + ) + } + + expected := h.outpoint() + if spendingInput.PreviousOutPoint != expected { + return fmt.Errorf( + "%w: input %d spends %v, expected %v", + errInvalidSpendDetails, spend.SpenderInputIndex, + spendingInput.PreviousOutPoint, expected, + ) + } + + return nil +} + const ( // expectedRemoteWitnessSuccessSize is the expected size of the witness // on the remote commitment transaction for an outgoing HTLC that is @@ -631,6 +672,9 @@ func (h *htlcTimeoutResolver) waitForConfirmedSpend(op *wire.OutPoint, if err != nil { return nil, err } + if err := h.validateSpend(spend); err != nil { + return nil, fmt.Errorf("invalid confirmed spend: %w", err) + } return spend, nil } @@ -873,6 +917,31 @@ func (h *htlcTimeoutResolver) waitForMempoolOrBlockSpend(op wire.OutPoint, } } +// handleBlockSpent translates a confirmed-spend notification into the result +// consumed by the watcher. Keeping shutdown and malformed-event handling here +// lets the select arm terminate uniformly without changing caller-visible +// error wrapping or successful spend delivery. +func (h *htlcTimeoutResolver) handleBlockSpent( + spendDetail *chainntnfs.SpendDetail, ok bool) *spendResult { + + if !ok { + return &spendResult{err: fmt.Errorf( + "block spent err: %w", errResolverShuttingDown, + )} + } + + if err := h.validateSpend(spendDetail); err != nil { + return &spendResult{err: fmt.Errorf( + "invalid block spend: %w", err, + )} + } + + log.Debugf("Found confirmed spend of HTLC output %s in tx=%s", + h.HtlcPoint(), spendDetail.SpenderTxHash) + + return &spendResult{spend: spendDetail} +} + // consumeSpendEvents consumes the spend events from the block and mempool // subscriptions. It exits when a spend event is received from the block, or // the resolver itself quits. When a spend event is received from the mempool, @@ -907,23 +976,7 @@ func (h *htlcTimeoutResolver) consumeSpendEvents(resultChan chan *spendResult, // the mempool again. Though a rare case, we should handle it // in a dedicated reorg system. case spendDetail, ok := <-blockSpent: - if !ok { - result.err = fmt.Errorf("block spent err: %w", - errResolverShuttingDown) - } else { - log.Debugf("Found confirmed spend of HTLC "+ - "output %s in tx=%s", op, - spendDetail.SpenderTxHash) - - result.spend = spendDetail - - // Once confirmed, persist the state on disk if - // we haven't seen the output's spending tx in - // mempool before. - } - - // Send the result and exit the loop. - resultChan <- result + resultChan <- h.handleBlockSpent(spendDetail, ok) return @@ -945,6 +998,14 @@ func (h *htlcTimeoutResolver) consumeSpendEvents(resultChan chan *spendResult, return } + if err := h.validateSpend(spendDetail); err != nil { + result.err = fmt.Errorf( + "invalid mempool spend: %w", err, + ) + resultChan <- result + + return + } log.Debugf("Found mempool spend of HTLC output %s "+ "in tx=%s", op, spendDetail.SpenderTxHash) @@ -1005,6 +1066,10 @@ func (h *htlcTimeoutResolver) isZeroFeeOutput() bool { func (h *htlcTimeoutResolver) waitHtlcSpendAndCheckPreimage() ( *chainntnfs.SpendDetail, error) { + if h.htlcResolution.SweepSignDesc.Output == nil { + return nil, fmt.Errorf("%w", errInvalidSecondLevelOutput) + } + // Wait for the htlc output to be spent, which can happen in one of the // paths, // 1. The remote party spends the htlc output using the preimage. @@ -1049,6 +1114,16 @@ func (h *htlcTimeoutResolver) sweepTimeoutTxOutput() error { return nil } + secondLevelOutpoint, matches, err := matchSecondLevelOutput( + commitSpend.SpendingTx, commitSpend.SpenderInputIndex, + h.htlcResolution.SweepSignDesc.Output, + ) + if err != nil { + return err + } + if !matches { + return nil + } waitHeight := h.deriveWaitHeight(h.htlcResolution.CsvDelay, commitSpend) @@ -1069,16 +1144,6 @@ func (h *htlcTimeoutResolver) sweepTimeoutTxOutput() error { 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, - } - var witType input.StandardWitnessType switch { case h.isTaprootFinal(): @@ -1092,7 +1157,7 @@ func (h *htlcTimeoutResolver) sweepTimeoutTxOutput() error { // Let the sweeper sweep the second-level output now that the CSV/CLTV // locks have expired. inp := h.makeSweepInput( - op, witType, + &secondLevelOutpoint, witType, input.LeaseHtlcOfferedTimeoutSecondLevel, &h.htlcResolution.SweepSignDesc, h.htlcResolution.CsvDelay, uint32(commitSpend.SpendingHeight), @@ -1209,9 +1274,20 @@ func (h *htlcTimeoutResolver) resolveRemoteCommitOutput() error { return h.claimCleanUp(spend) } + // TODO(yy): should also update the `RecoveredBalance` and + // `LimboBalance` like other paths? + + return h.resolveTimeoutSpend(spend) +} + +// resolveTimeoutSpend fails the incoming HTLC and checkpoints its confirmed +// on-chain timeout spend. +func (h *htlcTimeoutResolver) resolveTimeoutSpend( + spend *chainntnfs.SpendDetail) error { + // Send the clean up msg to fail the incoming HTLC. failureMsg := &lnwire.FailPermanentChannelFailure{} - err = h.DeliverResolutionMsg(ResolutionMsg{ + err := h.DeliverResolutionMsg(ResolutionMsg{ SourceChan: h.ShortChanID, HtlcIndex: h.htlc.HtlcIndex, Failure: failureMsg, @@ -1220,9 +1296,6 @@ func (h *htlcTimeoutResolver) resolveRemoteCommitOutput() error { return err } - // TODO(yy): should also update the `RecoveredBalance` and - // `LimboBalance` like other paths? - // Checkpoint the resolver, and write the outcome to disk. return h.checkpointClaim(spend) } @@ -1234,6 +1307,11 @@ func (h *htlcTimeoutResolver) resolveTimeoutTx() error { h.log.Debug("waiting for first-stage 2nd-level HTLC timeout tx to " + "confirm") + expectedOutput := h.htlcResolution.SweepSignDesc.Output + if expectedOutput == nil { + return fmt.Errorf("%w", errInvalidSecondLevelOutput) + } + // Wait for the second level transaction to confirm. spend, err := h.watchHtlcSpend() if err != nil { @@ -1248,15 +1326,32 @@ func (h *htlcTimeoutResolver) resolveTimeoutTx() error { } op := h.htlcResolution.ClaimOutpoint - spenderTxid := *spend.SpenderTxHash + var spenderTxid chainhash.Hash // If the timeout tx is a re-signed tx, we will need to find the actual // spent outpoint from the spending tx. if h.isZeroFeeOutput() { - op = wire.OutPoint{ - Hash: spenderTxid, - Index: spend.SpenderInputIndex, + var matches bool + op, matches, err = matchSecondLevelOutput( + spend.SpendingTx, spend.SpenderInputIndex, + expectedOutput, + ) + if err != nil { + return err + } + if !matches { + spenderTxid = spend.SpendingTx.TxHash() + terminalSpend := *spend + spentOutpoint := h.outpoint() + terminalSpend.SpentOutPoint = &spentOutpoint + terminalSpend.SpenderTxHash = &spenderTxid + + return h.resolveTimeoutSpend(&terminalSpend) } + + spenderTxid = op.Hash + } else { + spenderTxid = *spend.SpenderTxHash } // If the 2nd-stage sweeping has already been started, we can diff --git a/contractcourt/htlc_timeout_resolver_test.go b/contractcourt/htlc_timeout_resolver_test.go index d57b9192c1..548881f204 100644 --- a/contractcourt/htlc_timeout_resolver_test.go +++ b/contractcourt/htlc_timeout_resolver_test.go @@ -93,6 +93,20 @@ type htlcTimeoutTestCase struct { outcome channeldb.ResolverOutcome } +type recordingSpendNotifier struct { + chainntnfs.ChainNotifier + registrations chan wire.OutPoint +} + +// RegisterSpendNtfn records the watched outpoint before delegating to the +// shared test notifier. +func (r *recordingSpendNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, + pkScript []byte, heightHint uint32) (*chainntnfs.SpendEvent, error) { + + r.registrations <- *outpoint + return r.ChainNotifier.RegisterSpendNtfn(outpoint, pkScript, heightHint) +} + func genHtlcTimeoutTestCases() []htlcTimeoutTestCase { fakePreimageBytes := testResPreimage[:] @@ -133,6 +147,8 @@ func genHtlcTimeoutTestCases() []htlcTimeoutTestCase { remoteCommit: true, timeout: true, txToBroadcast: func() (*wire.MsgTx, error) { + templateTx.TxIn[0].PreviousOutPoint = + testChanPoint2 witness, err := input.ReceiverHtlcSpendTimeout( signer, fakeSignDesc, sweepTx, fakeTimeout, @@ -203,6 +219,8 @@ func genHtlcTimeoutTestCases() []htlcTimeoutTestCase { remoteCommit: true, timeout: false, txToBroadcast: func() (*wire.MsgTx, error) { + templateTx.TxIn[0].PreviousOutPoint = + testChanPoint2 witness, err := input.ReceiverHtlcSpendRedeem( &mock.DummySignature{}, txscript.SigHashAll, fakePreimageBytes, signer, fakeSignDesc, @@ -556,6 +574,174 @@ func TestHtlcTimeoutResolver(t *testing.T) { } } +// TestHtlcTimeoutRejectsMalformedSpendEvent tests validation of notifier +// spending transaction inputs. +func TestHtlcTimeoutRejectsMalformedSpendEvent(t *testing.T) { + // Arrange a valid watched outpoint and table mutations covering every + // malformed notifier field consumed by validateSpend. + htlcOutpoint := wire.OutPoint{Index: 3} + validSpend := newSpendDetail(htlcOutpoint, &wire.MsgTx{ + TxIn: []*wire.TxIn{{PreviousOutPoint: htlcOutpoint}}, + }, 0) + + testCases := []struct { + name string + spend *chainntnfs.SpendDetail + valid bool + }{ + { + name: "valid spend", + spend: validSpend, + valid: true, + }, + { + name: "missing detail", + }, + { + name: "missing transaction", + spend: &chainntnfs.SpendDetail{}, + }, + { + name: "input index out of range", + spend: &chainntnfs.SpendDetail{ + SpendingTx: &wire.MsgTx{}, + SpenderInputIndex: 1, + }, + }, + { + name: "missing indexed input", + spend: &chainntnfs.SpendDetail{ + SpendingTx: &wire.MsgTx{ + TxIn: []*wire.TxIn{nil}, + }, + }, + }, + { + name: "unexpected previous outpoint", + spend: newSpendDetail(htlcOutpoint, &wire.MsgTx{ + TxIn: []*wire.TxIn{{}}, + }, 0), + }, + } + + resolver := &htlcTimeoutResolver{ + htlcResolution: lnwallet.OutgoingHtlcResolution{ + ClaimOutpoint: htlcOutpoint, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + // Act by validating the spend before a watcher can log, + // classify, or forward its transaction. + err := resolver.validateSpend(testCase.spend) + + // Assert valid metadata passes unchanged. Malformed + // shapes retain the spend-details sentinel. + if testCase.valid { + require.NoError(t, err) + return + } + + require.ErrorIs(t, err, errInvalidSpendDetails) + }) + } +} + +// TestHtlcTimeoutConsumeSpendEvents tests that malformed block and mempool +// events are returned as errors before they are inspected further. +func TestHtlcTimeoutConsumeSpendEvents(t *testing.T) { + // Arrange equivalent block and mempool cases so both subscription + // branches receive malformed notifier data through their real channels. + testCases := []struct { + name string + mempool bool + }{ + {name: "block"}, + {name: "mempool", mempool: true}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + // Arrange channels and route a nil spend through only + // the backend selected by this case. + blockSpend := make(chan *chainntnfs.SpendDetail, 1) + mempoolSpend := make(chan *chainntnfs.SpendDetail, 1) + if testCase.mempool { + mempoolSpend <- nil + } else { + blockSpend <- nil + } + + resolver := &htlcTimeoutResolver{ + htlcResolution: lnwallet.OutgoingHtlcResolution{ + ClaimOutpoint: wire.OutPoint{Index: 3}, + }, + } + resultChan := make(chan *spendResult, 1) + + // Act by consuming the malformed event through the loop + // shared by the block and mempool subscriptions. + resolver.consumeSpendEvents( + resultChan, blockSpend, mempoolSpend, + ) + + // Assert the watcher returns validation failure without + // forwarding a spend that later code could dereference. + result := <-resultChan + require.ErrorIs(t, result.err, errInvalidSpendDetails) + require.Nil(t, result.spend) + }) + } +} + +// TestHtlcTimeoutRejectsMalformedConfirmedSpend tests that the confirmed-only +// watcher returns malformed remote commitment spends without side effects. +func TestHtlcTimeoutRejectsMalformedConfirmedSpend(t *testing.T) { + // Arrange a confirmed-only resolver whose notifier reports a + // transaction that does not spend the watched HTLC outpoint. + htlcOutpoint := wire.OutPoint{Index: 3} + ctx := newHtlcResolverTestContext(t, func(htlc channeldb.HTLC, + cfg ResolverConfig) ContractResolver { + + resolution := lnwallet.OutgoingHtlcResolution{ + ClaimOutpoint: htlcOutpoint, + SweepSignDesc: testSignDesc, + } + + return newTimeoutResolver(resolution, 0, htlc, 0, cfg) + }) + var checkpoints int + ctx.checkpoint = func(_ ContractResolver, + _ ...*channeldb.ResolverReport) error { + + checkpoints++ + return nil + } + ctx.notifier.SpendChan <- newSpendDetail( + htlcOutpoint, &wire.MsgTx{TxIn: []*wire.TxIn{{}}}, 0, + ) + + resolver, ok := ctx.resolver.(*htlcTimeoutResolver) + require.True(t, ok) + + // Act by running the remote-commit resolution path that waits directly + // for the malformed confirmed notification. + err := resolver.resolveRemoteCommitOutput() + + // Assert validation stops resolution before checkpoint, notification, + // outcome persistence, or sweep submission can produce side effects. + require.ErrorIs(t, err, errInvalidSpendDetails) + require.False(t, resolver.IsResolved()) + require.Zero(t, checkpoints) + require.Empty(t, ctx.resolutionChan) + require.False(t, ctx.finalHtlcOutcomeStored) + require.Empty(t, ctx.htlcNotifier.finalHtlcEvents) + + sweeper, ok := resolver.Sweeper.(*mockSweeper) + require.True(t, ok) + require.Empty(t, sweeper.sweptInputs) +} + // NOTE: the following tests essentially checks many of the same scenarios as // the test above, but they expand on it by checking resuming from checkpoints // at every stage. @@ -568,7 +754,9 @@ func TestHtlcTimeoutSingleStage(t *testing.T) { commitOutpoint := wire.OutPoint{Index: 3} sweepTx := &wire.MsgTx{ - TxIn: []*wire.TxIn{{}}, + TxIn: []*wire.TxIn{{ + PreviousOutPoint: commitOutpoint, + }}, TxOut: []*wire.TxOut{{}}, } @@ -792,7 +980,9 @@ func TestHtlcTimeoutSingleStageRemoteSpend(t *testing.T) { htlcOutpoint := wire.OutPoint{Index: 3} spendTx := &wire.MsgTx{ - TxIn: []*wire.TxIn{{}}, + TxIn: []*wire.TxIn{{ + PreviousOutPoint: commitOutpoint, + }}, TxOut: []*wire.TxOut{{}}, } @@ -1021,6 +1211,10 @@ func TestHtlcTimeoutSecondStageRemoteSpend(t *testing.T) { //nolint:ll func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { htlcOutpoint := wire.OutPoint{Index: 3} + secondLevelOutput := cloneTxOut(testSignDesc.Output) + secondLevelOutput.PkScript = []byte{0xff, 0xff} + sweepSignDesc := testSignDesc + sweepSignDesc.Output = secondLevelOutput timeoutTx := &wire.MsgTx{ TxIn: []*wire.TxIn{ @@ -1028,12 +1222,7 @@ func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { PreviousOutPoint: htlcOutpoint, }, }, - TxOut: []*wire.TxOut{ - { - Value: 123, - PkScript: []byte{0xff, 0xff}, - }, - }, + TxOut: []*wire.TxOut{cloneTxOut(secondLevelOutput)}, } // We set the timeout witness since the script is used when subscribing @@ -1068,7 +1257,7 @@ func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { Value: 111, PkScript: []byte{0xaa, 0xaa}, }, - timeoutTx.TxOut[0], + cloneTxOut(secondLevelOutput), }, } reSignedHash := reSignedTimeoutTx.TxHash() @@ -1091,7 +1280,7 @@ func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { SignDesc: testSignDesc, PeerSig: testSig, }, - SweepSignDesc: testSignDesc, + SweepSignDesc: sweepSignDesc, } firstStage := &channeldb.ResolverReport{ @@ -1143,6 +1332,7 @@ func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { } } + registrations := make(chan wire.OutPoint, 3) checkpoints := []checkpoint{ { // The output should be given to the sweeper. @@ -1201,6 +1391,15 @@ func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { return nil } + var watched wire.OutPoint + for watched != timeoutTxOutpoint { + select { + case watched = <-registrations: + case <-time.After(time.Second): + t.Fatal("expected spend registration") + } + } + mockSweepTxSpend(ctx) // The resolver should deliver a failure @@ -1256,8 +1455,261 @@ func TestHtlcTimeoutSecondStageSweeper(t *testing.T) { } testHtlcTimeout( - t, twoStageResolution, checkpoints, + t, twoStageResolution, checkpoints, registrations, + ) +} + +// testHtlcTimeoutOutputMatch builds a valid live zero-fee timeout spend, lets +// the caller mutate one output-matching condition, and verifies either clean +// rejection or terminal timeout handling without a phantom sweep. +func testHtlcTimeoutOutputMatch(t *testing.T, + prepare func(*lnwallet.OutgoingHtlcResolution, + *chainntnfs.SpendDetail), expectedErr error) { + + t.Helper() + + // Arrange a valid nonzero-index timeout spend, apply the caller's + // mutation, and wire it into a side-effect-tracking resolver. + htlcOutpoint := wire.OutPoint{Index: 3} + timeoutTx := &wire.MsgTx{ + TxIn: []*wire.TxIn{{ + PreviousOutPoint: htlcOutpoint, + Witness: wire.TxWitness{{0x01}}, + }}, + TxOut: []*wire.TxOut{cloneTxOut(testSignDesc.Output)}, + } + resolution := lnwallet.OutgoingHtlcResolution{ + ClaimOutpoint: htlcOutpoint, + SignedTimeoutTx: timeoutTx, + SignDetails: &input.SignDetails{ + SignDesc: testSignDesc, + PeerSig: testSig, + }, + SweepSignDesc: testSignDesc, + } + spendingTx := &wire.MsgTx{ + TxIn: []*wire.TxIn{ + {}, + { + PreviousOutPoint: htlcOutpoint, + Witness: wire.TxWitness{{0x01}}, + }, + }, + TxOut: []*wire.TxOut{ + {}, cloneTxOut(testSignDesc.Output), + }, + } + spend := newSpendDetail(htlcOutpoint, spendingTx, 1) + prepare(&resolution, spend) + + ctx := newHtlcResolverTestContext(t, func(htlc channeldb.HTLC, + cfg ResolverConfig) ContractResolver { + + return newTimeoutResolver(resolution, 0, htlc, 0, cfg) + }) + var reports []*channeldb.ResolverReport + ctx.checkpoint = func(_ ContractResolver, + got ...*channeldb.ResolverReport) error { + + reports = got + return nil + } + ctx.notifier.SpendChan <- spend + + resolver, ok := ctx.resolver.(*htlcTimeoutResolver) + require.True(t, ok) + + // Act by resolving the live timeout spend through output matching and + // terminal classification. + err := resolver.resolveTimeoutTx() + + // Assert malformed state stops before resolution, while a formed + // non-match checkpoints the actual spend as a terminal timeout. + if expectedErr != nil { + require.ErrorIs(t, err, expectedErr) + require.False(t, resolver.IsResolved()) + require.Empty(t, reports) + require.Empty(t, ctx.resolutionChan) + } else { + require.NoError(t, err) + require.True(t, resolver.IsResolved()) + require.Len(t, reports, 1) + require.Equal(t, htlcOutpoint, reports[0].OutPoint) + require.Equal(t, spendingTx.TxHash(), *reports[0].SpendTxID) + require.Equal(t, channeldb.ResolverOutcomeTimeout, + reports[0].ResolverOutcome) + require.NotNil(t, (<-ctx.resolutionChan).Failure) + } + + // Assert both outcomes avoid phantom sweep and unrelated persistence or + // notification side effects. + sweeper, ok := resolver.Sweeper.(*mockSweeper) + require.True(t, ok) + require.Empty(t, sweeper.sweptInputs) + require.False(t, ctx.finalHtlcOutcomeStored) + require.Empty(t, ctx.htlcNotifier.finalHtlcEvents) +} + +// TestHtlcTimeoutCheckpointsForeignOutput tests that a fully formed non-match +// is checkpointed as a terminal timeout without a phantom sweep. +func TestHtlcTimeoutCheckpointsForeignOutput(t *testing.T) { + // Arrange a foreign spend by removing the committed output and optional + // notifier provenance that resolution must normalize. + // Act by delegating the live resolution to testHtlcTimeoutOutputMatch. + // Assert there that the actual transaction is checkpointed. + // No second-level sweep may be submitted for the terminal timeout. + testHtlcTimeoutOutputMatch(t, func(_ *lnwallet.OutgoingHtlcResolution, + spend *chainntnfs.SpendDetail) { + + spend.SpendingTx.TxOut = spend.SpendingTx.TxOut[:1] + spend.SpentOutPoint = nil + spend.SpenderTxHash = nil + }, nil) +} + +// TestHtlcTimeoutRejectsMalformedOutput tests that malformed spend or resolver +// data returns before producing terminal side effects. +func TestHtlcTimeoutRejectsMalformedOutput(t *testing.T) { + // Arrange table mutations for malformed resolver and transaction + // outputs, together with the sentinel each shape must preserve. + testCases := []struct { + name string + prepare func(*lnwallet.OutgoingHtlcResolution, + *chainntnfs.SpendDetail) + expectedErr error + }{ + { + name: "missing expected output", + prepare: func( + resolution *lnwallet.OutgoingHtlcResolution, + _ *chainntnfs.SpendDetail) { + + resolution.SweepSignDesc.Output = nil + }, + expectedErr: errInvalidSecondLevelOutput, + }, + { + name: "nil indexed output", + prepare: func(_ *lnwallet.OutgoingHtlcResolution, + spend *chainntnfs.SpendDetail) { + + spend.SpendingTx.TxOut[1] = nil + }, + expectedErr: errInvalidSpendDetails, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + // Act by applying the case mutation in the shared live + // timeout harness. + // Assert there that the expected error precedes all + // checkpoint, outcome, notification, and sweep effects. + testHtlcTimeoutOutputMatch( + t, testCase.prepare, testCase.expectedErr, + ) + }) + } +} + +// TestHtlcTimeoutSkipsRestoredForeignOutput tests that Launch skips a phantom +// sweep and leaves a replayed foreign spend for Resolve to checkpoint. +func TestHtlcTimeoutSkipsRestoredForeignOutput(t *testing.T) { + // Arrange an incubating resolver restored from disk with a historical + // foreign spend that omits the committed second-level output. + commitOutpoint := wire.OutPoint{Index: 3} + timeoutTx := &wire.MsgTx{ + TxIn: []*wire.TxIn{{ + PreviousOutPoint: commitOutpoint, + Witness: wire.TxWitness{{0x01}}, + }}, + TxOut: []*wire.TxOut{cloneTxOut(testSignDesc.Output)}, + } + resolution := lnwallet.OutgoingHtlcResolution{ + ClaimOutpoint: commitOutpoint, + SignedTimeoutTx: timeoutTx, + SignDetails: &input.SignDetails{ + SignDesc: testSignDesc, + PeerSig: testSig, + }, + SweepSignDesc: testSignDesc, + } + foreignTx := &wire.MsgTx{ + TxIn: []*wire.TxIn{{ + PreviousOutPoint: commitOutpoint, + Witness: wire.TxWitness{{0x01}}, + }}, + } + foreignSpend := newSpendDetail(commitOutpoint, foreignTx, 0) + + ctx := newHtlcResolverTestContext(t, func(htlc channeldb.HTLC, + cfg ResolverConfig) ContractResolver { + + resolver := newTimeoutResolver(resolution, 0, htlc, 0, cfg) + resolver.outputIncubating = true + var state bytes.Buffer + require.NoError(t, resolver.Encode(&state)) + + restored, err := newTimeoutResolverFromReader(&state, cfg) + require.NoError(t, err) + restored.Supplement(htlc) + + return restored + }) + var ( + checkpoints int + reports []*channeldb.ResolverReport ) + ctx.checkpoint = func(_ ContractResolver, + got ...*channeldb.ResolverReport) error { + + checkpoints++ + reports = got + return nil + } + + resolver, ok := ctx.resolver.(*htlcTimeoutResolver) + require.True(t, ok) + sweeper, ok := resolver.Sweeper.(*mockSweeper) + require.True(t, ok) + ctx.notifier.SpendChan <- foreignSpend + + // Act by launching the restored resolver before Resolve can consume + // the replayed spend. + require.NoError(t, resolver.Launch()) + + // Assert Launch leaves cleanup ownership intact and submits no phantom + // sweep, checkpoint, report, outcome, or notification. + require.False(t, resolver.IsResolved()) + require.True(t, resolver.outputIncubating) + require.Empty(t, sweeper.sweptInputs) + require.Zero(t, checkpoints) + require.Empty(t, reports) + require.Empty(t, ctx.resolutionChan) + require.False(t, ctx.finalHtlcOutcomeStored) + require.Empty(t, ctx.htlcNotifier.finalHtlcEvents) + + ctx.notifier.SpendChan <- foreignSpend + + // Act again by resolving the same historical spend through the normal + // terminal lifecycle. + nextResolver, err := resolver.Resolve() + + // Assert Resolve owns the timeout checkpoint and failure notification + // while leaving the sweeper untouched. + require.NoError(t, err) + require.Nil(t, nextResolver) + require.True(t, resolver.IsResolved()) + require.Equal(t, 1, checkpoints) + require.Len(t, reports, 1) + require.Equal(t, commitOutpoint, reports[0].OutPoint) + require.Equal(t, foreignTx.TxHash(), *reports[0].SpendTxID) + require.Equal(t, channeldb.ResolverOutcomeTimeout, + reports[0].ResolverOutcome) + require.NotNil(t, (<-ctx.resolutionChan).Failure) + require.Empty(t, sweeper.sweptInputs) + require.False(t, ctx.finalHtlcOutcomeStored) + require.Empty(t, ctx.htlcNotifier.finalHtlcEvents) } // TestHtlcTimeoutSecondStageSweeperRemoteSpend tests that if a local timeout @@ -1292,7 +1744,9 @@ func TestHtlcTimeoutSecondStageSweeperRemoteSpend(t *testing.T) { timeoutTx.TxIn[0].Witness = timeoutWitness spendTx := &wire.MsgTx{ - TxIn: []*wire.TxIn{{}}, + TxIn: []*wire.TxIn{{ + PreviousOutPoint: commitOutpoint, + }}, TxOut: []*wire.TxOut{{}}, } @@ -1396,7 +1850,7 @@ func TestHtlcTimeoutSecondStageSweeperRemoteSpend(t *testing.T) { } func testHtlcTimeout(t *testing.T, resolution lnwallet.OutgoingHtlcResolution, - checkpoints []checkpoint) { + checkpoints []checkpoint, registrations ...chan wire.OutPoint) { t.Helper() @@ -1412,6 +1866,12 @@ func testHtlcTimeout(t *testing.T, resolution lnwallet.OutgoingHtlcResolution, htlc: htlc, htlcResolution: resolution, } + if len(registrations) != 0 { + r.Notifier = &recordingSpendNotifier{ + ChainNotifier: r.Notifier, + registrations: registrations[0], + } + } r.initLogger("htlcTimeoutResolver") return r diff --git a/docs/release-notes/release-notes-0.20.4.md b/docs/release-notes/release-notes-0.20.4.md index 1dbd1c0fe1..86aa91700b 100644 --- a/docs/release-notes/release-notes-0.20.4.md +++ b/docs/release-notes/release-notes-0.20.4.md @@ -39,6 +39,10 @@ incoming HTLC resolver could treat a foreign commitment spend as its own success transaction and offer a phantom input to the sweeper. +* [Fixed an issue](https://github.com/lightningnetwork/lnd/pull/11085) where an + outgoing HTLC resolver could treat a foreign commitment spend as its own + timeout transaction and offer a phantom input to the sweeper. + * Native SQL invoice migration [now correctly associates legacy AMP invoice HTLCs](https://github.com/lightningnetwork/lnd/pull/11106) with their AMP sub-invoices. Previously, the HTLC rows were inserted without those diff --git a/docs/release-notes/release-notes-0.21.3.md b/docs/release-notes/release-notes-0.21.3.md index aac3250846..03190e884d 100644 --- a/docs/release-notes/release-notes-0.21.3.md +++ b/docs/release-notes/release-notes-0.21.3.md @@ -39,6 +39,10 @@ incoming HTLC resolver could treat a foreign commitment spend as its own success transaction and offer a phantom input to the sweeper. +* [Fixed an issue](https://github.com/lightningnetwork/lnd/pull/11085) where an + outgoing HTLC resolver could treat a foreign commitment spend as its own + timeout transaction and offer a phantom input to the sweeper. + * Native SQL invoice migration [now correctly associates legacy AMP invoice HTLCs](https://github.com/lightningnetwork/lnd/pull/11106) with their AMP sub-invoices. Previously, the HTLC rows were inserted without those