Commit Graph

85 Commits

Author SHA1 Message Date
Andrew Poelstra 732a83c3a9
Merge rust-bitcoin/rust-bitcoin#4629: Improve locktime docs
70221ffa08 Fix link to use new name (Tobin C. Harding)
ebf92fcb01 Use ASCII in rusdocs (Tobin C. Harding)
64ece63f19 Add missing whitespace character to rustdoc (Tobin C. Harding)
64e10686de Improve rustdoc examples for absolute locktime (Tobin C. Harding)

Pull request description:

  In preparation for release go over the locktime rustdocs. 
  
  Close: #4463


ACKs for top commit:
  jamillambert:
    ACK 70221ffa08
  apoelstra:
    ACK 70221ffa085e3cd0a50683f788f5e78cdfae04ae; successfully ran local tests


Tree-SHA512: 67bc51400056e08018f92f8e7ff91c8f8db9e6b575e83f8937a382bde45541a31e75277e200694180fa5098b681e200858fb7b0c9330065af0019955ed126a61
2025-06-23 13:16:48 +00:00
Tobin C. Harding 70221ffa08
Fix link to use new name
Use `NumberOfBlocks` not the `Height` alias. Fixes link.
2025-06-20 08:41:59 +10:00
Tobin C. Harding ebf92fcb01
Use ASCII in rusdocs
Feels unnecessarily fancy, lets just use ASCII. Fix the column width to
be below the 100 character conventional width while we are at it.
2025-06-20 08:41:59 +10:00
Tobin C. Harding 64ece63f19
Add missing whitespace character to rustdoc
Fix trivial typo.
2025-06-20 08:41:58 +10:00
Tobin C. Harding 64e10686de
Improve rustdoc examples for absolute locktime
Improve the rustdoc examples and fleshing them out, using non-deprecated
functions, and returning an error if required (so we can use `?`).
2025-06-20 08:41:58 +10:00
Jamil Lambert, PhD bd50943234
Add a roundtrip test to kill a mutant
There is a mutant found in relative locktime `to_512_second_intervals`.

Add a round trip test to kill the mutant.
2025-06-19 20:33:57 +01:00
Tobin C. Harding f6dea36e31
units: Make error constructor private
We typically do not want to have public constructors on error types.
Currently we do on the `TimeOverflowError` so that we can call it in
`primitives` but it turns out we can just use `NumberOf512Seconds`
constructors instead.
2025-06-19 13:13:37 +10:00
Tobin C. Harding 7fbe07a6e0
Use uniform docs for overflow
Trivial change but make all the docs (rustdocs and code comments) use
the same phrase when describing overflow.
2025-06-10 06:43:52 +10:00
Fmt Bot 4b5c6dd547 2025-06-08 automated rustfmt nightly 2025-06-08 01:44:53 +00:00
merge-script a1be2fdf73
Merge rust-bitcoin/rust-bitcoin#4593: units: Add `is_satisfied_by` locktime tests
2fa5c062d5 Add is_satisfied_by locktime tests (Jamil Lambert, PhD)

Pull request description:

  Weekly mutation testing found new mutants in both `Height::is_satisfied_by` and `MedianTimePast::is_satisfied_by` functions.

  Test these two functions and kill the mutants.

  Closes #4587

ACKs for top commit:
  tcharding:
    ACK 2fa5c062d5
  apoelstra:
    ACK 2fa5c062d5e07580bdb7ea5e4c58e4607c716ecc; successfully ran local tests

Tree-SHA512: 28d69cdf575bb17eff6d685b1fee05e3f9a821c8796c82655b2d2eda6ee1d9dc79043853fbe0475f6bdb548cef52ac710b3c632f7784788035392e29e70ce48e
2025-06-06 18:25:58 +00:00
Jamil Lambert, PhD 2fa5c062d5
Add is_satisfied_by locktime tests
Weekly mutation testing found new mutants in both height and median time
past `is_satisfied_by` functions.

Test these two functions and kill the mutants.
2025-06-05 11:27:25 +01:00
Tobin C. Harding 4621d2bde1
Modify locktime serde implemenations
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.
2025-06-01 14:07:33 +01:00
merge-script 6d8299e8b8
Merge rust-bitcoin/rust-bitcoin#4468: Improve lock times - fix off-by-one bug
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
2025-05-31 15:48:29 +00:00
Fmt Bot c73131c8f0 2025-05-25 automated rustfmt nightly 2025-05-25 01:42:57 +00:00
frankomosh 0a0e23fedf style: use Self:: instead of type aliases in error impls
Use  instead of 'use FooError as E' when pattern matching
on the same type in Display/Error trait implementations.
2025-05-22 20:28:33 +03:00
Jamil Lambert, PhD fd0a756344
Add tests to relative locktime
New mutants found in weekly mutation testing.

