New clippy lint in rustc nightly "lifetime flowing from input to output
with different syntax can be confusing".
Apply the suggested fix and use the anonymous lifetime for paths.
There is a new lint error on nightly-2025-04-25 "variables can be used
directly in the `format!` string".
Exclude the lint to allow the existing syntax in `format!` strings.
492073f288 Strengthen the type of `taproot_control_block()` (Martin Habovstiak)
e8a42d5851 Unify/reduce usage of `unsafe` (Martin Habovstiak)
d42364bd9d Swap around the fields in `Address` (Martin Habovstiak)
7a115e3cf1 Make `Address` obey sanity rules (Martin Habovstiak)
bc6da1fe07 Swap around the fields in `sha256t::Hash` (Martin Habovstiak)
8ee088df74 Make `sha256t` obey sanity rules (Martin Habovstiak)
Pull request description:
Well, I thought this PR will be just the last commit... 😅
Anyway, this implements a bunch of changes to allow returning `ControlBlock` from `Witness` method(s). One cool side effect is that this PR also reduces the number of `unsafe` blocks.
ACKs for top commit:
apoelstra:
ACK 492073f28876406f8fe5a07a8a2495c8e0ba1fb3; successfully ran local tests
Tree-SHA512: 11979517cc310abf25644fc93a75deccacae66af8ba2d9b4011fdc3f414b15fac7e748399c7eef492ca850c11b7aacc3f24ec46fccf95e6d57a400212979637e
Since the introduction of `Script` `unsafe` started slowly creeping in
as more types with similar semantics were added. The `unsafe` in these
cases is just for trivial conversions between various pointer-like
types. As such, it's possible to move these into a single macro that
takes care of the conversions at one place and avoid repeating the same
`unsafe` code in the codebase. This decreases the cost of audits which
now only need to happen in `internals`, focuses any changes to happen in
that single macro and decreases the chance that we will mess up
similarly to the recent `try_into().expect()` issue (but this time with
UB rather than panic).
The new macro accepts syntax very similar to the already-existing struct
declarations with these differences:
* The struct MUST NOT have `#[repr(transparent)]` - it's added by the
macro
* If the struct uses `PhantomData` it must be the first field and the
real data must be the second field (to allow unsized types).
* The struct must be immediately followed by an impl block containing at
least on conversion function.
* If the struct has generics the impl block has to use the same names of
generics.
* The conversion functions don't have bodies (similarly to required
trait methods) and have a fixed set of allowed signatures.
* Underscore (`_`) must be used in place of the inner type in the
conversion function parameters.
The existing code can simply call the macro with simple changes and get
the same behavior without any direct use of `unsafe`. This change
already calls the macro for all relevant existing types. There are still
some usages left unrelated to the macro, except one additional
conversion in reverse direction on `Script`. It could be moved as well
but since it's on a single place so far it's not really required.
9a572dabde refactor: use path dependencies for workspace members in bitcoin/Cargo.toml (Eval EXEC)
Pull request description:
This PR want to:
1. make all workspace members use `workspace = true` syntax to import dependencies.
2. use `path = [balabala]` to define dependencies, instead of useing `[patch.crates-io.balabala]` , fix: https://github.com/rust-bitcoin/rust-bitcoin/issues/4283
ACKs for top commit:
Kixunil:
ACK 9a572dabde
apoelstra:
ACK 9a572dabdeb077f96b2ab66be1a80fcec3e805e3; successfully ran local tests
Tree-SHA512: 834ef881ed3fd324a9ecca440e8e591984a7e474eb6aeab86a0301cbd08b6dc96ecdc34b306ad146b11b50f7488024c289b8f8c7c6de1a2bdba7aec515b722ee
280eb2239a io: Remove the impl_write macro (Tobin C. Harding)
Pull request description:
We have found calling macros from other crates to be error prone, especially when they involve feature gates. Furthermore the `impl_write` macro is trivial to write, users can just write it in the crate that needs it.
Lets just remove the macro. While we are at it point users to the examples in `bitcoin/examples/io.rs` for usage of the `io` crate.
Close: #3872
ACKs for top commit:
Kixunil:
ACK 280eb2239a
apoelstra:
ACK 280eb2239a2fcaa83a35ae6fb1ce62fe35a1e222; successfully ran local tests
Tree-SHA512: 62766ff6e4b5f3fae2d6214e87bd90c15b3103248effd4399a070ac831473b0288e599eed2377c08d2b7eca263220f172c8399a607756793477f82d3dc8f1b67
We would like to remove the `GeneralHash` trait but we want to keep the
`hash_reader` functionality.
Add a stand alone function (when `hashes` is enabled) `hash_reader`.
Remove the extension trait.
We would like to do away with the `GeneralHash` trait. Currently we
bound `Hmac` and `HmacEngine` on it but this is unnecessary now that we
have added `HashEngine::finalize` and `HashEngine::Hash`.
Bound the `HmacEngine` on `HashEngine` (which has an associated `Hash`
type returned by `finilalize`).
Bound `Hmac` type on `T::Hash` where `T` is `HashEngine`.
Includes some minor shortening of local variable names around hmac
engine usage.
Note this means that `Hmac` no longer implements `GeneralHash`.
The tests in `hash` require hex. Previously this was enabled when
`alloc` was enabled. Now `alloc` doesn't enable `hex` add `hashes` to
`dev-dependecies` and set it to require `hex`
Currently in order to release `hashes v1.0` we need to 1.0 `io` as well.
For multiple reasons, many out of our control, the `io` crate may not
stabalise any time soon.
Instead we can invert the dependency between the two crates.
This is an ingenious idea, props to Kixunil for coming up with it.
Notes
- `io` does not currently re-export the `hashes` crate.
- This work highlights that we cannot call `hash_reader` on a siphash.
- The `Hmac::hash_reader` uses the default key which may not be obvious.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
We have found calling macros from other crates to be error prone,
especially when they involve feature gates. Furthermore the `impl_write`
macro is trivial to write, users can just write it in the crate that
needs it.
Remove the macro. While we are at it point users to the examples in
`bitcoin/examples/io.rs` for usage of the `io` crate.
71fab82a1b io: Improve docs on macro (Tobin C. Harding)
9c81d5e747 io: Move macro_export below docs (Tobin C. Harding)
b109177e11 io: Improve rustdocs (Tobin C. Harding)
07d8703a00 io: Add SPDX identifier (Tobin C. Harding)
b844637935 io: Remove rustdoc for private module (Tobin C. Harding)
Pull request description:
As part of #3643 improve the public documentation on the `io` crate.
While doing this I also checked:
- C-FAILURE
- C-LINK
- C-METADATA
- C-RELNOTES
- C-HIDDEN
ACKs for top commit:
apoelstra:
ACK 71fab82a1b32e6077adb896be9d8ac12d5513e0a; successfully ran local tests
Tree-SHA512: 91b8807102277fb7f6602837b7d0e64e4276c9c5bf748ab875ea0e4f1f7f91bc989413acd25e4412d72d6f3744a22b746ed63cbac20d9b42217cd3164c7e6847
Currently we use a marker that contains an `UnsafeCell` but `UnsafeCell`
is not `Sync` so this makes `io::Error` not `Sync`. We can instead wrap
the `UnsafeCell` and implement `Sync` for it.
Fix: #3883
The `description` method was deprecated in Rust `v1.42`. The `cause`
method was deprecated in Rust `v1.33`. Our MSRV is now Rust `v1.63`.
We do not need to implement the deprecated functions any longer.
Fix: #3869
Add two unit tests:
- Check we can read into an empty buffer as validation of args as
part of C-VALIDATE
- Do basic read using `Take::read_to_end` since it is currently
untested.
Note that the docsrs build enables all features so the `impl_write`
macro gets a `std` feature flag even though it is available without the
`std` feature enabled. I can't think of a solution to that slight
annoyance ATM.
The current docs on `impl_write` seem to be stale. The macro just does a
simple call through implementation of `crate::Write` and the same for
`std::io::Write` if `std` is enabled.
Improve the rusdocs by doing:
- Remove module level docs because this is a private module and they
only add minimal value.
- Put the docs on the `std` macro only (docs build uses --all-features)
- Explain just what the macro does and include an `# Arguments` section.
The `io::macros` crate uses a kind of nifty trick of putting
`macro_export` _above_ the docs. But we do not do this anywhere else in
the code base so its a bit surprising. We should be uniform.
Simply because its an easier change move the `macro_export` attribute to
be under the docs.
Internal change only.
In preparation for releasing `io v1.0` make a sweep of the rustdocs and
improve them for all modules except `macros`.
With this applied I believe we can tick off:
- C-FAILURE
- C-LINK
- C-HIDDEN
The `io` crate is licensed in the manifest using the CC0-1.0 license
same as the rest of the codebase but none of the individual files have a
license blurb.
Add a CC0-1.0 license blurb by way of an SPDX-License-Identifier tag.
426f585a47 api: Run just check-api (Tobin C. Harding)
6cf90132bc io: Add traits (Tobin C. Harding)
Pull request description:
So that our `io` crate is not surprising it seems we should generally, unless there is a good reason not to, follow `std::io`.
Copy the derived trait implementations from `std::io` for `Cursor`, and `Sink`. `Take` is correct already, just `Debug`.
Done while investigating C-COMMON-TRAITS
ACKs for top commit:
apoelstra:
ACK 426f585a479ca20b7c3390c589b836f8726b9b03; successfully ran local tests
Tree-SHA512: 0fdacfa0295c36e9ee2bdffd5e649b923f48eeff7baa3afc99fda0836ae30b794610a176c0e76341a268c712baab51139355976c23913a0744692cd1e38a4d11
e315aaf1c6 io: Allow setting position bigger than inner buffer (Tobin C. Harding)
Pull request description:
Currently if one calls `set_position` on a cursor setting the position greater than the total length of the inner buffer future calls to `Read` will panic. Bad `Cursor` - no biscuit.
Mirror the stdlib `Cursor` and allow setting the position to any value. Make reads past the end of the buffer return `Ok(0)`.
ACKs for top commit:
apoelstra:
ACK e315aaf1c65225b89435d0a60907a3233caa5db1; successfully ran local tests
Tree-SHA512: 71b151218e96343a86c8b2c21c6e8212e2d1c2aea6517b4662da3ad01b3b598cec497028b863a8e36a6b1fae1d4f7f975c8610c3b6f39f1c256adc5751d06ea0
96f427a8b8 units: Refactor Send/Sync api test (Tobin C. Harding)
Pull request description:
The `api` test for types implementing `Send` and `Sync` is part of both C-SEND-SYNC and also C-GOOD-ERR (for error types).
Refactor the two tests into a single one and document appropriately. This is mirrors how we do it in `io/tests/api.rs`.
ACKs for top commit:
apoelstra:
ACK 96f427a8b84129179e86f3914be8e4712a89f660; successfully ran local tests; lgtm
Tree-SHA512: 7b24780ac2b4f73d0cad952555f005553d9b8c248da6f92c28e7e9510b58eba6c165720ded9bd2f2db19f9a19d72fe7dd333e68312f1291a47e044a94902be0b
So that our `io` crate is not surprising it seems we should generally,
unless there is a good reason not to, follow `std::io`.
Copy the derived trait implementations from `std::io` for `Cursor`,
`Sink`, and `Take`.
Done while investigating C-COMMON-TRAITS
Currently if one calls `set_position` on a cursor setting the position
greater than the total length of the inner buffer future calls to `Read`
will panic. Bad `Cursor` - no biscuit.
Mirror the stdlib `Cursor` and allow setting the position to any value.
Make reads past the end of the buffer return `Ok(0)`.
The `api` test for types implementing `Send` and `Sync` is part of both
C-SEND-SYNC and also C-GOOD-ERR (for error types).
Refactor the Send/Sync tests in both `units` and `io` and improve
comments.
Currently in the `bridge` module to get a reference and mutable
reference we provide the `as_inner` and `inner_mut` functions. These
names are not in line with the `std::io` module which favours `get_ref`
and `get_mut`.
Add inherent functions `get_ref` and `get_mut` for accessing the wrapped
value in `bridge::ToStd` and deprecate `as_inner` and `inner_mut`.
To convince yourself this API is correct grep the stdlib `io` module for
`fn get_ref` as opposed to `fn as_ref`.
Close: #3832
Our `Cursor` is a replacement of sorts for the `std::io::Cursor`. Users
of the `Cursor` are likely to expect the same API so to get a reference
and/or mutable reference users will likely reach for `get_ref` and
`get_mut`. We should provide these.
Deprecate `inner` since it is the same as `get_ref` but less
idiomatically named (should have been `as_inner`).
Add `get_ref` and `get_mut` methods to the `Cursor` type.
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
This has been fixed and we use nightly to lint so we have access to the
merged fix.
Removing the attribute uncovers a bunch of real lint warnings, fix
them while we are at it.
In preparation for releasing `io v0.2.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 `io`.
18110a51f2 Bump version of internals to 0.4.0 (Tobin C. Harding)
Pull request description:
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`.
ACKs for top commit:
apoelstra:
ACK 18110a51f2 successfully ran local tests; lots of nice stuff here
Tree-SHA512: a4d3d5279b7d7fa993cbc3b7b34fc6dc4024dd54c0bfa1ecd0f0d5f09b984871f156c3695092a1f6c44b7571f8b2051699040f5f77636d44d4cae6c972ab597f
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
Throughout all of the crates except internals (another commit) unused
variables have been prefixed with `_`, unused imports have been removed,
and a warn attribute added to all of the `lib.rs` files.
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`.
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.)
4ead0adcb5 Add blanket impl of io traits for `&mut T` (Martin Habovstiak)
Pull request description:
The impl wasn't previously available because we thought we can do a blanket impl for `std` traits. We've removed the `std` blanket impl when we realized it's broken but forgot to add the `&mut T` impls. This adds them.
Note: this is in part an experiment to see if this is API breaking. I suspect it might be.
ACKs for top commit:
apoelstra:
ACK 4ead0adcb5 successfully ran local tests
tcharding:
ACK 4ead0adcb5
Tree-SHA512: 1e4411fdf019f3793e6366eda7e8b46246e3263bd7e41802877c20cc5e8ea451a79d89b4c59204ea9c1eb590054975de52a791a9d17a1d180a5cfac316efa959
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
The impl wasn't previously available because we thought we can do a
blanket impl for `std` traits. We've removed the `std` blanket impl when
we realized it's broken but forgot to add the `&mut T` impls. This adds
them.
Jamil Lambert, PhD
merge-script
Martin Habovstiak
Eval EXEC
RiceChuan
Tobin C. Harding
Fmt Bot