[Olorunshogo]

PR summary

Description

compile parsed policies as Concrete<String>, so placeholder strings (pk(ABC)) or hashes were accepted as "keys" with no validation, failing only later when a real consumer used the descriptor. Now parses as Concrete<DescriptorPublicKey> by default, validating at parse time; the old permissive behavior moves behind --allow-placeholders for inspecting a policy's compiled shape without real keys.

A key can pass that parse-time check and still be the wrong shape for the chosen script type (e.g. an x-only key under wsh), which miniscript's compiler turned into a panic instead of an error. compile now validates every key against the chosen script context (Legacy/Segwitv0/Tap) before compiling, so this returns a clean Invalid policy error instead of crashing.

Fixes #344

Notes to reviewers

• compile_policy<Pk> is generic over DescriptorPublicKey/String to avoid duplicating the tr branch's NUMS-tweak internal-key math per key type.
• String can't impl From<XOnlyPublicKey> here (orphan rules), so tr's internal-key construction goes through a local TrInternalKey trait.
• --type now uses ValueEnum (ScriptType) instead of a raw string with value_parser, so this PR also closes Use ValueEnum for --type in compile command #345.
• check_policy_keys walks the policy via ForEachKey::for_each_key and calls ScriptContext::check_pk per key; it's a no-op for Pk = String since String never reports itself as uncompressed or x-only.
• test_compile_policy_beyond_legacy_limits now passes --allow-placeholders.
• Added test_compile_rejects_x_only_key_for_non_tr_types and test_compile_accepts_x_only_key_for_tr covering the panic fix.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[Jennycj]

Concept ACK a139efc
I tested a139efc and the tests all pass, however I found that trying compile with a valid key (I used an x-only key) with the wrong script type(wsh), caused a panic (exit 101) in miniscript 12.3.7
I think it would be best to call miniscript’s check_pk for the chosen script type on every key before compiling, so the user gets a clear error instead of a crash.

Also, a CHANGELOG entry will be great since the fix breaks existing placeholder usage.

[Olorunshogo]

Thanks for catching this, Jennycj. Confirmed and reproduced it exactly as you described: an x-only key compiled with --type wsh (and same for sh/sh-wsh) panics in miniscript 12.3.7 with Terminal creation must always succeed: ContextError(XOnlyKeysNotAllowed(...)), exit 101.

Root cause: the current validation checks that a key parses as a valid DescriptorPublicKey, but not that it's the right shape for the script context being compiled to. An x-only key parses fine (it's valid for tr), but Segwitv0/Legacy reject x-only keys internally, and miniscript's compiler treats that as a should-never-happen case and panics instead of returning a Result.

Implemented your suggestion: added a check_policy_keys helper that walks the policy's keys with ForEachKey::for_each_key and calls ScriptContext::check_pk (Legacy for sh, Segwitv0 for wsh/sh-wsh, Tap for tr) before calling .compile(). Now it returns a clean Invalid policy error instead of crashing. It's a no-op under --allow-placeholders since String keys never report themselves as x-only/uncompressed, so that path is unaffected.

I alsoAdded two regression tests (test_compile_rejects_x_only_key_for_non_tr_types, test_compile_accepts_x_only_key_for_tr) and a CHANGELOG entry. Pushed as a new commit on this branch.

[codecov]

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.77%. Comparing base (79b2b83) to head (6e8f2e1).
⚠️ Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers/descriptor.rs	98.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   61.46%   61.77%   +0.30%     
==========================================
  Files          23       23              
  Lines        3989     4021      +32     
==========================================
+ Hits         2452     2484      +32     
  Misses       1537     1537              

Flag	Coverage Δ
rust	61.77% <98.14%> (+0.30%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tvpeter]

Hello @Olorunshogo , thank you for working on this.

In addition to the comments, you should squash all the commits into probably just two. You should aim for a clean rebase each time you have to get changes from master. Having merge branch ... will pollute the git history.