From 7a08825b1ead9251a56b328bccfd7b5ad1cbd542 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 1 Apr 2019 18:20:52 -0700 Subject: [PATCH 1/2] chainntnfs/bitcoindnotify: always call UpdateSpendDetails This commit fixes a bug that would cause the notifier not to commit spend hints for items that are not found. This is done by calling UpdateSpendDetails with a nil detail, permitting the notifier to begin updating the spend hints with new blocks that arrive at tip. The change is designed to mimic the behavior for historical confirmation dispatch. The symptom of this bug is needing to do many long rescans on startup, even if new blocks arrive after the rescan had completed. With this change, nodes will have to do the scans once more before their hints will be properly updated. Restarts from then on should not have this behavior. --- chainntnfs/bitcoindnotify/bitcoind.go | 70 +++++++++++++++------------ 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 15f52250..74e525ab 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -25,13 +25,6 @@ const ( notifierType = "bitcoind" ) -var ( - // ErrTransactionNotFound is an error returned when we attempt to find a - // transaction by manually scanning the chain within a specific range - // but it is not found. - ErrTransactionNotFound = errors.New("transaction not found within range") -) - // chainUpdate encapsulates an update to the current main chain. This struct is // used as an element within an unbounded queue in order to avoid blocking the // main rpc dispatch rule. @@ -264,7 +257,10 @@ out: go func() { defer b.wg.Done() - err := b.dispatchSpendDetailsManually(msg) + spendDetails, err := b.historicalSpendDetails( + msg.SpendRequest, + msg.StartHeight, msg.EndHeight, + ) if err != nil { chainntnfs.Log.Errorf("Rescan to "+ "determine the spend "+ @@ -273,6 +269,24 @@ out: msg.SpendRequest, msg.StartHeight, msg.EndHeight, err) + return + } + + // If the historical dispatch finished + // without error, we will invoke + // UpdateSpendDetails even if none were + // found. This allows the notifier to + // begin safely updating the height hint + // cache at tip, since any pending + // rescans have now completed. + err = b.txNotifier.UpdateSpendDetails( + msg.SpendRequest, spendDetails, + ) + if err != nil { + chainntnfs.Log.Errorf("Unable "+ + "to update spend "+ + "details of %v: %v", + msg.SpendRequest, err) } }() @@ -823,16 +837,13 @@ func (b *BitcoindNotifier) RegisterSpendNtfn(outpoint *wire.OutPoint, return ntfn.Event, nil } -// disaptchSpendDetailsManually attempts to manually scan the chain within the -// given height range for a transaction that spends the given outpoint/output -// script. If one is found, it's spending details are sent to the TxNotifier, -// which will then dispatch the notification to all of its clients. -func (b *BitcoindNotifier) dispatchSpendDetailsManually( - historicalDispatchDetails *chainntnfs.HistoricalSpendDispatch) error { - - spendRequest := historicalDispatchDetails.SpendRequest - startHeight := historicalDispatchDetails.StartHeight - endHeight := historicalDispatchDetails.EndHeight +// historicalSpendDetails attempts to manually scan the chain within the given +// height range for a transaction that spends the given outpoint/output script. +// If one is found, the spend details are assembled and returned to the caller. +// If the spend is not found, a nil spend detail will be returned. +func (b *BitcoindNotifier) historicalSpendDetails( + spendRequest chainntnfs.SpendRequest, startHeight, endHeight uint32) ( + *chainntnfs.SpendDetail, error) { // Begin scanning blocks at every height to determine if the outpoint // was spent. @@ -841,20 +852,20 @@ func (b *BitcoindNotifier) dispatchSpendDetailsManually( // processing the next height. select { case <-b.quit: - return chainntnfs.ErrChainNotifierShuttingDown + return nil, chainntnfs.ErrChainNotifierShuttingDown default: } // First, we'll fetch the block for the current height. blockHash, err := b.chainConn.GetBlockHash(int64(height)) if err != nil { - return fmt.Errorf("unable to retrieve hash for block "+ - "with height %d: %v", height, err) + return nil, fmt.Errorf("unable to retrieve hash for "+ + "block with height %d: %v", height, err) } block, err := b.chainConn.GetBlock(blockHash) if err != nil { - return fmt.Errorf("unable to retrieve block with hash "+ - "%v: %v", blockHash, err) + return nil, fmt.Errorf("unable to retrieve block "+ + "with hash %v: %v", blockHash, err) } // Then, we'll manually go over every input in every transaction @@ -863,29 +874,24 @@ func (b *BitcoindNotifier) dispatchSpendDetailsManually( for _, tx := range block.Transactions { matches, inputIdx, err := spendRequest.MatchesTx(tx) if err != nil { - return err + return nil, err } if !matches { continue } txHash := tx.TxHash() - details := &chainntnfs.SpendDetail{ + return &chainntnfs.SpendDetail{ SpentOutPoint: &tx.TxIn[inputIdx].PreviousOutPoint, SpenderTxHash: &txHash, SpendingTx: tx, SpenderInputIndex: inputIdx, SpendingHeight: int32(height), - } - - return b.txNotifier.UpdateSpendDetails( - historicalDispatchDetails.SpendRequest, - details, - ) + }, nil } } - return ErrTransactionNotFound + return nil, nil } // RegisterConfirmationsNtfn registers an intent to be notified once the target From 90ffc4a0ab7124a20f6faabedfcf4c0cd7ae44a1 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 1 Apr 2019 18:21:07 -0700 Subject: [PATCH 2/2] chainntnfs/bitcoindnotify: improve historical conf dispatch logging --- chainntnfs/bitcoindnotify/bitcoind.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 74e525ab..b7e598fb 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -228,7 +228,13 @@ out: msg.StartHeight, msg.EndHeight, ) if err != nil { - chainntnfs.Log.Error(err) + chainntnfs.Log.Errorf("Rescan to "+ + "determine the conf "+ + "details of %v within "+ + "range %d-%d failed: %v", + msg.ConfRequest, + msg.StartHeight, + msg.EndHeight, err) return } @@ -243,7 +249,10 @@ out: msg.ConfRequest, confDetails, ) if err != nil { - chainntnfs.Log.Error(err) + chainntnfs.Log.Errorf("Unable "+ + "to update conf "+ + "details of %v: %v", + msg.ConfRequest, err) } }()