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

wallet: add new PSBT funding and finalizing methods #711

Merged
merged 6 commits into from
Sep 4, 2020

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Jun 19, 2020

Depends on btcsuite/btcutil#178

Adds the methods FundPsbt which can perform coin selection and/or fee estimation to fund outputs in a PSBT and FinalizePsbt which signs known inputs of a PSBT packet.

We also add FetchInputInfo and ComputeInputScript which previously resided in lnd to make them available to the PSBT signing code and other users of btcwallet.

go.mod Outdated Show resolved Hide resolved
wallet/createtx_test.go Outdated Show resolved Hide resolved
wallet/signer.go Outdated Show resolved Hide resolved
wallet/signer_test.go Show resolved Hide resolved
wallet/psbt.go Outdated Show resolved Hide resolved
wallet/psbt.go Show resolved Hide resolved
wallet/psbt.go Show resolved Hide resolved
wallet/psbt.go Outdated Show resolved Hide resolved
wallet/psbt.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
wallet/utxos.go Outdated Show resolved Hide resolved
wallet/psbt.go Outdated Show resolved Hide resolved
wallet/psbt.go Outdated

// We can only sign witness inputs. Everything else must already
// have been signed or the finalize step later will fail.
if in.WitnessUtxo == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this cover nested P2WKH?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, from the perspective of a signer, a NP2WKH looks exactly like a native SegWit output except that FinalScriptSig is set to a non-empty slice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I'm missing something, but I don't see that being done here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, you're right. I assigned the script to WitnessScript instead of FinalScriptSig. The added test case caught that now. Thanks!

wallet/psbt.go Show resolved Hide resolved
@guggero guggero force-pushed the psbt-signing branch 2 times, most recently from 0b0fc85 to a797c79 Compare June 25, 2020 13:19
@guggero guggero requested a review from Roasbeef June 25, 2020 13:20
wallet/psbt.go Outdated

// We can only sign witness inputs. Everything else must already
// have been signed or the finalize step later will fail.
if in.WitnessUtxo == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I'm missing something, but I don't see that being done here.

wallet/psbt_test.go Outdated Show resolved Hide resolved
wallet/psbt.go Outdated Show resolved Hide resolved
wallet/psbt.go Show resolved Hide resolved
wallet/psbt.go Show resolved Hide resolved
wallet/psbt.go Show resolved Hide resolved
wallet/psbt.go Outdated Show resolved Hide resolved
@kornpow
Copy link

kornpow commented Jul 10, 2020

Can you show me how to us this PR with my current LND, so I can test PSBT funding with latest electrum releases?

@guggero
Copy link
Collaborator Author

guggero commented Jul 13, 2020

See lightningnetwork/lnd#4389, that adds PSBT signing to lnd. Though you won't be able to use PSBTs created with Electrum, because they already added the bugfix for CVE-2020-14199 (see spesmilo/electrum#6257 (comment)). I'm working on a fix for that in btcutil/psbt as well.

@guggero
Copy link
Collaborator Author

guggero commented Jul 20, 2020

I updated the PR to include the compatibility fix for patched HW wallets: btcsuite/btcutil#178

I also made sure we always include the non-witness UTXO in our PSBTs as well to make sure we produce packets that can be signed by HW wallets with the newest firmware.

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 🎷

Ready to land once the btcutil dep is updated.

@guggero
Copy link
Collaborator Author

guggero commented Aug 27, 2020

Dependency updated.

@kornpow
Copy link

kornpow commented Aug 27, 2020

Can’t wait to test this out in LND!

@Roasbeef Roasbeef merged commit 2c5947a into btcsuite:master Sep 4, 2020
@guggero guggero deleted the psbt-signing branch September 4, 2020 07:11
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.

4 participants