From 17b6205f3a7b06b2db5f7807420629211ecb86ad Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 16 Aug 2019 19:57:26 -0700 Subject: [PATCH] chainntnfs: validate conf/spend ntfn registration parameters A height hint not being set would cause lnd to scan for the confirmation/spend of a txid/outpoint/address from genesis. The number of confirmations not being set within a confirmation request would cause the internal TxNotifier to deadlock when dispatching updates. --- chainntnfs/txnotifier.go | 54 +++++++++++++++++---- chainntnfs/txnotifier_test.go | 91 +++++++++++++++++++++++++++-------- 2 files changed, 116 insertions(+), 29 deletions(-) diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index 82961ab8..654b757e 100644 --- a/chainntnfs/txnotifier.go +++ b/chainntnfs/txnotifier.go @@ -46,10 +46,22 @@ var ( // with the TxNotifier but it been shut down. ErrTxNotifierExiting = errors.New("TxNotifier is exiting") - // ErrTxMaxConfs signals that the user requested a number of - // confirmations beyond the reorg safety limit. - ErrTxMaxConfs = fmt.Errorf("too many confirmations requested, max is %d", - MaxNumConfs) + // ErrNoScript is an error returned when a confirmation/spend + // registration is attempted without providing an accompanying output + // script. + ErrNoScript = errors.New("an output script must be provided") + + // ErrNoHeightHint is an error returned when a confirmation/spend + // registration is attempted without providing an accompanying height + // hint. + ErrNoHeightHint = errors.New("a height hint greater than 0 must be " + + "provided") + + // ErrNumConfsOutOfRange is an error returned when a confirmation/spend + // registration is attempted and the number of confirmations provided is + // out of range. + ErrNumConfsOutOfRange = fmt.Errorf("number of confirmations must be "+ + "between %d and %d", 1, MaxNumConfs) ) // rescanState indicates the progression of a registration before the notifier @@ -531,15 +543,27 @@ func NewTxNotifier(startHeight uint32, reorgSafetyLimit uint32, func (n *TxNotifier) newConfNtfn(txid *chainhash.Hash, pkScript []byte, numConfs, heightHint uint32) (*ConfNtfn, error) { - confRequest, err := NewConfRequest(txid, pkScript) - if err != nil { - return nil, err + // An accompanying output script must always be provided. + if len(pkScript) == 0 { + return nil, ErrNoScript } // Enforce that we will not dispatch confirmations beyond the reorg // safety limit. - if numConfs > n.reorgSafetyLimit { - return nil, ErrTxMaxConfs + if numConfs == 0 || numConfs > n.reorgSafetyLimit { + return nil, ErrNumConfsOutOfRange + } + + // A height hint must be provided to prevent scanning from the genesis + // block. + if heightHint == 0 { + return nil, ErrNoHeightHint + } + + // Ensure the output script is of a supported type. + confRequest, err := NewConfRequest(txid, pkScript) + if err != nil { + return nil, err } confID := atomic.AddUint64(&n.confClientCounter, 1) @@ -910,6 +934,18 @@ func (n *TxNotifier) dispatchConfDetails( func (n *TxNotifier) newSpendNtfn(outpoint *wire.OutPoint, pkScript []byte, heightHint uint32) (*SpendNtfn, error) { + // An accompanying output script must always be provided. + if len(pkScript) == 0 { + return nil, ErrNoScript + } + + // A height hint must be provided to prevent scanning from the genesis + // block. + if heightHint == 0 { + return nil, ErrNoHeightHint + } + + // Ensure the output script is of a supported type. spendRequest, err := NewSpendRequest(outpoint, pkScript) if err != nil { return nil, err diff --git a/chainntnfs/txnotifier_test.go b/chainntnfs/txnotifier_test.go index f79fd59d..79dc735a 100644 --- a/chainntnfs/txnotifier_test.go +++ b/chainntnfs/txnotifier_test.go @@ -124,30 +124,81 @@ func newMockHintCache() *mockHintCache { } } -// TestTxNotifierMaxConfs ensures that we are not able to register for more -// confirmations on a transaction than the maximum supported. -func TestTxNotifierMaxConfs(t *testing.T) { +// TestTxNotifierRegistrationValidation ensures that we are not able to register +// requests with invalid parameters. +func TestTxNotifierRegistrationValidation(t *testing.T) { t.Parallel() - hintCache := newMockHintCache() - n := chainntnfs.NewTxNotifier( - 10, chainntnfs.ReorgSafetyLimit, hintCache, hintCache, - ) - - // Registering one confirmation above the maximum should fail with - // ErrTxMaxConfs. - _, err := n.RegisterConf( - &chainntnfs.ZeroHash, testRawScript, chainntnfs.MaxNumConfs+1, 1, - ) - if err != chainntnfs.ErrTxMaxConfs { - t.Fatalf("expected chainntnfs.ErrTxMaxConfs, got %v", err) + testCases := []struct { + name string + pkScript []byte + numConfs uint32 + heightHint uint32 + checkSpend bool + err error + }{ + { + name: "empty output script", + pkScript: nil, + numConfs: 1, + heightHint: 1, + checkSpend: true, + err: chainntnfs.ErrNoScript, + }, + { + name: "zero num confs", + pkScript: testRawScript, + numConfs: 0, + heightHint: 1, + err: chainntnfs.ErrNumConfsOutOfRange, + }, + { + name: "exceed max num confs", + pkScript: testRawScript, + numConfs: chainntnfs.MaxNumConfs + 1, + heightHint: 1, + err: chainntnfs.ErrNumConfsOutOfRange, + }, + { + name: "empty height hint", + pkScript: testRawScript, + numConfs: 1, + heightHint: 0, + checkSpend: true, + err: chainntnfs.ErrNoHeightHint, + }, } - _, err = n.RegisterConf( - &chainntnfs.ZeroHash, testRawScript, chainntnfs.MaxNumConfs, 1, - ) - if err != nil { - t.Fatalf("unable to register conf ntfn: %v", err) + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + hintCache := newMockHintCache() + n := chainntnfs.NewTxNotifier( + 10, chainntnfs.ReorgSafetyLimit, hintCache, hintCache, + ) + + _, err := n.RegisterConf( + &chainntnfs.ZeroHash, testCase.pkScript, + testCase.numConfs, testCase.heightHint, + ) + if err != testCase.err { + t.Fatalf("conf registration expected error "+ + "\"%v\", got \"%v\"", testCase.err, err) + } + + if !testCase.checkSpend { + return + } + + _, err = n.RegisterSpend( + &chainntnfs.ZeroOutPoint, testCase.pkScript, + testCase.heightHint, + ) + if err != testCase.err { + t.Fatalf("spend registration expected error "+ + "\"%v\", got \"%v\"", testCase.err, err) + } + }) } }