rpcserver: filter backup subscription events#10821
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 refines the event subscription logic for channel backups to reduce unnecessary notifications. By switching from a deny-list to an explicit allow-list of lifecycle events, the system avoids triggering backup snapshots on every commitment update, ensuring the stream remains focused on relevant channel lifecycle changes. 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
|
There was a problem hiding this comment.
Code Review
This pull request updates the documentation for channel event subscriptions to warn about high-frequency updates and refactors the backup subscription logic to use an allow-list for lifecycle events. Feedback was provided regarding a potential infinite loop in the event processing logic; if the event channel closes, the new default case would continuously execute, so an explicit check for nil values is required to terminate the loop.
755687b to
82c2e24
Compare
|
PR Severity: CRITICAL. rpcserver.go is explicitly CRITICAL (core server coordination). channelnotifier/channelnotifier.go is MEDIUM. Highest severity wins. 2 files, 31 lines - no severity bump applied. <!-- pr-severity-bot --> |
82c2e24 to
953f7d2
Compare
953f7d2 to
8adddd7
Compare
|
A note on test coverage: I considered adding a dedicated unit test for the event filter but decided against it. The original regression came from a new event type being added upstream in Two things in this PR address that more directly:
A reflection-based test enumerating all |
8adddd7 to
a38d589
Compare
Channel update notifications now flow through ChannelNotifier, so the existing backup subscription deny-list started treating commitment updates as backup-relevant events. This makes SubscribeChannelBackups emit on every channel update, even though those updates can happen much more frequently than lifecycle changes. Switch the backup subscription to an allow-list of lifecycle events that should trigger the stream. Also document that ChannelNotifier includes high-frequency state updates, so lifecycle-only consumers should filter explicitly.
a38d589 to
57b912c
Compare
|
Seems like we could enhance the backup mechanism a bit by also writing a backup once we update the commitment transaction. Otherwise the close transaction is outdated for sure, but not sure about the write frequency tradeoff. |
|
Successfully created backport PR for |
…21.x-branch [v0.21.x-branch] Backport #10821: rpcserver: filter backup subscription events
ChannelUpdate events where introduced in #10543
Change Description
Channel update notifications now flow through
ChannelNotifier, so theexisting
SubscribeChannelBackupsdeny-list started treating commitmentupdates as backup-relevant events. That makes the backup stream emit on every
channel update, which is much noisier than the lifecycle events this stream
historically tracked.
This PR switches
SubscribeChannelBackupsto an allow-list of lifecycle eventsthat should trigger a backup snapshot. It also documents that
ChannelNotifierincludes higher-frequency channel state updates, so lifecycle-only consumers
should explicitly filter for the event types they consume.
Steps to Test