[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. It now parses as Concrete<DescriptorPublicKey> by default, validating at parse time. The old permissive behavior moves behind --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 turns 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: FromStrKey> is generic over DescriptorPublicKey/String to avoid duplicating the tr branch's NUMS-tweak internal-key math per key type.
• The tr internal key is built with Pk::from_str(&xonly_internal_key.to_string()), which works for both key types since FromStrKey requires FromStr.
• --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 is a no-op for Pk = String, since String never reports itself as uncompressed or x-only.
• test_compile_policy_beyond_legacy_limits now passes --placeholders.
• Added test_compile_rejects_x_only_key_for_non_tr_types, test_compile_accepts_x_only_key_for_tr and test_compile_rejects_uncompressed_key_for_tr covering the context checks.

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.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.99%. Comparing base (4445a26) to head (b166c7d).
⚠️ Report is 1 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   58.74%   58.99%   +0.24%     
==========================================
  Files          25       25              
  Lines        4608     4636      +28     
==========================================
+ Hits         2707     2735      +28     
  Misses       1901     1901              

Flag	Coverage Δ
rust	58.99% <98.00%> (+0.24%)	⬆️

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.

[tvpeter]

I have left some minor comments

[vadim-anfv]

@Olorunshogo thanks for working on this!

Built the branch and ran the compile scenarios by hand: placeholders, real keys and xpubs, x-only and uncompressed keys across all four script types. Every rejection is a clean error, no crashes.

One note on the tests. check_policy_keys rejects an uncompressed key under --type tr, but no test compiles an uncompressed key for tr, so that check is never exercised - the one test that compiles for tr uses an x-only key, which Tap accepts. It would be great to add that case.

Happy to retest once it is rebased.

[Olorunshogo]

Addressed both comments, doc comments now sit above the gating attributes in both spots. In addition to that, I wrote a test compiling an uncompressed key under --type tr cover the rejection path vadim-anfv flagged, it wasn't exercised before since the existing tr test uses an x-only key.

Rebased on master and ready for another look.

[Christopherdominic]

Looks good overall. Two small documentation fixes:

• The PR description still mentions --allow-placeholders and TrInternalKey; please update it to match the current implementation.
• The comment above test_compile_rejects_uncompressed_key_for_tr says uncompressed keys are valid under wsh/sh-wsh, but both reject them too. Of the supported types, only sh accepts them.
  All 11 compiler integration tests and the formatting check passed locally. No blocking issues found.

[Olorunshogo]

Thanks for testing this. Both fixed:

• Updated the PR description to match the current implementation (--placeholders, Pk::from_str for the tr internal key, no TrInternalKey).
• Corrected the comment above test_compile_rejects_uncompressed_key_for_tr: uncompressed keys are only valid under sh; wsh/sh-wsh and tr all reject them. Amended into the existing commit, so history is still two cmits.

[Olorunshogo]

Thanks for testing this. Both fixed:

• Updated the PR description to match the current implementation (--placeholders, Pk::from_str for the tr internal key, no TrInternalKey).
• Corrected the comment above test_compile_rejects_uncompressed_key_for_tr: uncompressed keys are only valid under sh; wsh/sh-wsh and tr all reject them. Amended into the existing commit, so history is still two cmits.