lnwallet: surface initial commitment key ring for N=0 force-close aux resolution#10804
lnwallet: surface initial commitment key ring for N=0 force-close aux resolution#10804GeorgeTsagk wants to merge 3 commits into
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 mechanism to provide auxiliary resolvers with the initial commitment key ring for channels that are force-closed at height 0. By correctly identifying the commitment keys used before the channel_ready rotation, the changes prevent output misidentification and ensure that auxiliary components function correctly during immediate post-funding force-close events. 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 (2 files)
AnalysisBoth changed files reside in the
This PR is small in scope (2 files, 61 lines), so no severity bump was triggered. However, the inherent criticality of 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 introduces an InitialKeyRing field to the ResolutionReq struct and implements initialCommitmentKeyRingFromState to derive the key ring for the initial commitment state at height 0. This allows downstream resolvers to correctly distinguish between initial and subsequent commitment states. The reviewer suggested minor documentation improvements to adhere to the repository's style guide, specifically regarding function comment formatting and the requirement for function documentation.
| return incomingHtlcs, outgoingHtlcs, nil | ||
| } | ||
|
|
||
| // initialCommitmentKeyRing derives the key ring for the initial commitment |
There was a problem hiding this comment.
The function comment should start with the function's name (initialCommitmentKeyRingFromState) to adhere to the repository's style guide.
| // initialCommitmentKeyRing derives the key ring for the initial commitment | |
| // initialCommitmentKeyRingFromState derives the key ring for the initial commitment |
References
- Function comments must begin with the function name. (link)
There was a problem hiding this comment.
I think lnd is pretty strict on commits, so the commit making this fix should probably be folded into the appropriate previous one.
| func (lc *LightningChannel) initialCommitmentKeyRing( | ||
| whoseCommit lntypes.ChannelParty) *CommitmentKeyRing { | ||
|
|
||
| return initialCommitmentKeyRingFromState(lc.channelState, whoseCommit) | ||
| } |
There was a problem hiding this comment.
This function is missing a comment, which is required by the repository's style guide. Please add a comment that describes its purpose.
| func (lc *LightningChannel) initialCommitmentKeyRing( | |
| whoseCommit lntypes.ChannelParty) *CommitmentKeyRing { | |
| return initialCommitmentKeyRingFromState(lc.channelState, whoseCommit) | |
| } | |
| // initialCommitmentKeyRing derives the key ring for the initial commitment | |
| // state at height 0 for the channel. | |
| func (lc *LightningChannel) initialCommitmentKeyRing( | |
| whoseCommit lntypes.ChannelParty) *CommitmentKeyRing { | |
| return initialCommitmentKeyRingFromState(lc.channelState, whoseCommit) | |
| } |
References
- Every function must be commented with its purpose and assumptions. (link)
GeorgeTsagk
force-pushed
the
close-immediately-itest-lnd-7c38
branch
from
077847d to
70cfed1
Compare
GeorgeTsagk
force-pushed
the
close-immediately-itest-lnd-7c38
branch
from
70cfed1 to
94fdcc7
Compare
|
@GeorgeTsagk, remember to re-request review from reviewers when ready |
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
I'm pretty sure this one can be simplified dramatically, and I think the simplifications will carry over to the associated tapd PR. I commented on a bunch of stuff, but didn't check any of it myself, so it could be wrong. Seems worth following these Opus-generated instructions to check:
- Drop ResolutionReq.InitialKeyRing; add ResolutionReq.CommitHeight fn.Option[uint64].
- Delete initialCommitmentKeyRingFromState.
- In NewLocalForceCloseSummary, set CommitHeight: fn.Some(chanState.LocalCommitment.CommitHeight).
- In NewUnilateralCloseSummary, set CommitHeight: fn.Some(remoteCommit.CommitHeight).
- Audit the other ResolutionReq construction sites (lnwallet/channel.go:2311, 2394, 7530, 7766, 7903, 8130) and set CommitHeight from the state-num in scope.
| return incomingHtlcs, outgoingHtlcs, nil | ||
| } | ||
|
|
||
| // initialCommitmentKeyRing derives the key ring for the initial commitment |
There was a problem hiding this comment.
I think lnd is pretty strict on commits, so the commit making this fix should probably be folded into the appropriate previous one.
| // initialCommitmentKeyRing derives the key ring for the initial commitment | ||
| // state at height 0. This is distinct from the live key ring after | ||
| // channel_ready, which already moved on to the next per-commitment point. | ||
| // If the initial commitment point can't be derived, then nil is returned and |
There was a problem hiding this comment.
Feels like this should return (nil, error)?
Whilst asking Opus if it agreed (it did), it also happened to point out that the remote and default cases of the switch are now dynamically, i.e. at runtime, unreachable. It is absolutely righteous to handle them anyway, but it recommended wiring up the initial keyring plumbing such that the remote branch was exercised "in case tapd also needs to do the proof-rewrite dance for remote-initiated force closes."
I asked it if tapd does need to do the proof-rewrite dance for remote-initiated force closes, and it seems to think it does. It recommends the following:
In NewUnilateralCloseSummary, populate InitialKeyRing on the ResolutionReq at lnwallet/channel.go:7252 when chanState.RemoteCommitment.CommitHeight == 0 — symmetric to what commit 2 did for the local path. The remote branch of initialCommitmentKeyRingFromState (currently the "dead" one) is exactly what this needs.
The itest (testCustomChannelsImmediateClose) only exercises Alice closing. Add a symmetric scenario where Bob force-closes immediately after funding — that would have caught this asymmetry as a regression.
Seems worth adding that itest case to check if it's actually on to something here.
There was a problem hiding this comment.
After looking into the CommitHeight idea I asked Opus what simplifications that might imply. It claims that this function "evaporates entirely:"
The helper exists only to produce a keyring tapd can use as a height-0 signal. Once CommitHeight carries the signal directly and tapd just uses req.KeyRing at height 0 (because KeyRing already is the height-0 keyring there), nothing in lnd needs this helper. It was 40-ish lines including the local/remote switch, the wrapper method (already removed by commit 3), and the doc comment. All gone. That subsumes:
- The (nil, error) question I raised — there's no helper to fix.
- The dead remote-branch concern — there's no branch.
- The mismatch between the helper's "fall back to live key ring" comment
and commit 2's actual gating — there's no comment.- The lntypes.ChannelParty parameter on the helper — gone with the helper.
Worth confirming it's right.
| // KeyRing is the key ring for the channel. | ||
| KeyRing *CommitmentKeyRing | ||
|
|
||
| // InitialKeyRing is the key ring for the initial commitment state at |
There was a problem hiding this comment.
This one feels a bit weird since it's a sort of temporary duplication of the KeyRing state. Might be cleaner to pass the commitment height itself, e.g.:
CommitHeight uint64
and then I think if that's equal to zero you can safely just check the keyring state to get what you need.
(Also, after checking some of the stuff mentioned in other comments, you might actually need fn.Option[uint64] here to distinguish actually-a-zero-commit-height from the default 0 value.)
3 participants
Description
This PR adds InitialKeyRing to lnwallet.ResolutionReq and populates it only when local commit height is 0 during force-close resolution. It gives aux resolvers the correct initial commitment key context for immediate post-funding (N=0) force closes, while leaving later commit-height force-close behavior unchanged (InitialKeyRing=nil).
At N=0, the commitment output keys are derived from the initial commitment point (pre-channel_ready rotation), so using only the live key ring can misidentify outputs and break the aux components.