082e185711 Add `consensus_decode_from_finite_reader` optimization (Dawid Ciężarkiewicz)
Pull request description:
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
ACKs for top commit:
apoelstra:
ACK 082e185711
tcharding:
ACK 082e185711
Tree-SHA512: fa04b62a4799c9a11c5f85ec78a18fa9c2cd4819c83a0d6148fbb203c6fa15c2689cb0847e612b35b8c285a756d81690b31a9bede4486b845f0c16b9fcc6d097
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.
88ce8fe923 Match against an optional single trailing colon (Tobin C. Harding)
Pull request description:
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.
Done as part of the [edition 2018 checklist](https://github.com/rust-bitcoin/rust-bitcoin/issues/510).
ACKs for top commit:
Kixunil:
ACK 88ce8fe923
apoelstra:
ACK 88ce8fe923
Tree-SHA512: 4409c094f6a0aa49ddebdad850fd1d5a31a57dae8828f5a1db0ee5a855e1bce9e43aea69fa0b4d132068c3a43f1f62d35409b9ac5b32ed876e4dd586829e8e68
553a6813c5 Do not pin transitive ryu dependency (Tobin C. Harding)
Pull request description:
We do not need to pin the `ryu` transitive dependency now that MSRV is not 1.29.
ACKs for top commit:
apoelstra:
ACK 553a6813c5
Kixunil:
ACK 553a6813c5
Tree-SHA512: 072a2fea39a0405424579e0e34603f27f12a5271a8979d6f9204b3114827b2c1931105df418ccb5071b641a108b7db803eec953ced04a670509d21652c6a6ca4
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.
Andrew Poelstra
Noah
Tobin C. Harding
Dawid Ciężarkiewicz
Martin Habovstiak
sanket1729
Riccardo Casatta