Skip to content

fix(descriptor): propagate threshold errors instead of panicking - #533

Open
busayo-OD wants to merge 1 commit into
bitcoindevkit:masterfrom
busayo-OD:fix-threshold-panic
Open

fix(descriptor): propagate threshold errors instead of panicking#533
busayo-OD wants to merge 1 commit into
bitcoindevkit:masterfrom
busayo-OD:fix-threshold-panic

Conversation

@busayo-OD

@busayo-OD busayo-OD commented Aug 14, 2026

Copy link
Copy Markdown

Fixes #532

Description

thresh_vec, multi_vec, and multi_a_vec used .expect(...) on Threshold::new, causing invalid runtime-supplied thresholds to panic instead of returning through the macros' usual Result<_, DescriptorError>.

For thresh_vec, the error is propagated through its existing Result chain. For multi_vec and multi_a_vec, keys::make_multi now accepts a closure returning Result<Terminal<...>, DescriptorError>, matching the existing make_sortedmulti pattern and removing the need for .expect(...).

Notes to the reviewers

miniscript already provides Error::Threshold for invalid thresholds, and DescriptorError already wraps miniscript::Error, so I reused the existing error path rather than introducing a new variant.

Added From<miniscript::ThresholdError> for DescriptorError and simplified all three fragments to propagate the error via ? directly.

Changelog notice

`keys::make_multi`'s closure now returns `Result<Terminal<DescriptorPublicKey, Ctx>, DescriptorError>` instead of `Terminal<...>` directly.

Checklists

All Submissions

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran just p before pushing

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 Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.92%. Comparing base (e13e8a0) to head (c53b9d9).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   81.84%   81.92%   +0.08%     
==========================================
  Files          25       25              
  Lines        6487     6541      +54     
  Branches      296      302       +6     
==========================================
+ Hits         5309     5359      +50     
+ Misses       1078     1075       -3     
- Partials      100      107       +7     
Flag Coverage Δ
rust 81.92% <100.00%> (+0.08%) ⬆️

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.

@Arowolokehinde Arowolokehinde left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this branch and ran all three tests locally. The fix correctly stops the fragment! macro from panicking on invalid thresholds and returns Err(DescriptorError::Miniscript(miniscript::Error::Threshold(_))) instead, routing errors through the existing Result channel rather than bypassing it with .expect().

@busayo-OD
busayo-OD force-pushed the fix-threshold-panic branch from d031217 to 7e1782a Compare August 19, 2026 12:35

@hash-ty hash-ty left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. The implementation could be cleaned up slightly by implementing From<ThresholdError> for DescriptorError, wrapping it in the Miniscript variant, and propagating the result of Threshold::new directly using ?.

thresh_vec, multi_vec, and multi_a_vec panicked via .expect() on
Threshold::new whenever k/n were invalid at runtime, even though the
surrounding macros return Result<_, DescriptorError>.

Adds From<miniscript::ThresholdError> for DescriptorError, reusing
the existing miniscript::Error::Threshold case, and propagates it
directly via ? in all three fragments.

keys::make_multi's closure now returns Result<Terminal<DescriptorPublicKey,
Ctx>, DescriptorError> instead of Terminal<...> directly.
@busayo-OD

Copy link
Copy Markdown
Author

This looks great. The implementation could be cleaned up slightly by implementing From<ThresholdError> for DescriptorError, wrapping it in the Miniscript variant, and propagating the result of Threshold::new directly using ?.

Thanks for the review. Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[dsl] Handle ThresholdError in fragment! macro

3 participants