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

guggero · 2024-07-31T18:51:38Z

This is the cleaned up code of previous PRs that all went into the 0-19-staging branch and represents all the changes needed to get custom channels into lnd.

Reviewers please read

All commits have been pre-reviewed in previous PRs by at least one person.
We are looking for a second round review, mostly with focus on safety and stability when running with non-custom channels.

The idea is that reviewers get assigned to a certain range of commits within this PR. We decided to not do multiple PRs to keep rebase effort minimal (and so we have all context in a single PR).

The following parts exist (see commit message prefixes marking the start of a part):

-PART1--: General tapscript leaf awareness and aux leaf store
- Assigned reviewers: @ProofOfKeags
- 🟢 Status: DONE.
- EXTRACTED from this PR into: [custom channels 1/5]: extract PART1 from mega staging branch #9030
-PART2--: Custom records on HTLCs, SCID alias management, TLV traffic shaper
- Assigned reviewers: @bitromortac @ProofOfKeags
- 🟢 Status: DONE.
- EXTRACTED from this PR into: [custom channels 2/5]: Extract PART2 from mega staging branch #9049
-PART3--: Aux funding and RPC/CLI custom data integration
- Assigned reviewers: @Crypt-iQ @Roasbeef
- 🟢 Status: DONE.
- EXTRACTED from this PR into: [custom channels 3/5]: Extract PART3 from mega staging branch #9072
-PART4--: Invoice HTLC modifier and aux channel closer
- Assigned reviewers: @ellemouton @bhandras
- 🟢 Status: DONE.
- EXTRACTED from this PR into: [custom channels 4/5]: Extract PART4 from mega staging branch #9095
-PART5--: Force close sweeping
- Assigned reviewers: @yyforyongyu @Crypt-iQ
- 🛠️ Status: Ready for full review, review in progress...

Included changes from recent PRs to the `0-19-staging` branch

The following PRs HAVE been integrated either into a previous part or this last, 5th part:

Not included PRs to the `0-19-staging` branch

