funding: require explicit taproot channel negotiation#10820
funding: require explicit taproot channel negotiation#10820yyforyongyu wants to merge 7 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 modifies the channel negotiation logic to prevent taproot channels from being selected implicitly. It enforces that taproot channels must be explicitly requested by the user, while also adding safety guards to prevent the creation of public taproot channels, which are not yet fully supported. These changes ensure more predictable channel type selection and prevent potential issues with unsupported public taproot configurations. 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)
🟢 Low (1 file)
AnalysisThis PR modifies the The change is relatively small (44 non-test lines), touching commitment type negotiation logic and the funding manager. Despite the modest size, the sensitivity of the funding package warrants expert review to ensure correctness of any negotiation logic changes. 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 updates the commitment type negotiation logic to prefer anchor channels over taproot for implicit opens, requiring taproot to be explicitly requested. It also adds a safeguard to reject public taproot channels at the funding manager level and includes new unit tests to verify these changes. I have no feedback to provide.
yyforyongyu
force-pushed
the
fix-channel-open
branch
from
6f97c50 to
44aeb19
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM (pending linter fixes)
@kaloudis not sure if that touches you but default channel opens for private channels will not be taproot with this change.
| remote *lnwire.FeatureVector) (*lnwire.ChannelType, | ||
| lnwallet.CommitmentType) { | ||
|
|
||
| // Taproot channels are checked before anchors intentionally: when both |
There was a problem hiding this comment.
Let's add a TODO to enable it as soon as public taproot channels are available ?
|
Small docs nit: the new release note says taproot must be requested via Maybe adjust this to: Taproot must be requested explicitly via `--channel_type=taproot-final`
(or `taproot` for staging).If the intent is to document the raw RPC enum rather than |
| openChannelReq, ok := msg.(*lnwire.OpenChannel) | ||
| require.True(t, ok) | ||
|
|
||
| // Flip the captured wire message to public so the responder path is |
| openChannelReq.ChannelFlags = lnwire.FFAnnounceChannel | ||
| bob.fundingMgr.ProcessFundingMsg(openChannelReq, alice) | ||
|
|
||
| // The specific taproot/public failure is logged locally; the wire error |
| remote *lnwire.FeatureVector) (*lnwire.ChannelType, | ||
| lnwallet.CommitmentType) { | ||
|
|
||
| // Taproot channels must be requested explicitly. This keeps implicit |
There was a problem hiding this comment.
this comment is introduced then deleted - we should instead remove it in bd2d0de
| RBF close state machine does not yet thread through the `AuxCloser` hook | ||
| that overlay channels rely on to build aux-aware close transactions. | ||
|
|
||
| * Fixed default channel opens between peers that both advertise simple taproot |
There was a problem hiding this comment.
there is no need for a release note here, given that this PR just restores the behavior in 0.20?
yyforyongyu
force-pushed
the
fix-channel-open
branch
3 times, most recently
from
dfdcf53 to
5569db3
Compare
saubyk
added
the
backport-v0.21.x-branch
calvinrzachman
left a comment
calvinrzachman
left a comment
There was a problem hiding this comment.
Gave this a spin locally against our integration test suite with loop. Cherry-picked the three PR commits onto the v0.21.x-branch, and removed the explicit --commitment_type=anchors workaround I had been running with. Public channel opens now succeed without the need to explicitly specify a commitment type. LGTM.
Roasbeef
force-pushed
the
fix-channel-open
branch
from
5569db3 to
abae492
Compare
github-actions
Bot
added
severity-critical
In this commit, we shuffle the CLI and RPC names so the bare "taproot"
identifier refers to the production taproot channel type (final
scripts, feature bits 80/81), i.e. the variant new integrations should
actually be using. Before this commit, "taproot" on the CLI mapped to
the staging bits, and anyone who wanted a real production taproot
channel had to spell out "taproot-final" on `lncli openchannel` or
`SIMPLE_TAPROOT_FINAL` over RPC. The recommended choice was hidden
behind the longer name.
On the CLI (`lncli openchannel --channel_type=...`):
- "taproot" now selects the production variant (it used to mean
staging).
- "taproot-staging" is added for the legacy development bits, for
peers that haven't moved over yet.
- "taproot-final" stays as a deprecated alias for "taproot" so
existing scripts don't break.
On the RPC (`CommitmentType`):
- `TAPROOT = 7` is added as the canonical name for the production
type. `SIMPLE_TAPROOT_FINAL = 7` is kept as a deprecated alias via
`option allow_alias = true`, so existing clients keep compiling
against the same Go constant and the wire value doesn't change.
- `SIMPLE_TAPROOT = 5` (staging) is unchanged.
- `SIMPLE_TAPROOT_OVERLAY = 6` is unchanged. The taproot-assets
daemon hard-codes this distinct enum value, so it's unaffected.
Wire compat is preserved end-to-end: only the comments, enum entry
order, and the CLI string-to-enum mapping change. The numeric values
and the existing generated Go identifiers stay stable.
Roasbeef
force-pushed
the
fix-channel-open
branch
from
2a5b7fd to
5bcc525
Compare
github-actions
Bot
added
severity-high
🟠 PR Severity: HIGH
🟠 High (4 files)
🟡 Medium (1 file)
🟢 Low (1 file)
AnalysisThe highest-severity files in this PR are in The previous Bump conditions checked:
To override, add a |
fixup! funding: require explicit taproot channel negotiation
fixup! funding: reject public taproot opens before sending
fixup! multi: rename taproot channel type to mean production variant
github-actions
Bot
added
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (2 files)
🟠 High (2 files)
🟡 Medium (1 file)
🟢 Low (1 file)
AnalysisThis PR touches the
The accompanying Severity bump checks:
The To override, add a |
5 participants
Summary
OpenChannelFixes #10819.
Tests
GOWORK=off go test ./funding -run 'TestCommitmentTypeNegotiation|TestFundingManagerRejectPublicTaprootInitiator|TestFundingManagerRejectPublicTaprootResponder'\n-GOWORK=off go test ./funding