The `units::locktime` types are used for two things:
- They are the inner types of `primitives` `LockTime`s
- They are used ephemerally for checking satisfaction
Neither of these use cases requires `serde` impls for the `units` types.
Since we are trying to release 1.0 with minimal amounts of code we
should remove them.
For `LockTime`s that need to be stored on disk or go over the wire we
can manually implement the `serde` traits. For `absolute::LockTime` this
is done already and there is no reason the `relative::LockTime` impl
cannot be the same [0]. This differs from the current `serde` trait impls
but we have already decided that in 0.33 we are going to accept breakage
and direct users to use 0.32 to handle it.
- Remove `serde` stuff from `units::locktime`
- Manually implement `serde` traits on `relative::LockTime`
- Fix the regression test to use the new format
While we are at it use a uniform terse call in `serialize`.
[0] This is because there is an unambiguous encoding for the whole set
of locktimes - consensus encoding.
4ccecf5dec Fix stale Height type link (Tobin C. Harding)
caebb1bf73 units: relative: Do minor rustdocs fixes (Tobin C. Harding)
40bb177bc2 Put is_satisfied_by functions together (Tobin C. Harding)
480a2cd62a Favour new function `from_mtp` over deprecated (Tobin C. Harding)
f9d6453d5b Shorten locktime type term (Tobin C. Harding)
727047bd39 Fix off-by-one-bug in absolute locktime (Tobin C. Harding)
3ffdc54ca5 Fix off-by-one bug in relative locktime (Tobin C. Harding)
a2ff8ddbbb Improve relative::LockTime is_satisfied_by_{height, time} (Tobin C. Harding)
Pull request description:
Make the APIs uniform in relative and absolute locktimes in relation to the `is_satisfied_by` functions. In doing so improve the API and fix an off-by-one bug when checking satisfaction of locks by height.
Done in three patches but maybe should be squashed? Probably easiest to review by looking at all the `is_satisfied_by*` functions and convincing yourself we got it right.
EDIT: Now has 5 cleanup patches also (mostly docs cleanups).
ACKs for top commit:
apoelstra:
ACK 4ccecf5decfead9818b74fbdee73115c349e2f3e; successfully ran local tests
Tree-SHA512: 9206cb464a06647510a35a7d564062823117e75df60251969be458616f4f5d04acf0aada53dbf7d493a2a2a72d26b3a300417a6499e45413d5f2a011538b7826
When checking a locktime against block height we add 1 because when the
next block is being validated that is what the height will be and
`is_satisfied_by` is defined to return true if a transaction with this
locktime can be included in the next block.
As we have in `relative`, and for the same reasons, add an API to the
absolute `Height` and `MedianTimePast`: `is_satisfied_by`. Also add
`is_satisfied_by_{height, time}` variants to `absolute::LockTime`.
Call through to the new functions in `units`.
Define 'is satisfied by' - this is a classic off-by-one problem, if a
relative lock is satisfied does that mean it can go in this block or the
next? Its most useful if it means 'it can go in the next' and this is
how relative height and MTP are used in Core.
Ramifications:
- When checking a time based lock we check against the chain tip MTP,
then when Core verifies a block with the output in it it uses the
previous block (and this is still the chain tip).
- When checking a height base lock we check against chain tip height + 1
because Core checks against height of the block being verified.
Additionally we currently have a false negative in the satisfaction
functions when the `crate` type (height or MTP) is to big to fit in a
u16 - in this case we should return true not false because a value too
big definitely is > the lock value.
One final API paper cut - currently if the caller puts the args in the
wrong order they get a false negative instead of an error.
Fix all this by making the satisfaction functions return errors, update
the docs to explicitly define 'satisfaction'.
For now remove the examples in rustdocs, we can circle back to these
once the dust settles.
API test of Errors:
Some of the errors are being 'API tested' tested in `primitives` but
they should be being done in `units/tests/api.rs` - put all the new
errors in the correct places.
We recently improved the relative locktime function `is_satisfied_by` by
adding mined at and chain tip. We can now do the same for the
height/time satisfaction functions.
Note I believe these functions should still be provided because a user
may for some reason have either blocktime data or height data and not
have the other.
Requires some work to the errors, elect to just remove the original
field that held the function argument.
For now remove the examples in rustdocs, we can circle back to these
once the dust settles.
Name the type exactly what it is. This used to be `Time`, then we tried
`MtpInterval`.
Note that this makes some of the original function names overly verbose
e.g., `NumberOf512seconds::from_512_second_intervals()` but given the
curlyness of locktimes too verbose is better than too terse. Also this
type, along with `NumberOfBlocks` is not going to be in very wide use so
the ergonomic hit is worth the additional clarity.
Name this type exactly what it is. Note for the error we just use
'height' even though this is a bit stale but the general concept is ok
in the error type because the name is long already.
47c77afaac units: delete MtpAndHeight type (Andrew Poelstra)
d82b8c0bcb primitives: stop using MtpAndHeight (Andrew Poelstra)
72d5fbad73 units: stop using MtpAndHeight in locktime::relative is_satisfied_by methods (Andrew Poelstra)
d933c754f5 units: change type of MtpHeight::to_mtp to BlockMtp (Andrew Poelstra)
dcbdb7ca8a units: add checked arithmetic to Block{Height,Mtp}{Interval,} (Andrew Poelstra)
4300271f0c units: add constructor for absolute::Mtp from timestamps (Andrew Poelstra)
4e4601b3d5 units: rename BlockInterval to BlockHeightInterval (Andrew Poelstra)
cb882c5ce1 units: add global `BlockMtpInterval` type (Andrew Poelstra)
4e3af5162f units: add global `BlockMtp` type (Andrew Poelstra)
a3228d4636 units: pull u32 conversions for BlockHeight/BlockInterval into macro (Andrew Poelstra)
Pull request description:
This is a more involved PR than I'd expected but hopefully the individual commits make sense and are well-motivated. Essentially, my goal was to replace `MtpAndHeight` as used by relative locktimes with a pair of `Mtp` and `Height`.
However, relative locktimes, when given a MTP/Height for the UTXO creation and the chain tip, are roughly modeled as "take a diff of MTPs to get a `relative::MtpInterval`, a diff of heights to get a `relative::HeightInterval`, and compare to the locktimes". *However*, we have no standalone MTP type to "take a diff of", and also there are failure modes when creating the diffs (e.g. if the diff would exceed the range of `MtpInterval` or `HeightInterval`).
So I backed up and decided to use the existing `BlockHeight`/`BlockInterval` as the type to "take a diff of". I needed to introduce a `BlockMtp`/`BlockMtpInterval` to work with MTPs. These types have full-u32 range, unlike the similarly-named types in `units::locktimes::absolute`. I then needed to add some conversion methods. Along the way, I cleaned up the APIs and documentation, added checked arithmetic, etc., as needed.
See the individual commit messages for more detail.
I believe the resulting API is much more consistent and discoverable, even though it has more surface than the old API.
I considered splitting this into 2 PRs but I think the first half of the changes aren't well-motivated with out the second half. Let me know.
ACKs for top commit:
tcharding:
ACK 47c77afaac
Tree-SHA512: ebe19a5b1684db8c2d913274347c994026aaa0dcdd79349c237920a82fe55560777278efdbbc7f1b1424c9391d9bbd891ae844db885deea75288000437a8a287
826acb8273 units: rename relative::HeightInterval constructors (Andrew Poelstra)
39b4f7670d units: rename relative::Height to HeightInterval (Andrew Poelstra)
d3619cc1bc units: deprecate relative::MtpInterval::to_consensus_u32 (Andrew Poelstra)
1a6b8b4c7a units: rename relative::Time to MtpInterval (Andrew Poelstra)
39b057fade units: rename absolute::Height consensus functions (Andrew Poelstra)
5a8f33f380 units: rename absolute::Mtp consensus functions (Andrew Poelstra)
8ffcd2cf30 units: rename absolute::Time to absolute::Mtp (Andrew Poelstra)
Pull request description:
This PR does a whole bunch of renames to the `units` locktime modules. These modules contain the underlying units for locktime types and hopefully aren't used directly too often, which should minimize the disruption from these renames.
This fixes a number of issues:
* Confusion between blocktimes and MTPs (which in fairness, *are* blocktimes, but they're not the one that users are likely to reach for)
* Constructor and conversion names that imply there is a "consensus encoding" for these bare units, which corresponded to the consensus encoding of the corresponding locktimes/sequence numbers
* `from_*` methods without `to_*` methods and vice-versa (overlaps with the above)
* the horribly named `value` method on relative heights and times (but not absolute ones)
This PR does **not** remove the `MtpAndHeight` type, nor does it add constructors for `Mtp` from lists of blocktimes. This is because the PR was too big already and I felt I should split it up.
Alternate to #4427
ACKs for top commit:
tcharding:
ACK 826acb8273
Tree-SHA512: 6e0491e17927625cde85c2cf92ff152a10613e632474122a626ee31b662d21c09fcb9fa3014c44708c97536535a33845cbbcd81e73dcdf98e9ee9fd6143c698f
Rename `value` to `to_height` to be symmetric with `from_height`;
deprecate `to_consensus_u32` which had no symmetric `from_consensus_u32`
and was only used to implement the corresponding methods in primitives
and bitcoin.
This is disruptive, but makes the type name consistent with
`MtpInterval` and also greatly improves clarity, helping to distinguish
between absolute and relative locktimes and reminding the author (and
reviewer) of locktime code that this needs to be a diff.
This method is weird. It's basically just used internally to implement
the locktime methods in `primitives` and `bitcoin`. It has no symmetric
from_consensus_u32.
Conversely the constructors from 512-second intervals have no symmetric
to_* method -- the inverse of these functions is called `value`, which
is a meaningless and undiscoverable name.
As with absolute::Mtp, there is no "consensus encoding" of a block
height, except that obtained by converting it to a locktime. For
symmetry with `Mtp`, rename the methods.
There is no "consensus encoding" for a MTP. The intention for these
methods was that a user could interpret the MTP as a locktime and then
consensus-encode that locktime. However, it was instead interpreted as
the MTP representing a *blocktime* as it is consensus-encoded in a block
header.
Evidence of this misinterpretation is in several doccomments, which
casually refer to the Mtp (which used to be just called Time) as a
"block time", which is simply incorrect.
This is not a generic UNIX timestamp, but rather a MTP restricted to
have values between 500 million and u32::MAX. Most importantly, it is
*not* a blocktime, which is what is implied by its name and
constructors.
If one wishes to verify a script that contains CLTV is valid in a
transaction then one must compare the argument to CLTV (the locktime) to
the transaction locktime. And to be valid the CLTV locktime must be less
than or equal to the transaction locktime. This usage kind of lends
itself to the term 'implied by' and we have a function already
`is_implied_by` that does exactly this.
Improve the docs by adding a section mentioning this usecase.
8b47068a2e feat(locktime): implement MtpAndHeight structure and validation logic (aagbotemi)
Pull request description:
This PR fixes#4299
- Computed MtpAndHeight structure
- Checked if relative time and height is satisfied by MtpAndHeight
- Compared the Ordering of MtpAndHeight with time and height
- Checked MtpAndHeight satisfaction and comparison in Locktime
- Added unit tests for all the implementation
I've reviewed and adhered to the contribution guidelines
ACKs for top commit:
apoelstra:
ACK 8b47068a2efada30aec21c61ae4be0da4d8e8fc8; successfully ran local tests
Kixunil:
ACK 8b47068a2e
tcharding:
ACK 8b47068a2e
Tree-SHA512: b00d1384d5deaa038b486ca9d77ad33cfa6cd8c987e08407863f2be8d540014bdcc971cd9d46acb51a2d105341accc04ba151e5cccb276e8352a5d45b33097eb
- Add MtpAndHeight for relative locktime checks
- Include unit tests for time/height comparisons
- Fix API design for mtp_as_time() error handling
- Update documentation and dependencies
- Fix BlockTime, CI, remove Ordering, and PR discussion fixed
- Fix UTXO height and timestamps
- Fix: chain_state and utxo_state handled seperately for is_satisfied_by
- Fix: panic on overflow fixed with check_add
- Fix: documentation updated and trailing whitespaces removed
- docs(mtpheight): documentation updated
- used accessors to_height and to_mtp over From impl
There was and inconsistent usage of `#`, `##` and `###` in rustdoc
headings. The difference in the rendered rustdocs is a minimal font
size change.
Change all headings to be H1 `#`.
Change all subheadings to be `###` to have a noticeable difference in
font size in the rendered docs.
Enable all the pedantic lints and fix warnings.
Notable items:
- `enum_glob_used` import types with a single character alias
- `doc_markdown`: add a whitelist that includes SegWit and OpenSSL
As part of the 1.0 effort and forward maintainability hide the internals
of the two error types in the `relative` locktime module. Doing so
allows us to remove the `non_exhaustive` attribute. Add getters to get
at the error innards.
It has turned out that the `rust-ordered` crate and it's
`ArbitraryOrd` trait are only useful for locktimes and only marginally
useful for them at best.
Remove the `ArbitraryOrd` impls and the `rust-ordered` dependency.
This topic was discussed in various places including:
- #2500
- #4002
- #3881Close: #4029
Currently it is possible to write
```rust
if this_locktime < that_locktime {
do_this();
}
```
with the hope that this code means if a locktime is satisfied by the
value in the other locktime. This is a footgun because locktimes are
incommensurate.
We provide the `is_satisfied_by` API to help users do locktime
comparisons.
Remove the `PartialOrd` implementation from both locktime types.
Users should be encouraged to explicitly use relative::* or
absolute::* instead of just LockTime, Time, or Height so that
it is clear when looking at the code if it is a relative or
absolute locktime.
The existing example at first look seemed to contradict the doc above
that the function would not round trip.
Update the example to show the useage for both a time and height based
lock time.
Replace unwrap() with ?
Back in 2022 we elected to use `mutagen` for mutation testing. Since
then `cargo mutants` has progressed to a point where we would now like
to use it instead.
Remove all the `mutagen` stuff and update the lock files.
Close: #2829
Tobin C. Harding
merge-script
Jamil Lambert, PhD
aagbotemi
Fmt Bot
ndungudedan