From 28c3c835bf8a73d2754752406c2556f751fc3cf3 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 11 Jul 2023 17:58:42 +0800 Subject: [PATCH] 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 | 7 ++++- chainntnfs/btcdnotify/btcd.go | 7 ++++- chainntnfs/interface.go | 28 +++++++++++++++++ chainntnfs/mempool.go | 29 ++++++++++++++--- chainntnfs/mempool_test.go | 45 ++++++++++++++++++++++++--- chainntnfs/txnotifier.go | 15 +++++++++ chainntnfs/txnotifier_test.go | 22 +++++++++++-- 7 files changed, 139 insertions(+), 14 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 94f746fd7..bbda6fe4b 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/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 } diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index cc52c89c2..5ad03fa6b 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/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 } diff --git a/chainntnfs/interface.go b/chainntnfs/interface.go index 3362a9d2d..aaefee9ca 100644 --- a/chainntnfs/interface.go +++ b/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, diff --git a/chainntnfs/mempool.go b/chainntnfs/mempool.go index 8bb2af4c9..414861f59 100644 --- a/chainntnfs/mempool.go +++ b/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 diff --git a/chainntnfs/mempool_test.go b/chainntnfs/mempool_test.go index c5ee25d81..c0da43fa1 100644 --- a/chainntnfs/mempool_test.go +++ b/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) diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index 977ad3def..63c8746d0 100644 --- a/chainntnfs/txnotifier.go +++ b/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 diff --git a/chainntnfs/txnotifier_test.go b/chainntnfs/txnotifier_test.go index 354c2103b..9f3e15b74 100644 --- a/chainntnfs/txnotifier_test.go +++ b/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()