875545517d Add clippy exceptions for needless_question_mark lint (Steven Roose)
Pull request description:
This lint forces you to write semantically different code that is in most cases inferior, just to save you 5 characters.
The reason why the code is inferior is because it doesn't do error conversion so it would break when either of the two function signatures changes while in the original code using the `?` operator, nothing would break if the inner error can be converted into the outer error.
ACKs for top commit:
apoelstra:
ACK 875545517d
tcharding:
ACK 875545517d
Tree-SHA512: 8429e0fb7d759a3d19231e7bcaed61b0988172d931e758a9522d7c994854fd403408bb93b06778a5c09746cd38b6a96d3d2e0a862fb4516f2dbfffffe8735ce0
750ee2ba56 Remove unnecessary clippy attribute on is_sighash_single_bug (Steven Roose)
f522a0290c Remove unnecessary clippy attribute on relative::LockTime (Steven Roose)
b7f11d4493 Remove unnecessary clippy attribute on absolute::LockTime (Steven Roose)
Pull request description:
Am I missing something and are they necessary in some other clippy configuration than the one I ran?
It's surprising really that clippy doesn't have a lint to notice unnecessary clippy exception attributes..
ACKs for top commit:
tcharding:
ACK 750ee2ba56
apoelstra:
ACK 750ee2ba56
Tree-SHA512: 0bed6e014d110a531d0a4a0b003778a7ae9c59ac90e8bbcc518321e48141a77fcdfaddf54b752d2733154ad1945609a9a8dcf121d2a813e7a2ee098af26951f7
d391ada5b8 ci: nightly rustfmt PR scheduled/manual (Einherjar)
Pull request description:
## Summary
- Removes nightly `rustfmt` checks from CI and `CONTRIBUTING.md`. Superseds #2127
- Adds automated nightly `rustfmt` PR creations manual and every Sunday 00:00 UTC. Closes#2128. Closes#1712.
## Details
The new `.github/workflows/rustfmt.yml` action does the following:
1. Checkouts the repo
1. Install the nightly Rust toolchain with `rustfmt` component
1. Run `cargo +nightly fmt` which format all the codebase with the nightly Rust toolchain writing the changes
1. Add the current date in `YYYY-MM-DD` format as an GH action ENV var
1. If changes are detected, create a PR with the following details:
- Title: Automated nightly rustfmt (`DATE`)
- Body: Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
- Commit message: `DATE` automated rustfmt nightly
- label: `rustfmt`
I did a test run (with `workflow_dispatch` and some minor changes to trigger the PR) in my fork.
Check how the PR looks in https://github.com/realeinherjar/rust-bitcoin/pull/5
Of course, all of these things can be changed.
They are all suggestions...
## Security Disclaimer
We are only introducing a new action, which is quite battle-checked, [`peter-evans/create-pull-request`](https://github.com/peter-evans/create-pull-request) with ~2k stars and proper maintained.
ACKs for top commit:
apoelstra:
ACK d391ada5b8
tcharding:
ACK d391ada5b8
Tree-SHA512: c8728d03467913005fc92fefd4150a8041a495b0ec08b3a3397a05f381a59ef904e8afe6182509fd295ad1b23875b43206a2f1238a253542da54420e4805ab6f
b163d9b59a Replace hex_script macro with a helper function (yancy)
7f26439e20 Add track_caller to test helper functions (yancy)
bf08ee4499 Replace helper macro with helper function (yancy)
Pull request description:
Remove the macro hex_psbt and replace with a function. Using track_caller to accurately report the test name and line number during a panic is used in place of a macro.
This is a refactor commit to the test suit.
ACKs for top commit:
apoelstra:
ACK b163d9b59a
tcharding:
ACK b163d9b59a
Tree-SHA512: 6955c966d1c37020515ae990e5e0ec154f591ce025321dee71264e4e53cbab38afdb681ca193e626efdb7be4b4e84763cd8baf41b2a1258d34c38acd58713254
Remove the macro hex_script and replace with a function. Using
track_caller to accuretly report the test name and line number
during a panic is used in place of a macro.
The test helper files can panic when calling hex_psbt(). hex_psbt()
will report the calling function, instead of the offending test. Adding
track_caller to functions that call hex_psbt will report the line number
of the failing test.
Remove the macro hex_psbt and replace with a function. Using
track_caller to accuretly report the test name and line number
during a panic is used in place of a macro.
38960ab5a5 Bump version to 0.31.0-rc2 (Tobin C. Harding)
79dfe8d270 justfile: Add update-lock-files command (Tobin C. Harding)
178069c13e Add changelog entries for v0.31.0 (Tobin C. Harding)
Pull request description:
Prepare for the v0.31.0-rc2 release.
- Add changlog entries
- Add a `just update-lock-files` command
- Bump the version ready for next RC release
ACKs for top commit:
apoelstra:
ACK 38960ab5a5
clarkmoody:
ACK 38960ab5a5
Tree-SHA512: d2d459666117992689ea2bde2ba4a5d9a9cd36a85c5f0df443ceca3bf9d4ec351baffb4be09fbe25bf0767145d11a59b97031e7c8f9182fa9348a6883420b8fe
If our custom dev scripts get a `just` command it makes them more
discoverable for new contributors.
Add a command to run the `update-lock-files.sh` script.
b108ffa2ec Implement manual fmt::Debug for BlockHeader to include block hash (Steven Roose)
Pull request description:
I think it makes sense for a block header to have the block hash shown in the debug print.
ACKs for top commit:
tcharding:
ACK b108ffa2ec
vincenzopalazzo:
ACK b108ffa2ec
apoelstra:
ACK b108ffa2ec
Tree-SHA512: 37b77f8b2d8da3903bc5ff329abd45dcb5f189ba2ac38fc309fa477b0eb569b01dd85c41f160e073bb56b4a599b7c4774fbfbd8da4b82ddb41540d60bfeafea3
6731e85dc1 bitcoin: Bump version to 0.31.0-rc1 (Tobin C. Harding)
Pull request description:
Lets go!
As discussed in #2099 we are going to do RC releases for this release. This PR is for the first one, as such it does not include changelog entries, we are going to try out using the new github action to create those.
ACKs for top commit:
apoelstra:
ACK 6731e85dc1
Kixunil:
ACK 6731e85dc1
Tree-SHA512: 8c97a6885f8b77e5be85dcaca79428bdb88167681bd6ff2b4fd020b9d695c0e847c37a57ac73d432f22b049e47fefa11c0b3e4ca486ea8af8b93df164542ef6c
38005f6aa7 Use Target for pow_limit (Tobin C. Harding)
Pull request description:
The `Params::pow_limit` field is currently a `Work` type, this is incorrect. The proof of work limit is the highest _target_ not the lowest work (even though these have a relationship).
Note that we use the highest _attainable_ target, this differs from Bitcoin Core and the reasoning is already documented in the code.
Add new consts and document where they came from as well as how they differ to Core.
Use the new consts in the various network specific `Params` types.
Fix: #2106
ACKs for top commit:
junderw:
ACK 38005f6aa7
apoelstra:
ACK 38005f6aa7
Tree-SHA512: 5e71f69cdd555fd95a52fc1479e27b5e11226772f6432758c1364107a068bd1271486da6db1ece694da0287ce10cfbc18d28d6e3dbb0b9d387ff968eea43ab18
678eee8d73 Use Message::from_digest (Tobin C. Harding)
6f30ac9d02 Upgrade the secp dependency (Tobin C. Harding)
818a3c5424 Use Keypair instead of KeyPair in identifiers (Tobin C. Harding)
Pull request description:
Upgrade the `secp256k1` dependency to the newly released `v0.28.0`.
FTR this includes two simple changes:
- Use `Message::from_digest_slice` instead of `Message::from_slice`.
- Use `secp256k1::Keypair` instead of `secp256k1::KeyPair`.
But to stay in line with the keypair change we deprecate and rename the tweaked alias.
ACKs for top commit:
vincenzopalazzo:
ACK 678eee8d73
apoelstra:
ACK 678eee8d73
sanket1729:
ACK 678eee8d73.
Tree-SHA512: 3a86eccc4a638e5e17c4e4a4a310c8812d6c9a0ea19be679234b2390ca586befe755984d1321c22f9adb6d5efd412f4638e99364ec18e82d74ca55ecf00f8587
539f4ba64c apoelstra run of generate-files.sh (Andrew Poelstra)
20f3a05d2a ci: replace fuzz toolchain (Einherjar)
Pull request description:
1. Changes the toolchain fom `actions-rs/toolchain` to `dtolnay/rust-toolchain`.
Closes#2113.
1. Changes the CI runner from `ubuntu-20.04` to `ubuntu-latest`.
Related #2114.
1. Updates the `fuzz/generate-files.sh` to match `.github/workflows/fuzz.yml`.
Closes#2115.
ACKs for top commit:
realeinherjar:
> ACK [539f4ba](539f4ba64c)
apoelstra:
ACK 539f4ba64c
vincenzopalazzo:
ACK 539f4ba64c
tcharding:
ACK 539f4ba64c
Tree-SHA512: 3f1c744022794f1d1dbb7c7dc10d6d5252431ce54ee2309c906183c3c12570ac5e7833bb352749ad9be336b404ae3dd42fcf414107fb7a5bc73043cf5158cb88
The `Params::pow_limit` field is currently a `Work` type, this is
incorrect. The proof of work limit is the highest _target_ not the
lowest work (even though these have a relationship).
Note that we use the highest _attainable_ target, this differs from
Bitcoin Core and the reasoning is already documented in the code.
Add new consts and document where they came from as well as how they
differ to Core.
Use the new consts in the various network specific `Params` types.
We have a new API function available with recent version of `secp256k1`
to create a `Message` directly from a sighash byte array.
Use `Message::from_digest(sighash.to_byte_array())` to construct
messages ready to sign.
old: `actions-rs/toolchain`
new: `dtolnay/rust-toolchain`
fix
ci(fuzz): change runner to ubuntu-latest
ci: update run syntax in fuzz job
ci: update and run fuzz/generate-files.sh
Upgrade the `secp256k1` dependency to the newly released `v0.28.0`.
FTR this includes two simple changes:
- Use `Message::from_digest_slice` instead of `Message::from_slice`.
- Use `secp256k1::Keypair` instead of `secp256k1::KeyPair`.
In preparation for updating the secp dependency to v0.28.0, which
includes a change of `KeyPair` to `Keypair`, change our identifier usage
to indicate that "keypair" is a single word.
Deprecate the old forms.
33ee49f8e5 Update bech32 dependency (Tobin C. Harding)
Pull request description:
Update the `bech32` dependency to use the newly release beta version.
The main fix here is silent, a bug fix in `bech32` that was being hit by our fuzzing suite.
ACKs for top commit:
vincenzopalazzo:
ACK 33ee49f8e5
clarkmoody:
ACK 33ee49f8e5
realeinherjar:
tACK 33ee49f8e5
apoelstra:
ACK 33ee49f8e5
Tree-SHA512: 1464d00ddc16bb4a3c72e2c6b667c8b115b6fc883cf5b0da03378b9a1ad07b49f5b43e1d8ece56e2a0c6d795d50ea3d809e9245aea69bde4b44d247041c7dcc5
Update the `bech32` dependency to use the newly release beta version.
The main fix here is silent, a bug fix in `bech32` that was being hit by
our fuzzing suite.
3e6021bbc6 ci: fuzz test YAML toolchain future-proof (Einherjar)
Pull request description:
To avoid YAML automatic parsing of inputs as floats, e.g. [`1.60` being parsed as `1.6`](https://github.com/rust-bitcoin/rust-bitcoin/pull/2109#issuecomment-1751392327), it is best to future-proof the `toolchain` input in CI fuzz testing as a string.
ACKs for top commit:
vincenzopalazzo:
ACK 3e6021bbc6
apoelstra:
ACK 3e6021bbc6
tcharding:
ACK 3e6021bbc6
Tree-SHA512: 838afb401fcfbb00717d6d3efff013cda08dabe4962ee0e914c3cf438a5cc8e83fa336dbd920b299fe95f935c0c1d76cf15d3f6fc6f763ea15a1746e7254d94f
6b5d06f23e ci: fix the byteorder to 1.4.3 for edition 2018 (Vincenzo Palazzo)
98513ef151 clippy: more worning fixes (Vincenzo Palazzo)
05d3dc5d72 Remove redundant guard (Tobin C. Harding)
4537634e7e ci: bump rustc to 1.60 for fuzz test (Vincenzo Palazzo)
Pull request description:
Ci looks like broken, so this should fix
it
ACKs for top commit:
apoelstra:
ACK 6b5d06f23e
Tree-SHA512: bfa0eaf8cbc02a671237d99221db8c21264ce9df91301818c95c41dcc5ad4935e91254b0b3fa8f36738a9d71b6541fb8784ac8280d67057960a3d20e385a9f17
8eff4d0385 Remove private hex test macro (Tobin C. Harding)
Pull request description:
We have this macro in `hex-conservative` now, remove the version here.
This patch does not change the public API and only touches test code.
ACKs for top commit:
apoelstra:
ACK 8eff4d0385
clarkmoody:
ACK 8eff4d0385
Tree-SHA512: 93a08fff778930071cd1a28c19202e4a94ca8881b2e873538de2e942b71c2cd6184ed6364c572538a8a699295a71761c6f836accaf251a15683138b71f148fab
685e5101ce Add wasm dev-deps using CI script (Tobin C. Harding)
Pull request description:
We only test WASM in CI using a stable toolchain however because we have a target specific dev-dependencies section the wasm deps get pulled in during MSRV builds - this breaks the MSRV build.
Instead of including WASM dev-dependencies in the manifest we can dynamically modify the manifest when running the WASM tests. We do this already to add the `crate-type` section so this is not really that surprising to see in the CI script.
Doing so allows us to stop pinning the transitive `syn` dependency also which is included in the dependency graph because of `wasm-bingen-test`.
Originally part of #2093
ACKs for top commit:
apoelstra:
ACK 685e5101ce
sanket1729:
ACK 685e5101ce. Removing wasm from lock-files is a win
Tree-SHA512: ad607a8a8af329e9830d9284a95a2540bf15a2ceda826b8d1c0312ee094f1f5f54599ee2faa6b611439997c349bdd9a29e6d5ae447bff96a9a538753434e2248
10374af75c Make error types uniform (Tobin C. Harding)
43d3306822 Use explicit error::Error impl instead of the default (Tobin C. Harding)
2512dbafc2 Remove impl_std_error macro (Tobin C. Harding)
6933ca4fc2 Add suffix to HiddenNodes error type (Tobin C. Harding)
2b40ea24fb Add suffix to IncompleteBuilder error type (Tobin C. Harding)
f41416a0ea Add suffix to UnknownMagic error type (Tobin C. Harding)
5658dac024 Add suffix to UnknownChainHash error type (Tobin C. Harding)
2fb71dd943 Move p2p error types to bottom of file (Tobin C. Harding)
39314ad52f Move error code to match conventional layout (Tobin C. Harding)
Pull request description:
PR aims to achieve two things:
- Make error code brain dead easy to read
- Get error code closer to being ready for v1.0
The first 8 patches are pretty basic, and are broken up into really small changes. The last patch is much bigger, it has a long git log to explain it but reviewing should not take too much brain power.
This PR does not introduce anything new, it just applies what we have been doing recently with errors. Before v1.0.0 others will likely want to re go over all the error types. As such I believe this PR can be merged under the one ack carve-out.
### TODOs (future PRs)
We have a few errors that still need splitting up:
- Split up `merkle_tree::block::MerkleBlockError`
- Split up `psbt::error::Error`
- Split up `IncompleteBuilderError`
Also, all error From's should probably have `#[inline]`, I noticed late in the process and did not have the heart to visit every error again.
ACKs for top commit:
apoelstra:
ACK 10374af75c
clarkmoody:
ACK 10374af75c
Tree-SHA512: 4f4f3533f42dc11af8e7978f3272752bb56d12a68199752ed4af0c02a46a87892b55c695b7007bc3d0bdf389493068d068e2be1780e8c3008815efec3a02eedf
be8aee6968 Remove stale link (Tobin C. Harding)
7d54cd3485 Fix typo in docs (Tobin C. Harding)
Pull request description:
Do 2 trivial docs fixes, found while investigating #2103
ACKs for top commit:
vincenzopalazzo:
ACK be8aee6968
sanket1729:
ACK be8aee6968
Tree-SHA512: 8ed50ffc30a01424fb9591c3079c4e107b4783371e5fc95df6183b95ba2bca7db243f5e1408618f7b6c024ed31f9498b68a6d12c2994839107d556246c9e25cf
We only test WASM in CI using a stable toolchain however because we have
a target specific dev-dependencies section the wasm deps get pulled in
during MSRV builds - this breaks the MSRV build.
Instead of including WASM dev-dependencies in the manifest we can
dynamically modify the manifest when running the WASM tests. We do this
already to add the `crate-type` section so this is not really that
surprising to see in the CI script.
Doing so allows us to stop pinning the transitive `syn` dependency also
which is included in the dependency graph because of `wasm-bingen-test`.
On our way to v1.0.0 we are defining a standard for our error types,
this includes:
- Uses the following derives (unless not possible, usually because of `io::Error`)
`#[derive(Debug, Clone, PartialEq, Eq)]`
- Has `non_exhaustive` unless we really know we can commit to not adding
anything.
Furthermore, we are trying to make the codebase easy to read. Error code
is write-once-read-many (well it should be) so if we make all the error
code super uniform the users can flick to an error and quickly see what
it includes. In an effort to achieve this I have made up a style and
over recent times have change much of the error code to that new style,
this PR audits _all_ error types in the code base and enforces the
style, specifically:
- Is layed out: definition, [impl block], Display impl, error::Error impl, From impls
- `error::Error` impl matches on enum even if it returns `None` for all variants
- Display/Error impls import enum variants locally
- match uses *self and `ref e`
- error::Error variants that return `Some` come first, `None` after
Re: non_exhaustive
To make dev and review easier I have added `non_exhaustive` to _every_
error type. We can then remove it error by error as we see fit. This is
because it takes a bit of thinking to do and review where as this patch
should not take much brain power to review.
Andrew Poelstra
Steven Roose
Einherjar
yancy
Tobin C. Harding
sanket1729
Vincenzo Palazzo