For internal macros used only in this crate we do not need to use
`macro_use` and pollute the top level namespace now that we have edition
2018. We can add a `pub(crate) use` statement to each and then path
imports work for the macros like normal types.
36f29d4357 Upgrade to secp256k1 v0.23.0 (Tobin C. Harding)
Pull request description:
We recently released a new version of `rust-secp256k1`, upgrade to use it.
ACKs for top commit:
apoelstra:
ACK 36f29d4357
Kixunil:
ACK 36f29d4357
Tree-SHA512: 46a909dec8bc59daa78acdb76824d93f4f1da0e9736cf6ca443d3bbadfa43867e720293bb7c4919cb0658e75ec59daeffea080611f0e7eed4df439ddac0305de
91ff2f628c Introduce SPDX license identifiers (Tobin C. Harding)
Pull request description:
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`
apoelstra, please confirm that I'm not treading on your toes here, especially, are you ok with the new 'written by' string format?
### Ref
- https://spdx.dev/ids/#how
- https://spdx.org/licenses/CC0-1.0.html
- https://spdx.dev/ids/
ACKs for top commit:
apoelstra:
ACK 91ff2f628c
sanket1729:
ACK 91ff2f628c. I am also in IDGAF camp, but I like more red lines in diff.
Kixunil:
ACK 91ff2f628c
Tree-SHA512: ca8aac00f015c18ec18de83dfeb50dd6f4f840653c7def85daa2436a339021ada5f3c34ad0cdf6b18e3e39c45a6d58a8313742e4001d467785b10eee7fdbc938
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
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`
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.
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.
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
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
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.
Clippy emits:
warning: struct `VarInt` has a public `len` method, but no `is_empty`
method
However, `VarInt` has no concept of 'is empty' so add a compiler
directive to allow the lint.
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`.
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.
We have a few instances of strange indentation:
- Incorrect number of characters
- Usage of neither "Block" style or "View" style (elect to use "Block")
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.
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.
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.
Calls to `unwrap` outside of tests are typically unfavourable.
In memory writers (`Vec`) do not error. We can use `expect` with a
descriptive message string to indicate this.
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.
c704ee7ffe [docs-only] Use backtick in addition to square parentheses for types references, clarify legacy, non_exhaustive comment, remove std:: (Riccardo Casatta)
f223be618f Rename access_witness to witness_mut and return Option (Riccardo Casatta)
c9bc0b928a [fmt-only] autoformatting with `rustfmt src/util/sighash.rs` (Riccardo Casatta)
07774917c2 Use get_or_insert_with in segwit_cache (Martin Habovstiak)
497dbfb7c3 Use get_or_insert_with in common_cache() (Martin Habovstiak)
ca80a5a030 Use get_or_insert_with in taproot_cache (Martin Habovstiak)
6e06a32ccc Wrap ErrorKind in Io enum variant, fix doc comment for the IO variant (Riccardo Casatta)
1a2b54ff23 introduce constant KEY_VERSION_0 (Riccardo Casatta)
417cfe31e3 Derive common traits for structs and enum, make internal struct not pub (Riccardo Casatta)
55ce3dd6ae Fix validation error if SINGLE with missing corresponding output, remove check_index and check with get().ok_or(), more details in errors (Riccardo Casatta)
2b3b22f559 impl Encodable for Annex to avoid allocation (Riccardo Casatta)
1a7afed068 Add Reserved variant to SigHashType for future use (ie SIGHASH_ANYPREVOUT) (Riccardo Casatta)
53d0e176d3 Deprecate bip143::SigHashCache in favor of sighash::SigHashCache (Riccardo Casatta)
15e3caf62d [test] Test also sighash legacy API with legacy tests (Riccardo Casatta)
24acfe3672 Implement Bip341 signature hash, create unified SigHashCache for taproot, segwit and legacy inputs (Riccardo Casatta)
683b9c14ff add [En|De]codable trait for sha256::Hash (Riccardo Casatta)
Pull request description:
Adds https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki message signature algorithm
The base is taken from `bip143::SigHashCache`, some code results duplicated but I think it's more clear to keep things separated
Would mark some bullet point on https://github.com/rust-bitcoin/rust-bitcoin/issues/503
Test vectors are taken by running d1e4c56309/test/functional/feature_taproot.py with a modified `TaprootSignatureHash` function to print intermediate values that I cannot found in the bip341 [test vector json](https://raw.githubusercontent.com/bitcoin-core/qa-assets/main/unit_test_data/script_assets_test.json)
UPDATE: Latest version includes the suggestion from @sanket1729 to create a unified tool for signature message hash for legacy, segwit, and taproot inputs. In particular, makes sense for mixed segwit v0 and taproot v1 inputs because cached values could be shared
ACKs for top commit:
sanket1729:
ACK c704ee7ffe. Reviewed the diff from a37de1ade475e0c31c932121abaa7aec701b9987 which I previously ACKed
dr-orlovsky:
utACK c704ee7ffe by diffing it to 6e06a32ccc having my ACK before.
apoelstra:
ACK c704ee7ffe
Tree-SHA512: 35530995fe9d078acd0178cfca654ca980109f4502c91d578c1a0d5c6cafacab7db1ffd6216288eac99f6a763776cbc0298cfbdff00b5a83e98ec4b15aa764e8
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
This is instead of encode::Errors because the encoders should
not be allowed to return errors that don't originate in the writer
they are writing into.
This is a part of the method definition that has been relied upon for a
while already.
Currently whenever an unrecognized network message is received, it is never
flushed from the read buffer, meaning that unless the stream is closed and
recreated it will keep returning the same error every time `read_next()` is
called.
This commit adds the length of the message to `UnrecognizedNetworkCommand`,
so that the `StreamReader` can flush those bytes before returning the error
to the caller.
Steven Roose
Tobin C. Harding
Andrew Poelstra
Dawid Ciężarkiewicz
sanket1729
Martin Habovstiak
Riccardo Casatta
Devrandom
Dr. Maxim Orlovsky
Alekos Filini
Jake Rawsthorne