c92290278e base58: Close all errors (Tobin C. Harding)
Pull request description:
Currently we have a bunch of public errors in the `base58` crate. Only two are returned by public functions `decode()` and `decode_check()` (`Error` and `InvalidCharacterError` respectively).
- Close the two public errors by adding private inner errors.
- Add getters on the public errors to get the error data.
- Make all other errors private.
- Call `impl_from_infallible` for _all_ error types.
Done as part of #3261
ACKs for top commit:
apoelstra:
ACK c92290278eaa4b4f062928c6bc6c2984e1ab7ba6; successfully ran local tests
Tree-SHA512: e2ecef89691b41dfcc8ea3ef02c3719c45c72dbb252c76eadd97d03e32518ac7e00a2623d753de947d607ac3edf785f1ffdef8178a4f17ac3e7fd26ee01031ab
Currently we have a bunch of public errors in the `base58` crate. Only
two are returned by public functions `decode()` and
`decode_check()` (`Error` and `InvalidCharacterError` respectively).
- Close the two public errors by adding private inner errors.
- Add getters on the public errors to get the error data.
- Make all other errors private.
- Call `impl_from_infallible` for _all_ error types.
Done as part of #3261
We use `TBD` in our `deprecated` string and it was discovered that there
is an exception on this string so as not to warn because it is used
internally by the Rust language. However there is a special lint to
enable warnings, lets use it.
Add `#![warn(deprecated_in_future)]` to the coding conventions section
of all crates except `fuzz`.
If folk really want to index into a hash they can us `as_byte_array`
then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this
is because otherwise `secp` won't build since we are breaking an API
that is used in the current release of secp.
Fix: #3115
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`.
Done in an effort to reduce the cognitive load of reading the loop.
The base68 decode and encode algorithm uses a `u32` intentionally for
multiplication and a cast to `u8` intentionally when carrying.
Use `From` where possible and document the cast.
`d58` is the iterator value from `Bytes` (iter returned by
`String::bytes`). As such we can infallibly convert it using `from`.
Internal change only, no external changes.
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.
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
Most base58 strings in Bitcoin are somewhat short. There was previously
an "optimization" putting part of the input on stack which was removed
in #2759 because it actually made the code slower. This appears to be
mostly because of branches caused by using `iter::Chain`.
Manually splitting the iterations into two helped bring the performance
close to what #2759 achieved but that still wasn't worth it. But given
that we know the input length in many cases (it's just a slice) we can
determine whether it'll fit a buffer upfront and then just call
different functions which don't have the branches in loops. To avoid
having two functions this uses generics instead. Further, we increase
the buffer length to 128 and use `ArrayVec` from `internals` which
internally avoids initializing the buffer thanks to `MaybeUninit`
In total this increases performance by around 4% on my machine.
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).
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.
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.
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
021bea89bb ci: shellcheck checks (Jose Storopoli)
Pull request description:
Closes#2739.
I am proposing that we use this GitHub Shellcheck action:
[`ludeeus/action-shellcheck`](https://github.com/ludeeus/action-shellcheck)
since it has most stars (and eyes on it).
I also did all fixes that I could find with
```bash
shellcheck **/*.sh
```
If I've missed any please let me know.
ACKs for top commit:
apoelstra:
ACK 021bea89bb
tcharding:
ACK 021bea89bb
Tree-SHA512: 67e37da9ae3ea0c5551af57b928016a2d9e76761af5558b3057ac47e773189629dd20eea9e659b4323c8568fb48dcdbe9ebd5c730f2c6266fb0db52886c9835f
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.
We have 2 crates that require an allocator, `bitcoin` and `base58ck` -
these crates should enable the "alloc" feature when depending on
`internals`.
For `units` we use the `internals::error::InputString` but do not enable
the "alloc" feature - this is a bug, it means that the parsed string is
being lost from the error types that use `InputString`.
Enable "alloc" for `bitcoin`, `base58ck`, and `units`.
- `bitcoin` and `base56ck` is just for good measure so we don't get
bitten later on.
- `units` is a bug fix and requires a point release.
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
0ca5a43ce5 hashes: Bump version to v0.14.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changlelog entry and bump the version.
Note the hashes 0.13.0 dependency stays in the dependency graph because of secp, we can update secp after releasing `hashes` then update the secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0` dependency - phew.
Note we are right to release this immediately, the two open PRs (#2337 and #2541) that touch `hashes` only add a clippy attribute so can safely be ignored.
ACKs for top commit:
apoelstra:
ACK 0ca5a43ce5
sanket1729:
ACK 0ca5a43ce5
Tree-SHA512: d1d26acb8fbf13f785b25add3f1dac05bb392b5bdbad16ead2bc5dd26f3d668824c4b653c373f88c3562a37e775146766680606cedd19db40e0f197b26ca86b8
In preparation for release add a minimal changelog to the `base58ck`
crate. This crate is currently unreleased and has the version number
correctly set to `v0.1.0` - as of today, the crate name `base58ck` is
available on crates.io.
In preparation for release add a changlelog entry and bump the version.
I'm not 100% sure that this release is API breaking, dependencies
definitely changed. The rest might be only additives but I didn't bother
looking exactly because I think its better to bump the minor version and
err on the side of caution.
Note the hashes 0.13.0 dependency stays in the dependency graph because
of secp, we can update secp after releasing `hashes` then update the
secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0`
dependency - phew.
We are currently using the `base58::Error` type to create errors in
`bitcoin`, these are bitcoin errors not `base58` errors.
Note that we add what looks like duplicate
`InvalidBase58PayloadLengthError` types but they are different because
of the expected length. This could have been a field but I elected not
to do so for two reasons:
1. We will need to do so anyways if we crate smash more
2. The `crypto::key` one can have one of two values 33 or 34.
With this applied we can remove the now unused error variants from
`base58::Error`.
In preparation for improving the `base58` error types crate an `error`
module and move the single current error type there. Make the module
public and reexport the type.
Tobin C. Harding
Fmt Bot
merge-script
Leo Nash
Jamil Lambert, PhD
Chris Hyunhum Cho
Martin Habovstiak
Jose Storopoli
Riccardo Casatta