-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1/?]: multi: add ability to fund+use musig2 channels that commit to a tapscript root #8683
Conversation
Pull reviewers statsStats of the last 30 days for lnd:
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
I can't approve the PR because I re-opened it. But I reviewed it and worked my feedback directly into the commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🥕 ⚡
solid changes
In this commit, we rename the files as assembler.go houses the primary interfaces/abstractions of the package. In the rest of the codebase, this file is near uniformly called interface.go, so we rename the file to make the repo more digestible at a scan.
08a6c9c
to
c4b9d94
Compare
channeldb/channel.go
Outdated
// to the existing hard-coded fields in the channel's root bucket. | ||
type chanAuxData struct { | ||
// revokeKeyLoc is the key locator for the revocation key. | ||
revokeKeyLoc tlv.RecordT[tlv.TlvType0, keyLocRecord] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have started at 0 originally, but I think now this should start at 1 because of records already in the database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think in @Roasbeef's original commit it did start at 1 and I thought that was an oversight. But if there's a reason for it, then I'll change it back as I missed that. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this again and I'm not sure what "records already in the database" you mean? The aux data is only written in putChanInfo()
and read in fetchChanInfo()
where we first have static entries with ReadElements/WriteElements
and then at the end a TLV stream (using the fact that EOF is a valid error when reading a TLV stream).
So there are no existing TLV streams in the chan-info-key
sub bucket that things could collide with. Or what am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an existing channel already in the database, revokeKeyLoc
will be stored with the tlv type of 1, but chanAuxData
will attempt to read it as though the type is 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, of course... Was looking at it from a completely wrong angle, sorry. Totally makes sense, will fix/revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, not sure what I was thinking when changing it in the first place...
In this commit, we consolidate the root bucket TLVs into a new struct. This makes it easier to see all the new TLV fields at a glance. We also convert TLV usage to use the new type param based APis.
This'll allow us to create a funding output that uses musig2, but uses a tapscript tweak rather than a normal BIP 86 tweak.
In most cases, we won't yet be passing a root. The option usage helps us keep the control flow mostly unchanged.
This isn't hooked up yet to the funding manager, but with this commit, we can now start to write internal unit tests that handle musig2 channels with a tapscript root.
With this commit, the channel is now aware of if it's a musig2 channel, that also has a tapscript root. We'll need to always pass in the tapscript root each time we: make the funding output, sign a new state, and also verify a new state.
c4b9d94
to
26ce8ee
Compare
Replaces #8546 (with correct base branch and upstream feature branch).
In this PR, we add the initial plumbing that'll allow users to create and use musig2 channels that also commit to a tapscript root. Along the way, we start to store a new optional tapscript root in the
channeldb.OpenChannel
struct within the existing TLV stream appended to the older hard coded byte stream. The musig2 session itself will now conditionally pass in a tapscript root to the key aggregation and signing operations. We've also hooked up the lower half of the funding flow (lnwallet reservations) as well to ensure the new field gets propagated all the way down the relevant call stacks.Link to all PRs in the saga: