adding type aliases for the block height and unix time instead of just using a `u32` and `u64`

[LLFourn]

          A little off-topic for this PR, but how'd you'all feel about adding type aliases for the block height and unix time instead of just using a `u32` and `u64`? It would make the code easier to read and I don't think there are any drawbacks. Could put them in `chain/src/lib.rs`.

// The bitcoin block height, 0 is the genesis block.
pub type BlockHeight = u32;

// Number of seconds that have elapsed since 00:00:00 UTC on 1 January 1970, the Unix epoch.
pub type UnixSeconds = u64;

Originally posted by @notmandatory in https://github.com/bitcoindevkit/bdk/issues/1002#issuecomment-1602955088

[ValuedMammal]

I think once this is done, the satoshi u64 will be one of the last remaining holdouts that refuses to have a friendly type.

[tnull]

If this is refactored anyways, wouldn't it make sense to introduce a dedicated type for UnixSeconds, or at least re-use the Duration type?

[ValuedMammal]

UnixSeconds can be a tuple struct that holds a u64

[15IITian]

@LLFourn Should we make them tuple struct or not? Otherwise struct fields like blockheight would show u32 as type not BlockHeight.

[apoelstra]

You may want to use (or alias) this type from rust-bitcoin https://docs.rs/bitcoin/latest/bitcoin/blockdata/locktime/absolute/struct.Height.html and/or the corresponding Time structure.

For seconds there is also std::time::Duration for some situations.

[15IITian]

so should I direcly use them or create an alias with the name given above. ( For BlockHeight -> no need to create alias of Height but for UnixSecond -> I think , It will better to make an alias of Time as this term is often used ) ?

[notmandatory]

I agree we should use rust-bitcoin BlockHeight and that for UnixSeconds it might be better to wrap the u64, unless it's already done somewhere in the core lib (for no-std compatibility) but I couldn't find anything. Good summary of the pattern here: https://effective-rust.com/newtype.html.

[15IITian]

@notmandatory

I want to ask that why we are using u64for UnixSeconds , as Time struct in Rust-Bitcoin library is using u32 respectively . Since both are meant for the same functionality -> so shouldn't we use u32 data types for it -> then we would just use Time struct in our code instead of making custom struct.

[notmandatory]

@15IITian which Time struct do you mean? The closest equivalent to what I think we need in BDK is block::Header.time which is a u32. But even that isn't exactly what we're tracking in the BDK chain structures which track first time WE saw the data (not what a miner put in the block header). In that case I think it makes sense to create our own UnixSeconds(u64) struct. This also avoids running into any u32 year 2038 (+68) issues (if BDK is still being used in 2106).

[apoelstra]

This also avoids running into any u32 year 2038 (+68) issues (if BDK is still being used in 2106)

As commented in BIP 65 OP_CSV Bitcoin itself has a year-2106 problem in its locktime field. So it may make sense to replicate that problem here rather than allowing users to create transactions that appear to be valid but are actually consensus-invalid.

[notmandatory]

For building transactions with a locktime I completely agree we should continue to use the rust-bitcoin absolute::LockTime enum which uses the absolute::Time(u32) struct.

The UnixSeconds() struct I have in mind would only be used for internal timestamps in thebdk_chain crate, eg. the last time we saw a Tx in the mempool. That said, the rust-bitcoin locktime::absolute::Time structure would also work. But the goals of this issue is to make the code easier to read and I think using a locktime module related struct for timestamps unrelated to locktimes is potentially confusing.

[LLFourn]

I agree we should distinguish between u64 which is the timestamp we last saw a transaction vs actual locktime. I think we could use the rust-bitcoin definition for actual chain confirmation time though since that's what's going to need to be converted to do relative lock time checks with miniscript etc.

[notmandatory]

To put the current proposal into nuts and bolts, in chain_data and code that calls it, change:

• u64 used for confirmed block or transaction time to rust-bitcoin locktime::absolute::Time
• u64 used for unconfirmed block or transaction time to a new UnixSeconds type
• u32 used anywhere for block or transaction height to rust-bitcoin locktime::absolute::Height

Since we don't really get any benefits from our own UnixSeconds type, I think it would be also be OK to use rust-bitcoin locktime::absolute::Time also for unconfirmed time since it gives us the extra sanity checks that it's a valid bitcoin protocol time. Sorry to flip-flop on my prior argument about readability, but it's only the package Time lives in that could be a little confusing, the actual docs are clear that it's just a valid Unix timestamp.