Add tests to kill them.
2025-05-20 16:05:47 +01:00
Tobin C. Harding e2d03fef72
units: Manually implement serde traits
For types that have private inner fields we would like to manually
implement `serde` traits instead of deriving them.

This hardens the crate against making future mistakes if we want to
change the inner fields.

Do so for:

- `Weight`
- `BlockTime`
- `NumberOfBlocks`
- `NumberOf512seconds`
2025-05-14 14:31:39 +10:00
Tobin C. Harding caebb1bf73
units: relative: Do minor rustdocs fixes
Slightly improve grammar and fix column width to 100.
2025-05-12 12:48:05 +10:00
Tobin C. Harding f9d6453d5b
Shorten locktime type term
Use lock-by-time and lock-by-height instead of lock-by-blocktime and
lock-by-blockheight respectively with no loss of clarity.
2025-05-12 12:30:53 +10:00
Tobin C. Harding 727047bd39
Fix off-by-one-bug in absolute locktime
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`.
2025-05-12 12:30:53 +10:00
Tobin C. Harding 3ffdc54ca5
Fix off-by-one bug in relative locktime
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.
2025-05-12 12:17:31 +10:00
Tobin C. Harding dca4266205
units: Fix rustdoc column width
We've done a bit of renaming which has adversely effected rustdoc column
width, fix it up.
2025-05-08 10:16:57 +10:00
Tobin C. Harding d557caf552
Run the formatter 2025-05-08 10:16:56 +10:00
Tobin C. Harding 7c2115b68f
Rename MtpInterval to NumberOf512Seconds
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.
2025-05-08 10:12:23 +10:00
Tobin C. Harding 3a97ea2259
Rename HeightInterval to NumberOfBlocks
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.
2025-05-08 10:12:22 +10:00
Tobin C. Harding c3b7457f6c
Rename Mtp to MedianTimePast
Favour expressiveness over terseness for
`units::locktime::absolute::Mtp` (formerly `Time`) because in general
locktimes are curly AF.
2025-05-08 10:09:09 +10:00
Andrew Poelstra 72d5fbad73
units: stop using MtpAndHeight in locktime::relative is_satisfied_by methods
Keep using it in primitives; will remove it there in the next commit.
2025-05-06 18:51:03 +00:00
Andrew Poelstra 4300271f0c
units: add constructor for absolute::Mtp from timestamps
This is a convenience method to allow direct construction of
absolute::Mtp rather than going through units::BlockMtp.
2025-05-06 15:50:46 +00:00
Andrew Poelstra cb882c5ce1
units: add global `BlockMtpInterval` type
See the previous commit message for justification; for sensible
arithmetic on block timestamps we need the ability to do MTP
calculations on arbitrary MTPs and arbitrary intervals between them.
However, the absolute::Mtp and relative::MtpInterval types are severely
limited in both range and precision.

Also adds a bunch of arithmetic ops to match the existing ops for
BlockHeight and BlockInterval. These panic on overflow, just like the
underlying std arithmetic, which I think is reasonable behavior for
types which are documented as being thin wrappers around u32.

We may want to add checked_add, checked_sub and maybe checked_sum
methods, but that's out of scope for this PR.
2025-05-06 15:19:36 +00:00
Andrew Poelstra 826acb8273
units: rename relative::HeightInterval constructors
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.
2025-05-03 03:12:08 +00:00
Andrew Poelstra 39b4f7670d
units: rename relative::Height to HeightInterval
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.
2025-05-03 03:12:08 +00:00
Andrew Poelstra d3619cc1bc
units: deprecate relative::MtpInterval::to_consensus_u32
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.
2025-05-03 03:12:07 +00:00
Andrew Poelstra 1a6b8b4c7a
units: rename relative::Time to MtpInterval
The name `Time` is misleading. In fact this represents an interval
between MTPs.
2025-05-03 03:12:07 +00:00
Andrew Poelstra 39b057fade
units: rename absolute::Height consensus functions
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.
2025-05-03 03:12:05 +00:00
Andrew Poelstra 5a8f33f380
units: rename absolute::Mtp consensus functions
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.
2025-05-02 17:44:58 +00:00
Andrew Poelstra 8ffcd2cf30
units: rename absolute::Time to absolute::Mtp
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.
2025-05-02 17:44:58 +00:00
Jamil Lambert, PhD 7bcdd5ff66
Add regression tests for Display impl
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 `units`.
2025-04-30 18:21:09 +01:00
merge-script f9bc0f517d
Merge rust-bitcoin/rust-bitcoin#4344: Locktime MtpAndHeight
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
2025-04-28 13:49:12 +00:00
aagbotemi 8b47068a2e
feat(locktime): implement MtpAndHeight structure and validation logic
- 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
2025-04-25 12:24:23 +01:00
Jamil Lambert, PhD 913360b112
Make struct titles consistent
Structs had various phrasings of titles.

Make the wording consistent by concisely stating what it is, instead of
what it does.

Make the wording of all error structs consistent.
2025-04-15 21:23:31 +01:00
lfgtwo 89d61304af chore: remove needless question mark 2025-03-29 01:00:06 +08:00
Shing Him Ng bfba2a85dd Kill remaining mutants 2025-01-21 17:01:36 -06:00
Jamil Lambert, PhD f9be30ddbe
units: Fix `missing_errors_doc` clippy lint
Change the lint to `warn` in `units/Cargo.toml`.
Allow `missing_errors_doc` in `amount/serde.rs` and `fee_rate/serde.rs`.
Add missing `# Errors` sections to rustdocs where the lint gives a
warning.
2025-01-15 20:44:19 +00:00
Tobin C. Harding 55a999e0b5
Add serde roundtrip tests to relative locktime types
As we do for absolute locktime types add a couple of `serde` roundtrip
tests to the relative locktime types.
2025-01-06 17:56:15 +11:00
Shing Him Ng 286bf900b1 Address mutants in units
Preemptively addressing these mutants before introducing the
cargo-mutants workflow