The following PRs have NOT yet been integrated into this PR (as they're currently in-flight):

lnwallet: aux signer batching fixes #9074 (only partially included, latest fixes need to be re-applied)
Add errors to protofsm.MsgEndpoint methods #9110

coderabbitai · 2024-07-31T18:51:42Z

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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
-- @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-07-31T18:51:57Z

Pull reviewers stats

Stats of the last 30 days for lnd:

User	Total reviews	Time to review	Total comments
guggero 🥇	14 ▀▀	2d 20h 48m	43 ▀▀
yyforyongyu 🥈	13 ▀▀	1d 13h 22m	22 ▀
bhandras 🥉	6 ▀	9h 54m	5
Roasbeef	6 ▀	6h 49m	9
ProofOfKeags	6 ▀	20d 6h 33m ▀▀▀	79 ▀▀▀
ellemouton	6 ▀	13d 18h 18m ▀▀	46 ▀▀
morehouse	4 ▀	10d 6h 18m ▀	15 ▀
GeorgeTsagk	3	17h 29m	3
bitromortac	2	17d 18h 49m ▀▀	8
carlaKC	2	20h 22m	10
ziggie1984	2	3d 20h 46m ▀	8
Webbdlee74	1	1d 6h 57m	0

Roasbeef · 2024-08-06T00:22:39Z

The following parts exist (see commit message prefixes marking the start of a part):

This prefix seems to have been dropped in the latest rebase?

guggero · 2024-08-06T09:19:55Z

This prefix seems to have been dropped in the latest rebase?

Oops, I haven't actually pushed it yet... It's now up to date and also rebased on top of the first refactor work in the channel state machine.

guggero · 2024-08-09T15:39:23Z

I addressed some of the TODOs and fixes the unit and integration test failures. Also added release notes that cover all new functionality of this PR.

I added a "Status" section to each part of this PR in the main PR body, will update that whenever status changes.
That means part 1, 3 and 5 are fully ready for review, @ProofOfKeags, @Crypt-iQ, @yyforyongyu.

yyforyongyu

Did a quick review on part 5, commits look pretty good. Will do a deep dive, meanwhile let me know if you need help patching unit tests and itest, especially for packages sweep and contractcourt.

lnwallet/channel.go

contractcourt/briefcase.go

lnwallet/interface.go

input/input.go

sweep/tx_input_set.go

ProofOfKeags

Ok so the biggest thing I want to challenge is the idea that we should use an Option for the tapscript root. IIRC we always have a tapscript root, irrespective of the presence of any aux leaves. We may not have any aux leaves and so I think it makes sense for that to be an option but we can probably save ourselves the effort and just compute the BIP86 root during initialization rather than computing it at the end and having to thread through optional arguments everywhere. The more cases we can eliminate earlier the better, I think. I'm open to some pushback here, though.

Other than that I have some SWE notes intended to simplify some of the interface/implementation code here.

NOTE: my review ONLY covers PART 1

lnwallet/chanfunding/interface.go

go.mod

channeldb/channel.go

lnwallet/chanfunding/canned_assembler.go

chainreg/chainregistry.go

contractcourt/chain_watcher.go

lnwallet/channel.go

Roasbeef · 2024-08-15T01:46:18Z

I pulled in the msg router specific comments from this PR into the original PR adding it: #8520

Ideally we can get this one in quickly, then rebase this over it.

Roasbeef · 2024-08-15T01:52:23Z

@ProofOfKeags

Ok so the biggest thing I want to challenge is the idea that we should use an Option for the tapscript root. IIRC we always have a tapscript root, irrespective of the presence of any aux leaves

We don't always have a tapscript root. When we do musig2 today for taproot channels, we don't have that extra tweak, so it's effectively BIP 86. This ensures that the only way the output can be spent is via the musig2 multi-sig with no script path.

Only with the new aux chan added here is the concept of a tapscript root for the funding output relevant.

We may not have any aux leaves and so I think it makes sense for that to be an option but we can probably save ourselves the effort and just compute the BIP86 root during initialization rather than computing it at the end and having to thread through optional arguments everywhere.

FWIW, the BIP 86 root is basically just nil, so we either need root != nil checks everywhere, or we thread through the option as is.

ProofOfKeags · 2024-08-16T23:19:23Z

FWIW, the BIP 86 root is basically just nil

Why don't we just compute the BIP86 value and drop it into that structure so that we can unify the types without needing the Option? I'm trying to push for symmetry/uniformity and I think there is natural symmetry here since there is always a value for this. I just think in the nil case you're computing it later?

guggero · 2024-08-19T09:25:44Z

Why don't we just compute the BIP86 value and drop it into that structure so that we can unify the types without needing the Option? I'm trying to push for symmetry/uniformity and I think there is natural symmetry here since there is always a value for this. I just think in the nil case you're computing it later?

The BIP86 script root is an empty byte slice: https://github.com/btcsuite/btcd/blob/master/txscript/taproot.go#L290
We can't put that into a chainhash.Hash, unless we use the all-zero hash. But then we need another distinction somewhere, which IMO is more dangerous.

guggero · 2024-08-19T15:27:00Z

Rebased after #8520 and addressed some first comments.

This commit was added to the 0-19-staging branch recently and therefore didn't make it into a previous part yet. So it's unrelated to the changes in this part but is required for the whole custom channel saga.

This way, it's possible to run induvidual tests to target failures.

This commit doesn't yet go all the way to modify all the other records quite yet.

This will be used by external callers to modify the way we resolve contracts on chain. For a given contract, we'll store an extra "blob", that will later be presented during the sweeping phase.

This'll be used to store the extra resolution information for the commitment outputs.

This way we can re-use it. We also make it slightly more generalized.

In this commit, we refactor all the other constructors for the input to use MakeBaseInput. We also add a new set of functional options as well. This'll be useful later on to ensure that new options are properly applied to all the input types.

We also update breachedOutput w/ the new API.

We convert it to use lnwallet.AddrWithKey, as in the future, knowing the internal key for an address will be useful.

In this commit, we add a new AuxSweeper interface. This'll take a set of inputs, and a change addr for the sweep transaction, then optionally return a new sweep output to be added to the sweep transaction. We also add a new NotifyBroadcast method. This'll be used to notify that we're _about_ to broadcast a sweeping transaction. The set of inputs is passed in, which allows the caller to prepare for the ultimate broadcast of the sweeping transaction. We also add ExtraTxOut to BumpRequest pass fees to NotifyBroadcast. This allows the callee to know the total fee of the sweeping transaction.

In this commit, we start to use the AuxSweeper (if present) to obtain a new extra change addr we should add to the sweeping transaction. With this, we'll take the set of inputs and our change addr, and then maybe gain a new change addr to add to the sweep transaction. The extra change addr will be treated as an extra required tx out, shared across all the relevant inputs. This'll also be used in NeedWalletInput to make sure that we add an extra input if needed to be able to pay for the change addr.

This is a hold over until the aux resolution is finalized for HTLC outputs.

For the upcoming aux sweeper integration, the internal key is needed for the call backs.

Similar to the sweeper, when we're about to make a new breach transaction, we ask the sweeper for a new change address, if it has one. Then when we go to publish, we notify broadcast.

This bit will be false by default in current production deployments.

We also add a sanity check to make sure they can't be signaled without the aux interfaces.

guggero · 2024-09-20T15:55:35Z

I didn't get to addressing all comments before my break. I resolved those that are addressed.

Roasbeef added the size/giga very large, specialized context, over 10000 lines label Aug 6, 2024

guggero force-pushed the 0-19-staging-rebased branch from 1341c89 to 008d29b Compare August 6, 2024 09:09

saubyk requested review from ProofOfKeags, yyforyongyu, ellemouton, bhandras and bitromortac August 6, 2024 17:01

saubyk assigned guggero Aug 6, 2024

guggero force-pushed the 0-19-staging-rebased branch 2 times, most recently from 4900438 to 529fd75 Compare August 8, 2024 12:20

saubyk requested a review from Crypt-iQ August 8, 2024 15:09

guggero force-pushed the 0-19-staging-rebased branch from 529fd75 to 94230af Compare August 9, 2024 15:35

guggero force-pushed the 0-19-staging-rebased branch from 94230af to 626d901 Compare August 9, 2024 15:41

yyforyongyu reviewed Aug 12, 2024

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

contractcourt/briefcase.go Show resolved Hide resolved

lnwallet/interface.go Outdated Show resolved Hide resolved

input/input.go Show resolved Hide resolved

sweep/tx_input_set.go Outdated Show resolved Hide resolved

ProofOfKeags requested changes Aug 14, 2024

View reviewed changes

Roasbeef mentioned this pull request Aug 15, 2024

[2/4] - peer: add new abstract message router #8520

Merged

guggero mentioned this pull request Aug 15, 2024

tracking: remaining cleanup to address before merging 0-19-staging branch into master #8769

Open

10 tasks

ProofOfKeags added the custom chans label Aug 15, 2024

guggero force-pushed the 0-19-staging-rebased branch from 626d901 to b6a20c7 Compare August 19, 2024 15:26

guggero changed the title ~~[custom channels]: merge 0-19-staging branch into master~~ [custom channels]: merge custom channel staging branch into master Aug 20, 2024

GeorgeTsagk and others added 26 commits September 20, 2024 17:06

routing: add htlcAmt to PaymentBandwidth method of TlvTrafficShaper

84247fa

This commit was added to the 0-19-staging branch recently and therefore didn't make it into a previous part yet. So it's unrelated to the changes in this part but is required for the whole custom channel saga.

lnwire: modify TestLightningWireProtocol to use sub-tests

873a6e2

This way, it's possible to run induvidual tests to target failures.

contractcourt: convert taprootBriefcase to use new tlv record type

069d8c0

This commit doesn't yet go all the way to modify all the other records quite yet.

lnwallet: add new aux resolver interface

b3621e4

This will be used by external callers to modify the way we resolve contracts on chain. For a given contract, we'll store an extra "blob", that will later be presented during the sweeping phase.

contractcourt: add CommitBlob to taprootBriefcase

c146ca6

This'll be used to store the extra resolution information for the commitment outputs.

lnwallet+peer: move internalKeyForAddr to lnwallet package

e069d99

This way we can re-use it. We also make it slightly more generalized.

input: add ResolutionBlob method to inputKit

16bf38b

We also update breachedOutput w/ the new API.

contractcourt: set resolution blob in commitSweepResolver

efbe593

server+sweep: convert GenSweepScript to use new addr type

85ae220

We convert it to use lnwallet.AddrWithKey, as in the future, knowing the internal key for an address will be useful.

contractcourt: pause resolution for HTLCs w/ custom records

4fcd502

This is a hold over until the aux resolution is finalized for HTLC outputs.

multi: hook up new aux interfaces

62797de

contractcourt: update GenSweepScript to return internal key

d0b102c

For the upcoming aux sweeper integration, the internal key is needed for the call backs.

contractcourt: update makeBreachedOutput to accept resolution blob

1461d8f

contractcourt: integration aux sweeper to breach arb

8a4af3c

Similar to the sweeper, when we're about to make a new breach transaction, we ask the sweeper for a new change address, if it has one. Then when we go to publish, we notify broadcast.

lnwire: add new taproot chans overlay feature bit

d53438a

feature: add awareness of new taproot chans overlay feature bit

79426b1

This bit will be false by default in current production deployments.

lnwallet: add awareness of taproot overlay chan type to reservations

2afa10f

funding: add chan type awareness for new taproot chans overlay

4eb0d6f

lnrpc: add SIMPLE_TAPROOT_OVERLAY feature bit

fe4e4ec

rpc+funding: add taproot overlay as RPC chan type

fabf087

lncfg: add new config option for taproot overlay chans

fdca15d

lnd: signal taproot overlay chans based on config

5304f58

We also add a sanity check to make sure they can't be signaled without the aux interfaces.

docs: update release notes

d1827a4

guggero force-pushed the 0-19-staging-rebased branch from 53cea84 to d1827a4 Compare September 20, 2024 15:54

saubyk assigned Roasbeef and unassigned guggero Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

guggero commented Jul 31, 2024 •

edited

Loading

coderabbitai bot commented Jul 31, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

github-actions bot commented Jul 31, 2024

Roasbeef commented Aug 6, 2024

guggero commented Aug 6, 2024

guggero commented Aug 9, 2024

yyforyongyu left a comment

ProofOfKeags left a comment •

edited

Loading

Roasbeef commented Aug 15, 2024

Roasbeef commented Aug 15, 2024 •

edited

Loading

ProofOfKeags commented Aug 16, 2024

guggero commented Aug 19, 2024

guggero commented Aug 19, 2024

guggero commented Sep 20, 2024

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

Are you sure you want to change the base?

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

Conversation

guggero commented Jul 31, 2024 • edited Loading

Reviewers please read

Included changes from recent PRs to the 0-19-staging branch

Not included PRs to the 0-19-staging branch

coderabbitai bot commented Jul 31, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

github-actions bot commented Jul 31, 2024

Pull reviewers stats

Roasbeef commented Aug 6, 2024

guggero commented Aug 6, 2024

guggero commented Aug 9, 2024

yyforyongyu left a comment

Choose a reason for hiding this comment

ProofOfKeags left a comment • edited Loading

Choose a reason for hiding this comment

Roasbeef commented Aug 15, 2024

Roasbeef commented Aug 15, 2024 • edited Loading

ProofOfKeags commented Aug 16, 2024

guggero commented Aug 19, 2024

guggero commented Aug 19, 2024

guggero commented Sep 20, 2024

guggero commented Jul 31, 2024 •

edited

Loading

Included changes from recent PRs to the `0-19-staging` branch

Not included PRs to the `0-19-staging` branch

coderabbitai bot commented Jul 31, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

ProofOfKeags left a comment •

edited

Loading

Roasbeef commented Aug 15, 2024 •

edited

Loading