test: add coverage for untested descriptor parse error paths #35819

Looking at the coverage report for master on https://corecheck.dev, a number of error branches in descriptor.cpp are never exercised by any test. This PR adds CheckUnparsable vectors for each reachable one, plus one positive boundary check:

musig()

• unterminated expression: tr(musig(00) → "Invalid musig() expression"
• invalid participant key
• trailing garbage after a participant key: tr(musig(KEY}{)) → "musig(): expected ',', got '}'" (the } closes the level opened by the ( of musig( and the { re-opens it, so the final ) stays inside the expression span; thanks to @151henry151 for the counterexample showing this branch is reachable)
• invalid derivation path element (the musig(): -prefixed wrapping of the keypath error; the underlying ParseKeyPath errors were already covered via pkh()/wpkh())
• participants with multipath derivations of mismatched lengths (/<0;1> vs /<0;1;2>; the multi() and Miniscript variants of this error were covered, the musig() one was not)

Context restrictions

• multi() inside tr(), multi_a() at top level, and addr()/tr()/rawtr()/raw() inside sh()

Taptree structure errors

• exceeding the 128 nesting level limit (129 {s, built with std::string(129, '{'))
• missing '}' after a right branch, missing ',' after a left branch, trailing garbage after a script expression and after the internal key
• a positive check that a taptree of exactly 128 nesting levels parses and expands successfully, so the limit is verified on both sides (suggested by @Herb-ops)

rawtr()

• invalid key. 00 is used (rather than the truncated-valid-key pattern used elsewhere in this file) because in Taproot contexts a 32-byte string would parse as a valid x-only key.

Since CheckUnparsable asserts on the exact error message and each targeted branch produces a distinct one, a passing vector proves the corresponding branch executed.

Note that replaying the qa-assets descriptor_parse/mocked_descriptor_parse fuzz corpora already reaches these branches, so the value of these vectors is deterministic coverage in the unit tests with the exact error messages pinned.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

tACK 004de4f835. Built on macOS ARM and ran ./build/bin/test_bitcoin --run_test=descriptor_tests; all 4 cases passed.

Verified by temporarily mutating descriptor.cpp:2041 from "Invalid musig() expression" to "MUTATED musig() expression". The new vector caught the failure with: tr(): MUTATED musig() expression != tr(): Invalid musig() expression

restored the original code and the test passed again.

Approach ACK 004de4f835943c938e9e56d015ac902a2ac630d4

All 16 new vectors match their error sites in descriptor.cpp exactly and hit 16 distinct ones (the two Pubkey '00' is invalid vectors share ParsePubkey's message but reach it via the two different wrap sites being targeted), matching corecheck's gained-coverage lines. The limit check (> TAPROOT_CONTROL_MAX_NODE_COUNT, 128) runs after each push, so 129 {s is minimal. Extending @sorukumar's mutation check: perturbing all 16 expected strings gives exactly 16 failures, each reporting the original message.

Correction to the description: musig(): expected ',', got '%c' is reachable. Expr() counts bracket levels without matching types, so below the } closes the ( of musig( and the { re-opens the level, keeping the final ) inside the span; Split() sees one ), and the }{ leftover then fails the Const(",") check. Passes locally on this branch:

CheckUnparsable("", "tr(musig(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd}{))", "tr(): musig(): expected ',', got '}'");

Replaying the qa-assets descriptor_parse and mocked_descriptor_parse corpora reaches all 16 branches plus that one, so fuzzing already covers these; the vectors add deterministic coverage with the messages pinned. Might be worth a line in the description.

nit: the "garbage after internal key" vector pins tr: expected ',', got '}' where neighboring errors compose as tr(): ... — pre-existing, but now locked in by a test; worth normalizing in a follow-up or a comment marking it deliberate.

Tested on Debian 13, gcc 14.2.0, Debug build: cmake --build build && ctest --test-dir build — all passed (script_assets_tests skipped)

Force-pushed 004de4f835 -> 81bd655cc9. Thanks @sorukumar and @151henry151 for the review!

Changes since 004de4f835:

• Added a 17th vector for musig(): expected ',', got '%c', following @151henry151's counterexample showing the branch is reachable after all (I had missed that Expr()'s type-agnostic bracket counting lets }{ keep the final ) inside the span). It is written as a prv/pub pair to match the conventions of the file, with a comment explaining the bracket trick.
• Updated the PR description accordingly: removed the incorrect "unreachable" paragraph and added a note that replaying the qa-assets fuzz corpora already reaches these branches, so the value of these vectors is deterministic coverage with the exact messages pinned.

Regarding the tr: expected ',', got '}' nit: agreed that it is inconsistent with the neighboring tr(): ...-style messages. Since this PR is test-only I would prefer to leave the message as-is here and normalize it in a follow-up.

ACK 81bd655cc929df54ee999813c61cb87588a25f61

Force-push only adds the 17th vector (plus comment); expected message matches, and the prv/pub pair is the right call for this file. Description updates look good; fine leaving the tr:/tr(): inconsistency for a follow-up.

Retested on Debian 13, gcc 14.2.0, Debug: ./build/bin/test_bitcoin --run_test=descriptor_tests — passed

Non-blocking: Could we also test that a valid tree at depth 128 parses successfully? The current case verifies that depth 129 is rejected, but it would still pass if the parser incorrectly rejected both 128 and 129. Testing both sides would verify that the limit is exactly 128.

re-tACK 81bd655cc929df54ee999813c61cb87588a25f61.

Rebuilt on macOS ARM and ran ./build/bin/test_bitcoin --run_test=descriptor_tests. All 4 cases pass.

Looked closely at the new 17th vector (}{). Confirmed it successfully hits the expected ',' branch down at descriptor.cpp:2053. The mutation check from my prior review still holds up fine here too.

Force-pushed 81bd655cc9 -> 0b3bb07: added a positive check that a taptree of exactly 128 nesting levels parses and expands successfully, as suggested by @Herb-ops. This verifies the limit from both sides.

ACK 0b3bb071036ac00649901b7a806e3882b61ddfde