Skip to content

Stop requiring imported pubkey to sign non-PKH schemes #14424

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
jonasschnelli merged 1 commit into from
Oct 15, 2018
Merged

Stop requiring imported pubkey to sign non-PKH schemes #14424

jonasschnelli merged 1 commit into from
Oct 15, 2018

Conversation

@sipa
Copy link
Member

@sipa sipa commented Oct 7, 2018 •
edited by maflcko
Loading

...

This is a fix for #14415

@promag
Copy link
Contributor

promag commented Oct 7, 2018

Test failing https://github.com/bitcoin/bitcoin/blob/a23e50fcd175a038690bd196cf0573fd98435e47/src/wallet/test/psbt_wallet_tests.cpp#L71

@meshcollider
Copy link
Contributor

meshcollider commented Oct 8, 2018

This is a fix for #14415 right?

@sipa
Copy link
Member Author

sipa commented Oct 8, 2018

@meshcollider Hopefully.

@maflcko maflcko added the Wallet label Oct 8, 2018
achow101
achow101 reviewed Oct 8, 2018
View reviewed changes
src/script/sign.cpp Outdated
Copy link
Member

@achow101 achow101 Oct 8, 2018

Choose a reason for hiding this comment

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

Removing this is also breaking the PSBT updating test. Without it, we don't know which pubkeys are needed for signing in order to properly update a PSBT with the pubkeys.

Copy link
Member

@instagibbs instagibbs Oct 8, 2018

Choose a reason for hiding this comment

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

sorry can you point to where this breaks stuff exactly, non-obvious to me

Copy link
Member

@achow101 achow101 Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

PSBT relies on the GetPubKey helper function to find out which public keys are used and to add the public key to a SignatureData object. The public keys are taken from the sigdata after ProduceSignature is done and moved into the PSBT. Without this call to GetPubKey, the pubkeys that were needed during signing are not added to the sigdata and thus not added to the PSBT. This causes the test to fail (and further breaks PSBT updating) as public keys that are watch only are not being added to the PSBT.

Copy link
Member Author

@sipa sipa Oct 8, 2018

Choose a reason for hiding this comment

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

Possibly fixed.

@sipa sipa force-pushed the 201810_importpubkeylol branch from a23e50f to 2f6b466 Compare October 8, 2018 05:15
@instagibbs
Copy link
Member

instagibbs commented Oct 8, 2018

one build was failing to finish building(?), kicked

@achow101
Copy link
Member

achow101 commented Oct 9, 2018

utACK 2f6b466

@instagibbs
Copy link
Member

instagibbs commented Oct 9, 2018

utACK 2f6b466

@sipa sipa added this to the 0.17.1 milestone Oct 9, 2018
@sipa
Copy link
Member Author

sipa commented Oct 9, 2018

Marked as 0.17.1.

@meshcollider
Copy link
Contributor

meshcollider commented Oct 9, 2018

utACK 2f6b466

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Oct 15, 2018

Tested ACK 2f6b466, fixes #14415

@jonasschnelli jonasschnelli merged commit 2f6b466 into bitcoin:master Oct 15, 2018
jonasschnelli added a commit that referenced this pull request Oct 15, 2018
@jonasschnelli
Merge #14424: Stop requiring imported pubkey to sign non-PKH schemes
2a2cac7 
2f6b466 Stop requiring imported pubkey to sign non-PKH schemes (Pieter Wuille)

Pull request description:

  ...

  This is a fix for #14415

Tree-SHA512: 113b4ddfbdfcce7dbaa15c565ac7ec68d16127aa4d47628e0801f2437cbe369e0fa8beb0de191d43dcb2f8cbb6f1bf8d79a5db0f4e352f38fe7280124475710a
@achow101 achow101 mentioned this pull request Oct 15, 2018
Closed
@sipa sipa mentioned this pull request Oct 15, 2018
Merged
@fanquake fanquake mentioned this pull request Oct 24, 2018
Merged
@achow101 achow101 mentioned this pull request Nov 8, 2018
Closed
This was referenced Dec 6, 2018
Merged
Merged
laanwj added a commit that referenced this pull request Dec 6, 2018
@laanwj
Merge #14889: [0.17] Backport #14424 (Stop requiring imported pubkey …
c6e4a1f 
…to sign non-PKH schemes)

89a9a9d Stop requiring imported pubkey to sign non-PKH schemes (Pieter Wuille)

Pull request description:

  Github-Pull: #14424
  Rebased-From: 2f6b466

Tree-SHA512: 1ea10dee66626f04918f197cd7c4949a836fa49c8f676f276b2328f8d79389059db7b30fc04d4c4bf8209f6a8d21f3ea49a017ddc7623eca6b7e6efc2fe0d749
@fanquake
Copy link
Member

fanquake commented Dec 9, 2018

Backported in #14889.

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 11, 2020
@sipa @majcosta
[backport#14424] Stop requiring imported pubkey to sign non-PKH schemes
d8f89cf 
Summary:
...

This is a fix for bitcoin/bitcoin#14415

---

This is a backport of Core [[bitcoin/bitcoin#14424 | PR14424]]

Test Plan:
  cmake .. -GENABLE_WERROR=ON -GNinja
  ninja check-all

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6028
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
@sipa @ftrader
[backport#14424] Stop requiring imported pubkey to sign non-PKH schemes
08af019 
Summary:
...

This is a fix for bitcoin/bitcoin#14415

---

This is a backport of Core [[bitcoin/bitcoin#14424 | PR14424]]

Test Plan:
  cmake .. -GENABLE_WERROR=ON -GNinja
  ninja check-all

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6028
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Jul 21, 2021
@jonasschnelli @Munkybooty
Merge bitcoin#14424: Stop requiring imported pubkey to sign non-PKH s…
985ea09 
…chemes

2f6b466 Stop requiring imported pubkey to sign non-PKH schemes (Pieter Wuille)

Pull request description:

  ...

  This is a fix for bitcoin#14415

Tree-SHA512: 113b4ddfbdfcce7dbaa15c565ac7ec68d16127aa4d47628e0801f2437cbe369e0fa8beb0de191d43dcb2f8cbb6f1bf8d79a5db0f4e352f38fe7280124475710a
kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021
@kwvg
merge bitcoin#14424: Stop requiring imported pubkey to sign non-PKH s…
1099ae7 
…chemes
kwvg added a commit to kwvg/dash that referenced this pull request Aug 9, 2021
@kwvg
merge bitcoin#14424: Stop requiring imported pubkey to sign non-PKH s…
a233e75 
…chemes
UdjinM6 added a commit to dashpay/dash that referenced this pull request Aug 10, 2021
@UdjinM6
Merge pull request #4229 from kittywhiskers/auxports
7aebf15 
merge bitcoin#16117, bitcoin#18358, bitcoin#17383, bitcoin#21052, bitcoin#14424, bitcoin#15159, bitcoin#14689, bitcoin#14978, partial bitcoin#16908, bitcoin#14978, bitcoin#13932: Auxillary Backports
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@achow101 achow101 achow101 left review comments

+1 more reviewer

@instagibbs instagibbs instagibbs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

0.17.1

Development

Successfully merging this pull request may close these issues.

9 participants

@sipa @promag @meshcollider @instagibbs @achow101 @jonasschnelli @fanquake @jjgg22zh @maflcko