SIGN IN SIGN UP

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

0b3bb071036ac00649901b7a806e3882b61ddfde test: add coverage for untested descriptor parse error paths (azuchi)

Pull request description:

  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.

ACKs for top commit:
  Herb-ops:
    ACK 0b3bb071036ac00649901b7a806e3882b61ddfde
  151henry151:
    re-ACK 0b3bb071036ac00649901b7a806e3882b61ddfde
  ryanofsky:
    Code review ACK 0b3bb071036ac00649901b7a806e3882b61ddfde. Seems good to add these cases since there are no checks for these error messages. It also seems like it could be good to fix the inconsistent "tr: expected" message this exposed in a followup.

Tree-SHA512: a3f14d44b5c03eb2c365efa3b530e0b5dc143fbd9782f3e2f5bb9a8119a16428e67dfbf20ff850e13f0fe5f2f6cb5123edaa7c6d3d21c32af735c34a6a1a1050
R
Ryan Ofsky committed
51ddab532cb38213e2258c24c492bc8a392ffc90