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.
e2d9a8a0d8 primitives: Add an API test module (Tobin C. Harding)
8ec2d353c9 primitives: Derive Clone on witness::Iter (Tobin C. Harding)
Pull request description:
In preparation for 1.0-ing `primitives` add an `api` test module that makes an effort to verify the API surface.
This is similar to what is in `units` and what is in development for `hashes` (in #4017).
Note, there is a WIP attempt at this in #3992.
Close: #3928
ACKs for top commit:
apoelstra:
ACK e2d9a8a0d86f0e836a2a42b1803f8f9d96fde0ca; successfully ran local tests
Tree-SHA512: 5ec5c87c9aa5e86e579283a5485dcb2b3b5ae59359ae5ab96f8e6634285072bef0d0f111b6780852fd88fe29677f1a84c791a3343a0cb2b09093e77125f3962b
In preparation for 1.0-ing `primitives` add an `api` test module that
makes an effort to verify the API surface.
This is similar to what is in `units` and what is in development for
`hashes` (in #4017).
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
52940d4e12 Prefix unused variables with _ in rustdocs (Jamil Lambert, PhD)
a852aef4b8 Remove unused imports in rustdocs (Jamil Lambert, PhD)
Pull request description:
There is a lint warning about unused variables and imports in the rustdoc examples.
Remove the unused imports and prefix the unused variables with an underscore.
ACKs for top commit:
apoelstra:
ACK 52940d4e1216ad5118f7980cb2e6b8b425c61589; successfully ran local tests
tcharding:
ACK 52940d4e12
Tree-SHA512: 953862d546dc6e0bcd64172e8b383f0fc2a1a851971a1bcad0c1e30cbaeeaea993a0de7dd8b424c4ac1410053e179c52d0b5c90cd1b6560c27123b6b7fa49732
For our relative locktime API, we are going to want to take differences
of arbitrary MTPs in order to check whether they meet some relative
timelock threshold.
However, the `locktime::absolute::Mtp` type can only represent MTPs that
exceed 500 million. In practice this is a non-issue; by consensus MTPs
must be monotonic and every real chain (even test chains) have initial
real MTPs well above 500 million, which as a UNIX timestamp corresponds
to November 5, 1985.
But in theory this is a big problem: if we were to treat relative MTPs
as "differences of absolute-timelock MTPs" then we will be unable to
construct relative timelocks on chains with weird timestamps (and on
legitimate chains, we'd have .unwrap()s everywhere that would be hard to
justify). But we need to treat them as a "difference of MTPs" in *some*
sense, because otherwise they'd be very hard to construct.
There is a new lint error on nightly-2025-04-25 "variables can be used
directly in the `format!` string".
Exclude the lint to allow the existing syntax in `format!` strings.
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.
16f6fc6b5b Improve docs on absolute::LockTime::is_implied_by (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK 16f6fc6b5b469741c076bc3a4d92f7d4e619ffc7; successfully ran local tests
Tree-SHA512: f7de177927150ae27b3db187465e0f699da9e8e00a490b0354176b2dedc34d731fda40bf4d55324aa28b0d55f2f036653ab14d99b82d56705c929986db8fb30e
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.
The output of `Display` should not change in stable crates for types
that have well defined formatting and ones that implement `FromStr`.
Error types do not need to be tested.
Add missing tests for all implementations in `primitives` and round
trips for types that implement `FromStr`.
5d5a19793a Run the formatter (Tobin C. Harding)
2b72f1f30b Make Lower/Upper hex print scripts with len prefix (Tobin C. Harding)
c27b95fb0d Make script to/from hex use consensus encoding (Tobin C. Harding)
f4bc58dc48 bitcoin: Add a script encoding example (Tobin C. Harding)
Pull request description:
Done while investigating #3910 - this PR is quite invasive.
- Patch 1 adds an example that shows the current behaviour of various API calls for converting scripts to/from hex.
- Patch 2 adds a pair of functions that do not use the length prefix and makes script to/from_hex use the prefix.
- Patch 3 makes `LowerHex` and `UpperHex` use the len prefix also
- Patch 4 runs the formatter
Explicitly keeps serde untouched - i.e., without the len prefix
ACKs for top commit:
apoelstra:
ACK 5d5a19793aae73ff09d7064455a1a995eb796c28; successfully ran local tests
Tree-SHA512: dbbec372ea7b01818fce68ffb807a4531f068e10147e8bedf37b77f66a068a628e380549c379c061b868973e97806ada59729248b03bbd1cf6809f6098170cb6
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 the length prefix when formatting hex strings by way of `LowerHex`
and `UpperHex`.
This looses formatting options because I can't remember right now how
not to - again.
- 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
c4d9c1b9f8 Use a consistent rustdoc heading level of H1 `#` (Jamil Lambert)
6325a7cdea Change rustdoc heading level of references (Jamil Lambert)
f22e997587 Use parameters instead of arguments in rustdocs (Jamil Lambert)
e2c7be6d2f Fix typo (Jamil Lambert)
Pull request description:
In the rustdocs both `# Parameters` and `# Arguments` are used to mean the same thing. In a previous PR #2792 it was decided to go with Parameters everywhere. Since then there have been a few additions of "Arguments" into the rustdocs.
There is also a mix in the usage of `#`, `##` or `###` for headings. Noticed here https://github.com/rust-bitcoin/rust-bitcoin/pull/2792#issuecomment-2125775974.
- Fix a typo found when looking into this.
- Change all occurances to use `# Parameters` in rustdocs.
- Change all heading levels to H1 `#`
- Change all subheading levels to H3 `###` to make the small difference in the rendered font size noticable
Closes#4380
ACKs for top commit:
apoelstra:
ACK c4d9c1b9f8e59bf795812c42bd1eee68d97b9bbd; successfully ran local tests
Tree-SHA512: c8cc77ccf7e2003dd2dd1d309268624576e3bf390cd8ac61b0a7bb1141ca05377c83627576b0b7ff258b8e51c2d255097a4363fbdd1b368db7d32ac32ece58a1
9eaaf114b0 fix(witness): remove explicit type (aagbotemi)
5b572c5976 docs(witness): use constructs instead of creates (aagbotemi)
Pull request description:
This PR fixes#4379
- Removed explicit type
- updated documentation
- PR is in two seperate patches
ACKs for top commit:
apoelstra:
ACK 9eaaf114b0d6afb810cec8e9dac603ecadf64ff4; successfully ran local tests
Tree-SHA512: 65a6ca880c64aa81d83dffbfcd21a0427c874237a606d82ab70f226839adb164c9ee7fd1ecd5d8cee662660efcd1c458c523a27964cd1d00704d40a3bb6a1deb
6370aac7e1 Make Version::maybe_non_standard and Version::is_standard const (Shing Him Ng)
Pull request description:
Closes#4206
ACKs for top commit:
apoelstra:
ACK 6370aac7e119042d8b6b40021bbcd6324ad02bbb; successfully ran local tests
Tree-SHA512: aaf6afbe39d6568e0d05776a2a2f58a9a657f28cd3942391b599390bb4db974684e7d76cdf8db51a0fbcb948cdc4e19f22ba4b8ca39537e162cbd5bb2d4cd40e
cf42c511d8 primitives: replace error propagation with `expect` (Andrew Poelstra)
Pull request description:
This error path cannot happen, and if it could, failing formatting is not the correct source of action, because the std docs say that formatting may only fail if the formatter says so.
Fixes comment in #4269
ACKs for top commit:
tcharding:
ACK cf42c511d8
Kixunil:
ACK cf42c511d8
Tree-SHA512: 0ad47bb210dc71568bde426b8fd7782a4d48415fbf27133783975117d174b58eb791e68290e53f150a7e65236efda75032852bb6ed30bf4a182f18b9440cfcf9
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.
`Bitcoin Core References` used H3 `###` in rustdocs, and one occurrence
had another `References` heading above it.
Change them to be H1 `#` like other headings.
Remove the redundant `References` heading. NB. there are no other
occurrences of `# References` in this crate.
This error path cannot happen, and if it could, failing formatting is
not the correct source of action, because the std docs say that
formatting may only fail if the formatter says so.
Fixes comment in #4269
e0b627ea81 deserialize witness from a list of hex strings - ci(primitives): enable hex feature in CI build - from_hex() implemented more efficiently (aagbotemi)
Pull request description:
This PR implements `from_hex` function for deserialize `Witness` from a list of hex strings. Added unit test.
This PR fixes#4350
ACKs for top commit:
apoelstra:
ACK e0b627ea816a730949cdb200105598600fcac094; successfully ran local tests
tcharding:
ACK e0b627ea81
Tree-SHA512: deec3f9e5f67a0915b11a811c40c341dd9f24d0394d6cfbd6a09f765ce3fc0dcce2740949c264d4aa2d2db748a5ce81416b4dac15b1b64475a7c024b205e40ab
Tobin C. Harding
merge-script
Jamil Lambert, PhD
Fmt Bot
aagbotemi
Shing Him Ng