During review of #2889 it was noted that we don't need to enable the
`derive` feature of `serde` in the `test-serde` feature.
Do not enable `derive` in the `test-serde` feature.
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`).
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
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.
In #2521 I removed the link from `std::error::Error` with the claim that
it broke no-std builds. However there are a ton of other places where we
link to `std::` types.
I have no idea where the breakage was, I assume it existed and I was
sane at the time, CI on this patch will tell us.
Close: #2571
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
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
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
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
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
In a few cases a function header documents the parameters of the following function under the heading"Arguments", this has been changed to "Parameters"
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` for functions
that return arrays
- Add `as_bytes` to return a slice
- Add `to_bytes` to return a vector
- Re-name the other macro to match what it now does
26b9782d8b CI: Re-write run_task.sh (Tobin C. Harding)
Pull request description:
Recently we re-wrote CI to increase VM level parallelism, in hindsite this has proved to be not that great because:
- It resulted in approx 180 jobs
- We are on free tier so only get 20 jobs (VMs) at a time so its slow to run
- The UI is annoying to dig through the long job list to find failures
Have another go at organising the jobs with the main aim of shortening total run time and making it easier to quickly see fails.
Re-write the `run_task.sh` script, notable moving manifest handling to the workflow. Also don't bother testing with beta toolchain.
### Note on review
The diff is hard to read for `rust.yml`, I tried splitting out a bunch of separate patches but it resulted in the same thing (because there are so many identical lines in the yaml file). I suggest just looking at the yaml file and not the diff.
ACKs for top commit:
apoelstra:
ACK 26b9782d8b
sanket1729:
ACK 26b9782d8b.
Tree-SHA512: 1b0a0bab5cf729c5890f7150953499b42aebd3b1c31a1b0d3dfa5b5e78fda11e17a62a2df6b610ab4a950d5709f3af6fff1ae64d9e67379338903497ab77ae0e
Recently we re-wrote CI to increase VM level parallelism, in hindsite
this has proved to be not that great because:
- It resulted in approx 180 jobs
- We are on free tier so only get 20 jobs (VMs) at a time so its slow to run
- The UI is annoying to dig through the long job list to find failures
Have another go at organising the jobs with the main aim of shortening
total run time and making it easier to quickly see fails.
Re-write the `run_task.sh` script, notable moving manifest handling
to the workflow. Also don't bother testing with beta toolchain.
WASM Note
Removes the `cdylib` and `rlib` from the manifest patching during wasm
build - I do not know the following:
- Why this breaks on this PR but not on other PRs
- Why I can't get wasm test to run locally on master but PRs are passing
- What the `cdylib` and `rlib` were meant to be doing
This is the docs from: https://doc.rust-lang.org/reference/linkage.html
* --crate-type=cdylib, #![crate_type = "cdylib"] - A dynamic system
library will be produced. This is used when compiling a dynamic library
to be loaded from another language. This output type will create *.so
files on Linux, *.dylib files on macOS, and *.dll files on Windows.
* --crate-type=rlib, #![crate_type = "rlib"] - A "Rust library" file
will be produced. This is used as an intermediate artifact and can be
thought of as a "static Rust library". These rlib files, unlike
staticlib files, are interpreted by the compiler in future linkage. This
essentially means that rustc will look for metadata in rlib files like
it looks for metadata in dynamic libraries. This form of output is used
to produce statically linked executables as well as staticlib outputs.
This lint triggers when parsing a reference to a large struct as a
generic argument, which is wrong.
Allow it crate wide because [subjectively] this lint never warns for
anything useful.
af6dc1db02 internals: Bump version to 0.3.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changelog and bump the version number.
Please note, the changelog is pretty terse.
ACKs for top commit:
apoelstra:
ACK af6dc1db02
sanket1729:
ACK af6dc1db02
Tree-SHA512: b70d4b9de7de90aba3cbff90dd7f25c5ac801d020dbdfe3e64af4c079347cba726aa783a94fc777e7bf177db8402b54948c2dfd4a766d90c1a7a7a6bdfd36136
e49a3c0bfe Remove link to std from rustdoc (Tobin C. Harding)
Pull request description:
We can't link to `std::error::Error` in rustdoc because it breaks non-std builds. Just use backticks - this is not an optimal solution but I know no other.
I have no clue why this is showing up now.
ACKs for top commit:
apoelstra:
ACK e49a3c0bfe
sanket1729:
ACK e49a3c0bfe
Tree-SHA512: 96c540aec59b1db7ad9b185d4ebf4e431dc2a4c9599e2e241b1948ca47995ffe88bf753bb63b01a35c9f82783231744bf8a2bdb89bfb95fbd9324172c4f9c608
We can't link to `std::error::Error` in rustdoc because it breaks
non-std builds. Just use backticks - this is not an optimal solution but
I know no other.
I have no clue why this is showing up now.
These are not run in CI since #2353 and are likely to go out of date. If
we want a script that users can run locally then we should create a new
script that wraps our current CI.
In an unrelated PR CI run we got the following error:
```
error: you should consider adding a `Default` implementation for `ArrayVec<T, CAP>`
```
I did not bother to dig any deeper since this seems like a reasonable
suggestion by the compiler since we provide a `new` function that takes
zero arguments.
Add a `Default` implementation to `ArrayVec`.
Use `bash` instead of `sh` to run shell scripts.
We would like to support Nix users who do not typically have any shell
other than `sh` at a known path, therefore use `/usr/bin/env bash`.
Make the trait level attributes uniform across all released crates in
the repo. Excludes things that are obviously not needed, eg, bench stuff
if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we
have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an
open PR at the moment to add that along with the required docs.
The `arrayvec::ArrayVec` type is not `Copy` which is not nice and we
would like to have a `Copy` type in our crates. While the PR to add
support to the `arrayvec` crate is not merged we implement our own
simplified version.
This one acts mostly as a dumb storage - it has just a few methods and
traits. The new ones can be added as needed later.
There are various challenges when making code `const`: making it
conditional, awkward copying of slices... This change adds tools that
help dealing with these challenges.
Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on
October 21 2021 Rust version 1.56.1 was released.
Debian stable is currently shipping `rustc 1.63.0`.
Our stated MSRV policy is: In Debian stable and at least 2 years old.
Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat
to bump our MSRV org wide.
Start by bumping the `rust-bitcoin` and `hashes` MSRV to Rust 1.56.1,
includes:
- Update docs.
- Update CI and remove pinning.
- Update the build files and remove now stale cfg attributes rust_v_1_x
for values less than the new MSRV.
- Use new `IntoIterator` for arrays so we no longer need to allocate a
vector to iterate.
Links:
- https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html
- https://blog.rust-lang.org/2021/10/21/Rust-1.56.0.html
- https://packages.debian.org/stable/rust/rustc
Previously, each unique compiler cfg attribute that appeared in the
codebase was hard coded and emitted to stdout at compile time. This
meant keeping the file up to date as different compiler cfg attributes
changed. It's inconsequential to emit a compiler version that's not
used, so this change just emits all possibilities to reduce the
maintenance burden of the build script.
We would like the codebase to be optimized for readability not ease of
development, as such code that is write-once-read-many should not use
macros.
Currently we use the `impl_std_error` macro to implement
`std::error::Error` for struct error types. This makes the code harder
to read at a glance because one has to think what the macro does.
Remove the `impl_std_error` macro and write the code explicitly.
8813a63ec9 internals: Bump version to 0.2.0 (Tobin C. Harding)
Pull request description:
In preparation for release bump the version and add a changelog entry. Includes updating the dependency in `bitcoin` and `hashes`.
ACKs for top commit:
apoelstra:
ACK 8813a63ec9
sanket1729:
utACK 8813a63ec9
Tree-SHA512: a9bd9d4d69cba21329f3f63a9948afe566bb97c8c65f5d46c329a696a814e9eb31372d378de1ecf0f43f0cb42f11d53dc51bc467223b34629e61315d48b39a29
In preparation for release bump the version and add a changelog entry.
Includes updating the dependency in `bitcoin` and `hashes` as well as
the minimal/recent lock files.
This implements basic facilities to conditionally carry string inputs in
parse errors. This includes:
* `InputString` type that may carry the input and format it
* `parse_error_type!` macro creating a special type for parse errors
* `impl_parse` implementing parsing for various types as well as its
`serde`-supporting alternative
6cab7beba3 Deprecate min/max_value methods (Tobin C. Harding)
5fbbd483ea Use MIN/MAX consts instead of min/max_value (Tobin C. Harding)
3885f4d430 Add MIN/MAX consts to amounts (Tobin C. Harding)
Pull request description:
The new MSRV (1.48.0) uses associated consts MAX/MIN instead of functions, we had functions to be compliant with the old MSRV.
~Remove all methods `min_value` and `max_value` including calls to these methods on stdlib types.~
PR is now split into three patches:
- patch 1: Add missing associated consts MIN/MAX as needed
- patch 2: Use consts instead of method calls
- patch 3: Deprecate methods `min_value` and `max_value`
ACKs for top commit:
sanket1729:
ACK 6cab7beba3
apoelstra:
ACK 6cab7beba3
Kixunil:
ACK 6cab7beba3
Tree-SHA512: 60949d1bb971e0dfbab7f573b4447f889b5fa1a5f1c9ac9325a2970fe17a19ccc93418dba57f07bed7e13864b130de48b6b3741d1d80266c6144237dd4565ff7
c4c64c0dc5 Test with minimal dependency versions (Martin Habovstiak)
d5655d503a Bump core2 dependency from 0.3.0 -> 0.3.2 (Tobin C. Harding)
Pull request description:
This is work originally done by Kixunil in #1272, I picked it up to help out. The only changes I made were rebasingg, updating the recent lock file, adding `--locked` to hashes contrib file, and adding a co-developed-by tag for accountability.
It could happen that we unknowingly depend on a new version of a crate without updating `Cargo.toml`. This could cause resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate.
This change commits two lock files: `minimal` and `recent`. `minimal` contains minimal dependency versions, while `recent` contains dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for `internals` crate, removes old `serde` pinning and prints a warning if `recent` is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.)
The documentation is also updated accordingly.
Closes#1230
ACKs for top commit:
apoelstra:
ACK c4c64c0dc5
Kixunil:
ACK c4c64c0dc5
Tree-SHA512: 7d386e96ab747f6a6bafeea828ac65bd8bb11975eaa3408acecac369cd2f235f6e9d4c57202be18a3dc2eeb2a2df532d73e4d35cd1f3fbf092eb6414c55b1524
We currently use the functions `min_value` and `max_value` because the
consts were not available in Rust 1.41.1, however we recently bumped the
MSRV so we can use the consts now.
It could happen that we unknowingly depend on a new version of a crate
without updating `Cargo.toml`. This could cause resolution issues for
downstream users. It's also unclear for outsiders to see which
dependencies we tested the crate with.
This change commits two lock files: `minimal` and `recent`. `minimal`
contains minimal depdendency versions, while `recent` contains
dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for
`internals` crate, removes old `serde` pinning and prints a warning if
`recent` is no longer up to date. (We may have to override it somehow if
any crate breaks MSRV.)
The documentation is also updated accordingly.
Co-developed-by: Tobin C. Harding <me@tobin.cc>
Closes#1230
As we did for the `bitcoin` crate, remove attribution from all files in
the `internals` crate.
While we are at it add an SPDX line to the few files missing it, whether
this license nonsense is even needed is left as an argument for another
day.
Justification:
Currently we have a mishmash of attribution lines accompanying the SPDX
identifier. These lines are basically meaningless because:
- The date is often wrong
- The original author attributed is not the only contributor to a file
- The term "rust bitcoin developers" is basically just noise
Just remove all the attribution lines and be done with it.
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
Enable formatting in CI by doing:
- Add a section to the `test.sh` scripts to run the formatter (guarded by
the env variable `DO_FMT`) for all crates (bitcoin, hashes, internals).
- Add `DO_FMT` to the nightly `Tests` CI job.
Various formatting issues have crept into the codebase because we do not
run the formatter in CI.
In preparation for enabling formatting checks in CI run `cargo +nightly
fmt` to fix current formatting issues. No changes other than those
create by the formatter.
We can check which files are included in the packaged release with
`cargo package --list `.
Add an `exclude` section to each manifest that excludes `tests/` and
`contrib/`. Not all crates have a `tests/` directory yet but they should
so add the exclude anyway to future proof the crates.
The `ToHex` trait was replaced by either simple `Display`/`LowerHex`
where appropriate or `DisplayHex` from `bitcoin_internals` which is
faster.
This change replaces the usages and removes the trait.
3e520f9094 Use hex from internals rather than hashes (Martin Habovstiak)
Pull request description:
`bitcoin-internals` contains a more performant implementation of hex encoding than what `bitcoin_hashes` uses internally. This switches the implementations for formatting trait implementations as a step towards moving over completely.
The public macros are also changed to delegate to inner type which is technically a breaking change but we will break the API anyway and the consuers should only call the macro on the actual hash newtypes where the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this reduces duplicated code generated and compiled. E.g. if you created 10 hash newtypes of SHA256 the formatting implementation would be instantiated 11 times despite being the same.
To do all this some other changes were required to the hex infrastructure. Mainly modifying `put_bytes` to accept iterator (so that `iter().rev()` can be used) and adding a new `DisplayArray` type. The iterator idea was invented by Tobin C. Harding, this commit just adds a bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and initialize a large array even for small arrays wasting performance. Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to return `impl LowerHex + UpperHex`. This makes selecting casing less annoying since the consumer no longer needs to import `Case` without cluttering the API with convenience methods.
ACKs for top commit:
tcharding:
ACK 3e520f9094
apoelstra:
ACK 3e520f9094
Tree-SHA512: 62988cec17550ed35990386e572c0d32dc7107e1c36b7c9099080747e15167e6d66497fb300178afbd22481c0360a6b7a1228fd09402d4ce5d295a8594c02aa6
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to
`impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions
and infallible conversions from arrays
Closes#1336
`bitcoin-internals` contains a more performant implementation of hex
encoding than what `bitcoin_hashes` uses internally. This switches the
implementations for formatting trait implementations as a step towards
moving over completely.
The public macros are also changed to delegate to inner type which is
technically a breaking change but we will break the API anyway and the
consuers should only call the macro on the actual hash newtypes where
the inner types already have the appropriate implementations.
Apart from removing reliance on internal hex from public API this
reduces duplicated code generated and compiled. E.g. if you created 10
hash newtypes of SHA256 the formatting implementation would be
instantiated 11 times despite being the same.
To do all this some other changes were required to the hex
infrastructure. Mainly modifying `put_bytes` to accept iterator (so that
`iter().rev()` can be used) and adding a new `DisplayArray` type. The
iterator idea was invented by Tobin C. Harding, this commit just adds a
bound check and generalizes over `u8` and `&u8` returning iterators.
While it may seem that `DisplayByteSlice` would suffice it'd create and
initialize a large array even for small arrays wasting performance.
Knowing the exact length `DisplayArray` fixes this.
Another part of refactoring is changing from returning `impl Display` to
return `impl LowerHex + UpperHex`. This makes selecting casing less
annoying since the consumer no longer needs to import `Case` without
cluttering the API with convenience methods.
Pnicking on oversized slice is useful to catch errors in code that's
supposed to know the exact sizes but this is undesirable in code that
doesn't. These two methods help with handling the case when `buf.len()`
is not known upfront.
All the types that we define with `impl_array_newtype` are
`Copy` so the correct conversion method to get the underlying byte array
is `to_bytes`. We currently provide `into_bytes` as well as `to_bytes`,
with one of them calling `clone` - this is unnecessary and against
convention.
- Remove `into_bytes` and for `to_bytes` just return the inner field.
- Add a method that causes build to fail if `Copy` is not implemented.
`impl_array_newtype` is an internal macro, move it to a new, ever so
meaningfully named, `macros` module.
Use `#[macro_export]`, no other changes to the macro.
1bf885550e Redesign `hex::BufEncoder` to accept owned arrays (Martin Habovstiak)
Pull request description:
Not being able to create an owned `BufEncoder` prevented returning it from functions which need to allocate the buffer on stack. Such is the case in WIP serde via consensus serialzation.
This change refactors `OutBytes` to be unsized, adds an `AsOutBytes` trait and uses that one instead of `Into` to perform the conversion.
Closes#1270
This is meant as potentially mergeable demonstration. Interestingly, it was easier than I expected.
ACKs for top commit:
tcharding:
Except for the question about the `out_bytes` module, ACK 1bf885550e
apoelstra:
ACK 1bf885550e
Tree-SHA512: 39bc48e54ae0e66b988ba5ad6ea7ecbe03e5f4c71792df0f8e2b03aa2e97e2c0fac1cb03e84ecac12ec6f13649554b57e1000710c34c638d17d9bb575d0ac0a1
Not being able to create an owned `BufEncoder` prevented returning it
from functions which need to allocate the buffer on stack. Such is the
case in WIP serde via consensus serialzation.
This change refactors `OutBytes` to be unsized, adds an `AsOutBytes`
trait and uses that one instead of `Into` to perform the conversion.
This adds a build script that automatically activates the cfg if the
version is sufficient. We don't make API changes here, just improve
debugging by enabling `track_caller`.
There's a `hex` module in `bitcoin_hashes` which is a bit out of place
and not very fast - it passes each *digit* through dynamic dispatch not
only adding overhead but also not allowing `String` to make better
allocations.
This change adds basic support for hex encoding using a stack-allocated
buffer which minimizes the overhead of dynamic dispatch. It also
provides a new `DisplayHex` trait designed to replace `ToHex` found in
`bitcoin_hashes`.
Add a new crate `bitcoin-internals` to be used for internal code needed
by multiple soon-to-be-created crates.
Add the `write_err` macro to `bitcoin-internals`, nothing else.
This patch uses a `path` dependency which means `rust-bitcoin` cannot be
released in its current state, will need to be changed once we release
the `bitcoin-internals` crate on `crates.io`.
Jamil Lambert, PhD
Tobin C. Harding
merge-script
Jose Storopoli
Fmt Bot
Liam Aharon
Martin Habovstiak
josibake
Steven Roose
yancy
Harshil Jani