fix(compile): compile only for the requested script context

[vadim-anfv]

The policy is compiled for all three script contexts one after another, before --type is looked at, and any of those failing aborts the command. So a policy that is valid for the type you asked for is rejected because it is invalid for one of the other two.

Here a 9-of-16 multisig is compiled with --type tr and fails on the sh limit: 16 keys don't fit into MAX_SCRIPT_ELEMENT_SIZE, the 520-byte consensus limit for the sh redeem script. Taproot has no such limit.

$ cargo run --all-features -- compile "thresh(9,pk(1),pk(2),pk(3),pk(4),pk(5),pk(6),pk(7),pk(8),pk(9),pk(10),pk(11),pk(12),pk(13),pk(14),pk(15),pk(16))" --type tr

thread 'main' panicked at miniscript-12.3.7/src/policy/compiler.rs:506:52:
Terminal creation must always succeed: ContextError(MaxRedeemScriptSizeExceeded)

The fix moves the compilation into the matching branch, so only the requested context is compiled.

Changelog notice

• Fixed compile rejecting policies that are valid for the requested script type

Checklists

All Submissions:

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

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.43%. Comparing base (e8fc1e3) to head (eadbdc7).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   61.14%   61.43%   +0.28%     
==========================================
  Files          23       23              
  Lines        4002     3993       -9     
==========================================
+ Hits         2447     2453       +6     
+ Misses       1555     1540      -15     

Flag	Coverage Δ
rust	61.43% <100.00%> (+0.28%)	⬆️

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.

[Musab1258]

ACK 6ed624e

• I pulled the change locally and ran the new test, which passes. I confirmed it fails on master with the miniscript MaxRedeemScriptSizeExceeded panic. I also ran a 2-of-20 pk threshold policy manually with --all-features: --type tr returns a tr(…,multi_a(2,…)) descriptor and --type wsh returns a wsh(multi(2,…)) descriptor. On master, wsh panics on the same policy.

• I suggest you add a wsh test with the same policy. The fix helps wsh, but only tr is covered in tests.

• --type sh with a legacy-oversized policy (same 2-of-20) still panics inside miniscript 12.3.7 (compiler.rs:506, exit 101) instead of returning an error. Worth a follow-up, either an upstream rust-miniscript issue or a clean error in bdk-cli?

[vadim-anfv]

ACK 6ed624e

Thanks for testing!

I suggest you add a wsh test with the same policy. The fix helps wsh, but only tr is covered in tests.

Added the wsh case to the test with the same policy, so both contexts are covered now.

--type sh with a legacy-oversized policy (same 2-of-20) still panics inside miniscript 12.3.7 (compiler.rs:506, exit 101) instead of returning an error. Worth a follow-up, either an upstream rust-miniscript issue or a clean error in bdk-cli?

On --type sh: already fixed upstream. In 12.3.7 from_ast returns ContextError(MaxRedeemScriptSizeExceeded) for a 20-key threshold and the expect turns it into a panic. rust-bitcoin/rust-miniscript#828 dropped it, released in 13.0.0, and master handles it with if let Ok(...), skipping such a policy instead of aborting. bdk-cli is still on 12.3.7 through bdk_wallet, so we get this fix as soon as we move to miniscript 13.

[Musab1258]

re-ACK 6e64d8f

I re-reviewed after the rebase and force-push. git range-diff shows the fix in src/handlers/descriptor.rs is unchanged; only the commit message, the CHANGELOG context, and the test differ. I confirmed the test, which has been renamed to test_compile_policy_beyond_legacy_limits, now covers tr and wsh. I ran the new test, and it passed.

I also confirmed that bdk-cli resolves miniscript 12.3.7 via bdk_wallet 3.1.0. PR #828 removes the .expect("Terminal creation must always succeed") in the compiler's terminal construction, which matches the panic location.

[tvpeter]

ACK 6e64d8f

Thanks for working on this @vadim-anfv .

While this addresses the issue stated in the PR description, it opens up a question (from the test) that we might investigate in another issue. Whether compile accepts non-key policies and generate invalid descriptors.

NIT: In the PR description, the statement "CHECKMULTISIG takes at most 15 keys" seemed incorrect. Maximum pubkeys per CHECKMULTISIG is 20 (check here). For sh, the redeem script is pushed as a single script element and must stay under 520 bytes (i.e at most 15 compressed keys). P2WSH can still use CHECKMULTISIG up to 20 keys.

[tvpeter]

Also, kindly rebase to fix the CHANGELOG confict.

[vadim-anfv]

Thanks for the review @tvpeter!

NIT: In the PR description, the statement "CHECKMULTISIG takes at most 15 keys" seemed incorrect.

Agreed, that was worded sloppily, I meant the sh limit, not CHECKMULTISIG itself. I've updated the description. With --type sh, 15 keys compile:

$ cargo run --all-features -- compile "thresh(9,pk(1),pk(2),pk(3),pk(4),pk(5),pk(6),pk(7),pk(8),pk(9),pk(10),pk(11),pk(12),pk(13),pk(14),pk(15))" --type sh

{
  "descriptor": "sh(multi(9,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15))#90zkckvc"
}

and 16 keys don't:

$ cargo run --all-features -- compile "thresh(9,pk(1),pk(2),pk(3),pk(4),pk(5),pk(6),pk(7),pk(8),pk(9),pk(10),pk(11),pk(12),pk(13),pk(14),pk(15),pk(16))" --type sh

thread 'main' panicked at miniscript-12.3.7/src/policy/compiler.rs:506:52:
Terminal creation must always succeed: ContextError(MaxRedeemScriptSizeExceeded)

it opens up a question (from the test) that we might investigate in another issue. Whether compile accepts non-key policies and generate invalid descriptors.

Yes, we already touched on it in #225. I have an idea for this, I'll open an issue with a proposed solution.

Also, kindly rebase to fix the CHANGELOG confict.

Done!

[tvpeter]

reACK eadbdc7