multi: fix SIMPLE_TAPROOT_FINAL acceptor and overlay RBF auto-enable#10763
Conversation
The feature-bits-to-lnrpc-enum switch in sendAcceptRequests covered every commitment type the RPC acceptor can be asked about, except the production taproot variant introduced alongside the prod-taproot-chans work. For a channel open using SimpleTaprootChannelsRequiredFinal (with any combination of the scid-alias / zero-conf modifiers), the switch fell through to the default branch, which logs a warning and leaves commitmentType at its zero value -- lnrpc.CommitmentType_UNKNOWN_COMMITMENT_TYPE. External acceptor clients then see UNKNOWN rather than the actual commitment type and either reject or misclassify the channel. Add the four missing cases so the new commitment type is reported to acceptor clients correctly.
An earlier commit added an auto-enable that forces RbfCoopClose=true whenever either taproot channel flag is set. This breaks taproot-overlay channels, because the RBF coop close state machine in lnwallet/chancloser/rbf_coop_*.go does not integrate the AuxCloser (or any other aux) hook that overlay channels depend on to build aux-aware close transactions. A node that enables --protocol.simple-taproot-overlay-chans ends up with RBF force-on and its overlay channel closes silently fail, leaving the aux closer unable to finalize on-chain. Narrow the auto-enable so it only fires for TaprootChans (staging / final taproot) and explicitly skips it when TaprootOverlayChans is set. Operators that positively want RBF can still opt in via --protocol.rbf-coop-close; this change only removes the forced path that silently breaks overlay closes.
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 improves channel management and protocol interoperability. It addresses a reporting issue for final-taproot channels in the RPC acceptor and modifies the server configuration logic to safely handle RBF cooperative close auto-enabling in the presence of taproot-overlay channels. 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
|
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (1 file)
AnalysisThis PR modifies The change is small (2 files, 41 lines total), so no severity bump was triggered. However, expert review is warranted given that To override, add a |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the SIMPLE_TAPROOT_FINAL commitment type by handling various feature bit combinations in the RPC acceptor. Additionally, it updates the server logic to ensure RBF cooperative close is not automatically enabled when taproot-overlay channels are active, preventing potential issues with the current state machine. A review comment suggests refactoring the repetitive case statements in rpcacceptor.go into a more concise and maintainable block by cloning the feature vector and unsetting allowed bits.
| case channelFeatures.OnlyContains( | ||
| lnwire.SimpleTaprootChannelsRequiredFinal, | ||
| lnwire.ZeroConfRequired, | ||
| lnwire.ScidAliasRequired, | ||
| ): | ||
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | ||
|
|
||
| case channelFeatures.OnlyContains( | ||
| lnwire.SimpleTaprootChannelsRequiredFinal, | ||
| lnwire.ZeroConfRequired, | ||
| ): | ||
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | ||
|
|
||
| case channelFeatures.OnlyContains( | ||
| lnwire.SimpleTaprootChannelsRequiredFinal, | ||
| lnwire.ScidAliasRequired, | ||
| ): | ||
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | ||
|
|
||
| case channelFeatures.OnlyContains( | ||
| lnwire.SimpleTaprootChannelsRequiredFinal, | ||
| ): | ||
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL |
There was a problem hiding this comment.
These four case statements for SIMPLE_TAPROOT_FINAL are all setting the same commitmentType and are quite repetitive. This could be simplified to improve readability and maintainability. A single case can handle all combinations by checking for the base feature and ensuring no other unexpected features are present.
This pattern of repeated case statements is also used for other commitment types in this switch block and could be refactored similarly.
| case channelFeatures.OnlyContains( | |
| lnwire.SimpleTaprootChannelsRequiredFinal, | |
| lnwire.ZeroConfRequired, | |
| lnwire.ScidAliasRequired, | |
| ): | |
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | |
| case channelFeatures.OnlyContains( | |
| lnwire.SimpleTaprootChannelsRequiredFinal, | |
| lnwire.ZeroConfRequired, | |
| ): | |
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | |
| case channelFeatures.OnlyContains( | |
| lnwire.SimpleTaprootChannelsRequiredFinal, | |
| lnwire.ScidAliasRequired, | |
| ): | |
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | |
| case channelFeatures.OnlyContains( | |
| lnwire.SimpleTaprootChannelsRequiredFinal, | |
| ): | |
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | |
| case channelFeatures.IsSet(lnwire.SimpleTaprootChannelsRequiredFinal): | |
| // We'll clone the feature vector and unset all allowed | |
| // bits. If the vector is empty after, we know that | |
| // no other unexpected bits were set. | |
| clone := (&channelFeatures).Clone() | |
| clone.Unset(lnwire.SimpleTaprootChannelsRequiredFinal) | |
| clone.Unset(lnwire.ZeroConfRequired) | |
| clone.Unset(lnwire.ScidAliasRequired) | |
| if clone.IsEmpty() { | |
| commitmentType = lnrpc.CommitmentType_SIMPLE_TAPROOT_FINAL | |
| } |
| // to build the aux-aware close transaction. Forcing RBF on for a | ||
| // node that holds overlay channels would silently break their coop | ||
| // closes. | ||
| if cfg.ProtocolOptions.TaprootChans && |
There was a problem hiding this comment.
does this mean if we have overlay channels the rbf protocol is also switch off for the normal taproot channels ?
There was a problem hiding this comment.
but if the user sets it via the config it still breaks the coop close flow for overlay channels, we should prio this fix on the taproot assets side then.
There was a problem hiding this comment.
yes we don't want to auto-enable it for overlay chans
the proper fix involves LND diff (integrating the AuxCloser in the RBF closer) and maybe some supporting code in taproot assets. Since we're already at the rc phase this should be enough. We control the flags in litd so for now we'll just try to protect & discourage the user from setting the flag (or even fail early in litd).
Description
Adds the missing
SIMPLE_TAPROOT_FINALcases to the RPC acceptor switch so external acceptor clients no longer seeUNKNOWN_COMMITMENT_TYPEfor final-taproot channel opens, and narrows the taproot RBF auto-enable to skip taproot-overlay (where the RBF coop-close state machine does not yet thread through theAuxCloserhook).