[custom channels 1/5]: extract PART1 from mega staging branch #9030
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
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 Configuration File (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
lnwallet/aux_leaf_store.go
Outdated
| type CommitDiffAuxInput struct { | ||
| // ChannelState is the static channel information of the channel this | ||
| // commitment transaction relates to. | ||
| ChannelState *channeldb.OpenChannel |
There was a problem hiding this comment.
yikes - guess we'll figure out how to not reference big struct in future PRs...
There was a problem hiding this comment.
Yeah, it's not optimal. But we're re-creating quite a bit of the channel logic in tapd, so we need access to a lot of fields on this struct. We could inline the fields we need into this new CommitDiffAuxInput struct instead, but that would just grow the diff with a lot of fields that would be copied.
There was a problem hiding this comment.
Yikes++. It's times like these when I wish we could distinguish between immutable and mutable pointers like rust. Personally I think that copying over the fields is the right thing to do if what you're doing is read-only.
There was a problem hiding this comment.
Okay, I've added a new struct AuxChanState that the required fields are copied into.
| // lets us skip sending the entire transaction over, instead we'll just | ||
| // send signatures. | ||
| commitSort := auxResult.CommitSortFunc.UnwrapOr(DefaultCommitSort) | ||
| err = commitSort(commitTx, cltvs, htlcIndexes) |
There was a problem hiding this comment.
I don't quite follow the need to sort based on the htlc index - since HtlcIndex is monotonically increasing, the sort should have no effect unless we are sorting descendingly in tap chan?
There was a problem hiding this comment.
If there are multiple HTLCs with the same amount, same CLTV timeout and same payment hash (e.g. MPP shards with same split amount), they look identical on the BTC level (exact same script and amount), so the sort order of them is "fungible" (e.g. it doesn't matter which comes first in the transaction).
But on the Taproot Asset level it matters, because the actual asset leaves that are committed to each HTLC might be different. So we need to know exactly what output index a specific HTLC is located at in the final on-chain transaction. Therefore we need to take the HTLC index as an additional sorting indicator.
I didn't want to put any asset specific comments into the lnd code. But I added a bit more rationale to the documentation of the CommitSortFunc comment.
There was a problem hiding this comment.
Thanks for the explanation!
There was a problem hiding this comment.
I must be missing something: Bolt 3 specifies that scriptpubkey is part of the sort order and afaik the tapscript root affects that, doesn't it?
There was a problem hiding this comment.
Yes, you are correct. We're basically sorting by amount and scriptpubkey and the tapscript root affects that. But because the actual custom channel tapscript leaf depends on the output index and the output index changes depending on the leaf, we have a potentially infinite recursion.
So we decided to break with Bolt3 and just do a single iteration of sort using the fields mentioned above, then add the extra leaves even though that causes the transaction not to be BIP-069 sorted anymore.
IMO this is fine as both parties of custom channels will be able to deterministically do that. The only slight downside is that such transactions would be identifiable as custom channel transactions if they ever went on chain.
|
Thanks a lot for the review, @yyforyongyu! I pushed 4 fixup commits. I'll rebase/squash them shortly before merging, there might be more after @ProofOfKeags' review. |
| // lets us skip sending the entire transaction over, instead we'll just | ||
| // send signatures. | ||
| commitSort := auxResult.CommitSortFunc.UnwrapOr(DefaultCommitSort) | ||
| err = commitSort(commitTx, cltvs, htlcIndexes) |
There was a problem hiding this comment.
Thanks for the explanation!
lnwallet/aux_leaf_store.go
Outdated
| type CommitDiffAuxInput struct { | ||
| // ChannelState is the static channel information of the channel this | ||
| // commitment transaction relates to. | ||
| ChannelState *channeldb.OpenChannel |
There was a problem hiding this comment.
Yikes++. It's times like these when I wish we could distinguish between immutable and mutable pointers like rust. Personally I think that copying over the fields is the right thing to do if what you're doing is read-only.
| // lets us skip sending the entire transaction over, instead we'll just | ||
| // send signatures. | ||
| commitSort := auxResult.CommitSortFunc.UnwrapOr(DefaultCommitSort) | ||
| err = commitSort(commitTx, cltvs, htlcIndexes) |
There was a problem hiding this comment.
I must be missing something: Bolt 3 specifies that scriptpubkey is part of the sort order and afaik the tapscript root affects that, doesn't it?
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.
In this commit, we update all the taproot scripts to also accept an optional aux leaf. This aux leaf can be used to add more redemption paths for advanced channels, or just as an extra commitment space.
In this commit, for each channel, we'll now start to store an optional custom blob. This can be used to store extra information for custom channels in an opauqe manner.
We'll need this later on to ensure we can always interact with the new aux blobs at all stages of commitment transaction construction.
In this commit, we also add the custom TLV blob to the internal commitment struct that we use within the in-memory commitment linked list. This'll be useful to ensure that we're tracking the current blob for our in memory commitment for when we need to write it to disk.
In this commit, we add some useful type definitions for the aux leaf.
guggero
force-pushed
the
extract-part1-from-staging-branch
branch
from
dbeeb37
to
4a23771
Compare
In this commit, we add a new AuxLeafStore which can be used to dynamically fetch the latest aux leaves for a given state. This is useful for custom channel types that will store some extra information in the form of a custom blob, then will use that information to derive the new leaf tapscript leaves that may be attached to reach state.
In this commit, we add a TLV blob to the PaymentDescriptor struct. We also now thread through this value from the UpdateAddHTLC message to the PaymentDescriptor mapping, and the other way around.
This'll be useful for custom channel types that want to store extra information that'll be useful to help handle channel revocation cases.
This may be useful for custom channel types that base everything off the index (a global value) rather than the output index (can change with each state).
In this commit, we start to thread thru the new aux tap leaf structures to all relevant areas. This includes: commitment outputs, resolution creation, breach handling, and also HTLC scripts.
guggero
force-pushed
the
extract-part1-from-staging-branch
branch
from
4a23771
to
9dfbde7
Compare
Extracts part 1 from #8960.