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

Check for nonWitness UTXO or witness UTXO data in the psbt inputs #7529

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

ziggie1984
Copy link
Collaborator

@ziggie1984 ziggie1984 commented Mar 20, 2023

This relates to #7527, btcsuite/btcwallet#852, btcsuite/btcd#1964

I guess we also want to use the new InputsReadyToSign when using the remote signer setup and the SignPsbt function.

If so, then I still need to add some tests for FinalizePsbt and SignPsbt. I would introduce those tests at the rpc level ? Maybe in a new walletkit_server_test.go?

Change Description

Description of change / link to associated issue.

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@saubyk saubyk linked an issue Mar 20, 2023 that may be closed by this pull request
@ziggie1984 ziggie1984 force-pushed the psbt-bug branch 2 times, most recently from 5ba4a3e to 4d90cde Compare March 20, 2023 22:48
@ziggie1984 ziggie1984 changed the base branch from master to 0-16-1-staging March 21, 2023 08:27
@positiveblue
Copy link
Collaborator

Blocking changes in github.com/btcsuite/btcd and github.com/btcsuite/btcwallet merged 🎉

I guess we can move this out of drafts?

@saubyk saubyk added this to the v0.16.1 milestone Mar 28, 2023
@saubyk saubyk requested a review from guggero March 28, 2023 17:39
@ziggie1984
Copy link
Collaborator Author

ziggie1984 commented Mar 29, 2023

I think, I need a new tag for the btcwallet instance to bump the version accordingly ?

As a preliminary step, I replaced the btcwallet import with my import to not fail at the lint, this must be replaced by a new btcwallet tag IMO, so it just to pass all the checks and see whether the PR is ready to go.

Still one question: this PR does not add any tests and I am unsure whether we wanna add some tests at the rpc level (not quite sure how to do it but I would try to implement it). But otherwise this change is so small, maybe not worth it to add tests? Happy to hear your perspective

@ziggie1984 ziggie1984 force-pushed the psbt-bug branch 2 times, most recently from 09a7e19 to bb66bbf Compare March 29, 2023 09:18
@ziggie1984 ziggie1984 marked this pull request as ready for review March 29, 2023 09:20
@guggero
Copy link
Collaborator

guggero commented Mar 29, 2023

The way you updated the btcwallet dependency is fine for now. Because there might be more changes/updates necessary for 0.16.1 we usually only tag the main repository modules when we create a new lnd release (this is a bit different from submodules like btcutil/psbt which might be used by other projects independently).

Regarding tests, we could very easily add a test here: https://github.com/lightningnetwork/lnd/blob/master/itest/lnd_psbt_test.go#L1023
For example create a copy of the packet, remove the witness and non-witness UTXOs on one input and assert the correct error is returned.

@ziggie1984
Copy link
Collaborator Author

I added a simple testcase stripping all the input UTXO data and trying to sign the psbt.

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.

Test case looks good to me, thanks for adding that!
Just a comment around naming and error passing, other than that this seems to be ready.

lntest/rpc/wallet_kit.go Outdated Show resolved Hide resolved
itest/lnd_psbt_test.go Show resolved Hide resolved
itest/lnd_psbt_test.go Outdated Show resolved Hide resolved
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.

LGTM, thanks 🎉

@guggero
Copy link
Collaborator

guggero commented Mar 30, 2023

Needs a rebase.

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.

Thank you @ziggie1984 🌮

Everything looks good, we only need to rebase and it is ready to be merged 🚀

lntest/rpc/wallet_kit.go Show resolved Hide resolved
itest/lnd_psbt_test.go Outdated Show resolved Hide resolved
Use the new method in the psbt package InputsReadyToSign which
makes sure that each input has either nonWitness Utxo or
witness Utxo data specified.
Adds a testcase in the itest suite which tests that psbt
input data needs its corresponding UTXO data when signing.
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.

[bug] lnd crashes when not correct psbt data is used as input
4 participants