descriptor: remove unreachable verification for `pkh` #31555

brunoerg commented at 12:43 PM on December 23, 2024: contributor

This PR removes an unreachable verification in the ParseScript function. It returns an error if pkh is not being used at top level, sh, wsh or tr. However, any usage of pkh without these contexts will not reach this verification but other ones like "invalid keys" (e.g. wpkh(pkh(L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd))).

descriptor: remove unreachable verification for `pkh` e366408590

DrahtBot commented at 12:43 PM on December 23, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31555.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, sipa, davidgumberg, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

sipa commented at 3:24 PM on December 23, 2024: member

ACK e36640859089baabc46f68217843f96a3ebdc20c

Took me a while to understand why, but it's just because ParseScript itself cannot be reached with ctx == ParseScriptContext::WPKH. Perhaps an assert or Assume could be added for that general property?

brunoerg commented at 5:10 PM on December 23, 2024: contributor

Took me a while to understand why, but it's just because ParseScript itself cannot be reached with ctx == ParseScriptContext::WPKH. Perhaps an assert or Assume could be added for that general property?

P2WPKH, right? Sounds good to add an Assume for this, will address it.

in src/script/descriptor.cpp:1764 in 6d59bcc674 outdated

1760 | @@ -1761,7 +1761,7 @@ struct KeyParser {
1761 |  std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index, Span<const char>& sp, ParseScriptContext ctx, FlatSigningProvider& out, std::string& error)
1762 |  {
1763 |      using namespace script;
1764 | -
1765 | +    Assume(ctx != ParseScriptContext::P2WPKH);

tdb3 commented at 12:26 AM on December 24, 2024:

Seems like it might be clearer and more defensive to check that we're entering with one of the appropriate contexts rather than check that we aren't entering in one of the inappropriate contexts (albeit currently the only inappropriate one).

-    Assume(ctx != ParseScriptContext::P2WPKH);
+    Assume(ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH || ctx == ParseScriptContext::P2WSH || ctx == ParseScriptContext::P2TR);

https://github.com/bitcoin/bitcoin/blob/fc7b21484703da606c5c69b23daee8c39506d90c/src/script/descriptor.cpp#L1406-L1412

brunoerg commented at 12:31 PM on December 24, 2024:

I'm not sure about this. I think Assume(ctx != ParseScriptContext::P2WPKH) is shorter and easier to understand.

tdb3 commented at 1:41 PM on December 24, 2024:

It's definitely shorter, for sure. A thought here is that if another context is added and it's not appropriate for the function, the check for allowed contexts would still work (conversely the !P2WPKH check would miss it silently). If another context is added and it is appropriate, then we're informed to add it (the assume would fail in a debug build), which also serves as a reminder to ensure the function is updated appropriately.

brunoerg commented at 2:02 PM on December 24, 2024:

Yes, fair enough. I will address it.

brunoerg commented at 2:03 PM on December 24, 2024:

Done.

tdb3 commented at 12:28 AM on December 24, 2024: contributor

Approach ACK Great catch! Left a small comment, but I don't feel very strongly about it.

descriptor: Assume `ParseScript` is not being called with a P2WPKH context 366ae00b77

brunoerg force-pushed on Dec 24, 2024

tdb3 approved

tdb3 commented at 2:49 PM on December 24, 2024: contributor

cr ACK 366ae00b779acd59a61719422f0597acb17fb3e0

DrahtBot requested review from sipa on Dec 24, 2024

sipa commented at 5:47 PM on December 24, 2024: member

crACK 366ae00b779acd59a61719422f0597acb17fb3e0

davidgumberg commented at 8:58 PM on December 30, 2024: contributor

crACK https://github.com/bitcoin/bitcoin/commit/366ae00b779acd59a61719422f0597acb17fb3e0

Checked that ParseScript() is never called with ParseScriptContext::P2WPKH.

achow101 commented at 9:34 PM on December 30, 2024: member

ACK 366ae00b779acd59a61719422f0597acb17fb3e0

achow101 merged this on Dec 30, 2024

achow101 closed this on Dec 30, 2024

sedited referenced this in commit 230a439a4a on Jan 17, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

bug-castercv502 referenced this in commit 44b075fe71 on Sep 28, 2025

bitcoin locked this on Dec 30, 2025