Skip to content

[custom channels 2/3]: add aux leaf signer #885

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

Merged
merged 3 commits into from
May 24, 2024

Conversation

guggero
Copy link
Member

@guggero guggero commented Apr 17, 2024

@guggero guggero added this to the v0.4 milestone Apr 17, 2024
@guggero guggero force-pushed the custom-channels-integration-signing branch 2 times, most recently from 5dfe9be to e34ecf6 Compare April 23, 2024 08:55
@guggero guggero force-pushed the custom-channels-integration branch from 33067d0 to d7b3b87 Compare April 23, 2024 08:55
@dstadulis dstadulis modified the milestones: v0.4, ⚡️PoC⚡️ Apr 23, 2024
@guggero guggero force-pushed the custom-channels-integration-signing branch from e34ecf6 to 496a48b Compare April 25, 2024 15:51
@guggero guggero force-pushed the custom-channels-integration branch from d7b3b87 to f7b587a Compare April 25, 2024 15:51
@guggero guggero force-pushed the custom-channels-integration branch 2 times, most recently from b8243ff to dd47bfd Compare May 13, 2024 15:59
@guggero guggero force-pushed the custom-channels-integration-signing branch 2 times, most recently from 6245b6f to 877f08a Compare May 14, 2024 11:58
@guggero guggero force-pushed the custom-channels-integration branch from dd47bfd to c7135e7 Compare May 14, 2024 11:58
@guggero guggero marked this pull request as ready for review May 14, 2024 11:58
@guggero guggero requested review from ffranr and GeorgeTsagk May 14, 2024 11:58
@guggero guggero force-pushed the custom-channels-integration branch from c7135e7 to bbcd2fb Compare May 17, 2024 14:48
@guggero guggero force-pushed the custom-channels-integration-signing branch from 0e33120 to 80b004a Compare May 17, 2024 14:48
@guggero guggero changed the title multi: add aux leaf signer [custom channels 2/3]: add aux leaf signer May 17, 2024
@guggero guggero force-pushed the custom-channels-integration branch from bbcd2fb to f49be36 Compare May 20, 2024 16:31
@guggero guggero force-pushed the custom-channels-integration-signing branch from 80b004a to eb7bb32 Compare May 20, 2024 16:31
@dstadulis dstadulis modified the milestones: ⚡️PoC⚡️, v0.4 May 20, 2024
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Some simple CI lint errors, otherwise 👍


// SigHashType is the sigHash type that was used to create the
// signature.
SigHashType tlv.RecordT[tlv.TlvType2, uint32]
Copy link
Contributor

Choose a reason for hiding this comment

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

Use txscript.SigHashType here instead of uint32?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would lead to panic: unknown primitive type: *txscript.SigHashType [recovered]. We either need to have types that have a Record() method or are from a list of defined primitive types (no type aliases allowed).

@guggero guggero force-pushed the custom-channels-integration-signing branch from eb7bb32 to e47bd52 Compare May 21, 2024 12:14
@guggero guggero force-pushed the custom-channels-integration branch from f49be36 to a558ec0 Compare May 21, 2024 12:14
@guggero guggero force-pushed the custom-channels-integration-signing branch from e47bd52 to 2fc91ba Compare May 21, 2024 12:56
@guggero guggero force-pushed the custom-channels-integration branch from a558ec0 to 953f49c Compare May 21, 2024 12:56
Base automatically changed from custom-channels-integration to main May 21, 2024 18:32
@guggero guggero force-pushed the custom-channels-integration-signing branch 2 times, most recently from 516c4d4 to 370a717 Compare May 23, 2024 05:06
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

Lgtm 🥬

@@ -423,6 +429,7 @@ func genServerConfig(cfg *Config, cfgLogger btclog.Logger,
UniverseQueriesBurst: cfg.Universe.UniverseQueriesBurst,
RfqManager: rfqManager,
AuxLeafCreator: auxLeafCreator,
AuxLeafSigner: auxLeafSigner,
Copy link
Member

Choose a reason for hiding this comment

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

nit: why not follow lnd codebase and use a "maybe" fn.Option[AuxLeafSigner] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're always creating and starting these components. Since they're pretty light-weight, it IMO doesn't make sense to only conditionally create and start them for the custom channel specific use case, as that would mean we'd need to deal with more .WhenSome() and .IsSome() constructs.

But maybe something to consider in this regard is to only start the HTLC and invoice acceptors if they're really needed, since both of them can only be registered once in lnd. So if we always occupy that single "slot" with tapd even if you don't use custom channels, that could create issues for some users. I'll look into that in the invoice PR.

guggero added 3 commits May 24, 2024 08:44
This test is helpful for inspecting/debugging a vPSBT packet from a unit
or integration test.
@guggero guggero force-pushed the custom-channels-integration-signing branch from 370a717 to feea448 Compare May 24, 2024 06:51
@guggero guggero enabled auto-merge May 24, 2024 06:59
@guggero guggero added this pull request to the merge queue May 24, 2024
Merged via the queue into main with commit 60613f8 May 24, 2024
14 checks passed
@guggero guggero deleted the custom-channels-integration-signing branch May 24, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants