Skip to content

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 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

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
Copy link
Member Author

achow101 commented Nov 8, 2018

@instagibbs I added a test

@Sjors
Copy link
Member

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

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

@DrahtBot
Copy link
Contributor

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
Contributor

utACK 6b8d86d

1 similar comment
@instagibbs
Copy link
Member

utACK 6b8d86d

@sipa
Copy link
Member

sipa commented Nov 8, 2018

utACK 6b8d86d

@sipa sipa merged commit 6b8d86d into bitcoin:master Nov 10, 2018
sipa added a commit that referenced this pull request 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 pull request 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

Backport?

@sipa
Copy link
Member

sipa commented Nov 22, 2018

Marked for backport.

@maflcko maflcko 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

Choose a reason for hiding this comment

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

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

Mind taking a look to backport this?

Copy link
Contributor

@meshcollider meshcollider Dec 6, 2018

Choose a reason for hiding this comment

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

@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
Contributor

@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 pull request 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
kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 9, 2021
knst pushed a commit to knst/dash that referenced this pull request 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 pushed a commit to knst/dash that referenced this pull request 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 pushed a commit to 5tefan/dash that referenced this pull request Aug 12, 2021
knst pushed a commit to knst/dash that referenced this pull request 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 pushed a commit to PastaPastaPasta/dash that referenced this pull request 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
Development

Successfully merging this pull request may close these issues.

8 participants