chainntnfs: add a pkscript notifier stream#10807
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a robust, stream-based interface for monitoring Bitcoin script activity. By moving away from single-target notifications to a script-based subscription model, it enables more efficient and flexible chain monitoring. The changes include backend support for bitcoind, btcd, and neutrino, along with a comprehensive gRPC API for external clients to leverage these new capabilities. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new PkScriptNotifier interface and implementation, allowing clients to watch for confirmation and spend events on specific output scripts. The changes include support for historical scans, resource limiting, and a new bidirectional gRPC stream in chainrpc. The reviewer suggested refactoring the duplicated historical dispatch logic across the bitcoind, btcd, and neutrino backends into a shared helper and noted that an error in completeHistoricalPkScriptDispatchLocked should be explicitly handled.
| // queueHistoricalPkScriptDispatch reserves capacity and queues a historical | ||
| // pkScript scan for the bitcoind backend. | ||
| func (b *BitcoindNotifier) queueHistoricalPkScriptDispatch( | ||
| msg *chainntnfs.HistoricalPkScriptDispatch) error { | ||
|
|
||
| if msg == nil { | ||
| return nil | ||
| } | ||
|
|
||
| err := b.reserveHistoricalPkScriptDispatchSlot() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return b.queueReservedHistoricalPkScriptDispatch(msg) | ||
| } | ||
|
|
||
| // reserveHistoricalPkScriptDispatchSlot reserves one pending historical pkScript | ||
| // scan slot. | ||
| func (b *BitcoindNotifier) reserveHistoricalPkScriptDispatchSlot() error { | ||
| select { | ||
| case <-b.quit: | ||
| return chainntnfs.ErrChainNotifierShuttingDown | ||
| default: | ||
| } | ||
|
|
||
| select { | ||
| case b.historicalPkScriptDispatchSlots <- struct{}{}: | ||
| return nil | ||
|
|
||
| case <-b.quit: | ||
| return chainntnfs.ErrChainNotifierShuttingDown | ||
|
|
||
| default: | ||
| return fmt.Errorf("%w: pending scans %d exceeds limit %d", | ||
| chainntnfs.ErrTooManyHistoricalPkScriptScans, | ||
| len(b.historicalPkScriptDispatchSlots), | ||
| chainntnfs.MaxPkScriptHistoricalDispatchQueueSize) | ||
| } | ||
| } | ||
|
|
||
| // releaseHistoricalPkScriptDispatchSlot releases one pending historical pkScript | ||
| // scan slot. | ||
| func (b *BitcoindNotifier) releaseHistoricalPkScriptDispatchSlot() { | ||
| select { | ||
| case <-b.historicalPkScriptDispatchSlots: | ||
| default: | ||
| } | ||
| } | ||
|
|
||
| // queueReservedHistoricalPkScriptDispatch queues a dispatch after capacity has | ||
| // already been reserved. | ||
| func (b *BitcoindNotifier) queueReservedHistoricalPkScriptDispatch( | ||
| msg *chainntnfs.HistoricalPkScriptDispatch) error { | ||
|
|
||
| select { | ||
| case b.historicalPkScriptDispatches <- msg: | ||
| return nil | ||
|
|
||
| case <-b.quit: | ||
| b.releaseHistoricalPkScriptDispatchSlot() | ||
|
|
||
| return chainntnfs.ErrChainNotifierShuttingDown | ||
| } | ||
| } | ||
|
|
||
| // historicalPkScriptDispatcher serially executes queued historical pkScript | ||
| // scans. | ||
| func (b *BitcoindNotifier) historicalPkScriptDispatcher() { | ||
| defer b.wg.Done() | ||
|
|
||
| for { | ||
| select { | ||
| case msg := <-b.historicalPkScriptDispatches: | ||
| err := b.historicalPkScriptDispatch(msg) | ||
| b.releaseHistoricalPkScriptDispatchSlot() | ||
| if err != nil { | ||
| chainntnfs.Log.Errorf("Historical pkScript dispatch "+ | ||
| "for sub=%d within range %d-%d failed: %v", | ||
| msg.SubscriptionID, msg.StartHeight, | ||
| msg.EndHeight, err) | ||
| } | ||
|
|
||
| case <-b.quit: | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication for the historical pkScript dispatch logic across bitcoindnotify, btcdnotify, and neutrinonotify backends. The functions queueHistoricalPkScriptDispatch, reserveHistoricalPkScriptDispatchSlot, releaseHistoricalPkScriptDispatchSlot, queueReservedHistoricalPkScriptDispatch, and historicalPkScriptDispatcher are nearly identical in all three files.
Consider refactoring this logic into a shared helper struct that can be embedded by each notifier. This would improve maintainability and reduce redundancy.
| _ = n.completeHistoricalPkScriptDispatchLocked( | ||
| sub, dispatch, completedHeight, err, | ||
| ) |
There was a problem hiding this comment.
The error returned by completeHistoricalPkScriptDispatchLocked is being ignored. This function can return an error if sending the notification fails, and ignoring it could mask problems. The error should be handled, for example by logging it.
| _ = n.completeHistoricalPkScriptDispatchLocked( | |
| sub, dispatch, completedHeight, err, | |
| ) | |
| if complErr := n.completeHistoricalPkScriptDispatchLocked( | |
| sub, dispatch, completedHeight, err, | |
| ); complErr != nil { | |
| Log.Errorf("Unable to send historical pkScript dispatch completion for sub=%d: %v", dispatch.SubscriptionID, complErr) | |
| } |
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🟠 High (9 files — base severity)
🟡 Medium (5 files)
🟢 Low (2 files — excluded from count)
⬛ Excluded from counting (test/auto-generated files)
AnalysisBase severity: HIGH — The PR touches Bumped to CRITICAL — The PR has approximately 4,938 lines changed in non-test, non-auto-generated files, well exceeding the 500-line threshold that triggers a one-level severity bump. The core of this PR introduces a new To override, add a |
github-actions
Bot
added
severity-critical
Add a TxNotifier-backed stream for watching pkScripts with confirmation, spend, partial confirmation, historical scan, and reorg notifications.
Support pkScript streams in bitcoind, btcd, and neutrino, including historical scan dispatch and neutrino compact-filter updates.
Expose RegisterPkScriptNtfn with register/add/remove mutations, acknowledgements, historical scan events, and pkScript notification conversion.
Add shared notifier tests and RPC integration scenarios for historical scans, removals, confirmation modes, multi-stream watching, validation, and reorg handling.
Document RegisterPkScriptNtfn capabilities, historical scan semantics, notification behavior, resource bounds, and backend support.
Show how to register a pkScript notification stream, defer cancellation, add scripts with all notifier options, and handle stream events.
Show how clients should apply Disconnected pkScript notifications for confirmation updates, final confirmations, and spends.
Mention the new RegisterPkScriptNtfn stream and supporting documentation in the 0.22.0 release notes.
1 participant
This PR introduces
chainntnfs.PkScriptNotifier, an interface that allows for flexible listening of confirms and spends over a long-lived stream.Interfaces
Examples
13819bd