Skip to content

fix(dsl): handle invalid RelLockTime without panicking#404

Merged
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
AmosOO7:fix/rellocktime-error
Mar 20, 2026
Merged

fix(dsl): handle invalid RelLockTime without panicking#404
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
AmosOO7:fix/rellocktime-error

Conversation

@AmosOO7
Copy link
Copy Markdown
Contributor

@AmosOO7 AmosOO7 commented Mar 13, 2026

Summary

This PR fixes a panic that occurred when the descriptor! DSL macro was invoked with an out-of-range relative locktime value. Prior to this change, older(<value>) used .expect("valid RelLockTime") on RelLockTime::from_consensus(), which panicked if the supplied value did not fit within the consensus limit. The new implementation returns a proper error instead, bringing the macro in line with usual Result-based
error handling elsewhere in the library.

Changes

  • Updated dsl.rs older rule to match on the result of RelLockTime::from_consensus() and propagate errors.
  • Added comprehensive tests in tests/test_rellocktime_issue.rs:
    • valid common and boundary values
    • invalid values now return an error, not panic
    • demonstration of proper error handling
  • Updated documentation/comments and improved test coverage.

No public API changes were made; the fix affects only the macro and error enum.

Testing

All existing tests (cargo test) continue to pass. Added a new descriptor_macro.rs test file exercising the DSL macro directly, and ran full descriptor and wallet test suites to ensure there are no
regressions. All tests currently succeed.

Review Notes

  • RelLockTimeError already provides a helpful Display implementation courtesy of the miniscript crate; the error variant simply wraps it.
  • The fix is small and isolated; it should be straightforward to backport to earlier releases if needed.

Checklist

Bugfixes:

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

Related issue

Closes #403


Thanks for the review! This is a small but important stability fix that prevents unexpected panics when using the DSL macros programmatically.

@luisschwab
Copy link
Copy Markdown
Member

@AmosOO7 you need to squash these two commits

@AmosOO7 AmosOO7 force-pushed the fix/rellocktime-error branch from 3fc4208 to 0d20401 Compare March 13, 2026 13:47
@AmosOO7
Copy link
Copy Markdown
Contributor Author

AmosOO7 commented Mar 13, 2026

Done, thanks @luisschwab

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.91%. Comparing base (fca6523) to head (e101b9e).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   79.77%   79.91%   +0.14%     
==========================================
  Files          24       24              
  Lines        5266     5283      +17     
  Branches      241      241              
==========================================
+ Hits         4201     4222      +21     
+ Misses        988      984       -4     
  Partials       77       77              
Flag Coverage Δ
rust 79.91% <100.00%> (+0.14%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet Mar 13, 2026
@ValuedMammal ValuedMammal added the bug Something isn't working label Mar 13, 2026
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Mar 13, 2026
@AmosOO7 AmosOO7 force-pushed the fix/rellocktime-error branch from 9024418 to 442c242 Compare March 15, 2026 00:31
@ValuedMammal
Copy link
Copy Markdown
Collaborator

Thank you @AmosOO7. As a follow-up you could consider fixing the .expect for AbsLockTime::from_consensus in the dsl module, or I can open a new issue to track it.

@AmosOO7
Copy link
Copy Markdown
Contributor Author

AmosOO7 commented Mar 15, 2026

Thank you @AmosOO7. As a follow-up you could consider fixing the .expect for AbsLockTime::from_consensus in the dsl module, or I can open a new issue to track it.

Hello @ValuedMammal, thank you for the review, I will also take a look at the .expect for AbsLockTime::from_consensus in the dsl module, and I will make all the required changes on this PR

@AmosOO7 AmosOO7 requested a review from ValuedMammal March 16, 2026 15:43
@ValuedMammal ValuedMammal removed this from the Wallet 3.0.0 milestone Mar 17, 2026
@AmosOO7 AmosOO7 force-pushed the fix/rellocktime-error branch from 2a51030 to 2b3df6f Compare March 19, 2026 17:03
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Mar 20, 2026
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Mar 20, 2026
The `descriptor!` DSL previously used `.expect("valid RelLockTime")`
when converting an integer into `miniscript::RelLockTime`, causing a
panic for out-of-range values (e.g. values with the high bit set).

This change returns
`DescriptorError::Miniscript(Error::RelativeLockTimeError)` for
invalid values, so callers can handle errors cleanly instead of
crashing.

Includes new tests verifying valid/invalid values and ensuring the
macro no longer panics.

Ran cargo fmt and cargo clippy, and just p
@ValuedMammal ValuedMammal force-pushed the fix/rellocktime-error branch from 2b3df6f to e101b9e Compare March 20, 2026 05:17
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK e101b9e

@ValuedMammal ValuedMammal changed the title Fix/rellocktime error fix(dsl): handle invalid RelLockTime without panicking Mar 20, 2026
@ValuedMammal ValuedMammal merged commit a910444 into bitcoindevkit:master Mar 20, 2026
17 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Bug: panic in DSL macro when using invalid RelLockTime value

3 participants