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

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:fix-pkh-pubkeys changing 2 files +5 −1
  1. achow101 commented at 3:16 PM on November 8, 2018: member

    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.

  2. instagibbs commented at 3:22 PM on November 8, 2018: member

    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.

  4. Require a public key to be retrieved when signing a P2PKH input
    If we do not have the public key for a P2PKH input, we should not
continue to attempt to sign for it.
    6b8d86ddb8
  5. achow101 force-pushed on Nov 8, 2018
  6. achow101 commented at 3:39 PM on November 8, 2018: member

    @instagibbs I added a test

  7. Sjors commented at 4:21 PM on November 8, 2018: member

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

  8. instagibbs commented at 4:26 PM on November 8, 2018: member

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

  9. DrahtBot commented at 5:09 PM on November 8, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.<!--2502f1a698b3751726fa55edcda76cd3-->

    Coverage

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

    <sup>Updated at: 2018-11-08T22:33:52.852772.</sup>

  10. meshcollider added the label Wallet on Nov 8, 2018
  13. sipa commented at 11:26 PM on November 8, 2018: member

    utACK 6b8d86ddb803d50d8608d95f7e8f791511dec8b9

  14. sipa merged this on Nov 10, 2018
  15. sipa closed this on Nov 10, 2018
  16. sipa referenced this in commit 16e3b17578 on Nov 10, 2018
  17. fanquake deleted a comment on Nov 10, 2018
  18. laanwj referenced this in commit ce7fcc3569 on Nov 13, 2018
  19. gmaxwell commented at 12:37 AM on November 20, 2018: contributor

    Backport?

  20. sipa added the label Needs backport on Nov 22, 2018
  21. sipa commented at 1:06 AM on November 22, 2018: member

    Marked for backport.

  22. MarcoFalke added this to the milestone 0.17.1 on Nov 30, 2018
  23. in test/functional/rpc_psbt.py:275 in 6b8d86ddb8 
     268 | @@ -269,6 +269,10 @@ def run_test(self):
 269 |  
 270 |          self.test_utxo_conversion()
 271 |  
 272 | +        # Test that psbts with p2pkh outputs are created properly
 273 | +        p2pkh = self.nodes[0].getnewaddress(address_type='legacy')
 274 | +        psbt = self.nodes[1].walletcreatefundedpsbt([], [{p2pkh : 1}], 0, {"includeWatching" : True}, True)
 275 | +        self.nodes[0].decodepsbt(psbt['psbt'])
    MarcoFalke commented at 10:46 PM on December 5, 2018:

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

    Mind taking a look to backport this?

    meshcollider commented at 11:29 AM on December 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?

  24. meshcollider removed the label Needs backport on Dec 6, 2018
  25. meshcollider removed this from the milestone 0.17.1 on Dec 6, 2018
  26. meshcollider commented at 7:54 PM on December 6, 2018: contributor

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

  27. deadalnix referenced this in commit 80241e1f4a on May 11, 2020
  28. kittywhiskers referenced this in commit 7ef678111b on Aug 5, 2021
  29. kittywhiskers referenced this in commit d6d41d784b on Aug 5, 2021
  30. kittywhiskers referenced this in commit 88298be853 on Aug 5, 2021
  31. kittywhiskers referenced this in commit de110acbcb on Aug 5, 2021
  32. kittywhiskers referenced this in commit 5ae8e75c24 on Aug 9, 2021
  33. knst referenced this in commit 79266d56af on Aug 10, 2021
  34. knst referenced this in commit d60446f198 on Aug 10, 2021
  35. UdjinM6 referenced this in commit 7aebf156e9 on Aug 10, 2021
  36. 5tefan referenced this in commit 4d17b869d4 on Aug 12, 2021
  37. knst referenced this in commit cd4f4ec60f on Aug 16, 2021
  38. PastaPastaPasta referenced this in commit 739c675f03 on Aug 23, 2021
  39. DrahtBot locked this on Sep 8, 2021
Contributors
achow101instagibbsSjorsDrahtBotmeshcollidersipagmaxwellMarcoFalke
Labels
Wallet
Linked (view graph)
#14690 Throw error if CPubKey is invalid during PSBT keypath serialization
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 21:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me