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

Require a public key to be retrieved when signing a P2PKH input #14689

Merged
merged 1 commit into from Nov 10, 2018

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Nov 8, 2018

If we do not have the public key for a P2PKH input, we should not continue to attempt to sign for it.

This fixes a problem where a PSBT with a P2PKH output would include invalid BIP 32 derivation paths that are missing the public key.

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Nov 8, 2018

This deserves a regression test. Have a walletcreatefundedpsbt call in rpc_psbt.py just make an output to a p2pkh output it doesn't control, do a PSBT decoding round trip. In fact, let's do it for all standard output types.

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Nov 8, 2018

utACK otherwise 409564a

If we do not have the public key for a P2PKH input, we should not
continue to attempt to sign for it.
@achow101 achow101 force-pushed the fix-pkh-pubkeys branch from 409564a to 6b8d86d Nov 8, 2018
@achow101
Copy link
Member Author

@achow101 achow101 commented Nov 8, 2018

@instagibbs I added a test

@Sjors
Copy link
Member

@Sjors Sjors commented Nov 8, 2018

I can confirm this test fails on master. It also fixes the downstream issue I was seeing.

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Nov 8, 2018

just to note this issue happens for any native "keyhash" type including native segwit, and this fixes that

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 8, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13932 (Additional utility RPCs for PSBT by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage Change (pull 14689) Reference (master)
Lines +0.0834 % 87.0730 %
Functions +0.0306 % 84.3717 %
Branches +0.0536 % 51.5638 %

Updated at: 2018-11-08T22:33:52.852772.

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Nov 8, 2018

utACK 6b8d86d

1 similar comment
@instagibbs
Copy link
Member

@instagibbs instagibbs commented Nov 8, 2018

utACK 6b8d86d

@sipa
Copy link
Member

@sipa sipa commented Nov 8, 2018

utACK 6b8d86d

@sipa sipa merged commit 6b8d86d into bitcoin:master Nov 10, 2018
1 check passed
sipa added a commit that referenced this issue Nov 10, 2018
…KH input

6b8d86d Require a public key to be retrieved when signing a P2PKH input (Andrew Chow)

Pull request description:

  If we do not have the public key for a P2PKH input, we should not continue to attempt to sign for it.

  This fixes a problem where a PSBT with a P2PKH output would include invalid BIP 32 derivation paths that are missing the public key.

Tree-SHA512: 850d5e74c06833da937d5bf0348bd134180be7167b6f9b9cecbf09f75e3543fbad60d0abbc0b9afdfa51ce165aa36168849f24a7c5abf1e75f37ce8f9a13d127
@bitcoin bitcoin deleted a comment from WorkShop-Office Nov 10, 2018
laanwj added a commit that referenced this issue Nov 13, 2018
…erialization

4e4de10 Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders)

Pull request description:

  Related to #14689

  We should catch this error before attempting to deserialize it later.

Tree-SHA512: d2f3ea7f363818ac70c81ee988231b2bb50d055b6919f7bff3f27120c85a7048bfa183efae33e23e6b81d684bcb8bb81e5b209abb3acbcaff1d88014f4f1aa93
@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Nov 20, 2018

Backport?

@sipa
Copy link
Member

@sipa sipa commented Nov 22, 2018

Marked for backport.

@MarcoFalke MarcoFalke added this to the 0.17.1 milestone Nov 30, 2018
# Test that psbts with p2pkh outputs are created properly
p2pkh = self.nodes[0].getnewaddress(address_type='legacy')
psbt = self.nodes[1].walletcreatefundedpsbt([], [{p2pkh : 1}], 0, {"includeWatching" : True}, True)
self.nodes[0].decodepsbt(psbt['psbt'])
Copy link
Member

@MarcoFalke MarcoFalke Dec 5, 2018

I tried to backport, but the test passes even without the code changes.

Mind taking a look to backport this?

Copy link
Member

@meshcollider meshcollider Dec 6, 2018

@MarcoFalke I believe the issue only arose after #14424 (which is also marked for backport), does the test fail if you backport that at the same time?

@meshcollider meshcollider removed this from the 0.17.1 milestone Dec 6, 2018
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Dec 6, 2018

@achow101 said on IRC that #13723 may have been involved with producing the bug too so its not present on 0.17

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue May 11, 2020
…P2PKH input

Summary:
If we do not have the public key for a P2PKH input, we should not
continue to attempt to sign for it.

---

Depends on D6028

This is a backport of Core [[bitcoin/bitcoin#14689 | PR14689]]

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

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6029
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 5, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 5, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 5, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 5, 2021
kittywhiskers added a commit to kittywhiskers/dash that referenced this issue Aug 9, 2021
knst added a commit to knst/dash that referenced this issue Aug 10, 2021
…ypath serialization

4e4de10 Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders)

Pull request description:

  Related to bitcoin#14689

  We should catch this error before attempting to deserialize it later.

Tree-SHA512: d2f3ea7f363818ac70c81ee988231b2bb50d055b6919f7bff3f27120c85a7048bfa183efae33e23e6b81d684bcb8bb81e5b209abb3acbcaff1d88014f4f1aa93
knst added a commit to knst/dash that referenced this issue Aug 10, 2021
…ypath serialization

4e4de10 Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders)

Pull request description:

  Related to bitcoin#14689

  We should catch this error before attempting to deserialize it later.

Tree-SHA512: d2f3ea7f363818ac70c81ee988231b2bb50d055b6919f7bff3f27120c85a7048bfa183efae33e23e6b81d684bcb8bb81e5b209abb3acbcaff1d88014f4f1aa93
5tefan added a commit to 5tefan/dash that referenced this issue Aug 12, 2021
knst added a commit to knst/dash that referenced this issue Aug 16, 2021
…ypath serialization

4e4de10 Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders)

Pull request description:

  Related to bitcoin#14689

  We should catch this error before attempting to deserialize it later.

Tree-SHA512: d2f3ea7f363818ac70c81ee988231b2bb50d055b6919f7bff3f27120c85a7048bfa183efae33e23e6b81d684bcb8bb81e5b209abb3acbcaff1d88014f4f1aa93
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Aug 23, 2021
…ypath serialization

4e4de10 Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders)

Pull request description:

  Related to bitcoin#14689

  We should catch this error before attempting to deserialize it later.

Tree-SHA512: d2f3ea7f363818ac70c81ee988231b2bb50d055b6919f7bff3f27120c85a7048bfa183efae33e23e6b81d684bcb8bb81e5b209abb3acbcaff1d88014f4f1aa93
@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.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants