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
14040e2ff5 psbt: Return the internal key for key path spend (Tobin C. Harding)
ffd5664c08 Do not panic if input_index is out of bounds (Tobin C. Harding)
f79f20d4e6 Remove stale rustdoc (Tobin C. Harding)
Pull request description:
When signing a Taproot input (in a PSBT) using a key path spend we currently return the pubkey associated with key that signs. However it is common to think of the internal key as being the one that signs even though this is not technically true. We also have the internal key in the PSBT so matching against it is less surprising.
When using the `Psbt` type to sign a Taproot input using a key path spend return the internal key.
- Patch 1: Fix stale docs
- Patch 2: Remove unnecessary panic
- Patch 3: Change the key returned when signing Taproot input as key path spend
Done as part of #2557, this is the release blocking part.
ACKs for top commit:
sanket1729:
ACK 14040e2ff5
apoelstra:
ACK 14040e2ff5
Tree-SHA512: 0b38b9009fd5dab682461dcb79f46c7540ec79cfd9c856fc5036ad8ecda061781c58b6fd157db9f034ab07dda6fa747417318f613092ee2017b8f2f44898dcd7
398fc6b73a Update pre-commit hook to use pinned nightly (Tobin C. Harding)
1a85eac01b Move nightly_version file to crate root (Tobin C. Harding)
Pull request description:
Move the nightly pinning config file out of `.github/` and use it in the pre-commit hook.
ACKs for top commit:
sanket1729:
ACK 398fc6b73a
apoelstra:
ACK 398fc6b73a
Tree-SHA512: 9b30c7064a43b068399927518732c3f4fe033693a7ad09af33f49b18a31c8965e6d4bb887ee558dd2a8d50b165545eb3de3fa7c415bff27efee5ac97385d27ff
The nightly pinning is used by a bunch of different tools outside of
github actions, move the config file to the crate root.
Update the path in the justfile.
Done in preparation for fixing the git pre-commit hook.
We just did a minor bug fix to error code in the `amounts` module. This
change did not effect the public API but improved the display of two
error types from that module.
In preparation for doing a point release bump the version number and add
a changelog entry.
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.
When signing a Taproot input (in a PSBT) using a key path spend we
currently return the pubkey associated with key that signs. However it
is common to think of the internal key as being the one that signs even
though this is not technically true. We also have the internal key in
the PSBT so matching against it is less surprising.
When using the `Psbt` type to sign a Taproot input using a key path
spend return the internal key.
There is no need to panic if input index is out of bounds because we
have a function to check the validity of the `input_index` argument and
use it in other places already.
Andrew Poelstra
Tobin C. Harding
Update Nightly Rustc Bot
Divyansh Gupta