24f0441d54 Add PublicKey::to_sort_key method for use with sorting by key (junderw)
Pull request description:
Replaces #524
See previous PR for reasoning.
This solution is a little more straightforward. The name and documentation should be enough to prevent misuse.
We can also impl a to_sort_key for any CompressedKey added later. (or just impl Ord in a BIP67 compliant way)
TODO:
- [x] Add more sorting test vectors. Ideas of edge cases to test are welcome.
ACKs for top commit:
apoelstra:
ACK 24f0441d54
tcharding:
ACK 24f0441d54
Kixunil:
ACK 24f0441d54
Tree-SHA512: 92d68cccaf32e224dd7328edeb3481dd7dcefb2f9090b7381e135e897c21f79ade922815cc766c5adb7ba751b71b51a773d103af6ba13fc081e1f5bfb846b201
Previous implementations of Witness (and Vec<Vec<u8>>) serde serialization
didn't support human-readable representations. This resulted in long unreadable
JSON/YAML byte arrays, which were especially ugly when pretty-printed (a line
per each byte).
Co-authored-by: Tobin C. Harding <me@tobin.cc>
This also removes tests for JSON backward-compatible encoding. Human-readable
encoding will be changed in the next commit and this will break backward
compatibility, thus that part of the test is removed.
35edcaa4ca Remove extern crate core statement (Tobin C. Harding)
Pull request description:
Now that we have an MSRV of 1.41.1 we no longer need `extern crate core`, remove it.
ACKs for top commit:
apoelstra:
ACK 35edcaa4ca
Kixunil:
ACK 35edcaa4ca
Tree-SHA512: 0efa0f05d3e9f797c493757fce6ca7703dd8439dbce4e9ff8113494fc71691ab4171c6f0fb5239b57ff5f0e223d48b057265a3df79898a399ca455040580aab2
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
19ba7ecc03 Add custom error for unknown address type parsing (Arturo Marquez)
Pull request description:
Adds a custom error `UnknownAddressType(_)` which is returned in cases where an unknown address type tries to be parsed via
`FromStr`. This provides more context for the error, since it contains the `String` that tried to be parsed.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/1064
ACKs for top commit:
Kixunil:
ACK 19ba7ecc03
dunxen:
ACK 19ba7ec
tcharding:
ACK 19ba7ecc03
apoelstra:
ACK 19ba7ecc03
Tree-SHA512: 2f94eb2e122c1acafcb8c852f7bfe22cb3725806541ae40f82d3a882011fb911ce8fc430153ebea4066f43c5a51813359a4c9b95d2a9077ce8f6dcaa58eee6fd
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
Adds a custom error `UnknownAddressType(_)` which is returned in
cases where an unknown address type tries to be parsed via
`FromStr`. This provides more context for the error.
For more info see [1].
[1] - `https://github.com/rust-bitcoin/rust-bitcoin/issues/1064`
Leading double colons are a relic of edition 2015. Remove all leading
double colons that follow a space, done like this so that reviewers can
do the same and verify the diff. Done with
search-and-replace ' ::' '::'
And, for the record:
```bash
function search-and-replace() {
if (($# != 2))
then
echo "Usage: $0 <this> <that>"
return
fi
local this="$1"
local that="$2"
for file in $(git grep -l "$this")
do
perl -pi -e "s/$this/$that/g" "$file"
done
}
```
32cfd93933 Use to_hex when available (Tobin C. Harding)
Pull request description:
We have a bunch of calls to `format!("{:x}", foo)` for types that implement `ToHex`. The code is terser with no loss of clarity if we use the trait method and call `to_hex()`.
ACKs for top commit:
apoelstra:
ACK 32cfd93933
Kixunil:
ACK 32cfd93933
Tree-SHA512: 87cb6660708c11dfafb56bd6e2ea2634043b2226d51903a20806c1ba51c6e7c4f0e4cc25e49f820b5b1236600af7da2a20893c49a5b8845d7652b143fd0ec388
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
We have a bunch of calls to `format!("{:x}", foo)` for types that
implement `ToHex`. The code is terser with no loss of clarity if we use
the trait method and call `to_hex()`.
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.
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.
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.
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.
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`.
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`.
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
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.
Clippy emits:
warning: digits grouped inconsistently by underscores
Add allow directive for grouping that aims to make explicit 100,000,000
sats/per bitcoin.
28049ce2d9 Document `Txid` being displayed backwards (Dawid Ciężarkiewicz)
Pull request description:
Fix#958
I hope putting it on the most notorious type where people actually notice it is enough. I couldn't find a good way to put it on all other `sha256d` automatically, and copy pasting it seems not worth it.
ACKs for top commit:
tcharding:
ACK 28049ce2d9
apoelstra:
ACK 28049ce2d9
Tree-SHA512: a5acf5d7a73361a6c48b45ed264fafb911930ae9f1bdb03895dc39c679d508dc56dbf44896fd38cf6569abb652e7fce721028ef06344462747a77078ef5a8f4f
99aab446c3 Remove network::Error (Tobin C. Harding)
Pull request description:
The `network::Error` is not used, remove it.
(This description has been changed, the thumbs up emojis were put on the previous PR description.)
ACKs for top commit:
sanket1729:
reACK 99aab446c3
apoelstra:
ACK 99aab446c3
Tree-SHA512: 2342531160966860b7b65f8c5df10e169876ec446e6fd30093d5d81d0b0304cad04e2c2057eb3ca6b23a2fc56453c91ad4ddf426d3796fb301acb7f7d03a66b9
43b684bbe6 Add non_exhaustive compiler directive to AddressType (Tobin C. Harding)
Pull request description:
Add non_exhaustive compiler directive to AddressType
Currently adding variants to enums is a breaking change. In an effort to
reduce the upgrade burden on users we can use the `non_exhaustive`
compiler directive so that adding a new variant does not cause
downstream code to break.
Add `non_exhaustive` to the `AddressType` since it may be extended in
the future.
ACKs for top commit:
sanket1729:
ACK 43b684bbe6
Kixunil:
ACK 43b684bbe6
apoelstra:
ACK 43b684bbe6
Tree-SHA512: 2b2a15fb501d23058acca94318776ffcccedf463d43d07afa290fba46a7bd58b3a730f6e1f25605ef399afcfdb5de4c7ad67eaa0adff0ba39b0096cbcec10f57
Currently adding variants to enums is a breaking change. In an effort to
reduce the upgrade burden on users we can use the `non_exhaustive`
compiler directive so that adding a new variant does not cause
downstream code to break.
Add `non_exhaustive` to the `AddressType` since it may be extended in
the future.
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
1875c912c3 Extend docstring for more types (Dawid Ciężarkiewicz)
325ea8fb7d Add "Relevant BIPs` to `Address` (Dawid Ciężarkiewicz)
7c2ca3d20b Add `BlockHeader` Bitcoin Core reference link (Dawid Ciężarkiewicz)
f4922f6fe7 Update `BlockHeader::version` documentation (Dawid Ciężarkiewicz)
Pull request description:
This is meant to make it more educational, and handy even for experienced developers.
A first step to make https://docs.rs/bitcoin (or `cargo doc --open`) a go-to place for
convenient Bitcoin documentation.
ACKs for top commit:
tcharding:
tACK 1875c912c3
apoelstra:
ACK 1875c912c3
sanket1729:
utACK 1875c912c3. Thanks for doing this.
Tree-SHA512: 8457e120f9979bfd95e55e8b18faf6131610aa2241f8e5fc4630fe61dc7e16ddfc35fb6eff46339804016db7b176465943cc0c02d84dcf478ed55da9f5e06fc5
2e7effc604 Feature `use-serde` renamed to `serde` (Martin Habovstiak)
Pull request description:
Features activating external crates are supposed to have same name as
those crates. However we depend on same feature in other crates so we
need a separate feature. After MSRV bump it is possible to rename the
crates and features so we can now fix this inconsistency.
Sadly, derive can't see that the crate was renamed so all derives must
be told to use the other one.
Replaces #373
ACKs for top commit:
apoelstra:
ACK 2e7effc604
Tree-SHA512: b20364b9e8f30c2269bef915e821b2b2ec929e71dd0e88af2bc3a021821f87011d35e095cb8efe99add77a23dde940a17537eb387fb4582b05c57c8679969eb0
8e29f2b493 Add ChainHash type (Tobin Harding)
cd8f511fcb blockdata: constants: Use wildcard import in unit tests (Tobin Harding)
71bf19621a Use fully qualified path in macro (Tobin Harding)
Pull request description:
The Lightning network defines a type called 'chain hash' that is used to uniquely represent the various Bitcoin networks as a 32 byte hash value. Chain hash is now being used by the DLC folks, as such it is useful to have it implemented in rust-bitcoin.
One method of calculating a chain hash is by hashing the genesis block for the respective network.
Add a `ChainHash` type that can be used to get the unique identifier of each of the 4 Bitcoin networks we support. Add a method that calculates the chain hash for a network using the double sha256 of the genesis block. Do so using hard coded consts and add unit tests (regression/sanity) that show these hard coded byte arrays match the hash of the data we return for the genesis block for the respective network.
The chain hash for the main Bitcoin network can be verified from LN docs (BOLT 0), add a link to this document.
Closes: #481
ACKs for top commit:
Kixunil:
ACK 8e29f2b493
sanket1729:
ACK 8e29f2b493.
Tree-SHA512: 8156bb55838b73694ddf77a606cbe403f53a31d363aa0dee11b97dc31aa9b62609d7d84b8f0f92c08e90372a3e8c7b416fb07989d6da9633763373b41339b1f5
6c10d77ecb Address::from_script() - Check witness v0 program lengths. (Noah)
Pull request description:
Adds a check in `Address::from_script()` that checks if segwit v0 scripts have a valid length.
Fix: #995
ACKs for top commit:
tcharding:
ACK 6c10d77ecb
sanket1729:
ACK 6c10d77ecb. Left a comment can be addressed in separate PR.
apoelstra:
ACK 6c10d77ecb
Tree-SHA512: 32aebb13477958b1455c688f668aaa3d3af4db0a7936b9549bcd1d03bd0e16635b8471549d96f1e8d408d6501e8fb515df2eb86b17a08c3152774a5be78ae8b1
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
A better way to write a byte string is to use write all so that
`ErrorKind::Interupted` is not returned.
Use `write_all` to write the non-sense (error indication) string to the
writer when we hit the SIGHASH_SINGLE bug.
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_`.
Rust convention is to use `to_` for conversion methods that convert from
an owned type to an owned `Copy` type. `into_` is for owned to owned
non-`Copy` types.
Re-name and deprecate conversion methods that use `into_` for `Copy`
types to use `to_`.
`self` and the referenced type returned by `as_inner` are both `Copy`
types. There is no need to provide an reference getter method to a
`Copy` type since implementing `Copy` implies that copying is cheap.
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.
Features activating external crates are supposed to have same name as
those crates. However we depend on same feature in other crates so we
need a separate feature. After MSRV bump it is possible to rename the
crates and features so we can now fix this inconsistency.
Sadly, derive can't see that the crate was renamed so all derives must
be told to use the other one.
Currently we allow multiple trailing colons when matching within the
`check_format_non_negative` macro. We can be more restrictive with no
loss of usability.
Use `$(;)?` instead of `$(;)*` to match against 0 or 1 semi-colons
instead of 0 or more.
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.
Clippy emits:
warning: you should consider adding a `Default` implementation for
`TaprootBuilder`
As suggested, implement `Default` or `TaprootBuilder`.
Clippy emits:
warning: you are using an explicit closure for copying elements
In one instance we have `map` followed by `flatten`, this can be
replaced by the `flat_map` combinator.
As suggested use `copied` combinator.
Clippy emits:
warning: unnecessary `if let` since only the `Some` variant of the
iterator element is used
Use combinator chain `iter().flatten().any()` to check for an node with
hidden nodes.
Clippy emits:
error: written amount is not handled
This code is explicitly writing garbage to the writer, no need to handle
the number of bytes written.
Clippy emits:
warning: this expression creates a reference which is immediately
dereferenced by the compiler
As suggested, remove the additional reference.
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
97a5bb1439 Implement std::error::source codebase wide (Tobin C. Harding)
0a9191b429 Add parenthesis around left hand side of companion (Tobin C. Harding)
7cf8af2f86 Put Error impl block below Display (Tobin C. Harding)
2384712364 Re-order Display match arms (Tobin C. Harding)
Pull request description:
Now that we have MSRV of 1.41.1 we should use `source` instead of `cause`. Audit the whole codebase and implement `source` for _every_ error type we have.
The first three patches are preparatory cleanup, patch 3 is particularly shameful (adds parenthesis to make my editor work).
CC @Kixunil because he is championing the error stuff.
ACKs for top commit:
apoelstra:
ACK 97a5bb1439
Tree-SHA512: 46313a28929445f32e01e30ca3b0246b30bc9d5e43db5754d4b441e9c30d3e427efaf247100eb6b452f98beec5a4fcde1daba7943a772114aa34f78ab52cbc60
9896f27eae psbt: Improve documentation (Tobin C. Harding)
33a50831ce sighash: Improve documentation (Tobin Harding)
Pull request description:
Done while working on sighash and PSBT signing. Just the usual docs fixes. Note, does not do the whole `psbt` module just the file mentioned.
ACKs for top commit:
apoelstra:
ACK 9896f27eae
Tree-SHA512: 5fbfa258cdb216189922a49a42b7ab9fb78faeee72d82f8cb99a1b3d930d170074013e317b0e7af259a404ac4db93841b4d2b525e933c5e145da71e7522800fd
58f94bee9b Remove sha256t_hash_newtype macro (Tobin C. Harding)
Pull request description:
Since commit `commit 275adc6c335a4326699cfbd444949e1725864ea1` on `bitcoin_hashes` we have the identical implementation of the macro `sha256t1_hash_newtype` in this crate and in `bitcoin_hashes`.
Remove the `sha256t_hash_newtype` macro from this crate in favour of the one in `bitcoin_hashes`.
ACKs for top commit:
apoelstra:
ACK 58f94bee9b
sanket1729:
ACK 58f94bee9b
Tree-SHA512: ec08fd25c1cca71a07ea61cb5838ce8962daae7cbb84d8beccc3d0d285439909721edd643292a8f3f6989e1c2c41fda9addfd5cdb063ef53ebc6ef646da79cf3
90b4f1cde8 Clear TapTreeIter clippy warning (Tobin C. Harding)
e6084a1af8 Improve documentation around EcdsaSig (Tobin Harding)
Pull request description:
Do a couple of trivial docs fixes, done during other work.
- Patch 1 improves docs on the `EcdsaSig` struct
- Patch 2 clears a clippy warning during docs build - no sure if the solution is the best available though
ACKs for top commit:
apoelstra:
re-ACK 90b4f1cde8
sanket1729:
ACK 90b4f1cde8
Tree-SHA512: 0647dc2e6550938ccca658a9dddffba7175d5c4eb8cec0e165d3a7fa8f2b1dfb902e795aca77d96a6c31092baf64244fa1d7151a304134d3b1895619a2823338
7ca30b6aa8 Move Address::payload_as_bytes to Payload::as_bytes (Fredrik Meringdal)
525ea00e0f Make Address::get_payload_bytes public (Fredrik Meringdal)
Pull request description:
Hi, thanks for the amazing work on this crate.
I am trying to upgrade from v0.27 to v0.28, but unable to do so because the `Address::get_payload_bytes` was made private. My use-case is that I have a script hash address and an `Address` and need to compare the two, and in order to do so I need access to the payload bytes of `Address`.
I hope you will consider making this function public again 🙏
ACKs for top commit:
apoelstra:
ACK 7ca30b6
tcharding:
ACK 7ca30b6aa8
sanket1729:
ACK 7ca30b6aa8. Sorry for the delay and congratz on your first time contribution
Tree-SHA512: 02af4565853d93506751ed7cb004f52cb5d8c7936067e06b3e237b448ccdf5716470448eeccbe211958e095b66bb37c7027800c0470c6988dc18d8bd5b48f459
Parenthesis are not needed around this expression but my editor is going
mad and cannot format the code without them. Since it does not hurt
readability add parenthesis around the expression.
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.
The Lightning network defines a type called 'chain hash' that is used to
uniquely represent the various Bitcoin networks as a 32 byte hash value.
Chain hash is now being used by the DLC folks, as such it is useful to
have it implemented in rust-bitcoin.
One method of calculating a chain hash is by hashing the genesis block
for the respective network.
Add a `ChainHash` type that can be used to get the unique identifier of
each of the 4 Bitcoin networks we support. Add a method that returns
the chain hash for a network using the double sha256 of the genesis
block. Do so using hard coded consts and add unit
tests (regression/sanity) that show these hard code byte arrays match
the hash of the data we return for the genesis block for the respective
network.
The chain hash for the main Bitcoin network can be verified from LN
docs (BOLT 0), add a link to this document.
Andrew Poelstra
Tobin C. Harding
Dr Maxim Orlovsky
Arturo Marquez
junderw
DanGould
Dawid Ciężarkiewicz
Matt Corallo
Riccardo Casatta
eunoia_1729
Steven Roose
sanket1729
Noah
Martin Habovstiak