[custom channels 3/5]: Extract PART3 from mega staging branch by guggero · Pull Request #9072 · lightningnetwork/lnd

guggero · 2024-09-09T09:32:28Z

Extracts part 3 from #8960.

coderabbitai · 2024-09-09T09:32:38Z

Important

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-- I pushed a fix in commit <commit_id>, please review it.
-- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
-- @coderabbitai generate unit testing code for this file.
-- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
-- @coderabbitai generate interesting stats about this repository and render them as a table.
-- @coderabbitai read src/utils.ts and generate unit testing code.
-- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
-- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

github-actions · 2024-09-09T10:30:39Z

Pull reviewers stats

Stats of the last 30 days for lnd:

User	Total reviews	Time to review	Total comments
guggero 🥇	19 ▀▀▀	12h 47m	34 ▀▀
yyforyongyu 🥈	14 ▀▀	15h	33 ▀
Roasbeef 🥉	8 ▀	5d 2h 26m ▀	18 ▀
ellemouton	5 ▀	6h 51m	13 ▀
Crypt-iQ	4 ▀	13h 28m	7
bitromortac	4 ▀	6d 20h 16m ▀	50 ▀▀
ziggie1984	4 ▀	5d 13h 36m ▀	17 ▀
ProofOfKeags	3	5d 6h 54m ▀	38 ▀▀
bhandras	2	5h 58m	0
hieblmi	2	1h 31m	2
alexbosworth	1	3d 21h 20m ▀	1
aakselrod	1	4d 2h 7m ▀	2
dstadulis	1	5d 7h 41m ▀	1
morehouse	1	1h 5m	1
GeorgeTsagk	1	11d 15h 22m ▀▀	3
carlaKC	1	17h 11m	0
MPins	1	5d 8h 9m ▀	2

guggero · 2024-09-12T16:17:31Z

Requires a spicy rebase now that #8981 is in...

funding/manager.go

Crypt-iQ · 2024-09-10T18:56:38Z

funding/manager.go

 			}),
 		)
+
+		err = fn.MapOptionZ(


These both call AuxFundingController.ChannelReady but are two slightly different states:

handleChannelReady means we've received channel_ready, we still need to send ours potentially

handleChannelReadyReceived means we've sent + received channel_ready
Not sure if that matters at all for the way the funding controller is using these notifications.

Well spotted! But fortunately it doesn't really matter for the current use case. Added a comment to make it more clear.

lnwire/commit_sig.go

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.

Crypt-iQ · 2024-09-18T14:55:13Z

lnwallet/channel.go

+	// 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


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

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

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.

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.

Crypt-iQ

LGTM and size change to come when custom channels no longer experimental

Roasbeef

LGTM 🪕

Roasbeef · 2024-09-18T22:53:01Z

lnwallet/aux_signer.go

+
+// AuxHtlcDescriptor is a struct that contains the information needed to sign or
+// verify an HTLC for custom channels.
+type AuxHtlcDescriptor struct {


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.

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.

Roasbeef · 2024-09-18T22:56:19Z

lnwire/commit_sig.go

-		return err
-	}
-
+	// Extract TLV records from the extra data field.


Was this commit pulled into an earlier version: 78f31da ?

Or was it just re-worked from scratch?

I had it reworked from scratch... So we kind of did the same thing twice unfortunately.

lnwallet/channel.go

Roasbeef · 2024-09-18T23:02:11Z

lnwallet/channel.go

 	// need this to determine which HTLCs are dust, and also the final fee
 	// rate.
 	view.FeePerKw = commitChain.tip().feePerKw
+	view.NextHeight = nextHeight


Set, but not added in this commit?

Yeah, we added it earlier in part 2. But looks like I forgot to squash this commit with an earlier one, so this remained.

guggero added no-changelog custom chans labels Sep 9, 2024

guggero added this to the v0.18.4 milestone Sep 9, 2024

guggero requested review from Crypt-iQ and Roasbeef September 9, 2024 09:32

guggero self-assigned this Sep 9, 2024

guggero mentioned this pull request Sep 11, 2024

lnwallet: aux signer batching fixes #9074

Merged

8 tasks

guggero force-pushed the extract-part3-from-staging-branch branch 3 times, most recently from 376ccb4 to 5e33608 Compare September 11, 2024 15:36

guggero mentioned this pull request Sep 12, 2024

[custom channels 5/5]: merge custom channel staging branch into master #8960

Merged

Crypt-iQ reviewed Sep 12, 2024

View reviewed changes

guggero force-pushed the extract-part3-from-staging-branch branch 2 times, most recently from 2199ea9 to 49a8371 Compare September 13, 2024 15:57

guggero requested a review from Crypt-iQ September 13, 2024 16:12

Roasbeef and others added 12 commits September 18, 2024 10:25

lnwallet: add new AuxFundingDesc struct

116a643

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.

lnwallet: use AuxFundingDesc to populate all custom chan info

72beb79

With this commit, we'll now populate all the custom channel information within the OpenChannel and ChannelCommitment structs.

config+serer: add AuxFundingController as top level cfg option

65f54cb

lnwallet: add TaprootInternalKey method to ShimIntent

7144a1c

If this is a taproot channel, then we'll return the internal key which'll be useful to callers.

lnwallet: for PsbtIntent return the internal key in the POutput

bed4562

We also add a new assertion to the itests to ensure the field is being properly set.

multi: add tapscript root to gossip message

5c854a2

funding: inform aux controller about channel ready/finalize

0b64b80

lnwallet: add new AuxSigner interface to mirror SigPool

aa0c680

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.

lnwallet: allow read-only access to HtlcView's HTLCs

953fb07

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 and others added 3 commits September 18, 2024 10:25

lnwallet: clarify usage of cancel and response channels

f52a163

lnwallet: add WithAuxSigner option to channel

1e85c50

lnwire: add custom records field to type CommitSig

bd84fd2

guggero force-pushed the extract-part3-from-staging-branch branch 2 times, most recently from 3dc1947 to 8cb20e7 Compare September 18, 2024 10:03

Crypt-iQ reviewed Sep 18, 2024

View reviewed changes

guggero force-pushed the extract-part3-from-staging-branch branch 3 times, most recently from 6a0d859 to f093d3d Compare September 18, 2024 16:06

Roasbeef and others added 6 commits September 18, 2024 19:04

lnrpc+rpcserver: add and populate custom channel data

5e1a98c

channeldb: add NextHeight, fix formatting

cdc3a4a

htlcswitch: override amount check on custom records

52e50d8

guggero force-pushed the extract-part3-from-staging-branch branch from f093d3d to 52e50d8 Compare September 18, 2024 17:08

Crypt-iQ approved these changes Sep 18, 2024

View reviewed changes

Roasbeef approved these changes Sep 18, 2024

View reviewed changes

guggero merged commit cdad5d9 into master Sep 19, 2024

guggero deleted the extract-part3-from-staging-branch branch September 19, 2024 07:22

guggero mentioned this pull request Sep 20, 2024

Limit number of HTLCs in custom channel lightninglabs/taproot-assets#1132

Merged

This was referenced Oct 11, 2024

release: create branch for v0.18.4-beta.rc1 #9174

Closed

release: create branch for v0.18.4-beta.rc1 #9183

Merged

Conversation

guggero commented Sep 9, 2024

Uh oh!

coderabbitai bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

github-actions bot commented Sep 9, 2024

Pull reviewers stats

Uh oh!

guggero commented Sep 12, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coderabbitai bot commented Sep 9, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

Crypt-iQ Sep 18, 2024 •

edited

Loading