64152ff6fc Remove unused lifetimes (Tobin C. Harding)
Pull request description:
We somehow have two lifetimes that are unused and causing clippy warnings, remove them.
This fixes the red CI runs on a bunch of other open PRs. FTR I have no idea how these got past clippy onto master.
ACKs for top commit:
Kixunil:
ACK 64152ff6fc
apoelstra:
ACK 64152ff6fc
Tree-SHA512: cced838b575b29d90c4325ab42ada93bae4751721d3ca2c19ec801892c38130570613e4ab8de757b73ecc83cda5c00a32867139e04a2615833d05dc21551af1a
4d2291930b Use fragment-specifier literal (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
Kixunil:
ACK 4d2291930b
apoelstra:
ACK 4d2291930b
Tree-SHA512: 51c109fe3a884191bf623508555c1d5ad337a3f3b48538d18aec13e581f2c5fbbd055be49600ced19f38541412c34090bd8bac61fd05d5aa9702c96ff521364f
a8e62f249b Remove Uninhabited (Tobin C. Harding)
Pull request description:
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.
For more context see the [original attempt ](https://github.com/rust-bitcoin/rust-bitcoin/pull/1025)at using `Infallible` (last comment in discussion thread).
ACKs for top commit:
Kixunil:
ACK a8e62f249b
dunxen:
ACK a8e62f2
apoelstra:
ACK a8e62f249b
Tree-SHA512: 8c03c44d7533af1a9a1185b7f9e0fa2c52369eaac8a45f0e2199e7e1bbd08ba2bfa23d829d2c2abf7f45fe8cc26ccad02f2e1a6d408d2c0fbca23140cafe3801
1fea098dfb Support unsized `R` and `W` in consensus encode/decode (Dawid Ciężarkiewicz)
a24a3b0194 Forward `consensus_decode` to `consensus_decode_from_finite_reader` (Dawid Ciężarkiewicz)
9c754ca4de Take Writer/Reader by `&mut` in consensus en/decoding (Dawid Ciężarkiewicz)
Pull request description:
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.
ACKs for top commit:
RCasatta:
ACK 1fea098dfb tested in downstream lib, space saving in compiled code confirmed
apoelstra:
ACK 1fea098dfb
Tree-SHA512: bc11994791dc97cc468dc9d411b9abf52ad475f23adf5c43d563f323bae0da180c8f57f2f17c1bb7b9bdcf523584b0943763742b81362880206779872ad7489f
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.
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
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
junderw
Andrew Poelstra
Tobin C. Harding
DanGould
Dawid Ciężarkiewicz
Matt Corallo
Riccardo Casatta
eunoia_1729
sanket1729
Martin Habovštiak