New clippy lint in rustc nightly "lifetime flowing from input to output
with different syntax can be confusing".
Apply the suggested fix and use the anonymous lifetime for paths.
We have a single test case that tests for the m prefix while all the
others do not.
Move one test case into the loop and then test on each iteration the
path with m prefix added.
c11772a768 Accept flexible input types for Taproot-related functions (Erick Cestari)
2a518d62e6 Wrap secp256k1::XOnlyPublicKey to improve error handling (Erick Cestari)
Pull request description:
This PR addresses issue #4361 by creating a wrapper type for XOnlyPublicKey instead of directly re-exporting it from the secp256k1 library.
### Key Changes
1. Created a new `XOnlyPublicKey` struct that wraps `secp256k1::XOnlyPublicKey`
2. Implemented custom error types:
- `ParseXOnlyPublicKeyError` for handling parsing errors
- `TweakXOnlyPublicKeyError` for tweaking an `XOnlyPublicKey`
3. Updated all imports and usage throughout the codebase
4. Implemented necessary traits and methods for compatibility
Closes#4361
ACKs for top commit:
apoelstra:
ACK c11772a768eefd89dcc0e3b1a369d535c191f94a; successfully ran local tests
tcharding:
ACK c11772a768
Tree-SHA512: c8da3486e7ffcab6c24cc08f9b2f964dd9158449ef2bd720e54d56176bc7027052314ea23cac3f673d217fa785238ea8a9b5323ba57f02199f20e56df5893965
Refactor Taproot functions to accept any type implementing `Into<XOnlyPublicKey>`,
instead of requiring `XOnlyPublicKey` directly. This improves ergonomics when working
with compatible types, avoiding unnecessary `.into()` conversions at call sites.
b9a12043b0 bip32: return error when trying to derive a path too deep (Andrew Poelstra)
73781e047b bip32: rename Error to ParseError (Andrew Poelstra)
a66ad97fb6 bip32: split InvalidChildNumber and InvalidChildNumberFormat out of error (Andrew Poelstra)
a891fb9b74 bip32: remove unused error variants (Andrew Poelstra)
f0a237c001 bip32: split out DerivationError from the main error enum (Andrew Poelstra)
32d96f6c33 bip32: make Xpriv::new_master be infallible (Andrew Poelstra)
0e5e021b69 bip32: change several cryptographically unreachable paths to expects (Andrew Poelstra)
Pull request description:
This PR makes a first pass at splitting the `bip32::Error` type into multiple distinct types -- one for derivation (which can fail if you try to derive a hardened child of an xpub, or if you try to derive too many layers), one for parsing child numbers or derivation paths, and one for parsing xkeys. Along the way it cleans up a ton of weird things and typos, e.g. the psbt `GetKeyError` having an unused `Bip32` variant whose display text references "bip 23".
Because all the error types get renamed, every part of this PR is an API break, but only the last commit is a "real" API break which uses the new `DerivationError::MaximumDepthExceeded` error variant to return an error when trying to derive a path of length 256 or longer. This means that `Xpriv::derive_xpriv` again returns an error result.
I will make a simpler version of this last commit suitable for backporting to 0.32.x. (In 0.32.x `Xpriv::derive_priv` returns an error, so we can change it to error out on max-depth-exceeded without breaking the API. Sadly most users are likely to be unwrapping the error because in 0.32.x currently the error path is cryptographically unreachable...but at least this way the panic will be in their control rather than ours.)
Fixes https://github.com/rust-bitcoin/rust-bitcoin/issues/4308
ACKs for top commit:
tcharding:
ACK b9a12043b0
Tree-SHA512: 688826126ff24066c6de9de3caa73db68c516ba8893d64d9226a498774a2fb9db7d7fd797375c6b3f820588c178632e1e6e8561022dfa7042a560582bf1509b4
The bip32::Error enum is now exclusively used for errors related to
parsing and decoding. It is still a little messy (mainly: it contains a
base58 variant which is used when parsing a string but not when decoding
from bytes) but much cleaner than it was.
Currently in this module we have a distinction between an "index" which
is a number between [0, 2^31 - 1] which indexes into the set of normal
or hardened child numbers. We also have a child *number*, which within
the library is an opaque type, but can be freely converted into/out of a
u32 in the consensus encoding which uses the MSB as a normal/hardened
flag and the other bits to encode the index.
Probably we want to change ChildNumber::From<u32> to some sort of
from_consensus_u32 method, but that's out of scope for this PR. What is
*in scope*, though is fixing the error types.
In the existing code we have three problems:
* Our error type for a bad index is called InvalidChildNumber, rather
than InvalidIndex, and the error message reflects this, which is wrong
and confusing. (Some with InvalidChildNumberFormat.)
* The InvalidChildNumberFormat is always constructed from a ParseIntError
from stdlib, but we always throw that away rather than preserving it.
* These two error variants only appear when parsing child numbers, or
derivation paths which are lists of child numbers, but they are part
of the main error enum.
This is a breaking change to the API of the deprecated `derive_pub`
function, but it's in a way that's unlikely to break consumers (who are
probably just .expect'ing the result) so we will retain the deprecated
function.
We have the `hex_lit` dependency for converting a hex string literal
to an array.
Currently we have a `test_hex_unwrap` macro in the `hex v0.3.0` release
but not on either `master` or the upcoming `v1.0.0-alpha.0` release.
This is making PRs around releasing and depending on the release more
noisy than required.
Use `hex_lit::hex` where possible (often needing an additional call to
`to_vec()`) and where not possible use `Vec::from_hex`.
These are defined in the BIP as invalid. The previous commit fixed a bug
where invalid key was parsed as valid and this bug can be caught by
these vectors. Therefore, if this commit is ordered before the last one
the test will fail.
Previously we've used `try_into().expect()` because const generics were
unavailable. Then they became available but we didn't realize we could
already convert a bunch of code to not use panicking conversions. But we
can (and could for a while).
This adds an extension trait for arrays to provide basic non-panicking
operations returning arrays, so they can be composed with other
functions accepting arrays without any conversions. It also refactors a
bunch of code to use the non-panicking constructs but it's certainly not
all of it. That could be done later. This just aims at removing the
ugliest offenders and demonstrate the usefulness of this approach.
Aside from this, to avoid a bunch of duplicated work, this refactors
BIP32 key parsing to use a common method where xpub and xpriv are
encoded the same. Not doing this already led to a mistake where xpriv
implemented some additional checks that were missing in xpub. Thus this
change also indirectly fixes that bug.
This commit adds additional validation checks when decoding extended private keys:
1. Verifies that byte 45 is zero as required by BIP-32 specification
2. For master keys (depth=0), ensures parent fingerprint is zero
3. For master keys (depth=0), ensures child number is zero
These checks improve security by rejecting malformed keys that could
potentially lead to unexpected behavior. Added corresponding error types
and unit tests to verify each validation rule.
We would like to do away with the `GeneralHash` trait. Currently we
bound `Hmac` and `HmacEngine` on it but this is unnecessary now that we
have added `HashEngine::finalize` and `HashEngine::Hash`.
Bound the `HmacEngine` on `HashEngine` (which has an associated `Hash`
type returned by `finilalize`).
Bound `Hmac` type on `T::Hash` where `T` is `HashEngine`.
Includes some minor shortening of local variable names around hmac
engine usage.
Note this means that `Hmac` no longer implements `GeneralHash`.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in the bitcoin, units, and internals crates
and just write the code.
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
These lints are valuable, lets get at em.
Changes are API breaking but because the changes make functions consume
self for types that are `Copy` downstream should not notice the breaks.
For the `hashes` crate we would like to make `hex` an optional
dependency. In preparation for doing so do the following:
- Remove the trait bounds from `GeneralHash`
- Split the hex/string stuff out of `impl_bytelike_traits` into a
separate macro.
The `impl_bytelike_traits` macro is public and it is used in the
`hash_newtype` macro, also public.
Currently if a user calls the `hash_newtype` macro in a crate that
depends on `hashes` without the `serde` feature enabled and with no
`serde` dependency everything works. However if the user then adds a
dependency that happens to enable the `serde` feature in `hashes` their
build will blow up because `serde` code will start getting called from
the original crate's call to `hash_newtype`.
Pull the serde stuff out of `hash_newtype` and provide a macro to
implement it `impl_serde_for_newtype`.
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
The current feature gating is wrong, this bug is unreleased because it
was introduced #2585.
The `impl_array_newtype` macro is only used in the `bitcoin` crate, it
does not need to be in `internals`. Also, other crates have an `alloc`
feature which `bitcoin` does not have so if we ever need it in other
places we'll need a duplicate with the correct feature gating anyways.
Move the macro to `bitcoin::internal_macros` and remove the incorrect
`alloc` feature gating.
323e706113 Add rustfmt config option style_edition (Tobin C. Harding)
2e4179ed0f Run the formatter (Tobin C. Harding)
2c40b4f4ec Configure formmater to skip read_compact_size (Tobin C. Harding)
Pull request description:
`rustfmt` is emitting:
Warning: the `version` option is deprecated. Use `style_edition` instead.
As suggested add a config option and set it to 2021.
- Patch 1: Manually configure rustfmt to skip some code
- Patch 2: Run the formmater with current configuration
- Patch 3: Add the new config option (remove old one), introduces no new formatting requirements
ACKs for top commit:
apoelstra:
ACK 323e706113 successfully ran local tests
Tree-SHA512: 7f80cc89f86d2d50936e51704344955fa00532424c29c0ee3fae1a6836e24030f909b770d28da13e1c5efde3d49ad7d52c6d909d120fb09c33abf1755f62cd38
If folk really want to index into a hash they can us `as_byte_array`
then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this
is because otherwise `secp` won't build since we are breaking an API
that is used in the current release of secp.
Fix: #3115
This change makes method names on Xpub and Xpriv more consistent and
easier to discover by following two patterns:
- if the method deals with extended key, it contains 'xpub' or
'xpriv' in its name
- if the method deals with non-extended key, it contains
'public_key' or 'private_key'
One exception is 'ckd_*' methods, which are lower-level and their names
come from BIP32; these keep using 'priv' and 'pub'.
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
unused_imports in docs have been removed in bitcoin, and a warn
attribute added to lib.rs.
7fa53440dc Move serde_round_trip macro to internals (Tobin C. Harding)
Pull request description:
We currently duplicate the serde_round_trip macro in `units` and `bitcoin`, this is unnecessary since it is a private test macro we can just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding also, elect to use the `bincode` crate because we already have it in our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and its usage (preventing the transient dependency on `bincode` and `serde_json`).
ACKs for top commit:
Kixunil:
ACK 7fa53440dc
apoelstra:
ACK 7fa53440dc
Tree-SHA512: f40c78bf2539940b7836ed413d5fe96ce4e9ce59bad7b3f86d831971320d1c2effcd23d0da5c785d6c372a2c6962bf720080ec4351248fbbdc0f2cfb4ffd602c
vicjuma
Jamil Lambert, PhD
Fmt Bot
Tobin C. Harding
merge-script
Erick Cestari
Martin Habovstiak
19年梦醒
Shing Him Ng
LLFourn
Jiri Jakes