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
Add a LockTime
type
#994
Add a LockTime
type
#994
Conversation
src/blockdata/transaction.rs
Outdated
} | ||
|
||
/// Return the nSequence for `input_index` as a relative timelock. | ||
pub fn relative_lock_time(&self, input_index: usize) -> timelock::Rel { |
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 think this belongs rather on TxIn
.
On Transaction
you could implement something like maximum_relative_lock_time
which provides the most strict nSequence
that needs to be satisfied.
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.
There is no concept of 'maximum' for all inputs because some could be time based and some block based, right?
(I moved relative_lock_time
as suggested.)
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.
Ah, yes -- though you could have an accessor that returns the maximum height-based lock and maximum time-based lock. (It should probably be one accessor returning two values, rather than separate accessors, to remind the user that they need to check both normally.)
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.
Done in, commit: d6c19f9 Add maximum_relative_timelock method to Transaction
.
Not super clean, but shows the idea.
concept ACK. This looks really great! |
Thanks for the review @dpc! I'll work on your suggestions. |
Thanks for your comments @dpc, I'm not sure I get all the nuance you describe, I also am learning about timelocks myself. Solely from my perspective, the main benefit of the After working on this there are still some things that to me don't add up, I'm still unsure if we have bugs in
The code above also highlights why I did not add a trait, the two timelocks are not exactly the same - one has a concept of 'disabled' the other does not (IIUC). |
b084010 Use terse functional programming terms (Tobin C. Harding) 6f3303d Improve docs on TimelockInfo (Tobin C. Harding) b2f6ef0 Add unit test for combine_threshold (Tobin C. Harding) a36f608 Be uniform in spelling of timelock (Tobin C. Harding) 51de643 Rename comibine methods (Tobin C. Harding) ef6803f Use > 1 instead of >= 2 (Tobin C. Harding) d1fdbaa Use LOCKTIME_THRESHOLD same as bitcoin core (Tobin C. Harding) Pull request description: Done while working on a [timelock module](rust-bitcoin/rust-bitcoin#994). This is all the initial patches (except one) from #408 (which is a PR displaying usage of the new timelock module). Note, does _not_ do the 'make `TimelockInfo` fields pub crate' change - I was unsure if this was correct. Top commit has no ACKs. Tree-SHA512: aa54e2d884f7cb1fb5dcb2d828ada29830ac4a7a09b04797e6e2fb448df476cbb31345841735e6cf4d5b7b1f6783781859778805fffef708f259fe780c6ba677
@tcharding I'm looking at that code and I first thing that is weird is that Also, now that I see the actual usage: if t.is_zero() || t.is_disabled() { I can already see that these API methods might not be ideal. In a perfect case, the code like that would always look somewhat like:
I wonder "what is the meaning of Eehhhh... time to do some reading. |
IMO, the And there should also be a From what I can tell looking at some documents everybody seems to always talk about "disable flag" everywhere because whole Bitcoin community is by necessity bunch of C++ devs and they can't let go of low level encoding of bits. Once you know there's a bit somewhere that you set somewhere to 1 to disable something, every documentation, every api, everything has to be about "disable". I'm going to drive that point until it sticks. In a bigger picture the opposite of "disabled" is obviously "enabled" and the other side is just one negation operator away. It's much easier to talk in positives. "nLockTime on tx is enabled if any of the inputs enabled it" is easier to grasp than "nLockTime is disabled if all the inputs disabled it" - again, because no negatives. You might disagree if you know Bitcoin-fu is strong, but go ask someone that doesn't know about that bit and check. (I'm aware there's a corner case of no-inputs here to be consider, but that's an implementation detail). So:
|
This whole absolute vs relative name is meh. Just keeps confusing me. You've got to decide. It's either an API to help people work with Edit: Or maybe you want both, but mixed them up?
or
if you really want to avoid |
I'm looking at https://github.com/rust-bitcoin/rust-miniscript/pull/408/files as the only datapoint I have and my initial guess would be that: enum Absolute {
Block { num: u32 },
Time { seconds: u32 }, // or `seconds_times_512` or something?
}
enum Relative {
Block { num: u32 },
Time { seconds: u32},
}
impl Absolute {
fn from_nlocktime(nlocktime: u32) -> Option<Self> { /* check zero */ ... }
}
impl Relative {
fn from_nsequence(nsequence: u32) -> Option<Self> { /* check disabled flag */ }
} would be a good start. It takes care of the low level details, it forces correct usage. There is no Users can just impl Absolute {
fn is_same_unit(other: Self) -> bool {
match (self, unit) => {
(Block, Block) | (Time, Time) => true,
_ => else,
}
}
}
fn is_less_than(other: Self) -> Option<bool> { ... } // just append `.expect()` if you're sure type the same, or already checked with is_same_unit
fn is_less_or_eq_than(other: Self) -> Option<bool>;
} Critically, the impl Transaction {
fn get_effective_timelock(&self) -> Option<timelock::Absolute> {
if self.ins.iter().any(|i| i.enables_nlocktime()) {
Absolute::from(self.lock_time)
} else {
None
}
}
} and of course there should be impl TxIn {
fn get_timelock(&self) -> Option<Relative> {... }
} And that's kind of it. Some extra APIs will probably be useful (converting back to nlocktime/squence maybe, usual derives, etc.). Importantly, timelock abstraction gets decopuled from the fields it originates in. It can conveniently extract itself from a raw value, but after that it means only what it is supposed to mean. |
I do not want to learn another high-level API for timelocks which disagrees with all other documentation about timelocks in Bitcoin. I don't want a "timelock" API which also exposes a bunch of other ad-hoc uses of the nsequence and locktime fields in a transaction. The most I should need to know is what transaction fields would contain the locktime data, if it existed. |
I have to respect that, but I see it as transliterating Bitcoin's Core C++ codebase to Rust.
I am confused here. So, you do want only the constants?
The API I posted in #994 (comment) does not expose any other ad-hoc uses. It take a
You can still keep the raw |
I'm sorry, I don't understand how this is meaningfully different from Tobin's API (once various nits are addressed, which are hard for me to list because it's hard to reply to large chunks of inline code through github's API). It looks like you are expecting the user to provide the nsequence/timelock values to the API, which then gives you a timelock type with appropriate methods. I don't think we should drop I could go either way on Are there other changes that you're proposing? |
Big point of abstractions making life of the user easier, domain easier to understand and chances of making mistakes (forgetting about important details) lower. If I see: The
Importantly, it does not have any disabled/enabled methods/zero methods. There's less to remember about, less to get wrong. A "timelock" here is a meaningful abstraction ("a time/block restriction on spending tx/utxo"). Not a large one, but also not just a wrapper over nlocktime/nsequence. A nlocktime or nsequence either has a timelock in it, or not. If it is not "enabled", it is simply not there: You look at definition of
Yes, because "UTXO spending restriction" in Bitcoin comes from nTimeLock or nSequence, so there's got to be a way to get it from there. Currently there is no type for nTimeLock or nSequence, but that's an argument for another day, so I accept it have a static constructor that takes I think I kind of described my reasoning before, so I don't want to waste everyone's time, repeating myself. It all comes down to fundamental view on abstraction building. I have been writing C (and less, but some C++) for more than a decade in embedded, kernel and system software before I felt in love in Rust around 2014. I am (at least used to be) a low C/C++ developer. I did my fair share of tweaking bits and everything. In C/C++ the facilities for type-based modeling are weak. Usually the more you abstractions you build, the quicker you get UBs and other fun stuff. So people naturally and wisely use them only when really need. Abstractions tend to be shallow and anemic. And this works. It's kind easier to see through such abstractions, and with some discipline, competent people that know their domain, can get robust and performant software done. After years of writing Rust, I can contrast it with what I used to do. The reason Rust software is so lovely and easy to work with and immensely composable and collaborative is because people stop worrying, and just let the type system take care of almost everything. The focus is on modeling the domain using type system, building the right concepts and APIs. The code is just implementation details of the domain. And then any noob can come over, look at the types, read the API docs, learn the domain in the process, and fight the compiler until they have a robust and fairly correct project /PR . I see here concepts of a concrete field on blockchain encoded data ( I agree that there is an initial additional work in having to come up with correct abstractions that actually correctly describe the low level details, but I think it's just so worth it. |
This is a actually a Miniscript rule, due to the weirdness of Miniscript's type system (something that is way too low-level for even miniscript users to care about). Agreed, we should not bother having a special method here. But nor should there be any 0-checking logic in the constructor. Regarding your philosophical comments, my development experience is almost identical to yours and I still don't understand the point you're making. It sounds like you consider specific methods in Tobin's proposal to be poor abstractions, which is fine, we can remove them (e.g. |
Do you mean that a nTimeLock field with a zero value actually does enforce a timelock it's just its effectively "always valid" (at block 0 or later which means always)? Yes, I see. That's my mistake. In document I've read it said that "nTimeLock ==0 disables the timelock" (or I misunderstood it somehow). Then I agree, it should be
Ehh. Point taken. I thought that
There's a difference between the field and the meaning(s) of it, and it's possible to have the type system and the API express it, and it helps write correct code and other people reason about the rules. There's a difference between |
Correct. AFAIK
Yeah,
Okay, I can get behind this, and I could buy the argument that the transaction's timelock field could have a rich type because it has a single use, which is always enforced by consensus logic. But the sequence field's "meaning", I subimt, is a 32-bit value, and any further interpretations are context-dependent and don't make sense to apply to the actual transaction field. |
This one is going to take me a while to untangle. I've written a couple of messages now and deleted them. I might hack up a few different branches with various ideas discussed above and see what falls out. For a start I've push a commit that implements |
I had to unsubscribe for a couple of days, because I've spent a bit too much time in this one discussion, and I felt I was already pushing too hard and sounding way more arrogant than I actually am. I apologize if it came across rude or annoying. I didn't want to push anyone to agree with me, just hoped to explain why I think it has a lot of benefits. If you feel like debating some specific points, I'm always one |
src/blockdata/transaction.rs
Outdated
let mut max_block = None; | ||
let mut max_time = None; | ||
|
||
for input in self.input.iter() { |
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.
This could probably be more like:
use timelock::Relative::*;
this.inputs.iter().fold((None, None), |(block, time), input| {
match input.relative_timelock() {
Some(t @ Time { .. }) => (block, time.or_else(t).max(t).expect("same type")),
Some(t @ Block { .. }) => (block.or_else(t).max(t).expect("same type"), time),
None => (block, time),
}
}
+/- Relative::max
and mistakes
The readability aspect of it is arguable, depends how used to fold
one is, I guess, but hopefully it makes enum
look good. :D
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 58c1afe
Very cool! Thank you for the thorough documentation and examples. |
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 58c1afe
Needs rebase:
|
Integers within Script can have a maximum value of 2^31 (i.e., they are signed) but we (miniscript) often uses unsigned ints, to facilitate checking the unsigned type is the correct size to fit in a signed int add a const `MAX_SCRIPTNUM_VALUE`.
Add a `LockTime` type to hold the nLockTime `u32` value. Use it in `Transaction` for `lock_time` instead of a `u32`. Make it public so this new type can be used by rust-miniscript and other downstream projects. Add a `PackedLockTime` type that wraps a raw `u32` and derives `Ord`, this type is for wrapping a consensus lock time value for nesting in types that would like to derive `Ord`.
58c1afe
to
0ed78e5
Compare
Rebased and fixed the type in |
I see why you run your own pre-merge scripts @apoelstra and don't rely on the green github CI run, this would have been mergable with the bip252 build failure in it. Perhaps, if we are going to have more folks merge things we should put these pre-merge scripts somewhere. I know you said in the past that they were just your personal scripts, I could spend some time, along with my new best friend |
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 0ed78e5
@tcharding this is called not rocket science rule of software engineering (coined by the founder of Rust. :)) and there are bots that can handle it automatically. Rust has their own but there's also bors-ng which should be general enough for any projects. Perhaps we should try to set it up. |
@tcharding I'm literally just using the bitcoin core merge script with Agreed that in general it'd be great to have bors or something here. |
Bors is now available as a hosted service, I didn't know before a month ago or so . https://bors.tech/ |
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 0ed78e5
Nice. I've added "setup bors" to my todo list.[ |
Oh yes! No more finding CI fails on master and spending the day doing draft PRs on top of the fix and then rebasing them all the next day - like I did all day yesterday in |
071a1c0 Implement string parsing for `Sequence` (Martin Habovstiak) c39bc39 Extend `ParseIntError` to carry more context (Martin Habovstiak) Pull request description: When debugging parsing errors it's very useful to know some context: what the input was and what integer type was parsed. `ParseIntError` from `core` doesn't contain this information. In this commit a custom `ParseIntError` type is crated that carries the one from `core` as well as additional information. Its `Display` implementation displays this additional information as a well-formed English sentence to aid users with understanding the problem. A helper function parses any integer type from common string types returning the new `ParseIntError` type on error. To clean up the error code a bit some new macros are added and used. New modules are added to organize the types, functions and macros. Closes #1113 Depends on #994 ACKs for top commit: apoelstra: ACK 071a1c0 tcharding: ACK 071a1c0 Tree-SHA512: 31cb84b9e4d5fe3bdeb1cd48b85da2cbe9b9d17d93d029c2f95e0eed5b8842d7a553afafcf8b4a87c075aa53cf0274776e893bed6dca37e7dbc2e1ee1d602b8e
0ed78e5 Add lock time types (Tobin C. Harding) 1390ee1 Add a max scriptnum constant (Tobin C. Harding) Pull request description: Implement a `LockTime` type that adds support for lock time values based on nLockTime and OP_CHECKLOCKTIMEVERIFY. For example usage in `rust-miniscript` please see rust-bitcoin/rust-miniscript#408. ### Notes: I probably should have opened a new PR, this is a total re-write and very different from what is being discussed below, sorry, my bad. This is just half of the 'timelock' story, its the easier half. The reason I switched terminology from timelock to locktime is that we have to compare two u32s so it does not make sense to call them _both_ timelocks. If I have missed, or apparently ignored, anything you said reviewers please accept my apology in advance, it was not intentional. The thread of discussion is long and this topic is complex. Please do restate your views liberally :) Here is a useful blog post I used while touching up on timelock specifics: https://medium.com/summa-technology/bitcoins-time-locks-27e0c362d7a1. It links to all the relevant bips too. ACKs for top commit: Kixunil: ACK 0ed78e5 apoelstra: ACK 0ed78e5 Tree-SHA512: 486fcce859b38fa1e8e6b11cd2f494462d6d7d1d9030d096ce6b260f6c9d0342b8952afe35152bdf3afe323a234a8165ac3d6c946304afcc13406d7a0489d75a
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.
Post merge ACK.
Awesome work, thanks. That'll be helpful for us to not duplicate (part of) this logic everywhere downstream. :)
Cheers man! |
Implement a
LockTime
type that adds support for lock time values based on nLockTime and OP_CHECKLOCKTIMEVERIFY.For example usage in
rust-miniscript
please see rust-bitcoin/rust-miniscript#408.Notes:
I probably should have opened a new PR, this is a total re-write and very different from what is being discussed below, sorry, my bad.
This is just half of the 'timelock' story, its the easier half. The reason I switched terminology from timelock to locktime is that we have to compare two u32s so it does not make sense to call them both timelocks.
If I have missed, or apparently ignored, anything you said reviewers please accept my apology in advance, it was not intentional. The thread of discussion is long and this topic is complex. Please do restate your views liberally :)
Here is a useful blog post I used while touching up on timelock specifics: https://medium.com/summa-technology/bitcoins-time-locks-27e0c362d7a1. It links to all the relevant bips too.