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
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
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.
12be5c0d27 clippy: fix a couple of nits in `clippy --no-default-features` (Andrew Poelstra)
7d07fc3fb9 ci: update the new nightly_version file in cron (Andrew Poelstra)
Pull request description:
Fixes an issue with #2658 which accidentally disabled our nightly rustc update cronjob, and also fixes a small clippy lint.
After this, we should be able to get back onto the latest nightly :).
ACKs for top commit:
tcharding:
ACK 12be5c0d27
Tree-SHA512: 771fb2c278b0aaea04454a3c397d4ca1c201ff59891d4f7af69a295aa4c6475320383fa5f4dd2a20323e79d5fe68b2ff37c3f01717a12f98afb9db3c2939c95a
We only check clippy in CI with --all-features, which usually is the
best way to get maximum coverage. But if you try a couple other feature
combos, especially those related to nostd, you can hit more code.
051c358bcb Remove deprecated legacy numeric methods (Divyansh Gupta)
Pull request description:
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replaced `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
ACKs for top commit:
tcharding:
ACK 051c358bcb
apoelstra:
ACK 051c358bcb thanks! I will remove an equivalent commit from my #2669
Tree-SHA512: c08c856f7f3b281417c29283351eac5e0f75cc1c8d23d9aae58d969219a327b2337fe57932053e53773ebb9dbec04254f90149266b6639a66c5c09f2ad1675ef
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replace `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
0073a17e20 bitcoin: Bump version to 0.32.0-rc1 (Tobin C. Harding)
Pull request description:
This PR is just the final patch, primarily the changelog. Pretty happy with my effort, check it out: https://github.com/tcharding/rust-bitcoin/blob/04-04-release-bitcoin-rc1/bitcoin/CHANGELOG.md
ACKs for top commit:
sanket1729:
utACK 0073a17e20.
apoelstra:
ACK 0073a17e20
Tree-SHA512: 9d2815c8739286350373eac3cd03d703082fb41b67cc83dcd694f41f2b97cc7d07b8942b62876552f490fddb588e30c346246ff0114b3b3adafb3482f76db242
In preparation for dropping the first release candidate bump the version
and add a changelog.
Please not I went to much more effort that usual with the changelog,
open to review on the overall form - not promising I'll change it but
definitely would like to keep iterating and improving.
If this changelog is appreciated then FWIW I don't think we should
bother automating it, a machine does not have all the context required
to create it.
e06ebd69e7 units: Bump version number to 0.1.1 (Tobin C. Harding)
a2b019f823 Enable internals "alloc" feature (Tobin C. Harding)
Pull request description:
Fix a minor internal bug in error code in `units` and bump the version number so we can do a point release.
This can go in after the RC drops as part of the release candidate cycle if its easier - as long as its in and released before the finale `v0.32.0` release.
ACKs for top commit:
apoelstra:
ACK e06ebd69e7
sanket1729:
ACK e06ebd69e7
Tree-SHA512: b6523fae57ba3becf27c0a11fe0b1d75db9226d01bde527390e2cf1697520d5daabc5ef3909b2c464b14f38ba4f8ab87aa49d17a1d054767920963ef1c3ef3b9
Christian Reitter
Andrew Poelstra
Update Nightly Rustc Bot
Tobin C. Harding
Divyansh Gupta