Merge pull request #7811 from yyforyongyu/fix-panix

contractcourt: stop lnd when received witness is empty

chainntnfs/bitcoindnotify/bitcoind.go

@@ -487,7 +487,12 @@ func (b *BitcoindNotifier) handleRelevantTx(tx *btcutil.Tx,
 	// If this is a mempool spend, we'll ask the mempool notifier to hanlde
 	// it.
 	if mempool {
-		b.memNotifier.ProcessRelevantSpendTx(tx)
+		err := b.memNotifier.ProcessRelevantSpendTx(tx)
+		if err != nil {
+			chainntnfs.Log.Errorf("Unable to process transaction "+
+				"%v: %v", tx.Hash(), err)
+		}
+
 		return
 	}
 

chainntnfs/btcdnotify/btcd.go

@@ -536,7 +536,12 @@ func (b *BtcdNotifier) handleRelevantTx(tx *btcutil.Tx,
 	// If this is a mempool spend, we'll ask the mempool notifier to hanlde
 	// it.
 	if mempool {
-		b.memNotifier.ProcessRelevantSpendTx(tx)
+		err := b.memNotifier.ProcessRelevantSpendTx(tx)
+		if err != nil {
+			chainntnfs.Log.Errorf("Unable to process transaction "+
+				"%v: %v", tx.Hash(), err)
+		}
+
 		return
 	}
 

chainntnfs/interface.go

@@ -278,6 +278,34 @@ type SpendDetail struct {
 	SpendingHeight    int32
 }
 
+// HasSpenderWitness returns true if the spending transaction has non-empty
+// witness.
+func (s *SpendDetail) HasSpenderWitness() bool {
+	tx := s.SpendingTx
+
+	// If there are no inputs, then there is no witness.
+	if len(tx.TxIn) == 0 {
+		return false
+	}
+
+	// If the spender input index is larger than the number of inputs, then
+	// we don't have a witness and this is an error case so we log it.
+	if uint32(len(tx.TxIn)) <= s.SpenderInputIndex {
+		Log.Errorf("SpenderInputIndex %d is out of range for tx %v",
+			s.SpenderInputIndex, tx.TxHash())
+
+		return false
+	}
+
+	// If the witness is empty, then there is no witness.
+	if len(tx.TxIn[s.SpenderInputIndex].Witness) == 0 {
+		return false
+	}
+
+	// If the witness is non-empty, then we have a witness.
+	return true
+}
+
 // String returns a string representation of SpendDetail.
 func (s *SpendDetail) String() string {
 	return fmt.Sprintf("%v[%d] spending %v at height=%v", s.SpenderTxHash,

chainntnfs/mempool.go

@@ -146,7 +146,13 @@ func (m *MempoolNotifier) UnsubsribeConfirmedSpentTx(tx *btcutil.Tx) {
 	Log.Tracef("Unsubscribe confirmed tx %s", tx.Hash())
 
 	// Get the spent inputs of interest.
-	spentInputs := m.findRelevantInputs(tx)
+	spentInputs, err := m.findRelevantInputs(tx)
+	if err != nil {
+		Log.Errorf("Unable to find relevant inputs for tx %s: %v",
+			tx.Hash(), err)
+
+		return
+	}
 
 	// Unsubscribe the subscribers.
 	for outpoint := range spentInputs {
@@ -160,15 +166,20 @@ func (m *MempoolNotifier) UnsubsribeConfirmedSpentTx(tx *btcutil.Tx) {
 // ProcessRelevantSpendTx takes a transaction and checks whether it spends any
 // of the subscribed inputs. If so, spend notifications are sent to the
 // relevant subscribers.
-func (m *MempoolNotifier) ProcessRelevantSpendTx(tx *btcutil.Tx) {
+func (m *MempoolNotifier) ProcessRelevantSpendTx(tx *btcutil.Tx) error {
 	Log.Tracef("Processing mempool tx %s", tx.Hash())
 	defer Log.Tracef("Finished processing mempool tx %s", tx.Hash())
 
 	// Get the spent inputs of interest.
-	spentInputs := m.findRelevantInputs(tx)
+	spentInputs, err := m.findRelevantInputs(tx)
+	if err != nil {
+		return err
+	}
 
 	// Notify the subscribers.
 	m.notifySpent(spentInputs)
+
+	return nil
 }
 
 // TearDown stops the notifier and cleans up resources.
@@ -182,7 +193,9 @@ func (m *MempoolNotifier) TearDown() {
 
 // findRelevantInputs takes a transaction to find the subscribed inputs and
 // returns them.
-func (m *MempoolNotifier) findRelevantInputs(tx *btcutil.Tx) inputsWithTx {
+func (m *MempoolNotifier) findRelevantInputs(tx *btcutil.Tx) (inputsWithTx,
+	error) {
+
 	txid := tx.Hash()
 	watchedInputs := make(inputsWithTx)
 
@@ -209,9 +222,23 @@ func (m *MempoolNotifier) findRelevantInputs(tx *btcutil.Tx) inputsWithTx {
 			SpendingHeight:    0,
 		}
 		watchedInputs[*op] = details
+
+		// Sanity check the witness stack. If it's not empty, continue
+		// to next iteration.
+		if details.HasSpenderWitness() {
+			continue
+		}
+
+		// Return an error if the witness data is not present in the
+		// spending transaction.
+		Log.Criticalf("Found spending tx for outpoint=%v in mempool, "+
+			"but the transaction %v does not have witness",
+			op, details.SpendingTx.TxHash())
+
+		return nil, ErrEmptyWitnessStack
 	}
 
-	return watchedInputs
+	return watchedInputs, nil
 }
 
 // notifySpent iterates all the spentInputs and notifies the subscribers about

chainntnfs/mempool_test.go

@@ -11,6 +11,9 @@ import (
 
 const testTimeout = 5 * time.Second
 
+// dummyWitness is used to fill the witness data in a transaction.
+var dummyWitness = [][]byte{{0x01}}
+
 // TestMempoolSubscribeInput tests that we can successfully subscribe an input.
 func TestMempoolSubscribeInput(t *testing.T) {
 	t.Parallel()
@@ -107,9 +110,9 @@ func TestMempoolUnsubscribeEvent(t *testing.T) {
 	require.True(t, loaded)
 }
 
-// TestMempoolFindRelevantInputs tests that the mempool notifier can find the
-// spend of subscribed inputs from a given transaction.
-func TestMempoolFindRelevantInputs(t *testing.T) {
+// TestMempoolFindRelevantInputsEmptyWitness tests that the mempool notifier
+// returns an error when the witness stack is empty.
+func TestMempoolFindRelevantInputsEmptyWitness(t *testing.T) {
 	t.Parallel()
 
 	// Create a new mempool notifier instance.
@@ -132,6 +135,37 @@ func TestMempoolFindRelevantInputs(t *testing.T) {
 	}
 	tx := btcutil.NewTx(msgTx)
 
+	// Call the method.
+	result, err := notifier.findRelevantInputs(tx)
+	require.ErrorIs(t, err, ErrEmptyWitnessStack)
+	require.Nil(t, result)
+}
+
+// TestMempoolFindRelevantInputs tests that the mempool notifier can find the
+// spend of subscribed inputs from a given transaction.
+func TestMempoolFindRelevantInputs(t *testing.T) {
+	t.Parallel()
+
+	// Create a new mempool notifier instance.
+	notifier := NewMempoolNotifier()
+
+	// Create two inputs and subscribe to the second one.
+	input1 := wire.OutPoint{Hash: [32]byte{1}}
+	input2 := wire.OutPoint{Hash: [32]byte{2}}
+
+	// Make input2 the subscribed input.
+	notifier.SubscribeInput(input2)
+
+	// Create a transaction that spends the above two inputs.
+	msgTx := &wire.MsgTx{
+		TxIn: []*wire.TxIn{
+			{PreviousOutPoint: input1, Witness: dummyWitness},
+			{PreviousOutPoint: input2, Witness: dummyWitness},
+		},
+		TxOut: []*wire.TxOut{},
+	}
+	tx := btcutil.NewTx(msgTx)
+
 	// Create the expected spend detail.
 	detailExp := &SpendDetail{
 		SpentOutPoint:     &input2,
@@ -141,7 +175,8 @@ func TestMempoolFindRelevantInputs(t *testing.T) {
 	}
 
 	// Call the method.
-	result := notifier.findRelevantInputs(tx)
+	result, err := notifier.findRelevantInputs(tx)
+	require.NoError(t, err)
 
 	// Verify that the result is as expected.
 	require.Contains(t, result, input2)
@@ -365,7 +400,7 @@ func TestMempoolUnsubscribeConfirmedSpentTx(t *testing.T) {
 	// Create a transaction that spends input1.
 	msgTx := &wire.MsgTx{
 		TxIn: []*wire.TxIn{
-			{PreviousOutPoint: input1},
+			{PreviousOutPoint: input1, Witness: dummyWitness},
 		},
 	}
 	tx := btcutil.NewTx(msgTx)

chainntnfs/txnotifier.go

@@ -74,6 +74,11 @@ var (
 	// out of range.
 	ErrNumConfsOutOfRange = fmt.Errorf("number of confirmations must be "+
 		"between %d and %d", 1, MaxNumConfs)
+
+	// ErrEmptyWitnessStack is returned when a spending transaction has an
+	// empty witness stack. More details in,
+	// - https://github.com/bitcoin/bitcoin/issues/28730
+	ErrEmptyWitnessStack = errors.New("witness stack is empty")
 )
 
 // rescanState indicates the progression of a registration before the notifier
@@ -1297,6 +1302,19 @@ func (n *TxNotifier) updateSpendDetails(spendRequest SpendRequest,
 		return nil
 	}
 
+	// Return an error if the witness data is not present in the spending
+	// transaction.
+	//
+	// NOTE: if the witness stack is empty, we will do a critical log which
+	// shuts down the node.
+	if !details.HasSpenderWitness() {
+		Log.Criticalf("Found spending tx for outpoint=%v, but the "+
+			"transaction %v does not have witness",
+			spendRequest.OutPoint, details.SpendingTx.TxHash())
+
+		return ErrEmptyWitnessStack
+	}
+
 	// If the historical rescan found the spending transaction for this
 	// request, but it's at a later height than the notifier (this can
 	// happen due to latency with the backend during a reorg), then we'll

chainntnfs/txnotifier_test.go

@@ -34,6 +34,8 @@ var (
 		0x86, 0xf4, 0xcb, 0xf9, 0x8e, 0xae, 0xd2, 0x21,
 		0xb3, 0x0b, 0xd9, 0xa0, 0xb9, 0x28,
 	}
+
+	testWitness = [][]byte{{0x01}}
 )
 
 type mockHintCache struct {
@@ -747,6 +749,7 @@ func TestTxNotifierHistoricalSpendDispatch(t *testing.T) {
 	spendTx := wire.NewMsgTx(2)
 	spendTx.AddTxIn(&wire.TxIn{
 		PreviousOutPoint: spentOutpoint,
+		Witness:          testWitness,
 		SignatureScript:  testSigScript,
 	})
 	spendTxHash := spendTx.TxHash()
@@ -894,10 +897,17 @@ func TestTxNotifierMultipleHistoricalSpendRescans(t *testing.T) {
 	// register another notification. We should also expect not to see a
 	// historical rescan request since the confirmation details should be
 	// cached.
+	msgTx := &wire.MsgTx{
+		TxIn: []*wire.TxIn{
+			{PreviousOutPoint: op, Witness: testWitness},
+		},
+		TxOut: []*wire.TxOut{},
+	}
+
 	spendDetails := &chainntnfs.SpendDetail{
 		SpentOutPoint:     &op,
 		SpenderTxHash:     &chainntnfs.ZeroHash,
-		SpendingTx:        wire.NewMsgTx(2),
+		SpendingTx:        msgTx,
 		SpenderInputIndex: 0,
 		SpendingHeight:    startingHeight - 1,
 	}
@@ -1021,10 +1031,17 @@ func TestTxNotifierMultipleHistoricalNtfns(t *testing.T) {
 	// We'll assume a historical rescan was dispatched and found the
 	// following spend details. We'll let the notifier know so that it can
 	// stop watching at tip.
+	msgTx := &wire.MsgTx{
+		TxIn: []*wire.TxIn{
+			{PreviousOutPoint: op, Witness: testWitness},
+		},
+		TxOut: []*wire.TxOut{},
+	}
+
 	expectedSpendDetails := &chainntnfs.SpendDetail{
 		SpentOutPoint:     &op,
 		SpenderTxHash:     &chainntnfs.ZeroHash,
-		SpendingTx:        wire.NewMsgTx(2),
+		SpendingTx:        msgTx,
 		SpenderInputIndex: 0,
 		SpendingHeight:    startingHeight - 1,
 	}
@@ -1744,6 +1761,7 @@ func TestTxNotifierSpendReorgMissed(t *testing.T) {
 	spendTx := wire.NewMsgTx(2)
 	spendTx.AddTxIn(&wire.TxIn{
 		PreviousOutPoint: op,
+		Witness:          testWitness,
 		SignatureScript:  testSigScript,
 	})
 	spendTxHash := spendTx.TxHash()

contractcourt/htlc_timeout_resolver.go

@@ -113,6 +113,11 @@ const (
 	// swept on-chain by them with pre-image.
 	expectedRemoteWitnessSuccessSize = 5
 
+	// expectedLocalWitnessSuccessSize is the expected size of the witness
+	// on the local commitment transaction for an outgoing HTLC that is
+	// swept on-chain by them with pre-image.
+	expectedLocalWitnessSuccessSize = 3
+
 	// remotePreimageIndex index within the witness on the remote
 	// commitment transaction that will hold they pre-image if they go to
 	// sweep it on chain.
@@ -130,6 +135,12 @@ const (
 	//     <control_block>
 	remoteTaprootWitnessSuccessSize = 5
 
+	// localTaprootWitnessSuccessSize is the expected size of the witness
+	// on the local commitment for taproot channels. The spend path will
+	// look like
+	//  - <receiver sig> <preimage> <success_script> <control_block>
+	localTaprootWitnessSuccessSize = 4
+
 	// taprootRemotePreimageIndex is the index within the witness on the
 	// taproot remote commitment spend that'll hold the pre-image if the
 	// remote party sweeps it.
@@ -329,10 +340,10 @@ func isPreimageSpend(isTaproot bool, spend *chainntnfs.SpendDetail,
 	//   - <sender sig> <receiver sig> <preimage> <success_script>
 	//     <control_block>
 	case isTaproot && !localCommit:
-		preImageIdx := taprootRemotePreimageIndex
-		//nolint:lll
-		return len(spendingWitness) == remoteTaprootWitnessSuccessSize &&
-			len(spendingWitness[preImageIdx]) == lntypes.HashSize
+		return checkSizeAndIndex(
+			spendingWitness, remoteTaprootWitnessSuccessSize,
+			taprootRemotePreimageIndex,
+		)
 
 	// Otherwise, then if this is our local commitment transaction, then if
 	// they're sweeping the transaction, it'll be directly from the output,
@@ -343,8 +354,10 @@ func isPreimageSpend(isTaproot bool, spend *chainntnfs.SpendDetail,
 	//
 	//  - <receiver sig> <preimage> <success_script> <control_block>
 	case isTaproot && localCommit:
-		return len(spendingWitness[localPreimageIndex]) ==
-			lntypes.HashSize
+		return checkSizeAndIndex(
+			spendingWitness, localTaprootWitnessSuccessSize,
+			localPreimageIndex,
+		)
 
 	// If this is the non-taproot, remote commitment then the only possible
 	// spends for outgoing HTLCs are:
@@ -358,9 +371,10 @@ func isPreimageSpend(isTaproot bool, spend *chainntnfs.SpendDetail,
 	// then this is a remote spend. If not, then we swept it ourselves, or
 	// revoked their output.
 	case !isTaproot && !localCommit:
-		return len(spendingWitness) == expectedRemoteWitnessSuccessSize &&
-			len(spendingWitness[remotePreimageIndex]) ==
-				lntypes.HashSize
+		return checkSizeAndIndex(
+			spendingWitness, expectedRemoteWitnessSuccessSize,
+			remotePreimageIndex,
+		)
 
 	// Otherwise, for our non-taproot commitment, the only possible spends
 	// for an outgoing HTLC are:
@@ -373,12 +387,25 @@ func isPreimageSpend(isTaproot bool, spend *chainntnfs.SpendDetail,
 	// element in the witness.
 	case !isTaproot:
 		fallthrough
+
 	default:
-		return len(spendingWitness[localPreimageIndex]) ==
-			lntypes.HashSize
+		return checkSizeAndIndex(
+			spendingWitness, expectedLocalWitnessSuccessSize,
+			localPreimageIndex,
+		)
 	}
 }
 
+// checkSizeAndIndex checks that the witness is of the expected size and that
+// the witness element at the specified index is of the expected size.
+func checkSizeAndIndex(witness wire.TxWitness, size, index int) bool {
+	if len(witness) != size {
+		return false
+	}
+
+	return len(witness[index]) == lntypes.HashSize
+}
+
 // Resolve kicks off full resolution of an outgoing HTLC output. If it's our
 // commitment, it isn't resolved until we see the second level HTLC txn
 // confirmed. If it's the remote party's commitment, we don't resolve until we

contractcourt/htlc_timeout_resolver_test.go

@@ -24,6 +24,11 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+var (
+	dummyBytes    = []byte{0}
+	preimageBytes = bytes.Repeat([]byte{1}, lntypes.HashSize)
+)
+
 type mockWitnessBeacon struct {
 	preImageUpdates chan lntypes.Preimage
 	newPreimages    chan []lntypes.Preimage
@@ -1324,3 +1329,146 @@ func testHtlcTimeout(t *testing.T, resolution lnwallet.OutgoingHtlcResolution,
 		_ = runFromCheckpoint(t, ctx, checkpoints[i+1:])
 	}
 }
+
+// TestCheckSizeAndIndex checks that the `checkSizeAndIndex` behaves as
+// expected.
+func TestCheckSizeAndIndex(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name     string
+		witness  wire.TxWitness
+		size     int
+		index    int
+		expected bool
+	}{
+		{
+			// Test that a witness with the correct size and index
+			// for the preimage.
+			name: "valid preimage",
+			witness: wire.TxWitness{
+				dummyBytes, preimageBytes,
+			},
+			size:     2,
+			index:    1,
+			expected: true,
+		},
+		{
+			// Test that a witness with the wrong size.
+			name: "wrong witness size",
+			witness: wire.TxWitness{
+				dummyBytes, preimageBytes,
+			},
+			size:     3,
+			index:    1,
+			expected: false,
+		},
+		{
+			// Test that a witness with the right size but wrong
+			// preimage index.
+			name: "wrong preimage index",
+			witness: wire.TxWitness{
+				dummyBytes, preimageBytes,
+			},
+			size:     2,
+			index:    0,
+			expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			result := checkSizeAndIndex(
+				tc.witness, tc.size, tc.index,
+			)
+			require.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+// TestIsPreimageSpend tests `isPreimageSpend` can successfully detect a
+// preimage spend based on whether the commitment is local or remote.
+func TestIsPreimageSpend(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name        string
+		witness     wire.TxWitness
+		isTaproot   bool
+		localCommit bool
+	}{
+		{
+			// Test a preimage spend on the remote commitment for
+			// taproot channels.
+			name: "tap preimage spend on remote",
+			witness: wire.TxWitness{
+				dummyBytes, dummyBytes, preimageBytes,
+				dummyBytes, dummyBytes,
+			},
+			isTaproot:   true,
+			localCommit: false,
+		},
+		{
+			// Test a preimage spend on the local commitment for
+			// taproot channels.
+			name: "tap preimage spend on local",
+			witness: wire.TxWitness{
+				dummyBytes, preimageBytes,
+				dummyBytes, dummyBytes,
+			},
+			isTaproot:   true,
+			localCommit: true,
+		},
+		{
+			// Test a preimage spend on the remote commitment for
+			// non-taproot channels.
+			name: "preimage spend on remote",
+			witness: wire.TxWitness{
+				dummyBytes, dummyBytes, dummyBytes,
+				preimageBytes, dummyBytes,
+			},
+			isTaproot:   false,
+			localCommit: false,
+		},
+		{
+			// Test a preimage spend on the local commitment for
+			// non-taproot channels.
+			name: "preimage spend on local",
+			witness: wire.TxWitness{
+				dummyBytes, preimageBytes, dummyBytes,
+			},
+			isTaproot:   false,
+			localCommit: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		// Run the test.
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			// Create a test spend detail that spends the HTLC
+			// output.
+			spend := &chainntnfs.SpendDetail{
+				SpendingTx:        &wire.MsgTx{},
+				SpenderInputIndex: 0,
+			}
+
+			// Attach the testing witness.
+			spend.SpendingTx.TxIn = []*wire.TxIn{{
+				Witness: tc.witness,
+			}}
+
+			result := isPreimageSpend(
+				tc.isTaproot, spend, tc.localCommit,
+			)
+			require.True(t, result)
+		})
+	}
+}

docs/release-notes/release-notes-0.18.0.md

@@ -32,6 +32,10 @@
   attempt](https://github.com/lightningnetwork/lnd/pull/8091) when sweeping new
   inputs with retried ones.
 
+* [Fixed](https://github.com/lightningnetwork/lnd/pull/7811) a case where `lnd`
+  might panic due to empty witness data found in a transaction. More details
+  can be found [here](https://github.com/bitcoin/bitcoin/issues/28730).
+
 # New Features
 ## Functional Enhancements