-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
5ba4a3e
to
4d90cde
Compare
Blocking changes in I guess we can move this out of drafts? |
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 |
09a7e19
to
bb66bbf
Compare
The way you updated the Regarding tests, we could very easily add a test here: https://github.com/lightningnetwork/lnd/blob/master/itest/lnd_psbt_test.go#L1023 |
I added a simple testcase stripping all the input UTXO data and trying to sign the psbt. |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks 🎉
Needs a rebase. |
There was a problem hiding this 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 🚀
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.
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 theSignPsbt
function.If so, then I still need to add some tests for
FinalizePsbt
andSignPsbt
. I would introduce those tests at the rpc level ? Maybe in a newwalletkit_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
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.