chanstate: use open channel from consumers#10809
Open
ziggie1984 wants to merge 55 commits into
Open
Conversation
Move the small value types referenced by chanstate.Store out of channeldb. This includes ChannelConfig, ChannelStatus, ChannelCloseSummary, ChannelShell, ChanCount, and FinalHtlcInfo. Leave aliases in channeldb so existing callers keep compiling while the backend still lives there. Parameterize the Store facets over the channel type and instantiate current callers with *channeldb.OpenChannel. This removes the chanstate -> channeldb import edge without moving OpenChannel yet, keeping the first step reviewable and backend-neutral.
Move ChannelType and its flag helpers into chanstate while leaving compatibility aliases in channeldb. This is a backend-neutral value type and does not require moving any KV serialization logic. Keep the full type documentation with the moved chanstate definition. The channeldb aliases preserve the existing public surface while later commits continue moving OpenChannel state out of the KV package.
Move the OpenChannel error definitions into chanstate and leave channeldb aliases for existing callers. These errors describe channel state behavior rather than a concrete KV bucket layout. Keeping the aliases preserves the public channeldb API while later commits move more OpenChannel state and receiver logic toward chanstate.
Move the ShutdownInfo state type, constructor, and closer helper into chanstate. The type describes channel shutdown state and is not tied to the concrete KV backend. Keep the TLV encode and decode helpers in channeldb for now, since those functions describe the current persisted format. The channeldb constructor remains as a compatibility wrapper.
Add a lifecycle facet to the chanstate Store contract for refresh, confirmation, open-state, and SCID mutations. Implement the facet on ChannelStateDB using the existing KV persistence code. Update the matching OpenChannel receivers to call through the store methods instead of reaching into the ChannelStateDB backend directly. Also convert fullSync into a channeldb helper so that KV-specific code is no longer an OpenChannel receiver.
Add a status facet to the chanstate Store contract for status bit updates and data-loss commit point handling. Implement the facet on ChannelStateDB using the existing persistence code. Update the matching OpenChannel receivers to call through the store methods. The broadcast path still uses a private channeldb helper until its closing-transaction facet is introduced in a later commit.
Add shutdown and close-transaction facets to the chanstate Store contract. These cover persisted shutdown info plus stored unilateral and cooperative closing transactions. Implement the facets on ChannelStateDB with the existing KV code and update OpenChannel receivers to call through the store methods. The backend-specific key selection remains private to channeldb.
Add pending-channel setup to the chanstate lifecycle store facet. This covers the path that writes a new pending channel and records the funding broadcast height. Move the OpenChannel receiver to call through ChannelStateDB and pass the backend explicitly into the channeldb sync helper. This keeps the link-node persistence detail in channeldb while removing another direct backend reference from OpenChannel.
Move ChannelCommitment and HTLC into chanstate so upcoming store facets can name commitment state without importing channeldb. Leave the KV serialization helpers in channeldb and keep aliases for existing call sites. This preserves the current disk format and keeps backend-specific persistence code out of chanstate for now.
Move LogUpdate into chanstate so commitment store interfaces can refer to pending update state without importing channeldb. Keep the log-update serialization helpers in channeldb. Those helpers remain part of the existing KV disk format and can move with the KV backend implementation later.
Add a commitment-focused store facet for updating local channel commitment state. This lets OpenChannel call through the chanstate store contract instead of reaching directly into the KV backend. Keep the existing KV transaction body on ChannelStateDB for now. The receiver still owns locking and in-memory state updates while the store method owns persistence.
Move CommitDiff and its forwarding reference types into chanstate. This lets the next commitment store facet name pending remote commitment state without importing channeldb. Keep forwarding package persistence and commit-diff serialization in channeldb for now. The aliases preserve existing call sites while the KV backend code remains in place.
Add the remote commitment-chain append method to the chanstate commitment store facet. Move the existing KV transaction body onto ChannelStateDB and have the OpenChannel receiver call through the store. This removes another direct backend dependency from OpenChannel while keeping KV persistence code in channeldb.
Add read-side commitment lookup methods to the chanstate commitment store facet. Move the existing OpenChannel KV view transaction bodies onto ChannelStateDB. Leave the OpenChannel receivers as store-call wrappers. This removes three more direct backend references from the receiver code without changing the persisted data format.
Add the next-revocation persistence method to the chanstate commitment store facet. Move the existing OpenChannel KV update body onto ChannelStateDB. The OpenChannel receiver keeps the external locking behavior and delegates persistence through the store interface.
Move FwdState, PkgFilter, and FwdPkg into chanstate with their existing comments and helper methods. Leave channeldb aliases for the moved value types and constructors so current callers keep compiling. The KV forwarding package persistence code stays in channeldb.
Add the commitment-tail advancement method to the chanstate commitment store facet. Move the existing AdvanceCommitChainTail KV transaction body onto ChannelStateDB. The OpenChannel receiver now keeps locking and restored channel checks before delegating persistence through the store.
Add a forwarding-package store facet to chanstate.Store. Move the existing OpenChannel forwarding-package KV transaction bodies onto ChannelStateDB. The OpenChannel receivers keep their locking behavior and delegate package loading, acking, filtering, and removal through the store.
Add commitment-height, latest-commitment, and remote revocation store lookups to the chanstate commitment store facet. Move the existing OpenChannel KV view transaction bodies onto ChannelStateDB. This leaves the receivers as store-call wrappers while keeping the persisted format and read behavior unchanged.
Move the remaining OpenChannel revocation-log KV reads onto ChannelStateDB. This keeps FindPreviousState and the unit-test tail-height helper as OpenChannel wrappers. It removes direct backend access from the receiver methods while leaving RevocationLog in channeldb for now.
Move the revocation-log value types and TLV serialization helpers into chanstate. Leave channeldb aliases and wrapper functions for the existing KV persistence code and tests. Bucket keys, errors, and transaction helpers stay in channeldb, so this commit only moves backend-neutral state data.
Add FindPreviousState to the chanstate commitment store facet now that RevocationLog is a chanstate value type. This extends the store contract without changing runtime behavior. The existing ChannelStateDB method already satisfies the new method.
Keep the revocation-log tail-height helper on ChannelStateDB instead of the OpenChannel receiver. The helper is only used by channeldb tests, so it should not become part of the backend-independent chanstate store contract. The tests now call the concrete helper directly.
Change OpenChannel.Db to the composed chanstate Store interface while keeping the existing field name. Tests that need raw channeldb access now assert the concrete test backend explicitly instead of reaching through OpenChannel.Db. This keeps backend setup out of the store contract.
Convert the KV-only OpenChannel helpers for TLV aux data and borked-state lookup into package-level channeldb helpers. This keeps serialization and bucket inspection code tied to the KV backend while leaving the OpenChannel receiver set closer to the future chanstate type.
Add transitional OpenChannel accessors for the channel status and confirmed SCID fields used by KV store code. These helpers keep the fields private while allowing channeldb backend code to continue hydrating and serializing channel state after OpenChannel moves to chanstate.
Remove the KV forwarding packager from OpenChannel and derive a ChannelPackager inside the channeldb store methods that need one. This keeps the backend-specific kvdb transaction helper in channeldb, so the OpenChannel type no longer carries that dependency toward chanstate.
Move the backend-neutral ChannelSnapshot value type into chanstate and leave channeldb with a compatibility alias. This keeps the future OpenChannel Snapshot receiver close to its return type without changing existing channeldb callers.
Move the backend-neutral taproot shachain and verification nonce helpers into chanstate with the thaw-height threshold they support. Leave channeldb aliases for existing callers while OpenChannel and its receiver methods are moved across the package boundary.
Add a transitional non-locking status predicate for channeldb store code and use it from KV serialization helpers. This avoids calling an unexported OpenChannel helper from channeldb after the type moves into chanstate.
Move OpenChannel and its backend-neutral receiver methods into the chanstate package. channeldb now keeps a compatibility alias while retaining the KV store implementation and serialization helpers. Tests that used private channel status fields now use store-facing accessors.
Drop the temporary channel type parameter from the channel-state store interfaces now that OpenChannel lives in chanstate. The domain store facets now refer to *OpenChannel directly while retaining the same backend-independent shape. Update callers and compatibility aliases to use the concrete Store and ChannelShell types.
Copy all HTLC fields when cloning channel commitment state. The old copy method only copied a subset of scalar fields and copied into nil slices for Signature and ExtraData. Allocate those slices and deep-copy custom record values so snapshots and channel copies retain complete HTLC metadata.
Move channelnotifier and invoice hop-hint code to the chanstate channel types. These consumers already depend on the chanstate store interfaces, so they no longer need to refer to the channeldb compatibility aliases for OpenChannel and ChannelCloseSummary.
Move small interface-only consumers to chanstate.OpenChannel. These packages only expose open-channel values through callback or store interfaces, so they can depend on the channel-state package without pulling in channeldb compatibility aliases.
Move channel event payloads in chanfitness to chanstate types. The event store still uses channeldb for flap-count persistence and related errors, but open-channel and close-summary values now come from the channel-state package.
Move the witness subscription HTLC parameter to chanstate.HTLC. The beacon still depends on channeldb for witness-cache errors, but it no longer needs the channel DB compatibility alias for HTLC payloads.
Move the channel status manager working set to chanstate.OpenChannel. The DB interface already returns channelstate channel values, so this removes another channeldb compatibility alias from the consumer path while keeping the graph and announcement behavior unchanged.
Move lnpeer.NewChannel to embed chanstate.OpenChannel. This keeps the peer-facing channel event type independent of the channeldb compatibility alias while preserving the existing embedded OpenChannel field shape for callers.
Move static channel backup construction to chanstate channel types. The package still imports channeldb where it uses real database concerns, including address sourcing and duplicate-channel recovery. The backup payload and live-channel source boundaries no longer depend on the channeldb compatibility aliases.
Move the root backup notifier adapter to chanstate.OpenChannel. The adapter still depends on channeldb for address sourcing and close type handling, but the new-channel payload it forwards into chanbackup now matches the chanstate-owned backup interfaces.
Move the waiting-close channel helper to chanstate.OpenChannel. The helper consumes channel state returned by the store interface, so it should not spell the channeldb compatibility alias. Other database errors and APIs in the wallet RPC server remain on channeldb.
Move the htlcswitch channel fetch callbacks to chanstate types. The switch still depends on channeldb for its KV circuit storage and forwarding package access. This commit only moves channel-state payloads at the switch and circuit-map boundaries.
Move the local channel manager fetch boundary to chanstate types. The manager still imports channeldb for the concrete not-found error, but channel state values and config constraints now use the chanstate package directly.
Move the gossiper channel lookup callback to chanstate.OpenChannel. Discovery still depends on channeldb for waiting-proof persistence. This commit only removes the channeldb compatibility alias from the channel state lookup boundary.
Move watchtower blob and client channel-type boundaries to chanstate. The wtclient manager still imports channeldb for closed-channel lookup errors, but the channel type and close-summary payloads now use the channel state package directly.
Move htlcswitch link-facing channel state boundaries to chanstate. The link still uses channeldb for forwarding-package persistence, but channel update callbacks, tower registration, and dust helper channel types now use the channel state package directly.
Update the funding manager callback and helper signatures to depend on the chanstate OpenChannel type instead of the channeldb alias. The funding manager already receives channel persistence through the chanstate Store interface, so this keeps its open-channel boundary aligned with the backend-independent package.
Update server callback wiring to use chanstate.OpenChannel at the funding manager boundary. This follows the funding package change and removes another consumer-facing dependency on the channeldb OpenChannel alias.
Update RPC helpers that format open channel data to accept chanstate.OpenChannel directly. These helpers only inspect channel state and do not need to name the channeldb OpenChannel alias.
Build restored channel shells with chanstate.OpenChannel instead of the channeldb alias. The restored shell is channel state data, so this keeps the constructor aligned with the package that now owns the type.
Update contractcourt channel and resolver state boundaries to use chanstate.OpenChannel instead of the channeldb alias. This keeps the contract resolution package depending on channel state data through the package that now owns the type, while leaving channeldb references for store and error types that still belong there.
Update lnwallet channel, reservation, wallet, and test helpers to use chanstate.OpenChannel directly. The wallet package still imports channeldb for database APIs and other channel-state aliases, but the OpenChannel type boundary now points at the package that owns the type.
Update peer channel loading, validation, and test helpers to use chanstate.OpenChannel directly. The peer package still depends on channeldb for store-level errors and helpers, but no longer needs the OpenChannel alias in its public channel-state boundary.
Update htlcswitch test utilities to construct and pass chanstate.OpenChannel values directly. This removes another test-only dependency on the channeldb OpenChannel alias while leaving the test database helpers unchanged.
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 continues the architectural refactor of the channel state management system. By decoupling core channel state definitions from the database-specific implementation in channeldb and moving them into a new chanstate package, the project achieves a cleaner separation of concerns. This change updates numerous consumers throughout the repository to utilize the new chanstate types, establishing a more robust foundation for upcoming backend-specific refactors. 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
|
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (46 files)
🟠 High (9 files)
🟡 Medium (11 files)
AnalysisThis PR is a large-scale architectural refactor that extracts channel state types from
Reviewer concerns:
To override, add a |
![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 refactors the channel state persistence layer by migrating core types and logic from channeldb into a new chanstate package, with corresponding updates across the codebase to use these new definitions. The review identifies a critical issue in the OpenChannel.Copy method where the Db field is not initialized in the cloned object, which would cause nil pointer dereferences during subsequent method calls. Furthermore, the reviewer suggests improving the robustness of HTLC matching in the ActiveHtlcs method by using unique identifiers like HtlcIndex and the Incoming flag instead of relying on OnionBlob hashes.
Comment on lines
+1095
to
+1126
| clone := &OpenChannel{ | ||
| ChanType: c.ChanType, | ||
| ChainHash: c.ChainHash, | ||
| FundingOutpoint: c.FundingOutpoint, | ||
| ShortChannelID: c.ShortChannelID, | ||
| IsPending: c.IsPending, | ||
| IsInitiator: c.IsInitiator, | ||
| chanStatus: c.chanStatus, | ||
| FundingBroadcastHeight: c.FundingBroadcastHeight, | ||
| ConfirmationHeight: c.ConfirmationHeight, | ||
| NumConfsRequired: c.NumConfsRequired, | ||
| ChannelFlags: c.ChannelFlags, | ||
| IdentityPub: c.IdentityPub, | ||
| Capacity: c.Capacity, | ||
| TotalMSatSent: c.TotalMSatSent, | ||
| TotalMSatReceived: c.TotalMSatReceived, | ||
| InitialLocalBalance: c.InitialLocalBalance, | ||
| InitialRemoteBalance: c.InitialRemoteBalance, | ||
| LocalChanCfg: c.LocalChanCfg, | ||
| RemoteChanCfg: c.RemoteChanCfg, | ||
| LocalCommitment: c.LocalCommitment.Copy(), | ||
| RemoteCommitment: c.RemoteCommitment.Copy(), | ||
| RemoteCurrentRevocation: c.RemoteCurrentRevocation, | ||
| RemoteNextRevocation: c.RemoteNextRevocation, | ||
| RevocationProducer: c.RevocationProducer, | ||
| RevocationStore: c.RevocationStore, | ||
| ThawHeight: c.ThawHeight, | ||
| LastWasRevoke: c.LastWasRevoke, | ||
| RevocationKeyLocator: c.RevocationKeyLocator, | ||
| confirmedScid: c.confirmedScid, | ||
| TapscriptRoot: c.TapscriptRoot, | ||
| } |
There was a problem hiding this comment.
The Copy() method does not assign the Db field to the cloned OpenChannel instance. Since many methods on OpenChannel (e.g., Refresh, ApplyChanStatus, CloseChannel) rely on the Db field being populated, calling these methods on a cloned instance will result in a nil pointer dereference panic.
clone := &OpenChannel{
ChanType: c.ChanType,
ChainHash: c.ChainHash,
FundingOutpoint: c.FundingOutpoint,
ShortChannelID: c.ShortChannelID,
IsPending: c.IsPending,
IsInitiator: c.IsInitiator,
chanStatus: c.chanStatus,
FundingBroadcastHeight: c.FundingBroadcastHeight,
ConfirmationHeight: c.ConfirmationHeight,
NumConfsRequired: c.NumConfsRequired,
ChannelFlags: c.ChannelFlags,
IdentityPub: c.IdentityPub,
Capacity: c.Capacity,
TotalMSatSent: c.TotalMSatSent,
TotalMSatReceived: c.TotalMSatReceived,
InitialLocalBalance: c.InitialLocalBalance,
InitialRemoteBalance: c.InitialRemoteBalance,
LocalChanCfg: c.LocalChanCfg,
RemoteChanCfg: c.RemoteChanCfg,
LocalCommitment: c.LocalCommitment.Copy(),
RemoteCommitment: c.RemoteCommitment.Copy(),
RemoteCurrentRevocation: c.RemoteCurrentRevocation,
RemoteNextRevocation: c.RemoteNextRevocation,
RevocationProducer: c.RevocationProducer,
RevocationStore: c.RevocationStore,
ThawHeight: c.ThawHeight,
LastWasRevoke: c.LastWasRevoke,
RevocationKeyLocator: c.RevocationKeyLocator,
confirmedScid: c.confirmedScid,
TapscriptRoot: c.TapscriptRoot,
Db: c.Db,
}
Comment on lines
+809
to
+841
| // We'll only return HTLC's that are locked into *both* commitment | ||
| // transactions. So we'll iterate through their set of HTLC's to note | ||
| // which ones are present on their commitment. | ||
| remoteHtlcs := make(map[[32]byte]struct{}) | ||
| for _, htlc := range c.RemoteCommitment.Htlcs { | ||
| log.Tracef("RemoteCommitment has htlc: id=%v, update=%v "+ | ||
| "incoming=%v", htlc.HtlcIndex, htlc.LogIndex, | ||
| htlc.Incoming) | ||
|
|
||
| onionHash := sha256.Sum256(htlc.OnionBlob[:]) | ||
| remoteHtlcs[onionHash] = struct{}{} | ||
| } | ||
|
|
||
| // Now that we know which HTLC's they have, we'll only mark the HTLC's | ||
| // as active if *we* know them as well. | ||
| activeHtlcs := make([]HTLC, 0, len(remoteHtlcs)) | ||
| for _, htlc := range c.LocalCommitment.Htlcs { | ||
| log.Tracef("LocalCommitment has htlc: id=%v, update=%v "+ | ||
| "incoming=%v", htlc.HtlcIndex, htlc.LogIndex, | ||
| htlc.Incoming) | ||
|
|
||
| onionHash := sha256.Sum256(htlc.OnionBlob[:]) | ||
| if _, ok := remoteHtlcs[onionHash]; !ok { | ||
| log.Tracef("Skipped htlc due to onion mismatched: "+ | ||
| "id=%v, update=%v incoming=%v", | ||
| htlc.HtlcIndex, htlc.LogIndex, htlc.Incoming) | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| activeHtlcs = append(activeHtlcs, htlc) | ||
| } | ||
|
|
There was a problem hiding this comment.
The logic for matching HTLCs between local and remote commitments using the hash of the OnionBlob is not robust. Identical HTLCs (same amount, payment hash, expiry, and path) will have the same OnionBlob if the ephemeral key is reused (which can happen with buggy or malicious senders). This can lead to an incorrect count of active HTLCs if they are not present in the same quantity on both commitments. It is recommended to use the HtlcIndex and the Incoming flag, which uniquely and consistently identify an HTLC across both commitment transactions.
// We'll only return HTLC's that are locked into *both* commitment
// transactions. So we'll iterate through their set of HTLC's to note
// which ones are present on their commitment.
type htlcKey struct {
id uint64
incoming bool
}
remoteHtlcs := make(map[htlcKey]struct{})
for _, htlc := range c.RemoteCommitment.Htlcs {
log.Tracef("RemoteCommitment has htlc: id=%v, update=%v "+
"incoming=%v", htlc.HtlcIndex, htlc.LogIndex,
htlc.Incoming)
remoteHtlcs[htlcKey{htlc.HtlcIndex, htlc.Incoming}] = struct{}{}
}
// Now that we know which HTLC's they have, we'll only mark the HTLC's
// as active if *we* know them as well.
activeHtlcs := make([]HTLC, 0, len(remoteHtlcs))
for _, htlc := range c.LocalCommitment.Htlcs {
log.Tracef("LocalCommitment has htlc: id=%v, update=%v "+
"incoming=%v", htlc.HtlcIndex, htlc.LogIndex,
htlc.Incoming)
// An HTLC is active if it's present on both commitments. On our
// commitment, an HTLC we received is marked as incoming. On
// their commitment, that same HTLC is marked as outgoing
// (incoming=false). Thus we flip the incoming flag when
// performing the lookup.
if _, ok := remoteHtlcs[htlcKey{htlc.HtlcIndex, !htlc.Incoming}]; !ok {
log.Tracef("Skipped htlc due to index mismatched: "+
"id=%v, update=%v incoming=%v",
htlc.HtlcIndex, htlc.LogIndex, htlc.Incoming)
continue
}
activeHtlcs = append(activeHtlcs, htlc)
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR continues the channel-state refactor by updating consumers to depend on
chanstate.OpenChanneland related channel-state types directly.\n\nThe intent is to make package boundaries reflect that open-channel state is owned bychanstate, while keeping the temporarychanneldbaliases in place for the later code move. The remaining backend-specific implementation can then move in follow-up PRs without mixing that larger movement with consumer updates.\n\nMigration code is intentionally left unchanged.\n\nVerification:\n-make lint-native\n-go test . ./channeldb ./funding ./lnwallet ./contractcourt ./peer ./input ./lnrpc/walletrpc\n-go test -tags=dev ./htlcswitch