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 timelock module is putting in one place all the convoluted rules around nSequence and nLockTime so we have a chance of catching bugs and ensuring we handle all corner cases in the usage of these two u32s.

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 rust-miniscript or in my understanding. E.g., the FIXME below (from rust-miniscript/src/miniscript/types/extra_props.rs on branch rust-bitcoin/rust-miniscript#408

            Terminal::After(t) => {
                // Note that for CLTV this is a limitation not of Bitcoin but Miniscript. The
                // number on the stack would be a 5 bytes signed integer but Miniscript's B type
                // only consumes 4 bytes from the stack.
                let n = t.to_u32(); // FIXME: Is the following check correct?
                if n == 0 || (n & SEQUENCE_LOCKTIME_DISABLE_FLAG) != 0 {
                    return Err(Error {
                        fragment: fragment.clone(),
                        error: ErrorKind::InvalidTime,
                    });
                }
                Ok(Self::from_after(t))
            }
            Terminal::Older(t) => {
                if t.is_zero() || t.is_disabled() {
                    return Err(Error {
                        fragment: fragment.clone(),
                        error: ErrorKind::InvalidTime,
                    });
                }
                Ok(Self::from_older(t))
            }

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).

@tcharding I'm looking at that code and I first thing that is weird is that After and Older are written in a completely different style. One is using methods, the other one is raw u32 all the way. Kind of weird.

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:

