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.
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
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.
57dd6739c3 Do not print error when displaying for std builds (Tobin C. Harding)
b80cfeed85 Bind to error_kind instead of e (Tobin C. Harding)
241ec72497 Bind to b instead of e (Tobin C. Harding)
01f481bf5c Bind to s instead of e (Tobin C. Harding)
5c6d369289 network: Remove unused error variants (Tobin C. Harding)
e67e97bb37 Put From impl below std::error::Error impl (Tobin C. Harding)
6ca98e5275 Remove error TODO (Tobin C. Harding)
Pull request description:
As part of the ongoing error improvement work and as a direct result of [this comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/987#issuecomment-1135563287) improve the `Display` implementations of all our error types so as to not repeat the source error when printing.
The first 5 patches are trivial clean ups around the errors. Patch 6 is the real work.
EDIT: ~CC @Kixunil, have I got the right idea here bro?~ Patch 6 now includes a macro as suggested.
ACKs for top commit:
Kixunil:
ACK 57dd6739c3
apoelstra:
ACK 57dd6739c3
sanket1729:
ACK 57dd6739c3. Did not check if we covered all cases. We need to remember to use `write_err!` instead of `write!` in future.
Tree-SHA512: 1ed26b0cc5f9a0f71684c431cbb9f94404c116c9136be696434c56a2f56fd93cb5406b0955edbd0dc6f8612e77345c93fa70a70650118968cc58e680333a41de
99f565f932 Add non_exhaustive to all error enums (Tobin C. Harding)
Pull request description:
Adding an error variant to a public enum is an API breaking change, this means making, what could be, small refactorings or improvements harder. If we use `non_exhaustive` for error types then we mitigate this cost.
There is a tradeoff however, downstream users who explicitly match on our public error types must include a wildcard pattern.
ACKs for top commit:
apoelstra:
ACK 99f565f932
Kixunil:
ACK 99f565f932
Tree-SHA512: ff329f87d52b3fbe24654f32e4062ddae73173cba5a13d511591158e68ee278e9bdc0a70a3e0b42d6606b369255923f9c46d8b3d1b2ff75f8461a82567df80cd
5fbb211085 Use fn name to_ instead of as_ (Tobin Harding)
8ffa32315d Use fn name to_ instead of into_ (Tobin Harding)
6874ce91e2 Remove as_inner (Tobin C. Harding)
Pull request description:
Rust has naming conventions surrounding conversion functions
We have a handful of methods that are not following convention. This PR is done as three patches, separated by incorrect function name (`into_` or `as_`) and by whether or not the original method needs deprecating. Can be squashed if folks prefer.
From the docs: https://rust-lang.github.io/api-guidelines/naming.html
<h2><a class="header" href="https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv" id="ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv">Ad-hoc conversions follow <code>as_</code>, <code>to_</code>, <code>into_</code> conventions (C-CONV)</a></h2>
<p>Conversions should be provided as methods, with names prefixed as follows:</p>
Prefix | Cost | Ownership
-- | -- | --
as_ | Free | borrowed -> borrowed
to_ | Expensive | borrowed -> borrowed
| | | borrowed -> owned (non-Copy types)
| | | owned -> owned (Copy types)
into_ | Variable | owned -> owned (non-Copy types)
EDIT: I did actually audit all uses of `to_` when I first did this, I did this by grepping for `fn to_` and checking the output against the table.
ACKs for top commit:
apoelstra:
ACK 5fbb211085
Kixunil:
ACK 5fbb211085
Tree-SHA512: f750b2d1a10bc1d4bb030d8528a582701cc3d615aa8a8ab391324dae639544bb3629a19b372784e1e274a8ddcc613c621c7aae21a3ea54fde356a6aa5e611ac0
a6efe982bd Use write_all to write whole buffer (Tobin C. Harding)
51c60b8507 Allow no is_empty method for VarInt (Tobin C. Harding)
841f1f5832 Implement Default for TaprootBuilder (Tobin C. Harding)
f81d4aa9bd Remove unnecessary call to clone (Tobin C. Harding)
27649ba182 Use copied instead of map to copy (Tobin C. Harding)
62ccc9102c Use iter().flatten().any() instead of if let Some (Tobin C. Harding)
4b28a1bb97 Remove unneeded return statement (Tobin C. Harding)
16cac3cd70 Derive Default for Witness (Tobin C. Harding)
c75189841a Remove unnecessary closure (Tobin C. Harding)
dfff85352a Ignore bytes written for sighash_single bug output (Tobin C. Harding)
14c72e755b Use contains combinator instead of manual range (Tobin C. Harding)
b7d6c3e02c Remove additional reference (Tobin C. Harding)
1940b00132 Implement From instead of Into (Tobin C. Harding)
fcd0f4deac Use struct field init shorthand (Tobin C. Harding)
641960f037 Use rustfmt::skip (Tobin C. Harding)
3cd00e5d47 Remove unnecessary whitespace (Tobin C. Harding)
Pull request description:
Clear all current Clippy warnings, codebase wide. Possibly contentious patches include:
- [commit](fcd0f4deac): `fcd0f4d Use struct field init shorthand`
- [commit](14c72e755b): `14c72e7 Use contains combinator instead of manual range`
- [commit](3b3c37803a): `3b3c378 Use iter().flatten() instead of if let Some`
## Notes
Please note commit `dfff8535 Ignore bytes written for sighash_single bug output` touches the same lines of code as commit `a6efe982 Use write_all to write whole buffer`.
ACKs for top commit:
apoelstra:
ACK a6efe982bd
Kixunil:
ACK a6efe982bd
Tree-SHA512: 5351a82fd3deadb8e53911c43b5a60a9517d5c57014f5fa833b79b32c0a4606ada0bcd28e06ce35d47aa74115c7cf70c27a1ba9c561a3424ac85a4f69774014d
Adding an error variant to a public enum is an API breaking change, this
means making what could be small refactorings or improvements harder. If
we use `non_exhaustive` for error types then we mitigate this cost.
There is a tradeoff however, downstream users who explicitly match on
our public error types must include a wildcard pattern.
As things are right now, memory exhaustion protection in `Decodable`
is based on checking input-decoded lengths against arbitrary limits,
and ad-hoc wrapping collection deserialization in `Take`.
The problem with that are two-fold:
* Potential consensus bugs due to incorrect limits.
* Performance degradation when decoding nested structured,
due to recursive `Take<Take<..>>` readers.
This change introduces a systematic approach to the problem.
A concept of a "size-limited-reader" is introduced to rely on
the input data to finish at enforced limit and fail deserialization.
Memory exhaustion protection is now achived by capping allocations
to reasonable values, yet allowing the underlying collections
to grow to accomodate rare yet legitmately oversized data (with tiny
performance cost), and reliance on input data size limit.
A set of simple rules allow avoiding recursive `Take` wrappers.
Fix#997
Rust convention is to use `to_` for conversion methods that convert from
an owned type to an owned `Copy` type. `as_` is for borrowed to borrowed
types.
Re-name and deprecate conversion methods that use `as_` for owned to
owned `Copy` types to use `to_`.
We implement `source` for all our error types. This means that we should
not display the source error explicitly because users can call `source`
to get the source error.
However, `std::Error::source()` is only available for "std" builds, so
that we do not loose the error source information in "no-std" builds add
a macro that conditionally adds the source onto the error message.
07c75304d2 Refactor address byte swapping (Tobin C. Harding)
Pull request description:
Refactor address byte swapping
When encoding a `network::Address` two of the fields are encoded
big-endian instead of little-endian as is done by `consensus_encode`. In
order to achieve this we have a helper function `addr_to_be` that swaps
the bytes. This function is miss-named because it is not converting to a
specific endian-ness (which implies different behaviour on machines with
different endian-ness) but is reversing the byte order irrespective of
the underlying architecture.
- Remove function `addr_to_be`
- Inline the endian-ness code when encoding an address
- Remove TODO and use `to_be_bytes` when encoding port
- Add a function for reading big-endian bytes `read_be_address`
- Use `read_be_address` when decoding `Address` and `Addrv2`
Refactor only, no logic changes. Code path is already covered by
unit tests.
ACKs for top commit:
apoelstra:
ACK 07c75304d2
Kixunil:
ACK 07c75304d2
Tree-SHA512: 186bc86512e264a7b306f3bc2e18d1619f3cd84fc54412148cfc2663e8d6e9616ea9e2fe19eafec72d76cc11367a9b39cac2b73210d9e43eb8f453bd253b33de
When encoding a `network::Address` two of the fields are encoded
big-endian instead of little-endian as is done by `consensus_encode`. In
order to achieve this we have a helper function `addr_to_be` that swaps
the bytes. This function is miss-named because it is not converting to a
specific endian-ness (which implies different behaviour on machines with
different endian-ness) but is reversing the byte order irrespective of
the underlying architecture.
- Remove function `addr_to_be`
- Inline the endian-ness code when encoding an address
- Remove TODO and use `to_be_bytes` when encoding port
- Add a function for reading big-endian bytes `read_be_address`
- Use `read_be_address` when decoding `Address` and `Addrv2`
Refactor only, no logic changes. Code path is already covered by
unit tests.
Use cargo to upgrade from edition 2015 to edition 2018.
cargo fix --edition
No manual changes made. The result of the command above is just to fix
all the use statements (add `crate::`) and fix the fully qualified path
formats i.e., `::Foo` -> `crate::Foo`.
This adds tests for the previously untested reject message
(de)serialization. The two reject messages were received from an
older Bitcoin Core peer that still sends reject messages.
In this library we specifically do not use rustfmt and tend to favour
terse statements that do not use extra lines unnecessarily. In order to
help new devs understand the style modify code that seems to use an
unnecessary number of lines.
None of these changes should reduce the readability of the code.
Do various whitespace refactorings, of note:
- Use space around equals e.g., 'since = "blah"'
- Put return/break/continue on separate line
Whitespace only, no logic changes.
247a14f4c3 Use test big block for bench_stream_reader instead of making one (Riccardo Casatta)
b92dfbb63f exclude test_data when publishing the crate (Riccardo Casatta)
f5a9681a2a include a big block in test_data, use it for ser/de benchmark (Riccardo Casatta)
09dada55d6 Move bip158 test vectors to test_data (Riccardo Casatta)
06d1a820c3 Remove testnet block hex from tests, use test_data with include_bytes! (Riccardo Casatta)
Pull request description:
In the first two commits I moved some data from source files to the newly introduced `test_data` dir, including it with `include_[str|bytes]!` macro.
The second-to-last commit introduces a big block in test_data which is very handy in ser/de benchmark (I used it for #672) because with smaller blocks you may not notice performance improvements.
Since I don't want to pollute the package the last commit excludes the `test_data` dir from the published package. I think it's fine to do it because dependent packages don't run dependencies tests.
ACKs for top commit:
apoelstra:
ACK 247a14f4c3
Kixunil:
tACK 247a14f4c3
Tree-SHA512: a2beb635b0a358737d0b57d3e7205b1ddf87652b9a8c889ce63e2867659a8eaf7e43a5b87a453345d56d953745913f40b58596f449e5fbc87340e0dd2aef0727
533120899e Put rustdocs above attributes (Tobin Harding)
Pull request description:
(Trivial / Very low priority PR)
Rust idiomatic style is to put the rustdoc _above_ any attributes on types, functions, etc.
Audit the codebase and move comments/attributes to the correct place. Add a trailing full stop at times to neaten things up a little extra.
Done after discussion [here](https://github.com/rust-bitcoin/rust-secp256k1/pull/353#discussion_r778393138)
ACKs for top commit:
Kixunil:
ACK 533120899e
RCasatta:
ACK 533120899e
Tree-SHA512: 7cd00dc46de813cbe3f96417bb4b13980064e10110b421224496c8b64bbe87b61b6c757cc621fde1d05754be6ecdc08acdb51fd8978e3f820d2d93f7104062d1
e860333bf3 Fix typos (Riccardo Casatta)
9189539715 Use BufReader internally in StreamReader to avoid performance regression on existing callers (Riccardo Casatta)
5dfb93df71 Deprecate StreamReader (Riccardo Casatta)
9ca6c75b18 Bench StreamReader (Riccardo Casatta)
Pull request description:
`StreamReader` performance is extremely poor in case the object decoded is "big enough" for example a full Block.
In the common case, the buffer is 64k, so to successfully parse a 1MB block 16 decode attempts are made.
Even if a user increases the buffer size, `read` is not going to necessarily fill the buffer, as stated in the doc https://doc.rust-lang.org/stable/std/io/trait.Read.html#tymethod.read. In my tests, the reads are 64kB even with a 1MB buffer.
I think this is the root issue of the performance issue found in electrs in https://github.com/romanz/electrs/issues/547 and they now have decided to decode the TCP stream with their own code in cd0531b8b7 and 05e0221b8e.
Using directly `consensus_encode` seems to make more sense (taking care of using `BufRead` if necessary) so the `StreamReader` is deprecated
ACKs for top commit:
Kixunil:
ACK e860333bf3
apoelstra:
ACK e860333bf3
Tree-SHA512: a15a14f3f087be36271da5008d8dfb63866c9ddeb5ceb0e328b4a6d870131132a8b05103f7a3fed231f5bca099865efd07856b4766834d56ce2384b1bcdb889b
Rust idiomatic style is to put the rustdoc _above_ any attributes on
types, functions, etc.
Audit the codebase and move comments/attributes to the correct place.
Add a trailing full stop at times to neaten things up a little extra.
StreamReader before this commit is trying to repeatedly parse big object like
blocks at every read, causing useless overhead.
consensus_encode deal with partial data by simply blocking.
After this changes it doesn't look what remain of the StreamReader is really giving
value, so it's deprecated
This is the initial step towards using and maybe enforcing clippy.
It does not fix all lints as some are not applicable. They may be
explicitly ignored later.
Docs can always do with a bit of love.
Clean up the module level (`//!`) rustdocs for all public modules.
I claim uniform is better than any specific method/style. I tried to fit
in with what ever was either most sane of most prevalent, therefore
attaining uniformity without unnecessary code churn (one exception being
the changes to headings described below).
Notes:
* Headings - use heading as a regular sentence for all modules e.g.,
```
//! Bitcoin network messages.
```
as opposed to
```
//! # Bitcoin Network Messages
```
It was not clear which style to use so I picked a 'random' mature
project and copied their style.
* Added 'This module' in _most_ places as the start of the module
description, however I was not religious about this one.
* Fixed line length if necessary since most of our code seems to follow
short (80 char) line lengths for comments anyways.
* Added periods and fixed obvious (and sometimes not so obvious)
grammatically errors.
* Added a trailing `//!` to every block since this was almost universal
already. I don't really like this one but I'm guessing it is Andrew's
preferred style since its on the copyright notices as well.
This documents cargo features in two ways: explictly in text and in code
using `#[doc(cfg(...))]` attribute where possible. Notably, this is
impossible for `serde` derives. The attribute is contitional and only
activated for docs.rs or explicit local builds.
This change also adds `package.metadata.docs.rs` field to `Cargo.toml`
which instructs docs.rs to build with relevant features and with
`docsrs` config activated enabling `#[doc(cfg(...))] attributes.
I also took the opportunity to fix a few missing spaces in nearby code.
Based on the original work by Justin Moon.
*MSRV unchanged from 1.29.0.*
When `std` is off, `no-std` must be on, and we use the [`alloc`](https://doc.rust-lang.org/alloc/) and core2 crates. The `alloc` crate requires the user define a global allocator.
* Import from `core` and `alloc` instead of `std`
* `alloc` only used if `no-std` is on
* Create `std` feature
* Create `no-std` feature which adds a core2 dependency to polyfill `std::io` features. This is an experimental feature and should be
used with caution.
* CI runs tests `no-std`
* MSRV for `no-std` is 1.51 or so
Dawid Ciężarkiewicz
Andrew Poelstra
Tobin C. Harding
sanket1729
0xb10c
Roman Zeyde
Riccardo Casatta
Martin Habovstiak
Ben Carman
Devrandom