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
dd3f3e44bc Split into_derivation_path tests out (Tobin C. Harding)
0c9dd31f53 Test with m prefix (Tobin C. Harding)
c5073f4c51 Refactor simple valid path tests into a loop (Tobin C. Harding)
3e7fdad5fd Split empty master test out (Tobin C. Harding)
ed36a980f8 Refactor invalid derivation path tests (jamillambert)
015fb1be3b Split invalid derivation path test (jamillambert)
76dd6100a2 Split derivation path test into valid and invalid (jamillambert)
Pull request description:
Refactor tests like its our job.
Co-developed-by: Jamil Lambert <jamil.lambert@proton.me>
ACKs for top commit:
apoelstra:
ACK dd3f3e44bc3329830aacacb252162c57858b0029; successfully ran local tests
Tree-SHA512: 8907ff127c11767a600cf856fb34645eb78392c755998565675a026dc4ddd9db00083963414f7e45fc7a2194ad3c5f251f7249bc7d7a2150a5840d1ec8b5d4da
e9f7ae4b74 Correct hash algorithm name in RIPEMD160 documentation (David Klank)
Pull request description:
Corrected the documentation comment for the `new()` method in the RIPEMD160 `HashEngine` struct. The comment incorrectly stated it constructs a "SHA256 hash engine" when it actually constructs a RIPEMD160 hash engine.
ACKs for top commit:
tcharding:
ACK e9f7ae4b74
apoelstra:
ACK e9f7ae4b74a4d2728b4b64bbb8fc2db92fefec3a; successfully ran local tests
Tree-SHA512: 3935f502709dc8ec570d7f112efbc4d55349f7072342ad0278ee2525bbf39ca1d7f60bcef0ad0a3bdcf3e67b2548d89bc01435532d848595a9824f4ec9e74bb7
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.
a1e8fabbbe Remove unnecessary closure (Tobin C. Harding)
Pull request description:
We can just pass the function directly. Found by clippy bizarrely after running `rustfmt` (in bot-created PR #4586).
Internal change only, no logic change.
ACKs for top commit:
yancyribbens:
cr ACK a1e8fabbbe
apoelstra:
ACK a1e8fabbbe26cbe62e1a966cc0c760407bfb063f; successfully ran local tests
Tree-SHA512: 19d5ee65da7cfdae82a9a6609afac6d6b03d1c44c0c5bed21916ec31665e76adfc836ac6a1980251db2b9d5704baa577783dc967b1f0035cf662389c87fc6a0a
028a0d6558 Remove conversion impl macro for `Magic`/`Network` (rustaceanrob)
Pull request description:
Closes#4560 (or at least one way to close it)
Handle the coversion of new networks directly in the `From` and `TryFrom` implementations, as new networks are added infrequently.
ACKs for top commit:
tcharding:
ACK 028a0d6558
apoelstra:
ACK 028a0d6558129f554de8c4247481bc0ad80f1c27; successfully ran local tests
Tree-SHA512: 07b768e229305878849f23e54d3fb4940a736ce44122950e4f4bf68ddeb4f82f2d35020840e8176bd7b562726e46055650ba6de8559bae7559c881b64a437169
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
We can just pass the function directly. Found by clippy bizarrely after
running `rustfmt` (in bot-created PR #4586).
Internal change only, no logic change.
We have a single test case that tests for the m prefix while all the
others do not.
Move one test case into the loop and then test on each iteration the
path with m prefix added.
cbe04b00c6 Remove all `p2p` dependency from `network` (rustaceanrob)
Pull request description:
Motivated by moving the `p2p` module to its own crate. `TryFrom` and `From` are already implement for converting to and from `Network`/`Magic`. The methods related to `Magic` are removed from `Network`, as well as any reference to `p2p` in the documentation, as `bitcoin` would no longer depend on `p2p`.
The deser roundtrip test are relocated to `p2p/mod.rs`
ACKs for top commit:
tcharding:
ACK cbe04b00c6
apoelstra:
ACK cbe04b00c67eab896b7ec0535194771ec36cb68f; successfully ran local tests
Tree-SHA512: ac3aa2eef4d78efd719ccc032a4266827faff8c87997111fa0050765b732462e5e5811c8aa923aedba335cbaad9a374fd54dbbe0f4978a1111d0839a5942af7d
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.
de320714fb Introduce empty p2p messages crate (rustaceanrob)
Pull request description:
Peer to peer messages change on occasion, and we would like to add messages as they are released from Bitcoin Core. Add an empty crate to take the crate name.
ACKs for top commit:
tcharding:
ACK de320714fb
apoelstra:
ACK de320714fbe46e4fda858d5c1c07f5dd1cc842f0; successfully ran local tests
Tree-SHA512: 68c8080c524c30eb5ec0aca14e74c8196a7cfffabe32d47773d56b8a64850617094b3c2088dfce20b4abfca31c82b19369703722d0a98aee636c383318953da7
20779bdbc8 Move `p2p` encodings out of `encode` (rustaceanrob)
94bcff28b1 p2p: Add wrappers for messages that use `Vec` (rustaceanrob)
Pull request description:
To move `p2p` into a crate:
1. All `Encodable` implementations for `p2p` must be defined in `p2p`. To accomplish this, and anything else in the future related to encoding, I added a `deser` module with the necessary macro to implement `Encodable` for `Vec<T> where T: Encodable`.
2. Because the orphan rule would apply for implementing `Encodable`for `Vec` within `p2p`, three types defined within `p2p` are used to wrap the `Vec` and snake around the orphan rule.
ACKs for top commit:
tcharding:
ACK 20779bdbc8
apoelstra:
ACK 20779bdbc8ebcac2365d122e857c23b3d8e8d1e7; successfully ran local tests
Tree-SHA512: 7abfe5b853e84bb465579309f80a0687c325217f6f342651278bedf540c4d17ae16683ab02dea5e3d98d90790deb12c6bc7d572a92cec8e19ff94e395bc0d29c
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.
357ae4051a fix typo Porallel to Parallel crypto.rs (Fallengirl)
Pull request description:
Hi devs! Found and fixed one typo in `hashes/src/ripemd160/crypto.rs`
`Porallel` - `Parallel`
ACKs for top commit:
tcharding:
ACK 357ae4051a
Tree-SHA512: 67b166f8f0262cc9f4fdd9d4cbe74977ca86e0256d0129f645559aa4e75fd30d978d8995d503932c90f000eee4d87b47cc4dd5eac8405d5cd02124346ad35b3a
7b52b9c3a3 Fix typo (yancy)
Pull request description:
Seems the comment has a stutter
ACKs for top commit:
tcharding:
ACK 7b52b9c3a3
apoelstra:
ACK 7b52b9c3a3c28afad41d9a246302ce95e4997ee5; successfully ran local tests
Tree-SHA512: 1cb11c03bb3043c19936dbe4c2ccfa1e8fa43e90c118c969da536d010d95762966cc86e257e3365d18d96cc6dfe4a63d94c4d16b1b766dde3882286ce9bb4925
e47836fb1e Do not derive Default on CompactTarget (Tobin C. Harding)
Pull request description:
It is not immediately obvious what a zero value compact target is. Because of this and the fact that we are trying to release minimal code in `primitives 1.0` its best to remove the `Default` derive.
This is an API breaking change.
ACKs for top commit:
apoelstra:
ACK e47836fb1e0c96936096efb2dfca4e2cbc9b9e5f; successfully ran local tests
Tree-SHA512: 7edb067a7ed66af8155b86b2391ee839b0a21b3d8d36b555123b14e93f1822b89c080328464b194e4ff47643440c56fa5afee929fe58b52d5f1cf84c9b96ca83
Motivated by moving the `p2p` module to its own crate. `TryFrom` and
`From` are already implement for converting to and from
`Network`/`Magic`. The methods related to `Magic` are removed from
`Network`, as well as any reference to `p2p` in the documentation, as
`bitcoin` would no longer depend on `p2p`.
The deser roundtrip test are relocated to `p2p/mod.rs`
2a3e606d89 units: Move some things out of impl_u32_macro (Tobin C. Harding)
Pull request description:
Recently we added a private `impl_u32_macro`. It included a bunch of associated consts and a pair of u32 constructor/getter functions.
We overlooked the fact that the macro produces incorrect docs.
Move the offending code out of the macro and into the already existent impl block for each type.
Docs only, no other logic change.
ACKs for top commit:
apoelstra:
ACK 2a3e606d89d25ce54ff91b38b0aa29f7f733b9d9; successfully ran local tests
Tree-SHA512: ad4bdbb35bc674e9664e293841e14dc2374c8baddf3e795edb666c75860f02728e939ef5a93ede6f4c951e92c5dd5368d6a6b9662cf6d5b268f73ab5ac97e2cc
6335c623f6 fix(p2p): remove `AddrV2` <> `SocketAddr` conversions (Luis Schwab)
Pull request description:
This PR reverts #4521 due to loss/creation of port information during the conversion. Users should use the `AddrV2` <> `IpAddr` conversion instead.
Closes#4566
ACKs for top commit:
Kixunil:
ACK 6335c623f6
tcharding:
ACK 6335c623f6
Tree-SHA512: f183756467ca17bb7e7078023a769ec33d3deb4146ab7ec63ef961107ab93ec975c9adffd9eeddc79250abfa1cee9eccbbcaef7466cf4c2c21205b69d8e9eb4f
merge-script
Fmt Bot
Jamil Lambert, PhD
David Klank
findmyhappy
Tobin C. Harding
Fallengirl
rustaceanrob
yancy