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
b355740da4 chore: format and standardize all markdowns files (Jose Storopoli)
Pull request description:
according to the github flavor
(https://github.github.com/gfm/)
ACKs for top commit:
apoelstra:
ACK b355740da4 thanks for going through all this!
tcharding:
ACK b355740da4
Tree-SHA512: a9d5671ddc6f922b42189cae11b2a2a877663c909f73f1e8c407a4de7ac93b291e435373b79be2c708f1ecb717b2ede147c0f7730d582a1cb5bee937603005f0
a60ce917c6 Automated update to Github CI to rustc nightly-2024-04-28 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK a60ce917c6
Tree-SHA512: 0a1eda56c4336669aa4fe6127e55424fe1824ac58d69e2b3c3bab4f22cd048bf3b7ee989d0bb2ce2387f881f585a57f53b11d331f7e88bd3b90e228b99a7b139
1c836acf30 bitcoin: Stop slicing hashes (Tobin C. Harding)
Pull request description:
As part of the ongoing effort to improve `hashes`; stop using slicing of hash types and use `as_byte_array()` to get an array reference instead. This gives us more flexability to modify the `hashes` module.
ACKs for top commit:
apoelstra:
ACK 1c836acf30
storopoli:
ACK 1c836acf30
clarkmoody:
ACK 1c836acf30
Tree-SHA512: ca4becfc9bae13127aae09bd1eb5e03efc670fb4e56c2aba1cf0e3cf5f5a762dd8c5cff8ff7c15167902b7440f70202e0884f1e4d3bb651476019c78d2be3408
e34e2fda10 bitcoin: Upgrade base64 dependency (Tobin C. Harding)
Pull request description:
Upgrade to the latest release of `base64`. Version 0.22.0 came out about 2 months ago.
No code changes needed and from the release notes it doesn't look like anything that will effect us too much.
https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md
The `base64` crate is part of our public API so this is a breaking change.
ACKs for top commit:
apoelstra:
ACK e34e2fda10
storopoli:
ACK e34e2fda10
clarkmoody:
ACK e34e2fda10
Tree-SHA512: ad3a9725583e79202b22161a26f6a14770be7f6fb014d6ae4931e01dd73101cf499a193d8e72aba8acc5dc4f13b98b5c21eb5ea6b5587d0ac0a7f7c16c374e7f
ed04b922ec Automated update to Github CI to rustc nightly-2024-04-27 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK ed04b922ec
Tree-SHA512: 04e9b4778f2870ecab7bbe18029fd0bbd7d1c8375999936b134a0c7ccb87dfefef301ceeb92adcc3a564d5c30f13525ae223b426c63d40d6f29ec1308f546143
a03bc2e5b1 CI: Enable sync-labels for labeler (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
storopoli:
ACK a03bc2e5b1
apoelstra:
ACK a03bc2e5b1
Tree-SHA512: 4082e52ed4c97215e76389e4a80bb1fac9a5f3b0c1d6ae555f3766c8cb12f2f5d2127a8aefad355117dd2ec3f5a9561cb08c6b3d5d2a16efaff2b966fb35965f
f8e6a2f44a Automated update to Github CI to rustc nightly-2024-04-25 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK f8e6a2f44a
Tree-SHA512: 4df63fa34884275cad1e194024f0b5f04dc9ddae0fbf4b075f822c5007516f11bcb935a8eb0a9bad33aa7a65d2fd787b0b4cadd594e2acf729f36c1ee3cc3017
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.
47ac342056 docs: Make rustdoc imports more terse (Tobin C. Harding)
Pull request description:
Make the rustdoc imports in `hashes/src/lib.rs` more terse and also use as-underscore.
ACKs for top commit:
apoelstra:
ACK 47ac342056 nice :)
Tree-SHA512: f684adba555495a08875663c7b78dde756d9213fdf775ea5d005d7a1a7cd49f3e93f60f0666ba96bc08b9f954dc069a72f3c95af68aec64964ba47094e6aeec4
As part of the ongoing effort to improve `hashes`; stop using slicing of
hash types and use `as_byte_array()` to get an array reference instead.
This gives us more flexability to modify the `hashes` module.
f162e13afb CI: Run kani daily job with latest version (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK f162e13afb ci only, can one ack merge
Tree-SHA512: cb6d42eeb6ce640e4696935ae09b438a62e79fcb7570922b88ed830fd7200958d4dfff098092cb294b1b54022d2054c74914e32e1a80f06c34bd508b7199b0c5
dc8b900dec Document the *_encode_signing_data_to functions (Tobin C. Harding)
Pull request description:
If one writes signing data using one of the two
`*_encode_signing_data_to` functions then creating the message to sign is slightly nuanced and different for each of the functions. For Taproot one must use a specific tagged hash and for ECDSA one must use a sha256d hash.
Add documentation that explains the hashing requirements for each function.
Close: #2703
ACKs for top commit:
apoelstra:
ACK dc8b900dec we maybe should say specifically which tagged hash, but I think this is fine
Tree-SHA512: bfbabb822858e5bab56c3ed63a3a6541f44343925f304d027c2d1566485356950857c76ce03d4936aacfa8e11a65edaeb0b3d12114859c0c9e14c0e38b6c4b41
If one writes signing data using one of the two
`*_encode_signing_data_to` functions then creating the message to sign
is slightly nuanced and different for each of the functions. For Taproot
one must use a specific tagged hash and for ECDSA one must use a sha256d
hash.
Add documentation that explains the hashing requirements for each
function.
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.
f96bbebdcc Set release version in deprecated attribute (Tobin C. Harding)
Pull request description:
In preparation for release replace "TBD" with the next release version - `v0.32.0`.
ACKs for top commit:
apoelstra:
ACK f96bbebdcc
storopoli:
ACK f96bbebdcc
Tree-SHA512: 7478808322357d853fab2bf25a7d42a972d5ee05ed6f206bfb73748efe1154fb392dc76c3d0e1a50314bcfdac3a55a415f3c6d40dfaaab802ae1c69dd1ad9e76
af7a7e402d Automated update to Github CI to rustc nightly-2024-04-21 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK af7a7e402d
Tree-SHA512: 9e2d9ce6b6edc3c5afb40f436c9bbbd70d22c291d50458624491267f6e624598a17e1999ef5a2ff13d395a53815d7f51bc966e31979f6fe8c53801306f959b45
b49e670772 Add backport section to contributing docs (Tobin C. Harding)
Pull request description:
Add a section on how we do backports and what the merge policy is.
Recently I backported a patch by pushing directly to the `0.32.x` release branch but this is not an optimal process because it leaves no history on github. Instead PR backports in like we do for patching `master` but allow one-ACK merging if the backport is a straight cherry pick of a patch from `master`.
ACKs for top commit:
apoelstra:
ACK b49e670772 looks much better!
storopoli:
ACK b49e670772
sanket1729:
ACK b49e670772
Tree-SHA512: f114cc015cd0c314567dc337c77ba9b021132f5bbd503f58fcac1c86acab3aae89d179af67cde89f2ac48d7391f7cc7fc23b4f7cfa63ad2b89d335a36cbe131b
e47e57c265 Fix kani test (Tobin C. Harding)
Pull request description:
In a kani test we are checking if the result of `to_unsigned()` is an error but setting `is_signed` to `true`, this field represents the signed-ness of the return type of `to_unsigned` (`Amount`) so should be `false`.
Fix kani daily job.
ACKs for top commit:
apoelstra:
ACK e47e57c265
Tree-SHA512: b0b4ace6d66dbbd30df3da1cbc233bfa994e7e003e049e949af4ab03b01c09d0cc29fd40d84a93a7d41e94751da91407e391c80d3de7064e9ac23485273309a6
In a kani test we are checking if the result of `to_unsigned()` is an
error but setting `is_signed` to `true`, this field represents the
signed-ness of the return type of `to_unsigned` (`Amount`) so should be
`false`.
Fix kani daily job.
Add a section on how we do backports and what the merge policy is.
Recently I backported a patch by pushing directly to the `0.32.x`
release branch but this is not an optimal process because it leaves no
history on github. Instead PR backports in like we do for patching
`master` but allow one-ACK merging if the backport is a straight cherry
pick of a patch from `master`.
3a2d86e0c6 Fix example spend amount (Tobin C. Harding)
Pull request description:
In the segwit signing example we are using the incorrect value when creating the signature - we should be using the utxo amount (input amount) not the spend amount (output spend amount).
Close: #2680
ACKs for top commit:
storopoli:
ACK 3a2d86e0c6
apoelstra:
ACK 3a2d86e0c6
sanket1729:
ACK 3a2d86e0c6
Tree-SHA512: dbd25c0adb2b0cd3ddd61893ce0c057edd7d62aad6a380b19b1e10ae1b8f0f22d81c15c3c7e2bb64a1740b1b6a7b096aaadab8040e507b73fef0bc2a1638d827
In the segwit signing example we are using the incorrect value when
creating the signature - we should be using the utxo amount (input
amount) not the spend amount (output spend amount).
Close: #2680
30a09670e8 Add docs for custom signets (Tobin C. Harding)
Pull request description:
We have started using `AsRef<Params>` in a few places as a function parameter. If a user of the library wishes to use these functions they need to create a type that can implement this trait. Because we use `non_exhaustive` on the `Params` struct it is not possible to just construct a `Params` type. This may be surprising for some folk.
Add module level docs to the `consensus::params` module with an example of how to create a type that can be used to describe a custom signet network. Use fields inspired by Mutiny Wallet's described usage.
Close: #2690
ACKs for top commit:
sanket1729:
ACK 30a09670e8.
apoelstra:
ACK 30a09670e8 this is great; would like to see more `const` but for example code no big deal
Tree-SHA512: 50881763aea99641e24871b0eae60650174c48f620742944e7d5617fcf1edff73a20b2a8f043433f6f114ff5f3f4691703fc37b28880c305bb052c2d75d1eeeb
6e84548b1f Allow deprecated Params field (Tobin C. Harding)
Pull request description:
I'm not sure why I haven't see this before during the whole test cycle but while running `cargo kani --only-codegen` we get a bunch of warnings of form:
warning: use of deprecated field `consensus::params::Params::pow_limit`
We deprecated the `pow_limit` field but still set it (obviously) in const structs - just shoosh the warning.
Found while investigating the current kani CI failures.
ACKs for top commit:
sanket1729:
utACK 6e84548b1f
Tree-SHA512: b3fe9108e6098bdca824785caf1b5ce5f89c415e014c6380302f3de16421083fd9b0f01f1559466e2a3a57da93cd7e14a2c3e59ef5c8fdd45762ebeeeeffeea0
We have started using `AsRef<Params>` in a few places as a function
parameter. If a user of the library wishes to use these functions they
need to create a type that can implement this trait. Because we use
`non_exhaustive` on the `Params` struct it is not possible to just
construct a `Params` type. This may be surprising for some folk.
Add module level docs to the `consensus::params` module with an example
of how to create a type that can be used to describe a custom signet
network. Use fields inspired by Mutiny Wallet's described usage.
Close: #2690
I'm not sure why I haven't see this before during the whole test cycle
but while running `cargo kani --only-codegen` we get a bunch of warnings
of form:
warning: use of deprecated field `consensus::params::Params::pow_limit`
We deprecated the `pow_limit` field but still set it (obviously) in
const structs - just shoosh the warning.
0f0bd91929 kani: Pin version to 0.48.0 (Tobin C. Harding)
5981b15902 kani: Rename tests (Tobin C. Harding)
17bacc6fb6 kani: Remove redundant import (Tobin C. Harding)
Pull request description:
Fix the current failing Kani job on todays PRs.
FTR our daily `kani` job has been red for ages, perhaps since we created the `units` crate? But today the `--codege-only` job broke (on all PRs). `kani` released a new version 10 days ago, pinning to the previous version seems to resolve the issue. I raised a bug report but did not investigate further.
- Patch 1: Remove redundant import
- Patch 2: Rename the tests
- Patch 3: Pin kani version
File bug report: https://github.com/model-checking/kani/issues/3142
PR is CI only, can go in with one ack.
(Patch 2 is the result of debugging the _real_ kani failure we have at the moment, I'll save the rant for the PR that fixes it.)
ACKs for top commit:
apoelstra:
ACK 0f0bd91929 though I wonder if we shoud comment out the test in rust.yml and file an issue
sanket1729:
utACK 0f0bd91929
Tree-SHA512: 1e510dd53f3474dd4891792e312444cec57239c865e4cd7d144932713b3ce2e66806a37b88d55ecaa514292ac936de569cc9126c773048a5a930c6c822faad29
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.
The tests currently include the word "add" but they test addition as
well as subtraction. Elect to keep the multiple assertions per test and
just make the names more general.
`cargo` is reporting a redundant import warning:
warning: the item `TryInto` is imported redundantly
Remove the import statement from the `verification` module.
830c1e9cfe Allow m prefix in derivation paths (Tobin C. Harding)
Pull request description:
Recently in #2451 we disallowed bip32 derivation paths with the leading 'm' variable.
There is some confusion as to what exactly the bip specifies however Bitcoin Core RPC call `getaddressinfo` returns a derivation path with a leading "m/". This means we need to be able to parse it irrespective of what the bip says.
Be more liberal in what we accept as a derivation path, including both with and without the leading 'm/'.
Leave the full investigation of the bip to a later date.
Change back some of the test strings as makes sense and include test strings to showcase the full current behaviour.
This PR replaces #2674.
ACKs for top commit:
apoelstra:
ACK 830c1e9cfe
sanket1729:
ACK 830c1e9cfe
junderw:
ACK 830c1e9cfe
Tree-SHA512: 7a4fccd49cb8cd91a6c8db51d758ae116d9d2e98fead7b87520ca302022b37ddbcf3f85453941c5f336f8e934ad224beba99527dc29ce8368fbb1f25508c1615
33ebbac4c8 Improve deprecation notice (Tobin C. Harding)
Pull request description:
The deprecation notice for `is_provably_unspendable` contains "is not very useful" which is a bit presumptuous to tell to users, it may very well be useful to them. Use the more helpful text that already exists in rustdoc on the function.
Function was deprecated in #2294
ACKs for top commit:
apoelstra:
ACK 33ebbac4c8 I am fine with the existing text
sanket1729:
utACK 33ebbac4c8
Tree-SHA512: 0611b9c414a5b01d453145d645dbfe6ef0be4000a7133bc79211c0494741638fcc19f97d6503546813f426cbc492816daf708045a76e7d67e5bd975ad8f56d5f
Recently in #2451 we disallowed bip32 derivation paths with the leading
'm' variable.
There is some confusion as to what exactly the bip specifies however
Bitcoin Core RPC call `getaddressinfo` returns a derivation path with a
leading "m/". This means we need to be able to parse it irrespective of
what the bip says.
Be more liberal in what we accept as a derivation path, including both
with and without the leading 'm/'.
Leave the full investigation of the bip to a later date.
Change back some of the test strings as makes sense and include test
strings to showcase the full current behaviour.
00572ae24b Automated update to Github CI to rustc nightly-2024-04-09 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 00572ae24b
Tree-SHA512: 1a76e4777f760607507eee4769e0e30c5b681fb4b2c75ac0581753c2cf3d36e8dc2bb76da14977e2e7ee0f64fd2aad04474b7f56858adbdaa5a81d1b1f8ddf70
The deprecation notice for `is_provably_unspendable` contains "is not
very useful" which is a bit presumptuous to tell to users, it may very
well be useful to them. Use the more helpful text that already exists in
rustdoc on the function.
jamil.lambert
Andrew Poelstra
Update Nightly Rustc Bot
Jose Storopoli
Tobin C. Harding