10374af75c Make error types uniform (Tobin C. Harding)
43d3306822 Use explicit error::Error impl instead of the default (Tobin C. Harding)
2512dbafc2 Remove impl_std_error macro (Tobin C. Harding)
6933ca4fc2 Add suffix to HiddenNodes error type (Tobin C. Harding)
2b40ea24fb Add suffix to IncompleteBuilder error type (Tobin C. Harding)
f41416a0ea Add suffix to UnknownMagic error type (Tobin C. Harding)
5658dac024 Add suffix to UnknownChainHash error type (Tobin C. Harding)
2fb71dd943 Move p2p error types to bottom of file (Tobin C. Harding)
39314ad52f Move error code to match conventional layout (Tobin C. Harding)
Pull request description:
PR aims to achieve two things:
- Make error code brain dead easy to read
- Get error code closer to being ready for v1.0
The first 8 patches are pretty basic, and are broken up into really small changes. The last patch is much bigger, it has a long git log to explain it but reviewing should not take too much brain power.
This PR does not introduce anything new, it just applies what we have been doing recently with errors. Before v1.0.0 others will likely want to re go over all the error types. As such I believe this PR can be merged under the one ack carve-out.
### TODOs (future PRs)
We have a few errors that still need splitting up:
- Split up `merkle_tree::block::MerkleBlockError`
- Split up `psbt::error::Error`
- Split up `IncompleteBuilderError`
Also, all error From's should probably have `#[inline]`, I noticed late in the process and did not have the heart to visit every error again.
ACKs for top commit:
apoelstra:
ACK 10374af75c
clarkmoody:
ACK 10374af75c
Tree-SHA512: 4f4f3533f42dc11af8e7978f3272752bb56d12a68199752ed4af0c02a46a87892b55c695b7007bc3d0bdf389493068d068e2be1780e8c3008815efec3a02eedf
On our way to v1.0.0 we are defining a standard for our error types,
this includes:
- Uses the following derives (unless not possible, usually because of `io::Error`)
`#[derive(Debug, Clone, PartialEq, Eq)]`
- Has `non_exhaustive` unless we really know we can commit to not adding
anything.
Furthermore, we are trying to make the codebase easy to read. Error code
is write-once-read-many (well it should be) so if we make all the error
code super uniform the users can flick to an error and quickly see what
it includes. In an effort to achieve this I have made up a style and
over recent times have change much of the error code to that new style,
this PR audits _all_ error types in the code base and enforces the
style, specifically:
- Is layed out: definition, [impl block], Display impl, error::Error impl, From impls
- `error::Error` impl matches on enum even if it returns `None` for all variants
- Display/Error impls import enum variants locally
- match uses *self and `ref e`
- error::Error variants that return `Some` come first, `None` after
Re: non_exhaustive
To make dev and review easier I have added `non_exhaustive` to _every_
error type. We can then remove it error by error as we see fit. This is
because it takes a bit of thinking to do and review where as this patch
should not take much brain power to review.
In a further effort to make the code brain-dead easy to read; use an
explicit implementation of `std::error::Error` that returns `None`
instead of relying on the default trait implementation.
As part of an ongoing effort to make our error types stable and useful
add a stand set of derives to all error types in the library.
`#[derive(Debug, Clone, PartialEq, Eq)]`
Add `Copy` if possible and the error type does not include
`#[non_exhaustive]`.
If an error type includes `io::Error` it only gets `#[derive(Debug)]`.
We have just released the `hex-conservative` crate, we can now use it.
Do the following:
- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Fix all the import statements (makes up the bulk of the lines changed
in this patch)
7cdc90565f Mutate mul_u64 with mutagen (Tobin C. Harding)
Pull request description:
Add the `mutate` attribute to mutate `mul_u64`. Add non-doc comments listing the two false positives. These are identical but we list them twice so when devs grep for `mutagen false pos` the same number of lines for each function is displayed as is displayed by the `mutagen` run. This coding false positives thing is also introduced in PR #1655.
ACKs for top commit:
apoelstra:
ACK 7cdc90565f
sanket1729:
utACK 7cdc90565f
Tree-SHA512: d066beb2f9ba198f5af36258ba15cfbd2c7c9ce7596f6340ed1fe2f62a2b0b5296eeb2cb4be30146d019671f1858521c29db917936895b5b3fd36bdb0bd07e57
8e6f953aa7 Expose valid (min, max) difficulty transition thresholds (Wilmer Paulino)
Pull request description:
Once `U256` was made private, we lost the ability to check whether a valid difficulty transition was made in the chain, since `Target` doesn't expose any operations. We only choose to expose `Shl<u32>` and `Shr<u32>` such that we can compute the min and max target thresholds allowed for a difficulty transition.
This is something we realized was missing after bumping to `rust-bitcoin v0.30.0` in `rust-lightning`, specifically for our `lightning-block-sync` crate. It may also be worth having a helper in `rust-bitcoin` that checks a header properly builds upon the previous, but that can be left for future work.
ACKs for top commit:
Kixunil:
ACK 8e6f953aa7
sanket1729:
ACK 8e6f953aa7 . Sorry, was confused by some details.
apoelstra:
ACK 8e6f953aa7
Tree-SHA512: 740dd64089426463dc6a19726d5a562276bd0966e0e31af8e1e67b28d18945644ac0e50be3cf0cc7fa604acc3d2c5b912a77a7caa69d8cff85f70fd57e5328c5
Once `U256` was made private, we lost the ability to check whether a
valid difficulty transition was made in the chain, since `Target`
no longer exposes any arithmetic operations.
Our previous MSRV did not support MIN/MAX associated consts so we had
methods min/max_value. Now that our MSRV is Rust 1.48.0 we can use the
consts.
Deprecate min/max_value methods in favor of MIN/MAX associated conts.
We currently use the functions `min_value` and `max_value` because the
consts were not available in Rust 1.41.1, however we recently bumped the
MSRV so we can use the consts now.
Currently we have a mishmash of attribution lines accompanying the SPDX
identifier. These lines are basically meaningless because:
- The date is often wrong
- The original author attributed is not the only contributor to a file
- The term "rust bitcoin developers" is basically just noise
Just remove all the attribution lines and be done with it. While we are
at it add an SPDX line to the few files missing it, whether this license
nonsense is even needed is left as an argument for another day.
fabcde036f Use package in manifest and shorten import (Tobin C. Harding)
Pull request description:
We can use `package` to rename `bitcoin_hashes` to `hashes` and `bitcoin_internals` to `internals`. This makes imports more terse with no loss of meaning.
ACKs for top commit:
apoelstra:
ACK fabcde036f
Kixunil:
ACK fabcde036f
Tree-SHA512: bc5bff6f7f6bf3b68ba1e0644a83da014081d8c6c9d578c21cb54fdd56a018f68733dd1135d05b590ba193ed9efd12fa9019182c1fed347e604d8548f6ef9103
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
We can use `package` to rename `bitcoin_hashes` to `hashes` and
`bitcoin_internals` to `internals`. This makes imports more terse with
no loss of meaning.
Add the `mutate` attribute to mutate `mul_u64`. Add non-doc comments
listing the two false positives. These are identical but we list them
twice so when devs grep for `mutagen false pos` the same number of lines
for each function is displayed as is displayed by the `mutagen` run.
This coding false positives thing is also introduced in PR #1655.
This adds a conversion function to U256 to get an f64. We use the method shown in the following blog post. https://blog.m-ou.se/floats/
Target::MAX was converted to a f64 and set as a const that is verified in a unit test.
Various formatting issues have crept into the codebase because we do not
run the formatter in CI.
In preparation for enabling formatting checks in CI run `cargo +nightly
fmt` to fix current formatting issues. No changes other than those
create by the formatter.
Currently we have an associated type on hash types `Inner` with
accompanying methods `into_inner`, `from_inner`, `as_inner`. Also, we
provide a way to create new wrapped hash types. The use of 'inner'
becomes ambiguous with the addition of wrapped types because the inner
could be the inner hash type or the `Inner` byte array of the inner
wrapped hash type.
In an effort to make the API more clear and uniform do the following:
- Rename `Inner` -> `Bytes`
- Rename `*_inner` -> `*_byte_array`
- Rename the inner hash to/from methods to `*_raw_hash`
Correct method prefix `into_` -> `to_` because theses methods convert
owned `Copy` types.
Add the trait Bound `Copy` to the `Bytes` type because we rely on this
trait bound for the conversion methods to be correctly named according
to convention.
Because of the dependency hole created by `secp256k1` this patch changes
the secp dependency to a git tag dependency that includes changes to the
hashes calls required so that we can get green lights on CI in this
repo.
When we added `Target` and `Work` types we implemented multiplication
and division by anything `Into<u64>`, this is not typically done in the
Rust stdlib and also is semantically incorrect for the types.
Remove `Mul` and `Div` impls from `Target` and `Work`. Also remove
`Mul<T>` for `T: Into<u64>` from the private `U256` type.
The `max_value` and `min_value` functions only exist to be
compatible/uniform with Rust 1.41.1 they will never change and they just
return a constant value. They can therefore be made const functions.
Recently we introduced some mutation testing to the `pow` module but
testing is never done - add more `mutate` attributes and add unit tests
to ensure all mutants are killed.
Of note, the `from_compact` and `to_compact_lossy` functions are not
done, doing so results in a bunch of surviving mutants.
The `ToHex` trait was replaced by either simple `Display`/`LowerHex`
where appropriate or `DisplayHex` from `bitcoin_internals` which is
faster.
This change replaces the usages and removes the trait.
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in
the current implementation due to overflow.
Re-write the `mul_u64` method.
Props to Elichai for the algorithm.
Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>
Introduce mutation testing by way of mutagen [0]
- Conditionally add the dev-dependency `mutagen` (using `do_mutate`
flag)
This flag is not very well named but `mutagen` and `mutate` are already
taken?
- Mutate all methods of the `U256` struct that do not require additional
unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need
to check on `test` as well i.e., I don't understand why we cannot use
`cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To
run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen`.
[0] https://github.com/llogiq/mutagen
Adds new module `string` to be later converted to its own
crate. The module currently contains the FromHexStr trait and an error
type to be used for implementing hex parsing on types. This change
also adds implementations of FromHexStr for types with a single u32
member such as `Sequence(pub u32)`. All structs that match the
following regex have been given this implementation
`\(u32\)` and `\(pub u32\)`. All implementations have associated
unit tests matching all possible cases. NonStandardSighashType has
been ommitted from this change as it is an error and should not be
constructed using the methods added in this change.
Adds parse::hex_u32 for future use to be made generic to allow
different sizes of integers to be parsed from hex strings.
The error type FromHexError implements required traits such as
Display and std::error::Error
Currently the types in the block module have longer names than
necessary, "header" and "version" identifiers contain the word "block",
this is unnecessary because we can write `block::Header` instead of
`BlockHeader` when context is required. This allows us to use the naked
type `Header` inside the `block` module with no loss of clarity.
We are stuck with `BlockHash` because the type is defined along with all
the other hash types in `hash_types`, leave it as is for now but
re-export it from the `block` module to assist in putting types that are
used together in scope in the same place, making import statements more
ergonomic.
A zero default value for `Target` and `Work` has no significance and/or
usecase, remove the derived `Default` implementation.
Includes making `Target::ZERO` public.
Currently we use the `Uint256` type to represent two proof of work
integers, namely target and difficulty (work).
It would be nice to not have a public integer type that is not fully
implemented (i.e., does not implement arithmetic etc as do integer types
in stdlib). Instead of implementing all the stdlib functions we can
instead add two new wrapper types, since these are not general purpose
integers they do not need to implement anything we do not need to use.
- Add a `pow` module.
- Put a modified version of `Uint256` to `pow`.
- Add two new wrapper types `Target` and `Difficulty`.
- Only implement methods that we use on each type.
Note this patch does not remove the original `Uint256`, that will be
done as a separate patch.
Andrew Poelstra
Tobin C. Harding
sanket1729
Wilmer Paulino
junderw
Martin Habovstiak
connormullett
Jiri Jakes