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..4cd181df8 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,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 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..f6cd0c1b8 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,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 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() diff --git a/contractcourt/htlc_timeout_resolver.go b/contractcourt/htlc_timeout_resolver.go index 480ec3b23..8adcb63b3 100644 --- a/contractcourt/htlc_timeout_resolver.go +++ b/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 ( // remoteTaprootWitnessSuccessSize = 5 + // localTaprootWitnessSuccessSize is the expected size of the witness + // on the local commitment for taproot channels. The spend path will + // look like + // - + 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, // - // 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, // // - 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 diff --git a/contractcourt/htlc_timeout_resolver_test.go b/contractcourt/htlc_timeout_resolver_test.go index 8b2451ed2..931361ff5 100644 --- a/contractcourt/htlc_timeout_resolver_test.go +++ b/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) + }) + } +} diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index cc346ff8b..1492d2b0c 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/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