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
Clippy emits:
warning: using `clone` on type `blockdata::transaction::OutPoint`
which implements the `Copy` trait
Remove calls to `clone` from types that implement `Copy`.
Clippy warns about creating a reference that is immediately
de-referenced.
Remove unnecessary explicit `&`, while we are at it remove unnecessary
explicit types that appear on the same lines of code.
We only simulate a single connection in the test function `serve_tcp`.
Remove the unused loop (includes an unconditional break after first
iteration) and use `next` directly.
Found by clippy. Refactor only, no logic changes.
Andrew Poelstra
Dawid Ciężarkiewicz
Tobin C. Harding
Riccardo Casatta
eunoia_1729
sanket1729
Martin Habovštiak