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 default permitsigdata value not working on converttopsbt command #607

Merged
merged 2 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@stevenroose
Copy link
Member

commented May 3, 2019

We're not really supporting PSBT for Elements yet. The rpc_psbt.py integration test is disabled because of this; that's also why we didn't catch this. It's fixed upstream.

I added an extra test to catch the error, but it's not actually executed.

@instagibbs, up to you to decide if it's worth having this patched right now. I cherry-picked that fix commit literally from the catchup PR I'm working on, so if we don't patch it here, it will get patched while catching up.

@stevenroose stevenroose requested a review from instagibbs May 3, 2019

@instagibbs

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

-0 on fixing this versus merging with upstream, but not a hard NACK

If this gets in the way of useful PSBT usage, I'd probably change my mind. @gwillen might have better idea based on his PR.

@gwillen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Oh, I remember noticing that and probably should have filed it at the time I noticed it. I believe the effect of the bug is that "permitsigdata" is actually "prohibitsigdata", defaulting to true (that is, the sense of the flag is reversed if you supply it but the default is correct.)

I can't imagine when you would run into this; trying to convert a signed transaction to a (unsigned) PSBT doesn't really seem like a sensible thing to ever do. So I don't have strong feelings about fixing it.

@stevenroose

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

I changed my mind; I'd like to just pull this in here. Please review & ACK :)

@instagibbs

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

utACK 794c305

@instagibbs instagibbs merged commit 794c305 into ElementsProject:master May 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

instagibbs added a commit that referenced this pull request May 14, 2019

Merge #607: Fix default permitsigdata value not working on converttop…
…sbt command

794c305 Test that default `permitsigdata` is false (Steven Roose)
c8525c9 Fix bug in converttopsbt (Steven Roose)

Pull request description:

  We're not really supporting PSBT for Elements yet. The `rpc_psbt.py` integration test is disabled because of this; that's also why we didn't catch this. It's fixed upstream.

  I added an extra test to catch the error, but it's not actually executed.

  @instagibbs, up to you to decide if it's worth having this patched right now. I cherry-picked that fix commit literally from the catchup PR I'm working on, so if we don't patch it here, it will get patched while catching up.

Tree-SHA512: 2ee9ed733659c028f349b75aba19b80cef13894eacb0d85d0ab4377ee9e07b4aa90888edb402a5c31784f5d0848a0f22a445736995d9a6fca0bbeccf15f75dcb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.