Skip to content
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

watchtower+refactor: CommitmentType and JusticeKit interface #7736

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

ellemouton
Copy link
Collaborator

@ellemouton ellemouton commented May 31, 2023

This PR is a pure refactor of some watchtower code. Currently, the
tower code is littered with many if isAnchor() {..} else {} blocks
and this will become even more hairy once we have to cater for
taproot channels. With that in mind, this PR does a few things so that
adding support for taproot channels can be a relatively small diff:

  1. A new CommitmentType enum is added which will hide some details
    about the inputs of the justice transaction.
  2. The JusticeKit is converted from a struct to an interface with various
    methods on it. Two implementations are added: legacyJusticeKit and
    anchorJusticeKit both of which use the same encoding.

This is a pre-requisite for #7733

@ellemouton ellemouton added the code health Related to code commenting, refactoring, and other non-behaviour improvements label May 31, 2023
@ellemouton ellemouton self-assigned this May 31, 2023
@saubyk saubyk added this to the v0.17.1 milestone Jun 15, 2023
@saubyk saubyk added the P1 MUST be fixed or reviewed label Aug 8, 2023
@saubyk saubyk modified the milestones: High Priority, v0.18.0 Aug 8, 2023
@ellemouton ellemouton force-pushed the towerInterfaces branch 3 times, most recently from fc39a66 to a90576f Compare August 29, 2023 07:21
@ellemouton ellemouton force-pushed the towerInterfaces branch 3 times, most recently from 7e1acea to a851935 Compare November 28, 2023 12:08
@saubyk saubyk requested review from guggero and bitromortac December 7, 2023 15:21
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, LGTM 🎉
Just a couple of nits, very clean and well structured 💯

watchtower/blob/commitments.go Outdated Show resolved Hide resolved
watchtower/blob/commitments.go Outdated Show resolved Hide resolved
watchtower/blob/commitments.go Show resolved Hide resolved
watchtower/blob/commitments.go Show resolved Hide resolved
watchtower/blob/commitments.go Outdated Show resolved Hide resolved
watchtower/blob/commitments.go Show resolved Hide resolved
watchtower/blob/justice_kit.go Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the speedy review @guggero 🎉 🚗

watchtower/blob/commitments.go Show resolved Hide resolved
watchtower/blob/justice_kit.go Outdated Show resolved Hide resolved
watchtower/blob/commitments.go Show resolved Hide resolved
@lightninglabs-deploy
Copy link

@bitromortac: review reminder

@ellemouton
Copy link
Collaborator Author

pushed a tiny update here to incorporate this comment from the follow up PR.

No logic changes - just changing the JusticeKit API to return types like txscript.PkScript and wire.TxWitness instead of []byte and [][]byte

Copy link
Collaborator

@bitromortac bitromortac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, awesome refactor PR 🔥! Only have some nits and a question regarding the JusticeKit interface.

watchtower/blob/commitments.go Outdated Show resolved Hide resolved
watchtower/blob/commitments.go Show resolved Hide resolved
watchtower/blob/justice_kit.go Outdated Show resolved Hide resolved
watchtower/blob/justice_kit.go Show resolved Hide resolved
In this commit a new enum, CommitmentType, is introduced and initially
there are 3 CommitmentTypes: Legacy, LegacyTweakless and Anchor.

Then, various methods are added to `CommitmentType`. This allows us to
remove a bunch of "if-else" chains from the `wtclient` and `lookout`
code. This will also make things easier to extend when a new commitment
type (like Taproot) is added.
In preparation for the next commit which will introduce the `JusticeKit`
interface, here we just move the code related to building the actual
justice kit packet into a separate file.
In this commit, we convert the `JusticeKit` struct to an interface.
Then, we add two implementations of that interface:
1) The `legacyJusticeKit` which implements all the methods of
   `JusticeKit`
2) The `anchorJusticKit` which wraps the `legacyJusticeKit` and just
   re-implements the `ToRemoteOutputSpendInfo` method since.
@ellemouton ellemouton merged commit 695bfc8 into lightningnetwork:master Jan 4, 2024
23 of 25 checks passed
@ellemouton ellemouton deleted the towerInterfaces branch January 4, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Related to code commenting, refactoring, and other non-behaviour improvements P1 MUST be fixed or reviewed refactoring watchtower
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants