[custom channels 3/5]: Extract PART3 from mega staging branch #9072
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
guggero
force-pushed
the
extract-part3-from-staging-branch
branch
3 times, most recently
from
376ccb4
to
5e33608
Compare
|
Requires a spicy rebase now that #8981 is in... |
| @@ -4043,6 +4059,21 @@ func (f *Manager) handleChannelReady(peer lnpeer.Peer, //nolint:funlen | |||
| PubNonce: remoteNonce, | |||
| }), | |||
| ) | |||
|
|
|||
| err = fn.MapOptionZ( | |||
There was a problem hiding this comment.
These both call AuxFundingController.ChannelReady but are two slightly different states:
handleChannelReadymeans we've receivedchannel_ready, we still need to send ours potentiallyhandleChannelReadyReceivedmeans we've sent + receivedchannel_ready
Not sure if that matters at all for the way the funding controller is using these notifications.
There was a problem hiding this comment.
Well spotted! But fortunately it doesn't really matter for the current use case. Added a comment to make it more clear.
guggero
force-pushed
the
extract-part3-from-staging-branch
branch
2 times, most recently
from
2199ea9
to
49a8371
Compare
This struct will house all the information we'll need to do a class of custom channels that relies primarily on adding additional items to the tapscript root of the HTLC/commitment/funding outputs.
With this commit, we'll now populate all the custom channel information within the OpenChannel and ChannelCommitment structs.
In this commit, we make a new `AuxFundingController` interface capable of processing messages off the wire. In addition, we can use it to abstract away details w.r.t how we obtain a `AuxFundingDesc` for a given channel. We'll now use this whenever we get a channel funding request, to make sure we pass along the custom state that a channel may require.
If this is a taproot channel, then we'll return the internal key which'll be useful to callers.
We also add a new assertion to the itests to ensure the field is being properly set.
In this commit, we modify the aux funding work flow slightly. We won't be able to generate the full AuxFundingDesc until both sides has sent+received funding params. So we'll now only attempt to bind the tapscript root as soon as we send+recv the open_channel message. We'll now also make sure that we pass the tapscript root all the way down into the musig2 session creation.
For the initiator, once we get the signal that the PSBT has been finalized, we'll call into the aux funder to get the funding desc. For the responder, once we receive the funding_created message, we'll do the same. We now also have local+remote aux leaves for the commitment transaction. Some old TODO comments that in retrospect aren't required anymore are removed as well.
In this commit, we add a new aux signer interface that's meant to mirror the SigPool. If present, this'll be used to (maybe) obtain signatures for second level HTLCs for certain classes of custom channels.
Due to a recent refactor, the HTLCs are no longer an exported type. Custom channels need access to those updates, so we provide them in a read-only manner.
guggero
force-pushed
the
extract-part3-from-staging-branch
branch
2 times, most recently
from
3dc1947
to
8cb20e7
Compare
lnwallet/channel.go
Outdated
| // We need to limit the size of the custom records to prevent the whole | ||
| // commitment_signed message to not exceed the maximum message size of | ||
| // 65k. If we assume a maximum number of HTLCs on the commitment of 483, | ||
| // and each signature being 64 bytes, we already use up 30k of the |
There was a problem hiding this comment.
The maximum number is 966, 483 is the one-way limit. Not sure how limiting that is for the custom channels use-case. If we want more space, we'd need to fragment the message across multiple brontide packets
There was a problem hiding this comment.
Ah, right... I'm not sure we can split the sigs over multiple messages. Or it would be a big refactor.
I changed the code to calculate the remaining number of bytes left for custom records. If we exceed that, we'll just fail the HTLC (I think). So for custom channels the real number of potential in-flight HTLCs will be quite a bit lower than the theoretical total of 966. But I think that's fine for now.
What do you think?
cc @Roasbeef
There was a problem hiding this comment.
I don't know if we can calculate the space dynamically here. We have to avoid a situation where createCommitDiff fails as it can cause the link to fail with ErrInternalError which causes us to send an Error to our peer and make them force close on us. This is made worse if a 3rd party can route HTLCs through us, causing both sides to hit the max_accepted_htlcs limit and then potentially triggering channel failure due to the custom records size. One solution may be to limit max_accepted_htlcs during funding and ensuring that if dynamic commitments are ever used, that this value isn't updated.
guggero
force-pushed
the
extract-part3-from-staging-branch
branch
3 times, most recently
from
6a0d859
to
f093d3d
Compare
In this commit, we start to use the new AuxSigner to obtain+verify aux sigs for all second level HTLCs. This is similar to the existing SigPool, but we'll only attempt to do this if the AuxSigner is present (won't be for most channels).
To make sure we attempt to read the results of the sig batches in the same order they're processed, we sort them _before_ submitting them to the batch processor. Otherwise it might happen that we try to read on a result channel that was never sent on because we aborted due to an error. We also use slices.SortFunc now which doesn't use reflection and might be slightly faster.
This commit adds an optional data parser that can inspect and in-place format custom data of certain RPC messages. We don't add an implementation of the interface itself, as that will be provided by external components when packaging up lnd as a bundle with other software.
guggero
force-pushed
the
extract-part3-from-staging-branch
branch
from
f093d3d
to
52e50d8
Compare
|
|
||
| // AuxHtlcDescriptor is a struct that contains the information needed to sign or | ||
| // verify an HTLC for custom channels. | ||
| type AuxHtlcDescriptor struct { |
There was a problem hiding this comment.
Nice, with this change then we aren't as blocked on some proposed refactors as we've separated concerns by using a new minimal struct.
There was a problem hiding this comment.
Yeah. The whole rebase on top of the first of these refactors just boiled down to making this struct. So not really that big of a deal, luckily.
| return err | ||
| } | ||
|
|
||
| // Extract TLV records from the extra data field. |
There was a problem hiding this comment.
Was this commit pulled into an earlier version: 78f31da ?
Or was it just re-worked from scratch?
There was a problem hiding this comment.
I had it reworked from scratch... So we kind of did the same thing twice unfortunately.
| @@ -4544,6 +4544,7 @@ func (lc *LightningChannel) computeView(view *HtlcView, | |||
| // need this to determine which HTLCs are dust, and also the final fee | |||
| // rate. | |||
| view.FeePerKw = commitChain.tip().feePerKw | |||
| view.NextHeight = nextHeight | |||
There was a problem hiding this comment.
Set, but not added in this commit?
There was a problem hiding this comment.
Yeah, we added it earlier in part 2. But looks like I forgot to squash this commit with an earlier one, so this remained.
Extracts part 3 from #8960.