chanstate: introduce Store interface (PR 1 of channel-state decomposition)#10777
chanstate: introduce Store interface (PR 1 of channel-state decomposition)#10777ziggie1984 wants to merge 4 commits intolightningnetwork:masterfrom
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 initiates the decomposition of the channel-state management logic into a dedicated subsystem. By introducing the 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 deferred cleanup mechanism for closed channels on KV-over-SQL backends (SQLite and Postgres) to prevent long write-lock stalls during synchronous channel closure. The implementation splits the closure into two phases: Phase 1 atomically marks the channel as closed and removes it from open views, while Phase 2 defers the deletion of bulky historical data (revocation logs and forwarding packages) to the next startup. The PR also introduces a ChannelStore interface to abstract the persistence layer. Feedback focuses on improving the robustness of the cleanup queue by handling malformed records and ensuring that individual purge failures do not block the entire node startup process, as well as minor code simplifications.
| return cleanupBkt.ForEach(func(k, v []byte) error { | ||
| var op wire.OutPoint | ||
| if err := graphdb.ReadOutpoint( | ||
| bytes.NewReader(k), &op, | ||
| ); err != nil { | ||
| log.Warnf("Skipping pending closed-channel "+ | ||
| "cleanup with malformed "+ | ||
| "chan_key=%x: %v", k, err) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| rec, err := decodePendingCleanupRecord(v) | ||
| if err != nil { | ||
| log.Warnf("Skipping pending closed-channel "+ | ||
| "cleanup for %v: unable to decode "+ | ||
| "record: %v", op, err) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| entries = append(entries, PendingCleanup{ | ||
| ChanPoint: op, | ||
| Record: rec, | ||
| }) | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
Malformed cleanup records are skipped and logged but remain in the pendingChanCleanupBucket. This results in persistent log noise on every startup. Consider collecting the keys of malformed records and deleting them in a separate transaction to keep the cleanup queue healthy.
ziggie1984
force-pushed
the
chanstate-sql-migration-pr1-interface
branch
from
577cb0e to
a91f13f
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟡 Medium (2 files)
AnalysisThis PR is classified CRITICAL due to changes in two independently critical areas:
The new No severity bump triggers were met (4 non-test files, 195 lines changed). To override, add a |
ziggie1984
changed the base branch from
master
to
elle-base-branch-channelstate-native-sql
All four call sites pass nil for tx today (server.go, two in channeldb/db_test.go, funding/manager_test.go). The internal channelScanner(nil, selector) call inside FetchChannelByID is preserved verbatim, so runtime behavior is unchanged. This is a prerequisite for the upcoming chanstate.ChannelStore interface: keeping the parameter would leak kvdb into a domain interface.
ziggie1984
force-pushed
the
chanstate-sql-migration-pr1-interface
branch
from
a91f13f to
9c66646
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ChannelStore interface within a new chanstate package, serving as a persistence contract to decouple the channel-state subsystem from the concrete channeldb implementation. As part of this refactoring, the FetchChannelByID method signature was simplified by removing the optional database transaction parameter, with corresponding updates made across tests and the main server logic. Regarding the review feedback, a style guide violation was identified in chanstate/log.go where the init function requires a comment explaining its purpose.
| func init() { | ||
| UseLogger(build.NewSubLogger("CHST", nil)) | ||
| } |
There was a problem hiding this comment.
According to the LND Style Guide (line 17), every function must have a comment explaining its purpose. The init function is missing such a comment.
| func init() { | |
| UseLogger(build.NewSubLogger("CHST", nil)) | |
| } | |
| // init initializes the package-global logger instance. | |
| func init() { | |
| UseLogger(build.NewSubLogger("CHST", nil)) | |
| } |
References
- Every function must be commented with its purpose and assumptions. (link)
Add a new chanstate package containing the Store interface plus a
package logger. The interface mirrors the public surface of
*channeldb.ChannelStateDB so the compile-time assertion
var _ ChannelStore = (*channeldb.ChannelStateDB)(nil)
No consumer migrates in this commit.
Introduce InitialForwardingPolicyStore as a separate sub-interface
covering the per-channel initial forwarding policy that the funding
manager records at channel-open time and the link consumes on
activation. The sub-interface is embedded into Store so the existing
compile-time assertion
var _ Store = (*channeldb.ChannelStateDB)(nil)
continues to enforce that *channeldb.ChannelStateDB satisfies the full
contract.
No consumer migrates in this commit.
Seeds the v0.22 "Native SQL migration of the channel state and switch data" series under the Database header so subsequent decomposition PRs (consumer migration, KV/SQL stores, revocation log, forwarding package, etc.) can hang off the same heading as nested bullets, mirroring the v0.21 payments series shape.
ziggie1984
force-pushed
the
chanstate-sql-migration-pr1-interface
branch
from
9c66646 to
299e8a8
Compare
github-actions
Bot
added
severity-critical
ziggie1984
changed the title
Add a sibling bullet under the existing "Native SQL migration of the channel state and switch data" heading, covering the consumer-side swap that pairs with the interface introduced in lightningnetwork#10777.
Add a sibling bullet under the existing "Native SQL migration of the channel state and switch data" heading, covering the consumer-side swap that pairs with the interface introduced in lightningnetwork#10777.
ziggie1984
changed the base branch from
elle-base-branch-channelstate-native-sql
to
master
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Straight forward diff. Main comment is that I think we can avoid the mega channel store interface in favor of more domain specific interfaces.
| // ChannelStore is the persistence contract for core channel-state operations. | ||
| // | ||
| //nolint:interfacebloat | ||
| type ChannelStore interface { |
There was a problem hiding this comment.
Perhaps we can split this up into some more domain specific types, I can see the following splits here:
- Open channels
- Closed channels
- Channel close mutations
- Link nodes
- Alias management
- SCB
Then that way all the relevant call sites can take in slimmer interfaces.
3 participants
Summary
First PR in the channel-state decomposition series. Introduces the
chanstate.Storeinterface as the seam between the channel-state subsystemand its consumers, mirroring the pattern in
invoices/,payments/db/, andgraph/db/. No consumer migrates, no data moves, no behavior changes.Storeis composed of two embedded sub-interfaces —ChannelStorefor corechannel-state operations and
InitialForwardingPolicyStorefor theper-channel forwarding policy chosen at channel-open time — so future
decomposition PRs can extend the contract without inflating a single
monolithic interface.
Four commits:
channeldb: drop unused kvdb.RTx parameter from FetchChannelByID—prerequisite so the interface stays free of
kvdbtypes. All four callsites already pass
nil.chanstate: introduce Store interface— new package withStoreand itsChannelStoresub-interface, a package logger, and the compile-timeassertion
var _ Store = (*channeldb.ChannelStateDB)(nil)that guardsagainst signature drift.
GetParentDB(test-only) andLinkNodeDB(separate domain) are intentionally excluded.
chanstate: add InitialForwardingPolicyStore sub-interface— adds thesecond sub-interface for the initial-forwarding-policy methods, embedded
into
Store.docs: release note for chanstate Store interface— seeds the v0.22"Native SQL migration of the channel state and switch data" series so
follow-up PRs (consumer swap, KV/SQL stores, revocation log, forwarding
package, etc.) hang off the same heading.
Test plan
go build ./...cleanmake unit pkg=channeldbandmake lintpass