Instead of accessing the inner type of a hash wrapper type when
consensus encoding we can call `as_byte_array()`.
Done in preparation for moving `Txid` and `Wtxid` to `primitives`.
Internal change only.
d65de7c7de Introduce and use new compact_size module (Tobin C. Harding)
Pull request description:
We would like to move the witness module to `primitives` but there is a bunch of usage of `VarInt`.
Introduce a module that does the encoding and decoding instead, note that while the functionality is internal decoding returns an error which may one day end up in the public API. So put the module in `primitives` and make it public.
Adds the module to `primitives`, adds a public `MAX_ENCODABLE_SIZE` variable that is commented with an issue link.
https://github.com/rust-bitcoin/rust-bitcoin/issues/3264
ACKs for top commit:
apoelstra:
ACK d65de7c7de successfully ran local tests
Kixunil:
ACK d65de7c7de
Tree-SHA512: d9483c29b2b324e27460564a23f4639dde4037e6e773f4356216b02ebdea893a6361c342002b8e93a54de47b71ac69369431554f8cd0a2522fc451bf8493c81c
733505148c Add tests for witness_program (Shing Him Ng)
Pull request description:
Add tests for witness_program
ACKs for top commit:
tcharding:
ACK 733505148c
Kixunil:
ACK 733505148c
apoelstra:
ACK 733505148c successfully ran local tests
Tree-SHA512: c1ee82edf22c7b39bc110d03836ba5ebfa785a63185a75c1a61781180ff907c1ea0c491c963629450f360152766845dacedccc6cf56bc3d8c607e66281427dd3
ae93e226e3 Remove hashes io feature (Tobin C. Harding)
Pull request description:
Currently we only get `std::io::Write` impls when the `bitcoin-io` dependency is used. This is overly restrictive, it would be nice to have `std::io::Write` imlps even without the `bitcoin-io` dependency.
Copy the logic out of the `bitcoin_io::impl_write` macro into `hashes` but feature gate it differently.
Call the new macro inside `hash_type` (and in `hmac`), remove the `impls` module, and move the tests to the integration test directory.
Remove the `io` feature from `hashes`, now if users enable `std` they get `std::io::Write` impls and if they enable `bitcoin-io` they get `bitcoin_io::Write` impls as well.
ACKs for top commit:
Kixunil:
ACK ae93e226e3
apoelstra:
ACK ae93e226e3 successfully ran local tests
Tree-SHA512: d47c9c060750e8a024c46cbf7afe8d0d1245fa1f5e575f36b3a11e2460d3620ad9def1a6331dafe77d46affc99b043ec9679e619ce8ddfa32436a5826ece09e4
fe46225ed0 Allow unused imports when running bench code (Tobin C. Harding)
eb67e873e0 Allow unused variables in release mode (Tobin C. Harding)
Pull request description:
Two patches to clear the million warnings when running the bench code.
ACKs for top commit:
apoelstra:
ACK fe46225ed0 successfully ran local tests; though in the first commit you could also use `cfg_attr` FWIW
Kixunil:
ACK fe46225ed0
Tree-SHA512: 3f705e0441d8c0e41e9ceb5473572810ff2513f7e5531c1b7889418a3a85ac8622e50e271c7a3b5c386fb3f5629b85d4bd79739c4a02b51d58da86890721d8d2
0f897f80a5 Re-write (and re-name) read_uint_iter (Tobin C. Harding)
Pull request description:
The `UintError` type (returned by `read_uint_iter`) is not that useful because one variant is unreachable. Re-write the function by doing:n
- Re-write the function to reduce the error cases returned.
- Re-name it to `read_push_data_len`
- Move it to `internals`
- Use `PushDataLenLen` enum instead of an int parameter
ACKs for top commit:
apoelstra:
ACK 0f897f80a5 successfully ran local tests; lol so much better than the old code
Kixunil:
ACK 0f897f80a5
Tree-SHA512: 095017a32c2d5bb2268cb1a059d0022e122faf8b41295f14970e7968374dd1c35c3b95357aba5aabaa17843439aebc237000009015ea9b8bc58ab1b337e8e1bc
ea2efc155e Add coinbase associated consts (Tobin C. Harding)
Pull request description:
Currently we have `all_zeros` functions and `null` functions but we can do better.
Add associated consts and improve the names to better describe what these dummy zero hashes are used for.
Deprecate related functions.
ACKs for top commit:
Kixunil:
ACK ea2efc155e
apoelstra:
ACK ea2efc155e successfully ran local tests
Tree-SHA512: bc7e840622a558bc46798e3606452ad24c16b7d23e7fe7a68fdf8a719326eb9d6d872ec1647620506f1de76b8086ae36cce0e1399e55e50bbd794efb8b4dda47
Currently we only get `std::io::Write` impls when the `bitcoin-io`
dependency is used. This is overly restrictive, it would be nice to have
`std::io::Write` imlps even without the `bitcoin-io` dependency.
Copy the logic out of the `bitcoin_io::impl_write` macro into `hashes`
but feature gate it differently.
Call the new macro inside `hash_type` (and in `hmac`), remove the
`impls` module, and move the tests to the integration test directory.
Remove the `io` feature from `hashes`, now if users enable `std` they
get `std::io::Write` impls and if they enable `bitcoin-io` they get
`bitcoin_io::Write` impls as well.
The `UintError` type (returned by `read_uint_iter`) is not that useful
because one variant is unreachable. Re-write the function by doing:n
- Re-write the function to reduce the error cases returned.
- Re-name it to `read_push_data_len`
- Move it to `internals`
- Use `PushDataLenLen` enum instead of an int parameter
c00afe8d52 Change MessageSignatureError to secp256k1::Error (Jamil Lambert, PhD)
a20d0bc4eb Deprecate `from_slice()` in sha256.rs (Jamil Lambert, PhD)
089043546f Deprecate `from_slice` methods in favor of arrays (Jamil Lambert, PhD)
Pull request description:
As brought up in issue #3102 support for Rust arrays is now much better so slice-accepting methods that require a fixed length can be replaced with a method that accepts an array.
`from_slice()` methods that require a fixed length have been deprecated and where needed a `from_byte_array()` method created that accepts an array.
There are still `from_slice` methods that rely on changes to external crates before they can be changed to arrays.
ACKs for top commit:
apoelstra:
ACK c00afe8d52 successfully ran local tests
tcharding:
ACK c00afe8d52
Kixunil:
ACK c00afe8d52
Tree-SHA512: c505b78d5ca57e7e1004df6761ac6760d5d9b63c93edc6ac1f9adf047bc67011883575f835b06f6d35d7f6c2b6a4c6c7f0a82a3f0e293bfb4ef58123b75d3809
333c8ab297 Add additional docs to Witness (Tobin C. Harding)
Pull request description:
The `Witness` struct is non-trivial, in particular it is not immediately obvious where and when the compact size encode value for each witness element is stored.
Make an effort to improve the docs on `Witness` in relation to the compact size encoded length of each witness element.
ACKs for top commit:
apoelstra:
ACK 333c8ab297 successfully ran local tests
Kixunil:
ACK 333c8ab297
Tree-SHA512: 1c61a9ad071c035d5ad2e54446120d29ebf8cc4a779c96f04eda825890687dcbd53accc17522f57ef4ffb226eb1d85c6a3a115f27bebcfc7ad3c677033a8a414
`from_byte_array` cannot error due to InvalidLength so the returned
MessageSignatureError has been changed to return a secp256k1::Error,
which is the only error type returned by the function.
Support for Rust arrays is now much better so slice-accepting methods
that require a fixed length can be replaced with a method that accepts
an array.
`from_slice()` has been deprecated and replaced with `from_byte_array()`
Currently we have `all_zeros` functions and `null` functions but we can
do better.
Add associated consts and improve the names to better describe what
these dummy zero hashes are used for.
Deprecate related functions.
In release mode we have a few unused variable warnings, lets just
allow them.
Found when running bench code, interestingly `cargo bench` must build
in release mode.
We would like to move the witness module to `primitives` but there is
a bunch of usage of `VarInt`.
Introduce a module that does the encoding and decoding instead, this
code is internal so put it in `internals`.
Note we add an unused public `MAX_ENCODABLE_SIZE` variable that is
commented with an issue link. Done like this because its quite
important that we see to it and it makes it clear that we are not and
we know about it.
https://github.com/rust-bitcoin/rust-bitcoin/issues/3264
The `Witness` struct is non-trivial, in particular it is not immediately
obvious where and when the compact size encode value for each witness
element is stored.
Make an effort to improve the docs on `Witness` in relation to the
compact size encoded length of each witness element.
c48d9d6523 Move transaction::Version to primitives (Tobin C. Harding)
f490222068 Introduce the VersionExt trait (Tobin C. Harding)
fb89974b82 Run the formatter (Tobin C. Harding)
bb3a3ecbaa Introduce temporary module for Version (Tobin C. Harding)
1fde868f51 Separate Version impl blocks (Tobin C. Harding)
Pull request description:
As per title, in tiny small chunks, move the `transaction::Version` over to `primitives`. Only the type, its associated consts, and its `Display` impl are moved. The two methods are left in an extension trait.
Was originally attempted in #3253
ACKs for top commit:
Kixunil:
ACK c48d9d6523
apoelstra:
ACK c48d9d6523 successfully ran local tests
Tree-SHA512: 83415cf0762dca5c263deb743734fc7abede804a6daac31df3d0101b51c6261e6d54452eb744727ae680cacce9e4ef726a6fa253d86c4e7a5d8ec789b137566c
We would like to move the `Transaction` type to `primitives`, as a step
towards this move the `transaction::Version` and its trait imps (just
`Display`) over there.
In preparation for adding an extension trait; separate the
`transaction::Version` impl blocks into stuff that will stay here and
stuff that will go to `primitives`.
Refactor only, no logic changes.
30bb93c676 Implement impl_to_hex_from_lower_hex macro for types that implement fmt::LowerHex (Shing Him Ng)
Pull request description:
Created a macro that implements `to_hex` for types that currently have `core::fmt::LowerHex` and called it on types that have `core::fmt::LowerHex` implemented. I put the macro in the `internals` crate since there are types across the whole project that can potentially use this.
Resolves#2869
ACKs for top commit:
Kixunil:
ACK 30bb93c676
apoelstra:
ACK 30bb93c676 successfully ran local tests
Tree-SHA512: d3ebc7b5c0c23f1a8f8eef4379c1b475e8c23845e18ce514cb1e98eb63fc4f215e6bc4425f97c7303053df13374ef931ae9d9373badd7ca1975a55b0d00d0e40
b6371b5801 Fix clippy rustdocs warnings (Tobin C. Harding)
Pull request description:
A new nightly version (`nightly-2024-08-28`) introduces a few warnings because of our rustdocs. These are valid warnings and should be fixed, thanks `clippy` team.
(The `bip152` change is a bit sloppy, open to suggestions.)
ACKs for top commit:
apoelstra:
ACK b6371b5801 successfully ran local tests
Kixunil:
ACK b6371b5801
Tree-SHA512: 503fb9d48772b74a5acdb26c0f77a85c52323c03360f983204fccee0f28bedeff142237b067caa1ce6ea04ea9842cc493e0d06dc141ca00a98151fa002b62392
345d3daa72 fix: deprecate wrong and unused max script num (ChrisCho-H)
Pull request description:
~~Script number can be up to 2^39 - 1 to encode locktime.~~
~~If it's only for the integer operation besides locktime, it must be 2^31 - 1, not 2^31.~~
I agree with apoelstra opinion to deprecate this value.
ACKs for top commit:
apoelstra:
ACK 345d3daa72 successfully ran local tests
Kixunil:
ACK 345d3daa72
Tree-SHA512: df4205415634d8db1412f16a75d1dbc110ed69930cefa085bdaa268a7fcaf47776dd1d8ed6965e6bbb476825b5f80e5c00bb375730ad102bae8868abe5894068
fa71b0e044 2024-09-01 automated rustfmt nightly (Fmt Bot)
Pull request description:
Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
apoelstra:
ACK fa71b0e044 successfully ran local tests; a bit big this week
Tree-SHA512: 875e8d54a06a22ccbef690a21f9d48a0acc1e6ea073a2d16632dd7ce63400cc7f2522339b7cb808653dade407517cd03d4fece4c8dec24abacf5303418aeaa75
9db6234ea9 Show compressed public key in Debug for CompressedPublicKey (Jiri Jakes)
Pull request description:
Currently `CompressedPublicKey` debug produces output of form:
```
CompressedPublicKey(PublicKey(2f8b18dc0adcb73d75f7934d9523ea7347083e41c48115398cb37e295a0a6ffe86e0bf8b1ef65888c880c8d8813a30e69e466380cbe2daec18f3ed1e7a553ff2))
```
Although it shows real internal structure together with inner uncompressed public key, it is not too helpful for the purpose of debugging _compressed_ public key.
After this patch, `Debug` output will be equal to `Display` (it, in fact, delegates rendering to `Display`), prepended by the name of the struct:
```
CompressedPublicKey(02fe6f0a5a297eb38c391581c4413e084773ea23954d93f7753db7dc0adc188b2f)
```
ACKs for top commit:
Kixunil:
ACK 9db6234ea9 unless other maintainers agree to drop the struct name.
apoelstra:
ACK 9db6234ea9 successfully ran local tests; we should keep the struct name since it is canonical for Debug output
Tree-SHA512: e2626678a4144357d3ff4ddd888459ae1283ea50bc7c8ef86deb9902d7d543de79109ac42883e5538f7e2f6a29426224bb43eb44a34f366821e47a9aca563753
3c7c8c44b6 Improve const_assert (Tobin C. Harding)
Pull request description:
Now that we can panic in const context we can improve the `const_assert` macro by adding a message string.
Original idea by Kix:
https://github.com/rust-bitcoin/rust-bitcoin/pull/2972#discussion_r1726328228
ACKs for top commit:
Kixunil:
ACK 3c7c8c44b6 in the sense that it does what it's supposed to with the only tiny issue being that the `bool` looks weird but not broken in any other way I can think of.
apoelstra:
ACK 3c7c8c44b6 successfully ran local tests
Tree-SHA512: 5ff721c0056f87d42c934818da6f780cd945f235291eb4b044752d67405a74f992d7f85853fec129e794ec3fcda1f319cc40daabc6a349d21bbdc977640d2572
a184066660 Move import inside feature gate (Tobin C. Harding)
Pull request description:
The `String` type is only used if the `serde` feature is enabled, move the import statement inside the already feature gated block.
ACKs for top commit:
apoelstra:
ACK a184066660 successfully ran local tests; sure
Kixunil:
ACK a184066660
Tree-SHA512: 688adb1e4489b0242856ac36ed9cf3503b147b84954cf1e6fd34b5e708ed6c1dd92da91e739243face1c1b4e4c8b19ce88e215117bef2a7af2e732859a6e108a
dae42bef9d do not enable bitcoin-io by default (Antoni Spaanderman)
a14cdaf859 don't enable std by default when testing (Antoni Spaanderman)
e83830dcfc use slice instead of array to not have to hardcode the length (Antoni Spaanderman)
55749d6f61 use `hash.to_byte_array` to check equality with `test.output` (Antoni Spaanderman)
969864e3b0 use fixed size array if possible, otherwise `&'static [u8]` (Antoni Spaanderman)
28ccf70fa6 remove unnecesarry borrow operator (`&`) (Antoni Spaanderman)
fa3a3afd02 remove unnecessary slicing (Antoni Spaanderman)
22e42ab86c fix test code being unnecessarily feature gated (Antoni Spaanderman)
Pull request description:
- remove 2 unnecessary cfg attributes from tests left over from #3167 (it made them not dependent on `alloc` anymore)
- simplify assertion logic by removing unnecessary conversions before comparing
- make tests `no_std` compatible by adding imports to alloc or std
- feature gate tests behind the `alloc` feature if they use anything from the alloc crate (like the `format!` macro)
- `schemars` feature enables `alloc` because (for example) its trait wants implementations to return `String`
- fix `bitcoin-io` always enabling when `std` is enabled (only useful if people depend on `hashes` only, `bitcoin` depends on `bitcoin-io` already)
ACKs for top commit:
tcharding:
ACK dae42bef9d
Kixunil:
ACK dae42bef9d
apoelstra:
ACK dae42bef9d successfully ran local tests
Tree-SHA512: 622fd4963ef21530a98af89bcfc71abe8723aac12d363ab88d9bd30dcf2f75392711bec10e2901fab5f1a30e11897d1aae36e22892738aa1e5670166f91fddd4
A new nightly version (`nightly-2024-08-28`) introduces a few warnings
because of our rustdocs. These are valid warnings and should be fixed,
thanks `clippy` team.
(The `bip152` change is a bit sloppy, open to suggestions.)
cf129ad314 fix: re-implement (de)serialization from/to readers/writers (elsirion)
Pull request description:
Fixes#3250.
The serialization is less than ideal and still allocates a lot. I can understand not wanting to (ab)use the consensus encoding traits, but they have a pretty good interface, copying it and creating some `EncodePsbt` and `DecodePsbt` traits with similar interfaces would have been nice imo.
ACKs for top commit:
Kixunil:
ACK cf129ad314
apoelstra:
ACK cf129ad314 successfully ran local tests; LGTM -- I believe this is non-breaking, as does cargo-semver-checks, so we can backport this to 0.32
Tree-SHA512: d7f218164d772db3a9fb4436953c3b5fd3677b92078d0843233197629df7d852d80615a3ff38c5b70771381ba1aeb30defdc98ee63653e570bb75dc553400cad
a76d76eca1 Change `T::from_str(s)` to `s.parse::<T>()` (Jamil Lambert, PhD)
Pull request description:
As mentioned in issue #3234 `s.parse::<T>()` is more idiomatic and produces more helpful error messages.
This has been changed in the main codebase, not including examples, rustdocs, and in the `test` modules.
`use std::str::FromStr;` has been removed where this change makes it unnecessary.
To close the issue it may also need to be changed in the examples and the `test` modules and `contributing.md` updated.
ACKs for top commit:
Kixunil:
ACK a76d76eca1
storopoli:
ACK a76d76eca1
tcharding:
ACK a76d76eca1
Tree-SHA512: 18be389579b5c2d0ba567dfb02b9c8d71c108a749ec5168c60a50e7af9d7f498e364c8f4c0ce1698d960e7d146486899cd10214ebc9c96708ed158a04dfff425
`s.parse` is more idiomatic and produces more helpful error messages.
This has been changed repo wide in the main codebase, not including
examples, rustdocs, and in the test module.
`use std::str::FromStr;` has been removed where this change makes
it unnecessary.
Tobin C. Harding
merge-script
Shing Him Ng
Fmt Bot
Jamil Lambert, PhD
ChrisCho-H
Jiri Jakes
elsirion