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

Fix p2tr spend/change bug #351

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Conversation

newtonick
Copy link
Collaborator

@newtonick newtonick commented Mar 19, 2023

In version 0.6.0 when p2tr signing was introduced a bug in the change logic causes an error psbt_parser.py. It'll show something like "script.py, 165 in p2tr". This PR fixes that issue and I created a unit test for the fix.

…esses and therefor my_pubkey will be None.

The additional indendation of script.p2tr(my_pubkey) will make it so it's only called when my_pubkey is not None.
@newtonick newtonick marked this pull request as ready for review March 19, 2023 13:07
tests/test_psbt_parser.py Outdated Show resolved Hide resolved
@newtonick newtonick added the bug Something isn't working label Jun 6, 2023
@kdmukai
Copy link
Contributor

kdmukai commented Jun 6, 2023

Minor notes above, but otherwise ACK!

@jdlcdl
Copy link

jdlcdl commented Jun 19, 2023

I'm unable to provoke this error so that I know that it happens before but not after this pr is applied.

I see the test, I confirm that the test fails without the change on line 151, and that it passes with it.

@newtonick
Copy link
Collaborator Author

I'm unable to provoke this error so that I know that it happens before but not after this pr is applied.

I see the test, I confirm that the test fails without the change on line 151, and that it passes with it.

Here are some screenshots of how I reproduced the bug in Sparrow

Tx:

image

Sending Wallet:

image

Receiving Wallet:

image

Receiving Address:

image

@jdlcdl
Copy link

jdlcdl commented Jun 19, 2023

Thank you Nick.

I had originally setup my tests, with existing code, but using a single taproot wallet, transferring internally to it else to native segwit (to the same seed). I still never succeeded in having provoked the error when taproot-to-taproot within the same wallet descriptor.

I was able to consistently provoke the error, with help from Manu who reported a similar problem yesterday -- after he mentioned "it only happened when transferring OUT OF the wallet to taproot". In fact, I could never get the existing code to succeed in that case. So anyone doing real taproot transactions, and not to themselves, must be having this problem.

Once I reinstalled your fix, the problem went away and I have not seen the same error again.

ACK tested -- with one request. There are some "tab" characters in the unit test file, would you consider converting them to 8 spaces for consistency?

We never mentioned this pr in last weeks meeting, but as soon as it's ready (I now believe it is), then I would push it to a priority for next release.

@newtonick
Copy link
Collaborator Author

There are some "tab" characters in the unit test file, would you consider converting them to 8 spaces for consistency?

Yes

@newtonick newtonick self-assigned this Jun 29, 2023
- add test doc string
@newtonick newtonick merged commit 9c36f5c into SeedSigner:dev Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

3 participants