Recently we discovered a bug in the `run_task` script where we forgot to
use `--no-default-features` in a subset of test runs (the feature matrix
stuff).
Upgrade to use the latest version of `run_task.sh`.
Currently we run the job at midnight here and in `sepc256k1`, this led
recently to one using the nightly toolchain from the 10th of Sep and the
other using the toolchain from 11th of Sep.
Update to run at 5 past so this doesn't happen again.
bd8ad1f5e2 Add basic `miri` checks (Martin Habovstiak)
fb5971cc2b Fix UB in `siphash24` (Martin Habovstiak)
Pull request description:
We have a bit of `unsafe` code in the crates which should really be checked with `miri`. Thus this adds a basic CI check that automatically determines which crates need `miri` checking and checks them. It also makes sure to enable all target features so that SIMD code can be checked as well.
This doesn't try to do anything fancy with maintainer tools or run task for now, since I just want to test the basic idea.
Closes#3192
ACKs for top commit:
storopoli:
ACK bd8ad1f5e2
tcharding:
ACK bd8ad1f5e2
sanket1729:
ACK bd8ad1f5e2
apoelstra:
ACK bd8ad1f5e2 successfully ran local tests; wow, good find!
Tree-SHA512: a0d33c7851d6d6b288ca8cc1a902f187814dd82e3528c6f8169fdc0ba71991b99451276aaba5e3b6cde6029e09158063d65e48a71d1e01ee20302b9f653584ef
We have a bit of `unsafe` code in the crates which should really be
checked with `miri`. Thus this adds a basic CI check that automatically
determines which crates need `miri` checking and checks them. It also
makes sure to enable all target features so that SIMD code can be
checked as well.
Instead of manually setting the crates list it is less error prone to do
so mechanically. This required some changes to the `run_task` script
which have now merged, so we update to use the new commit hash at the
same time.
- Use shell to set the `CRATES` env var used by `run_task.sh`.
- Use latest revision of rust-bitcoin-maintainer-tools
83831e6363 Show output of failed semver checks (Martin Habovstiak)
Pull request description:
Debugging the API breaks without seeing what failed is pretty much impossible. This simply prints the output when a check fails.
ACKs for top commit:
tcharding:
ACK 83831e6363
apoelstra:
ACK 83831e6363 successfully ran local tests
Tree-SHA512: d3acd41ef745e8e19c0be8ca9c205f038a72d6d66c459564f6ea35cdef9402c07026e497e6ff6c8e1b80b4856fedfbf1d6f74adb40789b30bffd6e2a1ee76a2b
Apparently the `checkout` action will just grab random shit in the case
that you configure it with an unknown key. There is a warning in the
middle of the CI output so ok, I guess there's that.
This commit does not change the pinned version of
rust-bitcoin-maintainer-tools, though we do want to update the pin,
because I want to make sure that the pin is actually working.
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
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.
So that updates don't break our CI setup used a specific version of
`cargo-public-api`, this means we will have to update it manually
periodically though.
For now, since the latest release just broke us, use the release from a
month ago.
We have a mechanism to run additional custom tests by way of the
`extra_tests.sh` script in each crate.
Remove the CI job and run the schemars test using `extra_tests.sh`. This
patch changes the test coverage because currently the schemars test is
only run with a stable toolchain but with this patch applied it runs
with stable, MSRV, and nightly.
Fix: #2787
We were using an outdated CBOR crate for MSRV reasons. But this old
crate is causing suprious test failures. So delete it. (Sadly, updating
the crate doesn't fix the issue, replacing it with ciborium breaks our
MSRV tests because it needs a more recent `half` dependency, and
replacing it with `minicbor` doesn't work because minicbor is not based
on serde. So we don't really have any options.)
In general, I am suspicious of this decode-then-reencode test. CBOR has
some ambiguity in integer encoding. Empirically it has seemed to
work for a long time, but this seems more like an indictment of our test
than a positive result.
Also, round-trip testing serde encoding of a byte vector is probably not
a great use of our fuzz resources. I don't believe we have ever had a
problem with this.
Fixes#2801
6def5bc974 CI: Use run_task from maintainer tools (Tobin C. Harding)
c5af52847b CI: Add docs and document start (Tobin C. Harding)
62ba10503a CI: Use correct spacing (Tobin C. Harding)
0c0e88165e CI: Add README file (Tobin C. Harding)
44cb2255d3 CI: Add sanitizer script (Tobin C. Harding)
3407257936 CI: Add WASM script (Tobin C. Harding)
cc14edf63f CI: Run the schemars job directly using cargo (Tobin C. Harding)
1fb12e1917 CI: Remove recent from schemars job (Tobin C. Harding)
8d7117bb0e CI: Use original name (Tobin C. Harding)
Pull request description:
First we do some clean up, then we pull the stuff that is specific to this repo out into separate tests, then as the last patch we switch over to use the new script for `rust-bitcoin-maintainer-tools` that was introduced in https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/pull/4.
ACKs for top commit:
apoelstra:
ACK 6def5bc974
Tree-SHA512: 70105d455294d49deb43461958435096d074d19ea8d9adc7036e15d74dfdab466f04b1d9e934574077ca0c32a6fe77d0e5aa11068d8c4d51acef634591227d33
Add a document start and comment to help try to stop the readme from
going stale.
This removes a `yamllint` warning.
While we are clearing lint warnings disable the one causing
warning truthy value should be one of [false, true] (truthy)
This is a know issue with GitHub actions yaml because `on` is a yaml
keyword, or something like that.
As we did for the wasm job.
In preparation for using the `run_task` script from maintainer tools we
want to have all the things that are particular to `rust-bitcoin` out of
the current `run_task` script.
The address/memory sanitizer test is specific to `hashes`. Add a script
in `hashes/contrib` and call it from the ASAN job.
No test coverage change.
In preparation for using the `run_task` script from maintainer tools we
want to have all the things that are particular to `rust-bitcoin` out of
the current `run_task` script.
The wasm test is specific to `hashes`. Add a script in `hashes/contrib`
and call it from the wasm job.
No test coverage change.
I've been a bit confused lately about pinning and the locks, at some
stage recently I changed 'Set dependencies' to 'Copy lock file'. Its no
biggy but the original that Kix wrote was correct and descriptive - it
was me who was confused. Revert it back to the original.
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.
The `labeler` job has an input `sync-labels` (default `false`) that
configures:
> Whether or not to remove labels when matching files are reverted or no
> longer changed by the PR
ref: https://github.com/actions/labeler
Currently we are using the default which means labels are not synced
when a PR is modified.
Example
A PR initially touches the `hashes` and `bitcoin` crates and the labeler
action adds `C-hashes` and `C-bitcoin`. Later, a force push removes the
changes to `hashes`, the labeler will not remove the `C-hashes` label.
Now the labels are incorrect.
Note on `pull_request_target`:
We use `on: pull_request_target`, this means the action is run in the
target branch not in the PR branch. So the changes in this patch are not
run when this PR is pushed up. Therefore we cannot test this patch but
have to merge it blindly - please review carefully.
Some further security notes that I learned while doing this patch. The
`pull_request_target` has access to GitHub secrets, and in
`manage-pr.yaml` we run `./contrib/gen_label_config.sh`. This is safe
because its the version of this script on `master` that is run not the
version in the PR (so a malicious attacker cannot patch
`gen_label_config.sh` and have it be run). However we need to be aware
that we do not accidentally merge some funky shell and inadvertently
leak secrets.
Version `0.49` of `kani` broke our daily CI job, there is now a new
release out that fixes the issue.
Remove the explicit `kani` version so we pull the latest version.
Kani version `0.49.0` came out 10 days ago, its odd that our CI just
broke today but in an attempt to see if its release related pin to the
version before the latest release.
Tobin C. Harding
merge-script
dependabot[bot]
Martin Habovstiak
Update cargo-semver-checks Bot
Jose Storopoli
Jamil Lambert, PhD