Skip to content

Commit 4d4adae

Browse files
committed
Merge #409: fix(dsl): handle invalid AbsLockTime without panicking
6980403 fix(dsl): handle invalid AbsLockTime without panicking (AmosOO7) Pull request description: ## Summary This PR fixes a potential panic in the descriptor! DSL macro when provided with an invalid absolute locktime. Previously, the `after(<value>)` rule used `.expect("valid AbsLockTime")` on `AbsLockTime::from_consensus()`. This caused the macro to panic if the value was `0` or exceeded the `MAX_ABSOLUTE_LOCKTIME`. The macro now correctly propagates a Result, ensuring the library remains crash-safe for all inputs. ### Changelog notice Fixed - dsl: handle invalid `AbsLockTime` without panicking ## Testing - Ran cargo test to verify all existing descriptor tests pass. - Added new unit tests specifically targeting the after() macro's boundary conditions and error mapping. - Verified that providing after(0) now returns a `Error::AbsLockTime` instead of terminating the process. ## Checklist * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `just p` before pushing #### Bugfixes: * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR Related issue Closes #408 ------------------------------ Thanks for the review! This ensures the descriptor! macro is robust against malformed or out-of-range absolute locktime values. ACKs for top commit: ValuedMammal: ACK 6980403 Tree-SHA512: aa0b90261a90c55ce9777dc4ec8009701a42716a4a5b6d46d94005185e9a85d904f70b2cbcab2f464e42e3e6360c4eec1b306f1cce887ce6d36638ced43b8a83
2 parents a910444 + 6980403 commit 4d4adae

File tree

3 files changed

+114
-1
lines changed

3 files changed

+114
-1
lines changed

src/descriptor/dsl.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,9 @@ macro_rules! fragment {
721721
$crate::keys::make_pkh($key, &secp)
722722
});
723723
( after ( $value:expr ) ) => ({
724-
$crate::impl_leaf_opcode_value!(After, $crate::miniscript::AbsLockTime::from_consensus($value).expect("valid `AbsLockTime`"))
724+
$crate::miniscript::AbsLockTime::from_consensus($value)
725+
.map_err($crate::descriptor::error::Error::from)
726+
.and_then(|abs_lock_time| $crate::impl_leaf_opcode_value!(After, abs_lock_time))
725727
});
726728
( older ( $value:expr ) ) => ({
727729
$crate::miniscript::RelLockTime::from_consensus($value)

src/descriptor/error.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,3 +131,9 @@ impl From<miniscript::RelLockTimeError> for Error {
131131
Error::Miniscript(miniscript::Error::RelativeLockTime(err))
132132
}
133133
}
134+
135+
impl From<miniscript::AbsLockTimeError> for Error {
136+
fn from(err: miniscript::AbsLockTimeError) -> Self {
137+
Error::Miniscript(miniscript::Error::AbsoluteLockTime(err))
138+
}
139+
}

tests/descriptor_macro.rs

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
///
66
/// This file shows the problem and how to catch it.
77
use bdk_wallet::descriptor;
8+
use miniscript::AbsLockTime;
89

910
#[test]
1011
fn test_older_with_invalid_rellocktime_too_large() {
@@ -117,3 +118,107 @@ fn test_rel_lock_time_error() {
117118
display_string
118119
);
119120
}
121+
122+
#[test]
123+
fn test_after_with_invalid_abslocktime_zero() {
124+
let invalid_value = 0;
125+
126+
let result = descriptor!(wsh(after(invalid_value)));
127+
128+
assert!(
129+
result.is_err(),
130+
"AbsLockTime of 0 should return an error in this context"
131+
);
132+
133+
// Check for the correct error variant
134+
if let Err(descriptor::DescriptorError::Miniscript(miniscript::Error::AbsoluteLockTime(_))) =
135+
result
136+
{
137+
// Success: Error was caught and mapped correctly
138+
} else {
139+
panic!("Expected AbsLockTime error, got {:?}", result);
140+
}
141+
}
142+
143+
#[test]
144+
fn test_after_with_valid_height() {
145+
// Values < 500,000,000 are interpreted as block heights
146+
let block_height = 840_000; // Example: Halving block height
147+
let result = descriptor!(wsh(after(block_height)));
148+
assert!(result.is_ok(), "Valid block height should work");
149+
}
150+
151+
#[test]
152+
fn test_after_with_valid_timestamp() {
153+
// Values >= 500,000,000 are interpreted as Unix timestamps
154+
let timestamp = 1715817600; // Example: A date in 2024
155+
let result = descriptor!(wsh(after(timestamp)));
156+
assert!(result.is_ok(), "Valid Unix timestamp should work");
157+
}
158+
159+
#[test]
160+
fn test_after_with_exact_max_valid_timestamp() {
161+
// 2,147,483,648 is the absolute maximum value supported
162+
// by this implementation of AbsLockTime.
163+
let max_valid = 2_147_483_648;
164+
let result = descriptor!(wsh(after(max_valid)));
165+
assert!(
166+
result.is_ok(),
167+
"Value 2,147,483,648 should be the valid upper bound"
168+
);
169+
}
170+
171+
#[test]
172+
fn test_after_with_invalid_just_above_max() {
173+
// 2,147,483,649 is too large for the internal representation
174+
let too_large = 2_147_483_649;
175+
let result = descriptor!(wsh(after(too_large)));
176+
177+
assert!(
178+
matches!(
179+
result,
180+
Err(descriptor::DescriptorError::Miniscript(
181+
miniscript::Error::AbsoluteLockTime(_)
182+
))
183+
),
184+
"Should fail specifically with a Miniscript AbsoluteLockTime error"
185+
);
186+
}
187+
188+
#[test]
189+
fn test_after_with_u32_max_is_invalid() {
190+
// Verify that u32::MAX actually triggers the error you're looking for.
191+
let max_value = u32::MAX;
192+
let result = descriptor!(wsh(after(max_value)));
193+
194+
assert!(
195+
result.is_err(),
196+
"u32::MAX is often rejected as an invalid consensus locktime"
197+
);
198+
}
199+
200+
#[test]
201+
fn test_abs_lock_time_error_mapping() {
202+
let invalid_value = 0;
203+
let abs_lock_result = AbsLockTime::from_consensus(invalid_value);
204+
205+
assert!(abs_lock_result.is_err());
206+
let abs_err = abs_lock_result.unwrap_err();
207+
208+
// Wrap in the general Miniscript Error enum
209+
let minisc_err = miniscript::Error::AbsoluteLockTime(abs_err);
210+
211+
// Convert to your local Error type using .into()
212+
let error: descriptor::DescriptorError = minisc_err.into();
213+
214+
// Assert it landed in the Miniscript variant
215+
assert!(matches!(error, descriptor::DescriptorError::Miniscript(_)));
216+
217+
// Verify the error message contains the expected text
218+
let display_string = format!("{}", error);
219+
assert!(
220+
display_string.contains("absolute locktime"),
221+
"Error message '{}' should mention locktime",
222+
display_string
223+
);
224+
}

0 commit comments

Comments
 (0)