Docs are clear but naming might be a little confusing.
Just from the name I would assume that this returns the underlying u32 of Sequence, not just the low 16 bits.

Maybe timelock_bits()?

or low_u16()

I like low_u16, cheers.

In 2e6a8aefc048c28f71d8a4efb5f1d2766354c9f2:

I don't think it makes sense for a locktime to be satisfied by a sequence no, or vice-versa. Rather, I would expect a sequence number to be convertable to a relative::Locktime, and then this would be satisfied by a blockheight and/or timestamp.

IOW I would replace this method with one that takes a height and a time, and then just passes through to is_satisfied_by_height() && is_satisfied_by_time().

Oh, if you keep the existing false returns (see below) then && should be replaced by ||.

I'm still not fully understanding where the height and time values would come from to pass into this new function. Would a user of this function have access to the chain and be expected to be able to compute the current height and time? I've based this (and the absolute::LockTime work) off of the usage in miniscript, is it that miniscript is not a normal user? How would we implement check_older in miniscript if we cannot use a Sequence number to satisfy the relative lock time?

Ah, I understand the question now. So, there are two pieces to locktime validation in Bitcoin:

1. The CHECKSEQUENCEVERIFY (and CHECKLOCKTIMEVERIFY) opcodes, which check the sequence/locktime values in a transaction against some "test" sequence/locktime value, and decides whether or not it passes. This is what rust-miniscript uses when reasoning about check_older/check_after.
2. Separately the Bitcoin blockchain checks whether the sequence/locktime values actually indicate a valid locktime, based on the state of the chain. This is what I think "normal users" are thinking of when they work with sequence/locktime values.

I would suggest that a "normal user" is more likely to be in situation (2), whereas situation (1) corresponds to Script interpretation. In this API we should somehow support both usecases somehow

I think that my suggested API, in which is_satisfied_by takes both a height and time, would serve both cases.

In case (1), rust-miniscript will see check_older(N), decide whether to interpret N as a height or time, and then use is_satisfied_by_height/is_satisfied_by_time as appropriate. In case (2) the user has both the current height and current time, and can use is_satisfied_by.

Miniscript additionally needs a way to serialize a height/time as a "sequence number" to embed into Script, and to parse out such a value.

Idea how to resolve having to come up with different names:

// maybe this doesn't have to be sealed, IDK
pub trait SatisfiesRelativeLockTime: sealed::SatisfiesRelativeLockTime {
    fn satisfies_relative_lock_time(self, lock_time: relative::LockTime) -> bool;
}

impl SatisfiesRelativeLockTime for Sequence { /* ... */ }
impl SatisfiesRelativeLockTime for (Height, Time) { /* ... */ }
// impl SatisfiesRelativeLockTime for (Time, Height) { /* ... */ } // for convenience so people don't have to remember the order?

impl relative::LockTime {
    pub fn is_satisfied_by<T: SatisfiesRelativeLockTime>(self, satisified_by: T) -> bool {
        satisfied_by.satisfies_relative_lock_time(self)
    }
}

Note: not implementing SatisfiesRelativeLockTime for relative::LockTime is intentional because sequence may disable it entirely. Not implementing it prevents accidents.

I like the trait idea. Implementing for Sequence goes against what @apoelstra says above

I don't think it makes sense for a locktime to be satisfied by a sequence no, or vice-versa. Rather, I would expect a sequence number to be convertable to a relative::Locktime, and then this would be satisfied by a blockheight and/or timestamp.

^ quote by Andrew from above

Note: not implementing SatisfiesRelativeLockTime for relative::LockTime is intentional because sequence may disable it entirely. Not implementing it prevents accidents.

If Sequence disables lock time it is not possible to construct a relative::LockTime from the sequence so this concern is addressed. I think we should implement for relative::LockTime, (Height, Time), and (Time, Height).

I implemented for Height and Time as well as for the tuple of both and documented ... excessively.

If Sequence disables lock time it is not possible to construct a relative::LockTime from the sequence so this concern is addressed.

I still didn't like the possibility of manual check but now I think maybe that's too much paranoia.

Oh, crap, I now realized it should've been the other way around. It makes sense to check if sequence is satisfied by (Height, Time)|RelativeLockTime but not that much for relative lock time.

In fb5abb43711a9f39e62f0e7b25dda8f85b32e98b:

The masking seems unnecessary.

I did it on purpose to make the as u16 explicitly safe because of the talk recently (@dpc and @Kixunil) about not using as. Can remove it (or leave it and document why).

I have a mild preference for removing it. I think the doccomment already justifies the loss of bits. The masking adds to the cognitive load.

Sure thing, thanks.

nit: Can this be an Option<T> instead? There is only one variant that can happen here. Most arithmetic operations in rust return an Option when the error is only possible before as overflow.
I would have suggested a dedicated error type, but it would be some work and a lot of code.

This would be annoying to unwrap because we not know all error variants

hmmm, a few considerations

1. There is an equivalent method on Sequence so we should do the same for both
2. It was a dedicated error type until I added the other variants
3. Option has less information because the error returns the input value, is that useful? I don't know really (@Kixunil is the resident "useful error message" bloke, what do you think man?)

I can do either

1. split the error into two different types
2. Use an Option as suggested

I tend to agree that 2 is the best

Either way, I'll do it as a follow up please - this one has been running for long enough now.

I personally prefer custom error types to Option even at the cost of added code. It provides more information when people blindly ? it. (and .ok() is possible if they want to ignore)

A special type would be better indeed.

To expand on this a bit, in my experience sprinkling .ok_or{_else} or map_err everywhere is quite annoying and not doing it will bite you when function deep in the call stack returns error (None) and you don't even know which one it was. For instance this is very common when people ? std::io::Error.

@sanket1729 could you raise an issue about this, if you feel strongly enough? I am going to merge this PR.

Furthering this discussion, I'm interested to learn if our method of having a single enum error type in each module (typically) is the correct way to go about things, alternatives (off the top of my head) are:

• An individual error struct for every error
• An enum for related errors
• An enum error per "some other semantic block" e.g. all of the networking code

My initial thoughts: for types convertible from byte arrays and such, parsing errors should be separate from other conversion errors - different layers-different errors
Functions that are very often called together can share error.
Everything else should have its own.

Splitting into crates will force us to at least split errors from independent parts.

Don't feel so strongly about it now. My workflow for detecting it is to try using it on a type that I know it should succeed on and match on error types before returning. I will raise issue/PR later about it