5b4e81b379 Implement Arbitrary for Transaction (Shing Him Ng)
Pull request description:
Implement `Arbitrary` for Transaction and its child types
ACKs for top commit:
apoelstra:
ACK 5b4e81b379 successfully ran local tests
tcharding:
ACK 5b4e81b379
Tree-SHA512: 411b97344e01e2bf38fff994902acda51f9a4fe748573e600fbdd643951a0a56b37395594349706b13c744304bd5396fd8ee31c27e5b1ff97e1a3cebd479e7c8
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
unused_imports in docs have been removed in bitcoin, and a warn
attribute added to lib.rs.
25d906d936 Use UFCS in macros (Liam Aharon)
Pull request description:
Closes#3304
ACKs for top commit:
apoelstra:
ACK 25d906d936 successfully ran local tests
tcharding:
ACK 25d906d936
Tree-SHA512: a02a8507bcec73dab1ec8d49e45eab327d989c276d931520e0fff312faf7d3165292c300fb9414314f9b94b1255ee49a0fb64303df6d971c9089226b6873c36a
In preparation for releasing `internals v0.4.0` bump the version number,
add a changelog entry, update the lock files, and depend on the new
version in all crates that depend on `internals`.
8ec3571d80 Implement GetKey for Vec<Xpriv> (Nadav Ivgi)
Pull request description:
It appears that the `BTreeSet<Xpriv>`/`HashSet<Xpriv>` sets currently implementing `GetKey` cannot actually be constructed, because `Xpriv` does not implement `Ord` nor `Hash`. (And that the rust-bitcoin code referencing these sets should not even compile? yet evidently it does 👀 )
This PR adds support for `Vec<Xpriv>` to enable signing with multiple `Xpriv`s, but does not address the issue with the existing sets.
The added test case demonstrates the issue:
```rust
error[E0277]: the trait bound `bip32::Xpriv: std:#️⃣:Hash` is not satisfied
--> bitcoin/src/psbt/mod.rs:2301:24
|
2301 | HashSet::new().insert(xpriv.clone());
| ^^^^^^ the trait `std:#️⃣:Hash` is not implemented for `bip32::Xpriv`
|
note: required by a bound in `std::collections::HashSet::<T, S>::insert`
--> /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/collections/hash/set.rs:888:5
error[E0277]: the trait bound `bip32::Xpriv: Ord` is not satisfied
--> bitcoin/src/psbt/mod.rs:2302:25
|
2302 | BTreeSet::new().insert(xpriv.clone());
| ^^^^^^ the trait `Ord` is not implemented for `bip32::Xpriv`
|
note: required by a bound in `std::collections::BTreeSet::<T, A>::insert`
--> /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/collections/btree/set.rs:899:5
```
ACKs for top commit:
apoelstra:
ACK 8ec3571d80 successfully ran local tests
tcharding:
ACK 8ec3571d80
Tree-SHA512: aceb95f8eaf11f91c6829e0b5e1c0264ebffbf587fd420145a22e924cb45678b2f4334f0b7de6ed99b57f0ce24c3d61f9e5c1e348e1b40975bc515e8fd16b75d
d942882b36 Document the magic bytes for witness commitment (Peter Ryszkiewicz)
Pull request description:
ACKs for top commit:
apoelstra:
ACK d942882b36 successfully ran local tests; neat! I don't think I was aware of this
tcharding:
ACK d942882b36
Tree-SHA512: 89766e986574e49b6c5fd00844db679eb8894a591463029f7f88079fa9ca65bd56265a42dd8fa5aad20ef4744bca3259c06f8a461abd1c695fb89433ed5cc145
b593c886e3 Support GetKey where the Xpriv is a direct child of the looked up KeySource (Nadav Ivgi)
055aa9d4dc Refactor GetKey to take the KeyRequest by reference (Nadav Ivgi)
d15c57bd1f Refactor GetKey for sets to internally use Xpriv::get_key() (Nadav Ivgi)
d25c62bf45 Fix GetKey for sets to properly compare the fingerprint (Nadav Ivgi)
Pull request description:
- The first commit is the simplest fix for a bug where the fingerprint wasn't compared correctly.
- The second & third commits are optional refactoring to reuse `Xpriv::get_key` for `$set<Xpriv>::get_key`, so the Xpriv matching logic only has to be maintained in one place.
- The forth commit adds support for signing with `Xpriv`s that are direct children of the `KeySource` -- possibly what the original (buggy) code author had in mind?
Of course, feel free to take just the first commit if the others seem unnecessary. The last one is kind of meh, not sure if really useful.
Note that multi-`Xpriv` signing does not actually work until #2850 is addressed too.
ACKs for top commit:
apoelstra:
ACK b593c886e3 successfully ran local tests
tcharding:
ACK b593c886e3
Tree-SHA512: a6f84fe12b68ddb71fc722649b1596d1561ab08a07d47f2345bd8932e155571f2b1e23c32a9c03d588fb697540340cb8ca38275dfe11f4910fd48265a993762a
70ccd6b629 signed_msg_hash takes impl AsRef<[u8]> (Liam Aharon)
Pull request description:
Closes#3249
Seems not required for Kixunil's immediate use case anymore, but figured I'd open a PR anyways.
ACKs for top commit:
apoelstra:
ACK 70ccd6b629 successfully ran local tests
Kixunil:
ACK 70ccd6b629
Tree-SHA512: b15bafd1af0749cbac36652a4e4c9ab3f91b3980bb6c3cbc1c977d339810fc15075af5d4bc6b210de364bedc496ce3759eaf4539e40d215ab66a122ba69dd56a
8f2f4cbb3c Re-order optional dependencies (Tobin C. Harding)
95f2a8dab6 Do not access ScriptBuf inner from builder (Tobin C. Harding)
900af453ff Stop accessing inner ScriptBuf field when encoding (Tobin C. Harding)
8b82363d97 Use Script::as_bytes instead of inner when indexing (Tobin C. Harding)
b0675a4a4f Use Script::len instead of inner field (Tobin C. Harding)
374c6118dc Deprecate Script::fmt_asm and to_asm_str (Tobin C. Harding)
Pull request description:
Move the `Script` and `ScriptBuf` types to `primitives`. There were still a few preparations required, things we had missed while creating the extension traits.
Note also please, in the last patch, we enable `hex` from the `serder` feature. This is not the final state we want but like we did for `alloc` it is a step to reduce the size of the diff.
ACKs for top commit:
Kixunil:
ACK 8f2f4cbb3c
apoelstra:
ACK 8f2f4cbb3c successfully ran local tests
Tree-SHA512: 62a5f3c253ecb54d95c37fdc7eb955f3952909dc3bca20444b85c44665f54d5a0c48daf729bed0dd60ff3e9571b41deed039984c8b757b075ac6e136cacd17d7
0403e52ce3 Move the transaction hash types over to primitives (Tobin C. Harding)
7e454d756d Define extension traits for txid types (Tobin C. Harding)
832b726d03 Stop using all_zeros (Tobin C. Harding)
d69c241b5c Improve docs on associated consts (Tobin C. Harding)
68c9e28165 Do not use private constructor for txid hash types (Tobin C. Harding)
98328b5a7b Use as_byte_array to encode hash type (Tobin C. Harding)
Pull request description:
Move the `Txid` and `Wtxid` hash wrapper types over to `primitives`. This introduces to `primitves` an unconditional dependency on `hashes`.
ACKs for top commit:
Kixunil:
ACK 0403e52ce3
apoelstra:
ACK 0403e52ce3 successfully ran local tests
Tree-SHA512: d14fa95bc12c2399d30d4d640b5a3fce625d51adf587a8037158f7d7e7b6288170b2d4418ca2cb68f612086ea0bdd0fae3b577f84f0d60627072fdb2217a6531
e064686397 Deprecate OutPoint::new constructor (Tobin C. Harding)
Pull request description:
The `OutPoint` type has two public fields, providing a `new` constructor that just sets these two fields adds no value.
Done after discussion in #3340 as part of `primitives` work.
ACKs for top commit:
Kixunil:
ACK e064686397
apoelstra:
ACK e064686397 successfully ran local tests
Tree-SHA512: d80cac85093946b3678883f4e3ad7fd2052d858dbd3fab4127916b0eb6153999d30827c84ccf8c6e4412fbc3842bd127a2e4d3ca0248d2307d0bca467a2555ce
f811e0adb6 Stop using deprecated OutPoint functions (Tobin C. Harding)
Pull request description:
Either our CI is failing us or `deprecated` does not work as expected, either way we should not be using the `OutPoint::null()` or `is_null` functions any more because we deprecated them already.
ACKs for top commit:
Kixunil:
ACK f811e0adb6
apoelstra:
ACK f811e0adb6 successfully ran local tests
Tree-SHA512: 64e75601ce8062da78bbd9aea97792e35c728aa31255348413fe9b57ee585974370d3f59b5467d7e4a65dc05d2718aea18bb381e128c8410fbfde4a851006416
The optional dependencies are ordered and separated by whitspace in a
manner that may not be obvious (or even have a reason).
Some of this is because since use of `?` deps changed name.
Put all the optional deps together in alphabetic order.
The `Builder` is staying in `bitcoin` while the `ScriptBuf` is moving to
`primitives`, so we cannot access the inner field of `ScriptBuf`.
Use the new `as_byte_vec` hack to mutate the inner `ScriptBuf` field.
In preparation for moving the `ScriptBuf` type to `primitives` stop
accessing the inner field when encoding/decoding, use `as_script`
and `from_bytes` instead.
In preparation for moving the `Script` type to `primitives` stop
accessing the inner field before doing slice operations, use `as_bytes`
to first get at the slice.
The `Script::fmt_asm` function is a legacy from days yore before
`Display` printed asm. We no longer need it.
Deprecate `Script::fmt_asm` and use the private `bytes_to_asm_fmt` or
`Display` impls.
Use the `define_extension_trait` macro to define two extension traits
for the two txid types. Each trait holds the deprecated `all_zeros`
function. There are no users of this trait in the code base.
Recently we deprecated the `all_zeros` functions on `Wtxid` and
`Txid` but for some reason our usage of them is not triggering a lint
warning.
Note please that this changes logic slightly, for example by using an
array of `0xFF` bytes instead of all zeros. Done in an effort to make it
even more obvious that the value is a dummy value and not mix it up with
the all zeros being used for coinbase thing.
In #3308 we added associated consts to the `Txid`, `Wtxid`, and
`OutPoint` types. During review and afterwards we realised the docs
could do with improving. Since we now want to move the types we should
do this first.
Close: #3331
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
Either our CI is failing us or `deprecated` does not work as expected,
either way we should not be using the `OutPoint::null()` or `is_null`
functions any more because we deprecated them already.
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.
The two `TxOut` fields are public and there are no construtors or
getters to move, only the associated const `NULL`.
Add a tmp module around the big impl block so we can trick the formatter
into indenting before we add the extension trait.
3e034d5ede Add Arbitrary dependency (yancy)
Pull request description:
Adds an example draft showing what is needed to use Arbitrary for coin selection.
Shot out to how nice Arbitrary is for fuzzing a target by taking unstructured randomness and creating structured rust-bitcoin types for fuzzing. Is there a way we could add this to rust-bitcoin for structuring the fuzz data needed?
This is then the example to fuzz test a SRD algo (after applying this PR to rust-bitcoin) using rust-bitcoin types :)
```
#![no_main]
use arbitrary::Arbitrary;
use bitcoin::{Amount, FeeRate};
use bitcoin_coin_selection::{select_coins_srd, WeightedUtxo};
use libfuzzer_sys::fuzz_target;
use rand::thread_rng;
#[derive(Arbitrary, Debug)]
pub struct Params {
target: Amount,
fee_rate: FeeRate,
weighted_utxos: Vec<WeightedUtxo>,
}
fuzz_target!(|params: Params| {
let Params { target: t, fee_rate: f, weighted_utxos: wu } = params;
select_coins_srd(t, f, &wu, &mut thread_rng());
});
```
ACKs for top commit:
tcharding:
ACK 3e034d5ede
Kixunil:
ACK 3e034d5ede
apoelstra:
ACK 3e034d5ede successfully ran local tests
Tree-SHA512: accd565815de3b37730d2ff12a24fcfc84e52ad357e5c940b1500a1e0bb17f4ff5fd6e52d31e8e96bb5290ee4fa050cfd2a9bbd6bbae13fc378f43093b64177f
9fb5edb39e ecdsa: Improve error types (Tobin C. Harding)
Pull request description:
There are a couple of issues around the ECDSA signature decoding / parsing code. We have duplicate code in `from_str` and `from_slice` and both use the same error type even though it is impossible to get a hex error in `from_slice`.
Create two errors:
- A `DecodeError` returned by `from_slice`
- A `FromStrError` that has a decode variant and a hex variant
Call through to `from_slice` after parsing hex into a byte vector.
Removes an instance of `unreachable!`.
Fix: #1193
ACKs for top commit:
Kixunil:
ACK 9fb5edb39e
apoelstra:
ACK 9fb5edb39e successfully ran local tests
Tree-SHA512: 3b3ae31887d603f1739d261b491b99f7847987f94dbbfefb9aa84d4250736eba2d007d28746bbb064946d3055e4cca01510677bf2cdbb11bbf83d7388dbd2620
c427d8b213 bitcoin: Compile time assert on index size (Tobin C. Harding)
49a6acc1a0 internals: Remove double parenthesis in const_assert (Tobin C. Harding)
2300b285ef units: Remove compile time pointer width check (Tobin C. Harding)
Pull request description:
3 patches in preparation for other size related work, this PR does not touch the `ToU64` issue which will be handled separately.
- Patch 1: Don't check pointer width in `units` because its not consensus code
- Patch 2: Modify internal macro `const_assert`
- Patch 3: Use index size to enforce not building on a 16 bit machine
ACKs for top commit:
Kixunil:
ACK c427d8b213 though I think the last commit was kinda a waste of time and it should have been adding the trait instead or leave it for later.
apoelstra:
ACK c427d8b213 successfully ran local tests; unsure if we want to merg this or wait for #3215
Tree-SHA512: 823df5b6a5af3265bce2422c00d287f45816faeb5f965685650ac974a1bd441cf548e25ac2962591732ff221bee91a55703da936382eb166c014ca5d4129edf8
a2be82c0c9 Use TBD in deprecated attribute (Tobin C. Harding)
Pull request description:
Our `release` job checks for 'TBD', I can't remember exactly why but I thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when we used TBD - clearly this is not the case now because we have a bunch of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
ACKs for top commit:
Kixunil:
ACK a2be82c0c9
apoelstra:
ACK a2be82c0c9 successfully ran local tests
Tree-SHA512: b383cc4095484291a7b4dca593ad5e017e3a9de9bfae9d6e9447ae36da32aa1c0d1fd593f49fd52c04db5ca5cdbaae8b30a772f792df13542f0a157a86295746
96e0e720fd feat(bip158): compute canonical filter hash (Rob N)
Pull request description:
From [BIP-157](https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#filter-headers)
> The canonical hash of a block filter is the double-SHA256 of the serialized filter.
If a user forgets the "double" in double-SHA256 they will be computing a nonsensical filter hash when this is easily handled by the API.
ACKs for top commit:
Kixunil:
ACK 96e0e720fd
tcharding:
ACK 96e0e720fd
Tree-SHA512: 5fc0b1632e2327adacbd0ab56b4cd7edce0774f8be2f819782519c51b9691a748f4b3c01887f3bf1146dee49a35f9dfac833f53ae69ee7a53858bd2cedcec01a
There are a couple of issues around the ECDSA signature decoding /
parsing code. We have duplicate code in `from_str` and `from_slice`
and both use the same error type even though it is impossible to get a
hex error in `from_slice`.
Create two errors:
- A `DecodeError` returned by `from_slice`
- A `ParseSignatureError` that has a decode variant and a hex variant
Call through to `from_slice` after parsing hex into a byte vector.
Removes an instance of `unreachable!`.
Fix: #1193
Our `release` job checks for 'TBD', I can't remember exactly why but I
thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when
we used TBD - clearly this is not the case now because we have a bunch
of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
c97389596b Remove stale docs from sha256t_hash_newtype (Tobin C. Harding)
39f7dcb816 Reduce API surface of tagged wrapped hash types (Tobin C. Harding)
Pull request description:
Recently we made it so that wrapper types created with `hash_newtype` were not general purpose hash types i.e., one could not easily hash arbitrary data into them. We would like to do the same for tagged wrapped hash types.
In `hashes` do:
- Create a new macro `sha256_tag` that does just the tag/engine stuff out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256_tag` and `hash_newtype` to create tagged wrapped hash types.
Note that we do not add private helper functions `engine` and `from_engine` to the tagged wrapper types as we do for legacy/segwit in `sighash`. Can be done later if wanted/needed.
Fix: #3135
ACKs for top commit:
Kixunil:
ACK c97389596b
apoelstra:
ACK c97389596b successfully ran local tests
Tree-SHA512: d937a8eac1a77298231f946f9dfbc2f7739af8da00f2075b0b54803b4111c0cec810bc6564515153769193056cf102a9c954e216664f055b249d4a6153b14bca
Recently we made it so that wrapper types created with `hash_newtype`
were not general purpose hash types i.e., one could not easily hash
arbitrary data into them. We would like to do the same for tagged
wrapped hash types.
In `hashes` do:
- Create a new macro `sha256t_tag` that does just the tag/engine stuff
out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256t_tag` and `hash_newtype` to create tagged
wrapped hash types.
Note that we do not add private helper functions `engine` and
`from_engine` to the tagged wrapper types as we do for legacy/segwit in
`sighash`. Can be done later if wanted/needed.
c9053511b2 Remove misleading version metadata (Martin Habovstiak)
Pull request description:
The metadata in dependency specification was misleading because the version was not guaranteed to be the same anyway this was correctly linted but nobody so far cared to fix it. This change fixes it and adds a hint how to get the real version since some people seem still confused about how these things work.
ACKs for top commit:
apoelstra:
ACK c9053511b2 successfully ran local tests
tcharding:
ACK c9053511b2
Tree-SHA512: 99d849b1c1b202a429bc7eb25394ff84ba720b3ca6e73bce615f767e0924f5ef63a87553214f0f4fadd3b68ea84246ab8284d75df56cbbe794da7dea0d169dd9
2bb90b8203 Introduce two extensions traits for ScriptBuf (Tobin C. Harding)
ae0a5bd64a Run cargo fmt (Tobin C. Harding)
3fdc574851 Add temporary script buf modules (Tobin C. Harding)
4ff5d6886b Add private ScriptBufAsVec type (Tobin C. Harding)
c81fb93359 Make push_slice_no_opt pub(crate) (Tobin C. Harding)
1001a33f19 Add second ScriptBuf impl block (Tobin C. Harding)
3625d74e8b Make pub in crate functions pub crate (Tobin C. Harding)
b368384317 Separate ScriptBuf POD methods (Tobin C. Harding)
Pull request description:
Similar to #3155 but for `ScriptBuf`, however it is a little more involved.
Note:
- the change to use `impl` syntax (and addition of #3179)
- mad trickery of `ScriptBufAsVec` (props to Kix)
- widening of scope of private functions
Onward and upward!
ACKs for top commit:
Kixunil:
ACK 2bb90b8203
apoelstra:
ACK 2bb90b8203 successfully ran local tests
Tree-SHA512: 7209d8dc436e52b23e1dbfd9db8432df225ebdb701f465e4d1b55328e22988c98a0f28efdf2a8b3edbafc754354d718ab36bd2f5b1621d12e061b2dadaf49a05
The metadata in dependency specification was misleading because the
version was not guaranteed to be the same anyway this was correctly
linted but nobody so far cared to fix it. This change fixes it and adds
a hint how to get the real version for people who are mistakenly
investigating wrong file.
ad34a98c61 Refactor Rust version checking (Martin Habovstiak)
7d5ce89dad Fix type ambiguity in IO tests (Martin Habovstiak)
Pull request description:
Conditional compilation depending on Rust version using `cfg` had the disadvantage that we had to write the same code multiple times, compile it multiple times, execute it multiple times, update it multiple times... Apart from obvious maintenance issues the build script wasn't generating the list of allowed `cfg`s so those had to be maintained manually in `Cargo.toml`. This was fixable by printing an appropriate line but it's best to do it together with the other changes.
Because we cannot export `cfg` flags from a crate to different crates we take a completely different approach: we define a macro called `rust_version` that takes a very naturally looking condition such as `if >= 1.70 {}`. This macro is auto-generated so that it produces different results based on the compiler version - it either expands to first block or the second block (after `else`).
This way, the other crates can simply call the macro when needed.
Unfortunately some minimal maintenance is still needed: to update the max version number when a newer version is used. (Note that code will still work with higher versions, it only limits which conditions can be used in downstream code.) This can be automated with the pin update script or we could just put the pin file into the `internals` directory and read the value from there. Not automating isn't terrible either since anyone adding a cfg with higher version will see a nice error about unknown version of Rust and can update it manually.
Because this changes syntax to a more naturally looking version number, as a side effect the `cond_const` macro could be also updated to use the new macro under the hood, providing much nicer experience - it is no longer needed to provide human-readable version of the version string to put in the note about `const`ness requiring a newer version. As such the note is now always there using a single source of truth.
It's also a great moment to introduce this change right now since there's currently no conditional compilation used in `bitcoin` crate making the changes minimal. However it is not yet added to `bitcoin-io` since `bitcoin-io` is not depending on `internals`. It might be a reason to start depending on it but that's for later discussion.
ACKs for top commit:
apoelstra:
ACK ad34a98c61 successfully ran local tests
tcharding:
ACK ad34a98c61
Tree-SHA512: 7a82017fc51ba1b473ca638c7bdbf5c893da0a78c70ea8f1a0241049e7874592fad328abd60b3e09a00439b771e7cfc5ebad5e874314d15a48271ec6cb2d7bb7
In preparation for moving the `ScritpBuf` type to `primitives` add a
public and private extension trait for the functions we want to leave
here in `bitcoin`.
Note, includes a change to the `difine_extension_trait` metavariable
used on `$gent` from `ident` to `path` to support the generic
`AsRef<PushBytes>`.
Conditional compilation depending on Rust version using `cfg` had the
disadvantage that we had to write the same code multiple times, compile
it multiple times, execute it multiple times, update it multiple
times... Apart from obvious maintenance issues the build script wasn't
generating the list of allowed `cfg`s so those had to be maintained
manually in `Cargo.toml`. This was fixable by printing an appropriate
line but it's best to do it together with the other changes.
Because we cannot export `cfg` flags from a crate to different crates we
take a completely different approach: we define a macro called
`rust_version` that takes a very naturally looking condition such as
`if >= 1.70 {}`. This macro is auto-generated so that it produces
different results based on the compiler version - it either expands to
first block or the second block (after `else`).
This way, the other crates can simply call the macro when needed.
Unfortunately some minimal maintenance is still needed: to update the
max version number when a newer version is used. (Note that code will
still work with higher versions, it only limits which conditions can be
used in downstream code.) This can be automated with the pin update
script or we could just put the pin file into the `internals` directory
and read the value from there. Not automating isn't terrible either
since anyone adding a cfg with higher version will see a nice error
about unknown version of Rust and can update it manually.
Because this changes syntax to a more naturally looking version number,
as a side effect the `cond_const` macro could be also updated to use the
new macro under the hood, providing much nicer experience - it is no
longer needed to provide human-readable version of the version string to
put in the note about `const`ness requiring a newer version. As such the
note is now always there using a single source of truth.
It's also a great moment to introduce this change right now since
there's currently no conditional compilation used in `bitcoin` crate
making the changes minimal.
Add a private type that allows us to mutate the inner vector of a
`ScriptBuf` only using public functions and never touching the inner
field.
Done in preparation for moving the `ScriptBuf` to `primitives`.
Mad hackery by Kix!
In preparation for adding script buf extension make the
`push_slice_no_opt` have the same scope as the other private functions,
this will be the scope of the private extension trait.
In preparation for adding a private extension trait change the scope to
`pub(crate)` because the more specific `pub(in ...)` is not currently
supported by our `define_extension_trait` macro.
In preparation for moving the `ScriptBuf` as a plain old datatype to
`primitives`; separate the POD methods into their own impl block.
Refactor only, no logic changes.
b8067da934 Parse MSRV minor version number from env (yancy)
Pull request description:
Parse MSRV minor version number from env
replaces: https://github.com/rust-bitcoin/rust-bitcoin/pull/3145
ACKs for top commit:
Kixunil:
ACK b8067da934
tcharding:
ACK b8067da934
apoelstra:
ACK b8067da934 successfully ran local tests
Tree-SHA512: 60b3898c9b2739ca4858218b21d5c136b91b6ef45b1dc8f798d97aac861c42d046f114efcefdfa79f514d6da392f715a715ee6b3f230eb459f34e554b48ccf0e
0857697665 Replace impl blocks with extension traits (Martin Habovstiak)
b99bdcfdd6 Format `Script` blocks (Martin Habovstiak)
b027edffe7 Wrap `Script` impl blocks in temporary modules (Martin Habovstiak)
5a461545c7 Separate private `Script` methods (Martin Habovstiak)
27adc09e9f Generalize fn params in `define_extension_trait` (Martin Habovstiak)
fcc3cb03f0 Support non-doc attrs in extension trait macro (Martin Habovstiak)
ca1735f24c Separate POD methods (Tobin C. Harding)
Pull request description:
This moves methods from `Script` to extension traits in steps that should be easy to follow.
Moving to `primitives` requires doing the same with `ScriptBuf` so I'm holding off until this approach gets concept ACK (or alternatively someone else can do it :))
Closes#3161
ACKs for top commit:
tcharding:
ACK 0857697665
apoelstra:
ACK 0857697665 successfully ran local tests
Tree-SHA512: 3768d879e36139cf971c1921d3236141cbe87d707fd4bab7852f6ed8857b7867fa4146dfe720bd54e3d8cc50ecdc93886a10254cf9a82246358253f0312ffb47
2ec901fd63 Move the CompactTarget type to primitives (Tobin C. Harding)
a00bd7cc4d Introduce CompactTargetExt trait (Tobin C. Harding)
100ce03643 Run cargo +nightly fmt (Tobin C. Harding)
9c4a629659 Wrap CompactTarget impl block in temporary module (Tobin C. Harding)
578143c09e Separate CompactTarget impl blocks (Tobin C. Harding)
22d5646f7b Stop using CompactTarget inner field (Tobin C. Harding)
244d7dbe6c Remove generic test impl (Tobin C. Harding)
3d85ee3a02 primitives: Fix alloc feature (Tobin C. Harding)
Pull request description:
Done in preparation for moving `BlockHash` and `block::Header` to `primitives`.
- Patch 1 introduces an extension trait using `define_extension_trait!`
- Patch 2 is the trivial copy and past to move the type to `primitives`
This one shouldn't be to arduous to review, thanks.
ACKs for top commit:
Kixunil:
ACK 2ec901fd63
apoelstra:
ACK 2ec901fd63 successfully ran local tests
Tree-SHA512: b0e4f1af0b268e249a056cae71d7cafd1b025c4a079e5393ce80cd0b9c9bb6d2c6306531dc6786d986ff8a094b61866a86285b20d54037ef1395d127876bfd9c
In preparation to move script types to `primitives` we replace impl
block with extension traits by replacing the temporary modules with
`define_extension_trait`.
`rustfmt` is unable to format macro calls so instead we wrap the impl
blocks in modules to enable formatting in the next commit. We need to
change the visibility of the methods but that's OK since they're
internal.
The macro was trying to "parse" the parameters of functions defined in
extension trait. This was not needed and it was causing problems around
the `self` parameter. In this commit we change the macro to just pass
the parameters through.
Potentially the whole `pow` module will move to `primitives` but this
is not possible easily right now. However, we would like to be able to
move the `BlockHash` and `block::Header` types over to `primitives`
and doing so requires the `CompactTarget` to be there.
Move the `CompactTarget` type to `primitives` and re-export it from the
`primitives` crate root.
Note also, we re-export the type publicly from `bitcoin::pow`.
In preparation for moving the `CompactTarget` type to `primitives` stop
using the inner field in code that will stay behind in the
`bitcoin::pow` module.
In preparation for moving the `CompactTarget` to `primitives` remove the
generic `Into` impl and explicitly implement for just the `From` impls
that the `pow` unit tests use.
Test code only.
The `define_extension_trait` macro originally didn't support `#[inline]`
or other attributes for simplicity. We still want them so this commit
adds basic support for it. It adds the `doc` attributes to trait
*definition* only and adds all other attributes to the *implementation*
only. This should support `#[inline]` and other attributes. The downside
is it doesn't support adding non-doc attributes to trait *definition*
but I can't think of any relevant ones that we would want and we can
find a solution later if we do.
34e8212594 Replace &self with self: &Self (Tobin C. Harding)
Pull request description:
`foo(&self)` is syntax sugar for `foo(self: &Self)`.
The `define_extension_trait` is currently large, ugly, and not that expressive. If we use `self: &Self` then the macro is greatly simplified.
(Also consuming version `self: Self`)
De-sugar only, no logic changes.
ACKs for top commit:
apoelstra:
ACK 34e8212594 successfully ran local tests; lol this looks so much better
Kixunil:
ACK 34e8212594
Tree-SHA512: 7ec81bee99ede328d73a661c9e683a774ba14404ceb89ecb06765bedddf04dc9721672775b9ad3a9e3356d510c76681848f24ce4392a59d53216d23e6a27d9ad
579b76b7cb Introduce ToU64 conversion trait (Tobin C. Harding)
Pull request description:
The idea for this was pulled out of Steven's work in #2133
We already explicitly do not support 16 bit machines.
Also, because Rust supports `u182`s one cannot infallibly convert from a `usize` to a `u64`. This is unergonomic and results in a ton of casts.
We can instead limit our code to running only on machines where `usize` is less that or equal to 64 bits then the infallible conversion is possible.
Since 128 bit machines are not a thing yet this does not in reality introduce any limitations on the library.
Add a "private" trait to the `internals` crate to do infallible conversion to a `u64` from `usize`.
Implement it for all unsigned integers smaller than `u64` as well so we have the option to use the trait instead of `u32::from(foo)`.
ACKs for top commit:
Kixunil:
ACK 579b76b7cb
apoelstra:
ACK 579b76b7cb successfully ran local tests
Tree-SHA512: 2eaddfff995987a346e052386c6dfef3510e4732e674e3a2cfab60ee391b4cce1bf7ba4fb2dfd4926f8203d7251eea2198ccb61f0b40332e624c88fda4fa7f48
191897f9ea Manually format (Tobin C. Harding)
Pull request description:
Run `rustfmt` and manually fix the places where comments are moved to the wrong place.
ACKs for top commit:
Kixunil:
ACK 191897f9ea
apoelstra:
ACK 191897f9ea successfully ran local tests
Tree-SHA512: f977ff373d1d410012734208c090bfcd8f9dbda414d0b19400acf8f552df481b4a2bc20d77c61538895a6fb66197be13cbdadf74956d67fd4d055b99ba8ab356
We already explicitly do not support 16 bit machines.
Also, because Rust supports `u182`s one cannot infallibly convert from a
`usize` to a `u64`. This is unergonomic and results in a ton of casts.
We can instead limit our code to running only on machines where `usize`
is less that or equal to 64 bits then the infallible conversion is
possible.
Since 128 bit machines are not a thing yet this does not in reality
introduce any limitations on the library.
Add a "private" trait to the `internals` crate to do infallible
conversion to a `u64` from `usize`.
Implement it for all unsigned integers smaller than `u64` as well so
we have the option to use the trait instead of `u32::from(foo)`.
`foo(&self)` is syntax sugar for `foo(self: &Self)`.
The `define_extension_trait` is currently large, ugly, and not that
expressive. If we use `self: &Self` then the macro is greatly
simplified.
De-sugar only, no logic changes.
6836de9ee6 Remove catch all pattern (Tobin C. Harding)
Pull request description:
The `PushBytes` type enforces len is less than 0x100000000 so we do not need to panic in a catch all pattern after explicitly matching against less than 0x100000000.
Refactor only because of the invariant on `PushBytes` - no logic changes.
ACKs for top commit:
apoelstra:
ACK 6836de9ee6 successfully ran local tests
Kixunil:
ACK 6836de9ee6
Tree-SHA512: a7cdb31683a8c00eecbdd0879886bec48133f9029f899b5279f1f5294ef40320592db196bfcafdeef4507636fc785a7ab87879b25b6d1b4905ae573b545f1ff4
The `PushBytes` type enforces len is less than 0x100000000 so we do
not need to panic in a catch all pattern after explicitly matching
against less than 0x100000000.
Refactor only because of the invariant on `PushBytes` - no logic
changes.
84df3438ca Fix markdown list items (Tobin C. Harding)
0a45c68cf8 Introduce helper function name policy (Tobin C. Harding)
Pull request description:
As much as it hurts the C hacker inside me we have settled on using `_internal` to mark private function names that clash with a public function of the same name.
Introduce a policy section and rename one instance, I did not grep the codebase looking for other violations.
This came up because I had to look at what `_inner` implied when reading the function name somewhere else.
ACKs for top commit:
storopoli:
ACK 84df3438ca
apoelstra:
ACK 84df3438ca successfully ran local tests; sgtm. should probably update rust-miniscript which uses a `real_` prefix
Tree-SHA512: aece73fac54c4f34bdba72c08721586eb6d76dc9191442bbca43301f2af3d3b3e3217383f1ad20752fa7654e7f5d09927d1a4b72602aa064e76d550b5d6e81bd
As much as it hurts the C hacker inside me we have settled on using
`_internal` to mark private function names that clash with a public
function of the same name.
Introduce a policy section and rename one instance, I did not grep the
codebase looking for other violations.
This came up because I had to look at what `_inner` implied when reading
the function name somewhere else.
In an effort to reduce the cognitive load of reading code we are
removing casts unless they are useful or obvious.
Move the cast onto the call to `min` and comment it for good measure.
This allows us to call infallible `from` for conversion when needed.
Refactor only, no logic changes.
In an effort to remove unnecessary casts use `u128::from` to convert
from `u64`s. Leave the cast to `u64` in there because it is right after
a shift right and is brain-dead obvious.
Currently we enforce that our code only runs on machines with a
certain pointer width (32 or 64 by failing to compile if pointer size
width is 16). One of the underlying reasons is because of requirements
in consensus code in Bitcoin Core which requires containers with more
than 2^16 (65536) items [0].
We can better express our requirements by asserting on Rust's index
size (the `usize` type).
As a side benefit, there is active work [1] to make Rust support
architectures where pointer width != idex size. With this patch applied
`rust-bitcoin` will function correctly even if that work progresses.
- [0] https://github.com/rust-bitcoin/rust-bitcoin/pull/2929#discussion_r1659399813
- [1] https://github.com/rust-lang/rust/issues/65473
A single trait bound can be expressed using the `impl` style. This is a
breaking change because callers can no longer use turbofish. In this
case that probably does not matter because users are likely just passing
an integer in and letting the compiler infer the type.
Done in preparation for moving logic into an extension trait so that the
functions can be parsed by the `define_extension_trait` macro.
ref: https://doc.rust-lang.org/reference/types/impl-trait.html
c72069e921 Bump MSRV to 1.63 (Martin Habovstiak)
Pull request description:
The version 1.63 satisfies our requirements for MSRV and provides significant benefits so this commit bumps it. This commit also starts using some advantages of the new MSRV, namely namespaced features, weak dependencies and the ability to use trait bounds in `const` context.
This however does not yet migrade the `rand-std` feature because that requires a release of `secp256k1` with the same kind of change - bumping MSRV to 1.63 and removing `rand-std` in favor of weak dependency. (Accompanying PR to secp256k1: https://github.com/rust-bitcoin/rust-secp256k1/pull/709 )
Suggested plan:
* merge both PRs
* at some point release `hashes` and `secp256k`
* remove `rand-std` from `bitcoin`
* release the rest of the crates
ACKs for top commit:
apoelstra:
ACK c72069e921
tcharding:
ACK c72069e921
Tree-SHA512: 0b301ef8145f01967318d3ed1c738d33e6cf9e44f835f3762122b460a536f926916dbd6ea39d6f80b4f95402cd845e924401e75427dbb0731ca5b12b4fa6915e
feef34fdea Make ScriptBuf::p2wpkh_script_code stand alone (Tobin C. Harding)
Pull request description:
We would like to move the `Script` type to `primitives` without moving any key stuff, including pubkey hashes. We may later, before releasing `primitives`, move the `WPubkeyHash` at which time this patch can be reverted or re-implemented on `ScriptBuf`.
The `ScriptBuf::p2wpkh_script_code` function does not take `self` as a parameter but it does return `Self` - this can trivially be made into a standalone function.
Make `ScriptBuf::p2wpkh_script_code` a standalone function.
ACKs for top commit:
Kixunil:
ACK feef34fdea
apoelstra:
ACK feef34fdea
Tree-SHA512: 9de43bb274480b85d328ddd4cb6d79b501a525837307ca399dea1c3d39e9a7c0d8719dbb0e5d852629351d2b1fe09ce0244df82c53ff9ef2342be85a71887329
The version 1.63 satisfies our requirements for MSRV and provides
significant benefits so this commit bumps it. This commit also starts
using some advantages of the new MSRV, namely namespaced features, weak
dependencies and the ability to use trait bounds in `const` context.
This however does not yet migrade the `rand-std` feature because that
requires a release of `secp256k1` with the same kind of change - bumping
MSRV to 1.63 and removing `rand-std` in favor of weak dependency.
29b213daca Move validation module to consensus_validation (Tobin C. Harding)
Pull request description:
The `consensus` module is currently doing two things, validation and encoding. These two things are orthogonal.
Move the `consensus::validation` module to `consensus_validation`. Remove the function re-exports from `consensus`.
This was originally discussed here: https://github.com/rust-bitcoin/rust-bitcoin/issues/2779
ACKs for top commit:
Kixunil:
ACK 29b213daca
apoelstra:
ACK 29b213daca
Tree-SHA512: 3bd0e43c220b0d89a47e9df0e0c92b776ccc65f5f60d57f413db834acc8e86269379bc9fdd688f8c4f0138db22f8eb8983770afa2d7d53d51acf063f2302121c
We would like to move the `Script` type to `primitives` without moving
any key stuff, including pubkey hashes. We may later, before releasing
`primitives`, move the `WPubkeyHash` at which time this patch can be
reverted or re-implemented on `ScriptBuf`.
The `ScriptBuf::p2wpkh_script_code` function does not take `self` as a
parameter but it does return `Self` - this can trivially be made into a
standalone function.
Make `ScriptBuf::p2wpkh_script_code` a standalone function.
3b15ef6d27 Fix rustdocs in `blockdata` (Jamil Lambert, PhD)
Pull request description:
Following up on the [comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/2646#discussion_r1548848611) in #2646 the rustdocs formatting was fixed in `blockdata`.
ACKs for top commit:
Kixunil:
ACK 3b15ef6d27
tcharding:
ACK 3b15ef6d27
Tree-SHA512: 509aa2b8ae559f5a7f8230c016c0866f468cc147773238e226b9a975784d7a424c41a5d966ddcf9b3f8f8d4fe197a4f272b5777c61f3cc53051803abb520b95e
ab581a90f8 Remove re-export of ParseIntError (Tobin C. Harding)
Pull request description:
In d242125 I claimed that `ParseIntError` was somehow special, I no longer thing this is the case. As we pin down the re-export policy (for errors and other types) it is hard if we have one non-typical re-export.
We have https://github.com/rust-bitcoin/rust-bitcoin/issues/3068 to discuss the policy, for now just remove the unusual re-export.
ACKs for top commit:
Kixunil:
ACK ab581a90f8
shinghim:
ACK ab581a90f8
apoelstra:
ACK ab581a90f8
Tree-SHA512: 5ac4123aeb27c8cee78e5760f21e70be8035d526ba7e14e72759cba27f98b51cc2cba9b2bf0eeb99e0f6b7210ec4a750986bb6c5dc0725ed892730fdec8a7e06
54c30556a2 Move params to network module (Tobin C. Harding)
045a661ebe Create network directory (Tobin C. Harding)
Pull request description:
Discussed in #2779. Patch one moves `network.rs` to `network/mod.rs`, and patch 2 moves the `params` module over there.
ACKs for top commit:
apoelstra:
ACK 54c30556a2 Yeah, this seems like a good place for it
Kixunil:
ACK 54c30556a2
Tree-SHA512: 134813419db21323d303d465b12fcbf37fd61dc1baf3305e156d324eafd822379e63dede02877ee99dce41540193a29e6e13acd13f9121f3e2fe11096524aa5e
The `consensus` module is currently doing two things, validation and
encoding. These two things are orthogonal.
Move the `consensus::validation` module to `consensus_validation`.
Remove the function re-exports from `consensus`.
386ad93253 Manually format function parameters (Tobin C. Harding)
871f4398b9 Add optional comma to function parameter list (Tobin C. Harding)
Pull request description:
During #2955 I was lazy and did not think through why the macro didn't handle function parameters on individual lines, I just manually re-formatted all function calls onto a single LOC. This was a bit slack of me.
- Patch 1: Fix the macro.
- Patch 2: Revert the manual formatting.
ACKs for top commit:
jamillambert:
ACK 386ad93253
Kixunil:
ACK 386ad93253
apoelstra:
ACK 386ad93253
Tree-SHA512: 577154668a4cd0bad225a5b306193a5cd3d38d275dd5df22b4a29737e0d2c910c695da33c0ed77265f6c5d876c162316e444f8eca80f9c30788d110d989493e3
beea3c1e5d Remove public error re-export (Tobin C. Harding)
Pull request description:
We do not have a policy to re-export things from other modules just because they are in the public API - I don't see any other reason to re-export this error, users should go to the `validation` module directly to get the error type.
Raising this trivial change as a separate PR so that we can really pin down our re-export policy. Please review the policy implications as well as the code change.
Note please that this change was introduced in 7d695f6b4 by me, and buried in a PR that did not mention the change. This was wrong, as in the code change was wrong and also the patching method was wrong.
ACKs for top commit:
Kixunil:
ACK beea3c1e5d
Tree-SHA512: 5fc072f3fb8a727f30751211c6bc85dc268d413ee62937c714bdf9f47405dfdbc93cfff3df76c201493c39f49d5d315907fc9e7e4fa0d927652c01038815fdc5
In d242125 I claimed that `ParseIntError` was somehow special, I no
longer thing this is the case. As we pin down the re-export policy (for
errors and other types) it is hard if we have one non-typical re-export.
We have https://github.com/rust-bitcoin/rust-bitcoin/issues/3068 to
discuss the policy, for now just remove the unusual re-export.
The `Params` struct is currently defined in the `consensus` module which
has become a collection of orthogonal consensus-ish things. We would
like to put things in more descriptive places.
The `Params` struct defines constants that are network specific so it
makes sense to put it in the `network` module. As soft proof of this
argument note in this patch how often the `Params` type is imported
along with the `Network` type.
API break:
The type is no longer available at `bitcoin::consensus::Params` but
rather is re-exported at `bitcoin::network::Params`.
We do not have a policy to re-export things from other modules just
because they are in the public API - I don't see any other reason to
re-export this error, users should go to the `validation` module
directly to get the error type.
Now that the `define_extension_trait` can handle function parameters on
individual lines revert the manual formatting that was introduced in
PR #2955.
Refactor only, no logic changes.
The `define_extension_trait` macro currently does not allow for a
trailing comma in the function paramater list, this occurs when the
function paramaters are put on individual lines (eg by `rustfmt`).
Extend the macro to support function paramaters being on individual
lines. This should have been done originally instead of my manual
override of formatting - bad Tobin no biscuit.
2169b75bba Use lower case error messages (Jamil Lambert, PhD)
Pull request description:
Error messages should be lower case, except for proper nouns and variable names. These have all been changed.
~~They should also state what went wrong. Some expect error messages were positive, giving the correct behaviour or correct input. These have been changed so that they are now negative, i.e. saying what went wrong.~~
EDIT: After further discussion it was decided not to change the expect messages.
ACKs for top commit:
Kixunil:
ACK 2169b75bba
tcharding:
ACK 2169b75bba
Tree-SHA512: 92442c869e0141532425f6fca5195fd319b65026f68c4230a65ad70253565d98931b2b44ee202975c307280525c505147e272297dc81207312e40c43d007021c
64c31cfb97 Move locktimes and Sequence to primitives (Tobin C. Harding)
Pull request description:
The `absolute` and `relative` locktimes as well as the `Sequence` are all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in `bitcoin` and we re-export everything from `blockdata`.
Note please `Sequence` is no longer publicly available at `bitcoin::transaction::Sequence` but it is available at `bitcoin::Sequence`.
ACKs for top commit:
Kixunil:
ACK 64c31cfb97
apoelstra:
ACK 64c31cfb97
Tree-SHA512: 968aa595bfb53e8fcfa860dae797ec5381ed49c67326a8ef9494086ec65d737502dffe4b24143e159042d07c59c5109d50103029082f87e1c3c26671e975f1b3
The `absolute` and `relative` locktimes as well as the `Sequence` are
all primitive bitcoin types.
Move the `Sequence`, and `locktime` stuff over to `primitives`.
There is nothing surprising here, the consensus encoding stuff stays in
`bitcoin` and we re-export everything from `blockdata`.
164b72e07b Update Key documentation (Shing Him Ng)
Pull request description:
The documentation for `Key` was a bit confusing since `<key> := <keylen> <keytype> <keydata>` was right above the `key` field, when in reality the `key` field represents the key data. Moving the documentation that describes a `Key` as a whole to the struct itself and specifying that `key` represents key data will hopefully clear things up a bit
ACKs for top commit:
Kixunil:
ACK 164b72e07b
tcharding:
ACK 164b72e07b
Tree-SHA512: 33c2b24d3006a0ed85a5d96351e0a01c1365c8ea01472233b024de54941319cdbefa0126f8b9538385e8f33ba7e2e3895f0dc5b6a36a1c501c8b97ebbede6502
bcf6d2839e Introduce scriptPubkey extension traits (Tobin C. Harding)
ee333defa4 Remove path from ScriptBuf (Tobin C. Harding)
Pull request description:
Done in preparation for moving the script types to `primitives`.
Add three public traits, one each for the three `script` types: `Builder`, `ScriptBuf`, and `Script`.
ACKs for top commit:
Kixunil:
ACK bcf6d2839e
apoelstra:
ACK bcf6d2839e
Tree-SHA512: 0ad11e474ddf1183d0119e36454cb4fd18d49a68655d274df800c6ef20afa7f8d0fdecd415c02595ea67a011e3a842b7ccc23c2d58f92ed9acbdc7f277fbd217
Done in preparation for moving the script types to `primitives`.
The script types have a bunch of functionality to support scriptPubkeys,
and scriptPubkeys are an address thing.
Create a module under `address` and in it create a bunch of extension
traits to hold all scriptPubkey functionality.
Includes adding an ugly-as-hell macro to create the traits.
ac4db6369d witness: Add Witness::witness_script inspector (Steven Roose)
6cc6c8621a witness: Add Witness::taproot_annex (Steven Roose)
b0848022eb witness: Add Witness::taproot_control_block (Steven Roose)
ef336e1387 witness: Improve Witness::tapscript (Steven Roose)
e48a2e4225 script: Add Script::redeem_script inspector (Steven Roose)
Pull request description:
Bundled these because they are very similar. Got a bunch of larger changes coming up based on these. I've been using these for a while for TXHASH work.
ACKs for top commit:
apoelstra:
ACK ac4db6369d but will need to wait for next release. I think we should merge these as-is although they will be much clearer after we do script tagging.
tcharding:
ACK ac4db6369d
Tree-SHA512: e1590d1bdc8b91aeba137453f0cdaa7e1ae6df3c8e9e1e0f087ed9be1a6beaf2286818379247d26c5dd27d07c12c10433db1c9b9a71667ab4d8d37c7deff1373
dc96475f58 Add/fix alloc features (Tobin C. Harding)
Pull request description:
Eventually we would like all our crates other than `bitcoin` to be able to be used without an allocator. Currently, and during crate smashing, this is not that useful because so much of the code comes from `bitcoin` and relies on the availability of an allocator.
As an initial step, add the `alloc` feature to `addresses` , `base58`, and `primitives`.
In order to to keep `--no-default-features` builds working make the crates empty if the `alloc` feature is not enabled. This is a suboptimal solution because the error messages users will get when they forget to enable `alloc` will be confusing (eg something like primitives does not contain Transaction). However our CI script (`run_task.sh`) expects `--no-default-features` to build cleanly (as do I).
ACKs for top commit:
apoelstra:
ACK dc96475f58
Kixunil:
ACK dc96475f58
Tree-SHA512: 28006ad638e72d3c712becaf94f6aaddc559fb2f7e7ad0d0810348fe979fb61e32f53e5b20894e472c5ac9e2b7f80cba6a3f0e12b766b817f412a927957ae3a2
Eventually we would like all our crates other than `bitcoin` to be able
to be used without an allocator. Currently, and during crate smashing,
this is not that useful because so much of the code comes from `bitcoin`
and relies on the availability of an allocator.
As an initial step, add the `alloc` feature to `addresses` , `base58`,
and `primitives`.
In order to to keep `--no-default-features` builds working make the
crates empty if the `alloc` feature is not enabled. This is a suboptimal
solution because the error messages users will get when they forget to
enable `alloc` will be confusing (eg something like primitives does not
contain Transaction). However our CI script (`run_task.sh`) expects
`--no-default-features` to build cleanly (as do I).
a738754f67 Add TxIdentifier trait (Tobin C. Harding)
Pull request description:
Add a new trait `TxIdentifier` that abstracts over the `Txid` and `Wtxid` types. We make `AsRef` a super trait so that the new trait needs no methods.
Seal the trait so consumers of the library cannot implement it.
Use the new trait in:
- the `bip152` module to tighten up the `with_siphash_keys` function
- as a trait bound on the `Leaf` associated type in the `MerkleNode` trait
ACKs for top commit:
apoelstra:
ACK a738754f67
Kixunil:
ACK a738754f67
Tree-SHA512: a7bda26a4a5107f96b24ea3c163286a7ab21a817bdec3434b3ab27d78e99c0548a7362a2271d362b89038c80d9251767c0d62e1df702ef57d9edaf141ab55cd6
51010777bf hashes: Strongly type the hash160::HashEngine (Tobin C. Harding)
d5dd54a489 hashes: Strongly type the sha256d::HashEngine (Tobin C. Harding)
a7422a779c hashes: Add const hash engine constructors (Tobin C. Harding)
Pull request description:
Currently we are using a type alias for a few of the hash engines.
Type alias' allow for potential mixing of types, a struct can better serve our users with not much additional complexity or maintenance burden.
- As preparation add const constructors where possible to hash engines (excl. `HmacEngine`).
- Add a `sha256d::HashEngine` struct
- Add a `hash160::HashEngine` struct
If this goes in we can improve the `sha256t::HashEngine` in a similar manner.
ACKs for top commit:
Kixunil:
ACK 51010777bf
apoelstra:
ACK 51010777bf but will hold off on merging until tcharding indicates whether he wants to address the midstate thing
Tree-SHA512: 810db3a8cd66e4d135b04a65d5b4c926cf2e5e8ac531619bbd884db69df17b29b64daeb6fb31b8b1fb32bffbf81cf84e55cd46945c743451c73f1b7f63489f63
9a586987d1 Move opcodes to primitives (Tobin C. Harding)
Pull request description:
Move the `opcodes` module to the new `primitives` crate. This is pretty straight forward, some things to note:
- Are we ok with the public wildcard re-export from `blockdata`? I think so because the whole `blockdata` module should, IMO, be deleted after everything in it is moved to `primitives`.
- `decode_pushnum` becomes public.
ACKs for top commit:
Kixunil:
ACK 9a586987d1
apoelstra:
ACK 9a586987d1
Tree-SHA512: ee9fa0ae4265f54ff7784dc873abc12572852c32ff24456e34cd6a8a004f9e1f932e01c80d3448107fca76507db4bdaa3dfff6b5a80de0707d59a033e582fb9e
afd19ebd61 Use super for imports in script module (Tobin C. Harding)
Pull request description:
In the `script` module we currently import `script` types using the fully qualified path, as recently discussed code is easier to maintain if we use `super` when `super != crate`.
Internal change only, no external changes.
ACKs for top commit:
Kixunil:
ACK afd19ebd61
apoelstra:
ACK afd19ebd61
Tree-SHA512: 5d8546de3d8d9014054fb53020ec0d742eb9698d6d70cd0b2e722b5e43792f4d1ad480d1c61d67b10ddda6a5ef42839118f0de0332251bd64cf31f0a566fd40b
Move the `opcodes` module to the new `primitives` crate. This is pretty
straight forward, some things to note:
- Are we ok with the public wildcard re-export from `blockdata`? I think
so because the whole `blockdata` module should, IMO, be deleted after
everything in it is moved to `primitives`.
- `decode_pushnum` becomes public.
Includes addition of a `patch` section for `primitives` in the
`bitcoin/embedded` crate.
In the `script` module we currently import `script` types using the
fully qualified path, as recently discussed code is easier to maintain
if we use `super` when `super != crate`.
Internal change only, no external changes.
Currently we are using a type alias for the `hash160::HashEngine`.
Type alias' allow for potential mixing of types, a `hash160::HashEngine`
struct can better serve our users with not much additional complexity or
maintenance burden.
As we did for the `sha256d::HashEngine`, add a new wrapper type
`hash160::HashEngine` that replaces the current type alias.
Currently we are using a type alias for the `sha256d::HashEngine`.
Type alias' allow for potential mixing of types, a `sha256d::HashEngine`
struct can better serve our users with not much additional complexity or
maintenance burden.
Add a new trait `TxIdentifier` that abstracts over the `Txid` and
`Wtxid` types. We make `AsRef` a super trait so that the new trait needs
no methods.
Seal the trait so consumers of the library cannot implement it.
Use the new trait in:
- the `bip152` module to tighten up the `with_siphash_keys` function
- as a trait bound on the `Leaf` associated type in the `MerkleNode` trait
837f466f72 bip152: check if indexes do not overflow (Bruno Garcia)
Pull request description:
When deserializing a `HeaderAndShortIds` we could check if the number of txs does not overflow 16 bits. If so, throw an error.
ACKs for top commit:
apoelstra:
ACK 837f466f72
Kixunil:
ACK 837f466f72
Tree-SHA512: 21797689758ae22666289cde123e3e9ae1bdb1ab2f85a1cd0f07ee8fa2a441e7b1b928f30b8fb1f65aef0c526c899eb96b83ed4fff074e70b31e0afad2c35930
7fa53440dc Move serde_round_trip macro to internals (Tobin C. Harding)
Pull request description:
We currently duplicate the serde_round_trip macro in `units` and `bitcoin`, this is unnecessary since it is a private test macro we can just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding also, elect to use the `bincode` crate because we already have it in our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and its usage (preventing the transient dependency on `bincode` and `serde_json`).
ACKs for top commit:
Kixunil:
ACK 7fa53440dc
apoelstra:
ACK 7fa53440dc
Tree-SHA512: f40c78bf2539940b7836ed413d5fe96ce4e9ce59bad7b3f86d831971320d1c2effcd23d0da5c785d6c372a2c6962bf720080ec4351248fbbdc0f2cfb4ffd602c
4bb9240992 bitcoin: Add comment to manifest (Tobin C. Harding)
c6c70a721e bitcoin: Update feature docs (Tobin C. Harding)
7712e5d891 bitcoin: Use 100 colum width in crate level docs (Tobin C. Harding)
Pull request description:
Update stale crate level docs in `bitcoin`. Done as two separate patches so the last one is trivial to review, whitespace only.
ACKs for top commit:
Kixunil:
ACK 4bb9240992
apoelstra:
ACK 4bb9240992
Tree-SHA512: b9d5474f7e7a0576f535df428ea20084acbcb74d0576e7f2934c547dd2c54f4a939d73ec547b3b254105a45c2372113c65ce136be1eabd63701259a3a6de3737
We currently duplicate the serde_round_trip macro in `units` and
`bitcoin`, this is unnecessary since it is a private test macro we can
just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding
also, elect to use the `bincode` crate because we already have it in
our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and
its usage (preventing the transient dependency on `bincode` and
`serde_json`).
The `bitcoin` crate documents its features in the crate level rustdocs,
currently they are stale.
Update and improve the feature docs section of crate level docs.
Currently we copy data into a new buffer before passing it into the
hasher, we can just hash the data directly.
Internal change only, no external change.
Fix: #2917
15f6bacec9 api: Run just check-api (Ryan Breen)
9684d496bb Add is_standard_op_return (Ryan Breen)
Pull request description:
This is the suggestion for #2292 to check OP_RETURN length
ACKs for top commit:
apoelstra:
ACK 15f6bacec9
Tree-SHA512: e346b5eff7cc40b98a08948c83cb5c064184541d819c37a977e432ec09df7f9e1a074f16a4df598142784bd875f1379e2b0848fe898923e4e12829f85b4c4520
865ba3fc39 Move serde string macros to internals (Tobin C. Harding)
4a2b13fcde internals: Feature gate whole serde module (Tobin C. Harding)
Pull request description:
The macros are internal things and can live in `internals`. This will help with future crate smashing.
ACKs for top commit:
apoelstra:
ACK 865ba3fc39
Kixunil:
ACK 865ba3fc39
Tree-SHA512: 7b3f029206c690ecf2894e0ad099d391312f7f8ec65ac9b5d4d9f25e6827f92075dcc851d0940a0faf1e27e7d0a305b575c8cc790939b3f222d7a2920d4d24fe
c717f7f424 Improve docs on private Witness fields (Tobin C. Harding)
Pull request description:
The `Witness` type is a reasonable complex data structure, make an effort to clarify its structure in the docs on the private fields.
Private docs only.
(Original idea pulled out of #2133.)
ACKs for top commit:
Kixunil:
ACK c717f7f424
apoelstra:
ACK c717f7f424 much clearer, thanks!
Tree-SHA512: 9d54b7eeefec97e584fb5f275049dbac0473c949fae8ab05c6961d6fc424c17a058af7037c2220ef1446af294d78c68bfee741cfeca1b18ecc402935d8069dab
d099b9c195 Remove wildcard from prelude import (Jamil Lambert, PhD)
Pull request description:
This patch replaces `prelude::*` wildcard imports with the types actually used. In a couple of cases `DisplayHex` was previously imported by the wildcard but was only used in the test module, an additional import was added to the test module instead of at the top where it causes an unused import warning.
Close: #2875
ACKs for top commit:
Kixunil:
ACK d099b9c195
tcharding:
ACK d099b9c195
Tree-SHA512: d59dfac0961d2649d509039a11c1b5574d81d05fef567a624cf15be2f587de796ea960ba5a08bef788199331c2f790fb06f7b393182538c7d8b1891ded119efc
The `Witness` type is a reasonable complex data structure, make an
effort to clarify its structure in the docs on the private fields.
Private docs only.
010141ecc9 api: Run just check-api (Tobin C. Harding)
cf800a1b07 Remove bech32 dependency from blockdata (Tobin C. Harding)
Pull request description:
We have a single usage of the `bech32` crate inside the `blockdata` module, to convert a `WitnessVersion` to a `Fe32`. We then have a single call site where we use the conversion in the `address` module.
This code was written without thinking to hard about the introduced dependency on `bech32`, in hindsite it shouldn't have been added.
In preparation for splitting a bunch of code in `blockdata` out into the `primitives` crate remove the `bech32` stuff from the `witness_version` module.
ACKs for top commit:
Kixunil:
ACK 010141ecc9
apoelstra:
ACK 010141ecc9
Tree-SHA512: 2d368ebf64ab7197b421e0dec48623f3fb03243081a988ff9ed2070135c8741efe24520c2aab496d54c52595808f10ee39816eaab8e3e4a7e64955cd25285670
a7731b2f33 api: Run just check-api (Marko Bencun)
47cba7a655 bip32: add from_32_slice method to DerivationPath (Marko Bencun)
Pull request description:
ChildNumber already has a `From<u32>` impl, but converting `&[u32]` to a `DerivationPath` was still difficult.
ACKs for top commit:
Kixunil:
ACK a7731b2f33
tcharding:
ACK a7731b2f33
Tree-SHA512: 2db94ee035e686102b8201f637422bb96bd79858aeffdb007594d722902d31a20f27e61b88fae8c854c80f785d9e7837b0158a046639ff8cc2d20d8883391842
dc10a49876 api: Run just check-api (Tobin C. Harding)
5e8f204581 Pass sigs and associated types by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference.
Currently this is not done in secp, but lets do it here in bitcoin.
Pass by value:
- `SerializedSignature`
- bitcoin sigs
- secp sigs
- secp `Message`
This is a continuation of the work to split up #2404 into manageable PRs.
ACKs for top commit:
apoelstra:
ACK dc10a49876
Kixunil:
ACK dc10a49876
Tree-SHA512: 8736eba067c74edb951c92357f5b3d0fc99c4fa6dc3beea579c10b3150873b74e8ec46c2c01f18818b37fca6e77c6b6edddeb6340edde6a9d8c28a4e69164c8c
6a7f780018 api: Run just check-api (Tobin C. Harding)
0c9223ac05 Manually format write_err statement (Tobin C. Harding)
43d7c750cc taproot: Add error types (Tobin C. Harding)
afe41c8a39 taproot: Split errors up (Tobin C. Harding)
Pull request description:
Currently there are a couple of errors in the `taproot` module that are too general, resulting in functions that return a general error type when a specific one would do.
Split two errors out and use them for for enum variants and function returns as possible.
Done as part of #2883
ACKs for top commit:
Kixunil:
ACK 6a7f780018
apoelstra:
ACK 6a7f780018
Tree-SHA512: bf5ed50dd8f913280d007f03124c7918c4b6cd642e67726bc3ffff23d9897f764a4391e167115668c3a1e951197485eba280fdbb8a0ce1bb7efb051388d13997
Wildcards have been replaced with what is actually used.
In a couple of cases an additional use statement was added to the test
module to import `DisplayHex` which is only used in test, but
previously imported with the wildcard at the top.
We have a single usage of the `bech32` crate inside the `blockdata`
module, to convert a `WitnessVersion` to a `Fe32`. We then have a single
call site where we use the conversion in the `address` module.
This code was written without thinking to hard about the introduced
dependency on `bech32`, in hindsite it shouldn't have been added.
In preparation for splitting a bunch of code in `blockdata` out into the
`primitives` crate remove the `bech32` stuff from the `witness_version`
module.
10440b36a2 api: Run just check-api (Shing Him Ng)
452a7cc091 Re-export public functions, enums, and structs from units::parse in the bitcoin crate root (Shing Him Ng)
Pull request description:
Re-export UnprefixedHexError in the bitcoin crate root
Fixes#2874
ACKs for top commit:
tcharding:
ACK 10440b36a2
Tree-SHA512: 78a0676aa6d6fe63d810eeeebd3bf053d679d966d0aebf752760bfb440b2d4e831b0ff6b54b0306b390126b3f96a70c0846b4a1ec20aca96994d4a7b75e438e3
8ee1744b9b Make 'use core::fmt' calls consistent (Shing Him Ng)
Pull request description:
I started taking a look at #2869 and looked for everything that was implementing the `Display` trait:
```rust
impl fmt::Display for _
```
but found some places where the imports weren't consistent:
```rust
impl Display for _
```
There were only a few instances of the latter, so I went ahead and cleaned those up before starting #2869
I started pulling this thread when I saw the same thing was happening for `fmt::Debug` and `fmt::Formatter` so I updated the rest of the `use core::fmt::*` statements with a few exceptions:
- No updates to `use core::fmt::*` if it was being called from within a function since I felt like the function scope was small enough to not cause confusion
- No updates to `use core::fmt::{self, Write as _};`
ACKs for top commit:
Kixunil:
ACK 8ee1744b9b
tcharding:
ACK 8ee1744b9b
Tree-SHA512: 33eb6ea0c4e808ef78bc87de6547144b756bde206c50d80488f740e97cd8d11f1abcb8936c487d7bfd29be5e21c7f40ff88f82acdaaec9aacb4b6362ffc4c680
bc25ed35d5 Order serde feature list alphabetically (Tobin C. Harding)
5bd3387c15 Move package metadata to be underneath package section (Tobin C. Harding)
a2a9f193fe Put workspace crates in alphabetical order (Tobin C. Harding)
05931cc0fa Run the formatter (Tobin C. Harding)
Pull request description:
We are getting an increasing number of crates in the repo, clean up the manifests a bit in an endevour to help keep things manageable.
All patches are trivial and the PR makes no logic changes.
ACKs for top commit:
Kixunil:
ACK bc25ed35d5
apoelstra:
ACK bc25ed35d5
Tree-SHA512: a9850449a6f71ac5d53f501e36175e900bf4986f44c7636d3b1b55df80804b92bb10d8da7798f6bb866722aa2354ad2880ab5c0f5c4633f198c137d2ca42b7c9
Currently there are a couple of errors in the `taproot` module that are
too general, resulting in functions that return a general error type
when a specific one would do.
Split two errors out and use them for for enum variants and function
returns as possible.
2bde5d002e api: Run just check-api (Jose Storopoli)
d1f84329e4 psbt: implement const for PsbtSighashType::ALL (Jose Storopoli)
Pull request description:
Closes#2751.
I only did the `ALL` which is by far the most common case.
ACKs for top commit:
tcharding:
ACK 2bde5d002e
apoelstra:
ACK 2bde5d002e
Tree-SHA512: 693575de758657a3e172d86ba5114ec0bf3b12b82df598e38c6a7916c99c20cfb5c4e74442108b51ae4e7bb1f1e940fd4a7269145e3f9838f727675c7711c890
The package metatadata never changes and is not necessary to look at
basically ever, put it down the bottom of the manifest out of the way.
Helps to keep features and dependencies closer together.
Refactor only, no logic changes.
We manually implement these methods (and the GeneralHash trait) on newtypes
around sha256t::Hash, because tagged hashes require a bit more work. In
the next commit (API diff) you will see that this affects two hashes,
which are the only things that appear green in the diff.
Users who want to implement their own engine/from_engine types now need
to do it on their own. We do this for the non-Taproot sighash types in
`bitcoin` (though only privately) to demonstrate that it's possible.
This is a continuation of the previous commit, but separated to make
review a little easier. This one replaces test vectors that were
previously computed by hashing garbage into Txids and various other hash
types with new test vectors which are directly-computed garbage
converted to hashes with from_byte_array.
In one case (src/hash_types.rs) this results in changing a bunch of
fixed test vectors. This is okay; this test is supposed to check the
direction of string serialization, which is unaffected by this commit
(or any commit in this PR). The existing test vectors, because they hash
the empty string, result in different bytes depending on the underlying
hash algo (sha256, sha256d, sha256t, etc). The new ones just use the
same fixed test vector for all of them.
This commit also updates a doctest in crypto/sighash.rs which
demonstrates how to manually feed sighash data into a hash engine and
correctly handle the sighash single bug. Because you can no longer
directly get a sighash object from an engine, this particular example
should maybe be rewritten to just encode to a Vec rather than a hash
engine, explaining that maybe you'd do this when implementing a HWW, to
verify the exact data being hashed. Or something.
Unrelatedly, you can check that there are no API changes in this commit
or the last several. The next commit will remove GeneralHash impls and
that's when you'll see changes.
In the next commits we are going to stop exposing the ability to hash
arbitrary data into wrapped hash types like Txid etc. In preparation for
this, stop using these methods internally.
This makes our internal code a little bit uglier and less DRY. An
alternative approach would be to implement the from_engine and engine
methods, but privately (and maybe having a macro to provide this). But I
think this approach is more straightforward.
The one exception is for the Taproot hashes, which are tagged hashes and
currently do not have their own engine type. I will address these in a
later PR because this one is already too big.
4652ce20ed API changes for "delete `all_zeros`" (Andrew Poelstra)
8869f35a69 hashes: drop the `all_zeros` method on arbitrary hashes (Andrew Poelstra)
9f8797f486 API changes for constification of hash constructors (Andrew Poelstra)
154e91af8c hashes: constify a bunch of constructors (Andrew Poelstra)
c155cbf8b2 hashes: use workaround to get constfns on tagged hashes with MSRV (Andrew Poelstra)
Pull request description:
I think these changes are both uncontroversial but they have fairly large diffs so I am PRing them together before making more invasive changes.
ACKs for top commit:
tcharding:
ACK 4652ce20ed
Tree-SHA512: 4560fa397deab50448598894b9364f9d8f8b48169901a84db6a44168cdba795ab69b48ad2cac61caebcee5e227a03271335b405cf5514265290a4d1f2fdf52a2
We have various different column widths being used in a single rustdoc
block, since we favour 100 for comments around here use it.
No text changes, whitespace only.
We should pass `Copy` types by value not by reference.
Currently this is not done in secp, but lets do it here in bitcoin.
Pass by value:
- `SerializedSignature`
- bitcoin sigs
- secp sigs
- secp `Message`
aebf216619 Use 100 column width for rustdoc (Tobin C. Harding)
c71ae9ac16 Move PushBytes::read_scriptint (Tobin C. Harding)
Pull request description:
The `push_bytes` module has a `private` module that exists solely to protect the invariant on the `PushBytes` inner byte slice. There is a `PushBytes` impl block outside the private module for functions that can not and do not violate the length invariant.
Recently we move the `read_scriptint` method to be on the `PushBytes` but we put it inside the `private` module, since the method only reads off of the slice it cannot invalidate the invariant and does not need to be inside the `private` module.
Move the `read_scriptint` method outside of the `private` module to keep that module as small as possible, helping with its stated aim of being the only place that requires auditing.
Patch 2 is whitespace only.
ACKs for top commit:
Kixunil:
ACK aebf216619
apoelstra:
ACK aebf216619
Tree-SHA512: f41eb691e7118a74767a2582a637a176a46e4a8d15259f1c1f3cef67bc57dd8dce4974407c5a5e31acd0b4665f722b20acad4e381747c9d496f18158a0ef9def
72655607b6 api: Run just check-api (Tobin C. Harding)
e8250cd96a Remove InvalidInternalKey variant from TaprootBuilderError (Tobin C. Harding)
Pull request description:
This variant is unused, remove it.
Done as part of #2883.
ACKs for top commit:
Kixunil:
ACK 72655607b6
apoelstra:
ACK 72655607b6
Tree-SHA512: 5cd27cacebcf078afdf1ceba4e3d51e78f20ee4883000b766efb7a246fd7a166240038a2e7d27249a22049c3258673a393ff2fd62cb4b27a2cade04b28ef2ac9
433fd6bf7e api: Run just check-api (Tobin C. Harding)
8fd583b069 Pass hash types by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the hash types by value.
Second step in the pass-copy-types-by-value work, pulled out of #2404.
ACKs for top commit:
apoelstra:
ACK 433fd6bf7e
Kixunil:
ACK 433fd6bf7e
Tree-SHA512: 999d12f60550cacc4ae19b4cbf505b25c1eed803820f22b1a706e9f95da1b7e7b422f393f4115d579927c0c476cd504036a39b3cdc06a1d6befbcff5513f7433
39df0a9fbe update api (Divyansh Gupta)
3a5f2932a4 create constants for ChildNumber enum (Divyansh Gupta)
Pull request description:
this aims to fix#2750
ACKs for top commit:
tcharding:
ACK 39df0a9fbe
Tree-SHA512: e1c38568facd2b9aa55b1b1ec0d5d5f68ff38ca3fe68962bc316c060a062299935aa51bcfc1c255a7f5c9ad97435cab22e2c160d3fd3f52a46f6b5cbb7d5743f
Manually implement it for Wtxid, Txid and BlockHash, where the all-zero
"hash" has a consensus meaning. But in general we should not be
implementing this method unless we have a good reason to do so. It can
be emulated or implemeted in terms of from_byte_array.
The use of Wtxid::all_zeros is obscure and specific enough that I am
tempted to drop it. But for txid and blockhash, the 0 hash appears in
actual blockdata and we should keep it.
All other uses of all_zeros were either in test code or in places where
the specific hash was not important and [u8; 32] was a more appropriate
type.
We use 100 column width for rustdoc in this project, while not a super
hard rule the docs on `read_scriptint` are long, using the 100 column
width reduces the line count a reasonable amount.
No text changes, only whitespace.
The `push_bytes` module has a `private` module that exists solely to
protect the invariant on the `PushBytes` inner byte slice. There is a
`PushBytes` impl block outside the private module for functions that can
not and do not violate the length invariant.
Recently we move the `read_scriptint` method to be on the `PushBytes`
but we put it inside the `private` module, since the method only reads
off of the slice it cannot invalidate the invariant and does not need
to be inside the `private` module.
Move the `read_scriptint` method outside of the `private` module to keep
that module as small as possible, helping with its stated aim of being
the only place that requires auditing.
a42bcdc22e Remove usage of blockdata from paths (Tobin C. Harding)
Pull request description:
the `blockdata` directory is code organisation thing, all the types/modules are re-exported from other places. In preparation for, and to make easier, the `primitives` crate smashing work - remove all explicit usage of `blockdata`.
Note that the few instances remain as they seem required e.g.,
`pub(in crate::blockdata::script)`
Refactor only, no logic changes.
Done as part of #2883
ACKs for top commit:
apoelstra:
ACK a42bcdc22e lgtm! I consider this "trivial" and will one-ACK merge it
Tree-SHA512: 310605e5203cf04aaeb91fe5512677b8f1438b183916686ba2cdc41ffdc18af7a0676206724e8a14c50ce6ed8faa9d48c69a2d5149eb1f56ae9c5f276fc5200f
16e4d22693 clarify the meaning of Height & Time based locktime. (Divyansh Gupta)
Pull request description:
this pr aims to fix : #2697
ACKs for top commit:
apoelstra:
ACK 16e4d22693
tcharding:
ACK 16e4d22693
Tree-SHA512: 55757d7e593cb284aff7040cf3298931c7f3d8e9e36d7328bd748a39be743e5c4202c55505add0219b2766d35d1660affc5ed4a7b9480b3a3bfb89982fe3970a
the `blockdata` directory is code organisation thing, all the
types/modules are re-exported from other places. In preparation for, and
to make easier, the `primitives` crate smashing work - remove all
explicit usage of `blockdata`.
Note that the few instances remain as they seem required e.g.,
`pub(in crate::blockdata::script)`
Refactor only, no logic changes.
Drop recursion, reduce memory usage to be logarithmic in size of tree
rather than linear, and put it all in one function rather than three.
Also make the method an trait method on MerkleNode which makes it a
easier on type inference, by writing e.g. TxMerkleNode::calculate_root.
Currently we are defining the two merkle tree hash types in the `block`
module, a better home for them is the `merkle_tree` module.
This is an API breaking change because the types were public in the
`block` module, however the change should/could be unnoticeable to users
if they use the crate level re-export - which is maintained.
e7f33a2a12 Update PushBytes::read_scriptint(x) to x.read_scriptint() (Shing Him Ng)
Pull request description:
Fixes#2849
There were a few other usages of PushBytes::read_scriptint(_) in the tests such as [this](406e3486ab/bitcoin/src/blockdata/script/tests.rs (L315)), but they couldn't be updated as cleanly as the changes in this PR. I wasn't sure if the intention of this issue was to fix those as well, but I can update this PR if needed
ACKs for top commit:
tcharding:
ACK e7f33a2a12
Kixunil:
ACK e7f33a2a12
Tree-SHA512: 4620f4972c40b0bf7333dbe302d1dabc5dbcb39749c734cc297a019d36983757f59659d76ae40b4a6121e51d0bde1e2b7883a0c77536be18927d1cfef53ba579
There are two limits that the Bitcoin network enforces in regard to
hashing scripts
- For P2SH the redeem script must be less than 520 bytes
- For P2WSH the witness script must be less than 10,000 bytes
Currently we are only enforcing the p2sh limit when creating an address
with `Address::p2sh`.
There are various ways to create addresses from script hashes and if
users manually hash a script then use the `ScriptHash` (or
`WScritpHash`) our APIs assume the script that was hashed is valid. This
means there is the potential for users to get burned by creating
addresses that cannot be spent, something we would like to avoid.
- Add fallible constructors to `ScriptHash` and `WScriptHash`
- Add `TryFrom` impls as well to both types
- Remove the `From` impls
The script in a p2wsh is typically referred to as the witness script not
the redeem script - rename the local test variable to follow suit.
Refactor only, no logic changes.
This test does an odd combination of function calls, its not obvious
what it is supposed to be testing. The `to_p2wsh.is_p2wsh` is already
tested above. The leading `to_p2sh` does not prove anything, one can put
currently pass any script to `to_p2wsh` so this tests nothing.
In preparation for patching the script hashing functionality first
remove this odd test.
3f4eb07769 Add a comment to regression test (Jamil Lambert, PhD)
fc2876ba10 Move use statements to top of module (Jamil Lambert, PhD)
778a44dd64 Refactor merkle block test (Jamil Lambert, PhD)
c4c1252a9e Change encode path (Jamil Lambert, PhD)
Pull request description:
Refactored the `extract_matches_from_merkleblock()` test function following https://github.com/rust-bitcoin/rust-bitcoin/pull/2859#issuecomment-2161710169.
Moved use statements to the top of the test module and changed it to use one level of path instead of importing the function names. e.g. `encode::serialize()` instead of `serialize()`.
Added the missing comment to the `regression_2606()` test. I was not sure where the hex value came from that was used to test that the deserialization fails. The comment was generated by copilot and may need to be edited, it does fit with the error given by deserialize: `OversizedVectorAllocation { requested: 12811880876963004416, max: 4000000 }`.
ACKs for top commit:
tcharding:
ACK 3f4eb07769
apoelstra:
ACK 3f4eb07769 the `prelude::*` is fine since it was already there since #298, but FYI I would not have accepted it today
Tree-SHA512: 203a30eee51ea91051cb10d5d7dd55b560d9d4d785120143c9fb29ea26ec77696124adc9c5bcb8cd736a7d293b897e665958bec5f66626a5c1c95c98b6029e0d
9f01871c11 api: Run just check-api (Tobin C. Harding)
7929b51640 Pass keys by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the key types by value.
This is patch 1 from #2404
ACKs for top commit:
apoelstra:
ACK 9f01871c11 this will annoy some people but I think we should do it
Tree-SHA512: 18afab537edf4ade4dc1c1e5992e50060b8935531f1e3cbe1d3b94b2fcb87aafa39947f342e0e762835bda3b4091dd35b3b74ea79f4dbb3b21660ffd21d1f82e
18b2788a5a api: Run just check-api (Tobin C. Harding)
6b7d02e5ae Add inherent functions to hashes (Tobin C. Harding)
Pull request description:
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`, and other use cases. However, it is unegonomic for users who just want to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make progress in the hashes API re-write. We can come back and de-dublicate later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and `all_zeros` const where easily possible.
ACKs for top commit:
apoelstra:
ACK 18b2788a5a
Tree-SHA512: 6b7a8d8a8501e981416d767040e5bd9fa8d1134be2ca133b5c53aa55f65c8456dccb63b642e30d0d571ca838c6f9eaeff6527d92a9b4212819a49ce619c4e093
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`,
and other use cases. However, it is unegonomic for users who just want
to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with
the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make
progress in the hashes API re-write. We can come back and de-dublicate
later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and
`all_zeros` const where easily possible.
8d256b4e79 Moved doctest to unit test (Jamil Lambert, PhD)
Pull request description:
Moved the doctest from the private module to a unit test to fix: #2840
ACKs for top commit:
apoelstra:
ACK 8d256b4e79
tcharding:
ACK 8d256b4e79
Tree-SHA512: 233961288579381cb5c51235331c78c9980046f0317a2062cb108a2e604adde322e2dd834e724d1fe05f54f92721fe6c9f5344dcf930f99aa3756a698ddf9732
fcf27a62a2 update api (Divyansh Gupta)
531aba0cf1 make `difficulty_float` general to all network (Divyansh Gupta)
Pull request description:
Fix#2783
ACKs for top commit:
tcharding:
ACK fcf27a62a2
apoelstra:
ACK fcf27a62a2
Tree-SHA512: aec9648b057677b89f4f397e1fa703ed6496436bd6e1e8052d5b7f52ef52c95b286babd425d97fe7d4f903cd0a3d68ae589d8fb255fc68f423714918cd485873
091d614aad bitcoin: Remove "std" feature from examples (Tobin C. Harding)
Pull request description:
The "rand-std" feature enables "std" but we use it in examples still. FTR I added this a while ago thinking the explicitness was clearer but in hindsight I think that was wrong and that it makes usage of our features _less_ clear.
No logic changes.
(Pulled out of #2756.)
ACKs for top commit:
storopoli:
ACK 091d614aad
apoelstra:
ACK 091d614aad
Tree-SHA512: d72eec37e3a434a1f850cb4257529fc18540cb5075bc7d3bd494cba59b08404c6c86223361a3c3a3de8d50a1168b656ff1123d28f7d2dcdf05c404caff716b1a
2db88a62fd Update bitcoinconsensus version (Jamil Lambert, PhD)
Pull request description:
Updated bitcoinconsensus version to 0.106.0+26.0 in bitcoin/Cargo.toml
The new version supports taproot and has a new parameter for `spent_outputs` in the `verify()` and `verify_with_flags()` functions.
The validation module was changed to keep the existing functionality by adding `None` as the `spent_outputs` and the flag `VERIFY_ALL_PRE_TAPROOT`.
This method does not add taproot features to the verify functions.
ACKs for top commit:
tcharding:
ACK 2db88a62fd
apoelstra:
ACK 2db88a62fd agreed, this is a good step forward
Tree-SHA512: b6cef395e065cfe859a7896ee2deb2f2d255051566751c10d06092064f6523338ca912aad81b789d6ca94e6f6164f2eef874ad6086c6ebe1384ce520d6eba366
8aa893ebd0 Remove repetition from sha256t_hash_newtype macro (Tobin C. Harding)
Pull request description:
The `sha256t_hash_newtype` macro is hard to reason about because we allow repetition so which tag goes with which type is slightly obscure.
Remove repetition and call the macro three times.
Internal change in `bitcoin`, API change in `hashes`.
Fix#2811
ACKs for top commit:
apoelstra:
ACK 8aa893ebd0 nice, small diff
Tree-SHA512: b38e7c307ac7288b4a5c1c3170ad6aa54c62bd3198922ec8bb091867b230bb9149f7dc996766fc8fa20a1af18b318c475b3e83e2689d322b7f4af0d5cb588e50
Updated bitcoinconsensus version to 0.106.0+26.0.
The new version supports taproot and has a new parameter for spent outputs in the `verify()` and `verify_with_flags()` functions.
The validation module was changed to keep the existing functionality by adding `None` as the `spent_outputs` and the flag `VERIFY_ALL_PRE_TAPROOT`.
ce585dc529 api: Run just check-api (Liu-Cheng Xu)
61565957ad Add API for extracting the inner payload of RawNetworkMessage (Liu-Cheng Xu)
Pull request description:
I'd like to take out the `payload` of `RawNetworkMessage` and then send it to the actual network message processor, but find there is no way to do it. This commit adds such an API to expose all the inner parts (UPD: so that I don't have to do an unnecessary clone to obtain the owned value of `payload`).
ACKs for top commit:
apoelstra:
ACK ce585dc529
tcharding:
ACK ce585dc529
Tree-SHA512: 89c5f1361a8c2d0ecf928325e9a37b26d39cf32c3a023b78742555a5a1e59e1579522e942ff6f48b07b2d3b81b48c5e8182fa6e88b57fc9c33ce13e313ba2982
The `sha256t_hash_newtype` macro is hard to reason about because we
allow repetition so which tag goes with which type is slightly obscure.
Remove repetition and call the macro three times.
Internal change in `bitcoin`, API change in `hashes`.
The "rand-std" feature enables "std" but we use it in examples still.
FTR I added this a while ago thinking the explicitness was clearer but
in hindsight I think that was wrong and that it makes usage of our
features _less_ clear.
No logic changes.
76826313a1 generate Network <-> Magic From and TryFrom with a macro (Antoni Spaanderman)
Pull request description:
Removes possible errors when a network is added to the enum (expressed with the comment `// Note: new network entries must explicitly be matched in 'try_from' below.`)
ACKs for top commit:
Kixunil:
ACK 76826313a1
apoelstra:
ACK 76826313a1
Tree-SHA512: 37aaf4b9021204c24e3dc405e666f3dea8c4f8e478b26892dd962a49c988904ba02c3dee7cec1ad78d4e4bb9ba9565ec4574d0e169dd215d26b010f1c4dd3d0b
I'd like to take out the `payload` of RawNetworkMessage and
then send it to the actual network message processor, but
finds there is no way to do it. This commit adds such an API
to expose the owned value of inner `payload`.
Add a unit test that fails if put before the "pow: Fix off-by-one error"
patch. Tests that we can correctly parse a 32 character long hex string
into a `U256`.
d6ef16af30 update api (Divyansh Gupta)
a336ec0dda refactor(script): move `read_scriptint` to `PushBytes` & create `read_int` function (Divyansh Gupta)
Pull request description:
* Moved `read_scriptint` method to `Push_Bytes` struct
* Created `Instruction::read_int` method which acts as a wrappper around this function.
Done as part of #1547
ACKs for top commit:
apoelstra:
ACK d6ef16af30
tcharding:
ACK d6ef16af30
Tree-SHA512: e33df8adcb1c23351da303f6bad1ea4a8eae30e65943d230ae886183a01f970aecd0c8c8fd3a6c337cfe6dde1b7590778d88c308415e393f137065ef7da4b29c
Some of our CI shell scripts are meant only to be sourced and not
run directly however they include an initial shebang line, implying that
they should be run.
Remove the shebang line from `crates.sh` and the various `test_vars.sh`
scripts. Add a `shellcheck` directive to inhibit the no-shebang warning.
Fix: #2764
We just added to now types that are thin wrappers around `u32`s for
block heights and intervals.
Add `Encodable` and `Decodable` impls and use the new types. While we
are at it re-export the types from the crate root so users don't have to
dig into the `units` crate.
9e4b092fce psbt: Use macro instead of function (Tobin C. Harding)
Pull request description:
We have a private function that makes use of the `Hash` trait to generically hash map entries. This usage makes patching the `hashes` module difficult. We can achieve the same thing by using a macro and passing in the concrete type.
This is an internal change, no effect on logic or public API.
ACKs for top commit:
apoelstra:
ACK 9e4b092fce
Tree-SHA512: 8b788fa91d21bbae556c746c2e55e6e9395e022bedf13193555ef7482109b6ef5032b233c5f37543a31ebda49d9b4761c161ca0db501472047eb661a48e944b7
6ba7758b30 Improve array macros (Tobin C. Harding)
Pull request description:
Currently we have two macros used when creating array wrapper types, one is in `internals` and the other in `bitcoin::internal_macros`. It is not immediately obvious what is what and why there are two.
Improve the macros by:
- Move the inherent functions to `impl_array_newtype`
- Use `*_byte_array` for the names instead of `*_bytes`
- Re-name the other macro to match what it now does
ACKs for top commit:
apoelstra:
ACK 6ba7758b30
Tree-SHA512: 36ed0fae0d28f24d29287062eb05bbc1e9e8b565f4ff41fd893503a25404ed8e185a34d75e398a8a660923ffda3b832b6157011598d5a75a5c4aafdffc74af2a
eda61ddfef Deprecate to_vec in favour of to_bytes (Tobin C. Harding)
Pull request description:
Currently we have to method names for the same thing "copy this object into a vector". The library is easier to use if we are uniform and just use one.
Elect to use `to_bytes`, for context see discussion in PR #2585.
ACKs for top commit:
apoelstra:
ACK eda61ddfef Nice. IMO we should start deprecating stuff for two releases rather than one, so that people have a year to update.
Tree-SHA512: 0aadd1258a07bfa53806f19a3c41af8d3b1132aa42e7a2015a59c58c4309d7a9b50b86d076c181ce5870ba5acd989feec32669352ecf857ae6fd982873482c34
1282b7f34b examples: drop a couple allocations (Andrew Poelstra)
45e0241267 doc: fix "lazy line continuations" in markdown (Andrew Poelstra)
Pull request description:
Rust nightly as of 2024-05-27 has a new lint which detects list items which are continued by a non-indented line. Markdown treats these as single list items, which they sometimes are, but sometimes we intended them to be on a separate line.
Also changes the docs for `UntweakedKeypair::tap_tweak` because the existing ones were overly technical and out-of-date.
Also fixes a minor "no need to create a vec then dereference it" lint in the examples.
ACKs for top commit:
storopoli:
ACK 1282b7f34b
tcharding:
ACK 1282b7f34b
Tree-SHA512: 989c83b6bbbfbf8ddd2ade63b5f590f76810f4ec56fab432b580a40e3cc1468a0ea657dcc7ad0d4a31d49eb1390219616fec5a76b5680d5fddc3a06d81db30d7
b5ef7db3c0 api: Run just check-api (Tobin C. Harding)
1b40550ce8 Add an AddressData type (Tobin C. Harding)
Pull request description:
In the 0.32.0 release we removed the `address::Payload` struct because it was deemed an implementation detail. As a byproduct of doing so we made it impossible for users to match on an enum and get the address payload (or data).
- Add a public `AddressData` enum that holds an address' encoded data.
- Add a conversion function to `Address` that returns the data enum.
This patch is additive and is expected to be backported and release as a `0.32` point release.
ACKs for top commit:
apoelstra:
ACK b5ef7db3c0 I still feel a little partial to calling the struct "DecodedAddress" and the method "decode"...but this is good, and I do not want to bikeshed
Tree-SHA512: d97836bb2d7fc0f6e9fbba2afb30eeefefc88e7105d4765a146dd444c8397dd4d1ef4fd3e3eb925589294d46bfc8a66d33797a05dbc2131923534364424c135c
Rust nightly as of 2024-05-27 has a new lint which detects list items
which are continued by a non-indented line. Markdown treats these as
single list items, which they sometimes are, but sometimes we intended
them to be on a separate line.
Also changes the docs for `UntweakedKeypair::tap_tweak` because the
existing ones were overly technical and out-of-date.
In the 0.32.0 release we removed the `address::Payload` struct because
it was deemed an implementation detail. As a byproduct of doing so we
made it impossible for users to match on an enum and get the address
payload (or data).
- Add a public `AddressData` enum that holds an address' encoded data.
- Add a conversion function to `Address` that returns the data enum.
This patch is additive and is expected to be backported and release as a
`0.32` point release.
3615410d21 api: Run just check-api (Tobin C. Harding)
a3d2d1a184 Make Address:p2sh_from_hash public (Tobin C. Harding)
Pull request description:
We previously made this function Private and added a comment that doing so was somehow better to remove the footgun of hashing the wrong length script. However in hindsight this was a bad idea and users want the functionality.
Make the `Address:p2sh_from_hash` public and document it as we do for `Address::p2sh`.
This is an additive change and is expected to be backported to `v0.32`, as part of the fix to #2784. Please note it introduces the footgun that is described in the function rustdoc. This will be improved as a separate patch and added to the current release.
ACKs for top commit:
apoelstra:
ACK 3615410d21
Tree-SHA512: 535bb7894eeef8ecb5afb7bf6e5c483cd42c6a4282d1c116e5bf86cd1364a8327bbec1efb8634a578f07ad2832c1e5daf7fe7e844574b88b1ad355a443627bef
11bb1ff6ff Standardize function doc Safety, Returns and Parameters (jamil.lambert)
df83016c98 Standardize function doc Errors (jamil.lambert)
d219ceb68e Standardize function doc Examples (jamil.lambert)
233a9133d8 Standardize function doc Panics (jamil.lambert)
Pull request description:
The subheadings in the rustdocs have been standardized according to [./CONTRIBUTING.md](https://github.com/rust-bitcoin/rust-bitcoin/blob/master/CONTRIBUTING.md):
```rust
impl FooBar {
/// Constructs a `FooBar` from a [`Baz`].
///
/// # Errors
///
/// Returns an error if `Baz` is not ...
///
/// # Panics
///
/// If the `Baz`, converted to a `usize`, is out of bounds.
pub fn from_baz(baz: Baz) -> Result<Self, Error> {
...
}
}
```
ACKs for top commit:
apoelstra:
ACK 11bb1ff6ff
tcharding:
ACK 11bb1ff6ff
Tree-SHA512: 163af3cd1cfb47cea3e55eddeaeb6843ff7ec89c57354e3247d6bae85e756b183e8045c2555cfcf87e8c23c1388ff9d7592cfb6a951a37a9ec41d27263e5a2e4
cf3e1eb198 api: Run just check-api (Tobin C. Harding)
98bf213c52 bitcoin: Remove error module (Tobin C. Harding)
a5b93cb159 Flesh out hex unit parsing API (Tobin C. Harding)
Pull request description:
Add to `units::parse` the complete suit of hex unit parsing functions:
- remove prefix
- assert without prefix
- parse with or without prefix
- parse with prefix
- parse without prefix
- parse prefix unchecked
Refactor `bitcoin` to use the exact function we need, removing code duplication.
This is a breaking change to `units`, it does however keep the current re-exports from the public, now empty, `bitcoin::error` module.
ACKs for top commit:
apoelstra:
ACK cf3e1eb198
Tree-SHA512: 1778108d4364e290e8956cfea6f23fcdd82c835844d034a00b4cf5cab5552e3efbe853dfbf8a3e0a4bd53a8e3da9d6f7c7408d332d18cd7090aec16fc1f02fe7
We previously made this function Private and added a comment that doing
so was somehow better to remove the footgun of hashing the wrong length
script. However in hindsight this was a bad idea and users want the
functionality.
Make the `Address:p2sh_from_hash` public and document it as we do for
`Address::p2sh`.
726ff25c46 Hard code genesis script bytes instead of hex (Tobin C. Harding)
6e5592db77 Use test_hex_unwrap in bench code (Tobin C. Harding)
Pull request description:
Currently we have a dependency on `hex_lit` and it is used in exactly one place outside of test code, if we instead use a hardcoded array instead we can move the `hex_lit` dependency to `dev-dependencies`.
Hard code the genesis block script bytes as an array of hex digits, link to the blockstream explorer for those interested and comment the bytes liberally since it took me a while to work out what they were.
Move the `hex_lit` dependency and update the lock files.
ACKs for top commit:
apoelstra:
ACK 726ff25c46
Tree-SHA512: 96110332fc24dd5b251150b32737fa198113244c3b51b35453c8c1fcc8386c5a2f68dddb30d78cf2f9e1762550099fdb4109dc550f4c144625795ce60b86e574
7f29313d36 Update API (Tobin C. Harding)
4f29adf163 Enable getting the witness program from an address (Tobin C. Harding)
Pull request description:
We have getters for the pubkey hash and script hash but we forgot one for the witness program - add it.
Done as part of fixing #2784, this is an additive change and is expected to be backported to `v0.32`
ACKs for top commit:
apoelstra:
ACK 7f29313d36
Tree-SHA512: 365aba572eaacb789f5424c233be067500ceff18dc27e28fc3be123c49d27333e95bbf4527469a9caf8cc2fe54f8e13a0fc83bc3fc7c3356aea876c9dd2fa5f0
9bb75703a1 Header change from arguments to parameters (jamil.lambert)
Pull request description:
In a few cases a function header documents the parameters of the following function under the heading "Arguments", this has been changed to "Parameters".
Since the description is at the level of the function definition and not where it is being called parameters seems the more accurate term.
ACKs for top commit:
apoelstra:
ACK 9bb75703a1
tcharding:
ACK 9bb75703a1
Tree-SHA512: aa24af3fd6e086c09f5e2605fa58289969fc7188f63d7f53c0e325315644f9704d51d4cf526ebfc51b2cf9216155fc3d48cc6bca759dc14bae15e4770de5116e
The `error` module is empty except for public re-exports. We are still
in the "break everything and get the API right" stage so this module
adds no value - remove it.
Add to `units::parse` the complete suit of hex unit parsing functions:
- remove prefix
- assert without prefix
- parse with or without prefix
- parse with prefix
- parse without prefix
- parse prefix unchecked
Refactor `bitcoin` to use the exact function we need, removing code
duplication.
This is a breaking change to `units`, it does however keep the current
re-exports from the public, now empty, `bitcoin::error` module.
30a482504b bump nightly-version (Andrew Poelstra)
5ad7c245e3 cargo: whitelist all cfgs used in this repo (Andrew Poelstra)
814786b0a6 crypto: enable and fix accidentally disabled unit test (Andrew Poelstra)
Pull request description:
https://github.com/rust-lang/rust/issues/124800 has been fixed and we can update our nightly version by whitelisting all cfgs that are used.
There was one place where we had an old `cfg(feature = "no-std")` despite having removed the feature. By removing that cfg check we re-enabled a previously disabled test.
ACKs for top commit:
tcharding:
ACK 30a482504b
Tree-SHA512: d25bed819091db74b9d47cb2c23caa3ceb0d7be323b37831326e2ec1608cb1577d41aad2e1cdf59d66df69397537bc3e17a3c2872935d5a4f46f4dc55b5e613c
Currently we have a dependency on `hex_lit` and it is used in exactly
one place outside of test code, if we instead use a hardcoded array
instead we can move the `hex_lit` dependency to `dev-dependencies`.
Hard code the genesis block script bytes as an array of hex digits, link
to the blockstream explorer for those interested and comment the bytes
liberally since it took me a while to work out what they were.
Move the `hex_lit` dependency and update the lock files.
We would like to move the dependency on `hex_lit` to be a
dev-dependency but currently are using it in bench code. The bench
code is enabled if any downstream crate tries to build with
`--cfg=bench` and during such a build our dev-dependencies are not
available.
We also have the `test_hex_unwrap` macro in the `hex` crate and since
the bench code is more or less test code (and the macro call is not
being benchmarked) we can use that macro instead.
c8caee2b5e Document CompactTarget order/equality (Tobin C. Harding)
Pull request description:
Add documentation to the `CompactTarget` type explaining the nuance surrounding order/equality.
Close: #2110
ACKs for top commit:
apoelstra:
ACK c8caee2b5e
Tree-SHA512: c724b31ee620ff08d3c8b547250bc7067f875ef6cf4ce9efa082d5a9cfbd8b92620f86034e58caf573c479ce7aaa89bb7e9fa93dc356524663d3ecf583df3507
In a few cases a function header documents the parameters of the following function under the heading"Arguments", this has been changed to "Parameters"
merge-script
Jamil Lambert, PhD
Tobin C. Harding
Shing Him Ng
Peter Ryszkiewicz
Fmt Bot
Nadav Ivgi
Jiri Jakes
Liam Aharon
ChrisCho-H
elsirion
Antoni Spaanderman
yancy
Rob N
Martin Habovstiak
leichak
douglaz
Bruno Garcia
Ryan Breen
Marko Bencun
Jose Storopoli
Divyansh Gupta
Liu-Cheng Xu
Velnbur