lnwallet: expose negotiated configs in aux chan state - #10804
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
|
🔴 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 |
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.
| 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)
077847d to
70cfed1
Compare
70cfed1 to
94fdcc7
Compare
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.
94fdcc7 to
769500d
Compare
|
I may revisit this one. Presently reviewing lightninglabs/taproot-assets#2133, and I think this change may not actually be necessary to solve the issue we encounter in tapd downstream? Will report more when I know more.. |
769500d to
de8a76a
Compare
jtobin
left a comment
There was a problem hiding this comment.
Just a couple of final recos after your last round of changes to lightninglabs/taproot-assets#1933.
de8a76a to
dce6625
Compare
dce6625 to
cc85166
Compare
|
The diff appears to no longer reflect the PR OP desc. |
Updated |
cc85166 to
bbde57a
Compare
ziggie1984
left a comment
There was a problem hiding this comment.
Change makes sense, wondering why we did not encounter this way earlier on the asset software side, wasn't the config used there form the beginning ?
From what I can tell the broken case is just super rare. IIRC one has to force close before any HTLCs have been exchanged. |
bbde57a to
b0a25b9
Compare
|
tests in |
taking a look |
b0a25b9 to
97f642e
Compare
|
@Roasbeef: review reminder |
|
@Roasbeef: review reminder |
2 similar comments
|
@Roasbeef: review reminder |
|
@Roasbeef: review reminder |
Description
Pass a populated aux channel state into aux funding so initial custom channel blobs include the negotiated local/remote configs.
This fixes height-0 aux channel state without adding force-close resolution plumbing in lnd.