Skip to content

Commit e101b9e

Browse files
AmosOO7ValuedMammal
authored andcommitted
fix(dsl): handle invalid RelLockTime without panicking
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
1 parent fca6523 commit e101b9e

File tree

3 files changed

+128
-1
lines changed

3 files changed

+128
-1
lines changed

src/descriptor/dsl.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,9 @@ macro_rules! fragment {
724724
$crate::impl_leaf_opcode_value!(After, $crate::miniscript::AbsLockTime::from_consensus($value).expect("valid `AbsLockTime`"))
725725
});
726726
( older ( $value:expr ) ) => ({
727-
$crate::impl_leaf_opcode_value!(Older, $crate::miniscript::RelLockTime::from_consensus($value).expect("valid `RelLockTime`")) // TODO!!
727+
$crate::miniscript::RelLockTime::from_consensus($value)
728+
.map_err($crate::descriptor::error::Error::from)
729+
.and_then(|rel_lock_time| $crate::impl_leaf_opcode_value!(Older, rel_lock_time))
728730
});
729731
( sha256 ( $hash:expr ) ) => ({
730732
$crate::impl_leaf_opcode_value!(Sha256, $hash)

src/descriptor/error.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,3 +125,9 @@ impl From<crate::descriptor::policy::PolicyError> for Error {
125125
Error::Policy(err)
126126
}
127127
}
128+
129+
impl From<miniscript::RelLockTimeError> for Error {
130+
fn from(err: miniscript::RelLockTimeError) -> Self {
131+
Error::Miniscript(miniscript::Error::RelativeLockTime(err))
132+
}
133+
}

tests/descriptor_macro.rs

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/// Test to demonstrate the RelLockTime issue in dsl.rs
2+
///
3+
/// The issue: older() macro uses .expect() on RelLockTime::from_consensus(),
4+
/// which can panic if given an invalid value.
5+
///
6+
/// This file shows the problem and how to catch it.
7+
use bdk_wallet::descriptor;
8+
9+
#[test]
10+
fn test_older_with_invalid_rellocktime_too_large() {
11+
// Value with high bit set causes RelLockTime::from_consensus() to fail
12+
let invalid_value = 0x80000000; // 2147483648
13+
14+
// This should now return an error instead of panicking
15+
let result = descriptor!(wsh(older(invalid_value)));
16+
assert!(
17+
result.is_err(),
18+
"Invalid RelLockTime {} should return an error",
19+
invalid_value
20+
);
21+
22+
// Check that it's the right kind of error
23+
if let Err(descriptor::DescriptorError::Miniscript(miniscript::Error::RelativeLockTime(_))) =
24+
result
25+
{
26+
} else {
27+
panic!("Expected RelLockTime error, got {:?}", result);
28+
}
29+
}
30+
31+
#[test]
32+
fn test_older_with_valid_rellocktime_max() {
33+
// Max valid value is 65,535 (0xFFFF)
34+
let max_valid_value = 65_535;
35+
let result = descriptor!(wsh(older(max_valid_value)));
36+
assert!(result.is_ok(), "Max valid RelLockTime should work");
37+
}
38+
39+
#[test]
40+
fn test_older_with_valid_rellocktime_min() {
41+
// Min valid value is 1 (0 is not valid for RelLockTime)
42+
let min_value = 1;
43+
44+
let result = descriptor!(wsh(older(min_value)));
45+
assert!(result.is_ok(), "Min valid RelLockTime should work");
46+
}
47+
48+
#[test]
49+
fn test_older_with_valid_common_values() {
50+
// Common usage: blocks or seconds
51+
let test_cases = vec![
52+
1, // 1 block/second
53+
144, // ~1 day in blocks
54+
1000, // Common value
55+
65535, // Max 16-bit value (common for CSV)
56+
4_209_713, // Valid but large
57+
];
58+
59+
for value in test_cases {
60+
let result = descriptor!(wsh(older(value)));
61+
assert!(result.is_ok(), "Valid RelLockTime {} should work", value);
62+
}
63+
}
64+
65+
// Alternative: Show how proper error handling should look
66+
#[test]
67+
fn test_demonstrate_proper_error_handling() {
68+
// This is what the fix should look like - proper Result handling
69+
// instead of .expect() which panics
70+
71+
use miniscript::RelLockTime;
72+
73+
// Valid case
74+
let valid_lock = RelLockTime::from_consensus(144);
75+
assert!(
76+
valid_lock.is_ok(),
77+
"144 blocks should be a valid RelLockTime"
78+
);
79+
80+
// Invalid case
81+
let invalid_value = 0x8000_0000;
82+
let invalid_result = RelLockTime::from_consensus(invalid_value);
83+
assert!(
84+
invalid_result.is_err(),
85+
"Value 0x{:x} should return a RelLockTime error",
86+
invalid_value
87+
);
88+
}
89+
90+
#[test]
91+
fn test_rel_lock_time_error() {
92+
// 1. Create the underlying RelLockTime error (high bit set)
93+
let invalid_value = 0x80000000u32;
94+
let rel_lock_time_result = miniscript::RelLockTime::from_consensus(invalid_value);
95+
assert!(rel_lock_time_result.is_err());
96+
97+
let rel_lock_time_error = rel_lock_time_result.unwrap_err();
98+
99+
// 2. Wrap it in the Miniscript error variant (matching your new macro logic)
100+
let minisc_err = miniscript::Error::RelativeLockTime(rel_lock_time_error);
101+
102+
// 3. Test the From impl (Miniscript -> DescriptorError)
103+
let error: descriptor::DescriptorError = minisc_err.into();
104+
105+
// Check that it matches the nested structure
106+
assert!(matches!(
107+
error,
108+
descriptor::DescriptorError::Miniscript(miniscript::Error::RelativeLockTime(_))
109+
));
110+
111+
// 4. Test the Display impl
112+
let display_string = format!("{}", error);
113+
114+
assert!(
115+
display_string.contains("relative locktime"),
116+
"Display string was: '{}'",
117+
display_string
118+
);
119+
}

0 commit comments

Comments
 (0)