chainntnfs: return an error when witness stack is empty

This commit adds a check when receiving transactions from
`BitcoindClient` so an error is returned when empty witness is found.

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,15 @@ func (m *MempoolNotifier) findRelevantInputs(tx *btcutil.Tx) inputsWithTx {
 			SpendingHeight:    0,
 		}
 		watchedInputs[*op] = details
+
+		// Return an error if the witness data is not present in the
+		// spending transaction.
+		if !details.HasSpenderWitness() {
+			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,16 @@ func (n *TxNotifier) updateSpendDetails(spendRequest SpendRequest,
 		return nil
 	}
 
+	// Return an error if the witness data is not present in the spending
+	// transaction.
+	//
+	// TODO(yy): maybe we should do a panic here instead to inform the user
+	// to reindex the bitcoind since it's critical error?
+	// panic("Please re-index your bitcoind...")
+	if !details.HasSpenderWitness() {
+		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()