b29ff9b715 Rename SchnorrSighashType::from_u8 -> from_consensus_u8 (Tobin C. Harding)
af16286679 Implement TryFrom sha256::Hash for TaprootMerkleBranch (Tobin C. Harding)
6b7b440cff Implement TryFrom<Key> for ProprietaryKey (Tobin C. Harding)
5c49fe775f Implement TryFrom<TaprootBuilder> for TapTree (Tobin C. Harding)
632a5db8d9 Implement TryFrom for WitnessVersion (Tobin C. Harding)
Pull request description:
Audit the whole codebase checking for any method that is of the form `from_foo` where foo is not an interesting identifier (like 'consensus' and 'standard'). Implement `TryFrom` for any such methods, deprecating the original.
Done as separate patches so any can be easily dropped if not liked.
ACKs for top commit:
apoelstra:
ACK b29ff9b715
Kixunil:
ACK b29ff9b715
Tree-SHA512: 40f1d96b505891080df1f7a9b3507979b0279a9e0f9d7cd32598bdc16c866785e6b13d5cb1face5ba50e3bc8484a5cd9c7f430d7abc86db9609962476dacd467
9bf959180b Optimize Witness Serialization (DanGould)
Pull request description:
fix#942
> self.to_vec() allocates, it should be possible to avoid it - just feed the items into serializer.
based on https://github.com/rust-bitcoin/rust-bitcoin/pull/1068
ACKs for top commit:
Kixunil:
ACK 9bf959180b
apoelstra:
ACK 9bf959180b
Tree-SHA512: 14553dfed20aee50bb6361d44986b38556cbb3e112e1b4d9e3b401c3da831b6bdf159089966254cf371c225ae929fc78516c96a6114b40a7bc1fda7305295e4a
We allocated a new vector when serializing a `Witness`. That was
inefficient and unnecessary. Use `serialize_seq` to feed the witness
elements directly into the serializer.
Optimize `Witness` serialization by removing the allocation.
When `rust-bitcoin` was started in 2014 the SPDX license list and short
identifiers where not a thing. Now that we have short identifiers and
they are gaining popularity in other projects we can consider using
them.
- Add links to the SPDX website in the readme
- Shorten the author section to a single line
- Remove all the licence information in each file and replace it with an
SPDX ID (see https://spdx.dev/ids/#how)
Of note:
- If the author of a file is explicitly listed, maintain this
information
- If the 'author' is listed as the generic 'Rust Bitcoin developers'
just remove the attribution, this is implicit. This does loose the date
info but that can be seen at any time from the git index using
`git log --follow --format=%ad --date default <FILE> | tail -1`
Last release, before we had access to `non_exhaustive` we added some
fancy types to enable conditionally having a `bitcoinconsensus::Error`.
Now that we have bumped the MSRV and have already added `non_exhaustive`
to the `Error` type in question, we can use a compiler attribute to
conditionally include the `bitcoinconsensus` error.
Remove `Uninhabited` and its usage.
0bae41129d Make `opcode::to_u8` a const function (Matt Corallo)
Pull request description:
In general, if a function can be const, it should be, and this one
trivially can be, so it should be.
ACKs for top commit:
tcharding:
ACK 0bae41129d
apoelstra:
ACK 0bae41129d
Tree-SHA512: 0df883894ec65dac1b709c581ffc3d2daca737561d05adb971dfef736e4147c59ea66cdc0520296c619e13e1dac70c07cb062b61304541cf488262244f313009
The `u8` parameter in the `SchnorrSighashType` constructor is a
consensus valid `u8`. Re-name the constructor to make this explicit.
Deprecate `from_u8` as is typical.
TryFrom` became available in Rust 1.34 so we can use it now we have
bumped our MSRV.
Add a macro for implementing `TryFrom` for various lists of
`sha256::Hash` types. Use the macro to for vec, slice, and boxed slice.
TryFrom` became available in Rust 1.34 so we can use it now we have
bumped our MSRV.
Implement `TryFrom<Key>` for `ProprietaryKey` and deprecate the
`from_key` method.
TryFrom` became available in Rust 1.34 so we can use it now we have
bumped our MSRV.
Implement `TryFrom<TaprootBuilder>` for `TapTree` and deprecate the
`from_builder` method.
We have a bunch of 'from' methods that are fallible; `TryFrom` became
available in Rust 1.34 so we can use it now we have bumped our MSRV.
Implement the various `WitnessVersion` from methods using `TryFrom` and
deprecate the originals.
bea5569cd3 Remove duplicate must_use (Tobin C. Harding)
Pull request description:
Clippy emits:
warning: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
This is because the return type of the function `legacy_encode_signing_data_to` is `EncodeSigningDataResult` which is
already marked as `must_use`. There is no need to have `must_use` on the function also.
##
Hey apoelstra, this PR is needed before any other PR can get a green CI run. The PR that introduced this warning must have had a green CI run before the 'add clippy to CI' PR merged allowing it to merge without re-running CI.
ACKs for top commit:
apoelstra:
ACK bea5569cd3
Kixunil:
ACK bea5569cd3
Tree-SHA512: 27b606685bc2d5317a5e332431511af9b1017bf58462c283b322e5b6faf645fd8e006455af3d6888ab2c3c680b11e4b89f538f26440f4f1422e0daabc61825ac
Clippy emits:
warning: this function has an empty `#[must_use]` attribute, but
returns a type already marked as `#[must_use]`
This is because the return type of the function
`legacy_encode_signing_data_to` is `EncodeSigningDataResult` which is
already marked as `must_use`. There is no need to have `must_use` on the
function also.
I'm guessing this got through to master because we only just added
clippy to CI.
Fix#1020 (see more relevant discussion there)
This definitely makes the amount of generics compiler
has to generate by avoding generating the same functions
for `R`, &mut R`, `&mut &mut R` and so on.
old:
```
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 9947832 Jun 2 22:42 target/release/deps/bitcoin-07a9dabf1f3e0266
> strip target/release/deps/bitcoin-07a9dabf1f3e0266
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 4463024 Jun 2 22:46 target/release/deps/bitcoin-07a9dabf1f3e0266
```
new:
```
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 9866800 Jun 2 22:44 target/release/deps/bitcoin-07a9dabf1f3e0266
> strip target/release/deps/bitcoin-07a9dabf1f3e0266
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 4393392 Jun 2 22:45 target/release/deps/bitcoin-07a9dabf1f3e0266
```
In the unit-test binary itself, it saves ~100KB of data.
I did not expect much performance gains, but turn out I was wrong(*):
old:
```
test blockdata::block::benches::bench_block_deserialize ... bench: 1,072,710 ns/iter (+/- 21,871)
test blockdata::block::benches::bench_block_serialize ... bench: 191,223 ns/iter (+/- 5,833)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 37,543 ns/iter (+/- 732)
test blockdata::block::benches::bench_stream_reader ... bench: 1,872,455 ns/iter (+/- 149,519)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 136 ns/iter (+/- 3)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 51 ns/iter (+/- 8)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 5 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_size ... bench: 3 ns/iter (+/- 0)
```
new:
```
test blockdata::block::benches::bench_block_deserialize ... bench: 1,028,574 ns/iter (+/- 10,910)
test blockdata::block::benches::bench_block_serialize ... bench: 162,143 ns/iter (+/- 3,363)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 30,725 ns/iter (+/- 695)
test blockdata::block::benches::bench_stream_reader ... bench: 1,437,071 ns/iter (+/- 53,694)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 92 ns/iter (+/- 2)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 17 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 5 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_size ... bench: 4 ns/iter (+/- 0)
```
(*) - I'm benchmarking on a noisy laptop. Take this with a grain of salt. But I think
at least it doesn't make anything slower.
While doing all this manual labor that will probably generate conflicts,
I took a liberty of changing generic type names and variable names to
`r` and `R` (reader) and `w` and `W` for writer.
e2e465056c Add clippy to pre-commit githook (Tobin C. Harding)
820adc0de0 Add githooks directory (Tobin C. Harding)
668b37af5c Enable clippy on CI (Tobin C. Harding)
a2a54b3982 Remove unnecessary ? operator (Tobin C. Harding)
67ed8f673e Remove unneeded clone (Tobin C. Harding)
eccd401fc4 Remove unneeded reference (Tobin C. Harding)
fd4239f1d2 Use custom digit grouping (Tobin C. Harding)
acd551e644 Remove unnecessary 'static lifetime (Tobin C. Harding)
3102a48d39 Allow clippy::collapsible_else_if (Tobin C. Harding)
63f4ff6406 Remove redundant field names (Tobin C. Harding)
Pull request description:
We are almost ready to enable clippy on CI!
First clear a few remaining warnings from feature gated code. Then add a CI job to run clippy using `--all-features` (note no `--all-targets`). Next add githooks directory (this is the patch from https://github.com/rust-bitcoin/rust-bitcoin/pull/1044/commits). Finally add `cargo clippy` to the pre-commit hook.
EDIT: I just realized the running of clippy could have been done in `test.sh` instead of as a github action. Does that matter?
ACKs for top commit:
apoelstra:
ACK e2e465056c
Kixunil:
ACK e2e465056c
Tree-SHA512: c78cb78973d3935e5be7908eec7d6ffaa7989724b3e29551b9fa2cb35df1f39574e31e5cc93fdfb32b35039ac9dac5c080ae4287a1e6979dd076bab56e6eda1b
Add a `githooks` directory. Copy the sample pre-commit hook into it.
Add a section to the README explaining how to take advantage of the
githooks.
The sample pre-commit hook includes checks for trailing whitespace that
we are seeing occasionally in PRs.
Done in preparation for adding a clippy githook.
Add a clippy configuration file configuring the MSRV. Add a github
actions job to run clippy on CI.
Please note the job does _not_ use `--all-targets` because doing so
causes:
```
error[E0554]: `#![feature]` may not be used on the stable release channel
--> src/lib.rs:46:54
|
46 | #![cfg_attr(all(test, feature = "unstable"), feature(test))]
```
clippy emits:
warning: this expression creates a reference which is immediately
dereferenced by the compiler
As suggested, remove the explicit reference.
clippy emits a bunch of:
warning: digits grouped inconsistently by underscores
We have a custom grouping elsewhere in this file
10_000_000_00 sats == 10 BTC
Fix up all instances of large sats amount to uniformly using this format
and add compiler directives where needed to shoosh clippy.
clippy emits:
warning: this `else { if .. }` block can be collapsed
In this instance the code is more readable how it is, we should ignore
clippy.
Add compiler directive to quieten warning.
281af7c1b9 Move broken-intra-doc-link lint config to command line (Tobin C. Harding)
Pull request description:
The docs lint `broken-intra-doc-links` has been changed but the new name
is not available in our MSRV, this means we get a build warning. We only
build docs with the nightly toolchain so we can move this lint control
to the docs build command in `test.sh` instead of doing it crate wide.
With this patch applied devs risk not noticing docs link issues until
they hit them on CI _if_ they do not build with the test script or
explicitly pass in `-- -D rustdoc::broken-intra-doc-links`, which no one
is going to do. Hence we add a line to the readme with a shell alias
that can be used to check docs, taken directly from `test.sh`.
ACKs for top commit:
apoelstra:
ACK 281af7c1b9
Kixunil:
ACK 281af7c1b9
Tree-SHA512: 7c9be3bcf097444a107199c9e9df62324d80b770659556a81eca160b807245e15921cda812f83e8b24d41716273704ff7b78be9300680f1efef3cb1fbffe6afd
The docs lint `broken-intra-doc-links` has been changed but the new name
is not available in our MSRV, this means we get a build warning. We only
build docs with the nightly toolchain so we can move this lint control
to the docs build command in `test.sh` instead of doing it crate wide.
With this patch applied devs risk not noticing docs link issues until
they hit them on CI _if_ they do not build with the test script or
explicitly pass in `-- -D rustdoc::broken-intra-doc-links`, which no one
is going to do. Hence we add a line to the readme with a shell alias
that can be used to check docs, taken directly from `test.sh`.
7a3bb7d3ec Replace runtime size check with compile time check (Tobin C. Harding)
Pull request description:
Add a macro `const_assert` that uses some const declaration trickery to trigger a compile time error if a boolean expression is false.
Replace runtime checks using `debug_assert_eq!` with the newly defined `const_asert!` macro.
## Note
This PR is the first patch from a [recently closed PR](https://github.com/rust-bitcoin/rust-bitcoin/pull/953). Props to @elichai for the macro idea in the review of that PR.
ACKs for top commit:
Kixunil:
ACK 7a3bb7d3ec
apoelstra:
ACK 7a3bb7d3ec
Tree-SHA512: cfd4dcf6c66e06796cab6dc49445f0f8c5d4e686893a17735420dccedd75ad7c632d240a5ab92ee47ce459b799daeaf3fdf9c6b77c1b81b09e87197a9f86c5ba
Currently we are using the fragment-specifier `expr` in a bunch of
macros for captures that are only used for literals. If we use `literal`
instead it allows the compiler to give slightly more specific error
messages.
The benefits of this change are minor. Of note, this patch found one
unusual macro call site (removed unnecessary `&`).
The macros changed are all internal macros, this is not a breaking change.
Add a macro `const_assert` that uses some const declaration trickery to
trigger a compile time error if a boolean expression is false.
Replace runtime checks using `debug_assert_eq!` with the newly defined
`const_assert!` macro.
ec8dadaf86 Implement iter::size_hint and ExactSizeIterator for Witness Iter (Riccardo Casatta)
Pull request description:
close https://github.com/rust-bitcoin/rust-bitcoin/issues/1050
I don't think we need to change the `collect` since it use the `size_hint()` lower bound to initially allocate
// with size_hint
// test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 313 ns/iter (+/- 13)
// test blockdata::witness::benches::bench_witness_to_vec ... bench: 204 ns/iter (+/- 11)
// without
// test blockdata::witness::benches::bench_big_witness_to_vec ... bench: 489 ns/iter (+/- 28)
// test blockdata::witness::benches::bench_witness_to_vec ... bench: 221 ns/iter (+/- 102)
The reason why the small witness doesn't get big perf boost is because by default vec allocates 4 slots
ACKs for top commit:
Kixunil:
ACK ec8dadaf86
apoelstra:
ACK ec8dadaf86
Tree-SHA512: dbe09ba6ebd4014fe0639412894beedab6cc7e844a5ec1697af8f0694b62ae5d423a801df1b48ac7029444c01877975e2d5168728f038fbd4f5808bda90e0f2f
abfeb32e35 Remove unnecessary local variable (Tobin C. Harding)
04b09a4e8d Remove unused loop (Tobin C. Harding)
380e0016cc Use write_all instead of write (Tobin C. Harding)
Pull request description:
Done while clearing clippy warnings, done as a separate PR because its not a simple glance to review like the others.
Remove 2 clippy warnings and remove unnecessary local variable.
ACKs for top commit:
apoelstra:
ACK abfeb32e35
Kixunil:
ACK abfeb32e35
Tree-SHA512: 965708999c067dd8c156bbc54b711f608d524fab7051a0e56066f53b5c8d7bea1c233f04e77873b2624cd22e26a58f1d22f47870d2afe4347aa85335c3142245
The `ParseAmountError` does not implement `std::error::Error`, must have
been missed when we did the rest.
Implement `std::error::Error` for `ParseAmount`.
7743be00cf Removed edition change heads up from CONTRIBUTING (Martin Habovštiak)
Pull request description:
It is done.
ACKs for top commit:
apoelstra:
ACK 7743be00cf
sanket1729:
ACK 7743be00cf
Tree-SHA512: 9cc63a97d959061db1b04757fbf993a5f2cf0e1fff388b23a8729995bf764aced18b3ef49ec25a5a324d8e49168b9e841baf2118f0769fc8689f46c3d9b3463c
29cfdc8614 README: remove stale info about upcoming edition change (Dawid Ciężarkiewicz)
Pull request description:
It is done.
ACKs for top commit:
tcharding:
ACK 29cfdc8614
apoelstra:
ACK 29cfdc8614
sanket1729:
ACK 29cfdc8614
Tree-SHA512: fc5b694229e4a3ebcc8f25cd4d73b65cb798d441934b2e2b473c0811367bb9c5bd6cfae9e5b68b072e3c6320f386ab54138b8589a57fc0816702474ab6de06f8
66e852cd19 Update format of ExcessiveScriptSize error message (eunoia_1729)
89bd4b61a4 Modify from_script functions in address.rs to return result (eunoia_1729)
Pull request description:
Modify from_script functions to return result instead of option so that, in case of errors, there is more
information on what went wrong.
Resolves: #1022
ACKs for top commit:
sanket1729:
ACK 66e852cd19. LGTM
tcharding:
ACK 66e852cd19
apoelstra:
ACK 66e852cd19
Tree-SHA512: 0d9529aee0a5459351bed2cc8b2c5571736d3293e2931c43d98f53330e9ac5f3d998a19da2b4575af0a3c1c4dcfd5a24c8813390bf6f5492a689c36ebb9cb426
Modify from_script functions to return result instead of option so that, in case of errors, there is more
information on what went wrong.
Resolves: #1022
271d0ba068 Allow many arguments in test function (Tobin C. Harding)
c0c88fe87d Use vec instead of pushing to a mutable vector (Tobin C. Harding)
73066e7e48 Use values() to iterate map values (Tobin C. Harding)
38ff025122 Remove useless use of vec! (Tobin C. Harding)
d8e82d5cd4 Remove length comparison to zero (Tobin C. Harding)
c1f34f5c0e Return Address directly (Tobin C. Harding)
ff8d585c17 Use flat_map instead of map().flatten() (Tobin C. Harding)
b24a112f08 Remove calls to clone from types that implement Copy (Tobin C. Harding)
2b8d93ec4b Remove unnecessary explicit reference (Tobin C. Harding)
ef90e3d4ed Use plus-equals operator (Tobin C. Harding)
922b820105 Replace assert!(false) with panic! (Tobin C. Harding)
a8039e1742 Remove redundant clone (Tobin C. Harding)
cf8de73169 Remove unnecessary cast of integer literal (Tobin C. Harding)
999ac450bb Do not use assert_eq with literal bool (Tobin C. Harding)
827fcd8a89 Allow unusual digit grouping (Tobin C. Harding)
242c640603 Remove redundant field names (Tobin C. Harding)
0f8f4c5609 Collapse if statements (Tobin C. Harding)
229fcb9f1f Use if let instead of destructuring pattern (Tobin C. Harding)
Pull request description:
Clear all the clippy warnings (excl. #1042) that are returned by running `cargo clippy --all-targets`.
I apologize in advance for the review burden :)
ACKs for top commit:
elichai:
ACK 271d0ba068
apoelstra:
ACK 271d0ba068
Tree-SHA512: 71ad2ec3db808e795791b7513f8b2a1c13dc90317f5328602c9ecbc31c09f82471f79c9c31a71a0ded5280554d1019d2bb4899fb9e8fa6421d46a2397cd31242
Andrew Poelstra
DanGould
Tobin C. Harding
Dawid Ciężarkiewicz
Matt Corallo
Riccardo Casatta
eunoia_1729
sanket1729
Martin Habovštiak