e2d03fef72 units: Manually implement serde traits (Tobin C. Harding)
Pull request description:
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`
ACKs for top commit:
apoelstra:
ACK e2d03fef72f4fcbdacd1cbecc0c2dddfbdeb3031; successfully ran local tests
Tree-SHA512: 6d3cc4106e44e08d59f8da6f8f3285923408ed05d1c0b2f6f2d83a22718744a38bf67381177856b5713810a5f5318d929daa2d12447c722628e0576fcd075e3d
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`
According to the `serde` docs:
> This is used in error messages. The message should complete the
> sentence “This Visitor expects to receive …”, for example the message
> could be “an integer between 0 and 64”. The message should not be
> capitalized and should not end with a period.
Use a lower case character to start the string.
According to the `serde` docs:
> This is used in error messages. The message should complete the
> sentence “This Visitor expects to receive …”, for example the message
> could be “an integer between 0 and 64”. The message should not be
> capitalized and should not end with a period.
However we have the `expecting` str using the converted type not the
thing the visitor expects.
Use `u64` instead of `FeeRate` since that is what is being parsed. Note
that in `amount` we got it _almost_ correct, subsequent patch will fix
the case.
fe577cd04e Implement Div<NonZeroU64|I64> for Amount, SignedAmount, FeeRate, and Weight (frankomosh)
Pull request description:
The pr implements `Div<NonZeroU64>` and `Div<NonZeroI64>` for the following types in `units` crate: `Amount`, `SignedAmount`, `FeeRate`, `Weight`
For handling owned/borrowed variants, each impl is wrapped in the existing `impl_op_for_references!` macro, which generates: `T / NonZero*` , `&T / NonZero*`, `T / &NonZero*`, `&T / &NonZero*`
close: #4442
ACKs for top commit:
Kixunil:
ACK fe577cd04e
apoelstra:
ACK fe577cd04e64488371aa62b872e2b88050d4948f; successfully ran local tests
tcharding:
ACK fe577cd04e
Tree-SHA512: b9b4a9f46d2fcf559d0a7f62ec397b6c2b174dd8ca9d80b37c0c393894ab4bc32019d64e2afcad1f780442b16aa3d15240ce607fc14b5e17b112243f7556b5b4
Adds an implementation of div by NonZeroU64 for Amount, FeeRate, and Weight
types. Also adds a div by NonZeroI64 for SignedAmount. The operations
helps to prevent div-by-zero errors at compile time, rather than runtime.
It follows same pattern as existing div operations
but leverages safety guarantees offered by non-zero types
a66ff8f8b1 Introduce `map` function for `NumOpResult` (Shing Him Ng)
Pull request description:
Closes#4476
ACKs for top commit:
Kixunil:
ACK a66ff8f8b1
apoelstra:
ACK a66ff8f8b143ebebb80ddc6b052cbc55e8dbb070; successfully ran local tests
Tree-SHA512: 63c79666685895033b9df0c46004fa4b042d038cc61e5ef443f56690be268ac6dd1ba461ab4f7d97c684e68623dfa53cdd37091f40ff7e6a5d3e53920c3fd40c
fd90c8782a Mark method as constant (yancy)
Pull request description:
Allow external const calls to access this method
Followup from https://github.com/rust-bitcoin/rust-bitcoin/pull/4428
ACKs for top commit:
tcharding:
ACK fd90c8782a
apoelstra:
ACK fd90c8782ab602861a715c72ca1481ed963e5c39; successfully ran local tests
Tree-SHA512: 2d04da9bddd58040972942e70096b2714405740236889f0909c00cb6993e38f3ae51ff05bbda13792a31243d3598f5976649590eecb0572c4f00c166f717399d
a7d059151e Assert error type (yancy)
2f7e74da45 Add MathOp helper methods (yancy)
Pull request description:
Follow up from https://github.com/rust-bitcoin/rust-bitcoin/pull/4428 to assert the error type which I agree improves the test.
Added some helper functions since it can be nice to see what type of overflow happened.
ACKs for top commit:
tcharding:
ACK a7d059151e
apoelstra:
ACK a7d059151eb47bf4202302604309c95c0d66371d; successfully ran local tests
Tree-SHA512: e1b3eba640de2e4f98e075270fd797582601c541e1eebef2959a4e609bf51129e8ad38baab1253b40474c39f82ee4802658ec545cc5c3590637f2ddb13f873f7
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
13cbead947 Use NumOpResult instead of Option (yancy)
002a0382aa Mark function constant (yancy)
Pull request description:
Prefer the more descriptive NumOpResult return type over Option where return types are fallible.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/4419
ACKs for top commit:
apoelstra:
ACK 13cbead94766987f59482b1fbc1d0ebd0799737c; successfully ran local tests
tcharding:
ACK 13cbead947
Kixunil:
ACK 13cbead947
Tree-SHA512: 1a870962dcafe901a07abd93bd8075e41696341c1a4b3efef615493c73d5e5728bbc2326f8c2c95b9034ab001d0b3c668c9d64793ab03486d3a19f31df907c96
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
We are going to delete MtpHeight in a couple commits, but to let us do
the transition with smaller diffs, we will first change it to be easily
convertible to a BlockHeight/BlockMtp pair.
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.
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 lot of duplicated code between BlockHeight and BlockInterval.
It obfuscates the differences between them: which timelock types they
can be converted to/from and what their arithmetic properties are.
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.
Prefer the more descriptive NumOpResult return type over Option where
return types are fallible.
The returned type NumOpResult is already annotated as must_use per the
lint, so must_use can be removed after changing the method return types
to NumOpResult.
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.
merge-script
Tobin C. Harding
frankomosh
Shing Him Ng
Fmt Bot
yancy
Jamil Lambert, PhD