When constructing an `Amount` it was observed recently that a `u32` is
often big enough. For the same reason we can use a `u32` to construct a
fee rate instead of overflowing.
Use `_u32`in the constructor and remove the panic.
b538a10956 Add deprecated functions to mutants exclude list (Jamil Lambert, PhD)
fd0a756344 Add tests to relative locktime (Jamil Lambert, PhD)
24cc059a78 Add tests to result (Jamil Lambert, PhD)
c1d2f0386d Add tests to block (Jamil Lambert, PhD)
Pull request description:
Weekly mutation testing found new mutants.
Add tests to kill the valid mutants.
Add deprecated functions to the exclude list so they are not mutated.
Closes#4488, Closes#4528
ACKs for top commit:
apoelstra:
ACK b538a1095652f535aeb15f6e3bbc44969db1ea88; successfully ran local tests
tcharding:
ACK b538a10956
Tree-SHA512: 8393ded6c073b2580fbb0fde9a8ce702a3d1e8c581c035870c2ba6a12d718cee577e345c9d92d0761552765248a6fb5ae9bbacbc88cac75e7153516de46de4ca
29b12bec6b Remove Display/FromStr for FeeRate (Tobin C. Harding)
Pull request description:
Parsing and displaying strings is a PITA. `FeeRate` is likely not that heavily used at the moment and users can always just call `to_sat_per_kwu()` to format it.
So we can get the `units-1.0-alpha.0` release out the door just remove the `Display` and `FromStr` impls for now. They can be added back in later when we have time to get #4339 in.
ACKs for top commit:
apoelstra:
ACK 29b12bec6b34148d6df9a4e6207132a667c53b4c; successfully ran local tests
Tree-SHA512: ffb49ab5d4f98be603eb1ca2f2e9d28ff7eaae66607eb9d2d5fef1f98ba2ac284a0007a86c3cae88f06e5b44f1e3e3ecb2014323b82ad4007e8ec59d6d04759b
Parsing and displaying strings is a PITA. `FeeRate` is likely not that
heavily used at the moment and users can always just call
`to_sat_per_kwu()` to format it.
So we can get the `units-1.0-alpha.0` release out the door just remove
the `Display` and `FromStr` impls for now. They can be added back in
later when we have time to get #4339 in.
9a9b41008f units: Make block-related types have private inner fields (Tobin C. Harding)
Pull request description:
We recently added a bunch of types in the `block` module that are wrappers around `u32`. When we did we slapped `pub` on the inner fields.
Lets be more mindful and make the inner fields private. Note all these types have `to_u32`, `from_u32` and `From` in both directions.
ACKs for top commit:
apoelstra:
ACK 9a9b41008f72cb8a00cfed2890d839aea2ea27c7; successfully ran local tests
Tree-SHA512: d4719bef57944000142ec110d4701486b4a7e5c5b24426379ed596ad83555ca1f75191fe618cc62f12b78b9d7a1ac5d18eff96bc407c9688b1210529c25329b3
We recently added a bunch of types in the `block` module that are
wrappers around `u32`. When we did we slapped `pub` on the inner fields.
Lets be more mindful and make the inner fields private. Note all these
types have `to_u32`, `from_u32` and `From` in both directions.
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.
Tobin C. Harding
merge-script
Jamil Lambert, PhD
Fmt Bot
frankomosh
Shing Him Ng
yancy