chanstate: migrate consumers to Store interface (PR 2 of channel-state decomposition)#10790
Conversation
ziggie1984
force-pushed
the
chanstate-sql-migration-pr2-consumers
branch
from
a468106 to
014cbfc
Compare
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 is the second installment in the channel-state decomposition series. It establishes a new persistence contract via the chanstate.Store interface and migrates all existing consumers of the concrete channeldb.ChannelStateDB to this abstraction. This change is purely architectural, ensuring that future work to swap the underlying storage backend (e.g., from KV to SQL) can be performed without requiring modifications to the various subsystems that depend on channel state. 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
|
ziggie1984
force-pushed
the
chanstate-sql-migration-pr2-consumers
branch
from
014cbfc to
a33d504
Compare
![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 the chanstate.Store interface to decouple the channel-state subsystem from the concrete channeldb.ChannelStateDB implementation, facilitating a future SQL migration. It updates several components—including the server, funding manager, and various RPC services—to use this new interface. The feedback focuses on improving the consistency of parameter naming within the new interface to align with established LND conventions (e.g., using chanID for lnwire.ChannelID and chanPoint for wire.OutPoint) and notes a placeholder in the release notes that needs to be updated.
| // FetchChannelByID attempts to locate a channel specified by the | ||
| // passed channel ID. If the channel cannot be found, then an error | ||
| // will be returned. | ||
| FetchChannelByID(id lnwire.ChannelID) (*channeldb.OpenChannel, error) |
There was a problem hiding this comment.
For consistency with other methods in this interface (e.g., SaveInitialForwardingPolicy) and LND conventions, the lnwire.ChannelID parameter should be named chanID instead of id.
| FetchChannelByID(id lnwire.ChannelID) (*channeldb.OpenChannel, error) | |
| FetchChannelByID(chanID lnwire.ChannelID) (*channeldb.OpenChannel, error) |
|
|
||
| // FetchHistoricalChannel fetches open channel data from the | ||
| // historical channel bucket. | ||
| FetchHistoricalChannel(outPoint *wire.OutPoint) ( |
There was a problem hiding this comment.
The parameter name outPoint should be renamed to chanPoint to be consistent with FetchChannel and other methods in this interface that use wire.OutPoint.
| FetchHistoricalChannel(outPoint *wire.OutPoint) ( | |
| FetchHistoricalChannel(chanPoint *wire.OutPoint) ( |
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟠 High (3 files)
🟡 Medium (5 files)
🟢 Low (1 file)
AnalysisThis PR introduces a new The bulk of the additions (~238 lines) are in the new To override, add a |
|
Can target master now? Or rather the 1st dep branch. |
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM 🧜🏼
Straight forward PR, only block dep is on the 1st in the series.
ziggie1984
changed the base branch from
elle-base-branch-channelstate-native-sql
to
master
ziggie1984
force-pushed
the
chanstate-sql-migration-pr2-consumers
branch
from
a33d504 to
0c0abff
Compare
github-actions
Bot
added
severity-critical
ziggie1984
force-pushed
the
chanstate-sql-migration-pr2-consumers
branch
2 times, most recently
from
10e1d2f to
599ec09
Compare
Replace the funding manager's concrete *channeldb.ChannelStateDB dependency with chanstate.Store. The manager only uses methods covered by the store contract, including channel opening state and initial forwarding policy persistence.
Replace the peer config's concrete channel state DB dependency with chanstate.Store. Brontide only needs channel lookups, closed-channel lookup, and initial forwarding policy access from the channel-state store.
Accept chanstate.Store in ChannelNotifier instead of the concrete ChannelStateDB. The notifier only fetches open and closed channel records to populate channel event payloads.
Replace BreachConfig's concrete ChannelStateDB dependency with chanstate.Store. The breach arbitrator only needs closed-channel reads and MarkChanFullyClosed from the channel-state store.
ziggie1984
force-pushed
the
chanstate-sql-migration-pr2-consumers
branch
from
599ec09 to
6659bde
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Need to fix the CI first.
Make chanDBRestorer persist restored channel shells through chanstate.Store instead of the concrete ChannelStateDB. The restorer still builds channeldb channel shell values, but only needs RestoreChannelShells from the store.
Replace concrete ChannelStateDB fields in the invoices and wallet RPC configs with chanstate.Store, and update the subserver dependency wiring to pass the interface. The affected RPC paths only need channel-state store methods for hop hints and waiting-close channel queries.
Store channel-state access on server as chanstate.Store instead of *channeldb.ChannelStateDB. Keep link-node access as a separate concrete *channeldb.LinkNodeDB field so LinkNodeDB does not leak into the channel-state store contract.
ziggie1984
force-pushed
the
chanstate-sql-migration-pr2-consumers
branch
from
6659bde to
0376270
Compare
Addressed. |
|
|
||
| chanStateDB *channeldb.ChannelStateDB | ||
| chanStateDB chanstate.Store | ||
| linkNodeDB *channeldb.LinkNodeDB |
There was a problem hiding this comment.
nits: since this only needs FetchAllLinkNodes, could this be a small private interface instead of storing the concrete *channeldb.LinkNodeDB? That keeps LinkNodeDB out of chanstate.Store while avoiding another concrete field on server.
| ChannelDB *channeldb.ChannelStateDB | ||
| // ChannelDB is the database that keeps track of channel state used by | ||
| // the funding flow. | ||
| ChannelDB chanstate.Store |
There was a problem hiding this comment.
nits: this only appears to use open-channel reads plus channel setup methods. Could this field use a narrower private interface embedding chanstate.OpenChannelStore and chanstate.ChannelSetupStore instead of the full chanstate.Store?
| // ChannelDB is used to fetch opened channels, and closed channels. | ||
| ChannelDB *channeldb.ChannelStateDB | ||
| // ChannelDB is used to fetch channel state needed by the peer. | ||
| ChannelDB chanstate.Store |
There was a problem hiding this comment.
nits: this only needs open-channel reads, closed-channel lookup, and initial forwarding policy access. Could this be narrowed to a small private interface rather than full chanstate.Store?
| ntfnServer *subscribe.Server | ||
|
|
||
| chanDB *channeldb.ChannelStateDB | ||
| chanDB chanstate.Store |
There was a problem hiding this comment.
nits: the notifier only calls FetchChannel and FetchClosedChannel, so a narrower private interface would preserve the benefit of the sub-interface split without requiring the full chanstate.Store.
4 participants
Summary
Second PR in the channel-state decomposition series. Migrates every
in-tree consumer of
*channeldb.ChannelStateDBover to thechanstate.Storeinterface introduced in PR 1, so subsequent PRs can swap in the
KV-backed and SQL-backed
Storeimplementations without further churnto call sites.
No data moves and no behavior changes — every commit is a pure
dependency-injection swap. The interface contract was sized in PR 1
exactly to cover the union of method calls touched by these consumers,
so each commit compiles against
Storewithout expanding theinterface.
Stacked on top of #10777. Until that lands, this PR's diff includes
its 4 commits; once 10777 merges into
elle-base-branch-channelstate-native-sql, the diff here collapses tothe 8 commits below.
Seven consumer commits, one release-note commit:
funding: depend on chanstate Store— funding manager only usesmethods covered by the store contract, including channel-opening
state and initial forwarding policy persistence.
peer: depend on chanstate Store— Brontide only needs channellookups, closed-channel lookup, and initial forwarding policy
access.
channelnotifier: depend on chanstate Store— notifier only fetchesopen and closed channel records to populate event payloads.
contractcourt: use chanstate Store in breach arb— breacharbitrator only needs closed-channel reads and
MarkChanFullyClosed.lnd: use chanstate Store for channel restore—chanDBRestoreronly needs
RestoreChannelShells; it still buildschanneldbchannel-shell values.
lnrpc: depend on chanstate Store— invoices/wallet RPC configsand subserver wiring; only hop-hint and waiting-close paths are
touched.
server: depend on chanstate Store— server now holds channel-stateaccess as
chanstate.Store. Link-node access stays a separateconcrete
*channeldb.LinkNodeDBfield soLinkNodeDBdoes not leakinto the channel-state contract.
docs: release note for chanstate Store consumer migration—sibling bullet under the existing v0.22 "Native SQL migration of
the channel state and switch data" heading.
Test plan
go build ./...cleanmake unitpasses for affected packages (funding,peer,channelnotifier,contractcourt,lnrpc,chanstate)make lintcleanchanstate(added in chanstate: introduce Store interface (PR 1 of channel-state decomposition) #10777) stillcatches any signature drift between
Storeand*channeldb.ChannelStateDB