There are several types of changes:
- Changes that address mutants that were actually missing
- Changes that address test values that cause `cargo-mutants` to think
  mutants were missed. For example, `cargo-mutants` will replace the
return values for unsigned integer types with 0 and 1. While a function
might be tested, the test might be testing the function with a call that
results in 0 or 1. When `cargo-mutants` substitutes the function call
with `Ok(1)`, the test will still pass, and it will consider this a
mutant.  `cargo-mutants` also replaces operations (+, -, /, %), bitwise
operations (&, |, ^, etc), so an operation such as `3 - 2` results in a
mutant because changing it to `3 / 2` yields the same result
- TODOs to ignore functions/impls in the future
2025-01-03 21:17:20 -06:00
Tobin C. Harding 0d8e9ef096
Remove usage of impl_from_infallible in leaf crates
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.

In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in all the leaf crates and just write the
code.
2025-01-02 07:51:21 +11:00
merge-script 1c405524e8
Merge rust-bitcoin/rust-bitcoin#3777: Implement Arbitrary for units types
04dfe8dd45 Add api test to check Arbitrary impls (Shing Him Ng)
678fc71b88 Implement Arbitrary for units types (Shing Him Ng)

Pull request description:

  Implement Arbitrary for the rest of the types in `units`. Also moved the implementation in `FeeRate` right before the `tests` module

  Closes #3705

ACKs for top commit:
  apoelstra:
    ACK 04dfe8dd45fae9b55dacfe9eb0d73ea306db14ba; successfully ran local tests
  tcharding:
    ACK 04dfe8dd45

Tree-SHA512: 156bd26d4de85d484711d476df1d2758805387125209f0307aa786dd1585ff9953dbe41b0864b00ae101419176647e3bde7994ed9257c18307d161463b1c8d2e
2024-12-21 17:30:19 +00:00
Shing Him Ng 678fc71b88 Implement Arbitrary for units types 2024-12-19 17:00:35 -06:00
Tobin C. Harding 5290a93a38
units: Add all pedantic lints
Add all the pedantic lints to the repository by way of the repository
manifest. Then enable these lints in the `units` manifest.

Some things worth mentioning:

- Fix `needless_pass_by_value` by adding derives to `FormatOptions`.
- Fix lint `cast_lossless` using `cargo clippy --fix``
- While fixing `lint enum_glob_use` introduce a new style to the
codebase; import enums using a single character. Doing so prevents
namespace clashes, improves clarity, and maintains terseness.

Audit:

Use the following lints locally and audit all the warnings, they produce
many false positives so we can't enable them permentently.

- `cast_possible_truncation`
- `cast_possible_lint`
- `cast_sign_loss`
2024-12-18 08:25:12 +11:00
merge-script 8e52f758b5
Merge rust-bitcoin/rust-bitcoin#3715: units: Add `must_use`
2fd8614f5d units: Add additional must_use (Tobin C. Harding)
79a229b391 units: Add pedantic lint return_self_not_must_use (Tobin C. Harding)

Pull request description:

  Enable return_self_must_use and also run the linter locally with must_use_candidate.

  Add must_use attribute as required excluding obvious functions (conversion, getters, etc).

  Done as part of https://github.com/rust-bitcoin/rust-bitcoin/issues/3185

ACKs for top commit:
  apoelstra:
    ACK 2fd8614f5d091377f179440b04ec71f4853fd187; successfully ran local tests; nice! will one-ACK merge

Tree-SHA512: 90868846d6877830ca2b6931e8b94208434acc5a7b21808a32e2e1568c0ad33185644b931cae500e6619b8b4f19c39bce6922502d9233173722ef0e6455edba6
2024-12-11 18:22:02 +00:00