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.

Sign up for GitHub

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

Jump to bottom

Taproot: integrate btcec/v2 and btcwallet changes to support Taproot key spend paths in lnd's wallet #6263

Merged
merged 9 commits into from Mar 25, 2022
Merged

Taproot: integrate btcec/v2 and btcwallet changes to support Taproot key spend paths in lnd's wallet #6263

merged 9 commits into from Mar 25, 2022

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Feb 14, 2022 •
edited

End-to-end integration of all the changes required to get the Taproot functionality into lnd.

Depends on btcsuite/btcd#1787, btcsuite/btcwallet#792 and #6285.

Fixes #6266.
Fixes #6267.
Fixes #6330.

guggero
guggero commented Feb 14, 2022
View reviewed changes
lnwire/signature_test.go Outdated Show resolved Hide resolved
guggero
guggero commented Feb 14, 2022
View reviewed changes
lnwire/signature_test.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added this to the v0.15.0 milestone Feb 14, 2022
@Roasbeef Roasbeef added this to In progress in v0.15.0-beta via automation Feb 14, 2022
@Roasbeef Roasbeef moved this from In progress to Review in progress in v0.15.0-beta Feb 14, 2022
@Roasbeef Roasbeef added P1 MUST be fixed or reviewed taproot labels Feb 14, 2022
Roasbeef
Roasbeef reviewed Feb 14, 2022
View reviewed changes
lnwire/signature_test.go Outdated Show resolved Hide resolved
@guggero guggero mentioned this pull request Feb 21, 2022
Merged
@guggero
Copy link
Collaborator Author

guggero commented Feb 21, 2022

I extracted most of the diff into #6285 which doesn't depend on any unmerged btcd PRs and can be merged whenever.

@guggero guggero force-pushed the schnorr-taproot-signet branch 3 times, most recently from a80a734 to 58a3940 Compare February 21, 2022 19:27
@guggero guggero mentioned this pull request Feb 22, 2022
Merged
Roasbeef
Roasbeef reviewed Feb 25, 2022
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Thought I posted these comments...

cmd/lncli/commands.go Show resolved Hide resolved
autopilot/graph.go Outdated Show resolved Hide resolved
lnwire/signature_test.go Outdated Show resolved Hide resolved
lnwire/signature_test.go Outdated Show resolved Hide resolved
brontide/noise.go Outdated Show resolved Hide resolved
input/signdescriptor.go Outdated Show resolved Hide resolved
input/size.go Outdated Show resolved Hide resolved
lnrpc/lightning.proto Show resolved Hide resolved
lnwallet/btcwallet/signer.go Outdated Show resolved Hide resolved
lnwallet/btcwallet/signer.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef mentioned this pull request Feb 25, 2022
Merged
5 tasks
@lightninglabs-deploy
Copy link

@guggero, remember to re-request review from reviewers when ready

@guggero guggero force-pushed the schnorr-taproot-signet branch 5 times, most recently from 443813e to 8964101 Compare March 8, 2022 13:36
@guggero
Copy link
Collaborator Author

guggero commented Mar 8, 2022

Rebased and addressed all current comments. Also made the itest a bit less confusing and bit more explicit.

Not sure if the new AssembleTapscriptSpendWitness RPC should be added in this PR or a follow-up one.

@positiveblue positiveblue mentioned this pull request Mar 22, 2022
Merged
3 tasks
@guggero guggero mentioned this pull request Mar 23, 2022
Merged
positiveblue
positiveblue approved these changes Mar 24, 2022
View reviewed changes
Copy link
Collaborator

@positiveblue positiveblue left a comment

Choose a reason for hiding this comment

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

LGTM, I think we are getting close to have it ready to be merged 🎉. Release notes are missing but functionality wise I think it is complete.

@guggero
Copy link
Collaborator Author

guggero commented Mar 24, 2022

Added release notes.

@guggero
multi: use prev output fetcher where possible
f130edd
@guggero
multi: add p2tr keyspend wallet support
c73cf03
@guggero
input+signer: test wallet script import
108f49f
@guggero
itest: assert P2TR on-chain fund recovery
5109c55
@guggero
multi: add p2tr tapscript signing capabilities
78db46b
@guggero
multi: add p2tr tapscript key path signing capabilities
ef98f2d
@guggero
multi: fix and test v1 output spend ntfns
3b5585c 
Because Taproot key spend only spends don't allow us to re-construct the
spent pkScript from the witness alone, we cannot support registering
spend notifications for v1 pkScripts only. We instead require the
outpoint to be specified. This commit makes it possible to only match by
outpoint and also adds an itest for it.
@guggero
docs: add release notes
f500892
@guggero
Copy link
Collaborator Author

guggero commented Mar 24, 2022

Fixed itests too. Ready to be merged IMO. How did your mainnet tests come along, @Roasbeef?

v0.15.0-beta automation moved this from Review in progress to Reviewer approved + active testing Mar 25, 2022
Roasbeef
Roasbeef approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐞

@Roasbeef Roasbeef merged commit dceb101 into lightningnetwork:master Mar 25, 2022
v0.15.0-beta automation moved this from Reviewer approved + active testing to Done Mar 25, 2022
@guggero guggero deleted the schnorr-taproot-signet branch March 28, 2022 06:36
@MatulefM MatulefM mentioned this pull request Apr 1, 2022
Merged
Roasbeef
Roasbeef reviewed Apr 22, 2022
View reviewed changes
lntest/itest/lnd_taproot_test.go
Output: utxoInfo[0],
InputIndex: 0,
KeyDesc: keyDesc,
SingleTweak: dummyKeyTweak[:],
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually need to be set?

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok it's just making sure that combination work as well. Though I don't think it would ever really be used....

@Sjors
Copy link

Sjors commented May 9, 2022

When I create a new (signet) wallet and fund a taproot address, my attempt to open a channel fails with:

[lncli] rpc error: code = Unknown desc = unsupported address type: 51200f5a550....8b

If that's intentional, maybe newaddress should have a warning not to use the p2tr just yet.

Fortunately coin selection seems to prefer non-taproot addresses, so I was able to just create a p2wpkh address, fund that and then open a channel with a sufficiently low balance.

Using master @ 10f7213

@Roasbeef Roasbeef mentioned this pull request May 9, 2022
Closed
@Roasbeef
Copy link
Member

Roasbeef commented May 9, 2022

Thx for the report @Sjors, I converted you comment into an issue so we can track it properly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@positiveblue positiveblue positiveblue approved these changes

Assignees
No one assigned
Labels
P1 MUST be fixed or reviewed taproot
Projects
No open projects
v0.15.0-beta
Done
Milestone
v0.15.0
6 participants
@guggero @lightninglabs-deploy @dstadulis @Sjors @Roasbeef @positiveblue