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

multi: use taproot errwhere applicable for change/delivery/tower addresses #6810

Merged
merged 8 commits into from
Aug 12, 2022

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Aug 9, 2022

With this PR, we'll start to use taproot addresses by default when: sweeping all funds from the wallet, funding channels, and sending blobs to watch towers.

Fixes #6666

@Roasbeef Roasbeef added this to the v0.15.1 milestone Aug 9, 2022
@Roasbeef Roasbeef requested a review from guggero August 9, 2022 02:19
@Roasbeef
Copy link
Member Author

Roasbeef commented Aug 9, 2022

@guggero I fixed that failing tower test by using a taproot address in the test itself. It was using a p2pkh address before.

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.

There seem to be more fee estimation issues hidden somewhere in the funding flow.
I see this error in the basic_funding_flow itest: unmatched backend error: -26: TX rejected: transaction 370af31500a362f8b0e2b097ed33dc2de533b0a917316934097cf00272c30c11 has insufficient priority (2.9268292682926827e+06 <= 5.76e+07)

We seem to assume P2WKH outputs in several places:

watchtower/wtclient/backup_task.go Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member Author

Alrighty, I pushed up a set of fresh commits that should cover all the outstanding cases. I broke up the commits to make things a bit easier to follow. There were a still areas where we also assumed the expected dust output that should be fixed.

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.

Nice, LGTM 🎉

Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

minor comment/qs

watchtower/lookout/justice_descriptor.go Outdated Show resolved Hide resolved
watchtower/wtclient/backup_task.go Outdated Show resolved Hide resolved
watchtower/wtclient/backup_task.go Outdated Show resolved Hide resolved
watchtower/wtpolicy/policy.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef force-pushed the taproot-errwhere branch 2 times, most recently from ed0d0c5 to 6093c75 Compare August 12, 2022 00:09
This change covers sweep addresses for: the breach arbitrator, and watch
tower clients.
In this commit, we modify the watch tower to use P2TR addrs for just
about anything sweep related.

One eye sore in this diff are the changes to
`backup_task_internal_test.go`. All the values are hard coded, and now
either differ by a value of 48, or needed to be modified to account for
the new assumptions propagated to rewards values and fees.
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

@Roasbeef
Copy link
Member Author

lgtm if sign_psbt here (https://github.com/lightningnetwork/lnd/runs/7798107595?check_suite_focus=true) is an unrelated flake

Investigated offline, appears to be a flake unique to the bitcoind backend. Landing this then will make an issue to investigate. Seems like the test isn't waiting long enough for the wallet to fully recognize the coins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sweep+funding: default to P2TR outputs for all change addresses and new addrs
3 participants