From 1200b75546753cfc45acb3da07cb29b6fae37291 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Sun, 10 Nov 2024 17:41:48 +0800 Subject: [PATCH 1/5] chainntnfs: always notify txns before block This commit changes the order of notifications when a relevant tx is found in a block and now we will always notify the tx subscribers before notifying the block, which has implications in the upcoming blockbeat. When a block notification is subscribed via `RegisterBlockEpochNtfn` and a confirm or spend is subscribed via `RegisterConfirmationsNtfn` or `RegisterSpendNtfn`, we would always notify the block first before the tx, causing the subsystem to think there's no relevant txns found in this block, while the notifications are sent later. We now fix it by always sending the txns notifications first, so the subsystem can receive the txns, process them, then attempt to advance its state based on the block received. --- chainntnfs/bitcoindnotify/bitcoind.go | 8 +++++++- chainntnfs/btcdnotify/btcd.go | 7 ++++++- chainntnfs/neutrinonotify/neutrino.go | 8 +++++++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 8bcf8872b1c..91c38d8d6eb 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -665,8 +665,14 @@ func (b *BitcoindNotifier) handleBlockConnected(block chainntnfs.BlockEpoch) err // satisfy any client requests based upon the new block. b.bestBlock = block + err = b.txNotifier.NotifyHeight(uint32(block.Height)) + if err != nil { + return fmt.Errorf("unable to notify height: %w", err) + } + b.notifyBlockEpochs(block.Height, block.Hash, block.BlockHeader) - return b.txNotifier.NotifyHeight(uint32(block.Height)) + + return nil } // notifyBlockEpochs notifies all registered block epoch clients of the newly diff --git a/chainntnfs/btcdnotify/btcd.go b/chainntnfs/btcdnotify/btcd.go index bcbfa571a58..ff91b5aee65 100644 --- a/chainntnfs/btcdnotify/btcd.go +++ b/chainntnfs/btcdnotify/btcd.go @@ -725,11 +725,16 @@ func (b *BtcdNotifier) handleBlockConnected(epoch chainntnfs.BlockEpoch) error { // satisfy any client requests based upon the new block. b.bestBlock = epoch + err = b.txNotifier.NotifyHeight(uint32(epoch.Height)) + if err != nil { + return fmt.Errorf("unable to notify height: %w", err) + } + b.notifyBlockEpochs( epoch.Height, epoch.Hash, epoch.BlockHeader, ) - return b.txNotifier.NotifyHeight(uint32(epoch.Height)) + return nil } // notifyBlockEpochs notifies all registered block epoch clients of the newly diff --git a/chainntnfs/neutrinonotify/neutrino.go b/chainntnfs/neutrinonotify/neutrino.go index 55d92357797..7b93bcd80db 100644 --- a/chainntnfs/neutrinonotify/neutrino.go +++ b/chainntnfs/neutrinonotify/neutrino.go @@ -689,10 +689,16 @@ func (n *NeutrinoNotifier) handleBlockConnected(newBlock *filteredBlock) error { n.bestBlock.Height = int32(newBlock.height) n.bestBlock.BlockHeader = newBlock.header + err = n.txNotifier.NotifyHeight(newBlock.height) + if err != nil { + return fmt.Errorf("unable to notify height: %w", err) + } + n.notifyBlockEpochs( int32(newBlock.height), &newBlock.hash, newBlock.header, ) - return n.txNotifier.NotifyHeight(newBlock.height) + + return nil } // getFilteredBlock is a utility to retrieve the full filtered block from a block epoch. From 4ff1f19fbf7b7ae11a8cf39c5e3ecc00c2bd4201 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 1 Nov 2024 08:41:01 +0800 Subject: [PATCH 2/5] chainntnfs: make sure notification is sent to the whole set This commit fixes a bug where the confirmation details may be missed. When the same tx is subscribed via `RegisterConfirmationsNtfn`, we will put them into the same set and notify the whole set. However, this logic is missing when performing the rescan - once the confirmation detail is found, we only notify the current subscriber. Later we will skip notifying other subscribers in `UpdateConfDetails` due to the `confSet.details != nil` check. We now fix it by immediately notify all the subscribers when the confirmation detail is found during the rescan. --- chainntnfs/interface.go | 3 +++ chainntnfs/txnotifier.go | 42 ++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/chainntnfs/interface.go b/chainntnfs/interface.go index 3337f1451a6..b2383636aa2 100644 --- a/chainntnfs/interface.go +++ b/chainntnfs/interface.go @@ -258,6 +258,9 @@ type ConfirmationEvent struct { // channels. func NewConfirmationEvent(numConfs uint32, cancel func()) *ConfirmationEvent { return &ConfirmationEvent{ + // We cannot rely on the subscriber to immediately read from + // the channel so we need to create a larger buffer to avoid + // blocking the notifier. Confirmed: make(chan *TxConfirmation, 1), Updates: make(chan uint32, numConfs), NegativeConf: make(chan int32, 1), diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index be471da3c77..ee48bd65a70 100644 --- a/chainntnfs/txnotifier.go +++ b/chainntnfs/txnotifier.go @@ -664,8 +664,8 @@ func (n *TxNotifier) RegisterConf(txid *chainhash.Hash, pkScript []byte, // already been found, we'll attempt to deliver them immediately // to this client. Log.Debugf("Attempting to dispatch confirmation for %v on "+ - "registration since rescan has finished", - ntfn.ConfRequest) + "registration since rescan has finished, conf_id=%v", + ntfn.ConfRequest, ntfn.ConfID) // The default notification we assigned above includes the // block along with the rest of the details. However not all @@ -679,9 +679,13 @@ func (n *TxNotifier) RegisterConf(txid *chainhash.Hash, pkScript []byte, confDetails = &confDetailsCopy } - err := n.dispatchConfDetails(ntfn, confDetails) - if err != nil { - return nil, err + // Deliver the details to the whole conf set where this ntfn + // lives in. + for _, subscriber := range confSet.ntfns { + err := n.dispatchConfDetails(subscriber, confDetails) + if err != nil { + return nil, err + } } return &ConfRegistration{ @@ -912,10 +916,16 @@ func (n *TxNotifier) dispatchConfDetails( // If there are no conf details to dispatch or if the notification has // already been dispatched, then we can skip dispatching to this // client. - if details == nil || ntfn.dispatched { - Log.Debugf("Skipping dispatch of conf details(%v) for "+ - "request %v, dispatched=%v", details, ntfn.ConfRequest, - ntfn.dispatched) + if details == nil { + Log.Debugf("Skipped dispatching nil conf details for request "+ + "%v, conf_id=%v", ntfn.ConfRequest, ntfn.ConfID) + + return nil + } + + if ntfn.dispatched { + Log.Debugf("Skipped dispatched conf details for request %v "+ + "conf_id=%v", ntfn.ConfRequest, ntfn.ConfID) return nil } @@ -925,8 +935,9 @@ func (n *TxNotifier) dispatchConfDetails( // we'll dispatch a confirmation notification to the caller. confHeight := details.BlockHeight + ntfn.NumConfirmations - 1 if confHeight <= n.currentHeight { - Log.Debugf("Dispatching %v confirmation notification for %v", - ntfn.NumConfirmations, ntfn.ConfRequest) + Log.Debugf("Dispatching %v confirmation notification for "+ + "conf_id=%v, %v", ntfn.NumConfirmations, ntfn.ConfID, + ntfn.ConfRequest) // We'll send a 0 value to the Updates channel, // indicating that the transaction/output script has already @@ -944,8 +955,8 @@ func (n *TxNotifier) dispatchConfDetails( return ErrTxNotifierExiting } } else { - Log.Debugf("Queueing %v confirmation notification for %v at tip ", - ntfn.NumConfirmations, ntfn.ConfRequest) + Log.Debugf("Queueing %v confirmation notification for %v at "+ + "tip", ntfn.NumConfirmations, ntfn.ConfRequest) // Otherwise, we'll keep track of the notification // request by the height at which we should dispatch the @@ -1743,8 +1754,9 @@ func (n *TxNotifier) NotifyHeight(height uint32) error { for ntfn := range n.ntfnsByConfirmHeight[height] { confSet := n.confNotifications[ntfn.ConfRequest] - Log.Debugf("Dispatching %v confirmation notification for %v", - ntfn.NumConfirmations, ntfn.ConfRequest) + Log.Debugf("Dispatching %v confirmation notification for "+ + "conf_id=%v, %v", ntfn.NumConfirmations, ntfn.ConfID, + ntfn.ConfRequest) // The default notification we assigned above includes the // block along with the rest of the details. However not all From 0c3b2b5e067a9bda326dddb44012a3b72954df66 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 12 Nov 2024 22:40:31 +0800 Subject: [PATCH 3/5] docs: update release notes --- docs/release-notes/release-notes-0.19.0.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md index 0294fc7cc63..2c73f7aa735 100644 --- a/docs/release-notes/release-notes-0.19.0.md +++ b/docs/release-notes/release-notes-0.19.0.md @@ -54,6 +54,9 @@ * [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9275) where the peer may block the shutdown process of lnd. +* [Fixed a case](https://github.com/lightningnetwork/lnd/pull/9258) where the + confirmation notification may be missed. + # New Features ## Functional Enhancements ## RPC Additions @@ -199,6 +202,10 @@ The underlying functionality between those two options remain the same. * Oliver Gugger * Pins * Viktor Tigerström +<<<<<<< HEAD * Yong Yu * Ziggie +======= +* Ziggie +>>>>>>> 5a6264b6a (docs: update release notes) From 7695880c13e1a2a9e49bcd4e63e694196fb06d0d Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 19 Nov 2024 16:02:19 +0800 Subject: [PATCH 4/5] chainntnfs: add new method `notifyNumConfsLeft` So it's easier to handle the following commit where we start skipping duplicate notifications. --- chainntnfs/txnotifier.go | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index ee48bd65a70..f7e29486fff 100644 --- a/chainntnfs/txnotifier.go +++ b/chainntnfs/txnotifier.go @@ -942,10 +942,9 @@ func (n *TxNotifier) dispatchConfDetails( // We'll send a 0 value to the Updates channel, // indicating that the transaction/output script has already // been confirmed. - select { - case ntfn.Event.Updates <- 0: - case <-n.quit: - return ErrTxNotifierExiting + err := n.notifyNumConfsLeft(ntfn, 0) + if err != nil { + return err } select { @@ -972,10 +971,9 @@ func (n *TxNotifier) dispatchConfDetails( // confirmations are left for the transaction/output script to // be confirmed. numConfsLeft := confHeight - n.currentHeight - select { - case ntfn.Event.Updates <- numConfsLeft: - case <-n.quit: - return ErrTxNotifierExiting + err := n.notifyNumConfsLeft(ntfn, numConfsLeft) + if err != nil { + return err } } @@ -1740,10 +1738,9 @@ func (n *TxNotifier) NotifyHeight(height uint32) error { continue } - select { - case ntfn.Event.Updates <- numConfsLeft: - case <-n.quit: - return ErrTxNotifierExiting + err := n.notifyNumConfsLeft(ntfn, numConfsLeft) + if err != nil { + return err } } } @@ -2081,3 +2078,15 @@ func (n *TxNotifier) TearDown() { } } } + +// notifyNumConfsLeft sends the number of confirmations left to the +// notification subscriber through the Event.Updates channel. +func (n *TxNotifier) notifyNumConfsLeft(ntfn *ConfNtfn, num uint32) error { + select { + case ntfn.Event.Updates <- num: + case <-n.quit: + return ErrTxNotifierExiting + } + + return nil +} From db6901c9f337c511cd4a057f8c2664ab7fffd8cb Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 19 Nov 2024 16:11:13 +0800 Subject: [PATCH 5/5] chainntnfs: skip duplicate `numConfsLeft` notifications This commit adds a new state to the `ConfNtfn` struct to start tracking the number of confs left to be notified to avoid sending duplicate notifications. --- chainntnfs/txnotifier.go | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/chainntnfs/txnotifier.go b/chainntnfs/txnotifier.go index f7e29486fff..71b9c929df4 100644 --- a/chainntnfs/txnotifier.go +++ b/chainntnfs/txnotifier.go @@ -244,20 +244,25 @@ type ConfNtfn struct { // notification is to be sent. NumConfirmations uint32 - // Event contains references to the channels that the notifications are to - // be sent over. + // Event contains references to the channels that the notifications are + // to be sent over. Event *ConfirmationEvent // HeightHint is the minimum height in the chain that we expect to find // this txid. HeightHint uint32 - // dispatched is false if the confirmed notification has not been sent yet. + // dispatched is false if the confirmed notification has not been sent + // yet. dispatched bool // includeBlock is true if the dispatched notification should also have // the block included with it. includeBlock bool + + // numConfsLeft is the number of confirmations left to be sent to the + // subscriber. + numConfsLeft uint32 } // HistoricalConfDispatch parametrizes a manual rescan for a particular @@ -589,6 +594,7 @@ func (n *TxNotifier) newConfNtfn(txid *chainhash.Hash, }), HeightHint: heightHint, includeBlock: opts.includeBlock, + numConfsLeft: numConfs, }, nil } @@ -1842,6 +1848,9 @@ func (n *TxNotifier) DisconnectTip(blockHeight uint32) error { default: } + // We also reset the num of confs update. + ntfn.numConfsLeft = ntfn.NumConfirmations + // Then, we'll check if the current // transaction/output script was included in the // block currently being disconnected. If it @@ -2081,7 +2090,22 @@ func (n *TxNotifier) TearDown() { // notifyNumConfsLeft sends the number of confirmations left to the // notification subscriber through the Event.Updates channel. +// +// NOTE: must be used with the TxNotifier's lock held. func (n *TxNotifier) notifyNumConfsLeft(ntfn *ConfNtfn, num uint32) error { + // If the number left is no less than the recorded value, we can skip + // sending it as it means this same value has already been sent before. + if num >= ntfn.numConfsLeft { + Log.Debugf("Skipped dispatched update (numConfsLeft=%v) for "+ + "request %v conf_id=%v", num, ntfn.ConfRequest, + ntfn.ConfID) + + return nil + } + + // Update the number of confirmations left to the notification. + ntfn.numConfsLeft = num + select { case ntfn.Event.Updates <- num: case <-n.quit: