ab8017d6b3 Automated update to Github CI to cargo-semver-checks version-0.38.0 (Update cargo-semver-checks Bot)
Pull request description:
Automated update to Github CI workflow `semver-checks.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK ab8017d6b3
Tree-SHA512: 568d90367d6251f2f4ad6c5d43b304290db1d903fb43ed98cd4fb1daeb297080f9bf587f32c9d3aa6b5df2ec27b0296935ac2386524eeee78e36e2cc1e976899
945fcd0920 fix ParsePublicKeyError using hex::InvalidCharError (Innocent Onyemaenu)
Pull request description:
Replaced the InvalidChar variant u8 with hex::InvalidCharError
Resolves#3835
changed InvalidChar variant of the ParsePublicKeyError from `u8` to `hex::InvalidCharError`
```
pub enum ParsePublicKeyError {
...
/// Hex decoding error.
InvalidChar(hex::InvalidCharError),
...
}
Also,
modified the test cases to accommodate the new variant
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
ACKs for top commit:
apoelstra:
ACK 945fcd09209120ef8869a2e4165e866328cc9bd5; successfully ran local tests; I like it
clarkmoody:
utACK 945fcd0920
Tree-SHA512: c13446c099cb02b4f253f9cc559a860aff3288a2cc5eac96d3cf910bf63e78957741bbdff69b936b16b36e46b366841a5c94876d16cbc0c41aea2a70866a6e45
85e04315d5 Remove test_ prefix from unit tests (Tobin C. Harding)
Pull request description:
There is a loose convention in Rust to not use `test_` prefix. The reason being that `cargo test` outputs 'test <test name>' using the prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some places and not others is confusing to new contributors and is leading me to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
ACKs for top commit:
shinghim:
ACK 85e04315d5
apoelstra:
ACK 85e04315d5eb90075ce55bf18fab8876a4583def; successfully ran local tests
Tree-SHA512: d90ae5ef75cc5e5a8f43f60819544f1a447f13cbe660ba71e84b8f27bfcc04a11d3afde0ed56e4eea5c73ebc3925024b800a1b995f73142cab892f97a414f14a
0d8e9ef096 Remove usage of impl_from_infallible in leaf crates (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK 0d8e9ef096fd60fcdb7928697c8f554bf428b754; successfully ran local tests; this is a great change
Tree-SHA512: c4cff4517f7846d91142f26d451c2bcafc014a0921d11ac1487eab087449f12023c3b4fc57e1bc72ed3ea58406b4c3d24bbd846df21353f5d7ecb4a4b8bfb0b2
fd8d563b87 Remove macro debug_from_display (Tobin C. Harding)
Pull request description:
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 `debug_from_display` macro and just write the code.
This is an API breaking change to `internals` but an internal change only to any of the _real_ crates.
ACKs for top commit:
apoelstra:
ACK fd8d563b873c87a996d82062285169e16e3f0c13; successfully ran local tests; this is a great change
Tree-SHA512: 26faa6645d010c1b5873d584c36d0fc52b73553d88be3226937431210ccc2479548757593b8a151936e9fa280cb3c604b241511f24ef0aa5cbf3f424d7ecf215
79791e7444 primitives: Add core re-exports (Tobin C. Harding)
Pull request description:
This is not strictly needed but we would like to copy the public macros from `units` to primitives and they use `$crate::_export` paths.
ACKs for top commit:
apoelstra:
ACK 79791e7444326d284bdf42c40926f6b05a6b65f7; successfully ran local tests; sure, lgtm
Tree-SHA512: cdf683041dca07bc53ae8e70806cfb74ef79e94b8699112774e3bfb3efd29cc270bd24ab03a448197edd1d143dec07d18d9a3a92d74097d87478d40954392ee4
b1399d193f units: Improve rustdocs on macros (Tobin C. Harding)
706c7c9f5f Hide impl_tryfrom_str (Tobin C. Harding)
2675cd1040 units: Re-order impl_parse_str (Tobin C. Harding)
4cc7aae6b9 Hide impl_tryfrom_str_from_int_infallible (Tobin C. Harding)
fc9e40ab1a units: Re-order impl_parse_str_from_int_infallible (Tobin C. Harding)
Pull request description:
Document the public macros in `units::parse`. Done as part of #3632
First 2 patches are trivial preparatory refactor.
ACKs for top commit:
apoelstra:
ACK b1399d193fcb20c09457a1f22c5c1dd8009c84b1; successfully ran local tests
Tree-SHA512: e2b0196adb37b3616963e3e3ded1c2be95f98fe33a4e6edb269b7eca1ddb66b82be139f4edb3269a5cf69a73b3c803845fe83a5f6e300b08abf9fcb0da602084
f5eb8f4747 api: Run just check-api (Tobin C. Harding)
472b1d3ff3 units: Add serde regression test (Tobin C. Harding)
dedae8acf2 Implement custom serde modules for FeeRate (Tobin C. Harding)
d94e5f03e6 Move fee_rate.rs to module (Tobin C. Harding)
c3c1f6f82d Add missing license comment to test file (Tobin C. Harding)
Pull request description:
Implement and enforce explicit unit when serializing. This is as we do for `Amount` (see #3672 for similar).
To test it, and as part of the 1.0 effort; add regression tests for `serde` stuff in `units`.
With this applied one must use attributes to serialize `FeeRate`.
```rust
#[derive(Serialize, Deserialize)]
pub struct Foo {
#[serde(with = "bitcoin_units::fee_rate::serde::as_sat_per_kwu")]
pub fee_rate: FeeRate,
}
```
ACKs for top commit:
apoelstra:
ACK f5eb8f4747a7cd303cad2b7f8f442bb31862c52a; successfully ran local tests; great idea!
Tree-SHA512: 0968ead568b1e3142efd4c0e856192ddde0f441de84215cbb0950b60a56922f1abaf6d4ccfe243b722a6883c0a927d26bcfba979acf3ca84c4f21baba73af764
What:
- Replaced the InvalidChar variant u8 with hex::InvalidCharError
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
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.
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 `debug_from_display`
macro and just write the code.
This is an API breaking change to `internals` but an internal change
only to any of the _real_ crates.
1a8f5b19fb Update to rust-ordered 0.4.0 (Tobin C. Harding)
Pull request description:
We just released a version of `ordered` that makes `ArbitraryOrd` object safe - use it.
Upgrade to the latest version of `rust-ordered` - `v0.4.0`.
ACKs for top commit:
apoelstra:
ACK 1a8f5b19fbc37c74bbdd7dcee55b3294046ef88e; successfully ran local tests
Tree-SHA512: 823962a9e6b956126c7ff7a63b780431e358a9f0713f30e3801de3febea1e1e569b8ba3f6e3c0dc3c38759ac233c3d0ddea2223588cd796db6849fae8b599e04
72e97c637f Add a hash value to Inventory's Error variant (Nick Johnson)
Pull request description:
I am working on adding BIP324 V2 p2p network message support to the p2p module and ran into a little snag. I can post a draft branch of that work if helpful, but the general strategy is to add a `V2NetworkMessage` which operates in parallel with the existing `RawNetworkMessage` type (which is a bit of misnomer and may be better described in the future as `V1NetworkMessage`, see #3157). This allows both p2p message types to hook into the existing `Encodable`/`Decodeable` chain.
But the `Error` variant of the `Inventory` type message does not have symmetrical encoding and decoding paths. Encoding writes out a zero'd 32 bytes while decoding ignores it completely. And while it is not clear to me if there is [requirement written down](https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors) anywhere, [bitcoin core does appear to always expect a hash](c1252b14d7/src/protocol.h (L499)) even with the `Error` variant where it is meaningless.
I believe rust-bitcoin's handling of this was never a problem before because the top level `RawNetworkPackage` pulls all the required bytes off a reader before decoding them. But this is not as easy to do with the v2 p2p network messages since the length is decoded at the transport level, not the message itself. I believe it is preferable for the Decoders to *not* assume that all bytes have been pulled off already given their input stream interface. Maybe somewhat surprisingly, this is the only issue I have run into so far adding the v2 encoding and decoding paths. As it is now, the code panics with an `Unconsumed` because it hasn't pulled the dummy zero bytes off the reader.
This patch adds the 32 byte array to the Error variant in order to make its Encoding and Decoding paths symmetrical. This also allows a reader to discard the 32 bytes when decoding a message while cleanly keeping things hooked up with the `Decodeable` chain. The hash is still not exposed to the caller.
This is *a way* to solve my issue, but not sure if it will cause more confusion than its worth. I tried a few other strategies, but preferred this one due to how well it hooks into `Decodeable`.
ACKs for top commit:
apoelstra:
ACK 72e97c637fa0916be75aef28ea8169ffbbe2c4f5; successfully ran local tests
tcharding:
ACK 72e97c637f
Tree-SHA512: 20cb9fec0768e0fdf7c7f520a00e1a37f5ef0583e2ebc7d47583249c6829c47826d83694a56338efa5844a9fe264a77f6d04c0c46c85f8732c7585515723d7ce
We have a public macro `impl_parse_str` and a helper macro used by it,
both pubicly exported.
As we did for `impl_parse_str_from_int_infallible`; to assist readers
understanding what is going on put the helper below the other.
Done as a separate patch to make the diff easier to read.
Internal change only, no logic change.
This macro is a helper for `impl_parse_str_from_int_infallible` used to
reduce code duplication. It does not, and should not, need to be part of
the public API - hide it.
We have a public macro `impl_parse_str_from_int_infallible` and a helper
macro used by it, both pubicly exported.
To assist readers understanding what is going on put the helper below
the other.
Code move only, no logic change.
7f2cf1dce8 api: Include generics in regex (Tobin C. Harding)
Pull request description:
The regex is still wrong, it misses structs with generics.
Currently `contrib/api.sh io types` returns:
ErrorKind
Error
Sink
But with this patch applied it returns:
FromStd<T>
ToStd<T>
ErrorKind
Cursor<T>
Error
Sink
Take<'a, R: bitcoin_io::Read + ?core::marker::Sized>
Thanks ChatGPT, good robot.
ACKs for top commit:
storopoli:
ACK 7f2cf1dce8
apoelstra:
ACK 7f2cf1dce8236c60931f729748c487d401878067; successfully ran local tests; nice!
Tree-SHA512: 92fde9788edf8bf3e9b0639aec0aa0c4d7509c46cb048505e8018af57d671e893dccb7e0e018246438cceeb9c64232b8a0639bfe1ee7a35983b9e2bf1f2d4a4a
20b798175c Update to latest rust-ordered (Tobin C. Harding)
Pull request description:
I just went to town on the `rust-ordered` crate to get it ready for releasing a `1.0` version. None of the changes effect our usage here in `rust-bitcoin`.
Upgrade to the latest version of `rust-ordered` - `v0.3.0`.
ACKs for top commit:
apoelstra:
ACK 20b798175c23e40988f821c9a90e28e216df2937; successfully ran local tests
Tree-SHA512: b21608ad1992df3a0c5c93a454c08c8e3fab4d9e10c2f05ee2d929e585134ad26045f707f77083e6b6c3952dd04e611db7a35389ffe8fd487acad5344e9d36f3
ae129d84ac api: Run just check-api (Tobin C. Harding)
3f433f1cde Improve Cursor ref API (Tobin C. Harding)
Pull request description:
Our `Cursor` is a replacement of sorts for the `std::io::Cursor`. Users of the `Cursor` are likely to expect the same API so to get a reference and/or mutable reference users will likely reach for `get_ref` and `get_mut`. We should provide these.
Deprecate `inner` since it is the same as `get_ref` but less idiomatically named (should have been `as_inner`).
Add `get_ref` and `get_mut` methods to the `Cursor` type.
ACKs for top commit:
apoelstra:
ACK ae129d84ace03fba57bb980cace5aa60ff6bc10e; successfully ran local tests
Tree-SHA512: 664d08cc977c9d6e3319767c49bbabc8f21d681d570f50c514797a7737469cf861af60158ccc1c71d28baed542dabbbca586968144d60691ee0183a549a7b765
2042d91be5 api: Run just check-api (Tobin C. Harding)
04bae4bb91 Use _unchecked in unit tests (Tobin C. Harding)
8e6784dd41 Introduce Amount::from_sat_unchecked (Tobin C. Harding)
f32af7dac6 Use ua_sat throughout test function (Tobin C. Harding)
75f0afd0a3 Add local variable to use as constructor (Tobin C. Harding)
940a244132 Replace Amount::from_sat(0) with ZERO (Tobin C. Harding)
Pull request description:
This is a small-ish changeset in order to mirror what was started in #3769 and reduce the diff in #3794
- Patch 1: Trivial refactor to use `Amount::ZERO`.
- Patch 2: Add the constructor and refactor a single unit test to use it.
- Patch 3: Use the two `_unchecked` constructors throughout `units/src/amount/tests.rs`.
- Patch 4: Add the API text file changes.
ACKs for top commit:
jamillambert:
ACK 2042d91be5
apoelstra:
ACK 2042d91be59a1d47afb986cef0152f9b5250f476; successfully ran local tests
Tree-SHA512: 8f9c3816509bb3fc0707cd4a47426130d001358e82273457b24170522644e10fe36b596e16500ce1778738f5546dfad592f1f3c4de552ec0afcb146442176cf5
I just went to town on the `rust-ordered` crate to get it ready for
releasing a `1.0` version. None of the changes effect our usage here in
`rust-bitcoin`.
Upgrade to the latest version of `rust-ordered` - `v0.3.0`.
87293dfdd3 api: Run just check-api (Tobin C. Harding)
ed387e5f1d hashes: Hide both macros (Tobin C. Harding)
Pull request description:
We have two macro definitions feature gated on `serde`. At some stage we added the `doc(hidden)` attribute to one of them but forgot to add it to the other. This technically makes our features non-additive. This macro is "internal" so its unlikely that this is being used in the wild.
Add `doc(hidden)` to the `serde_impl` macro that is missing it.
Found by `cargo semver-checks` after recent upgrade to 0.38
ACKs for top commit:
storopoli:
ACK 87293dfdd3
apoelstra:
ACK 87293dfdd38be71938ae9d6a9981eb4569de3521; successfully ran local tests
Tree-SHA512: 3773eb42684bce56928f2d3f87993d010dd36634fda8dfa6b18b27a36b6c4bb521d711340a4868270dbd2d0c6a11715f9a1998ff3deb6900bd4227a44722a11e
We now have both `Amount::from_sat_unchecked` and
`SignedAmount::from_sat_unchecked`. These constructors are explicitly
for ignoring any invariant (implied or otherwise) especially in test
code.
Note we do not enforce an invariant currently. This patch is a baby step
towards getting the `amount` module in order.
Replace all calls to `from_sat` for const int values with the
`_unchecked` constructor. Done in `amount::tests` only.
As we did for `SignedAmount` add a constructor to the `Amount` type that
does no checks on its argument.
(This is in preparation for enforcing the MAX_MONEY invariant.)
Use the `_unchecked` version in a single unit test. The rest of the unit
tests will be refactored later to minimise the size of this patch.
As we already do in this test function for other constructors; add a
local variable and bind it to the `SignedAmount::from_sat` constructor.
Refactor only, no logic change.
2e482f0fdd Remove unnecessary floating code comment (Tobin C. Harding)
Pull request description:
Code comments that comment and arbitrary block "section" of code are almost always pointless and almost always go stale over time.
These particular code comments add almost no value.
Remove code comments.
ACKs for top commit:
jamillambert:
ACK 2e482f0fdd
apoelstra:
ACK 2e482f0fddb55da897f0ba8ea4d3fa5bb0fba1b5; successfully ran local tests; yeah, in this case I agree
Tree-SHA512: 9cd5891e4d91af5206d99b5a2021bc82cc33e3c11d66364442a1a16866d2329ed3a005865cec1a76db80eb3191495a1710a683bc5a69284a29f164a1285b42ea
e13355318e Add From impl (yancy)
364e9ff775 Change method return type (yancy)
fdf3336ed5 Add unchecked variant (yancy)
Pull request description:
Any SignedAmount can now be cast to Amount since the range is the same. Specifically, the range for SignedAmount is (- 21 million, 21 million) while the range for Amount is (0, 21 million). Therefore any value from Amount can be cast to a SignedAmount and it will work. Note it's not the same and still requires checking when going from SignedAmount to Amount since Amount can't handle the negative range.
ACKs for top commit:
tcharding:
ACK e13355318e
Tree-SHA512: c016b51bdd87a12eb09d9c1a82699dad1e866bf96bd3235eeb131f216f02422acb992ddb3a8135af00bbc10e240178fde5e37fb7860d9e6eaf433cf917d4d16a
Our `Cursor` is a replacement of sorts for the `std::io::Cursor`. Users
of the `Cursor` are likely to expect the same API so to get a reference
and/or mutable reference users will likely reach for `get_ref` and
`get_mut`. We should provide these.
Deprecate `inner` since it is the same as `get_ref` but less
idiomatically named (should have been `as_inner`).
Add `get_ref` and `get_mut` methods to the `Cursor` type.
The regex is still wrong, it misses structs with generics.
Currently `contrib/api.sh io types` returns:
ErrorKind
Error
Sink
But with this patch applied it returns:
FromStd<T>
ToStd<T>
ErrorKind
Cursor<T>
Error
Sink
Take<'a, R: bitcoin_io::Read + ?core::marker::Sized>
We have two macro definitions feature gated on `serde`. At some stage we
added the `doc(hidden)` attribute to one of them but forgot to add it to
the other. This technically makes our features non-additive. This macro
is "internal" so its unlikely that this is being used in the wild.
Add `doc(hidden)` to the `serde_impl` macro that is missing it.
Found by `cargo semver-checks` after recent upgrade to 0.38
de0f940b90 Automated update to Github CI to rustc nightly-2024-12-26 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK de0f940b90
Tree-SHA512: 83ba49d2df44853ac2179edf432fb05e77b2e2d98d515eb86743f96bbf637b4ea1b75e5975991049635e5372847cafdb96a2503beb5a8915c4d5af3335f86dce
Add an integration test the verifies we have serde traits implemented
for all the main crate types (excl. errors and helper structs).
This also acts as a regression test.
The `FeeRate` type wraps a `u64` but the inner value implicitly contains
information about the unit. As such when serializing and deserializing
the unit information is not explicit and if users try to deserialize
with a different unit their code will be silently buggy.
As we do for Amount; add custom serde modules so that users can
serialize in an explicit unit. Furthermore remove the derived impls
forcing users to make the decision. This is as we do for `Amount`.
With this applied one can write
```rust
#[derive(Serialize, Deserialize)]
pub struct Foo {
#[serde(with = "bitcoin_units::fee_rate::serde::as_sat_per_kwu")]
pub fee_rate: FeeRate,
}
```
In preparation for adding `serde` modules to the `fee_rate` module
create a sub directory.
Create a directory and move `fee_rate.rs` to `fee_rate/mod.rs`.
Code comments that comment and arbitrary block "section" of code are
almost always pointless and almost always go stale over time.
These particular code comments add almost no value.
Remove code comments.
merge-script
merge-script
Update cargo-semver-checks Bot
Innocent Onyemaenu