if sentenceLikeExplanationOfTheCondition() { 

I wonder "what is the meaning of t.is_zero() || t.is_disabled()"?

Eehhhh... time to do some reading.

IMO, the is_disabled should be is_enabled(). Dealing with positives is just easier. nseqlock.is_enabled() reads better than !nseqlock.is_disabled() (double negation). And !nseqlock.is_enabled() reads naturally as "is not enabled".

And there should also be a enables_nlocktime() (!= 0xffffffff IIUC) (or does_enable_nlocktime - need to check the style guide)?

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:

impl Tx {
  fn is_nlocktime_enabled(&self) -> bool {
    self.ins.iter().any(|i| i.enables_nlocktime())
  }
}

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 nLockTime and nSequence, or it's an API about some abstract concepts of timelocks. Because these two are two different things, and would have to be designed differently. So far, it seems to me that you want to have an API to struct NLockTime and struct NSequence, because these are the actual entities that are there. I advocate for thinking in higher-level concepts, but making up stuff that aren't actually there is not the right approach. Reading the details on how these two work, I don't see natural generalization of a "timelock" concept. There are just bunch of rules that can be made more accessible and harder to get wrong, but not entirely abstracted away.

Edit: Or maybe you want both, but mixed them up?
Edit2: I guess you did aim at "abstract timelock", but since it's so intertwined with the details of the fields, it spills over everywhere. Simple thing: is_zero(). If you are talking about "timelock" as a concept, then there is no 0-timelock. From what I'm reading nTimelock == 0 means "there is no timelock". So why do we have is_zero on the "timelock"? It should be:

struct NTimelock {
   fn is_enabled(&self) -> bool {
      self.0 != 0
   }

   fn get_timelock(&self) -> Option<SomeTimelock> { ... }
}

or

struct AbsTimelock {
   fn from_ntimelock(n_timelock: u32) -> Option<Self> { ... }
}

if you really want to avoid struct NTimeLock. Once the user gets the AbsTimelock it should not have to be calling some magic is_constant() to get a correct result. The point of this abstraction was to free them from having to care about low level details.

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 is_enabled() or whatever, because it is already handled "by construction". "Make Invalid States Unrepresentable".

Users can just match and take care of different variants themselves, and some functions to help them could be provided:

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 Absolute::from_nlocktime should warn about effective dependency on inputs and recommend using instead:

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 do not want to learn another high-level API for timelocks which disagrees with all other documentation about timelocks in Bitcoin.

I have to respect that, but I see it as transliterating Bitcoin's Core C++ codebase to Rust.

The most I should need to know is what transaction fields would contain the locktime data, if it existed.

I am confused here. So, you do want only the constants?

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 API I posted in #994 (comment) does not expose any other ad-hoc uses. It take a u32 and gives you a corresponding "timelock" aspect of it (if enabled), I dropped the struct NLockTime and so on. I still think it would be better to have it, but it's an orthogonal concern.

In other contexts there are rules regarding finality/non-finality (which is purely a policy thing and non-consensus). You can use these values as (signed) counters alongside OP_CSV/OP_CLTV in some contexts (e.g. eltoo). At one point in the past the Liquid functionaries used these values to sideload some pegout-processing state. Future softforks may interpret these values yet differently.

You can still keep the raw u32 around and use different things to do other logic just fine. The entirety of consensus is a long gnarly list of things to check, sure. It doesn't prevent that each step being expressed using idiomatically typed code.

The API I posted in #994 (comment) does not expose any other ad-hoc uses. It take a u32 and gives you a corresponding "timelock" aspect of it (if enabled), I dropped the struct NLockTime and so on. I still think it would be better to have it, but it's an orthogonal concern.

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 is_zero because it's a useful check, and we definitely shouldn't be gating a constructor on it because 0 is a perfectly valid timelock.

I could go either way on is_enabled ... though it should probably return a "valid" timelock of value zero or infinity (I forget which one it's semantically equivalent to) rather than failing the constructor.

Are there other changes that you're proposing?

I don't think we should drop is_zero because it's a useful check, and we definitely shouldn't be gating a constructor on it because 0 is a perfectly valid timelock.

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:

https://github.com/rust-bitcoin/rust-miniscript/pull/408/files#diff-692f07910f91eef032ec530a82c31d6394e8346c0cf057c4ad8d7de3a2c89c24R834

The 0 case seems to have to be handled separately and user needs to remembered about it. And turning t == 0 into t.is_zero() is achieving literally nothing. The "abstraction" here is so anemic, that it is worse than nothing having it. Before I knew that t is just a primitive number and we are comparing it to 0, because we operate on a low level of abstraction, and 0 is some magic number. It is a low level concern code, and it was honest about it. Now, I have wonder "What does is_zero mean? Does it mean it "literally", in which case, why is it even a function? Or is there more to it?" t.is_disabled() is similar, though possibly more defensible. I would just make it a free fn is_timelock_disable_flag_set(x: u32) -> bool and stop pretending there's any useful abstraction here, and just a a shorthand function.

I'm sorry, I don't understand how this is meaningfully different from Tobin's API

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: None, and not magic zero value.

You look at definition of Absolute it and with a short docstring it reads simple "A restriction on spending UTXO that is absolute block or time based ". It's a concept. It's not a field.

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.

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 u32.

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 (struct NTimeLock and struct NSequence`), and a separate abstract concept of a timelock: "time based UTXO spending restriction". Among many other things the concrete fields can (but don't have to) enforcing a timelock. I just put it in types, and I'm fairly confident it will lead to easy to use and hard to mess up API.

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.

The 0 case seems to have to be handled separately and user needs to remembered about 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. is_zero) but this is not difference between a "C developer mindset" and a "Haskell developer mindset". It appears mostly to be a difference between somebody who knows how timelocks work in Bitcoin and somebody trying to infer it from the rust-miniscript source.

But nor should there be any 0-checking logic in the constructor.

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 Some(Absolute::Block(0)), and Absolute::from_ntimelock doesn't even have to return an option, because it always encodes some timelock.

It appears mostly to be a difference between somebody who knows how timelocks work in Bitcoin and somebody trying to infer it from the rust-miniscript source.

Ehh. Point taken. I thought that nTimeLock encoded either block or time, or if 0, is disabled. Since the 0-case actually is not special, the meaning of "nTimeLock the field" and "utxo spending time based spending restriction" is somewhat blurry. Not so much with a nSequence though, unless I'm still missing something. This one either enforces a timelock or not, and has more things inside than just

I still don't understand the point you're making.

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 u32, struct NTimeLock(u32); and enum Absolute { ... } and it's valuable. Right now if I want to know how timelocks in Bitcoin work, I can maybe find https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/struct.Transaction.html and see there's a u32 field there and that's it. Had it been a NTimeLock type, I could check it has a to_timelock() -> timelock::Absolute on it, and then click and see it has two cases with comments on it, and I wouldn't have to grep Bitcoin Core for some constants or google around and read bunch of lengthy documentations, just to get confused on some details.

Ehh. Point taken. I thought that nTimeLock encoded either block or time, or if 0, is disabled. Since the 0-case actually is not special, the meaning of "nTimeLock the field" and "utxo spending time based spending restriction" is somewhat blurry.

Correct. AFAIK nTimeLock is only ever used to encode timelocks and there are no "invalid" values. It is possible in Script to interpret it as a generic counter by using CLTV to enforce that it has a minimum height but this is perfectly consistent with an interpretation as a blockheight.

Not so much with a nSequence though, unless I'm still missing something. This one either enforces a timelock or not, and has more things inside than just

Yeah, nSequence is a mess. It was originally introduced as part of a broken scheme to allow transaction replacement, we believe based on historical evidence, and was later reused in replace-by-fee to signal "finality" or not. Slightly later we got the CSV opcode which interpreted it as a relative locktime. It kinda-sorta copied the height-vs-time rules from the timelock field but has extra rules to retain consistency with its other uses, leave room for future updates, etc. The result is really a mess.

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,

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 timelock::Absolute as an enum. I don't personally see any benefit in it over the Absolute(u32) version. I actually don't like it but cannot articulate exactly why at the moment. FTR I started out using an enum before moving to the Abs(u32).

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 @ away here or on Matrix, but at the moment I think I've exhausted my case. :)

Comment out is_le in rustdocs (because it requires Rust 1.53)

I went over the module docs again and made a few minor improvements, no other changes in force push.

Very cool! Thank you for the thorough documentation and examples.

Needs rebase:

   Compiling bitcoin v0.28.0 (/store/home/apoelstra/code/rust-bitcoin/rust-bitcoin/master)
error[E0308]: mismatched types
   --> src/util/bip152.rs:375:24
    |
375 |             lock_time: 2,
    |                        ^ expected struct `PackedLockTime`, found integer

For more information about this error, try `rustc --explain E0308`.
error: could not compile `bitcoin` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed
ERROR: Running 'cargo test && cargo clippy' failed.

Rebased and fixed the type in bip152, thanks.

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 shellcheck and polish them if you want.

@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 testcmd set to cargo test. It doesn't even enable all the features, it's really inadequate to what I'd like to be doing. But it does catch stuff like this :).

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/

Nice. I've added "setup bors" to my todo list.[

this is called not rocket science rule of software engineering

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 bech32 - bors on every repo!

Cheers man!