Skip to content

Write PSBTs to file with binary mode #188

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
Mar 11, 2021
Merged

Write PSBTs to file with binary mode #188

maflcko merged 1 commit into from
Mar 11, 2021

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Jan 19, 2021

As noted in bitcoin/bitcoin#20959, PSBT files should be opened in binary mode as on windows, all newlines are turned into CRLF which produces invalid PSBTs.

Fixes bitcoin/bitcoin#20959

@maflcko maflcko added this to the 0.21.1 milestone Jan 19, 2021
@maflcko maflcko changed the title GUI: Write PSBTs to file with binary mode Write PSBTs to file with binary mode Jan 21, 2021
@maflcko
Copy link
Contributor

maflcko commented Jan 21, 2021

Approach ACK. Is there a way to test this on non-windows?

@hebasto
Copy link
Member

hebasto commented Feb 22, 2021

Concept ACK.

For reference: https://en.cppreference.com/w/cpp/io/c/FILE#Binary_and_text_modes

@hebasto
Copy link
Member

hebasto commented Feb 22, 2021

@Quantris Does this pr fix bitcoin/bitcoin#20959 for you?

Talkless
Talkless approved these changes Mar 11, 2021
View reviewed changes
Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

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

utACK cc3971c.

I DID test, but only on Linux (Debian Sid with Qt 5.15.2). I've created .psbt with PR version, imported into master version, and was able to sing and broadcast (regtest), so at least I don't see regressions in Linux.

@maflcko maflcko merged commit b972913 into bitcoin-core:master Mar 11, 2021
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Mar 11, 2021
@achow101
GUI: Write PSBTs to file with binary mode
3a12672 
Github-Pull: #bitcoin-core/gui#188
Rebased-From: cc3971c
@maflcko
Copy link
Contributor

maflcko commented Mar 11, 2021

Backported in bitcoin/bitcoin#20901

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 11, 2021
Merge bitcoin-core/gui#188: Write PSBTs to file with binary mode
92e4f92 
cc3971c GUI: Write PSBTs to file with binary mode (Andrew Chow)

Pull request description:

  As noted in bitcoin#20959, PSBT files should be opened in binary mode as on windows, all newlines are turned into CRLF which produces invalid PSBTs.

  Fixes bitcoin#20959

ACKs for top commit:
  Talkless:
    utACK cc3971c.

Tree-SHA512: fee62b66da844017a44d7d6da6d2d2794b097a7dec33fb07711615df1e94dccc76f987ffcbb325ad1f8db2a2dd6eaf514b6cbd2453e7658b9f6c9fb5c4c41dab
@Quantris
Copy link

Quantris commented Jun 10, 2021

@Quantris Does this pr fix bitcoin/bitcoin#20959 for you?

Sorry I missed replying to this at the time. I'm not really set up to build the project myself so I wouldn't have been able to test this without someone providing a binary.

I can affirm that the issue seems fixed in 0.21.1 which includes this change; I tested in Win10 with the following steps in a watch-only wallet:

  1. create a partial transaction & save to file test.psbt (during this process it is also copied to clipboard in base64)
  2. load from clipboard, confirm it displays as expected & save to another file test2.psbt
  3. load from file test.psbt, confirm it displays as expected
  4. load from file test2.psbt, confirm it displays as expected

The same process in 0.21.0 fails in steps 3 & 4.

I also looked around a bit for info on testing this kind of issue. Unfortunately I didn't find anything that makes text-mode vs. binary-mode distinguishable in linux (I was thinking maybe there would be some kind of env var or build flag for the stdlib). IOW seems like the only way to test this would be to run the tests in a Windows env.

Thanks for the fix @achow101 !

apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
@achow101 @apoelstra
GUI: Write PSBTs to file with binary mode
b27e907 
Github-Pull: #bitcoin-core/gui#188
Rebased-From: cc3971c
(cherry picked from commit 3a12672)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 21, 2021
@achow101 @apoelstra
GUI: Write PSBTs to file with binary mode
2b372d3 
Github-Pull: #bitcoin-core/gui#188
Rebased-From: cc3971c
(cherry picked from commit 3a12672)
apoelstra pushed a commit to apoelstra/elements that referenced this pull request Sep 22, 2021
@achow101 @apoelstra
GUI: Write PSBTs to file with binary mode
337297d 
Github-Pull: #bitcoin-core/gui#188
Rebased-From: cc3971c
(cherry picked from commit 3a12672)
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@apoelstra
Merge b972913 into merged_master (Bitcoin PR bitcoin-core/gui#188)
4135bed
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Talkless Talkless Talkless approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Windows

Projects

None yet

Milestone

0.21.1

Development

Successfully merging this pull request may close these issues.

Invalid format when saving PSBT files

5 participants

@achow101 @maflcko @hebasto @Quantris @Talkless