New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug in absolute timelock usage #419
Fix bug in absolute timelock usage #419
Conversation
We have mixed up relative/absolute on the trait methods for `from_after` and `from_older`.
src/timelock.rs
Outdated
/// Returns true if `a` and `b` are the same unit i.e., both are block heights or both are UNIX | ||
/// timestamps. `a` and `b` are nLockTime values. | ||
pub fn absolute_timelocks_are_same_unit(a: u32, b: u32) -> bool { | ||
n_lock_time_is_block_height(a) && n_lock_time_is_block_height(b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if n_lock_time_is_block_height(a) == n_lock_time_is_block_height(b)
is more readable to other people, or just me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
Currently if we mix up height/time absolute timelocks when filtering policies the result is incorrect. Add a `timelock` module that includes a single public function for checking if absolute timelocks are the same unit. Use the new function to fix a bug in policy filtering.
Currently if we mix up height/time absolute timelocks when filtering policies the result is incorrect. Add a bunch of assertions that verify the bug.
9c744be
to
42e1216
Compare
Force push is |
Oh and I made the helper functions public because clippy warns otherwise, I think there is value in having both even though one is now not used. I specifically put the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 42e1216
Thanks for this, will catch up the lock time discussions upstream this week |
Edited the description to remove the @ mention |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently if we mix up height/time absolute timelocks when filtering policies the result is incorrect (to the best of my understanding).
Add a
timelock
module that includes a single public function for checking if absolute timelocks are the same unit. Use the new function to fix a bug in policy filtering.Note
There is ongoing discussion around trying to design a suitable timelock API. This PR is an attempt to make some forward progress by taking baby steps and making objective improvements. I decided to do it here in miniscript because it will be easier to iterate on and more obvious when there are concrete usage examples along with each change.
cc dpc