descriptor: remove unreachable verification for pkh #31555

1. 
   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))).
2. descriptor: remove unreachable verification for `pkh` e366408590
3. 
   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.

4. 
   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?

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

6. 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 0: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).

   0-    Assume(ctx != ParseScriptContext::P2WPKH);
   1+    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.
7. 
   tdb3 commented at 0:28 am on December 24, 2024: contributor

   Approach ACK Great catch! Left a small comment, but I don’t feel very strongly about it.
8. descriptor: Assume `ParseScript` is not being called with a P2WPKH context 366ae00b77
9. brunoerg force-pushed on Dec 24, 2024
10. tdb3 approved
11. 
    tdb3 commented at 2:49 pm on December 24, 2024: contributor

    cr ACK 366ae00b779acd59a61719422f0597acb17fb3e0
12. DrahtBot requested review from sipa on Dec 24, 2024
13. 
    sipa commented at 5:47 pm on December 24, 2024: member

    crACK 366ae00b779acd59a61719422f0597acb17fb3e0
14. 
    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.

15. 
    achow101 commented at 9:34 pm on December 30, 2024: member

    ACK 366ae00b779acd59a61719422f0597acb17fb3e0
16. achow101 merged this on Dec 30, 2024
17. achow101 closed this on Dec 30, 2024

    ---

Contributors
brunoerg DrahtBot sipa tdb3 davidgumberg achow101