chanevents: implement GetChannelEvents rpc#239
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 introduces a new RPC endpoint, GetChannelEvents, which allows users to retrieve a detailed history of channel lifecycle events such as online/offline status changes and balance updates. The changes include the necessary database query updates, RPC service definitions, CLI command implementation, and a comprehensive integration test to ensure the functionality works as expected. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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 GetChannelEvents RPC endpoint and a corresponding chanevents CLI command to retrieve lifecycle events for a specific Lightning channel. The implementation includes database updates for pagination support, server-side validation, and integration tests. Feedback highlights a potential issue with the pagination strategy where second-precision timestamps and inclusive queries could lead to duplicate events or infinite loops. Additionally, a contradiction in the integration tests was noted regarding exact event count assertions.
bitromortac
force-pushed
the
2604-fwd-prep-9
branch
from
c37ae7b to
8513ccc
Compare
|
@bitromortac, remember to re-request review from reviewers when ready |
bitromortac
force-pushed
the
2604-fwd-prep-9
branch
3 times, most recently
from
5fe1fb4 to
d7a9e4e
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Generally this PR looks great, so mainly leaving a question 🔥.
Additionally, you should add details to commit messages before this is merged.
| WHERE channel_id = $1 AND timestamp >= $2 AND timestamp < $3 | ||
| ORDER BY timestamp ASC, id ASC; | ||
| WHERE channel_id = $1 | ||
| AND id > $2 |
There was a problem hiding this comment.
In general this PR looks great, so I think this is my main comment for the PR:
Is it for performance reasons or that you think it's a better UI that you use the lastId approach over using an offset (i.e. something like LIMIT $5 OFFSET $6)? The approach choosen effects the rest of the PR.
I think the offset approach is much more common, and is what we use in litd for actions and I think potentially more natural for a user at it leaks no internals of how the ChannelEvent is stored (i.e. the id), and is also independent of the store used (i.e. not requiring that an incremental id is used).
Would therefore be interesting to hear your motivation behind this approach, which I do see some pros with as well!
There was a problem hiding this comment.
Good point. Previously I had time-based pagination, but that didn't work well because of timestamp precision. I didn't really consider offset pagination, but a good point that this would be more consistent with other projects and doesn't expose internal state. However, I see that in the future we may want to delete older events, because I expect a constant stream of updates. In a new terminal web feature I think we also want to sync balances. With deletion I guess that it's easier to maintain the sync state by using the id. Let me know if you prefer the offset way, though I think the id approach has also speed advantages.
There was a problem hiding this comment.
Ok, ultimately I'll let you decide on which approach you'd want to use!
Personally I have no strong preference. I do think the offset approach is usually standard practise though, even when deletion is possible. I think the main drawback with the current approach is that it'll bind the store to always use an incremental id for the rows in the db.
But ultimately I think there are pros with the current approach as well, so I'll let you decide :)
There was a problem hiding this comment.
Ok, I thought about it a bit more, I'd accept the small implementation leak for the performance and for incremental sync that survives head deletions (which I think isn't possible in a straightforward manner with an offset unless one introduces an additional absolute sync watermark value like a timestamp). Thanks for the input here!
Logs every AddChannelEvent at trace level so operators can confirm which lnd events are being persisted.
Switches the GetChannelEvents query from a timestamp-ordered scan to an id-keyset cursor (WHERE id > $cursor ORDER BY id ASC LIMIT $n). The keyset cursor is stable under concurrent inserts and survives a future retention job that prunes the oldest rows: a positional OFFSET would silently skip events whenever rows below the cursor are deleted, while "id > $cursor" keeps advancing past whatever the caller has already seen. The id field is documented in the proto as a server-assigned monotonic identity, so callers persist last_id as their sync watermark. Adds a (channel_id, id) composite index to back the new query; the existing (channel_id, timestamp) index does not cover it and would force a per-channel filter after a global id scan.
Adds the GetChannelEvents RPC to the proto and regenerates the gRPC, gateway, swagger, and JSON stubs. The request carries a chan_point, inclusive start_time and exclusive end_time bounds, a max_events cap, and a last_id keyset cursor; the response echoes last_id and a has_more flag so callers can drive pagination without server-side state.
Threads the chanevents.Store the daemon already constructs into the frdrpcserver.Config so the upcoming GetChannelEvents handler has a read path. Both standalone and subserver startup wire the same store instance.
Implements the handler against the chanevents store. A zero end_time defaults to the server's current wall clock so callers can omit it for "up to now" queries. max_events is clamped to a 10000-row hard cap that also serves as the implicit default when the caller leaves the field at zero. An unknown chan_point maps to NotFound; negative time bounds and start_time after end_time map to InvalidArgument. The response sets has_more whenever the page filled to the requested limit so the client knows to keep paginating. Also registers the new endpoint in the macaroon permissions table under the channels:read entitlement.
Drives a regtest channel through open, payment, and force-close and asserts that GetChannelEvents surfaces the expected mix of online, offline, and balance-update events. The same window is then walked with a small page size to verify last_id round-trip, has_more termination, and MaxEvents clamping in one pass.
Adds a `chanevents` subcommand that wraps GetChannelEvents and exposes chan_point, start/end time, max_events, and last_id flags. The help text documents the manual pagination contract: keep re-running with --last_id set to the previous response's last_id until has_more is false, while leaving --start_time and --end_time fixed across calls.
bitromortac
force-pushed
the
2604-fwd-prep-9
branch
from
d7a9e4e to
f118640
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks for the update and responding to my previous questions. I'm good to merge this as is if you prefer the current approach. uTACK LGTM 🔥
| WHERE channel_id = $1 AND timestamp >= $2 AND timestamp < $3 | ||
| ORDER BY timestamp ASC, id ASC; | ||
| WHERE channel_id = $1 | ||
| AND id > $2 |
There was a problem hiding this comment.
Ok, ultimately I'll let you decide on which approach you'd want to use!
Personally I have no strong preference. I do think the offset approach is usually standard practise though, even when deletion is possible. I think the main drawback with the current approach is that it'll bind the store to always use an incremental id for the rows in the db.
But ultimately I think there are pros with the current approach as well, so I'll let you decide :)
bitromortac
merged commit a568dfe
into
lightninglabs:faraday-forwarding-ability
4 participants
Exposes recorded channel events via RPC and adds an itest to test correct channel update recording.