d8377d90dd units: Remove serde derive feature (Tobin C. Harding)
1031851da4 units: Manually implement serde traits for block types (Tobin C. Harding)
Pull request description:
Currently we only need the `derive` feature of `serde` in test code.
Observe:
- We do not need the error testing logic because `ParseAmountError` is already exhaustively tested.
- The rest of the `serde` test logic in `amount` can be done using the public API so it can be moved to the integration test directory.
Move the unit test code to `tests/` excluding the error testing logic. Remove the `derive` feature from the `serde` dependency. Add a `dev-dependency` on `serde` that enables the `derive` feature.
ACKs for top commit:
apoelstra:
ACK d8377d90dd8d6bd066f51f45c23ffdfe2d0ab694; successfully ran local tests; nice!
Tree-SHA512: 03eb24ae1917e838a2e20c3c62ef9381e2a1eaccdb6474f60a2db59af98d9533054227af4c404013ea8deb4cfe4d57075ae4890857f8af283ebb4338ddb4ed3f
f746aecb61 Use NumberOfBlocks in rustdoc (Tobin C. Harding)
dc5249aae6 Use new type names instead of deprecated aliases (Tobin C. Harding)
Pull request description:
Recently we changed the names of some locktime types but kept aliases to the original names. To assist with ongoing maintenance lets use the new names already.
~Internal change only.~
Added a patch that does the same in some rustdocs.
ACKs for top commit:
jamillambert:
ACK f746aecb61
apoelstra:
ACK f746aecb61456f1eb97280e1d091434250832e72; successfully ran local tests
Tree-SHA512: f3304fcc62069f72fa6c49b4aee5579e4ef6e81cb466b9c7e79ddd1a04b63f2f4d3f0ffdcb9303c3bee2501beead3fceb9be79cefe35d7615223738291535152
2b07f59545 units: Fix up the api test (Tobin C. Harding)
a6ab5c9fd0 Implement Arbitrary for result types (Tobin C. Harding)
Pull request description:
A lot has changed over the last few months. Fix up the API integration test to match the current state of the crate.
Includes a patch to implement `Arbitrary` for types from the new private `result` module.
ACKs for top commit:
apoelstra:
ACK 2b07f59545de16b66b3bf8ee988757c2d0c14afb; successfully ran local tests
Tree-SHA512: fa9bd5b67ea6217cfb11a985183d2d83ae126678fa9fbe5b540fcec31efc0fbafb3e6010c7e831c84c9161300ad5ccc771f848875fdcfe09e072bb0d2881a104
Currently we only need the `derive` feature of `serde` in test code.
Observe:
- We do not need the error testing logic because `ParseAmountError` is
already exhaustively tested.
- The rest of the `serde` test logic in `amount` can be done using the
public API so it can be moved to the integration test directory.
Move the unit test code to `tests/` excluding the error testing logic.
Remove the `derive` feature from the `serde` dependency. Add a
`dev-dependency` on `serde` that enables the `derive` feature.
Recently we changed the names of some locktime types but kept aliases to
the original names. To assist with ongoing maintenance lets use the new
names already.
Internal change only.
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.
We now have constructors that take an arbitrary size fee
rate (`Amount`). The `from_sat_per_foo` constructors can be made
infallible by taking a `u32` instead of `u64`. This makes the API more
ergonomic but limits the fee rate to just under 42 BTC which is plenty.
Note we just delete the `from_sat_per_vb_u32` function because it is
unreleased, in the past we had `from_sat_per_vb_unchecked` so we could
put that back in if we wanted to be a bit more kind to downstream. Can
be done later, we likely want to go over the public API before release
and add a few things back in that we forgot to deprecate or could not
for some reason during dev.
Fuzz with a new function that consumes a `u32`.
As we did for `Amount` change the fee functions on `FeeRate` and
`Weight` to return `NumOpResult`.
This change does not add that much value but it does not make things
worse and it makes the API more uniform. Also reduces lines of code.
Currently we call the `Amount` fee calculation functions `div_by_foo`
and return an `Option` to indicate error. We have the `NumOpResult`
type that better indicates the error case.
Includes a bunch of changes to the `psbt` tests because extracting the
transaction from a PSBT has a bunch of overflow paths that need testing
caused by fee calculation.
The `Amount` and `FeeRate` types are not simple int wrappers, as such we
do not need to mimic the stdlib too closely. Having the `checked_`
prefix to the functions that do fee calcualtions adds no additional
meaning. Note that I'm about to change the return type to use
`NumOpResult` further justifying this change.
Note we leave the 'Checked foo' in the functions docs title because
the functions are still checked.
e7c90c57e7 Remove reachable unreachable call in psbt (Tobin C. Harding)
c736e73ae0 Add unwrap_or and unwrap_or_else to NumOpResult (Tobin C. Harding)
Pull request description:
A bunch of changes have been implemented lately in the fee calculation logic. As a result of this a at once time `unreachable` statement is now reachable - bad rust-bitcoin devs, no biscuit.
Saturate to `FeeRate::MAX` when calculating the fee rate, check against the limit arg, and win.
(Patch 1 adds the new combinators to `NumOpResult`. This was pulled out of #4610.)
ACKs for top commit:
apoelstra:
ACK e7c90c57e7b5ee20a2c523706cc4ea9957594857; successfully ran local tests
Tree-SHA512: d33b3d5d4442fb17cae4c52880bc5dafdd611d686c6923744bc5f218761e8bff32fc5ab169af9f2ed2f02011516ed850ac8c6bd4ce1d6de40c0ac0b17486bbb4
The `fee` module is now empty as far as API surface. It still holds the
`core::ops` impls for fee calculation. It also does have useful
rustdocs which are not currently visible online because module is
private.
Make the module public. Functionally all this does is provide a place
for the module level docs under a discoverable name.
Improve the docs by adding a bunch of links to fee related functions.
A while back we move all the 'fee' stuff into a separate module
because I thought it would help with clarity - I was wrong.
Move the checked mul function back into the `weight` module on the main
`Weight` impl block.
Internal change only - code move.
A while back we move all the 'fee' stuff into a separate module
because I thought it would help with clarity - I was wrong.
Move the checked mul and to fee functions back into the `fee_rate`
module on the main `FeeRate` impl block.
Internal change only - code move.
A while back we move all the 'fee' stuff into a separate module
because I thought it would help with clarity - I was wrong.
Move the checked div functions back into the `unsigned` module on the
main `Amount` impl block.
Internal change only - code move.
Two useful combinators, add them.
I copied the function signature and docs from stdlib and wrote the
functions myself - thereby operating within licensing requirements.
Some users may find it more ergonomic to pass in an `Amount` when
constructing fee rates. Also the strong type adds some semantic meaning
as well as imposes the `Amount::MAX` limit.
Add an equivalent constructor for each of the existing ones that uses an
argument of type `Amount` instead of `u64` sats.
7fbe07a6e0 Use uniform docs for overflow (Tobin C. Harding)
153a6a2f3c Make Weight docs uniform with FeeRate (Tobin C. Harding)
c87f7292be Fix rustdocs on Weight (Tobin C. Harding)
02b523a8ad Remove whitespace from encapsulate module (Tobin C. Harding)
Pull request description:
Make a sweep of the `units` crate's rustdocs.
ACKs for top commit:
apoelstra:
ACK 7fbe07a6e0b3e398aca845d64ec86f3f0068edf4; successfully ran local tests
Tree-SHA512: ba50f3afb94dda43f89d04eb53c6e85df302292d4647fe81a20e3f7d1ca75e8ee8cdf6548864b2f3c33ed661205d109dbd763db1061ea45a59eab25f134191f8
The `block` module does not follow the `encapsulate` pattern but we can
still use the getters instead of accessing the inner field directly.
Refactor, no logic change.
afc0ce6175 units: Add must_use to checked arithmetic functions (Tobin C. Harding)
Pull request description:
The checked arithmetic functions all consume self so we use `must_use` to help users not miss this point.
Most are done, add the missing ones.
ACKs for top commit:
apoelstra:
ACK afc0ce617554303d7fd25e052b039af44b6efc1c; successfully ran local tests
Tree-SHA512: 7105affff43827ed47a1c0b6e41a996aa538c7d53b891faf03e79a83164706d7e86db5fb184ac740fdf57bb43f8401a496cc64ea4da0da71eaa8c8cca16444c7
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
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.
4621d2bde1 Modify locktime serde implemenations (Tobin C. Harding)
200c276315 bitcoin: Make test code spacing uniform (Tobin C. Harding)
Pull request description:
Patch 1 is preparatory clean up. Patch 2 is the meat and potatoes. See commit log there for full explanation.
Briefly:
- Remove `serde` stuff from `units::locktime`
- Manually implement `serde` traits on `relative::LockTime`
- Fix the regression test to use the new format
ACKs for top commit:
apoelstra:
ACK 4621d2bde14d71b3d6ef0b14258a7479c049ba3b; successfully ran local tests
Tree-SHA512: bc96d79dd4d9f5a124c22f0d3be4750cb7b6e86ba448b31e233982567578337d331b2582c6e1f953c00e8393c4a4552c4b574fe8a55323c911115b269b24090e
56516757ad Add code comment to amount calculation (Tobin C. Harding)
8cf1dc39b5 Fix incorrect code comment (Tobin C. Harding)
7c186e6081 Refactor fee functions (Tobin C. Harding)
bf0776e3dd Remove panic using checked arithmetic (Tobin C. Harding)
b65860067f Make fee functions const (Tobin C. Harding)
9b2fc021b2 Improve rustdocs on FeeRate (Tobin C. Harding)
1bd1e89458 Re-introduce FeeRate encapsulate module (Tobin C. Harding)
b27d8e5819 Change the internal representation of FeeRate (Tobin C. Harding)
2e0b88ba76 bitcoin: Fix dust 'fee' identifiers (Tobin C. Harding)
399bca531c Reduce the FeeRate::MAX value (Tobin C. Harding)
d174c06a4a Saturate to_fee to Amount::MAX (Tobin C. Harding)
64ac33754f Add missing argument docs (Tobin C. Harding)
fe0a448e78 Temporarily remove const from fee calc function (Tobin C. Harding)
b929022d56 Add floor/ceil versions of to_sat_per_kwu (Tobin C. Harding)
64098e4578 Remove encapsulate module from fee rate (Tobin C. Harding)
Pull request description:
The `FeeRate` is a bit entangled with amount and weight. Also we have an off-by-one bug caused by rounding errors and the fact that we use kwu internally.
We can get more precision in the fee rate by internally using per million virtual bytes.
- Fix: #4516
- Fix: #4497
- Fix: #3806
ACKs for top commit:
apoelstra:
ACK 56516757ad8874d8121dba468946546bb8fd7d4e; successfully ran local tests
Tree-SHA512: 0c6e7e2c9420886d5332e32519838d6ea415d269abda916e51d5847aa2475c87fa4abfc29b5f75e8b10c44df67ae29d823aa93f7cfabbc89eb27f2173b103242
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
To get more precision use sats per million virtual bytes.
To make review easier keep most calls in tests using
`FeeRate::from_sats_per_kwu` and just unwrap. These can likely be
cleaned up later on if we want to.
For `serde` just change the module to `_floor` and leave it at that. The
serde stuff likely needs re-visiting before release anyways.
Currently we get the fee_rate per kwu then multiply it by 4. Instead
lets add a per_kvb function to `FeeRate`. We are about to change the
internal representation of `FeeRate` to use MvB so for now just panic on
ovelflow.
Also these are fee _rates_ - we already have suboptimal names from Core
in the consts in `policy`, no need to let them infect other identifiers.
In preparation for changing the internal representation of `FeeRate` to
use MvB reduce the max value by 4_000.
Done separately to make the change explicit.
The `FeeRate::checked_mul_by_weight` function currently returns a
`NumOpResult` but all other `checked_` functions return an `Option`.
This is surprising and adds no additional information.
Change `checked_mul_by_weight` to return `None` on overflow. But in
`to_fee` saturate to `Amount::MAX` because doing so makes a few APIs
better without any risk since a fee must be checked anyway so
`Amount::MAX` as a fee is equally descriptive in the error case.
This leads to removing the `NumOpResult` from `effective_value` also.
Note that sadly we remove the very nice docs on `NumOpResult::map`
because they no longer work.
Fix: #4497
Code that works with `const` is annoying to use and hard to reason
about. Just remove all the consts for now so we can hack on `FeeRate`.
Introduces two lint warnings about manual implementation of `map` but
they will go away later.
In preparation for changing the inner representation of `FeeRate` add
floor and ceil versions of the getter function `to_sat_per_kwu`.
For now both functions return the same thing but still call the
correct one so that when we change the representation we do not need
to re-visit them.
Fmt Bot
Andrew Poelstra
Tobin C. Harding
Jamil Lambert, PhD